lkml.org 
[lkml]   [2014]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH v3] xen/grant-table: Avoid m2p_override during mapping
On Tue, 21 Jan 2014, David Vrabel wrote:
> On 21/01/14 12:26, Stefano Stabellini wrote:
> > On Mon, 20 Jan 2014, Zoltan Kiss wrote:
> >
> >> - ret = m2p_add_override(mfn, pages[i], kmap_ops ?
> >> - &kmap_ops[i] : NULL);
> >> + if (m2p_override)
> >> + ret = m2p_add_override(mfn, pages[i], kmap_ops ?
> >> + &kmap_ops[i] : NULL);
> >> + else {
> >> + unsigned long pfn = page_to_pfn(pages[i]);
> >> + WARN_ON(PagePrivate(pages[i]));
> >> + SetPagePrivate(pages[i]);
> >> + set_page_private(pages[i], mfn);
> >> + pages[i]->index = pfn_to_mfn(pfn);
> >> + if (unlikely(!set_phys_to_machine(pfn, FOREIGN_FRAME(mfn))))
> >> + return -ENOMEM;
> >
> > What happens if the page is PageHighMem?
> >
> > This looks like a subset of m2p_add_override, but it is missing some
> > relevant bits, like the PageHighMem check, or the p2m(m2p(mfn)) == mfn
> > check. Maybe we can find a way to avoid duplicating the code.
> > We could split m2p_add_override in two functions or add yet another
> > parameter to it.
>
> The PageHighMem() check isn't relevant as we're not mapping anything
> here. Also, a page for a kernel grant mapping only cannot be highmem.
>
> The check for a local mfn and the additional set_phys_to_machine() is
> only necessary if something tries an mfn_to_pfn() on the local mfn. We
> can only omit adding an m2p override if we know thing will try
> mfn_to_pfn(), therefore the check and set_phys_to_machine() is unnecessary.

OK, you convinced me that the two checks are superfluous for this case.

Can we still avoid the code duplication by removing the corresponding
code from m2p_add_override and m2p_remove_override and doing the
set_page_private thing uniquely in grant-table.c?


\
 
 \ /
  Last update: 2014-01-21 16:01    [W:0.037 / U:2.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site