Re: [Xen-devel] [PATCH v2 1/2] VT-d: re-phrase logic in vtd_set_hwdom_mapping() for clarity

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 14 June 2018 16:44
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> 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.durrant@xxxxxxxxxx> 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);

> 
> >      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).

  Paul

> 
> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel