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

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

2017-03-20 Thread Jan Beulich

>>> On 20.03.17 at 12:43,  wrote:
>>  -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 20 March 2017 11:27
>> To: Paul Durrant 
>> Cc: Andrew Cooper ; Wei Liu
>> ; Ian Jackson ; xen-
>> de...@lists.xenproject.org 
>> Subject: Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the
>> specification
>> 
>> >>> On 17.03.17 at 10:57,  wrote:
>> > @@ -48,20 +48,60 @@
>> >  /* Viridian Hypercall Flags. */
>> >  #define HV_FLUSH_ALL_PROCESSORS 1
>> >
>> > -/* Viridian CPUID 403, 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 404, 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 
>> 
>

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

2017-03-20 Thread Paul Durrant

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 20 March 2017 11:27
> To: Paul Durrant 
> Cc: Andrew Cooper ; Wei Liu
> ; Ian Jackson ; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the
> specification
> 
> >>> On 17.03.17 at 10:57,  wrote:
> > @@ -48,20 +48,60 @@
> >  /* Viridian Hypercall Flags. */
> >  #define HV_FLUSH_ALL_PROCESSORS 1
> >
> > -/* Viridian CPUID 403, 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 404, 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 
> 

Thanks. Could you fix up that typo and commit.

  Paul

> Jan

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

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

2017-03-20 Thread Jan Beulich

>>> On 17.03.17 at 10:57,  wrote:
> @@ -48,20 +48,60 @@
>  /* Viridian Hypercall Flags. */
>  #define HV_FLUSH_ALL_PROCESSORS 1
>  
> -/* Viridian CPUID 403, 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 404, 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 

Jan

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

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information