[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

> -----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
> 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?


> --
> Brian Woods

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.