Re: [Xen-devel] [PATCH 1/7] x86/viridian: update to version 5.0a of the specification

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 20 March 2017 11:27
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the
> specification
> 
> >>> On 17.03.17 at 10:57, <paul.durrant@xxxxxxxxxx> wrote:
> > @@ -48,20 +48,60 @@
> >  /* Viridian Hypercall Flags. */
> >  #define HV_FLUSH_ALL_PROCESSORS 1
> >
> > -/* Viridian CPUID 4000003, Viridian MSR availability. */
> > -#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1)
> > -#define CPUID3A_MSR_APIC_ACCESS    (1 << 4)
> > -#define CPUID3A_MSR_HYPERCALL      (1 << 5)
> > -#define CPUID3A_MSR_VP_INDEX       (1 << 6)
> > -#define CPUID3A_MSR_REFERENCE_TSC  (1 << 9)
> > -#define CPUID3A_MSR_FREQ           (1 << 11)
> > -
> > -/* Viridian CPUID 4000004, Implementation Recommendations. */
> > +/*
> > + * Viridian Partition Privilege Flags.
> > + *
> > + * This is taken from section 4.2.2 of the specification, and fixed for
> > + * style and correctness.
> > + */
> > +typedef struct {
> > +    /* Access to virtual MSRs */
> > +    uint64_t AccessVpRunTimeReg:1;
> > +    uint64_t AccessPartitionReferenceCounter:1;
> > +    uint64_t AccessSynicRegs:1;
> > +    uint64_t AccessSyntheticTimerRegs:1;
> > +    uint64_t AccessIntrCtrlRegs:1;
> > +    uint64_t AccessHypercallMsrs:1;
> > +    uint64_t AccessVpIndex:1;
> > +    uint64_t AccessResetReg:1;
> > +    uint64_t AccessStatsReg:1;
> > +    uint64_t AccessPartitionReferenceTsc:1;
> > +    uint64_t AccessGuestIdleReg:1;
> > +    uint64_t AccessFrequencyRegs:1;
> > +    uint64_t AccessDebugRegs:1;
> > +    uint64_t Reserved1:19;
> > +
> > +    /* Access to hypercalls */
> > +    uint64_t CreatePartitions:1;
> > +    uint64_t AccessPartitionId:1;
> > +    uint64_t AccessMemoryPool:1;
> > +    uint64_t AdjustMessageBuffers:1;
> > +    uint64_t PostMessages:1;
> > +    uint64_t SignalEvents:1;
> > +    uint64_t CreatePort:1;
> > +    uint64_t ConnectPort:1;
> > +    uint64_t AccessStats:1;
> > +    uint64_t Reserved2:2;
> > +    uint64_t Debugging:1;
> > +    uint64_t CpuManagement:1;
> > +    uint64_t Reserved3:1;
> > +    uint64_t Reserved4:1;
> > +    uint64_t Reserved5:1;
> > +    uint64_t AccessVSM:1;
> > +    uint64_t AccessVpRegisters:1;
> > +    uint64_t Reserved6:1;
> > +    uint64_t Reserved7:1;
> > +    uint64_t EnableExtendedHypercalls:1;
> > +    uint64_t StartVirtualProcessor:1;
> > +    uint64_t Reserved8:10;
> > +} HV_PARTITION_PRIVILEGE_MASK;
> 
> I can see the use of uint64_t here matching the spec's UINT64, but
> I don't see why we need a 64-bit (or even fixed width) type here.
> Personally I'd also prefer if reserved entries in bit fields were simply
> left unnamed.

I'm trying not deviate from the spec too much. I resisted typdef-ing UINT64 but 
the spec does use that type and it does (albeit incorrectly in some cases ) 
name its reserved fields, so I'd rather leave this as-is.

> 
> > @@ -101,17 +141,35 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> uint32_t leaf,
> >          break;
> >
> >      case 3:
> > -        /* Which hypervisor MSRs are available to the guest */
> > -        res->a = (CPUID3A_MSR_APIC_ACCESS |
> > -                  CPUID3A_MSR_HYPERCALL   |
> > -                  CPUID3A_MSR_VP_INDEX);
> > +    {
> > +        /*
> > +         * Section 2.4.4 details this leaf and states that EAX and EBX
> > +         * are defined to the the low and high parts of the partition
> 
> ... to be the ...

Oops, yes.

> 
> > +         * privilege mask respectively.
> > +         */
> > +        HV_PARTITION_PRIVILEGE_MASK mask = {
> > +            .AccessIntrCtrlRegs = 1,
> > +            .AccessHypercallMsrs = 1,
> > +            .AccessVpIndex = 1,
> > +        };
> > +        union {
> > +            HV_PARTITION_PRIVILEGE_MASK mask;
> > +            uint32_t lo, hi;
> > +        } u;
> > +
> >          if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> > -            res->a |= CPUID3A_MSR_FREQ;
> > +            mask.AccessFrequencyRegs = 1;
> >          if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
> > -            res->a |= CPUID3A_MSR_TIME_REF_COUNT;
> > +            mask.AccessPartitionReferenceCounter = 1;
> >          if ( viridian_feature_mask(d) & HVMPV_reference_tsc )
> > -            res->a |= CPUID3A_MSR_REFERENCE_TSC;
> > +            mask.AccessPartitionReferenceTsc = 1;
> > +
> > +        u.mask = mask;
> > +
> > +        res->a = u.lo;
> > +        res->b = u.hi;
> >          break;
> > +    }
> 
> I think the code would be more clear without the "mask" helper
> variable. But you're the maintainer ... (But please let me know
> whether you want to do a v2 or rather have this committed as
> is.)
> 
> Other than these minor items
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 

Thanks. Could you fix up that typo and commit.

  Paul

> Jan

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