> -----Original Message----- > From: Jan Beulich [mailto:[email protected]] > Sent: 25 October 2018 11:29 > To: Brian Woods <[email protected]>; Paul Durrant > <[email protected]> > Cc: Suravee Suthikulpanit <[email protected]>; xen-devel <xen- > [email protected]> > Subject: Re: [Xen-devel] [PATCH] amd-iommu: get rid of pointless > IOMMU_PAGING_MODE_LEVEL_X definitions > > >>> On 12.10.18 at 19:18, <[email protected]> wrote: > >> -----Original Message----- > >> From: Woods, Brian [mailto:[email protected]] > >> Sent: 12 October 2018 18:14 > >> To: Paul Durrant <[email protected]> > >> Cc: [email protected]; Suthikulpanit, Suravee > >> <[email protected]>; Woods, Brian <[email protected]> > >> 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 <[email protected]> > >> > >> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
