Re: [PATCH] xen/arm64: Hide FEAT_SME

[Julien Grall <julien@xxxxxxx>]

On 15/08/2024 09:58, Ayan Kumar Halder wrote:
> Hi Julien,
Hi Ayan,

> On 14/08/2024 22:00, Julien Grall wrote:
> > CAUTION: This message has originated from an External Source. Please 
> > use proper judgment and caution when opening attachments, clicking 
> > links, or responding to this email.
> >
> > Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
> > it will still expose the feature to the VM. If the OS is trying to use
> > SME, then it will crash.
> >
> > Solve by hiding FEAT_SME.
> >
> > Signed-off-by: Julien Grall <julien@xxxxxxx>
> >
> > ---
> >
> > The current approach used to create the domain cpuinfo is to hide
> > (i.e. a denylist) what we know Xen is not supporting. The drawback
> > with this approach is for newly introduced feature, Xen will expose it
> > by default.
> >
> > If a kernel is trying to use it then it will crash. I can't really
> > make my mind whether it would be better to expose only what we support
> > (i.e. use an allowlist).
> >
> > AFAICT, there is no security concerns with the current approach because
> > ID_* registers are not a way to tell the kernel which features are
> > supported. A guest kernel could still try to access the new registers.
> >
> > So the most annoying bits is that booting Xen on a new HW may lead to
> > an OS crashing.
> > ---
> >   xen/arch/arm/cpufeature.c             | 3 +++
> >   xen/arch/arm/include/asm/cpufeature.h | 4 +++-
> >   2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > index ef77473bf8e3..b45dbe3c668d 100644
> > --- a/xen/arch/arm/cpufeature.c
> > +++ b/xen/arch/arm/cpufeature.c
> > @@ -208,6 +208,9 @@ static int __init create_domain_cpuinfo(void)
> >       domain_cpuinfo.pfr64.sve = 0;
> >       domain_cpuinfo.zfr64.bits[0] = 0;
> >
> > +    /* Hide SMT support as Xen does not support it */
> > +    domain_cpuinfo.pfr64.sme = 0;
> Instead of this, can we do the following :-
>
> domain_cpuinfo.pfr64.res1 = 0;
> This would imply that SME, RNDR_trap, CSV2_frac, NMI, etc are not 
> supported.
I could but I haven't done it for two reasons:
  * AFAICT, there are no issue to expose RNDR_trap to the guest. Also 
not all the bits in the field res1 are defined yet. So effectively, we 
would be implementing an allowlist like.
  * In the future, we could split res1 in two. If we are not careful, 
we would expose the feature again.
So I find this approach rather risky. There is also a more general 
question on how the features should be handled (see what I wrote after 
--- and below).
> If later Xen decides to support any of these, then they can be 
> selectively turned on for a domain in do_sysreg() 
do_sysreg() is just returning the ID registers. This only helps the OS 
to discover the features at runtime and it is free to ignore them. What 
matter is the configuration in of the trap registers (such as HCR_EL2).
If a feature is not gated by a trap register, then it could be enabled 
everywhere. So effectively, the decision will depend on the feature. In 
the future, we may have to take care of suspend/resume or live-migration 
between two Xen versions. At that point the feature may need to be 
per-domain.
> (Similar to SVE).

The main reason SVE is enabled per-domain is because of the large state. 
But if we have feature that doesn't have an impact on the memory usage, 
then they could be enabled everywhere.
Anyway, the first decision we need to take is whether we want to change 
to an allowlist. There are pros and cons with both of them (see what I 
wrote above). At the moment, I am still leaning towards the existing 
model which is expose everything by default but hide features when they 
appear and Xen needs more handling.
Cheers,

--
Julien Grall