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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.02.14 at 03:37, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 10 Feb 2014 15:33:18 +0000
> Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 
>> 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
>> >>
>> >> 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.
> 
> 
> So, how about following:
> 
> +++ b/xen/common/domain.c
> @@ -484,8 +484,20 @@ struct domain *rcu_lock_domain_by_any_id(domid_t dom)
>  
>   int rcu_lock_remote_domain_by_id(domid_t dom, struct domain **d)
>    {
>    -    if ( (*d = rcu_lock_domain_by_id(dom)) == NULL )
>    +
>    +    if ( (dom == DOMID_XEN && (*d = rcu_lock_domain(dom_xen)) == NULL) ||
>    +         (dom != DOMID_XEN && (*d = rcu_lock_domain_by_id(dom)) == NULL) )
>    +        return -ESRCH;
>    +

Changing a very generic function for a very special purpose is
almost never the right thing: I'd suppose that _if_ this is really the
right thing to do in the first place (which I continue to doubt without
being presented with full context), it should be done in the one or
very few call sites that actually need this (depending on their count
a new helper function might then be worthwhile introducing).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel