Re: [Xen-devel] [PATCH 2/2] x86/altp2m: allow specifying external-only use-case

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Tue, Aug 9, 2016 at 2:06 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 09.08.16 at 01:56, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> @@ -5238,18 +5238,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] )
>> +    {
>> +        rc = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d,
>> +                d->arch.hvm_domain.params[HVM_PARAM_ALTP2M])) )
>
> As per the 3rd of the non-commit message notes in
> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00865.html
> (subject to a still outstanding response from Daniel or others)
> I'd prefer if no new uses of XSM_OTHER got added.

The problem is that the existing xsm function does an assert on the
type of action, in this case XSM_TARGET. In the external only case we
want to do something else so we can't get pass the assert. We could
introduce a new xsm op just for the external check where the assert is
for another type of action, but that is ugly when XSM is enabled as we
would have two ops that do the same thing. So if we don't want to use
XSM_OTHER then we would have to get rid of the xsm assert so the op
can be used with different actions.

>
>> --- a/xen/include/public/hvm/params.h
>> +++ b/xen/include/public/hvm/params.h
>> @@ -225,8 +225,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
>> + */
>>  #define HVM_PARAM_ALTP2M       35
>> +#define HVMALTP2M_disabled      0
>> +#define HVMALTP2M_mixed         1
>> +#define HVMALTP2M_external_only 2
>
> No new classes / groups of identifiers in the public interface please
> which are in violation of the name spacing rules (read: should be
> prefixed by XEN_).

Ack.

>
> Apart from that - would a guest-only mode make sense, i.e. would
> making this individual flags rather than an enumeration like thing be
> better?

I don't see a reason for a guest-only mode. Also, I'm not sure if it's
possibly to do that with XSM disabled.

>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -554,10 +554,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 HVMALTP2M_mixed:
>> +        return xsm_default_action(XSM_TARGET, current->domain, d);
>> +    case HVMALTP2M_external_only:
>> +        return xsm_default_action(XSM_PRIV, current->domain, d);
>
> Does this really need to be PRIV instead of just DM_PRIV?

It could be either, I'm OK with relaxing it to DM_PRIV.

Thanks,
Tamas

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