Re: [Xen-devel] [PATCH] amd-iommu: get rid of pointless IOMMU_PAGING_MODE_LEVEL_X definitions

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 25 October 2018 11:29
> To: Brian Woods <brian.woods@xxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] amd-iommu: get rid of pointless
> IOMMU_PAGING_MODE_LEVEL_X definitions
> 
> >>> On 12.10.18 at 19:18, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Woods, Brian [mailto:Brian.Woods@xxxxxxx]
> >> Sent: 12 October 2018 18:14
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Suthikulpanit, Suravee
> >> <Suravee.Suthikulpanit@xxxxxxx>; Woods, Brian <Brian.Woods@xxxxxxx>
> >> Subject: Re: [PATCH] amd-iommu: get rid of pointless
> >> IOMMU_PAGING_MODE_LEVEL_X definitions
> >>
> >> On Thu, Sep 27, 2018 at 01:46:01PM +0100, Paul Durrant wrote:
> >> > The levels are absolute numbers such that IOMMU_PAGING_MODE_LEVEL_X
> >> > evaluates to X (for the valid range of 0 - 7) so simply use numbers
> in
> >> > the code.
> >> >
> >> > No functional change.
> >> >
> >> > NOTE: This patch also adds emacs boilerplate to amd-iommu-defs.h
> >> >
> >> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>
> >> Is there a strong reason to get rid of these?  Some of examples below
> >> create seemingly magic numbers in the code.  While if you're familiar
> >> with the functions this isn't a big deal, otherwise you have to dig
> >> further to tell.
> >>
> >
> > The numbers aren't magic though. The spec refers to levels by number
> rather
> > than any sort of name. If the levels were named then it would be
> absolutely
> > right to #define <level name> <level number>, but that is not the case.
> Thus IMO
> > getting rid of the definitions actually makes the code clearer for those
> > (like myself) reading the spec.
> >
> >> > +    pte = table + pfn_to_pde_idx(gfn, 1);
> >>
> >> > +    need_flush = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> >>
> >> If there's a general consensus that getting rid of these is better, I
> >> don't mind and will agree to it.
> >>
> >
> > Anyone else care to comment?
> 
> I think that, quite the opposite of what is often the case, the amount
> of manifest constants the AMD IOMMU code uses is quite a bit too
> large. I therefore welcome this change, and I've been planning some
> other reduction there (but haven't got to it in a couple of years).
> 

Brian,

  Are you ok to ack this patch now, or would you like more opinions?

  Paul


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel