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

[David Vrabel <david.vrabel@citrix.com>]

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