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

[Klaus Jensen]

On Nov  3 21:37, Philippe Mathieu-Daudé wrote:
> 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;
> >>            };
> >>        };
> >>

I align (hehe...) towards this. The amount of bit-juggling we need for
commands justify the need for separate NvmeCmd's, but in this case I
think an NvmeCqeZA is just unnecessary clutter. If the result value is
complex, the approach used by AERs is better I think:

   typedef struct QEMU_PACKED NvmeAerResult {
       uint8_t event_type;
       uint8_t event_info;
       uint8_t log_page;
       uint8_t resv;
   } NvmeAerResult;

   NvmeAerResult *result = (NvmeAerResult *)&req->cqe.result32;

Since storing the Zone Append ALBA in the result64 isn't really a
complex operation, let's just assign it into that member directly.

(Addendum) That DW1 is command specific and no longer reserved is
defined by TP 4056 (Namespace Types) - not v1.4 or any of its revisions.

signature.asc
Description: PGP signature