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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.17 at 15:26, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 09 October 2017 14:06
>> >>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> wrote:
>> > --- a/xen/common/memory.c
>> > +++ b/xen/common/memory.c
>> > @@ -965,6 +965,67 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>> >      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>> >  }
>> >
>> > +#ifdef CONFIG_X86
>> 
>> Could you try to (a) have only a single such #ifdef and (b) reduce
>> its scope as much as possible? Even if for now the code is dead on
>> ARM, making sure it continues to compile there would be helpful.
>> 
> 
> Ok. I don't have an arm machine to build on so I can't tell if anything is 
> broken though.

Well, that's the case for most of us x86 people. I use a cross
compiler to be able to at least check the ARM build is okay.

>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -599,6 +599,36 @@ struct xen_reserved_device_memory_map {
>> >  typedef struct xen_reserved_device_memory_map
>> xen_reserved_device_memory_map_t;
>> >  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_map_t);
>> >
>> > +/*
>> > + * Get the pages for a particular guest resource, so that they can be
>> > + * mapped directly by a tools domain.
>> > + */
>> > +#define XENMEM_acquire_resource 28
>> > +struct xen_mem_acquire_resource {
>> > +    /* IN - the domain whose resource is to be mapped */
>> > +    domid_t domid;
>> > +    /* IN - the type of resource */
>> > +    uint16_t type;
>> > +    /*
>> > +     * IN - a type-specific resource identifier, which must be zero
>> > +     *      unless stated otherwise.
>> > +     */
>> > +    uint32_t id;
>> > +    /* IN - number of (4K) frames of the resource to be mapped */
>> > +    uint32_t nr_frames;
>> > +    uint32_t pad;
>> > +    /* IN - the index of the initial frame to be mapped */
>> > +    uint64_aligned_t frame;
>> 
>> Does this really need to be 64 bits wide?
> 
> I'd prefer not to limit to 32 bits just in case we want to use this 
> hypercall to map something with a large frame space.

In which case you'll need to take care of the truncation issues I've
pointed out elsewhere.

>> > +    /* IN/OUT - If the tools domain is PV then, upon return, gmfn_list
>> > +     *          will be populated with the MFNs of the resource.
>> > +     *          If the tools domain is HVM then it is expected that, on
>> > +     *          entry, gmfn_list will be populated with a list of GFNs
>> > +     *          that will be mapped to the MFNs of the resource.
>> > +     */
>> > +    XEN_GUEST_HANDLE(xen_ulong_t) gmfn_list;
>> 
>> Why not xen_pfn_t?
> 
> I've debated this with myself a few times. I'm not convinced that something 
> that could be an mfn or a gfn should have a xen_pfn_t type.

Well, looking over the present uses of xen_pfn_t it looks like quite
a few are for mixed meaning uses between PV and HVM. And btw,
since we try to get rid of the term "GMFN", perhaps "frame_list"
might be a better field name here.

Jan


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