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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.10.17 at 16:07, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 16 October 2017 14:53
>> >>> On 12.10.17 at 18:25, <paul.durrant@xxxxxxxxxx> wrote:
>> > --- a/xen/common/memory.c
>> > +++ b/xen/common/memory.c
>> > @@ -965,6 +965,88 @@ static long xatp_permission_check(struct domain
>> *d, unsigned int space)
>> >      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>> >  }
>> >
>> > +static int acquire_resource(XEN_GUEST_HANDLE_PARAM(void) arg)
>> > +{
>> > +    struct domain *d, *currd = current->domain;
>> > +    xen_mem_acquire_resource_t xmar;
>> > +    unsigned long mfn_list[2];
>> > +    int rc;
>> > +
>> > +    if ( copy_from_guest(&xmar, arg, 1) )
>> > +        return -EFAULT;
>> > +
>> > +    if ( xmar.pad != 0 )
>> > +        return -EINVAL;
>> > +
>> > +    if ( xmar.nr_frames == 0 ||
>> > +         xmar.nr_frames > ARRAY_SIZE(mfn_list) )
>> > +    {
>> > +        rc = xmar.nr_frames == 0 ? -EINVAL : -E2BIG;
>> 
>> Querying the implementation limit should be possible without
>> receiving an error. Hence my original suggestion to key this
>> off of the handle being a null one (in which case non-zero
>> nr_frames would indeed be -EINVAL), which afaics would also
>> simplify some of the compat handling.
> 
> Ok, FAOD, do you mean that passing in nr_frames and a NULL handle should not 
> yield an error but should pass back the implementation limit of nr_frames? 

If you mean "passing in zero nr_frames and a null handle", then
yes. Non-zero nr_frames and a null handle, as said, could certainly
be -EINVAL (you could as well try to read/write from/to that handle,
but I think Andrew wouldn't like that).

>> > +        {
>> > +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
>> > +                                       _mfn(mfn_list[i]));
>> > +            if ( rc )
>> > +            {
>> > +                while ( i-- != 0 )
>> > +                {
>> > +                    int ignore;
>> > +
>> > +                    ignore = guest_physmap_remove_page(
>> > +                        currd, _gfn(gfn_list[i]), _mfn(mfn_list[i]), 0);
>> 
>> Why would an error here be plain ignored?
>> 
> 
> What could I usefully do with it? Should I just crash the domain at this 
> point, since I can't restore a consistent state?

Not being silent is the most important aspect. Crashing the domain
is one approach. Reporting the error (and documenting the possibly
resulting inconsistent state) is another (and a sub-option thereof is
to return the number of failed entries, perhaps allowing the caller
some way to recover). Plus note that if the error happens on the
first iteration, no inconsistency would result.

>> > @@ -1406,6 +1488,14 @@ long do_memory_op(unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> >      }
>> >  #endif
>> >
>> > +    case XENMEM_acquire_resource:
>> > +#ifdef CONFIG_X86
>> > +        rc = acquire_resource(arg);
>> > +#else
>> > +        rc = -EOPNOTSUPP;
>> > +#endif
>> 
>> I think this will cause an "unused static function" warning on ARM.
> 
> ...which is why I originally had the function wrapped in the #ifdef as well. 
> What do you want me to do?

As said before - I'd like to see the #ifdef placed inside the function
around the smallest possible range of code that still allows ARM to
build (with the alternative of introducing a dummy stub or two on
ARM to avoid #ifdef-s altogether).

>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -599,6 +599,47 @@ 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/OUT - As an IN parameter number of (4K) frames of the resource
>> 
>> Please don't say 4k here - this not being an x86-specific interface
>> other system page sizes ought to be permitted.
> 
> I was under the impression that resources such as grant tables were only 
> every mapped in 4k chunks. Perhaps the 4k should type-specific? It needs to 
> be 
> specified somewhere.

I don't think there's an inherent limit to granted pages being larger
than 4k; v2 sub-page grants limit things to less than 64k though.
There's no single mention of "4k" throughout the public grant_table.h
or the implementation in grant_table.c.

>> > +     *          to be mapped. However, if the specified value is 0 then
>> > +     *          -EINVAL will be returned and this field will be set to the
>> > +     *          maximum value supported by the implementation. Also,
>> > +     *          if the specified value exceeds the implementaton limit
>> > +     *          then -E2BIG will be returned and, similarly, this field
>> > +     *          will be set the maximum value supported by the
>> > +     *          implementation.
>> > +     */
>> > +    uint32_t nr_frames;
>> > +    uint32_t pad;
>> > +    /* IN - the index of the initial frame to be mapped. This parameter
>> > +     *      is optional if nr_frames is 0.
>> > +     */
>> > +    uint64_aligned_t frame;
>> > +    /* 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.
>> > +     *          This parameter is optional if nr_frames is 0.
>> > +     */
>> 
>> For both of these comments - s/optional/ignored/? And this, afaics,
>> also applies to domid, type, and id, so perhaps better state once
>> in the comment to (currently) nr_frames that all other fields except
>> for pad will be ignored.
> 
> No, I think it's reasonable for domid, type and id to always be valid even 
> if nr_frames is zero.

Okay, I can see why, but then you also need to check all of them.

Jan

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