| Message ID | cf992d6e4dd09acc48afb42d43be4a900baee189.1581362050.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 10.02.2020 20:21, 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. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> I find it quite disappointing that the blank lines requested to be added ... > --- 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) ) .. above here and ... > @@ -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); ... above here still haven't appeared. No matter that it's easy to do so while committing, when you send a new version you should really address such remarks yourself, I think. Jan
On Tue, Feb 11, 2020 at 2:17 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.02.2020 20:21, 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. > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > I find it quite disappointing that the blank lines requested to be > added ... > > > --- 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) ) > > .. above here and ... > > > @@ -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); > > ... above here still haven't appeared. No matter that it's easy to > do so while committing, when you send a new version you should > really address such remarks yourself, I think. Noted. I haven't addressed it since it appeared to me that this patch has been ready to go in for like 3 revisions already as-is given the blank-lines were non-blockers. By the time I get around rolling a new one I simply forget nuisance style issues like this. I know we have been having the discussion about having automated style-checks and style-formatting added to Xen, this just further highlights to me the need for it as we are wasting time and energy on stuff like this for no real reason. Tamas
On 11.02.2020 11:29, Tamas K Lengyel wrote: > On Tue, Feb 11, 2020 at 2:17 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 10.02.2020 20:21, 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. >>> >>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> >> >> I find it quite disappointing that the blank lines requested to be >> added ... >> >>> --- 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) ) >> >> .. above here and ... >> >>> @@ -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); >> >> ... above here still haven't appeared. No matter that it's easy to >> do so while committing, when you send a new version you should >> really address such remarks yourself, I think. > > Noted. I haven't addressed it since it appeared to me that this patch > has been ready to go in for like 3 revisions already as-is given the > blank-lines were non-blockers. The patch continues to lack a maintainer ack. Hence it hasn't been ready to go in at any point in time. Jan
On Tue, Feb 11, 2020 at 4:04 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 11.02.2020 11:29, Tamas K Lengyel wrote: > > On Tue, Feb 11, 2020 at 2:17 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 10.02.2020 20:21, 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. > >>> > >>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > >> > >> I find it quite disappointing that the blank lines requested to be > >> added ... > >> > >>> --- 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) ) > >> > >> .. above here and ... > >> > >>> @@ -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); > >> > >> ... above here still haven't appeared. No matter that it's easy to > >> do so while committing, when you send a new version you should > >> really address such remarks yourself, I think. > > > > Noted. I haven't addressed it since it appeared to me that this patch > > has been ready to go in for like 3 revisions already as-is given the > > blank-lines were non-blockers. > > The patch continues to lack a maintainer ack. Hence it hasn't been > ready to go in at any point in time. I meant there has been no comments or anything blocking noted for three resends now. Tamas
On 10/02/2020 19:21, 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. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> Given the recent change in p2m maintainership, I've committed this and fixed up the style issues. ~Andrew
On Fri, Feb 21, 2020 at 6:49 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 10/02/2020 19:21, 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. > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > Given the recent change in p2m maintainership, I've committed this and > fixed up the style issues. Thanks, appreciated! Tamas
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index fd9f09536d..2c0bb7e869 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 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> --- xen/arch/x86/mm/p2m.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)