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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.17 at 12:43, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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.

Sure.

Jan

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