Re: [PATCH v2 1/2] xen/balloon: set a mapping for ballooned out pages

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

On 23/07/13 15:27, Ian Campbell wrote:
> On Tue, 2013-07-23 at 10:58 +0100, David Vrabel wrote:
>> On 22/07/13 18:32, Ian Campbell wrote:
>>> On Mon, 2013-07-22 at 18:26 +0100, David Vrabel wrote:
>>>> On 22/07/13 18:22, Ian Campbell wrote:
>>>>> On Mon, 2013-07-22 at 17:51 +0100, David Vrabel wrote:
>>>>>> On 22/07/13 17:28, Stefano Stabellini wrote:
>>>>>>>
>>>>>>>  #ifdef CONFIG_HIGHMEM
>>>>>>>  #define inc_totalhigh_pages() (totalhigh_pages++)
>>>>>>> @@ -423,7 +426,8 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>  		if (xen_pv_domain() && !PageHighMem(page)) {
>>>>>>>  			ret = HYPERVISOR_update_va_mapping(
>>>>>>>  				(unsigned long)__va(pfn << PAGE_SHIFT),
>>>>>>> -				__pte_ma(0), 0);
>>>>>>> +				pfn_pte(page_to_pfn(get_balloon_trade_page()),
>>>>>>> +					PAGE_KERNEL_RO), 0);
>>>>>>
>>>>>> Preemption needs to be disabled while using the trade page, see
>>>>>> suggestion below.
>>>>>
>>>>> Hopefully you mean just when setting up/manipulating it?
>>>>
>>>> Yes, sorry.
>>>>
>>>> get_...()
>>>> update_va_mapping()
>>>> put_...()
>>>
>>> I can see why it would matter in the unmap_and_replace+fixup case (since
>>> you need them to happen "atomically") but why in this case? We don't
>>> actually care which of the trade pages gets used for this purpose, so
>>> even if we happen to get preempted and change CPU it doesn't really
>>> matter.
>>
>> If a trade page from another CPU is used, it may concurrently have it's
>> MFN cleared by a unmap_and_replace call on the other CPU.
> 
> The call on the other CPU clears the virtual address, not the MFN, so
> long as we have the MFN in our hand we are OK, I think? Nothing updates
> the m2p or p2m does it?

Yes, I think you're correct here.

But, does this need to be mfn_pte(page_to_mfn(get_balloon_trade_page())?
 The pte parameter to update_va_mapping() needs to contains the machine
address, right?

David