[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/7] x86/viridian: update to version 5.0a of the specification
> -----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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |