[PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Wei Liu]

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

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[David Vrabel]

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

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Wei Liu]

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

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Stefano Stabellini]

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?

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[David Vrabel]

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

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Wei Liu]

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.

Re: [PATCH] xen: set P2M entry to INVALID_P2M_ENTRY for ballooned out pages

[Stefano Stabellini]

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.