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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> @@ -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 ...

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

Jan

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