| Message ID | 20201027135547.374946-14-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/nvme: Fix Aarch64 host | expand |
Hi, On 10/27/20 2:55 PM, Philippe Mathieu-Daudé wrote: > Rename Submission Queue flags with 'Sq' ... to differentiate submission queue flags from command queue flags. and introduce > Completion Queue flag definitions. besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > > 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, > }; > > typedef struct QEMU_PACKED NvmeIdentify { >
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, > }; Why have these constants at all if nothing uses them? I would rather remove dead code than spend time modifying it. Stefan
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. I didn't check if there are additional magic numbers in hw/block/nvme.c that should be converted. Stefan
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. > > I didn't check if there are additional magic numbers in hw/block/nvme.c > that should be converted. FYI we discussed with Klaus and might convert "block/nvme.h" to use the registerfields API during the 6.0 dev cycle.
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 didn't check if there are additional magic numbers in hw/block/nvme.c >> that should be converted. > > FYI we discussed with Klaus and might convert "block/nvme.h" to > use the registerfields API during the 6.0 dev cycle.
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
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é <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? 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.
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, }; typedef struct QEMU_PACKED NvmeIdentify {
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(-)