Re: [PATCH 12/22] x86/spec-ctrl: introduce Address Space Isolation command line option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2024 17:21, Roger Pau Monne wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2387,7 +2387,7 @@ By default SSBD will be mitigated at runtime (i.e 
> `ssbd=runtime`).
>  
>  ### spec-ctrl (x86)
>  > `= List of [ <bool>, xen=<bool>, {pv,hvm}=<bool>,
> ->              {msr-sc,rsb,verw,{ibpb,bhb}-entry}=<bool>|{pv,hvm}=<bool>,
> +>              {msr-sc,rsb,verw,{ibpb,bhb}-entry,asi}=<bool>|{pv,hvm}=<bool>,

Is it really appropriate to hide this underneath an x86-only option? Even
of other architectures won't support it right away, they surely will want
to down the road? In which case making as much of this common right away
is probably the best we can do. This goes along with the question whether,
like e.g. "xpti", this should be a top-level option.

> @@ -2414,10 +2414,10 @@ in place for guests to use.
>  
>  Use of a positive boolean value for either of these options is invalid.
>  
> -The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=` and `bhb-entry=`
> -options offer fine grained control over the primitives by Xen.  These impact
> -Xen's ability to protect itself, and/or Xen's ability to virtualise support
> -for guests to use.
> +The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=`, `bhb-entry=` 
> and
> +`asi=` options offer fine grained control over the primitives by Xen.  These

Here, ahead of "by Xen", it looks like "used" was missing. Maybe a good
opportunity to add it?

> @@ -2449,6 +2449,11 @@ for guests to use.
>    is not available (see `bhi-dis-s`).  The choice of scrubbing sequence can 
> be
>    selected using the `bhb-seq=` option.  If it is necessary to protect dom0
>    too, boot with `spec-ctrl=bhb-entry`.
> +* `asi=` offers control over whether the hypervisor will engage in Address
> +  Space Isolation, by not having sensitive information mapped in the VMM
> +  page-tables.  Not having sensitive information on the page-tables avoids
> +  having to perform some mitigations for speculative attacks when
> +  context-switching to the hypervisor.

Is "not having" and ...

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -458,6 +458,9 @@ struct arch_domain
>      /* Don't unconditionally inject #GP for unhandled MSRs. */
>      bool msr_relaxed;
>  
> +    /* Run the guest without sensitive information in the VMM page-tables. */
> +    bool asi;

... "without" really going to be fully true? Wouldn't we better say "as little
as possible" or alike?

> @@ -143,6 +148,10 @@ static int __init cf_check parse_spec_ctrl(const char *s)
>              opt_unpriv_mmio = false;
>              opt_gds_mit = 0;
>              opt_div_scrub = 0;
> +
> +            opt_asi_pv = 0;
> +            opt_asi_hwdom = 0;
> +            opt_asi_hvm = 0;
>          }
>          else if ( val > 0 )
>              rc = -EINVAL;

I'm frequently in trouble when deciding where the split between "=no" and
"=xen" should be. opt_xpti_* are cleared ahead of the disable_common label;
considering the similarity I wonder whether the same should be true for ASI
(as this is also or even mainly about protecting guests from one another),
or whether the XPTI placement is actually wrong.

> @@ -378,6 +410,13 @@ int8_t __ro_after_init opt_xpti_domu = -1;
>  
>  static __init void xpti_init_default(void)
>  {
> +    ASSERT(opt_asi_pv >= 0 && opt_asi_hwdom >= 0);
> +    if ( (opt_xpti_hwdom == 1 || opt_xpti_domu == 1) && opt_asi_pv == 1 )

There is a separate opt_asi_hwdom which isn't used here, but only ...

> +    {
> +        printk(XENLOG_ERR
> +               "XPTI is incompatible with Address Space Isolation - 
> disabling ASI\n");
> +        opt_asi_pv = 0;
> +    }
>      if ( (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ||
>           cpu_has_rdcl_no )
>      {
> @@ -389,9 +428,9 @@ static __init void xpti_init_default(void)
>      else
>      {
>          if ( opt_xpti_hwdom < 0 )
> -            opt_xpti_hwdom = 1;
> +            opt_xpti_hwdom = !opt_asi_hwdom;
>          if ( opt_xpti_domu < 0 )
> -            opt_xpti_domu = 1;
> +            opt_xpti_domu = !opt_asi_pv;
>      }

... here?

It would further seem desirable to me if opt_asi_hwdom had its default set
later, when we know the kind of Dom0, such that it could be defaulted to
what opt_asi_{hvm,pv} are set to. This, however, wouldn't be compatible
with the use here. Perhaps the invocation of xpti_init_default() would
need deferring, too.

> @@ -643,22 +683,24 @@ static void __init print_details(enum ind_thunk thunk)
>             opt_eager_fpu                             ? " EAGER_FPU"     : "",
>             opt_verw_hvm                              ? " VERW"          : "",
>             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM)  ? " IBPB-entry"    : "",
> -           opt_bhb_entry_hvm                         ? " BHB-entry"     : 
> "");
> +           opt_bhb_entry_hvm                         ? " BHB-entry"     : "",
> +           opt_asi_hvm                               ? " ASI"           : 
> "");
>  
>  #endif
>  #ifdef CONFIG_PV
> -    printk("  Support for PV VMs:%s%s%s%s%s%s%s\n",
> +    printk("  Support for PV VMs:%s%s%s%s%s%s%s%s\n",
>             (boot_cpu_has(X86_FEATURE_SC_MSR_PV) ||
>              boot_cpu_has(X86_FEATURE_SC_RSB_PV) ||
>              boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV) ||
> -            opt_bhb_entry_pv ||
> +            opt_bhb_entry_pv || opt_asi_pv ||
>              opt_eager_fpu || opt_verw_pv)            ? ""               : " 
> None",
>             boot_cpu_has(X86_FEATURE_SC_MSR_PV)       ? " MSR_SPEC_CTRL" : "",
>             boot_cpu_has(X86_FEATURE_SC_RSB_PV)       ? " RSB"           : "",
>             opt_eager_fpu                             ? " EAGER_FPU"     : "",
>             opt_verw_pv                               ? " VERW"          : "",
>             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV)   ? " IBPB-entry"    : "",
> -           opt_bhb_entry_pv                          ? " BHB-entry"     : 
> "");
> +           opt_bhb_entry_pv                          ? " BHB-entry"     : "",
> +           opt_asi_pv                                ? " ASI"           : 
> "");
>  
>      printk("  XPTI (64-bit PV only): Dom0 %s, DomU %s (with%s PCID)\n",
>             opt_xpti_hwdom ? "enabled" : "disabled",

Should this printk() perhaps be suppressed when ASI is in use?

Jan