Message ID | 920fa307-190e-dc11-f338-5b44a2126050@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn | expand |
On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: > In this case up to now we've been freeing the page (through > guest_remove_page(), with the actual free typically happening at the > put_page() later in the function), but then failing the call on the > subsequent GFN consistency check. However, in my opinion such a request > should complete as an "expensive" no-op (leaving aside the potential > unsharing of the page). > > This points out that f33d653f46f5 ("x86: replace bad ASSERT() in > xenmem_add_to_physmap_one()" would really have needed an XSA, despite > its description claiming otherwise, as in release builds we then put in > place a P2M entry referencing the about to be freed page. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I've been considering to make such operations "cheap" NOPs rather than > "expensive" ones, by comparing idx and gpfn early in the function in > the XENMAPSPACE_gmfn case block, but I've come to the conclusion that > having the operation perform otherwise normally is better - this way, > errors that would result if idx != gpfn will still result. While I'm > open to reasons towards the other alternative, having the added check be > MFN-based makes crystal clear that we're dealing with the same > underlying physical resource, i.e. also covers the hypothetical(?) case > of two GFNs referring to the same MFN. > > I'm unconvinced that it is correct for prev_mfn's p2mt to not be > inspected at all - I don't think things will go right if p2m_shared() > was true for it. But I'm afraid I'm not up to correcting mem-sharing > related logic. > > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( > if ( is_special_page(mfn_to_page(prev_mfn)) ) > /* Special pages are simply unhooked from this phys slot. */ > rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); > - else > + else if ( !mfn_eq(mfn, prev_mfn) ) > /* Normal domain memory is freed, to avoid leaking memory. */ > rc = guest_remove_page(d, gfn_x(gpfn)); What about the access differing between the old and the new entries, while pointing to the same mfn, would Xen install the new entry successfully? Seems easier to me to use guest_physmap_remove_page in that case to remove the entry from the p2m without freeing the page. Thanks, Roger.
On 21.10.2020 11:39, Roger Pau Monné wrote: > On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( >> if ( is_special_page(mfn_to_page(prev_mfn)) ) >> /* Special pages are simply unhooked from this phys slot. */ >> rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); >> - else >> + else if ( !mfn_eq(mfn, prev_mfn) ) >> /* Normal domain memory is freed, to avoid leaking memory. */ >> rc = guest_remove_page(d, gfn_x(gpfn)); > > What about the access differing between the old and the new entries, > while pointing to the same mfn, would Xen install the new entry > successfully? Yes - guest_physmap_add_page() doesn't get bypassed. > Seems easier to me to use guest_physmap_remove_page in that case to > remove the entry from the p2m without freeing the page. Why do any removal when none is really needed? I also don't see this fit the "special pages" clause and comment very well. I'd question the other way around whether guest_physmap_remove_page() needs calling at all (the instance above; the other one of course is needed). Jan
On Wed, Oct 21, 2020 at 12:38:48PM +0200, Jan Beulich wrote: > On 21.10.2020 11:39, Roger Pau Monné wrote: > > On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: > >> --- a/xen/arch/x86/mm.c > >> +++ b/xen/arch/x86/mm.c > >> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( > >> if ( is_special_page(mfn_to_page(prev_mfn)) ) > >> /* Special pages are simply unhooked from this phys slot. */ > >> rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); > >> - else > >> + else if ( !mfn_eq(mfn, prev_mfn) ) > >> /* Normal domain memory is freed, to avoid leaking memory. */ > >> rc = guest_remove_page(d, gfn_x(gpfn)); > > > > What about the access differing between the old and the new entries, > > while pointing to the same mfn, would Xen install the new entry > > successfully? > > Yes - guest_physmap_add_page() doesn't get bypassed. But will it succeed if the default access is different from the one the installed entry currently has? Will it update the access bits to match the new ones? > > > Seems easier to me to use guest_physmap_remove_page in that case to > > remove the entry from the p2m without freeing the page. > > Why do any removal when none is really needed? I also don't see > this fit the "special pages" clause and comment very well. I'd > question the other way around whether guest_physmap_remove_page() > needs calling at all (the instance above; the other one of course > is needed). Right, replying to my question above: it will succeed, since guest_physmap_add_entry will overwrite the previous entry. I agree, it looks like the guest_physmap_remove_page call done for special pages is not really needed, as guest_physmap_add_entry would already overwrite such entries and not free the associated mfn? Roger.
On 21.10.2020 12:58, Roger Pau Monné wrote: > On Wed, Oct 21, 2020 at 12:38:48PM +0200, Jan Beulich wrote: >> On 21.10.2020 11:39, Roger Pau Monné wrote: >>> On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: >>>> --- a/xen/arch/x86/mm.c >>>> +++ b/xen/arch/x86/mm.c >>>> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( >>>> if ( is_special_page(mfn_to_page(prev_mfn)) ) >>>> /* Special pages are simply unhooked from this phys slot. */ >>>> rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); >>>> - else >>>> + else if ( !mfn_eq(mfn, prev_mfn) ) >>>> /* Normal domain memory is freed, to avoid leaking memory. */ >>>> rc = guest_remove_page(d, gfn_x(gpfn)); >>> >>> What about the access differing between the old and the new entries, >>> while pointing to the same mfn, would Xen install the new entry >>> successfully? >> >> Yes - guest_physmap_add_page() doesn't get bypassed. > > But will it succeed if the default access is different from the one > the installed entry currently has? Will it update the access bits > to match the new ones? It will construct and put in place a completely new entry. Old values are of concern only for keeping statistics right, and of course for refusing certain changes. >>> Seems easier to me to use guest_physmap_remove_page in that case to >>> remove the entry from the p2m without freeing the page. >> >> Why do any removal when none is really needed? I also don't see >> this fit the "special pages" clause and comment very well. I'd >> question the other way around whether guest_physmap_remove_page() >> needs calling at all (the instance above; the other one of course >> is needed). > > Right, replying to my question above: it will succeed, since > guest_physmap_add_entry will overwrite the previous entry. > > I agree, it looks like the guest_physmap_remove_page call done for > special pages is not really needed, as guest_physmap_add_entry would > already overwrite such entries and not free the associated mfn? That's my understanding, yes. Jan
On Wed, Oct 21, 2020 at 01:10:20PM +0200, Jan Beulich wrote: > On 21.10.2020 12:58, Roger Pau Monné wrote: > > On Wed, Oct 21, 2020 at 12:38:48PM +0200, Jan Beulich wrote: > >> On 21.10.2020 11:39, Roger Pau Monné wrote: > >>> On Fri, Oct 16, 2020 at 08:44:10AM +0200, Jan Beulich wrote: > >>>> --- a/xen/arch/x86/mm.c > >>>> +++ b/xen/arch/x86/mm.c > >>>> @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( > >>>> if ( is_special_page(mfn_to_page(prev_mfn)) ) > >>>> /* Special pages are simply unhooked from this phys slot. */ > >>>> rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); > >>>> - else > >>>> + else if ( !mfn_eq(mfn, prev_mfn) ) > >>>> /* Normal domain memory is freed, to avoid leaking memory. */ > >>>> rc = guest_remove_page(d, gfn_x(gpfn)); > >>> > >>> What about the access differing between the old and the new entries, > >>> while pointing to the same mfn, would Xen install the new entry > >>> successfully? > >> > >> Yes - guest_physmap_add_page() doesn't get bypassed. > > > > But will it succeed if the default access is different from the one > > the installed entry currently has? Will it update the access bits > > to match the new ones? > > It will construct and put in place a completely new entry. Old > values are of concern only for keeping statistics right, and > of course for refusing certain changes. > > >>> Seems easier to me to use guest_physmap_remove_page in that case to > >>> remove the entry from the p2m without freeing the page. > >> > >> Why do any removal when none is really needed? I also don't see > >> this fit the "special pages" clause and comment very well. I'd > >> question the other way around whether guest_physmap_remove_page() > >> needs calling at all (the instance above; the other one of course > >> is needed). > > > > Right, replying to my question above: it will succeed, since > > guest_physmap_add_entry will overwrite the previous entry. > > > > I agree, it looks like the guest_physmap_remove_page call done for > > special pages is not really needed, as guest_physmap_add_entry would > > already overwrite such entries and not free the associated mfn? > > That's my understanding, yes. Then: Acked-by: Roger Pau Monné <roger.pau@citrix.com> Albeit I would also like to see the call to guest_physmap_remove_page for special pages removed for consistency. Thanks, Roger.
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -4555,7 +4555,7 @@ int xenmem_add_to_physmap_one( if ( is_special_page(mfn_to_page(prev_mfn)) ) /* Special pages are simply unhooked from this phys slot. */ rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K); - else + else if ( !mfn_eq(mfn, prev_mfn) ) /* Normal domain memory is freed, to avoid leaking memory. */ rc = guest_remove_page(d, gfn_x(gpfn)); }
In this case up to now we've been freeing the page (through guest_remove_page(), with the actual free typically happening at the put_page() later in the function), but then failing the call on the subsequent GFN consistency check. However, in my opinion such a request should complete as an "expensive" no-op (leaving aside the potential unsharing of the page). This points out that f33d653f46f5 ("x86: replace bad ASSERT() in xenmem_add_to_physmap_one()" would really have needed an XSA, despite its description claiming otherwise, as in release builds we then put in place a P2M entry referencing the about to be freed page. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I've been considering to make such operations "cheap" NOPs rather than "expensive" ones, by comparing idx and gpfn early in the function in the XENMAPSPACE_gmfn case block, but I've come to the conclusion that having the operation perform otherwise normally is better - this way, errors that would result if idx != gpfn will still result. While I'm open to reasons towards the other alternative, having the added check be MFN-based makes crystal clear that we're dealing with the same underlying physical resource, i.e. also covers the hypothetical(?) case of two GFNs referring to the same MFN. I'm unconvinced that it is correct for prev_mfn's p2mt to not be inspected at all - I don't think things will go right if p2m_shared() was true for it. But I'm afraid I'm not up to correcting mem-sharing related logic.