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

[Elena Ufimtseva <elena.ufimtseva@oracle.com>]

On Fri, Nov 06, 2015 at 04:05:25AM -0700, Jan Beulich wrote:
> >>> On 06.11.15 at 05:22, <elena.ufimtseva@oracle.com> wrote:
> > On Wed, Oct 28, 2015 at 10:05:31AM -0600, Jan Beulich wrote:
> >> >>> On 27.10.15 at 21:36, <elena.ufimtseva@oracle.com> 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.

Ok, I see your point.
> 
> >> > +        }
> >> > +        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.

You are right, I actually meant to say end_pfn for extra rmrr in not inclusive.
And this case is only valid when base_pfn == end_pfn as the parser does
not take care of the case where there is only one pfn specified. The
assumption in this case is that user meant [base_pfn, base_pfn + 1].
I think it will be safe to add the condition when incrementing.


> 
> Jan
>