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

2017-10-17 Thread Paul Durrant 
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 17 October 2017 13:53
> To: Paul Durrant 
> Cc: Andrew Cooper ; George Dunlap
> ; Ian Jackson ; Wei Liu
> ; Stefano Stabellini ; xen-
> de...@lists.xenproject.org; KonradRzeszutek Wilk
> ; Daniel de Graaf ; Tim
> (Xen.org) 
> Subject: RE: [PATCH v11 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 17.10.17 at 14:28,  wrote:
> >>  -Original Message-
> >>
> >> > --- 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)?
> >
> > Looking at this again it seems that various related things, e.g.
> > domain_memory_map, are x86 only so adding at the end seems like the
> best
> > thing to do.
> 
> Well, okay then (unless Daniel, whom it looks like you forgot to Cc,
> has a better suggestion).
> 

Yes, I realised that I forgot to cc him in since the code was added. He's on 
the v12 list.

  Paul

> Jan


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

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

2017-10-17 Thread Jan Beulich 
>>> On 17.10.17 at 14:28,  wrote:
>>  -Original Message-
>> 
>> > --- 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)?
> 
> Looking at this again it seems that various related things, e.g. 
> domain_memory_map, are x86 only so adding at the end seems like the best 
> thing to do.

Well, okay then (unless Daniel, whom it looks like you forgot to Cc,
has a better suggestion).

Jan


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

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

2017-10-17 Thread Paul Durrant 
> -Original Message-
> 
> > --- 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)?

Looking at this again it seems that various related things, e.g. 
domain_memory_map, are x86 only so adding at the end seems like the best thing 
to do.

  Paul

> 
> Jan

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

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

2017-10-16 Thread Jan Beulich 
>>> On 16.10.17 at 16:07,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 16 October 2017 14:53
>> >>> On 12.10.17 at 18:25,  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(, 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
>>

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

2017-10-16 Thread Paul Durrant 
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 16 October 2017 14:53
> To: Paul Durrant 
> Cc: Andrew Cooper ; Wei Liu
> ; George Dunlap ; Ian
> Jackson ; Stefano Stabellini
> ; xen-de...@lists.xenproject.org; Konrad Rzeszutek
> Wilk ; Tim (Xen.org) 
> Subject: Re: [PATCH v11 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 12.10.17 at 18:25,  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.

Oh yes.

> 
> >  {
> > -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, , 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, , 1) )
> > +rc = -EFAULT;
> > +}
> > +
> > +break;
> > +
> > +case XENMEM_acquire_resource:
> > +{
> > +xen_ulong_t *xen_frame_list = (xen_ulong_t *)(nat.mar + 1);
> 
> const

Ok.

> 
> > +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, , 1) )
> > +rc = -EFAULT;
> 
> The two if()s should be combined. Also - __copy_field_to_guest()?

Yes, maybe that would neater.

> 
> > +}
> > +
> > +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.
> 

Ok.

> > @@ -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

Ok.

> 
> > +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 ..."?

Ok.

> 
> > +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

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

2017-10-16 Thread Jan Beulich 
>>> On 12.10.17 at 18:25,  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, , 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, , 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, , 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(, 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