Re: [V7 PATCH 5/7] pvh: change xsm_add_to_physmap

[Julien Grall <julien.grall@linaro.org>]

On 02/10/2014 03:27 PM, Ian Campbell wrote:
> On Mon, 2014-02-10 at 15:16 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 02/10/2014 01:42 PM, Ian Campbell wrote:
>>> On Sun, 2014-02-09 at 16:51 +0000, Julien Grall wrote:
>>>> Hello Mukesh,
>>>>
>>>> On 30/01/14 01:33, Mukesh Rathor wrote:
>>>>>>> I'm not sure what you mean:
>>>>>>>   - the code that Mukesh is adding doesn't have a struct page, it's
>>>>>>>     just grabbing the foreign domid from the hypercall arg;
>>>>>>>   - if we did have a struct page, we'd just need to take a ref to
>>>>>>>     stop the owner changing underfoot; and
>>>>>>>   - get_pg_owner() takes a domid anyway.
>>>>>>
>>>>>> Sorry, I was confused/mislead by the name...
>>>>>>
>>>>>> rcu_lock_live_remote_domain_by_id does look like what is needed.
>>>>
>>>> Following the xentrace tread: 
>>>> http://www.gossamer-threads.com/lists/xen/devel/315883, 
>>>> rcu_lock_live_remote_domain_by_id will not correctly works.
>>>>
>>>> On Xen on ARM, xentrace is using this hypercall to map XEN page (via 
>>>> DOMID_XEN). In this case, rcu_lock_*domain* will always fails which will 
>>>> prevent xentrace to works on Xen on ARM (and on PVH).
>>>
>>> I'm not sure how that extends to add_to_physmap though -- doing add to
>>> physmap of a DOMID_XEN owned page through the "back door" in this way
>>> isn't supposed to work.
>>
>> Currently xentrace is using xc_map_foreign_page to map the trace buffer
>> (with DOMID_XEN in argument).
> 
> Sorry, I misunderstood, I thought you were suggesting that rcu_lock_...
> didn't work for xentrace and so couldn't work for add_to_physmap either
> -- but actually xentrace ends up using add_to_physmap itself.
> 
>> AFAIK, on x86 PV domain, this called is resulting by an
>> HYPERVISOR_mmu_update which allow do map xen page on priviliged domain
>> (with the dummy XSM policy).
>>
>> For ARM, a call to xc_map_foreign_page will end up to
>> XENMEM_add_to_physmap_range(XENMAPSPACE_gmfn_foreign).
>>
>> For both architecture, you can look at the function
>> xen_remap_map_domain_mfn_range (implemented differently on ARM and x86)
>> which is the last function called before going to the hypervisor.
>>
>> If we don't modify the hypercall XENMEM_add_to_physmap, we will have a
>> add a new way to map Xen page for xentrace & co.
> 
> Wouldn't it be incorrect to generically return OK for mapping a
> DOMID_XEN owned page -- at least something needs to validate that the
> particular mfn being mapped is supposed to be shared with the guest in
> question.

It's already the case. By default a xen heap page doesn't belong to
DOMID_XEN. Xen has to explicitly call share_xen_page_with_privileged
guess (see an example in xen/common/trace.c:244) to set DOMID_XEN to the
given page.

> 
> TBH, it doesn't seem that this mechanism for sharing xenpages with
> guests is a good fit for PVH or HVM guests/dom0. Perhaps a specific
> mapspace would be more appropriate?

Following my explanation just above, I don't think we need to have a
specific mapspace. XSM and DOMID_XEN will already protect correctly the
mapping of xen heap page.

-- 
Julien Grall