Re: [PATCH v8 07/11] hw/block/nvme: Support Zoned Namespace Command Set

[Philippe Mathieu-Daudé]

On 11/3/20 8:48 PM, Dmitry Fomichev wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
...
>>>  typedef struct QEMU_PACKED NvmeCqe {
>>> -    uint32_t    result;
>>> -    uint32_t    rsvd;
>>> +    union {
>>> +        uint64_t     result64;
>>> +        uint32_t     result32;
>>> +    };
>>
>> When using packed structure you want to define all fields to
>> avoid alignment confusion (and I'm surprised the compiler doesn't
>> complain...). So this would be:
>>
>>        union {
>>            uint64_t     result64;
>>            struct {
>>                uint32_t    result32;
>>                uint32_t    rsvd32;
>>            };
>>        };
>>
> 
> IMO, the compiler doesn't complain because it's a union. Smaller
> variants in unions are "padded" to the size of the largest variant
> regardless of whether the struct is packed or not.
> 
>> But since the ZNS is still a technical proposal and not in the spec,
>> this doesn't look correct (the spec list this field as 32-bit).
>>
>> What do you think about adding NvmeCqeZNS?
>>
>> Maybe:
>>
>>   typedef struct QEMU_PACKED NvmeCqeZNS {
>>       uint64_t    result;
>>       uint16_t    sq_head;
>>       uint16_t    sq_id;
>>       uint16_t    cid;
>>       uint16_t    status;
>>   } NvmeCqeZNS;
>>
>> Or clever:
>>
>>   typedef union QEMU_PACKED NvmeCqeZNS {
>>       union {
>>           struct {
>>               uint64_t result;
>>               uint32_t dw2;
>>               uint32_t dw3;
>>           };
>>           NvmeCqe      cqe;
>>       };
>>   } NvmeCqeZNS;
>>
> 
> The 1.4 base spec changes Reserved DW1 in CQE to become the
> Command Specific DW1, so it would rather make sense to define
> a command-specific CQE for Zone Append -
> 
> In include/block/nvme.h:
> 
> typedef struct QEMU_PACKED NvmeCqe {
>      uint32_t    result;
> -    uint32_t    rsvd;
> +    uint32_t    dw1;
>      uint16_t    sq_head;
>      uint16_t    sq_id;
>      uint16_t    cid;
>      uint16_t    status;
> } NvmeCqe;
> 
> +/* Zone Append - specific CQE */
> +typedef struct QEMU_PACKED NvmeCqeZA {
> +    uint64_t    za_slba;
> +    uint16_t    sq_head;
> +    uint16_t    sq_id;
> +    uint16_t    cid;
> +    uint16_t    status;
> +} NvmeCqeZA;
> 
> ...
> 
> +    QEMU_BUILD_BUG_ON(sizeof(NvmeCqe) != sizeof(NvmeCqeZA));
> 
> This will go away with all CQE unions and it will also allow the returned SLBA
> value to be properly named. What do you think?

This is cleaner, I like it :)

> 
>> I wonder what part could go in hw/block/nvme-ns.h or "block/nvme-zns.h".
> 
> NvmeCqeZA could simply be defined in include/block/nvme.h next to NvmeCqe.
> The problem with adding include/block/nvme-zns.h is that it would be hard if
> not impossible to separate all ZNS-specific content from block/nvme.h and it
> would become necessary for developers to deal with two files that present
> different parts of ZNS definitions instead of just one.

Got it.

Regards,

Phil.

> 
> Best regards,
> Dmitry