| Message ID | 8f7138e4d6a11975ef85458c000a337a60a4e13e.1580148227.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/9] x86/p2m: Allow p2m_get_page_from_gfn to return shared entries | expand |
On 27.01.2020 19:06, Tamas K Lengyel wrote: > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -574,11 +574,12 @@ struct page_info *p2m_get_page_from_gfn( > if ( fdom == NULL ) > page = NULL; > } > - else if ( !get_page(page, p2m->domain) && > - /* Page could be shared */ > - (!dom_cow || !p2m_is_shared(*t) || > - !get_page(page, dom_cow)) ) > - page = NULL; > + else > + { > + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; > + if ( !get_page(page, d) ) > + page = NULL; > + } > } > p2m_read_unlock(p2m); > > @@ -594,8 +595,9 @@ struct page_info *p2m_get_page_from_gfn( > mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL); > if ( p2m_is_ram(*t) && mfn_valid(mfn) ) > { > + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; > page = mfn_to_page(mfn); > - if ( !get_page(page, p2m->domain) ) > + if ( !get_page(page, d) ) > page = NULL; > } > put_gfn(p2m->domain, gfn_x(gfn)); Personally I would have preferred to go without new local variables here, but I'm not the maintainer of this code. However, at the very least blank lines would need inserting between declaration and statements. With at least this done (which could be done while committing) Reviewed-by: Jan Beulich <jbeulich@suse.com> As an aside, I don't think the title accurately describes the change, since there's just one out of two cases which shared entries weren't taken care of. Similarly the description doesn't reflect this at all. Jan
On Tue, Jan 28, 2020 at 9:38 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 27.01.2020 19:06, Tamas K Lengyel wrote: > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -574,11 +574,12 @@ struct page_info *p2m_get_page_from_gfn( > > if ( fdom == NULL ) > > page = NULL; > > } > > - else if ( !get_page(page, p2m->domain) && > > - /* Page could be shared */ > > - (!dom_cow || !p2m_is_shared(*t) || > > - !get_page(page, dom_cow)) ) > > - page = NULL; > > + else > > + { > > + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; > > + if ( !get_page(page, d) ) > > + page = NULL; > > + } > > } > > p2m_read_unlock(p2m); > > > > @@ -594,8 +595,9 @@ struct page_info *p2m_get_page_from_gfn( > > mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL); > > if ( p2m_is_ram(*t) && mfn_valid(mfn) ) > > { > > + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; > > page = mfn_to_page(mfn); > > - if ( !get_page(page, p2m->domain) ) > > + if ( !get_page(page, d) ) > > page = NULL; > > } > > put_gfn(p2m->domain, gfn_x(gfn)); > > Personally I would have preferred to go without new local variables > here, but I'm not the maintainer of this code. However, at the very > least blank lines would need inserting between declaration and > statements. With at least this done (which could be done while > committing) > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > As an aside, I don't think the title accurately describes the > change, since there's just one out of two cases which shared > entries weren't taken care of. Similarly the description doesn't > reflect this at all. Well, for our use-case it is broken right now. So just because it's not broken in another doesn't make the title/description incorrect. Tamas
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 49cc138362..f54360b43d 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -574,11 +574,12 @@ struct page_info *p2m_get_page_from_gfn( if ( fdom == NULL ) page = NULL; } - else if ( !get_page(page, p2m->domain) && - /* Page could be shared */ - (!dom_cow || !p2m_is_shared(*t) || - !get_page(page, dom_cow)) ) - page = NULL; + else + { + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; + if ( !get_page(page, d) ) + page = NULL; + } } p2m_read_unlock(p2m); @@ -594,8 +595,9 @@ struct page_info *p2m_get_page_from_gfn( mfn = get_gfn_type_access(p2m, gfn_x(gfn), t, a, q, NULL); if ( p2m_is_ram(*t) && mfn_valid(mfn) ) { + struct domain *d = !p2m_is_shared(*t) ? p2m->domain : dom_cow; page = mfn_to_page(mfn); - if ( !get_page(page, p2m->domain) ) + if ( !get_page(page, d) ) page = NULL; } put_gfn(p2m->domain, gfn_x(gfn));
The owner domain of shared pages is dom_cow, use that for get_page otherwise the function fails to return the correct page. Fixing the error now and simplifying the checks since we can't have any shared entries with dom_cow being NULL. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- v6: use simplified check for dom_cow in both slow and fast path --- xen/arch/x86/mm/p2m.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)