|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources
>>> On 12.10.17 at 18:25, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -402,14 +469,56 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> rc = do_memory_op(cmd, nat.hnd);
> if ( rc < 0 )
> {
> - if ( rc == -ENOBUFS && op == XENMEM_get_vnumainfo )
> + switch ( op)
Missing blank.
> {
> - cmp.vnuma.nr_vnodes = nat.vnuma->nr_vnodes;
> - cmp.vnuma.nr_vcpus = nat.vnuma->nr_vcpus;
> - cmp.vnuma.nr_vmemranges = nat.vnuma->nr_vmemranges;
> - if ( __copy_to_guest(compat, &cmp.vnuma, 1) )
> - rc = -EFAULT;
> + case XENMEM_get_vnumainfo:
> + if ( rc == -ENOBUFS )
> + {
> + cmp.vnuma.nr_vnodes = nat.vnuma->nr_vnodes;
> + cmp.vnuma.nr_vcpus = nat.vnuma->nr_vcpus;
> + cmp.vnuma.nr_vmemranges = nat.vnuma->nr_vmemranges;
> + if ( __copy_to_guest(compat, &cmp.vnuma, 1) )
> + rc = -EFAULT;
> + }
> +
> + break;
> +
> + case XENMEM_acquire_resource:
> + {
> + xen_ulong_t *xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
const
> + if ( rc == -EINVAL && xen_frame_list[0] != 0 )
I think this will go wrong if you get -EINVAL for other than the
specific reason you consider here, in particular when caller
passed in a valid array. You'd need to also check for
cmp.mar.nr_frames being zero. But see also below.
> + {
> + /*
> + * The value of nr_frames passed to the implementation
> + * was not the value passed by the caller, it was
> + * overridden.
> + * The value in xen_frame_list[0] is the maximum
> + * number of frames that can be bounced so we need
> + * to set cmp.nr_frames to the minimum of this and
> + * the maximum number of frames allowed by the
> + * implementation before passing back to the caller.
> + */
> + cmp.mar.nr_frames = min_t(unsigned int,
> + xen_frame_list[0],
> + nat.mar->nr_frames);
> + rc = -E2BIG;
> + }
> +
> + /* In either of these cases nr_frames is an OUT value */
> + if ( rc == -EINVAL || rc == -E2BIG )
> + {
> + if ( copy_to_guest(compat, &cmp.mar, 1) )
> + rc = -EFAULT;
The two if()s should be combined. Also - __copy_field_to_guest()?
> + }
> +
> + break;
> + }
> + default:
> + break;
No real need for a default label. Yet if you want to keep it, please
have a blank line ahead of it.
> @@ -535,6 +644,30 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
> rc = -EFAULT;
> break;
>
> + case XENMEM_acquire_resource:
> + {
> + xen_ulong_t *xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
const
> + compat_ulong_t *compat_frame_list =
> + (compat_ulong_t *)(nat.mar + 1);
> +
> + /* NOTE: the compat array overwrites the native array */
Perhaps "the smaller compat array ..."?
> + for ( i = 0; i < cmp.mar.nr_frames; i++ )
> + {
> + compat_ulong_t frame = xen_frame_list[i];
> +
> + if ( frame != xen_frame_list[i] )
> + return -ERANGE;
> +
> + compat_frame_list[i] = frame;
> + }
> +
> + if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
> + compat_frame_list,
> + cmp.mar.nr_frames) )
> + return -EFAULT;
> +
> + break;
> + }
> default:
Again missing a blank line above here.
> --- 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.
> + xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +
> + if ( copy_to_guest(arg, &xmar, 1) )
> + return -EFAULT;
> +
> + return rc;
> + }
> +
> + d = rcu_lock_domain_by_any_id(xmar.domid);
> + if ( d == NULL )
> + return -ESRCH;
> +
> + rc = xsm_domain_resource_map(XSM_DM_PRIV, d);
> + if ( rc )
> + goto out;
> +
> + switch ( xmar.type )
> + {
> + default:
> + rc = -EOPNOTSUPP;
> + break;
> + }
> +
> + 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,
> + ARRAY_SIZE(gfn_list)) )
You shouldn't copy more than xmar.nr_frames here, or else you
risk running past a page boundary and perhaps into a non-
present page. You consume ...
> + goto out;
> +
> + for ( i = 0; i < xmar.nr_frames; i++ )
... exactly this many frames anyway.
> + {
> + 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?
> @@ -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.
> --- 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.
> + * 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.
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -724,3 +724,9 @@ static XSM_INLINE int xsm_xen_version (XSM_DEFAULT_ARG
> uint32_t op)
> return xsm_default_action(XSM_PRIV, current->domain, NULL);
> }
> }
> +
> +static XSM_INLINE int xsm_domain_resource_map(XSM_DEFAULT_ARG struct domain
> *d)
> +{
> + XSM_ASSERT_ACTION(XSM_DM_PRIV);
> + return xsm_default_action(action, current->domain, d);
> +}
Perhaps better place this near something similar/related (also for
some of the other additions further down)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |