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 <[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 ;)
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é <[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) { >>>>> >>>> >>> >> >
