Xen devel - Aug 2013 - [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
we have the ballooned out page''s mapping set to a scratch page. That
commit
also set the P2M entry of ballooned out page to the scratch, which is
not necessary. That triggers BUG_ONs in __set_phys_to_machine when the
page is ballooned in again.

We only need to ensure that the ballooned out pages have valid mapping.
P2M entries of those pages should still be set to INVALID_P2M_ENTRY.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/balloon.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index a3dc75d..b1a37f3 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -430,8 +430,7 @@ static enum bp_state decrease_reservation(unsigned long
nr_pages, gfp_t gfp)
 	/* No more mappings: invalidate P2M and add to balloon. */
 	for (i = 0; i < nr_pages; i++) {
 		pfn = mfn_to_pfn(frame_list[i]);
-		__set_phys_to_machine(pfn,
-				pfn_to_mfn(page_to_pfn(__get_cpu_var(balloon_scratch_page))));
+		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 		balloon_append(pfn_to_page(pfn));
 	}
 
-- 
1.7.10.4

On 22/08/13 13:57, Wei Liu wrote:
> In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
> we have the ballooned out page''s mapping set to a scratch page.
That commit
> also set the P2M entry of ballooned out page to the scratch, which is
> not necessary. That triggers BUG_ONs in __set_phys_to_machine when the
> page is ballooned in again.
> 
> We only need to ensure that the ballooned out pages have valid mapping.
> P2M entries of those pages should still be set to INVALID_P2M_ENTRY.
I know I suggested this but I would like to get Stefano''s acked-by or
reviewed-by for this.

David

On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel
wrote:
> On 22/08/13 13:57, Wei Liu wrote:
> > In commit cd9151e2: xen/balloon: set a mapping for ballooned out pages
> > we have the ballooned out page''s mapping set to a scratch
page. That commit
> > also set the P2M entry of ballooned out page to the scratch, which is
> > not necessary. That triggers BUG_ONs in __set_phys_to_machine when the
> > page is ballooned in again.
> > 
> > We only need to ensure that the ballooned out pages have valid
mapping.
> > P2M entries of those pages should still be set to INVALID_P2M_ENTRY.
> 
> I know I suggested this but I would like to get Stefano''s acked-by
or
> reviewed-by for this.
> 
Stefano, what do you think?

Wei.
> David

On Mon, 26 Aug 2013, Wei Liu wrote:
> On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel wrote:
> > On 22/08/13 13:57, Wei Liu wrote:
> > > In commit cd9151e2: xen/balloon: set a mapping for ballooned out
pages
> > > we have the ballooned out page''s mapping set to a
scratch page. That commit
> > > also set the P2M entry of ballooned out page to the scratch,
which is
> > > not necessary. That triggers BUG_ONs in __set_phys_to_machine
when the
> > > page is ballooned in again.
> > > 
> > > We only need to ensure that the ballooned out pages have valid
mapping.
> > > P2M entries of those pages should still be set to
INVALID_P2M_ENTRY.
> > 
> > I know I suggested this but I would like to get Stefano''s
acked-by or
> > reviewed-by for this.
> > 
> 
> Stefano, what do you think?
I think it is wrong: if the page has a valid mapping it should also have
a valid p2m entry. It''s easier to think about and it''s going
to avoid
bugs when generic linux code ends up calling a pvop that on Xen looks
into the p2m.

Which one of the BUG_ON in __set_phys_to_machine are hit?  We could
avoid calling set_phys_to_machine in decrease_reservation on
XENFEAT_auto_translated_physmap guests, that would avoid the first
BUG_ON in __set_phys_to_machine. Arguably it''s the right thing to do
anyway.

Regarding the second BUG_ON:

if (unlikely(pfn >= MAX_P2M_PFN)) {
    BUG_ON(mfn != INVALID_P2M_ENTRY);
    return true;
}

We shouldn''t really be hitting it, right? Why is pfn >= MAX_P2M_PFN?

On 27/08/13 14:34, Stefano Stabellini wrote:
> On Mon, 26 Aug 2013, Wei Liu wrote:
>> On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel wrote:
>>> On 22/08/13 13:57, Wei Liu wrote:
>>>> In commit cd9151e2: xen/balloon: set a mapping for ballooned
out pages
>>>> we have the ballooned out page''s mapping set to a
scratch page. That commit
>>>> also set the P2M entry of ballooned out page to the scratch,
which is
>>>> not necessary. That triggers BUG_ONs in __set_phys_to_machine
when the
>>>> page is ballooned in again.
>>>>
>>>> We only need to ensure that the ballooned out pages have valid
mapping.
>>>> P2M entries of those pages should still be set to
INVALID_P2M_ENTRY.
>>>
>>> I know I suggested this but I would like to get Stefano''s
acked-by or
>>> reviewed-by for this.
>>>
>>
>> Stefano, what do you think?
> 
> I think it is wrong: if the page has a valid mapping it should also have
> a valid p2m entry. It''s easier to think about and it''s
going to avoid
> bugs when generic linux code ends up calling a pvop that on Xen looks
> into the p2m.
But in the auto_translated physmap case it does /not/ have a valid p2m
entry -- only the virtual mappping....
> Which one of the BUG_ON in __set_phys_to_machine are hit?  We could
> avoid calling set_phys_to_machine in decrease_reservation on
> XENFEAT_auto_translated_physmap guests, that would avoid the first
> BUG_ON in __set_phys_to_machine. Arguably it''s the right thing to
do
> anyway.
... which makes the suggestion to avoid the __set_phys_to_machine()
calls sensible to me.
> Regarding the second BUG_ON:
> 
> if (unlikely(pfn >= MAX_P2M_PFN)) {
>     BUG_ON(mfn != INVALID_P2M_ENTRY);
>     return true;
> }
> 
> We shouldn''t really be hitting it, right? Why is pfn >=
MAX_P2M_PFN?
I believe it was the check for pfn == mfn.

David

On Tue, Aug 27, 2013 at 02:34:42PM +0100, Stefano Stabellini
wrote:
> On Mon, 26 Aug 2013, Wei Liu wrote:
> > On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel wrote:
> > > On 22/08/13 13:57, Wei Liu wrote:
> > > > In commit cd9151e2: xen/balloon: set a mapping for ballooned
out pages
> > > > we have the ballooned out page''s mapping set to a
scratch page. That commit
> > > > also set the P2M entry of ballooned out page to the scratch,
which is
> > > > not necessary. That triggers BUG_ONs in
__set_phys_to_machine when the
> > > > page is ballooned in again.
> > > > 
> > > > We only need to ensure that the ballooned out pages have
valid mapping.
> > > > P2M entries of those pages should still be set to
INVALID_P2M_ENTRY.
> > > 
> > > I know I suggested this but I would like to get
Stefano''s acked-by or
> > > reviewed-by for this.
> > > 
> > 
> > Stefano, what do you think?
> 
> I think it is wrong: if the page has a valid mapping it should also have
> a valid p2m entry. It''s easier to think about and it''s
going to avoid
> bugs when generic linux code ends up calling a pvop that on Xen looks
> into the p2m.
> 
> Which one of the BUG_ON in __set_phys_to_machine are hit?  We could
Both.
> avoid calling set_phys_to_machine in decrease_reservation on
> XENFEAT_auto_translated_physmap guests, that would avoid the first
> BUG_ON in __set_phys_to_machine. Arguably it''s the right thing to
do
> anyway.
> 
OK.
> Regarding the second BUG_ON:
> 
> if (unlikely(pfn >= MAX_P2M_PFN)) {
>     BUG_ON(mfn != INVALID_P2M_ENTRY);
>     return true;
> }
> 
> We shouldn''t really be hitting it, right? Why is pfn >=
MAX_P2M_PFN?
I think I did something silly, but I could only vaguely remember that I
tried to play with very high virtual address, which could lead to pfn
>MAX_P2M_PFN.

AIUI we can definitively hit this case otherwise we would just have

 BUG_ON(pfn >= MAX_P2M_PFN)

right?

Wei.

On Tue, 27 Aug 2013, Wei Liu wrote:
> On Tue, Aug 27, 2013 at 02:34:42PM +0100, Stefano Stabellini wrote:
> > On Mon, 26 Aug 2013, Wei Liu wrote:
> > > On Thu, Aug 22, 2013 at 02:00:05PM +0100, David Vrabel wrote:
> > > > On 22/08/13 13:57, Wei Liu wrote:
> > > > > In commit cd9151e2: xen/balloon: set a mapping for
ballooned out pages
> > > > > we have the ballooned out page''s mapping set
to a scratch page. That commit
> > > > > also set the P2M entry of ballooned out page to the
scratch, which is
> > > > > not necessary. That triggers BUG_ONs in
__set_phys_to_machine when the
> > > > > page is ballooned in again.
> > > > > 
> > > > > We only need to ensure that the ballooned out pages
have valid mapping.
> > > > > P2M entries of those pages should still be set to
INVALID_P2M_ENTRY.
> > > > 
> > > > I know I suggested this but I would like to get
Stefano''s acked-by or
> > > > reviewed-by for this.
> > > > 
> > > 
> > > Stefano, what do you think?
> > 
> > I think it is wrong: if the page has a valid mapping it should also
have
> > a valid p2m entry. It''s easier to think about and
it''s going to avoid
> > bugs when generic linux code ends up calling a pvop that on Xen looks
> > into the p2m.
> > 
> > Which one of the BUG_ON in __set_phys_to_machine are hit?  We could
> 
> Both.
> 
> > avoid calling set_phys_to_machine in decrease_reservation on
> > XENFEAT_auto_translated_physmap guests, that would avoid the first
> > BUG_ON in __set_phys_to_machine. Arguably it''s the right
thing to do
> > anyway.
> > 
> 
> OK.
> 
> > Regarding the second BUG_ON:
> > 
> > if (unlikely(pfn >= MAX_P2M_PFN)) {
> >     BUG_ON(mfn != INVALID_P2M_ENTRY);
> >     return true;
> > }
> > 
> > We shouldn''t really be hitting it, right? Why is pfn >=
MAX_P2M_PFN?
> 
> I think I did something silly, but I could only vaguely remember that I
> tried to play with very high virtual address, which could lead to pfn
>> MAX_P2M_PFN.
> 
> AIUI we can definitively hit this case otherwise we would just have
> 
>  BUG_ON(pfn >= MAX_P2M_PFN)
> 
> right?
I think that the idea is that if you try to set the p2m for a pfn
>MAX_P2M_PFN then the mfn has to be invalid, otherwise it would be an
existing and correct p2m mapping but in that case pfn would not be
>MAX_P2M_PFN.