On 11/3/20 8:48 PM, Dmitry Fomichev wrote: >> -----Original Message----- >> From: Philippe Mathieu-Daudé <phi...@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