[prev in list] [next in list] [prev in thread] [next in thread]
List: xen-devel
Subject: Re: [Xen-devel] [PATCH v12 3/3] iommu: add rmrr Xen command line option for extra rmrrs
From: Elena Ufimtseva <elena.ufimtseva () oracle ! com>
Date: 2015-11-06 17:25:21
Message-ID: 20151106172521.GA12570 () elena ! ufimtseva
[Download RAW message or body]
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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic