[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
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |