Re: [PATCH 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.08.14 at 17:21, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5533,8 +5533,20 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                      rc = -EINVAL;
>                  break;
>              case HVM_PARAM_VIRIDIAN:
> -                if ( a.value > 1 )
> -                    rc = -EINVAL;
> +                /* This should only ever be set once by the tools and read by the guest. */
> +                rc = -EPERM;
> +                if ( curr_d == d )
> +                    break;
> +
> +                rc = -EPERM;
> +                if ( d->arch.hvm_domain.params[a.index] )
> +                    break;

Wouldn't it make sense to not fail a (redundant) attempt to set the
value to what it already is?

> +                

Line with only spaces.

> +                rc = -EINVAL;
> +                if ( a.value & ~HVMPV_feature_mask )
> +                    break;

Shouldn't you better also require any non-zero value having
HVMPV_base_freq set?

> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
>          /* Which hypervisor MSRs are available to the guest */
>          *eax = (CPUID3A_MSR_APIC_ACCESS |
>                  CPUID3A_MSR_HYPERCALL   |
> -                CPUID3A_MSR_VP_INDEX    |
> -                CPUID3A_MSR_FREQ);
> +                CPUID3A_MSR_VP_INDEX);
> +        if ( ~viridian_feature_mask(d) & HVMPV_no_freq )

Could I talk you into using the more conventional

        if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )

here?

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -344,8 +344,11 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
>      return hvm_funcs.get_shadow_gs_base(v);
>  }
>  
> -#define is_viridian_domain(_d)                                             \
> - (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
> +#define viridian_feature_mask(_d) \
> +    ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> +
> +#define is_viridian_domain(_d) \
> +    (is_hvm_domain(_d) && (viridian_feature_mask(_d) & HVMPV_base_freq))

Since you're re-writing this anyway, can you please drop the bogus
leading underscores on both macros' parameter?

Jan