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

["Woods, Brian" <Brian.Woods@amd.com>]

On Fri, Oct 26, 2018 at 03:55:32PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 25 October 2018 11:29
> > To: Brian Woods <brian.woods@amd.com>; Paul Durrant
> > <Paul.Durrant@citrix.com>
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; xen-devel <xen-
> > devel@lists.xenproject.org>
> > 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@citrix.com> wrote:
> > >>  -----Original Message-----
> > >> From: Woods, Brian [mailto:Brian.Woods@amd.com]
> > >> Sent: 12 October 2018 18:14
> > >> To: Paul Durrant <Paul.Durrant@citrix.com>
> > >> Cc: xen-devel@lists.xenproject.org; Suthikulpanit, Suravee
> > >> <Suravee.Suthikulpanit@amd.com>; Woods, Brian <Brian.Woods@amd.com>
> > >> 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@citrix.com>
> > >>
> > >> 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
> 

I'd still rather shorter names rather than getting rid of them all
together but,

Acked-by: Brian Woods <brian.woods@amd.com>

-- 
Brian Woods

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