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

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

Hi Paul,

On 27/10/17 16:19, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@linaro.org]
>> Sent: 27 October 2017 12:46
>> To: Jan Beulich <JBeulich@suse.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George
>> Dunlap <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org;
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Daniel De Graaf
>> <dgdegra@tycho.nsa.gov>; Tim (Xen.org) <tim@xen.org>
>> Subject: Re: [Xen-devel] [PATCH v12 05/11] x86/mm: add
>> HYPERVISOR_memory_op to acquire guest resources
>>
>> Hi,
>>
>> On 26/10/17 16:39, Jan Beulich wrote:
>>>>>> On 26.10.17 at 17:32, <julien.grall@linaro.org> wrote:
>>>> On 26/10/17 16:26, Jan Beulich wrote:
>>>>>>>> On 17.10.17 at 15:24, <paul.durrant@citrix.com> wrote:
>>>>>> +    /* IN/OUT - If the tools domain is PV then, upon return, frame_list
>>>>>> +     *          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;
>>>>>
>>>>> This is still xen_ulong_t, which I can live with, but then you shouldn't
>>>>> copy into / out of arrays of other types in acquire_resource() (the
>>>>> more that this is common code, and iirc xen_ulong_t and
>>>>> unsigned long aren't the same thing on ARM32).
>>>>
>>>> xen_ulong_t is always 64-bit on Arm (32-bit and 64-bit). But shouldn't
>>>> we use xen_pfn_t here?
>>>
>>> I had put this question up earlier, but iirc Paul didn't like it.
>>
>> I'd like to understand why Paul doesn't like it. We should never assume
>> that a frame fit in xen_ulong_t. xen_pfn_t was exactly introduced for
>> that purpose.
> 
> My reservation is whether xen_pfn_t is intended to hold either gfns or mfns, since this hypercall uses the same array for both. If it suitable then I am happy to change it, but Andrew led me to believe otherwise.

Looking at the public hearders, xen_pfn_t is been used for both MFN (see 
xenpf_add_memtype) and GFN (see gnttab_setup_table).

So I think it would be fine to do the same here.

Cheers,

-- 
Julien Grall

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