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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jan 09, 2025 at 02:58:29PM +0000, Alejandro Vallejo wrote:
> 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@citrix.com>
> > ---
> > 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.

Since I have to modify the whole line I can indeed add the braces
later.

> > +
> > +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.

It's IMO a bit too vague to make such promises, but I can add:

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.  It might also offer some protection
against unmitigated speculation-related attacks.

> > +
> > +* `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?

Hm, indeed.  I can probably make them booleans then.  I wrongly
recall that checking whether they haven't been initialized was needed
somewhere, but that doesn't seem to be the case.

Thanks, Roger.