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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> Sent: 27 October 2017 12:46
> To: Jan Beulich <JBeulich@xxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> 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@xxxxxxxxxx> wrote:
> >> On 26/10/17 16:26, Jan Beulich wrote:
> >>>>>> On 17.10.17 at 15:24, <paul.durrant@xxxxxxxxxx> 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.

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel