[PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Roger Pau Monne]

Hello,

Introduce support for exposing {IPRED,RRSBA,BHI}_CTRL feature bits and
allow setting the corresponding SPEC_CTRL MSR fields.

The bits are documented in:

https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/branch-history-injection.html

Xen doesn't use the bits itself.

Thanks, Roger.

Roger Pau Monne (3):
  x86/intel: expose IPRED_CTRL to guests
  x86/intel: expose RRSBA_CTRL to guests
  x86/intel: expose BHI_CTRL to guests

 xen/arch/x86/msr.c                          | 7 +++++++
 xen/include/public/arch-x86/cpufeatureset.h | 6 +++---
 xen/tools/gen-cpuid.py                      | 3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c                          | 3 +++
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py                      | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index c33dc78cd8f6..d500f87a5fd1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
     return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
             (ssbd       ? SPEC_CTRL_SSBD       : 0) |
             (psfd       ? SPEC_CTRL_PSFD       : 0) |
+            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
+                                    SPEC_CTRL_IPRED_DIS_S)
+                                 : 0) |
             0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index c897c2040136..e586e141c329 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -302,7 +302,7 @@ XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor Inventory
 
 /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */
 XEN_CPUFEATURE(INTEL_PSFD,         13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
-XEN_CPUFEATURE(IPRED_CTRL,         13*32+ 1) /*   MSR_SPEC_CTRL.IPRED_DIS_* */
+XEN_CPUFEATURE(IPRED_CTRL,         13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
 XEN_CPUFEATURE(RRSBA_CTRL,         13*32+ 2) /*   MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,           13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
 XEN_CPUFEATURE(BHI_CTRL,           13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index df5222a3cdd0..45fab5e75d1c 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -319,7 +319,8 @@ def crunch_numbers(state):
         # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
         # as dependent features simplifies Xen's logic, and prevents the guest
         # from seeing implausible configurations.
-        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
+        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
+                IPRED_CTRL],
         IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
                IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
         IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

[PATCH 2/3] x86/intel: expose RRSBA_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the RRSBA_DIS_{U,S} controls in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c                          | 3 +++
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py                      | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index d500f87a5fd1..b3b4f75c021a 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -327,6 +327,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
             (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
                                     SPEC_CTRL_IPRED_DIS_S)
                                  : 0) |
+            (cp->feat.rrsba_ctrl ? (SPEC_CTRL_RRSBA_DIS_U |
+                                    SPEC_CTRL_RRSBA_DIS_S)
+                                 : 0) |
             0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index e586e141c329..bade4edab30c 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -303,7 +303,7 @@ XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor Inventory
 /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */
 XEN_CPUFEATURE(INTEL_PSFD,         13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
 XEN_CPUFEATURE(IPRED_CTRL,         13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
-XEN_CPUFEATURE(RRSBA_CTRL,         13*32+ 2) /*   MSR_SPEC_CTRL.RRSBA_DIS_* */
+XEN_CPUFEATURE(RRSBA_CTRL,         13*32+ 2) /*A  MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,           13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
 XEN_CPUFEATURE(BHI_CTRL,           13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
 XEN_CPUFEATURE(MCDT_NO,            13*32+ 5) /*A  MCDT_NO */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 45fab5e75d1c..1c6d76244177 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -320,7 +320,7 @@ def crunch_numbers(state):
         # as dependent features simplifies Xen's logic, and prevents the guest
         # from seeing implausible configurations.
         IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
-                IPRED_CTRL],
+                IPRED_CTRL, RRSBA_CTRL],
         IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
                IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
         IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

[PATCH 3/3] x86/intel: expose BHI_CTRL to guests

[Roger Pau Monne]

The CPUID feature bit signals the presence of the BHI_DIS_S control in
SPEC_CTRL MSR.

Note that those controls are not used by the hypervisor.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c                          | 1 +
 xen/include/public/arch-x86/cpufeatureset.h | 2 +-
 xen/tools/gen-cpuid.py                      | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index b3b4f75c021a..e0d57bce40ec 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -330,6 +330,7 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
             (cp->feat.rrsba_ctrl ? (SPEC_CTRL_RRSBA_DIS_U |
                                     SPEC_CTRL_RRSBA_DIS_S)
                                  : 0) |
+            (cp->feat.bhi_ctrl   ? SPEC_CTRL_BHI_DIS_S : 0) |
             0);
 }
 
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index bade4edab30c..be5c1b748e27 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -305,7 +305,7 @@ XEN_CPUFEATURE(INTEL_PSFD,         13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
 XEN_CPUFEATURE(IPRED_CTRL,         13*32+ 1) /*A  MSR_SPEC_CTRL.IPRED_DIS_* */
 XEN_CPUFEATURE(RRSBA_CTRL,         13*32+ 2) /*A  MSR_SPEC_CTRL.RRSBA_DIS_* */
 XEN_CPUFEATURE(DDP_CTRL,           13*32+ 3) /*   MSR_SPEC_CTRL.DDP_DIS_U */
-XEN_CPUFEATURE(BHI_CTRL,           13*32+ 4) /*   MSR_SPEC_CTRL.BHI_DIS_S */
+XEN_CPUFEATURE(BHI_CTRL,           13*32+ 4) /*A  MSR_SPEC_CTRL.BHI_DIS_S */
 XEN_CPUFEATURE(MCDT_NO,            13*32+ 5) /*A  MCDT_NO */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 1c6d76244177..25d329ce486f 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -320,7 +320,7 @@ def crunch_numbers(state):
         # as dependent features simplifies Xen's logic, and prevents the guest
         # from seeing implausible configurations.
         IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS,
-                IPRED_CTRL, RRSBA_CTRL],
+                IPRED_CTRL, RRSBA_CTRL, BHI_CTRL],
         IBRS: [AMD_STIBP, AMD_SSBD, PSFD, AUTO_IBRS,
                IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
         IBPB: [IBPB_RET, SBPB, IBPB_BRTYPE],
-- 
2.43.0

[PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monne]

Dummy set/clear tests for additional spec_ctrl bits.
---
 docs/all-tests.dox  |   2 +
 tests/test/Makefile |   9 ++++
 tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 tests/test/Makefile
 create mode 100644 tests/test/main.c

diff --git a/docs/all-tests.dox b/docs/all-tests.dox
index 892a9e474743..5a66ac252ea5 100644
--- a/docs/all-tests.dox
+++ b/docs/all-tests.dox
@@ -187,3 +187,5 @@ states.
 
 @subpage test-nested-vmx - Nested VT-x tests.
 */
+# Placeholder: Merge into the appropriate location above
+@subpage test-test - @todo title
diff --git a/tests/test/Makefile b/tests/test/Makefile
new file mode 100644
index 000000000000..19bc4b6a4639
--- /dev/null
+++ b/tests/test/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := test
+CATEGORY  := utility
+TEST-ENVS := hvm32 pv64
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/test/main.c b/tests/test/main.c
new file mode 100644
index 000000000000..9a25e95d91b7
--- /dev/null
+++ b/tests/test/main.c
@@ -0,0 +1,100 @@
+/**
+ * @file tests/test/main.c
+ * @ref test-test
+ *
+ * @page test-test test
+ *
+ * @todo Docs for test-test
+ *
+ * @see tests/test/main.c
+ */
+#include <xtf.h>
+
+#define MSR_SPEC_CTRL                       0x00000048
+#define  SPEC_CTRL_IPRED_DIS_U              (_AC(1, ULL) <<  3)
+#define  SPEC_CTRL_IPRED_DIS_S              (_AC(1, ULL) <<  4)
+#define  SPEC_CTRL_RRSBA_DIS_U              (_AC(1, ULL) <<  5)
+#define  SPEC_CTRL_RRSBA_DIS_S              (_AC(1, ULL) <<  6)
+#define  SPEC_CTRL_DDP_DIS_U                (_AC(1, ULL) <<  8)
+#define  SPEC_CTRL_BHI_DIS_S                (_AC(1, ULL) << 10)
+
+const char test_title[] = "SPEC_CTRL";
+
+static void update_spec_ctrl(uint64_t mask, bool set)
+{
+    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
+
+    if ( set )
+        spec_ctrl |= mask;
+    else
+        spec_ctrl &= ~mask;
+
+    wrmsr(MSR_SPEC_CTRL, spec_ctrl);
+}
+
+static void assert_spec_ctrl(uint64_t mask, bool set)
+{
+    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
+
+    if ( (spec_ctrl & mask) != (set ? mask : 0) )
+    {
+        xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
+                    set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
+                    spec_ctrl);
+        xtf_exit();
+    }
+}
+
+static void test_loop(uint64_t mask)
+{
+    update_spec_ctrl(mask, true);
+    assert_spec_ctrl(mask, true);
+    /* Ensure context switch to Xen. */
+    hypercall_yield();
+    assert_spec_ctrl(mask, true);
+
+    update_spec_ctrl(mask, false);
+    assert_spec_ctrl(mask, false);
+    /* Ensure context switch to Xen. */
+    hypercall_yield();
+    assert_spec_ctrl(mask, false);
+}
+
+void test_main(void)
+{
+    static const struct {
+        const char *name;
+        unsigned int feat;
+        uint64_t mask;
+    } tests[] = {
+        { "IPRED CTRL", 1, SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S },
+        { "RRSBA CTRL", 2, SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S },
+        { "DDP DIS", 3, SPEC_CTRL_DDP_DIS_U },
+        { "BHI DIS", 4, SPEC_CTRL_BHI_DIS_S },
+    };
+    unsigned int i;
+    uint32_t regs[4];
+
+    cpuid_count(7, 2, &regs[0], &regs[1], &regs[2], &regs[3]);
+
+    for ( i = 0; i < ARRAY_SIZE(tests); i++ )
+    {
+        if ( !test_bit(tests[i].feat, &regs[3]) )
+            continue;
+
+        printk("Testing %s\n", tests[i].name);
+        test_loop(tests[i].mask);
+    }
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Jan Beulich]

On 30.01.2024 11:27, Roger Pau Monne wrote:
> Dummy set/clear tests for additional spec_ctrl bits.
> ---
>  docs/all-tests.dox  |   2 +
>  tests/test/Makefile |   9 ++++
>  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 tests/test/Makefile
>  create mode 100644 tests/test/main.c

I'm puzzled: Why "test"? That doesn't describe in any way what this test
is about.

> --- /dev/null
> +++ b/tests/test/Makefile
> @@ -0,0 +1,9 @@
> +include $(ROOT)/build/common.mk
> +
> +NAME      := test
> +CATEGORY  := utility
> +TEST-ENVS := hvm32 pv64

Any reason for this limitation?

> --- /dev/null
> +++ b/tests/test/main.c
> @@ -0,0 +1,100 @@
> +/**
> + * @file tests/test/main.c
> + * @ref test-test
> + *
> + * @page test-test test
> + *
> + * @todo Docs for test-test
> + *
> + * @see tests/test/main.c
> + */
> +#include <xtf.h>
> +
> +#define MSR_SPEC_CTRL                       0x00000048
> +#define  SPEC_CTRL_IPRED_DIS_U              (_AC(1, ULL) <<  3)
> +#define  SPEC_CTRL_IPRED_DIS_S              (_AC(1, ULL) <<  4)
> +#define  SPEC_CTRL_RRSBA_DIS_U              (_AC(1, ULL) <<  5)
> +#define  SPEC_CTRL_RRSBA_DIS_S              (_AC(1, ULL) <<  6)
> +#define  SPEC_CTRL_DDP_DIS_U                (_AC(1, ULL) <<  8)
> +#define  SPEC_CTRL_BHI_DIS_S                (_AC(1, ULL) << 10)
> +
> +const char test_title[] = "SPEC_CTRL";
> +
> +static void update_spec_ctrl(uint64_t mask, bool set)
> +{
> +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +    if ( set )
> +        spec_ctrl |= mask;
> +    else
> +        spec_ctrl &= ~mask;
> +
> +    wrmsr(MSR_SPEC_CTRL, spec_ctrl);
> +}
> +
> +static void assert_spec_ctrl(uint64_t mask, bool set)
> +{
> +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> +
> +    if ( (spec_ctrl & mask) != (set ? mask : 0) )
> +    {
> +        xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
> +                    set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
> +                    spec_ctrl);
> +        xtf_exit();
> +    }
> +}
> +
> +static void test_loop(uint64_t mask)
> +{
> +    update_spec_ctrl(mask, true);
> +    assert_spec_ctrl(mask, true);
> +    /* Ensure context switch to Xen. */
> +    hypercall_yield();

I'm afraid yielding doesn't guarantee context switching in Xen, if the
system (or even just the one CPU) is otherwise idle. Hence at the very
least please don't say "ensure" in the comment. But perhaps more
reliable to e.g. use "poll" with a timeout. While I didn't post that
addition, I've used such for testing my vCPU-area-registration work:

        struct sched_poll poll = { .timeout = s + SECONDS(1) };
        rc = hypercall_sched_op(SCHEDOP_poll, &poll);
        if ( rc )
            xtf_error("Could not poll (%d)\n", rc);

(there also to ensure enough time passes for the time area to be
updated).

I actually found this to have another neat side effect: The guest then
can't go away so quickly that "xl console" doesn't manage to attach to
the guest (which otherwise I observe to work only about every other
time).

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 10:13, Roger Pau Monne wrote:
> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
> SPEC_CTRL MSR.
> 
> Note that those controls are not used by the hypervisor.

Despite this, ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> +                                    SPEC_CTRL_IPRED_DIS_S)
> +                                 : 0) |
>              0);
>  }

... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
description it wants clarifying why it is acceptable to run Xen with the
guest chosen settings for at least the DIS_S bit (assuming that it is
okay to do so). Likely (didn't look there yet) also applicable to the
further two patches.

Jan

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
> On 30.01.2024 11:27, Roger Pau Monne wrote:
> > Dummy set/clear tests for additional spec_ctrl bits.
> > ---
> >  docs/all-tests.dox  |   2 +
> >  tests/test/Makefile |   9 ++++
> >  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 111 insertions(+)
> >  create mode 100644 tests/test/Makefile
> >  create mode 100644 tests/test/main.c
> 
> I'm puzzled: Why "test"? That doesn't describe in any way what this test
> is about.

That's just my place holder for random XTF stuff.  I don't intend this
to be committed.

> > --- /dev/null
> > +++ b/tests/test/Makefile
> > @@ -0,0 +1,9 @@
> > +include $(ROOT)/build/common.mk
> > +
> > +NAME      := test
> > +CATEGORY  := utility
> > +TEST-ENVS := hvm32 pv64
> 
> Any reason for this limitation?

Just wanted a PV and an HVM context.

> > --- /dev/null
> > +++ b/tests/test/main.c
> > @@ -0,0 +1,100 @@
> > +/**
> > + * @file tests/test/main.c
> > + * @ref test-test
> > + *
> > + * @page test-test test
> > + *
> > + * @todo Docs for test-test
> > + *
> > + * @see tests/test/main.c
> > + */
> > +#include <xtf.h>
> > +
> > +#define MSR_SPEC_CTRL                       0x00000048
> > +#define  SPEC_CTRL_IPRED_DIS_U              (_AC(1, ULL) <<  3)
> > +#define  SPEC_CTRL_IPRED_DIS_S              (_AC(1, ULL) <<  4)
> > +#define  SPEC_CTRL_RRSBA_DIS_U              (_AC(1, ULL) <<  5)
> > +#define  SPEC_CTRL_RRSBA_DIS_S              (_AC(1, ULL) <<  6)
> > +#define  SPEC_CTRL_DDP_DIS_U                (_AC(1, ULL) <<  8)
> > +#define  SPEC_CTRL_BHI_DIS_S                (_AC(1, ULL) << 10)
> > +
> > +const char test_title[] = "SPEC_CTRL";
> > +
> > +static void update_spec_ctrl(uint64_t mask, bool set)
> > +{
> > +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> > +
> > +    if ( set )
> > +        spec_ctrl |= mask;
> > +    else
> > +        spec_ctrl &= ~mask;
> > +
> > +    wrmsr(MSR_SPEC_CTRL, spec_ctrl);
> > +}
> > +
> > +static void assert_spec_ctrl(uint64_t mask, bool set)
> > +{
> > +    uint64_t spec_ctrl = rdmsr(MSR_SPEC_CTRL);
> > +
> > +    if ( (spec_ctrl & mask) != (set ? mask : 0) )
> > +    {
> > +        xtf_failure("SPEC_CTRL expected: %#" PRIx64 " got: %#" PRIx64 "\n",
> > +                    set ? (spec_ctrl | mask) : (spec_ctrl & ~mask),
> > +                    spec_ctrl);
> > +        xtf_exit();
> > +    }
> > +}
> > +
> > +static void test_loop(uint64_t mask)
> > +{
> > +    update_spec_ctrl(mask, true);
> > +    assert_spec_ctrl(mask, true);
> > +    /* Ensure context switch to Xen. */
> > +    hypercall_yield();
> 
> I'm afraid yielding doesn't guarantee context switching in Xen,

It will ensure a vmexit/trap, which is what I was after here.  Maybe the
comment should be "Trap into Xen." or some such.  It wasn't about
ensuring VM context switching.

Thanks, Roger.

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> On 30.01.2024 10:13, Roger Pau Monne wrote:
> > The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
> > SPEC_CTRL MSR.
> > 
> > Note that those controls are not used by the hypervisor.
> 
> Despite this, ...
> 
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
> >      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> >              (psfd       ? SPEC_CTRL_PSFD       : 0) |
> > +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> > +                                    SPEC_CTRL_IPRED_DIS_S)
> > +                                 : 0) |
> >              0);
> >  }
> 
> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> description it wants clarifying why it is acceptable to run Xen with the
> guest chosen settings for at least the DIS_S bit (assuming that it is
> okay to do so). Likely (didn't look there yet) also applicable to the
> further two patches.

"The added feature is made dependent on IBRSB, which ensures it will
only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
ensures the value of SPEC_CTRL will get context switched on exit/entry
to guest."

Would adding the above to the commit message clarify the intended
implementation?

Thanks, Roger.

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Jan Beulich]

On 30.01.2024 12:46, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
>> On 30.01.2024 11:27, Roger Pau Monne wrote:
>>> Dummy set/clear tests for additional spec_ctrl bits.
>>> ---
>>>  docs/all-tests.dox  |   2 +
>>>  tests/test/Makefile |   9 ++++
>>>  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 111 insertions(+)
>>>  create mode 100644 tests/test/Makefile
>>>  create mode 100644 tests/test/main.c
>>
>> I'm puzzled: Why "test"? That doesn't describe in any way what this test
>> is about.
> 
> That's just my place holder for random XTF stuff.  I don't intend this
> to be committed.

Could have been said then one way or another.

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 13:06, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>> On 30.01.2024 10:13, Roger Pau Monne wrote:
>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
>>> SPEC_CTRL MSR.
>>>
>>> Note that those controls are not used by the hypervisor.
>>
>> Despite this, ...
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
>>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
>>> +                                    SPEC_CTRL_IPRED_DIS_S)
>>> +                                 : 0) |
>>>              0);
>>>  }
>>
>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>> description it wants clarifying why it is acceptable to run Xen with the
>> guest chosen settings for at least the DIS_S bit (assuming that it is
>> okay to do so). Likely (didn't look there yet) also applicable to the
>> further two patches.
> 
> "The added feature is made dependent on IBRSB, which ensures it will
> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> ensures the value of SPEC_CTRL will get context switched on exit/entry
> to guest."
> 
> Would adding the above to the commit message clarify the intended
> implementation?

It would improve things, at least hinting towards there being a connection
between exposure and updating on entry to Xen. I'd like to ask though to
avoid "context switch" when talking about entry from guest context. While
in a way technically correct, our normal meaning of the term is the
process of switching vCPU-s out/in on a pCPU.

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
> On 30.01.2024 13:06, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> >> On 30.01.2024 10:13, Roger Pau Monne wrote:
> >>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
> >>> SPEC_CTRL MSR.
> >>>
> >>> Note that those controls are not used by the hypervisor.
> >>
> >> Despite this, ...
> >>
> >>> --- a/xen/arch/x86/msr.c
> >>> +++ b/xen/arch/x86/msr.c
> >>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
> >>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> >>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
> >>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> >>> +                                    SPEC_CTRL_IPRED_DIS_S)
> >>> +                                 : 0) |
> >>>              0);
> >>>  }
> >>
> >> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> >> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> >> description it wants clarifying why it is acceptable to run Xen with the
> >> guest chosen settings for at least the DIS_S bit (assuming that it is
> >> okay to do so). Likely (didn't look there yet) also applicable to the
> >> further two patches.
> > 
> > "The added feature is made dependent on IBRSB, which ensures it will
> > only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> > ensures the value of SPEC_CTRL will get context switched on exit/entry
> > to guest."
> > 
> > Would adding the above to the commit message clarify the intended
> > implementation?
> 
> It would improve things, at least hinting towards there being a connection
> between exposure and updating on entry to Xen. I'd like to ask though to
> avoid "context switch" when talking about entry from guest context. While
> in a way technically correct, our normal meaning of the term is the
> process of switching vCPU-s out/in on a pCPU.

"The added feature is made dependent on IBRSB, which ensures it will
only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
ensures the value of SPEC_CTRL will get toggled between guest and Xen
values on exit/entry to guest."

But I wonder, we already allow guests the play with other SPEC_CTRL
bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
to Xen, so I'm unsure why adding more bits needs so much
justification.

Thanks, Roger.

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Jan Beulich]

On 30.01.2024 15:35, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
>> On 30.01.2024 13:06, Roger Pau Monné wrote:
>>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>>>> On 30.01.2024 10:13, Roger Pau Monne wrote:
>>>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
>>>>> SPEC_CTRL MSR.
>>>>>
>>>>> Note that those controls are not used by the hypervisor.
>>>>
>>>> Despite this, ...
>>>>
>>>>> --- a/xen/arch/x86/msr.c
>>>>> +++ b/xen/arch/x86/msr.c
>>>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>>>>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>>>>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>>>>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
>>>>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
>>>>> +                                    SPEC_CTRL_IPRED_DIS_S)
>>>>> +                                 : 0) |
>>>>>              0);
>>>>>  }
>>>>
>>>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>>>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>>>> description it wants clarifying why it is acceptable to run Xen with the
>>>> guest chosen settings for at least the DIS_S bit (assuming that it is
>>>> okay to do so). Likely (didn't look there yet) also applicable to the
>>>> further two patches.
>>>
>>> "The added feature is made dependent on IBRSB, which ensures it will
>>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
>>> ensures the value of SPEC_CTRL will get context switched on exit/entry
>>> to guest."
>>>
>>> Would adding the above to the commit message clarify the intended
>>> implementation?
>>
>> It would improve things, at least hinting towards there being a connection
>> between exposure and updating on entry to Xen. I'd like to ask though to
>> avoid "context switch" when talking about entry from guest context. While
>> in a way technically correct, our normal meaning of the term is the
>> process of switching vCPU-s out/in on a pCPU.
> 
> "The added feature is made dependent on IBRSB, which ensures it will
> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> ensures the value of SPEC_CTRL will get toggled between guest and Xen
> values on exit/entry to guest."
> 
> But I wonder, we already allow guests the play with other SPEC_CTRL
> bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
> to Xen, so I'm unsure why adding more bits needs so much
> justification.

Well, yes, I'm sorry, it was me forgetting the open-coded effect
SC_MSR_{PV,HVM} has on exposing of the MSR. I guess I'd be happy with
extending the last sentence a little, maybe "Note that those controls
are not used by the hypervisor, and they're cleared on entry to Xen."
If you're okay with that, I'd be happy to adjust while committing
(and assuming no other concerns are raised):
Reviewed-by: Jan Beulich <jbeulich@suse.com>
for all three patches.

Jan

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 03:47:37PM +0100, Jan Beulich wrote:
> On 30.01.2024 15:35, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 01:59:14PM +0100, Jan Beulich wrote:
> >> On 30.01.2024 13:06, Roger Pau Monné wrote:
> >>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
> >>>> On 30.01.2024 10:13, Roger Pau Monne wrote:
> >>>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
> >>>>> SPEC_CTRL MSR.
> >>>>>
> >>>>> Note that those controls are not used by the hypervisor.
> >>>>
> >>>> Despite this, ...
> >>>>
> >>>>> --- a/xen/arch/x86/msr.c
> >>>>> +++ b/xen/arch/x86/msr.c
> >>>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
> >>>>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
> >>>>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
> >>>>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
> >>>>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
> >>>>> +                                    SPEC_CTRL_IPRED_DIS_S)
> >>>>> +                                 : 0) |
> >>>>>              0);
> >>>>>  }
> >>>>
> >>>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
> >>>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
> >>>> description it wants clarifying why it is acceptable to run Xen with the
> >>>> guest chosen settings for at least the DIS_S bit (assuming that it is
> >>>> okay to do so). Likely (didn't look there yet) also applicable to the
> >>>> further two patches.
> >>>
> >>> "The added feature is made dependent on IBRSB, which ensures it will
> >>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> >>> ensures the value of SPEC_CTRL will get context switched on exit/entry
> >>> to guest."
> >>>
> >>> Would adding the above to the commit message clarify the intended
> >>> implementation?
> >>
> >> It would improve things, at least hinting towards there being a connection
> >> between exposure and updating on entry to Xen. I'd like to ask though to
> >> avoid "context switch" when talking about entry from guest context. While
> >> in a way technically correct, our normal meaning of the term is the
> >> process of switching vCPU-s out/in on a pCPU.
> > 
> > "The added feature is made dependent on IBRSB, which ensures it will
> > only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
> > ensures the value of SPEC_CTRL will get toggled between guest and Xen
> > values on exit/entry to guest."
> > 
> > But I wonder, we already allow guests the play with other SPEC_CTRL
> > bits, and Xen toggles the SPEC_CTRL values as required on entry/exit
> > to Xen, so I'm unsure why adding more bits needs so much
> > justification.
> 
> Well, yes, I'm sorry, it was me forgetting the open-coded effect
> SC_MSR_{PV,HVM} has on exposing of the MSR. I guess I'd be happy with
> extending the last sentence a little, maybe "Note that those controls
> are not used by the hypervisor, and they're cleared on entry to Xen."
> If you're okay with that, I'd be happy to adjust while committing

Sure.

> (and assuming no other concerns are raised):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> for all three patches.

Thanks.

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 01:55:56PM +0100, Jan Beulich wrote:
> On 30.01.2024 12:46, Roger Pau Monné wrote:
> > On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
> >> On 30.01.2024 11:27, Roger Pau Monne wrote:
> >>> Dummy set/clear tests for additional spec_ctrl bits.
> >>> ---
> >>>  docs/all-tests.dox  |   2 +
> >>>  tests/test/Makefile |   9 ++++
> >>>  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 111 insertions(+)
> >>>  create mode 100644 tests/test/Makefile
> >>>  create mode 100644 tests/test/main.c
> >>
> >> I'm puzzled: Why "test"? That doesn't describe in any way what this test
> >> is about.
> > 
> > That's just my place holder for random XTF stuff.  I don't intend this
> > to be committed.
> 
> Could have been said then one way or another.

Yes, realized later when speaking with Andrew that I had forgot to send
the test I've used, and then didn't adjust the message when sending to
note this wasn't supposed to be applied.

Regards, Roger.

Re: [PATCH] XTF: tests SPEC_CTRL added bits

[Andrew Cooper]

On 30/01/2024 3:02 pm, Roger Pau Monné wrote:
> On Tue, Jan 30, 2024 at 01:55:56PM +0100, Jan Beulich wrote:
>> On 30.01.2024 12:46, Roger Pau Monné wrote:
>>> On Tue, Jan 30, 2024 at 11:42:43AM +0100, Jan Beulich wrote:
>>>> On 30.01.2024 11:27, Roger Pau Monne wrote:
>>>>> Dummy set/clear tests for additional spec_ctrl bits.
>>>>> ---
>>>>>  docs/all-tests.dox  |   2 +
>>>>>  tests/test/Makefile |   9 ++++
>>>>>  tests/test/main.c   | 100 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 111 insertions(+)
>>>>>  create mode 100644 tests/test/Makefile
>>>>>  create mode 100644 tests/test/main.c
>>>> I'm puzzled: Why "test"? That doesn't describe in any way what this test
>>>> is about.
>>> That's just my place holder for random XTF stuff.  I don't intend this
>>> to be committed.
>> Could have been said then one way or another.
> Yes, realized later when speaking with Andrew that I had forgot to send
> the test I've used, and then didn't adjust the message when sending to
> note this wasn't supposed to be applied.

I've got a local test with some of this in, which I'll extend.

But as with many other things, it's waiting on fixing the test-revision
build infrastructure so the OSSTest Bisector doesn't break when adding
new content to an existing test.

~Andrew

Re: [PATCH 1/3] x86/intel: expose IPRED_CTRL to guests

[Andrew Cooper]

On 30/01/2024 12:59 pm, Jan Beulich wrote:
> On 30.01.2024 13:06, Roger Pau Monné wrote:
>> On Tue, Jan 30, 2024 at 11:57:17AM +0100, Jan Beulich wrote:
>>> On 30.01.2024 10:13, Roger Pau Monne wrote:
>>>> The CPUID feature bit signals the presence of the IPRED_DIS_{U,S} controls in
>>>> SPEC_CTRL MSR.
>>>>
>>>> Note that those controls are not used by the hypervisor.
>>> Despite this, ...
>>>
>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -324,6 +324,9 @@ uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>>>>      return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>>>>              (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>>>>              (psfd       ? SPEC_CTRL_PSFD       : 0) |
>>>> +            (cp->feat.ipred_ctrl ? (SPEC_CTRL_IPRED_DIS_U |
>>>> +                                    SPEC_CTRL_IPRED_DIS_S)
>>>> +                                 : 0) |
>>>>              0);
>>>>  }
>>> ... if I'm not mistaken exposing SPEC_CTRL bits to guests is independent
>>> of whether we write SPEC_CTRL on entry to Xen. Therefore I think in the
>>> description it wants clarifying why it is acceptable to run Xen with the
>>> guest chosen settings for at least the DIS_S bit (assuming that it is
>>> okay to do so). Likely (didn't look there yet) also applicable to the
>>> further two patches.
>> "The added feature is made dependent on IBRSB, which ensures it will
>> only be exposed if X86_FEATURE_SC_MSR_{PV,HVM} is available, and that
>> ensures the value of SPEC_CTRL will get context switched on exit/entry
>> to guest."
>>
>> Would adding the above to the commit message clarify the intended
>> implementation?
> It would improve things, at least hinting towards there being a connection
> between exposure and updating on entry to Xen. I'd like to ask though to
> avoid "context switch" when talking about entry from guest context. While
> in a way technically correct, our normal meaning of the term is the
> process of switching vCPU-s out/in on a pCPU.

The guests can only see MSR_SPEC_CTRL when Xen knows (and is) context
switching them properly.

The logic is weird for legacy IBRS reasons, but I don't think any
further justification is necessary.

~Andrew

Re: [PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Andrew Cooper]

On 30/01/2024 9:13 am, Roger Pau Monne wrote:
> Roger Pau Monne (3):
>   x86/intel: expose IPRED_CTRL to guests
>   x86/intel: expose RRSBA_CTRL to guests
>   x86/intel: expose BHI_CTRL to guests

A couple of things.  First,

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index a04a11858045..382bc07785d0 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -309,8 +309,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr,
uint64_t *val)
 
 /*
  * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
- * separate CPUID features for this functionality, but only set will be
- * active.
+ * separate CPUID features for some of this functionality, but only one set
+ * will be active on a single host.
  */
 uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
 {


There was a typo (missing the one in "but only one set"), but you're
also adding bits now which are Intel-only and very likely to stay that way.

IPRED_CTRL finally gives Intel CPUs a control with a similar security
property to AMD IBRS;  i.e. I doubt AMD are going to gain support for
these bits when they already guarantee that property and have done for
years already.


Next, I can't say that I particularly love that indentation.  This seems
marginally less bad

    return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
            (ssbd       ? SPEC_CTRL_SSBD       : 0) |
            (psfd       ? SPEC_CTRL_PSFD       : 0) |
            (cp->feat.ipred_ctrl
             ? (SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S) : 0) |
            (cp->feat.rrsba_ctrl
             ? (SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S) : 0) |
            (cp->feat.bhi_ctrl ? SPEC_CTRL_BHI_DIS_S : 0) |
            0);

insofar as at least it's fewer lines.  Given the length of these new
constants, I can't think of anything better.

Happy to fix both on commit.

~Andrew

Re: [PATCH 0/3] x86/intel: expose additional SPEC_CTRL MSR controls

[Roger Pau Monné]

On Tue, Jan 30, 2024 at 04:25:40PM +0000, Andrew Cooper wrote:
> On 30/01/2024 9:13 am, Roger Pau Monne wrote:
> > Roger Pau Monne (3):
> >   x86/intel: expose IPRED_CTRL to guests
> >   x86/intel: expose RRSBA_CTRL to guests
> >   x86/intel: expose BHI_CTRL to guests
> 
> A couple of things.  First,
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index a04a11858045..382bc07785d0 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -309,8 +309,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr,
> uint64_t *val)
>  
>  /*
>   * Caller to confirm that MSR_SPEC_CTRL is available.  Intel and AMD have
> - * separate CPUID features for this functionality, but only set will be
> - * active.
> + * separate CPUID features for some of this functionality, but only one set
> + * will be active on a single host.
>   */
>  uint64_t msr_spec_ctrl_valid_bits(const struct cpu_policy *cp)
>  {
> 
> 
> There was a typo (missing the one in "but only one set"), but you're
> also adding bits now which are Intel-only and very likely to stay that way.

Oh, didn't realize the existing typo.

> IPRED_CTRL finally gives Intel CPUs a control with a similar security
> property to AMD IBRS;  i.e. I doubt AMD are going to gain support for
> these bits when they already guarantee that property and have done for
> years already.
> 
> 
> Next, I can't say that I particularly love that indentation.  This seems
> marginally less bad
> 
>     return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP |
>             (ssbd       ? SPEC_CTRL_SSBD       : 0) |
>             (psfd       ? SPEC_CTRL_PSFD       : 0) |
>             (cp->feat.ipred_ctrl
>              ? (SPEC_CTRL_IPRED_DIS_U | SPEC_CTRL_IPRED_DIS_S) : 0) |
>             (cp->feat.rrsba_ctrl
>              ? (SPEC_CTRL_RRSBA_DIS_U | SPEC_CTRL_RRSBA_DIS_S) : 0) |
>             (cp->feat.bhi_ctrl ? SPEC_CTRL_BHI_DIS_S : 0) |
>             0);
> 
> insofar as at least it's fewer lines.  Given the length of these new
> constants, I can't think of anything better.

I prefer my indentation, but it adds an extra line which might not be
desirable.

Feel free to adjust on commit.

Regards, Roger.