> -----Original Message----- > From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf > Of Paul Durrant > Sent: 11 June 2018 11:35 > To: Roger Pau Monne <roger....@citrix.com> > Cc: Kevin Tian <kevin.t...@intel.com>; Stefano Stabellini > <sstabell...@kernel.org>; Wei Liu <wei.l...@citrix.com>; Andrew Cooper > <andrew.coop...@citrix.com>; Tim (Xen.org) <t...@xen.org>; George > Dunlap <george.dun...@citrix.com>; Julien Grall <julien.gr...@arm.com>; > Jan Beulich <jbeul...@suse.com>; Ian Jackson <ian.jack...@citrix.com>; > xen-devel@lists.xenproject.org > Subject: Re: [Xen-devel] [PATCH 1/2] VT-d: re-phrase logic in > vtd_set_hwdom_mapping() for clarity > > > -----Original Message----- > > From: Roger Pau Monne > > Sent: 11 June 2018 11:31 > > To: Paul Durrant <paul.durr...@citrix.com> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.t...@intel.com>; > > Stefano Stabellini <sstabell...@kernel.org>; Wei Liu <wei.l...@citrix.com>; > > George Dunlap <george.dun...@citrix.com>; Andrew Cooper > > <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim > > (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich > > <jbeul...@suse.com> > > Subject: Re: [Xen-devel] [PATCH 1/2] VT-d: re-phrase logic in > > vtd_set_hwdom_mapping() for clarity > > > > On Fri, Jun 08, 2018 at 04:30:29PM +0100, Paul Durrant wrote: > > > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen- > > command-line.markdown > > > index 8712a833a2..6beb28dada 100644 > > > --- a/docs/misc/xen-command-line.markdown > > > +++ b/docs/misc/xen-command-line.markdown > > > @@ -1212,8 +1212,8 @@ wait descriptor timed out', try increasing this > > value. > > > > > > Use this to work around firmware issues providing incorrect RMRR > entries. > > > Rather than only mapping RAM pages for IOMMU accesses for Dom0, > with > > this > > > -option all pages not marked as unusable in the E820 table will get a > > mapping > > > -established. > > > +option all pages up to and including 4GB, not marked as unusable in the > > > +E820 table, will get a mapping established. > > > > Sorry, I've reviewed the patches in the wrong order. You can ignore > > the comments I've made related to this in patch 2. > > > > > ### irq\_ratelimit (x86) > > > > `= <integer>` > > > diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c > > b/xen/drivers/passthrough/vtd/x86/vtd.c > > > index 88a60b3307..5c440ba183 100644 > > > --- a/xen/drivers/passthrough/vtd/x86/vtd.c > > > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c > > > @@ -118,22 +118,26 @@ void __hwdom_init > > vtd_set_hwdom_mapping(struct domain *d) > > > > > > for ( i = 0; i < top; i++ ) > > > { > > > + unsigned long pfn = pdx_to_pfn(i); > > > + bool map; > > > int rc = 0; > > > > > > /* > > > - * Set up 1:1 mapping for dom0. Default to use only conventional > RAM > > > - * areas and let RMRRs include needed reserved regions. When set, > > the > > > - * inclusive mapping maps in everything below 4GB except unusable > > > - * ranges. > > > + * Set up 1:1 mapping for dom0. Default to include only > > > + * conventional RAM areas and let RMRRs include needed reserved > > > + * regions. When set, the inclusive mapping maps in every pfn up > > > + * to and including 4GB except those that fall in unusable > > > ranges. > > > */ > > > - unsigned long pfn = pdx_to_pfn(i); > > > + if ( iommu_inclusive_mapping && > > > + pfn <= (0xffffffffUL >> PAGE_SHIFT) ) > > > > Please use GB(4) here for clarity. > > That would be better. I left it since that's what the old code used but it > I'll > change this one and the one below.
Actually since you pointed this out that my reasoning before was wrong. Because the comparison was with 0xffffffff >> PAGE_SHIFT (i.e. GB(4) - 1), the inclusive map does not include GB(4) itself... so the documentation was correct. Paul > > > > > > + map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE); > > > + else > > > + map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL); > > > + > > > + if ( !map ) > > > + continue; > > > > > > - if ( pfn > (0xffffffffUL >> PAGE_SHIFT) ? > > > - (!mfn_valid(_mfn(pfn)) || > > > - !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL)) : > > > - iommu_inclusive_mapping ? > > > - page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) : > > > - !page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) ) > > > + if ( pfn > (0xffffffffUL >> PAGE_SHIFT) && !mfn_valid(_mfn(pfn)) > > > ) > > > > I would maybe do this check before the page_is_ram_type one, so that > > you can discard invalid mfns earlier. > > True. > > Paul > > > > > Thanks, Roger. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
