[08/10] x86/cpuid: Introduce and use default CPUID policies
diff mbox series

Message ID 20200226202221.6555-9-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86: Default vs Max policies
Related show

Commit Message

Andrew Cooper Feb. 26, 2020, 8:22 p.m. UTC
For now, the default and max policies remain identical, but this will change
in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
with simple feature differences for now.

Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
default policies.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpuid.c        | 55 +++++++++++++++++++++++++++++++++++++++++++--
 xen/arch/x86/sysctl.c       |  4 ++--
 xen/include/asm-x86/cpuid.h |  3 ++-
 3 files changed, 57 insertions(+), 5 deletions(-)

Comments

Jan Beulich Feb. 27, 2020, 8:19 a.m. UTC | #1
On 26.02.2020 21:22, Andrew Cooper wrote:
> For now, the default and max policies remain identical, but this will change
> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
> with simple feature differences for now.
> 
> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
> default policies.

For the sysctl the statement looks to be broader than reality,
as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.

> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>  }
>  
> +static void __init calculate_pv_def_policy(void)
> +{
> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
> +    uint32_t pv_featureset[FSCAPINTS];
> +    unsigned int i;
> +
> +    *p = pv_max_cpuid_policy;
> +    cpuid_policy_to_featureset(p, pv_featureset);
> +
> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
> +        pv_featureset[i] &= pv_def_featuremask[i];
> +
> +    sanitise_featureset(pv_featureset);
> +    cpuid_featureset_to_policy(pv_featureset, p);
> +    recalculate_xstate(p);
> +}

Is there a reason the call to guest_common_feature_adjustments()
is missing here? If so, I think you want to say a word on the why
in the description. If not, with it added
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper Feb. 27, 2020, 10:55 a.m. UTC | #2
On 27/02/2020 08:19, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> For now, the default and max policies remain identical, but this will change
>> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
>> with simple feature differences for now.
>>
>> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
>> default policies.
> For the sysctl the statement looks to be broader than reality,
> as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.

I'm afraid I don't understand what you mean here.  What would I need to
touch in XEN_SYSCTL_cpu_policy_*_max at all?

>> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>>  }
>>  
>> +static void __init calculate_pv_def_policy(void)
>> +{
>> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
>> +    uint32_t pv_featureset[FSCAPINTS];
>> +    unsigned int i;
>> +
>> +    *p = pv_max_cpuid_policy;
>> +    cpuid_policy_to_featureset(p, pv_featureset);
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
>> +        pv_featureset[i] &= pv_def_featuremask[i];
>> +
>> +    sanitise_featureset(pv_featureset);
>> +    cpuid_featureset_to_policy(pv_featureset, p);
>> +    recalculate_xstate(p);
>> +}
> Is there a reason the call to guest_common_feature_adjustments()
> is missing here?

Yes, for the same reason that other logic is dropped.  Inheriting from
pv_max_cpuid_policy means that it has already been run on this object.

The host to *_max derivation is non-trivial.  Some features get added
in, others are conditional on external factors.  The *_max to *_def
derivation is much more simple in comparison.

Long term, I expect this logic to move into libx86 and further simplify
cpuid.c

However, I'm not sure why guest_common_feature_adjustments() is special
compared to the other removed logic, and why it should be called out.

~Andrew
Jan Beulich Feb. 27, 2020, 11:29 a.m. UTC | #3
On 27.02.2020 11:55, Andrew Cooper wrote:
> On 27/02/2020 08:19, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> For now, the default and max policies remain identical, but this will change
>>> in the future.  Write calculate_{pv,hvm}_def_policy() in a way which will cope
>>> with simple feature differences for now.
>>>
>>> Update XEN_SYSCTL_get_cpu_policy and init_domain_cpuid_policy() to use the
>>> default policies.
>> For the sysctl the statement looks to be broader than reality,
>> as (of course) you don't touch XEN_SYSCTL_cpu_policy_*_max.
> 
> I'm afraid I don't understand what you mean here.  What would I need to
> touch in XEN_SYSCTL_cpu_policy_*_max at all?

Nothing, and hence my "too broad" response. Only part of
XEN_SYSCTL_get_cpu_policy gets updated. But yes, thinking about it
again, I think I see now how you mean this. So never mind.

>>> @@ -381,6 +386,23 @@ static void __init calculate_pv_max_policy(void)
>>>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>>>  }
>>>  
>>> +static void __init calculate_pv_def_policy(void)
>>> +{
>>> +    struct cpuid_policy *p = &pv_def_cpuid_policy;
>>> +    uint32_t pv_featureset[FSCAPINTS];
>>> +    unsigned int i;
>>> +
>>> +    *p = pv_max_cpuid_policy;
>>> +    cpuid_policy_to_featureset(p, pv_featureset);
>>> +
>>> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
>>> +        pv_featureset[i] &= pv_def_featuremask[i];
>>> +
>>> +    sanitise_featureset(pv_featureset);
>>> +    cpuid_featureset_to_policy(pv_featureset, p);
>>> +    recalculate_xstate(p);
>>> +}
>> Is there a reason the call to guest_common_feature_adjustments()
>> is missing here?
> 
> Yes, for the same reason that other logic is dropped.  Inheriting from
> pv_max_cpuid_policy means that it has already been run on this object.
> 
> The host to *_max derivation is non-trivial.  Some features get added
> in, others are conditional on external factors.  The *_max to *_def
> derivation is much more simple in comparison.
> 
> Long term, I expect this logic to move into libx86 and further simplify
> cpuid.c
> 
> However, I'm not sure why guest_common_feature_adjustments() is special
> compared to the other removed logic, and why it should be called out.

Well, the oddity isn't with removed logic (and in fact in this patch I
can't see much of a removal of anything), but with the call being there
in calculate_hvm_def_policy(). This difference, if intentional, is what
I think needs calling out.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index cd9a02143c..6e01394fd2 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -18,6 +18,9 @@  const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
 static const uint32_t pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
 static const uint32_t hvm_shadow_max_featuremask[] = INIT_HVM_SHADOW_MAX_FEATURES;
 static const uint32_t hvm_hap_max_featuremask[] = INIT_HVM_HAP_MAX_FEATURES;
+static const uint32_t pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
+static const uint32_t hvm_shadow_def_featuremask[] = INIT_HVM_SHADOW_DEF_FEATURES;
+static const uint32_t hvm_hap_def_featuremask[] = INIT_HVM_HAP_DEF_FEATURES;
 static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 
 static int __init parse_xen_cpuid(const char *s)
@@ -99,9 +102,11 @@  struct cpuid_policy __read_mostly     raw_cpuid_policy,
                     __read_mostly    host_cpuid_policy;
 #ifdef CONFIG_PV
 struct cpuid_policy __read_mostly  pv_max_cpuid_policy;
+struct cpuid_policy __read_mostly  pv_def_cpuid_policy;
 #endif
 #ifdef CONFIG_HVM
 struct cpuid_policy __read_mostly hvm_max_cpuid_policy;
+struct cpuid_policy __read_mostly hvm_def_cpuid_policy;
 #endif
 
 static void sanitise_featureset(uint32_t *fs)
@@ -381,6 +386,23 @@  static void __init calculate_pv_max_policy(void)
     p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
 }
 
+static void __init calculate_pv_def_policy(void)
+{
+    struct cpuid_policy *p = &pv_def_cpuid_policy;
+    uint32_t pv_featureset[FSCAPINTS];
+    unsigned int i;
+
+    *p = pv_max_cpuid_policy;
+    cpuid_policy_to_featureset(p, pv_featureset);
+
+    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
+        pv_featureset[i] &= pv_def_featuremask[i];
+
+    sanitise_featureset(pv_featureset);
+    cpuid_featureset_to_policy(pv_featureset, p);
+    recalculate_xstate(p);
+}
+
 static void __init calculate_hvm_max_policy(void)
 {
     struct cpuid_policy *p = &hvm_max_cpuid_policy;
@@ -440,16 +462,45 @@  static void __init calculate_hvm_max_policy(void)
     recalculate_xstate(p);
 }
 
+static void __init calculate_hvm_def_policy(void)
+{
+    struct cpuid_policy *p = &hvm_def_cpuid_policy;
+    uint32_t hvm_featureset[FSCAPINTS];
+    unsigned int i;
+    const uint32_t *hvm_featuremask;
+
+    *p = hvm_max_cpuid_policy;
+    cpuid_policy_to_featureset(p, hvm_featureset);
+
+    hvm_featuremask = hvm_hap_supported() ?
+        hvm_hap_def_featuremask : hvm_shadow_def_featuremask;
+
+    for ( i = 0; i < ARRAY_SIZE(hvm_featureset); ++i )
+        hvm_featureset[i] &= hvm_featuremask[i];
+
+    guest_common_feature_adjustments(hvm_featureset);
+
+    sanitise_featureset(hvm_featureset);
+    cpuid_featureset_to_policy(hvm_featureset, p);
+    recalculate_xstate(p);
+}
+
 void __init init_guest_cpuid(void)
 {
     calculate_raw_policy();
     calculate_host_policy();
 
     if ( IS_ENABLED(CONFIG_PV) )
+    {
         calculate_pv_max_policy();
+        calculate_pv_def_policy();
+    }
 
     if ( hvm_enabled )
+    {
         calculate_hvm_max_policy();
+        calculate_hvm_def_policy();
+    }
 }
 
 bool recheck_cpu_features(unsigned int cpu)
@@ -625,8 +676,8 @@  void recalculate_cpuid_policy(struct domain *d)
 int init_domain_cpuid_policy(struct domain *d)
 {
     struct cpuid_policy *p = is_pv_domain(d)
-        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_max_cpuid_policy : NULL)
-        : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_cpuid_policy : NULL);
+        ? (IS_ENABLED(CONFIG_PV)  ?  &pv_def_cpuid_policy : NULL)
+        : (IS_ENABLED(CONFIG_HVM) ? &hvm_def_cpuid_policy : NULL);
 
     if ( !p )
     {
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index cad7534373..b7948f2663 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -48,7 +48,7 @@  const struct cpu_policy system_policies[6] = {
         &pv_max_msr_policy,
     },
     [ XEN_SYSCTL_cpu_policy_pv_default ] = {
-        &pv_max_cpuid_policy,
+        &pv_def_cpuid_policy,
         &pv_def_msr_policy,
     },
 #endif
@@ -58,7 +58,7 @@  const struct cpu_policy system_policies[6] = {
         &hvm_max_msr_policy,
     },
     [ XEN_SYSCTL_cpu_policy_hvm_default ] = {
-        &hvm_max_cpuid_policy,
+        &hvm_def_cpuid_policy,
         &hvm_def_msr_policy,
     },
 #endif
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 1b00e832d6..7baf6c9628 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -49,7 +49,8 @@  DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
 extern struct cpuidmasks cpuidmask_defaults;
 
 extern struct cpuid_policy raw_cpuid_policy, host_cpuid_policy,
-    pv_max_cpuid_policy, hvm_max_cpuid_policy;
+    pv_max_cpuid_policy, pv_def_cpuid_policy,
+    hvm_max_cpuid_policy, hvm_def_cpuid_policy;
 
 extern const struct cpu_policy system_policies[];