Re: [PATCH v2 13/18] x86/spec-ctrl: introduce Address Space Isolation command line option

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Wed Jan 8, 2025 at 2:26 PM GMT, Roger Pau Monne wrote:
> No functional change, as the option is not used.
>
> Introduced new so newly added functionality is keyed on the option being
> enabled, even if the feature is non-functional.
>
> When ASI is enabled for PV domains, printing the usage of XPTI might be
> omitted if it must be uniformly disabled given the usage of ASI.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Improve comments and documentation about what ASI provides.
>  - Do not print the XPTI information if ASI is used for pv domUs and dom0 is
>    PVH, or if ASI is used for both domU and dom0.
>
> FWIW, I would print the state of XPTI uniformly, as otherwise I find the 
> output
> might be confusing for user expecting to assert the state of XPTI.
> ---
>  docs/misc/xen-command-line.pandoc    |  19 +++++
>  xen/arch/x86/include/asm/domain.h    |   3 +
>  xen/arch/x86/include/asm/spec_ctrl.h |   2 +
>  xen/arch/x86/spec_ctrl.c             | 115 +++++++++++++++++++++++++--
>  4 files changed, 133 insertions(+), 6 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 08b0053f9ced..3c1ad7b5fe7d 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -202,6 +202,25 @@ to appropriate auditing by Xen.  Argo is disabled by 
> default.
>      This option is disabled by default, to protect domains from a DoS by a
>      buggy or malicious other domain spamming the ring.
>  
> +### asi (x86)
> +> `= List of [ <bool>, {pv,hvm}=<bool>,
> +               {vcpu-pt}=<bool>|{pv,hvm}=<bool> ]`

nit: While this grows later, the braces around vcpu-pt aren't strictly needed 
here.

> +
> +Offers control over whether the hypervisor will engage in Address Space
> +Isolation, by not having potentially sensitive information permanently mapped
> +in the VMM page-tables.  Using this option might avoid the need to apply
> +mitigations for certain speculative related attacks, at the cost of mapping
> +sensitive information on-demand.

Might be worth mentioning that this provides some defense in depth against
unmitigated attacks too.

> +
> +* `pv=` and `hvm=` sub-options allow enabling for specific guest types.
> +
> +**WARNING: manual de-selection of enabled options will invalidate any
> +protection offered by the feature.  The fine grained options provided below 
> are
> +meant to be used for debugging purposes only.**
> +
> +* `vcpu-pt` ensure each vCPU uses a unique top-level page-table and setup a
> +  virtual address space region to map memory on a per-vCPU basis.
> +
>  ### asid (x86)
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> index ced84750015c..9463a8624701 100644
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -2075,6 +2165,19 @@ void __init init_speculation_mitigations(void)
>           hw_smt_enabled && default_xen_spec_ctrl )
>          setup_force_cpu_cap(X86_FEATURE_SC_MSR_IDLE);
>  
> +    /* Disable all ASI options by default until feature is finished. */
> +    if ( opt_vcpu_pt_pv == -1 )
> +        opt_vcpu_pt_pv = 0;
> +    if ( opt_vcpu_pt_hwdom == -1 )
> +        opt_vcpu_pt_hwdom = 0;
> +    if ( opt_vcpu_pt_hvm == -1 )
> +        opt_vcpu_pt_hvm = 0;

Why not preinitialise them to zero instead in the static declarations?

> +
> +    if ( opt_vcpu_pt_pv || opt_vcpu_pt_hvm )
> +        warning_add(
> +            "Address Space Isolation is not functional, this option is\n"
> +            "intended to be used only for development purposes.\n");
> +
>      xpti_init_default();
>  
>      l1tf_calculations();

Cheers,
Alejandro