Re: [PATCH v12 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

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

Hi,

On 10/19/2017 01:57 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 19 October 2017 13:23
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
>> <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>;
>> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim
>> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan Beulich
>> <jbeulich@suse.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Roger
>> Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi,
>>
>> On 17/10/17 14:24, Paul Durrant wrote:
>>> Certain memory resources associated with a guest are not necessarily
>>> present in the guest P2M.
>>>
>>> This patch adds the boilerplate for new memory op to allow such a
>> resource
>>> to be priv-mapped directly, by either a PV or HVM tools domain.
>>>
>>> NOTE: Whilst the new op is not intrinsicly specific to the x86 architecture,
>>>         I have no means to test it on an ARM platform and so cannot verify
>>>         that it functions correctly.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> ---
>>
>> [...]
>>
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index ad987e0f29..cdd2e030cf 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -965,6 +965,95 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>>
>> [...]
>>
>>> +    if ( rc )
>>> +        goto out;
>>> +
>>> +    if ( !paging_mode_translate(currd) )
>>> +    {
>>> +        if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
>>> +            rc = -EFAULT;
>>> +    }
>>> +    else
>>> +    {
>>> +        xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
>>> +        unsigned int i;
>>> +
>>> +        rc = -EFAULT;
>>> +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
>>> +            goto out;
>>> +
>>> +        for ( i = 0; i < xmar.nr_frames; i++ )
>>> +        {
>>> +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>>> +                                       _mfn(mfn_list[i]));
>>
>> Something looks a bit odd to me here. When I read foreign mapping, I
>> directly associate to mapping from a foreign domain.
>>
>> On Arm, we will always get a reference on that page to prevent it
>> disappearing if the foreign domain is destroyed but the mapping is still
>> present.
>>
>> This reference will either be put with an unmapped hypercall or while
>> teardown the domain.
>>
>> Per my understanding, this MFN does not belong to any domain (or at
>> least currd). Right?
>  
> No. The mfns do belong to the target domain.

To be fully safe, you need to take a reference on each page you mapped. 
So who is going to get a reference on them? Who is going to drop that?

> 
>> So there is no way to get/put a reference on that
>> page. So I am unconvinced that this is very safe.
>>
>> Also looking at the x86 side, I can't find such reference in the foreign
>> path in p2m_add_foreign. Did I miss anything?
> 
> No, I don't think there is any reference counting there... but this is no different to priv mapping. I'm not trying to fix the mapping infrastructure at this point.
> 
>>
>> Note that x86 does not handle p2m teardown with foreign map at the
>> moment (see p2m_add_foreign).
>>
>> You are by-passing this check and I can't see how this would be safe for
>> the x86 side too.
>>
> 
> I don't follow. What check am I by-passing that is covered when priv mapping?

     /*
      * hvm fixme: until support is added to p2m teardown code to 
cleanup any
      * foreign entries, limit this to hardware domain only.
      */

How this is safe with your new solution? That looks like a regression...

[...]

>>> +     *          will be populated with the MFNs of the resource.
>>> +     *          If the tools domain is HVM then it is expected that, on
>>> +     *          entry, frame_list will be populated with a list of GFNs
>>> +     *          that will be mapped to the MFNs of the resource.
>>> +     *          If -EIO is returned then the frame_list has only been
>>> +     *          partially mapped and it is up to the caller to unmap all
>>> +     *          the GFNs.
>>> +     *          This parameter may be NULL if nr_frames is 0.
>>> +     */
>>> +    XEN_GUEST_HANDLE(xen_ulong_t) frame_list;
>>> +};
>>> +typedef struct xen_mem_acquire_resource
>> xen_mem_acquire_resource_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_mem_acquire_resource_t);
>>> +
>>>    #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>>>
>>>    /*
>>
> 
> Sorry to be getting frustrated with this, but I'm wondering how many more colours I need to paint this bike-shed.

I don't know how x86 looks like and maybe this is fine for Andrew and 
Jan. But for Arm, it does not look correct.

To give you an idea, my first thought to implement your newly wrongly 
named function was to just call p2m_set_entry with p2m_map_foreign. But 
from this discussion it would look plain wrong.

So this means the interface is not clear enough.

Cheers,

-- 
Julien Grall

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