[PATCH v2 0/4] x86/cpuid: Introduce dom0-cpuid=

[PATCH v2 0/4] x86/cpuid: Introduce dom0-cpuid=

[Andrew Cooper]

Andrew Cooper (4):
  x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()
  x86/cpuid: Factor common parsing out of parse_xen_cpuid()
  x86/cpuid: Introduce dom0-cpuid command line option
  x86/cpuid: Advertise SERIALIZE by default to guests

 docs/misc/xen-command-line.pandoc           |  17 +++++
 xen/arch/x86/cpuid.c                        | 100 ++++++++++++++++++++++------
 xen/arch/x86/include/asm/cpuid.h            |   3 +
 xen/arch/x86/setup.c                        |  15 +++--
 xen/include/public/arch-x86/cpufeatureset.h |   2 +-
 5 files changed, 112 insertions(+), 25 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()

[Andrew Cooper]

To implement dom0-cpuid= support, the special cases would need extending.
However there is already a problem with late hwdom where the special cases
override toolstack settings, which is unintended and poor behaviour.

Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and
ARCH_CAPS logic.  The is_hardware_domain() can be dropped, and for now there
is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive
operation, and will become more-so over time.

Rearrange the logic in create_dom0() to make room for a call to
init_dom0_cpuid_policy().  The AMX plans for having variable sized XSAVE
states require that modifications to the policy happen before vCPUs are
created.

Additionally, factor out domid into a variable so we can be slightly more
correct in the case of a failure, and also print the error from
domain_create().  This will at least help distinguish -EINVAL from -ENOMEM.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New

It is slightly weird now having CPUID split into two helpers, while the MSR
side of things remains as a special case.  It doesn't actually matter right
now, and it is going to have to change anyway in due course, so I've decided
to go with the simple option for now.
---
 xen/arch/x86/cpuid.c             | 25 +++++++++++++++----------
 xen/arch/x86/include/asm/cpuid.h |  3 +++
 xen/arch/x86/setup.c             | 15 +++++++++++----
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 151944f65702..f63f5efc17f5 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -727,23 +727,28 @@ int init_domain_cpuid_policy(struct domain *d)
     if ( !p )
         return -ENOMEM;
 
-    /* The hardware domain can't migrate.  Give it ITSC if available. */
-    if ( is_hardware_domain(d) )
-        p->extd.itsc = cpu_has_itsc;
+    d->arch.cpuid = p;
+
+    recalculate_cpuid_policy(d);
+
+    return 0;
+}
+
+void __init init_dom0_cpuid_policy(struct domain *d)
+{
+    struct cpuid_policy *p = d->arch.cpuid;
+
+    /* dom0 can't migrate.  Give it ITSC if available. */
+    if ( cpu_has_itsc )
+        p->extd.itsc = true;
 
     /*
      * Expose the "hardware speculation behaviour" bits of ARCH_CAPS to dom0,
      * so dom0 can turn off workarounds as appropriate.  Temporary, until the
      * domain policy logic gains a better understanding of MSRs.
      */
-    if ( is_hardware_domain(d) && cpu_has_arch_caps )
+    if ( cpu_has_arch_caps )
         p->feat.arch_caps = true;
-
-    d->arch.cpuid = p;
-
-    recalculate_cpuid_policy(d);
-
-    return 0;
 }
 
 void guest_cpuid(const struct vcpu *v, uint32_t leaf,
diff --git a/xen/arch/x86/include/asm/cpuid.h b/xen/arch/x86/include/asm/cpuid.h
index 46904061d0ef..9c3637549a10 100644
--- a/xen/arch/x86/include/asm/cpuid.h
+++ b/xen/arch/x86/include/asm/cpuid.h
@@ -59,6 +59,9 @@ bool recheck_cpu_features(unsigned int cpu);
 /* Allocate and initialise a CPUID policy suitable for the domain. */
 int init_domain_cpuid_policy(struct domain *d);
 
+/* Apply dom0-specific tweaks to the CPUID policy. */
+void init_dom0_cpuid_policy(struct domain *d);
+
 /* Clamp the CPUID policy to reality. */
 void recalculate_cpuid_policy(struct domain *d);
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f40a9fe5d351..e716005145d3 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -772,6 +772,7 @@ static struct domain *__init create_dom0(const module_t *image,
     };
     struct domain *d;
     char *cmdline;
+    domid_t domid;
 
     if ( opt_dom0_pvh )
     {
@@ -786,10 +787,16 @@ static struct domain *__init create_dom0(const module_t *image,
     if ( iommu_enabled )
         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
-    /* Create initial domain 0. */
-    d = domain_create(get_initial_domain_id(), &dom0_cfg, !pv_shim);
-    if ( IS_ERR(d) || (alloc_dom0_vcpu0(d) == NULL) )
-        panic("Error creating domain 0\n");
+    /* Create initial domain.  Not d0 for pvshim. */
+    domid = get_initial_domain_id();
+    d = domain_create(domid, &dom0_cfg, !pv_shim);
+    if ( IS_ERR(d) )
+        panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
+
+    init_dom0_cpuid_policy(d);
+
+    if ( alloc_dom0_vcpu0(d) == NULL )
+        panic("Error creating d%uv0\n", domid);
 
     /* Grab the DOM0 command line. */
     cmdline = image->string ? __va(image->string) : NULL;
-- 
2.11.0

[PATCH v2 2/4] x86/cpuid: Factor common parsing out of parse_xen_cpuid()

[Andrew Cooper]

dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
out into parse_cpuid().

Irritatingly, despite being static const, the features[] array gets duplicated
each time parse_cpuid() is inlined.  As it is a large (and ever growing with
new CPU features) datastructure, move it to being file scope so all inlines
use the same single object.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New

We probably want to be wary of fallout from this pattern elsewhere.  I only
noticed it by chance.
---
 xen/arch/x86/cpuid.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index f63f5efc17f5..e11f5a3c9a6b 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -26,17 +26,26 @@ static const uint32_t __initconst 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)
+static const struct feature_name {
+    const char *name;
+    unsigned int bit;
+} feature_names[] __initconstrel = INIT_FEATURE_NAMES;
+
+/*
+ * Parse a list of cpuid feature names -> bool, calling the callback for any
+ * matches found.
+ *
+ * always_inline, because this is init code only and we really don't want a
+ * function pointer call in the middle of the loop.
+ */
+static int __init always_inline parse_cpuid(
+    const char *s, void (*callback)(unsigned int feat, bool val))
 {
     const char *ss;
     int val, rc = 0;
 
     do {
-        static const struct feature {
-            const char *name;
-            unsigned int bit;
-        } features[] __initconstrel = INIT_FEATURE_NAMES;
-        const struct feature *lhs, *rhs, *mid = NULL /* GCC... */;
+        const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */;
         const char *feat;
 
         ss = strchr(s, ',');
@@ -49,8 +58,8 @@ static int __init parse_xen_cpuid(const char *s)
             feat += 3;
 
         /* (Re)initalise lhs and rhs for binary search. */
-        lhs = features;
-        rhs = features + ARRAY_SIZE(features);
+        lhs = feature_names;
+        rhs = feature_names + ARRAY_SIZE(feature_names);
 
         while ( lhs < rhs )
         {
@@ -72,11 +81,7 @@ static int __init parse_xen_cpuid(const char *s)
 
             if ( (val = parse_boolean(mid->name, s, ss)) >= 0 )
             {
-                if ( !val )
-                    setup_clear_cpu_cap(mid->bit);
-                else if ( mid->bit == X86_FEATURE_RDRAND &&
-                          (cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_RDRAND)) )
-                    setup_force_cpu_cap(X86_FEATURE_RDRAND);
+                callback(mid->bit, val);
                 mid = NULL;
             }
 
@@ -95,6 +100,20 @@ static int __init parse_xen_cpuid(const char *s)
 
     return rc;
 }
+
+static void __init _parse_xen_cpuid(unsigned int feat, bool val)
+{
+    if ( !val )
+        setup_clear_cpu_cap(feat);
+    else if ( feat == X86_FEATURE_RDRAND &&
+              (cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_RDRAND)) )
+        setup_force_cpu_cap(X86_FEATURE_RDRAND);
+}
+
+static int __init parse_xen_cpuid(const char *s)
+{
+    return parse_cpuid(s, _parse_xen_cpuid);
+}
 custom_param("cpuid", parse_xen_cpuid);
 
 #define EMPTY_LEAF ((struct cpuid_leaf){})
-- 
2.11.0

[PATCH v2 3/4] x86/cpuid: Introduce dom0-cpuid command line option

[Andrew Cooper]

Specifically, this lets the user opt in to non-default for dom0.

Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it
to dom0's policy when other tweaks are being made.

As recalculate_cpuid_policy() is an expensive action, and dom0-cpuid= is
likely to only be used by the x86 maintainers for development purposes, forgo
the recalculation in the general case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Rework almost from scratch, on top of broken-out changes.
---
 docs/misc/xen-command-line.pandoc | 17 +++++++++++++++++
 xen/arch/x86/cpuid.c              | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f7797ea233f9..383a854dec60 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -801,6 +801,23 @@ Controls for how dom0 is constructed on x86 systems.
 
     If using this option is necessary to fix an issue, please report a bug.
 
+### dom0-cpuid
+    = List of comma separated booleans
+
+    Applicability: x86
+
+This option allows for fine tuning of the facilities dom0 will use, after
+accounting for hardware capabilities and Xen settings as enumerated via CPUID.
+
+Options are accepted in positive and negative form, to enable or disable
+specific features, but specify both forms of the same option is undefined.
+All selections via this mechanism are subject to normal CPU Policy safety and
+dependency logic.
+
+This option is intended for developers to opt dom0 into non-default features,
+and is not intended for use in production circumstances.  If using this option
+is necessary to fix an issue, please report a bug.
+
 ### dom0-iommu
     = List of [ passthrough=<bool>, strict=<bool>, map-inclusive=<bool>,
                 map-reserved=<bool>, none ]
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e11f5a3c9a6b..83a80ba6de70 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -116,6 +116,23 @@ static int __init parse_xen_cpuid(const char *s)
 }
 custom_param("cpuid", parse_xen_cpuid);
 
+static bool __initdata dom0_cpuid_cmdline;
+static uint32_t __initdata dom0_enable_feat[FSCAPINTS];
+static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
+
+static void __init _parse_dom0_cpuid(unsigned int feat, bool val)
+{
+    __set_bit(feat, val ? dom0_enable_feat : dom0_disable_feat);
+}
+
+static int __init parse_dom0_cpuid(const char *s)
+{
+    dom0_cpuid_cmdline = true;
+
+    return parse_cpuid(s, _parse_dom0_cpuid);
+}
+custom_param("dom0-cpuid", parse_dom0_cpuid);
+
 #define EMPTY_LEAF ((struct cpuid_leaf){})
 static void zero_leaves(struct cpuid_leaf *l,
                         unsigned int first, unsigned int last)
@@ -768,6 +785,25 @@ void __init init_dom0_cpuid_policy(struct domain *d)
      */
     if ( cpu_has_arch_caps )
         p->feat.arch_caps = true;
+
+    /* Apply dom0-cpuid= command line settings, if provided. */
+    if ( dom0_cpuid_cmdline )
+    {
+        uint32_t fs[FSCAPINTS];
+        unsigned int i;
+
+        cpuid_policy_to_featureset(p, fs);
+
+        for ( i = 0; i < ARRAY_SIZE(fs); ++i )
+        {
+            fs[i] |= dom0_enable_feat[i];
+            fs[i] &= ~dom0_disable_feat[i];
+        }
+
+        cpuid_featureset_to_policy(fs, p);
+
+        recalculate_cpuid_policy(d);
+    }
 }
 
 void guest_cpuid(const struct vcpu *v, uint32_t leaf,
-- 
2.11.0

[PATCH v2 4/4] x86/cpuid: Advertise SERIALIZE by default to guests

[Andrew Cooper]

I've played with SERIALIZE, TSXLDTRK, MOVDIRI and MOVDIR64 on real hardware,
and they all seem fine, including emulation support.

SERIALIZE exists specifically to have a userspace usable serialising operation
without other side effects.  (The only other two choices are CPUID which is a
VMExit under virt and clobbers 4 registers, and IRET-to-self which very slow
and consumes content from the stack.)

TSXLDTRK is a niche TSX feature, and TSX itself is niche outside of demos of
speculative sidechannels.  Leave the feature opt-in until a usecase is found,
in an effort to preempt the multiple person years of effort it has taken to
mop up TSX issues impacting every processor line.

MOVDIRI and MOVDIR64 are harder to judge.  They're architectural building
blocks towards ENQCMD{,S} without obvious usecases on their own.  They're of
no use to domains without PCI devices, so leave them opt-in for now.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New
---
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 647ee9e5e277..0b399375566f 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -278,7 +278,7 @@ XEN_CPUFEATURE(SRBDS_CTRL,    9*32+ 9) /*   MSR_MCU_OPT_CTRL and RNGDS_MITG_DIS.
 XEN_CPUFEATURE(MD_CLEAR,      9*32+10) /*A  VERW clears microarchitectural buffers */
 XEN_CPUFEATURE(RTM_ALWAYS_ABORT, 9*32+11) /*! June 2021 TSX defeaturing in microcode. */
 XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
-XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*a  SERIALIZE insn */
+XEN_CPUFEATURE(SERIALIZE,     9*32+14) /*A  SERIALIZE insn */
 XEN_CPUFEATURE(TSXLDTRK,      9*32+16) /*a  TSX load tracking suspend/resume insns */
 XEN_CPUFEATURE(CET_IBT,       9*32+20) /*   CET - Indirect Branch Tracking */
 XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by Intel) */
-- 
2.11.0

Re: [PATCH v2 3/4] x86/cpuid: Introduce dom0-cpuid command line option

[Andrew Cooper]

On 15/12/2021 22:21, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index e11f5a3c9a6b..83a80ba6de70 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -116,6 +116,23 @@ static int __init parse_xen_cpuid(const char *s)
>  }
>  custom_param("cpuid", parse_xen_cpuid);
>  
> +static bool __initdata dom0_cpuid_cmdline;
> +static uint32_t __initdata dom0_enable_feat[FSCAPINTS];
> +static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
> +
> +static void __init _parse_dom0_cpuid(unsigned int feat, bool val)
> +{
> +    __set_bit(feat, val ? dom0_enable_feat : dom0_disable_feat);

Based on Jan's observation in v1, I've folded this delta in:

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 83a80ba6de70..39baeae9a6cd 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -122,7 +122,8 @@ static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
 
 static void __init _parse_dom0_cpuid(unsigned int feat, bool val)
 {
-    __set_bit(feat, val ? dom0_enable_feat : dom0_disable_feat);
+    __set_bit  (feat, val ? dom0_enable_feat  : dom0_disable_feat);
+    __clear_bit(feat, val ? dom0_disable_feat : dom0_enable_feat );
 }
 
 static int __init parse_dom0_cpuid(const char *s)

~Andrew

Re: [PATCH v2 1/4] x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()

[Jan Beulich]

On 15.12.2021 23:21, Andrew Cooper wrote:
> To implement dom0-cpuid= support, the special cases would need extending.
> However there is already a problem with late hwdom where the special cases
> override toolstack settings, which is unintended and poor behaviour.
> 
> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and
> ARCH_CAPS logic.  The is_hardware_domain() can be dropped, and for now there
> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive
> operation, and will become more-so over time.

Would you mind leaving it there in a commented out form, hinting at when
it may need re-enabling?

> Rearrange the logic in create_dom0() to make room for a call to
> init_dom0_cpuid_policy().  The AMX plans for having variable sized XSAVE
> states require that modifications to the policy happen before vCPUs are
> created.
> 
> Additionally, factor out domid into a variable so we can be slightly more
> correct in the case of a failure, and also print the error from
> domain_create().  This will at least help distinguish -EINVAL from -ENOMEM.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Preferably with said comment added:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 2/4] x86/cpuid: Factor common parsing out of parse_xen_cpuid()

[Jan Beulich]

On 15.12.2021 23:21, Andrew Cooper wrote:
> dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
> out into parse_cpuid().
> 
> Irritatingly, despite being static const, the features[] array gets duplicated
> each time parse_cpuid() is inlined.  As it is a large (and ever growing with
> new CPU features) datastructure, move it to being file scope so all inlines
> use the same single object.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> We probably want to be wary of fallout from this pattern elsewhere.  I only
> noticed it by chance.

While that sounds at least close to a bug, there might by some subtle reason
for why they have to do it that way.

Jan

Re: [PATCH v2 1/4] x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()

[Andrew Cooper]

On 16/12/2021 16:38, Jan Beulich wrote:
> On 15.12.2021 23:21, Andrew Cooper wrote:
>> To implement dom0-cpuid= support, the special cases would need extending.
>> However there is already a problem with late hwdom where the special cases
>> override toolstack settings, which is unintended and poor behaviour.
>>
>> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and
>> ARCH_CAPS logic.  The is_hardware_domain() can be dropped, and for now there
>> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive
>> operation, and will become more-so over time.
> Would you mind leaving it there in a commented out form, hinting at when
> it may need re-enabling?

Leave what?  The recalculate_cpuid_policy()?  That comes back in later
in the series.

~Andrew

>
>> Rearrange the logic in create_dom0() to make room for a call to
>> init_dom0_cpuid_policy().  The AMX plans for having variable sized XSAVE
>> states require that modifications to the policy happen before vCPUs are
>> created.
>>
>> Additionally, factor out domid into a variable so we can be slightly more
>> correct in the case of a failure, and also print the error from
>> domain_create().  This will at least help distinguish -EINVAL from -ENOMEM.
>>
>> No practical change in behaviour.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Preferably with said comment added:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>
>

Re: [PATCH v2 3/4] x86/cpuid: Introduce dom0-cpuid command line option

[Jan Beulich]

On 15.12.2021 23:21, Andrew Cooper wrote:
> Specifically, this lets the user opt in to non-default for dom0.
> 
> Collect all dom0 settings together in dom0_{en,dis}able_feat[], and apply it
> to dom0's policy when other tweaks are being made.
> 
> As recalculate_cpuid_policy() is an expensive action, and dom0-cpuid= is
> likely to only be used by the x86 maintainers for development purposes, forgo
> the recalculation in the general case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 3/4] x86/cpuid: Introduce dom0-cpuid command line option

[Jan Beulich]

On 16.12.2021 12:56, Andrew Cooper wrote:
> On 15/12/2021 22:21, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
>> index e11f5a3c9a6b..83a80ba6de70 100644
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -116,6 +116,23 @@ static int __init parse_xen_cpuid(const char *s)
>>  }
>>  custom_param("cpuid", parse_xen_cpuid);
>>  
>> +static bool __initdata dom0_cpuid_cmdline;
>> +static uint32_t __initdata dom0_enable_feat[FSCAPINTS];
>> +static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
>> +
>> +static void __init _parse_dom0_cpuid(unsigned int feat, bool val)
>> +{
>> +    __set_bit(feat, val ? dom0_enable_feat : dom0_disable_feat);
> 
> Based on Jan's observation in v1, I've folded this delta in:
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index 83a80ba6de70..39baeae9a6cd 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -122,7 +122,8 @@ static uint32_t __initdata dom0_disable_feat[FSCAPINTS];
>  
>  static void __init _parse_dom0_cpuid(unsigned int feat, bool val)
>  {
> -    __set_bit(feat, val ? dom0_enable_feat : dom0_disable_feat);
> +    __set_bit  (feat, val ? dom0_enable_feat  : dom0_disable_feat);
> +    __clear_bit(feat, val ? dom0_disable_feat : dom0_enable_feat );
>  }

FAOD my R-b applies with this included; I had meant to reply here
but then replied to the original patch.

Jan

Re: [PATCH v2 2/4] x86/cpuid: Factor common parsing out of parse_xen_cpuid()

[Andrew Cooper]

On 16/12/2021 16:41, Jan Beulich wrote:
> On 15.12.2021 23:21, Andrew Cooper wrote:
>> dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
>> out into parse_cpuid().
>>
>> Irritatingly, despite being static const, the features[] array gets duplicated
>> each time parse_cpuid() is inlined.  As it is a large (and ever growing with
>> new CPU features) datastructure, move it to being file scope so all inlines
>> use the same single object.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> We probably want to be wary of fallout from this pattern elsewhere.  I only
>> noticed it by chance.
> While that sounds at least close to a bug, there might by some subtle reason
> for why they have to do it that way.

Now I've thought about this more, probably C's "every object has a
unique address" rule.

~Andrew

Re: [PATCH v2 4/4] x86/cpuid: Advertise SERIALIZE by default to guests

[Jan Beulich]

On 15.12.2021 23:21, Andrew Cooper wrote:
> I've played with SERIALIZE, TSXLDTRK, MOVDIRI and MOVDIR64 on real hardware,
> and they all seem fine, including emulation support.
> 
> SERIALIZE exists specifically to have a userspace usable serialising operation
> without other side effects.  (The only other two choices are CPUID which is a
> VMExit under virt and clobbers 4 registers, and IRET-to-self which very slow
> and consumes content from the stack.)
> 
> TSXLDTRK is a niche TSX feature, and TSX itself is niche outside of demos of
> speculative sidechannels.  Leave the feature opt-in until a usecase is found,
> in an effort to preempt the multiple person years of effort it has taken to
> mop up TSX issues impacting every processor line.
> 
> MOVDIRI and MOVDIR64 are harder to judge.  They're architectural building
> blocks towards ENQCMD{,S} without obvious usecases on their own.  They're of
> no use to domains without PCI devices, so leave them opt-in for now.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 1/4] x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()

[Jan Beulich]

On 16.12.2021 17:41, Andrew Cooper wrote:
> On 16/12/2021 16:38, Jan Beulich wrote:
>> On 15.12.2021 23:21, Andrew Cooper wrote:
>>> To implement dom0-cpuid= support, the special cases would need extending.
>>> However there is already a problem with late hwdom where the special cases
>>> override toolstack settings, which is unintended and poor behaviour.
>>>
>>> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and
>>> ARCH_CAPS logic.  The is_hardware_domain() can be dropped, and for now there
>>> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive
>>> operation, and will become more-so over time.
>> Would you mind leaving it there in a commented out form, hinting at when
>> it may need re-enabling?
> 
> Leave what?  The recalculate_cpuid_policy()?  That comes back in later
> in the series.

I've meanwhile spotted it, yes. Let's hope its conditional invocation
there makes clear enough that with certain other changes it may also
be needed.

Jan

Re: [PATCH v2 1/4] x86/cpuid: Split dom0 handling out of init_domain_cpuid_policy()

[Andrew Cooper]

On 16/12/2021 16:51, Jan Beulich wrote:
> On 16.12.2021 17:41, Andrew Cooper wrote:
>> On 16/12/2021 16:38, Jan Beulich wrote:
>>> On 15.12.2021 23:21, Andrew Cooper wrote:
>>>> To implement dom0-cpuid= support, the special cases would need extending.
>>>> However there is already a problem with late hwdom where the special cases
>>>> override toolstack settings, which is unintended and poor behaviour.
>>>>
>>>> Introduce a new init_dom0_cpuid_policy() for the purpose, moving the ITSC and
>>>> ARCH_CAPS logic.  The is_hardware_domain() can be dropped, and for now there
>>>> is no need to rerun recalculate_cpuid_policy(); this is a relatively expensive
>>>> operation, and will become more-so over time.
>>> Would you mind leaving it there in a commented out form, hinting at when
>>> it may need re-enabling?
>> Leave what?  The recalculate_cpuid_policy()?  That comes back in later
>> in the series.
> I've meanwhile spotted it, yes. Let's hope its conditional invocation
> there makes clear enough that with certain other changes it may also
> be needed.

In reality, I expect ITSC never to need a recalc, and ARCH_CAPS is going
to turn into not-a-special-case just as soon as I can possibly make it
happen.

~Andrew