* [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
@ 2020-10-16 6:44 Jan Beulich
2020-10-21 9:39 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-10-16 6:44 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap
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));
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
2020-10-16 6:44 [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn Jan Beulich
@ 2020-10-21 9:39 ` Roger Pau Monné
2020-10-21 10:38 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2020-10-21 9:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
2020-10-21 9:39 ` Roger Pau Monné
@ 2020-10-21 10:38 ` Jan Beulich
2020-10-21 10:58 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-10-21 10:38 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
2020-10-21 10:38 ` Jan Beulich
@ 2020-10-21 10:58 ` Roger Pau Monné
2020-10-21 11:10 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2020-10-21 10:58 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
2020-10-21 10:58 ` Roger Pau Monné
@ 2020-10-21 11:10 ` Jan Beulich
2020-10-21 11:36 ` Roger Pau Monné
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2020-10-21 11:10 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn
2020-10-21 11:10 ` Jan Beulich
@ 2020-10-21 11:36 ` Roger Pau Monné
0 siblings, 0 replies; 6+ messages in thread
From: Roger Pau Monné @ 2020-10-21 11:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-21 11:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 6:44 [PATCH] x86: XENMAPSPACE_gmfn{,_batch,_range} want to special case idx == gpfn Jan Beulich
2020-10-21 9:39 ` Roger Pau Monné
2020-10-21 10:38 ` Jan Beulich
2020-10-21 10:58 ` Roger Pau Monné
2020-10-21 11:10 ` Jan Beulich
2020-10-21 11:36 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.