> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 14 June 2018 17:03
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger
> Pau Monne <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; Kevin
> Tian <kevin.t...@intel.com>; Stefano Stabellini <sstabell...@kernel.org>;
> xen-devel <xen-devel@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 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).
>
Ok.
> >> > 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.
>
Ok, I suppose it does. I'll move it below the comment.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
