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 <[email protected]> > >>> > >>> 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é <[email protected]> > > > > >>> > >>> Signed-off-by: Klaus Jensen <[email protected]> > >>> --- > >>> 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) { > >>> > >> > > >
