Re: [PATCH 13/25] block/nvme: Introduce Completion Queue definitions

[Stefan Hajnoczi]

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é <philmd@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?

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?

Stefan

signature.asc
Description: PGP signature