>>> On 14.06.18 at 17:55, <paul.durr...@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 14 June 2018 16:44 >> To: Paul Durrant <paul.durr...@citrix.com> >> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper >> <andrew.coop...@citrix.com>; Roger Pau Monne <roger....@citrix.com>; >> Wei Liu <wei.l...@citrix.com>; George Dunlap <george.dun...@citrix.com>; >> Ian Jackson <ian.jack...@citrix.com>; Kevin Tian <kevin.t...@intel.com>; >> Stefano Stabellini <sstabell...@kernel.org>; xen-devel <xen- >> de...@lists.xenproject.org>; Konrad Rzeszutek Wilk >> <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org> >> Subject: Re: [PATCH v2 1/2] VT-d: re-phrase logic in >> vtd_set_hwdom_mapping() for clarity >> >> >>> On 12.06.18 at 15:47, <paul.durr...@citrix.com> wrote: >> > --- a/xen/drivers/passthrough/vtd/x86/vtd.c >> > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c >> > @@ -114,26 +114,29 @@ void __hwdom_init >> vtd_set_hwdom_mapping(struct domain *d) >> > >> > BUG_ON(!is_hardware_domain(d)); >> > >> > - top = max(max_pdx, pfn_to_pdx(0xffffffffUL >> PAGE_SHIFT) + 1); >> > + top = max(max_pdx, pfn_to_pdx(GB(4) >> PAGE_SHIFT)); >> >> At certain boundaries >> >> pfn_to_pdx(x + 1) != pfn_to_pdx(x) + 1 >> >> i.e. there was a reason for the way this was phrased before. > > Oh, I'd misread the bracketing (and I actually didn't know that either). How > about: > > max_pfn = (GB(4) >> PAGE_SHIFT) - 1; > top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
Yes, that's fine (but I'd be fine also without the extra local variable). >> > for ( i = 0; i < top; i++ ) >> > { >> > + unsigned long pfn = pdx_to_pfn(i); >> > + bool map; >> > int rc = 0; >> > >> > + if ( pfn >= (GB(4) >> PAGE_SHIFT) && !mfn_valid(_mfn(pfn)) ) >> > + continue; >> >> Why ahead of the comment ... >> >> > /* >> > - * 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 4GB except those that fall in unusable ranges. >> > */ >> >> ... explaining it, when the other parts that get explained follow after it? > > Because the comment only refers to what happens below 4G, so the prior if > statement is not covered (and was not covered by the old version of the > comment either). "Default to include only conventional RAM areas" very much covers the if() above, according to my reading. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel