Re: [PATCH] Remove support for ThumbEE

[Julien Grall <julien@xxxxxxx>]

On 13/04/2021 10:59, Michal Orzel wrote:
> On 13.04.2021 11:42, Julien Grall wrote:
> > On 13/04/2021 10:32, Michal Orzel wrote:
> > > Hi Julien,
> > >
> > > On 13.04.2021 11:07, Julien Grall wrote:
> > > > Hi Michal,
> > > >
> > > > On 13/04/2021 09:24, Michal Orzel wrote:
> > > > > ThumbEE(T32EE) was introduced in ARMv7 and removed in ARMv8.
> > > > > In 2011 ARM deprecated any use of the ThumbEE instruction set.
> > > > This doesn't mean this is not present in any CPU. In fact, in the same section 
> > > > (see A2.10 in ARM DDI 0406C.d):
> > > >
> > > > "ThumbEE is both the name of the instruction set and the name of the extension 
> > > > that provides support for that
> > > > instruction set. The ThumbEE Extension is:
> > > >     - Required in implementations of the ARMv7-A profile.
> > > >     - Optional in implementations of the ARMv7-R profile.
> > > > "
> > > >
> > > > > This feature is untested and as per my understanding
> > > > > there are no reported users for it. >
> > > > > Remove all the bits related to it.
> > > > >
> > > > > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx> > ---
> > > > >     xen/arch/arm/cpufeature.c        |  3 +++
> > > > >     xen/arch/arm/domain.c            | 12 ------------
> > > > >     xen/arch/arm/setup.c             |  3 +--
> > > > >     xen/include/asm-arm/cpregs.h     |  6 ------
> > > > >     xen/include/asm-arm/cpufeature.h |  1 -
> > > > >     xen/include/asm-arm/domain.h     |  1 -
> > > > >     6 files changed, 4 insertions(+), 22 deletions(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> > > > > index 1d88783809..82265a72f4 100644
> > > > > --- a/xen/arch/arm/cpufeature.c
> > > > > +++ b/xen/arch/arm/cpufeature.c
> > > > > @@ -209,6 +209,9 @@ static int __init create_guest_cpuinfo(void)
> > > > >         guest_cpuinfo.pfr32.ras = 0;
> > > > >         guest_cpuinfo.pfr32.ras_frac = 0;
> > > > >     +    /* Hide ThumbEE support */
> > > > > +    guest_cpuinfo.pfr32.thumbee = 0;
> > > > Even if you hide the feature from the guest, the registers are still 
> > > > accessible. So you are not removing support but just opening a potential 
> > > > security hole as the registers now gets shared...
> > > >
> > > > Looking at the spec, it doesn't look like it is possible to trap them.
> > > Looking at the spec for ARMv7A/R:
> > > https://developer.arm.com/documentation/ddi0406/c/System-Level-Architecture/System-Control-Registers-in-a-VMSA-implementation/VMSA-System-control-registers-descriptions--in-register-order/HSTR--Hyp-System-Trap-Register--Virtualization-Extensions
> > > we can trap Thumbee operations.
> > > This means that we will not open the security hole.
> > That's for pointing that out. However, I am not still not sure why you want to 
> > drop the code. Yes this is technically untested, but:
> >    1) The change is very small
> >    2) There are CPU out there supporting it (it is mandated after all).
> I wanted to drop the support for thumbee due to the following reasons:
> -it is untested
> -it is deprecated
Deprecated only means new code should avoid to use the instruction set. 
But it is still present for existing code until they are migrated to a 
newer instruction set.
However, as I pointed out only the instruction set is deprecated. The 
extension itself is mandated by Armv7-A.
> -no reported users for this feature
We probably only see the tip of the iceberg in term of the users. If the 
feature/pain is quite small to the community, then we should try to not 
drop feature.
> -KVM did that
Well, KVM fully dropped 32-bit support. But I am not yet in favor of 
that in Xen.
> If you think we should not do this, then I am ok to abandon this patch.
Based on the information you provided, I think we should keep support.

Cheers,

--
Julien Grall