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 17:03
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger
> Pau Monne <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@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 14.06.18 at 17:55, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----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);
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel