On 10/30/20 12:46 PM, Stefan Hajnoczi wrote: > On Thu, Oct 29, 2020 at 10:02:37AM +0100, Philippe Mathieu-Daudé wrote: >> On 10/28/20 7:24 PM, Philippe Mathieu-Daudé wrote: >>> On 10/28/20 4:16 PM, Stefan Hajnoczi wrote: >>>> On Tue, Oct 27, 2020 at 02:55:35PM +0100, Philippe Mathieu-Daudé wrote: >>>>> Rename Submission Queue flags with 'Sq' and introduce >>>>> Completion Queue flag definitions. >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>>>> --- >>>>> include/block/nvme.h | 17 +++++++++++------ >>>>> 1 file changed, 11 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/include/block/nvme.h b/include/block/nvme.h >>>>> index 65e68a82c89..079f884a2d3 100644 >>>>> --- a/include/block/nvme.h >>>>> +++ b/include/block/nvme.h >>>>> @@ -491,6 +491,11 @@ typedef struct QEMU_PACKED NvmeCreateCq { >>>>> #define NVME_CQ_FLAGS_PC(cq_flags) (cq_flags & 0x1) >>>>> #define NVME_CQ_FLAGS_IEN(cq_flags) ((cq_flags >> 1) & 0x1) >>>>> >>>>> +enum NvmeFlagsCq { >>>>> + NVME_CQ_PC = 1, >>>>> + NVME_CQ_IEN = 2, >>>>> +}; >>>>> + >>>>> typedef struct QEMU_PACKED NvmeCreateSq { >>>>> uint8_t opcode; >>>>> uint8_t flags; >>>>> @@ -508,12 +513,12 @@ typedef struct QEMU_PACKED NvmeCreateSq { >>>>> #define NVME_SQ_FLAGS_PC(sq_flags) (sq_flags & 0x1) >>>>> #define NVME_SQ_FLAGS_QPRIO(sq_flags) ((sq_flags >> 1) & 0x3) >>>>> >>>>> -enum NvmeQueueFlags { >>>>> - NVME_Q_PC = 1, >>>>> - NVME_Q_PRIO_URGENT = 0, >>>>> - NVME_Q_PRIO_HIGH = 1, >>>>> - NVME_Q_PRIO_NORMAL = 2, >>>>> - NVME_Q_PRIO_LOW = 3, >>>>> +enum NvmeFlagsSq { >>>>> + NVME_SQ_PC = 1, >>>>> + NVME_SQ_PRIO_URGENT = 0, >>>>> + NVME_SQ_PRIO_HIGH = 1, >>>>> + NVME_SQ_PRIO_NORMAL = 2, >>>>> + NVME_SQ_PRIO_LOW = 3, >>>>> }; >>>> >>>> There is also: >>>> >>>> #define NVME_SQ_FLAGS_PC(sq_flags) (sq_flags & 0x1) >>>> #define NVME_SQ_FLAGS_QPRIO(sq_flags) ((sq_flags >> 1) & 0x3) >>>> >>>> These macros should use the new constants. >> >> SQ_PC is bit#0, NVME_SQ_PC is "bit SQ_PC set (PC enabled)", >> SQ_PRIO are bits #1-2 (shift by 1, mask 2 bits), >> NVME_SQ_PRIO_xxx is the enum of these 2 bits. >> >> The NVME_SQ_FLAGS_X() macros extract the flags. >> >> So the macros can not use the new constants. > > I'm not sure I understand. Does this mean the header only defines the > flag values but not the bit shift constants?
Yes. > > It seems like hw/block/nvme.c and block/nvme.c are expressing flags in > slightly different approaches. Can they be unified instead of > introducing hw/block/nvme.c- and block/nvme.c-only constants in the > shared header file? I suggested the hw/block/nvme.c to unify the style. Klaus agreed (at least to have a look). Any change will be for 6.0 anyway.