| Message ID | b784bc7edf0c267e03100b30a0250454be37f59c.1580746020.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 03.02.2020 17:12, Tamas K Lengyel wrote: > The owner domain of shared pages is dom_cow, use that for get_page > otherwise the function fails to return the correct page under some > situations. The check if dom_cow should be used was only performed in > a subset of use-cases. Fixing the error and simplifying the existing check > since we can't have any shared entries with dom_cow being NULL. Better, thanks. > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > --- > v7: update commit message > --- > xen/arch/x86/mm/p2m.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index def13f657b..007fef720d 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -575,11 +575,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) ) There's still a blank line missing between these two lines and ... > + page = NULL; > + } > } > p2m_read_unlock(p2m); > > @@ -595,8 +596,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); ... similarly between these two. As indicated before, with them added Reviewed-by: Jan Beulich <jbeulich@suse.com> despite me still thinking that the title suggests more of a problem than there really was. (Of course adding the missing blank lines ought to be easy enough while committing.) Jan
On Mon, Feb 3, 2020 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.02.2020 17:12, Tamas K Lengyel wrote: > > The owner domain of shared pages is dom_cow, use that for get_page > > otherwise the function fails to return the correct page under some > > situations. The check if dom_cow should be used was only performed in > > a subset of use-cases. Fixing the error and simplifying the existing check > > since we can't have any shared entries with dom_cow being NULL. > > Better, thanks. > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > --- > > v7: update commit message > > --- > > xen/arch/x86/mm/p2m.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > > index def13f657b..007fef720d 100644 > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -575,11 +575,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) ) > > There's still a blank line missing between these two lines and ... > > > + page = NULL; > > + } > > } > > p2m_read_unlock(p2m); > > > > @@ -595,8 +596,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); > > ... similarly between these two. As indicated before, with them > added > Reviewed-by: Jan Beulich <jbeulich@suse.com> > despite me still thinking that the title suggests more of a > problem than there really was. (Of course adding the missing > blank lines ought to be easy enough while committing.) Thanks, I did forgot to add those extra lines. To me this looks fine as-is so it just doesn't come to mind naturally. Tamas
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index def13f657b..007fef720d 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -575,11 +575,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); @@ -595,8 +596,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 under some situations. The check if dom_cow should be used was only performed in a subset of use-cases. Fixing the error and simplifying the existing check since we can't have any shared entries with dom_cow being NULL. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- v7: update commit message --- xen/arch/x86/mm/p2m.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)