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

[Venu Busireddy <venu.busireddy@oracle.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, January 24, 2017 10:08 AM
> To: Venu Busireddy
> Cc: Feng Wu; Kevin Tian; xen-devel@lists.xen.org; Elena Ufimtseva; Konrad
> Rzeszutek Wilk
> Subject: Re: [PATCH v14 3/3] iommu: add rmrr Xen command line option for
> extra rmrrs
> 
> >>> On 24.01.17 at 16:58, <venu.busireddy@oracle.com> wrote:
> > On Tue, Jan 24, 2017 at 08:52:50AM -0700, Jan Beulich wrote:
> >> >>> On 24.01.17 at 16:35, <venu.busireddy@oracle.com> wrote:
> >> > On Tue, Jan 24, 2017 at 01:46:44AM -0700, Jan Beulich wrote:
> >> >> >>> On 23.01.17 at 19:20, <venu.busireddy@oracle.com> wrote:
> >> >> > +        overlap = false;
> >> >> > +        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
> >> >> > +        {
> >> >> > +            if ( pfn_to_paddr(base) <= rmrru->end_address &&
> >> >> > +                 rmrru->base_address <= pfn_to_paddr(end) )
> >> >>
> >> >> So this now looks correct as long as rmrru->base_address is
> >> >> page aligned (as required by the spec), which should be good
> >> >> enough for now (considering that we make this assumption
> >> >> elsewhere). Nevertheless it would have been nice if you had,
> >> >> following the subsequent discussion with Elena, accounted for
> >> >> spec violations here.
> >> >>
> >> >> > +        rmrr->segment = seg;
> >> >> > +        rmrr->base_address =
> pfn_to_paddr(user_rmrrs[i].base_pfn);
> >> >> > +        /* Align the end_address to the end of the page */
> >> >> > +        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) |
> >> > ~PAGE_MASK_4K;
> >> >>
> >> >> Hmm, Ive just checked - in my reply to Elena I had intentionally
> used
> >> >> PAGE_MASK here (and I recall correcting it from PAGE_MASK_4K).
> >> >> What has led you to use PAGE_MASK_4K here, when pfn_to_paddr()
> >> >> uses PAGE_SHIFT?
> >> >
> >> > Elena suggested to use PAGE_MASK_4K because the functions in
> >> > drivers/passthrough/vtd/iommu.c (including rmrr_identity_mapping())
> >> > use the _4K. With the current assumptions, both will work.
> >>
> >> Granted this is somewhat of a mess, but I'd prefer if at least within
> >> a single statement things would be consistent in which page size is
> >> being meant.
> >>
> >> > If you would like me to change this to PAGE_MASK, I will do so before
> >> > committing. Please let me know.
> >>
> >> As said, I don't see a need for you to re-submit, unless there are
> >> other issues in need of taking care of.
> >
> > Sure. I will change it to PAGE_MASK, but I will not resubmit the
> > patches. I will change it and commit it. Of course, assuming that there
> > are no other changes suggested by others.
> 
> I don't follow this repeated mentioning of "commit" by you. I don't
> think you can commit to our staging tree. And actually committing
> the series depends on - as also stated before - Kevin confirming
> his acks, which you had wrongly left in place.

Sorry Jan! I was confused. I thought that once I have the Ack's from the Maintainers, I can push the changes myself. Hence I was saying commit - meaning, commit to my local tree, with the intent to push them upstream once Ack'ed. I am told that I cannot push the changes.

Therefore, once we have Kevin's Ack, please change PAGE_MASK_4K to PAGE_MASK and commit.

Thanks,

Venu

> 
> > Do I understand that I have your Ack?
> 
> Well, I had given you my (conditional) R-b in the (my) morning.
> 
> Jan
> 

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