Re: [Xen-devel] [PATCH v12 3/3] iommu: add rmrr Xen command line option for extra rmrrs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.11.15 at 05:22, <elena.ufimtseva@xxxxxxxxxx> wrote:
> On Wed, Oct 28, 2015 at 10:05:31AM -0600, Jan Beulich wrote:
>> >>> On 27.10.15 at 21:36, <elena.ufimtseva@xxxxxxxxxx> wrote:
>> > +static void __init add_extra_rmrr(void)
>> > +{
>> > +    struct acpi_rmrr_unit *acpi_rmrr;
>> > +    struct acpi_rmrr_unit *rmrru;
>> > +    unsigned int dev, seg, i;
>> > +    unsigned long pfn;
>> > +    bool_t overlap;
>> > +
>> > +    for ( i = 0; i < nr_rmrr; i++ )
>> > +    {
>> > +        if ( extra_rmrr_units[i].base_pfn > extra_rmrr_units[i].end_pfn )
>> > +        {
>> > +            printk(XENLOG_ERR VTDPREFIX
>> > +                   "Invalid RMRR Range "ERMRRU_FMT"\n",
>> > +                   ERMRRU_ARG(extra_rmrr_units[i]));
>> > +            continue;
>> > +        }
>> > +
>> > +        if ( extra_rmrr_units[i].end_pfn - extra_rmrr_units[i].base_pfn >=
>> > +             MAX_EXTRA_RMRR_PAGES )
>> > +        {
>> > +            printk(XENLOG_ERR VTDPREFIX
>> > +                   "RMRR range "ERMRRU_FMT" exceeds 
>> > "__stringify(MAX_EXTRA_RMRR_PAGES)" pages\n",
>> > +                   ERMRRU_ARG(extra_rmrr_units[i]));
>> > +            continue;
>> > +        }
>> > +
>> > +        overlap = 0;
>> > +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
>> > +        {
>> > +            if ( pfn_to_paddr(extra_rmrr_units[i].base_pfn) < 
>> > rmrru->end_address &&
>> > +                 rmrru->base_address < 
>> > pfn_to_paddr(extra_rmrr_units[i].end_pfn + 1) )
>> 
>> Aren't both ranges inclusive? I.e. shouldn't the first one be <= (and
>> the second one could be <= too when dropping the +1), matching
>> the check acpi_parse_one_rmrr() does?
> 
> The end_address is not inclusive, while the start_address is.
> These to from  rmrr_identity_mapping()
>     ...
>     ASSERT(rmrr->base_address < rmrr->end_address);                           
>   

These are byte-granular addresses.

> and:
>     ...
>     while ( base_pfn < end_pfn )
>     {
>         int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
>                                                                              
>    
>         if ( err )                                                           
>    
>             return err;                                                      
>    
>         base_pfn++;                                                          
>    
>     }
>     ...
> 
> I think this condition should not be a problem. But yes, its not uniform 
> with acpi_parse_one_rmrr.

Did you actually pay attention to how end_pfn gets calculated?

> I guess I should send another version then?

Yes of course.

>> > +        }
>> > +        if ( seg != PCI_SEG(extra_rmrr_units[i].sbdf[0]) )
>> > +        {
>> > +            printk(XENLOG_ERR VTDPREFIX
>> > +                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
>> > +                   ERMRRU_ARG(extra_rmrr_units[i]));
>> > +            scope_devices_free(&acpi_rmrr->scope);
>> > +            xfree(acpi_rmrr);
>> > +            continue;
>> > +        }
>> > +
>> > +        acpi_rmrr->segment = seg;
>> > +        acpi_rmrr->base_address = 
> pfn_to_paddr(extra_rmrr_units[i].base_pfn);
>> > +        acpi_rmrr->end_address = pfn_to_paddr(extra_rmrr_units[i].end_pfn 
>> > + 
> 1);
>> 
>> And this seems wrong too, unless I'm mistaken with the inclusive-ness.
>>
> The end_address is exclusive, see above.

No - see above.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel