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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 01 August 2014 16:41
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx;
> Keir (Xen.org)
> Subject: Re: [PATCH 1/2] x86/viridian: Re-purpose the HVM parameter to be
> a feature mask
> 
> >>> On 01.08.14 at 17:21, <paul.durrant@xxxxxxxxxx> 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?
> 

Yes, I guess that would be ok.

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

Yes I should. Thanks for spotting that.

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

Sure.

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

Yes I'll do that.

  Paul

> Jan


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