Re: [Xen-devel] [PATCH v4] altp2m: Allow specifying external-only use-case

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.17 at 20:27, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

I'll need to make this conditional upon a few more adjustments:

> @@ -4370,18 +4370,19 @@ static int do_altp2m_op(
>          goto out;
>      }
>  
> -    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_TARGET, d)) )
> +    if ( !d->arch.hvm_domain.params[HVM_PARAM_ALTP2M] )

I think it would be better for readers if you compared against
XEN_ALTP2M_disabled here.

> +    {
> +        rc = -EINVAL;
> +        goto out;
> +    }
> +
> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
> +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )

Note the implicit truncation here. Not a problem at present, but
at the very least I'd like to ask for the function parameter to
become unsigned int.

Furthermore, wasn't HVMOP_altp2m_vcpu_enable_notify
supposed to always be available to the guest (as long as altp2m
is enabled)? You don't allow this here anymore.

> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -228,8 +228,16 @@
>  /* Location of the VM Generation ID in guest physical address space. */
>  #define HVM_PARAM_VM_GENERATION_ID_ADDR 34
>  
> -/* Boolean: Enable altp2m */
> +/*
> + * Set mode for altp2m:
> + *  disabled: don't activate altp2m (default)
> + *  mixed: allow access to altp2m for both in-guest and external tools
> + *  external_only: allow access to external privileged tools only

This needs to be more precise: VMFUNC is an access mechanism too,
and aiui this isn't meant to be disabled by external_only.

> + */
>  #define HVM_PARAM_ALTP2M       35
> +#define XEN_ALTP2M_disabled      0
> +#define XEN_ALTP2M_mixed         1
> +#define XEN_ALTP2M_external_only 2

I'd drop the _only suffix.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -555,10 +555,18 @@ static XSM_INLINE int 
> xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
>      return xsm_default_action(action, current->domain, d);
>  }
>  
> -static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d)
> +static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, 
> int mode)
>  {
> -    XSM_ASSERT_ACTION(XSM_TARGET);
> -    return xsm_default_action(action, current->domain, d);
> +    XSM_ASSERT_ACTION(XSM_OTHER);
> +    switch ( mode )
> +    {
> +    case XEN_ALTP2M_mixed:
> +        return xsm_default_action(XSM_TARGET, current->domain, d);
> +    case XEN_ALTP2M_external_only:
> +        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
> +    default:
> +        return -EPERM;

I think -EPERM is correct at most for XEN_ALTP2M_disabled, all
others should get -EINVAL or -EOPNOTSUPP or some such. Perhaps
that also doesn't really belong here, but rather into the caller (which
right now produces -EINVAL for XEN_ALTP2M_disabled only).

Jan


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