Re: [Xen-devel] [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

2017-01-20 Thread Andrew Cooper 
On 20/01/17 15:58, Jan Beulich wrote:
 On 18.01.17 at 20:40,  wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
>>  }
>>  }
>>  
>> +static void recalculate_common(struct cpuid_policy *p)
>> +{
>> +switch ( p->x86_vendor )
>> +{
>> +case X86_VENDOR_INTEL:
>> +p->extd.vendor_ebx = 0;
>> +p->extd.vendor_ecx = 0;
>> +p->extd.vendor_edx = 0;
>> +break;
>> +
>> +case X86_VENDOR_AMD:
>> +p->extd.vendor_ebx = p->basic.vendor_ebx;
>> +p->extd.vendor_ecx = p->basic.vendor_ecx;
>> +p->extd.vendor_edx = p->basic.vendor_edx;
>> +break;
>> +}
>> +}
> I find the word "common" in the name here not very indicative
> of what the function does, especially with ...
>
>> @@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
>>  min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>>  p->feat.max_subleaf =
>>  min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
>> -p->extd.max_leaf =
>> -min_t(uint32_t, p->extd.max_leaf,
>> -  0x8000u + ARRAY_SIZE(p->extd.raw) - 1);
>> +p->extd.max_leaf = 0x8000 | min_t(uint32_t, p->extd.max_leaf & 
>> 0x,
>> +  ARRAY_SIZE(p->extd.raw) - 1);
>>  
>>  cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
>>  recalculate_xstate(p);
>> +recalculate_common(p);
>>  }
> ... the neighboring call here (which is quite a bit more specific).
> Of course possible alternatives depend on what further uses of
> this function you do (or do not) plan, but by the name of the
> other function here it could be recalculate_extd_vendor().

It adjustments common to the global calculations, and the per-domain
recalculations.

It isn't however just per-vendor adjustments; there are global
adjustments (like clobbering the reserved leaves).

I am open to any naming suggestions.

>
>> @@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>>  return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>>  
>>  case 0x8000 ... 0x8000 + CPUID_GUEST_NR_EXTD - 1:
>> -if ( leaf > p->extd.max_leaf )
>> +ASSERT((p->extd.max_leaf & 0x) < ARRAY_SIZE(p->extd.raw));
>> +if ( (leaf & 0x) > min_t(uint32_t, p->extd.max_leaf & 0x,
>> + ARRAY_SIZE(p->extd.raw) - 1) )
>>  return;
>> -goto legacy;
>> +
>> +switch ( leaf )
>> +{
>> +default:
>> +goto legacy;
>> +
>> +case 0x8000:
>> +*res = p->extd.raw[leaf & 0x];
> I take it that the plan is to have further leaves and up here, or else
> the array index could simply be literal zero.

I believe you found your answer in the following patch?  Eventually
(when the legacy path disappears), this entire switch statement will go.

~Andrew

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

Re: [Xen-devel] [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

2017-01-20 Thread Jan Beulich 
>>> On 18.01.17 at 20:40,  wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
>  }
>  }
>  
> +static void recalculate_common(struct cpuid_policy *p)
> +{
> +switch ( p->x86_vendor )
> +{
> +case X86_VENDOR_INTEL:
> +p->extd.vendor_ebx = 0;
> +p->extd.vendor_ecx = 0;
> +p->extd.vendor_edx = 0;
> +break;
> +
> +case X86_VENDOR_AMD:
> +p->extd.vendor_ebx = p->basic.vendor_ebx;
> +p->extd.vendor_ecx = p->basic.vendor_ecx;
> +p->extd.vendor_edx = p->basic.vendor_edx;
> +break;
> +}
> +}

I find the word "common" in the name here not very indicative
of what the function does, especially with ...

> @@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
>  min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>  p->feat.max_subleaf =
>  min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
> -p->extd.max_leaf =
> -min_t(uint32_t, p->extd.max_leaf,
> -  0x8000u + ARRAY_SIZE(p->extd.raw) - 1);
> +p->extd.max_leaf = 0x8000 | min_t(uint32_t, p->extd.max_leaf & 
> 0x,
> +  ARRAY_SIZE(p->extd.raw) - 1);
>  
>  cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
>  recalculate_xstate(p);
> +recalculate_common(p);
>  }

... the neighboring call here (which is quite a bit more specific).
Of course possible alternatives depend on what further uses of
this function you do (or do not) plan, but by the name of the
other function here it could be recalculate_extd_vendor().

> @@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>  return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
>  
>  case 0x8000 ... 0x8000 + CPUID_GUEST_NR_EXTD - 1:
> -if ( leaf > p->extd.max_leaf )
> +ASSERT((p->extd.max_leaf & 0x) < ARRAY_SIZE(p->extd.raw));
> +if ( (leaf & 0x) > min_t(uint32_t, p->extd.max_leaf & 0x,
> + ARRAY_SIZE(p->extd.raw) - 1) )
>  return;
> -goto legacy;
> +
> +switch ( leaf )
> +{
> +default:
> +goto legacy;
> +
> +case 0x8000:
> +*res = p->extd.raw[leaf & 0x];

I take it that the plan is to have further leaves and up here, or else
the array index could simply be literal zero.

Jan


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

Re: [Xen-devel] [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

2017-01-18 Thread Doug Goldstein 
On 1/18/17 2:40 PM, Andrew Cooper wrote:
> The calculations for p->extd.max_leaf are reworked to force a value of at
> least 0x8000, and to take the domains chosen vendor into account when
> clamping maximum value.
> 
> The high short vendor information is clobbered or duplicated according to the
> chosen vendor.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Doug Goldstein 

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 4/6] x86/cpuid: Handle leaf 0x80000000 in guest_cpuid()

2017-01-18 Thread Andrew Cooper 
The calculations for p->extd.max_leaf are reworked to force a value of at
least 0x8000, and to take the domains chosen vendor into account when
clamping maximum value.

The high short vendor information is clobbered or duplicated according to the
chosen vendor.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
---
 xen/arch/x86/cpuid.c| 48 +++--
 xen/include/asm-x86/cpuid.h |  6 +++---
 2 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 85c829d..dc76cf4 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -163,6 +163,24 @@ static void recalculate_xstate(struct cpuid_policy *p)
 }
 }
 
+static void recalculate_common(struct cpuid_policy *p)
+{
+switch ( p->x86_vendor )
+{
+case X86_VENDOR_INTEL:
+p->extd.vendor_ebx = 0;
+p->extd.vendor_ecx = 0;
+p->extd.vendor_edx = 0;
+break;
+
+case X86_VENDOR_AMD:
+p->extd.vendor_ebx = p->basic.vendor_ebx;
+p->extd.vendor_ecx = p->basic.vendor_ecx;
+p->extd.vendor_edx = p->basic.vendor_edx;
+break;
+}
+}
+
 static void __init calculate_raw_policy(void)
 {
 struct cpuid_policy *p = _policy;
@@ -227,12 +245,12 @@ static void __init calculate_host_policy(void)
 min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
 p->feat.max_subleaf =
 min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
-p->extd.max_leaf =
-min_t(uint32_t, p->extd.max_leaf,
-  0x8000u + ARRAY_SIZE(p->extd.raw) - 1);
+p->extd.max_leaf = 0x8000 | min_t(uint32_t, p->extd.max_leaf & 0x,
+  ARRAY_SIZE(p->extd.raw) - 1);
 
 cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
 recalculate_xstate(p);
+recalculate_common(p);
 }
 
 static void __init calculate_pv_max_policy(void)
@@ -360,7 +378,10 @@ void recalculate_cpuid_policy(struct domain *d)
 
 p->basic.max_leaf   = min(p->basic.max_leaf,   max->basic.max_leaf);
 p->feat.max_subleaf = min(p->feat.max_subleaf, max->feat.max_subleaf);
-p->extd.max_leaf= min(p->extd.max_leaf,max->extd.max_leaf);
+p->extd.max_leaf= 0x8000 | min(p->extd.max_leaf & 0x,
+   (p->x86_vendor == X86_VENDOR_AMD
+? CPUID_GUEST_NR_EXTD_AMD
+: CPUID_GUEST_NR_EXTD_INTEL) - 1);
 
 cpuid_policy_to_featureset(p, fs);
 cpuid_policy_to_featureset(max, max_fs);
@@ -428,6 +449,7 @@ void recalculate_cpuid_policy(struct domain *d)
 
 cpuid_featureset_to_policy(fs, p);
 recalculate_xstate(p);
+recalculate_common(p);
 }
 
 int init_domain_cpuid_policy(struct domain *d)
@@ -683,6 +705,7 @@ static void pv_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 case 0x0:
 case 0x7:
 case XSTATE_CPUID:
+case 0x8000:
 ASSERT_UNREACHABLE();
 /* Now handled in guest_cpuid(). */
 }
@@ -832,6 +855,7 @@ static void hvm_cpuid(uint32_t leaf, uint32_t subleaf, 
struct cpuid_leaf *res)
 case 0x0:
 case 0x7:
 case XSTATE_CPUID:
+case 0x8000:
 ASSERT_UNREACHABLE();
 /* Now handled in guest_cpuid(). */
 }
@@ -901,9 +925,21 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
 return cpuid_hypervisor_leaves(v, leaf, subleaf, res);
 
 case 0x8000 ... 0x8000 + CPUID_GUEST_NR_EXTD - 1:
-if ( leaf > p->extd.max_leaf )
+ASSERT((p->extd.max_leaf & 0x) < ARRAY_SIZE(p->extd.raw));
+if ( (leaf & 0x) > min_t(uint32_t, p->extd.max_leaf & 0x,
+ ARRAY_SIZE(p->extd.raw) - 1) )
 return;
-goto legacy;
+
+switch ( leaf )
+{
+default:
+goto legacy;
+
+case 0x8000:
+*res = p->extd.raw[leaf & 0x];
+break;
+}
+break;
 
 default:
 return;
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 299a026..4712b73 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -81,7 +81,7 @@ struct cpuid_policy
  *   - All of the feat and xstate unions
  *   - max_{,sub}leaf
  *   - All FEATURESET_* words
- *   - Low short vendor infomation
+ *   - Short vendor infomation
  *
  * Per-domain objects:
  *
@@ -89,7 +89,7 @@ struct cpuid_policy
  *   - All of the feat and xstate unions
  *   - max_{,sub}leaf
  *   - All FEATURESET_* words
- *   - Low short vendor infomation
+ *   - Short vendor infomation
  *
  * Everything else should be considered inaccurate, and not necesserily 0.
  */
@@ -168,7 +168,7 @@ struct cpuid_policy
 struct cpuid_leaf