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

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

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.