Message ID | 20200703063420.2241014-16-its@irrelevant.dk |
---|---|
State | New |
Headers | show |
Series | hw/block/nvme: bump to v1.3 | expand |
On 7/3/20 8:34 AM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the > Active Namespace ID list. Can we have a definition instead of this 0xfffffffe magic value please? > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > --- > hw/block/nvme.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 65c2fa3ac1f4..0dac7a41ddae 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) > > trace_pci_nvme_identify_nslist(min_nsid); > > + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { > + return NVME_INVALID_NSID | NVME_DNR; > + } > + > list = g_malloc0(data_len); > for (i = 0; i < n->num_namespaces; i++) { > if (i < min_nsid) { >
On Jul 3 10:20, Philippe Mathieu-Daudé wrote: > On 7/3/20 8:34 AM, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the > > Active Namespace ID list. > > Can we have a definition instead of this 0xfffffffe magic value please? > Hmm, not really actually. It's not a magic value, its just because the logic in Active Namespace ID list would require that it should report any namespaces with ids *higher* than the one specified, so since 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1 needs to be as well. What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`? The original condition just reads well if you are sitting with the spec on the side. > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > --- > > hw/block/nvme.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index 65c2fa3ac1f4..0dac7a41ddae 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) > > > > trace_pci_nvme_identify_nslist(min_nsid); > > > > + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { > > + return NVME_INVALID_NSID | NVME_DNR; > > + } > > + > > list = g_malloc0(data_len); > > for (i = 0; i < n->num_namespaces; i++) { > > if (i < min_nsid) { > > >
On 7/3/20 10:37 AM, Klaus Jensen wrote: > On Jul 3 10:20, Philippe Mathieu-Daudé wrote: >> On 7/3/20 8:34 AM, Klaus Jensen wrote: >>> From: Klaus Jensen <k.jensen@samsung.com> >>> >>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the >>> Active Namespace ID list. >> >> Can we have a definition instead of this 0xfffffffe magic value please? >> > > Hmm, not really actually. It's not a magic value, its just because the > logic in Active Namespace ID list would require that it should report > any namespaces with ids *higher* than the one specified, so since > 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1 > needs to be as well. OK. > > What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`? > The original condition just reads well if you are sitting with the spec > on the side. IMO this is clearer: if (min_nsid + 1 >= NVME_NSID_BROADCAST) { return NVME_INVALID_NSID | NVME_DNR; } Whichever form you prefer you can amend to the respin patch: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> >>> --- >>> hw/block/nvme.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c >>> index 65c2fa3ac1f4..0dac7a41ddae 100644 >>> --- a/hw/block/nvme.c >>> +++ b/hw/block/nvme.c >>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) >>> >>> trace_pci_nvme_identify_nslist(min_nsid); >>> >>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { >>> + return NVME_INVALID_NSID | NVME_DNR; >>> + } >>> + >>> list = g_malloc0(data_len); >>> for (i = 0; i < n->num_namespaces; i++) { >>> if (i < min_nsid) { >>> >> >
On Jul 3 11:14, Philippe Mathieu-Daudé wrote: > On 7/3/20 10:37 AM, Klaus Jensen wrote: > > On Jul 3 10:20, Philippe Mathieu-Daudé wrote: > >> On 7/3/20 8:34 AM, Klaus Jensen wrote: > >>> From: Klaus Jensen <k.jensen@samsung.com> > >>> > >>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the > >>> Active Namespace ID list. > >> > >> Can we have a definition instead of this 0xfffffffe magic value please? > >> > > > > Hmm, not really actually. It's not a magic value, its just because the > > logic in Active Namespace ID list would require that it should report > > any namespaces with ids *higher* than the one specified, so since > > 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1 > > needs to be as well. > > OK. > > > > > What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`? > > The original condition just reads well if you are sitting with the spec > > on the side. > > IMO this is clearer: > > if (min_nsid + 1 >= NVME_NSID_BROADCAST) { > return NVME_INVALID_NSID | NVME_DNR; > } > But since min_nsid is uint32_t that would not be wise ;) I'll go with the - 1 and add a comment! > Whichever form you prefer you can amend to the respin patch: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > >>> > >>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > >>> --- > >>> hw/block/nvme.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c > >>> index 65c2fa3ac1f4..0dac7a41ddae 100644 > >>> --- a/hw/block/nvme.c > >>> +++ b/hw/block/nvme.c > >>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) > >>> > >>> trace_pci_nvme_identify_nslist(min_nsid); > >>> > >>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { > >>> + return NVME_INVALID_NSID | NVME_DNR; > >>> + } > >>> + > >>> list = g_malloc0(data_len); > >>> for (i = 0; i < n->num_namespaces; i++) { > >>> if (i < min_nsid) { > >>> > >> > > >
On 7/3/20 11:22 AM, Klaus Jensen wrote: > On Jul 3 11:14, Philippe Mathieu-Daudé wrote: >> On 7/3/20 10:37 AM, Klaus Jensen wrote: >>> On Jul 3 10:20, Philippe Mathieu-Daudé wrote: >>>> On 7/3/20 8:34 AM, Klaus Jensen wrote: >>>>> From: Klaus Jensen <k.jensen@samsung.com> >>>>> >>>>> Reject the nsid broadcast value (0xffffffff) and 0xfffffffe in the >>>>> Active Namespace ID list. >>>> >>>> Can we have a definition instead of this 0xfffffffe magic value please? >>>> >>> >>> Hmm, not really actually. It's not a magic value, its just because the >>> logic in Active Namespace ID list would require that it should report >>> any namespaces with ids *higher* than the one specified, so since >>> 0xffffffff (NVME_NSID_BROADCAST) is invalid, NVME_NSID_BROADCAST - 1 >>> needs to be as well. >> >> OK. >> >>> >>> What do you say I change it to `min_nsid >= NVME_NSID_BROADCAST - 1`? >>> The original condition just reads well if you are sitting with the spec >>> on the side. >> >> IMO this is clearer: >> >> if (min_nsid + 1 >= NVME_NSID_BROADCAST) { >> return NVME_INVALID_NSID | NVME_DNR; >> } >> > > But since min_nsid is uint32_t that would not be wise ;) Hmm indeed. > > I'll go with the - 1 and add a comment! Good, thanks. > >> Whichever form you prefer you can amend to the respin patch: >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> >>> >>>>> >>>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> >>>>> --- >>>>> hw/block/nvme.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c >>>>> index 65c2fa3ac1f4..0dac7a41ddae 100644 >>>>> --- a/hw/block/nvme.c >>>>> +++ b/hw/block/nvme.c >>>>> @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) >>>>> >>>>> trace_pci_nvme_identify_nslist(min_nsid); >>>>> >>>>> + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { >>>>> + return NVME_INVALID_NSID | NVME_DNR; >>>>> + } >>>>> + >>>>> list = g_malloc0(data_len); >>>>> for (i = 0; i < n->num_namespaces; i++) { >>>>> if (i < min_nsid) { >>>>> >>>> >>> >> >
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 65c2fa3ac1f4..0dac7a41ddae 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -956,6 +956,10 @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c) trace_pci_nvme_identify_nslist(min_nsid); + if (min_nsid == 0xfffffffe || min_nsid == NVME_NSID_BROADCAST) { + return NVME_INVALID_NSID | NVME_DNR; + } + list = g_malloc0(data_len); for (i = 0; i < n->num_namespaces; i++) { if (i < min_nsid) {