Re: [PATCH] Remove support for ThumbEE

[Michal Orzel <michal.orzel@xxxxxxx>]

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
-no reported users for this feature
-KVM did that

If you think we should not do this, then I am ok to abandon this patch.