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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> >      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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel