[v2,1/2] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

Commit Message

Shaoqin Huang June 18, 2024, 6:38 a.m. UTC

Allow userspace to change the guest-visible value of the register with
some severe limitation:

  - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
---
 arch/arm64/kvm/sys_regs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Marc Zyngier June 18, 2024, 7:39 a.m. UTC | #1

On Tue, 18 Jun 2024 07:38:06 +0100,
Shaoqin Huang <[e-mail]> wrote:
> 
> Allow userspace to change the guest-visible value of the register with
> some severe limitation:
> 
>   - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
> ---
>  arch/arm64/kvm/sys_regs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22b45a15d068..bead81867bce 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  		   ID_AA64PFR0_EL1_GIC |
>  		   ID_AA64PFR0_EL1_AdvSIMD |
>  		   ID_AA64PFR0_EL1_FP), },
> -	ID_SANITISED(ID_AA64PFR1_EL1),
> +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
> +				       ID_AA64PFR1_EL1_MPAM_frac)),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
>  	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),

This isn't a valid patch.

Furthermore, how about all the other features that may or may not be
currently handled by KVM? Please see [1] and make sure that all
existing fields have a known behaviour (a combination of masked,
preserved, capped, writable or read-only).

I can at least see problems with MTE_frac and MTEX, plus all the other
things that KVM doesn't know how to save/restore (THE, GCS, NMI...).

What I asked you to handle the whole register, I really meant it.

	M.

[1] https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Registers/ID-AA64PFR1-EL1--AArch64-Processor-Feature-Register-1?lang=en

Shaoqin Huang June 19, 2024, 3:14 a.m. UTC | #2

Hi Marc,

On 6/18/24 15:39, Marc Zyngier wrote:
> On Tue, 18 Jun 2024 07:38:06 +0100,
> Shaoqin Huang <[e-mail]> wrote:
>>
>> Allow userspace to change the guest-visible value of the register with
>> some severe limitation:
>>
>>    - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
>> ---
>>   arch/arm64/kvm/sys_regs.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 22b45a15d068..bead81867bce 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>   		   ID_AA64PFR0_EL1_GIC |
>>   		   ID_AA64PFR0_EL1_AdvSIMD |
>>   		   ID_AA64PFR0_EL1_FP), },
>> -	ID_SANITISED(ID_AA64PFR1_EL1),
>> +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
>> +				       ID_AA64PFR1_EL1_MPAM_frac)),
>>   	ID_UNALLOCATED(4,2),
>>   	ID_UNALLOCATED(4,3),
>>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> 
> This isn't a valid patch.
> 
> Furthermore, how about all the other features that may or may not be
> currently handled by KVM? Please see [1] and make sure that all
> existing fields have a known behaviour (a combination of masked,
> preserved, capped, writable or read-only).
> 
> I can at least see problems with MTE_frac and MTEX, plus all the other
> things that KVM doesn't know how to save/restore (THE, GCS, NMI...).
> 
> What I asked you to handle the whole register, I really meant it.

Thanks for pointing out the problem. I will figure those field out and 
update this patch later on.

> 
> 	M.
> 
> [1] https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Registers/ID-AA64PFR1-EL1--AArch64-Processor-Feature-Register-1?lang=en

Ok I found that I used the obsolete ARM document so I didn't see the 
MTE_frac and MTEX. Thanks for letting me know the latest ARM document.

Thanks,
Shaoqin

>

Shaoqin Huang June 21, 2024, 6:17 a.m. UTC | #3

Hi Marc,

On 6/18/24 15:39, Marc Zyngier wrote:
> On Tue, 18 Jun 2024 07:38:06 +0100,
> Shaoqin Huang <[e-mail]> wrote:
>>
>> Allow userspace to change the guest-visible value of the register with
>> some severe limitation:
>>
>>    - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
>> ---
>>   arch/arm64/kvm/sys_regs.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 22b45a15d068..bead81867bce 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>   		   ID_AA64PFR0_EL1_GIC |
>>   		   ID_AA64PFR0_EL1_AdvSIMD |
>>   		   ID_AA64PFR0_EL1_FP), },
>> -	ID_SANITISED(ID_AA64PFR1_EL1),
>> +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
>> +				       ID_AA64PFR1_EL1_MPAM_frac)),
>>   	ID_UNALLOCATED(4,2),
>>   	ID_UNALLOCATED(4,3),
>>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> 
> This isn't a valid patch.
> 
> Furthermore, how about all the other features that may or may not be
> currently handled by KVM? Please see [1] and make sure that all
> existing fields have a known behaviour (a combination of masked,
> preserved, capped, writable or read-only).
> 
> I can at least see problems with MTE_frac and MTEX, plus all the other
> things that KVM doesn't know how to save/restore (THE, GCS, NMI...).
> 
> What I asked you to handle the whole register, I really meant it.

I currently only found the BT and SSBS fields can be written without any 
unknown behavior.

All other fields in the ID_AA64PFR1_EL1 are either not supported by KVM 
or the field involved with other register and KVM don't know how to 
handle them.

I'm not sure if this is right. So I want to ask about your opinion about 
how to allow each of the field to be writable in the ID_AA64PFR1_EL1 
register?

Thanks,
Shaoqin

> 
> 	M.
> 
> [1] https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Registers/ID-AA64PFR1-EL1--AArch64-Processor-Feature-Register-1?lang=en
>

Marc Zyngier June 21, 2024, 7:53 a.m. UTC | #4

On Fri, 21 Jun 2024 07:17:57 +0100,
Shaoqin Huang <[e-mail]> wrote:
> 
> Hi Marc,
> 
> On 6/18/24 15:39, Marc Zyngier wrote:
> > On Tue, 18 Jun 2024 07:38:06 +0100,
> > Shaoqin Huang <[e-mail]> wrote:
> >> 
> >> Allow userspace to change the guest-visible value of the register with
> >> some severe limitation:
> >> 
> >>    - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
> >> ---
> >>   arch/arm64/kvm/sys_regs.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 22b45a15d068..bead81867bce 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >>   		   ID_AA64PFR0_EL1_GIC |
> >>   		   ID_AA64PFR0_EL1_AdvSIMD |
> >>   		   ID_AA64PFR0_EL1_FP), },
> >> -	ID_SANITISED(ID_AA64PFR1_EL1),
> >> +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
> >> +				       ID_AA64PFR1_EL1_MPAM_frac)),
> >>   	ID_UNALLOCATED(4,2),
> >>   	ID_UNALLOCATED(4,3),
> >>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> > 
> > This isn't a valid patch.
> > 
> > Furthermore, how about all the other features that may or may not be
> > currently handled by KVM? Please see [1] and make sure that all
> > existing fields have a known behaviour (a combination of masked,
> > preserved, capped, writable or read-only).
> > 
> > I can at least see problems with MTE_frac and MTEX, plus all the other
> > things that KVM doesn't know how to save/restore (THE, GCS, NMI...).
> > 
> > What I asked you to handle the whole register, I really meant it.
> 
> I currently only found the BT and SSBS fields can be written without
> any unknown behavior.

I can only assume you haven't looked hard enough.

> 
> All other fields in the ID_AA64PFR1_EL1 are either not supported by
> KVM or the field involved with other register and KVM don't know how
> to handle them.

Why can't CSV2_frac be writable? Why can't most of the other fields be
hidden depending on the VM configuration, as pointed out above?

	M.

Shaoqin Huang June 26, 2024, 7:33 a.m. UTC | #5

Hi Marc,

On 6/21/24 15:53, Marc Zyngier wrote:
> On Fri, 21 Jun 2024 07:17:57 +0100,
> Shaoqin Huang <[e-mail]> wrote:
>>
>> Hi Marc,
>>
>> On 6/18/24 15:39, Marc Zyngier wrote:
>>> On Tue, 18 Jun 2024 07:38:06 +0100,
>>> Shaoqin Huang <[e-mail]> wrote:
>>>>
>>>> Allow userspace to change the guest-visible value of the register with
>>>> some severe limitation:
>>>>
>>>>     - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac)
>>>> ---
>>>>    arch/arm64/kvm/sys_regs.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 22b45a15d068..bead81867bce 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>    		   ID_AA64PFR0_EL1_GIC |
>>>>    		   ID_AA64PFR0_EL1_AdvSIMD |
>>>>    		   ID_AA64PFR0_EL1_FP), },
>>>> -	ID_SANITISED(ID_AA64PFR1_EL1),
>>>> +	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
>>>> +				       ID_AA64PFR1_EL1_MPAM_frac)),
>>>>    	ID_UNALLOCATED(4,2),
>>>>    	ID_UNALLOCATED(4,3),
>>>>    	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
>>>
>>> This isn't a valid patch.
>>>
>>> Furthermore, how about all the other features that may or may not be
>>> currently handled by KVM? Please see [1] and make sure that all
>>> existing fields have a known behaviour (a combination of masked,
>>> preserved, capped, writable or read-only).
>>>
>>> I can at least see problems with MTE_frac and MTEX, plus all the other
>>> things that KVM doesn't know how to save/restore (THE, GCS, NMI...).
>>>
>>> What I asked you to handle the whole register, I really meant it.
>>
>> I currently only found the BT and SSBS fields can be written without
>> any unknown behavior.
> 
> I can only assume you haven't looked hard enough.
> 
>>
>> All other fields in the ID_AA64PFR1_EL1 are either not supported by
>> KVM or the field involved with other register and KVM don't know how
>> to handle them.
> 
> Why can't CSV2_frac be writable? Why can't most of the other fields be
> hidden depending on the VM configuration, as pointed out above?

I looked at the "struct arm64_ftr_bits ftr_id_aa64pfr1[]" in the 
kernel/cpufeature.c, I don't see the CSV2_frac has been supported on ARM 
bare-mental. In this situation, can we first support it in KVM? If so, 
how can we do that, I don't understand that, could you give me some 
hints about that.

Other fields are same with CSV2_frac I think. The KVM don't know the 
configuration about them. Why we should allow them writable and hidden 
them right now? Instead of just make them still unwrittable?

Thanks,
Shaoqin

> 
> 	M.
>

Shaoqin Huang June 26, 2024, 9:01 a.m. UTC | #6

On 6/26/24 15:33, Shaoqin Huang wrote:
> Hi Marc,
> 
> On 6/21/24 15:53, Marc Zyngier wrote:
>> On Fri, 21 Jun 2024 07:17:57 +0100,
>> Shaoqin Huang <[e-mail]> wrote:
>>>
>>> Hi Marc,
>>>
>>> On 6/18/24 15:39, Marc Zyngier wrote:
>>>> On Tue, 18 Jun 2024 07:38:06 +0100,
>>>> Shaoqin Huang <[e-mail]> wrote:
>>>>>
>>>>> Allow userspace to change the guest-visible value of the register with
>>>>> some severe limitation:
>>>>>
>>>>>     - No changes to features not virtualized by KVM (MPAM_frac, 
>>>>> RAS_frac)
>>>>> ---
>>>>>    arch/arm64/kvm/sys_regs.c | 3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>>> index 22b45a15d068..bead81867bce 100644
>>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>>> @@ -2306,7 +2306,8 @@ static const struct sys_reg_desc 
>>>>> sys_reg_descs[] = {
>>>>>               ID_AA64PFR0_EL1_GIC |
>>>>>               ID_AA64PFR0_EL1_AdvSIMD |
>>>>>               ID_AA64PFR0_EL1_FP), },
>>>>> -    ID_SANITISED(ID_AA64PFR1_EL1),
>>>>> +    ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
>>>>> +                       ID_AA64PFR1_EL1_MPAM_frac)),
>>>>>        ID_UNALLOCATED(4,2),
>>>>>        ID_UNALLOCATED(4,3),
>>>>>        ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
>>>>
>>>> This isn't a valid patch.
>>>>
>>>> Furthermore, how about all the other features that may or may not be
>>>> currently handled by KVM? Please see [1] and make sure that all
>>>> existing fields have a known behaviour (a combination of masked,
>>>> preserved, capped, writable or read-only).
>>>>
>>>> I can at least see problems with MTE_frac and MTEX, plus all the other
>>>> things that KVM doesn't know how to save/restore (THE, GCS, NMI...).
>>>>
>>>> What I asked you to handle the whole register, I really meant it.
>>>
>>> I currently only found the BT and SSBS fields can be written without
>>> any unknown behavior.
>>
>> I can only assume you haven't looked hard enough.
>>
>>>
>>> All other fields in the ID_AA64PFR1_EL1 are either not supported by
>>> KVM or the field involved with other register and KVM don't know how
>>> to handle them.
>>
>> Why can't CSV2_frac be writable? Why can't most of the other fields be
>> hidden depending on the VM configuration, as pointed out above?
> 
> I looked at the "struct arm64_ftr_bits ftr_id_aa64pfr1[]" in the 
> kernel/cpufeature.c, I don't see the CSV2_frac has been supported on ARM 
> bare-mental. In this situation, can we first support it in KVM? If so, 
> how can we do that, I don't understand that, could you give me some 
> hints about that.

More description about the CSV2_frac.

static const struct arm64_ftr_bits ftr_id_aa64pfr1[] = {
	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_SME),
		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_EL1_SME_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
ID_AA64PFR1_EL1_MPAM_frac_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
ID_AA64PFR1_EL1_RAS_frac_SHIFT, 4, 0),
	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_MTE),
		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_EL1_MTE_SHIFT, 4, 
ID_AA64PFR1_EL1_MTE_NI),
	ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_LOWER_SAFE, 
ID_AA64PFR1_EL1_SSBS_SHIFT, 4, ID_AA64PFR1_EL1_SSBS_NI),
	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_BTI),
				    FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_EL1_BT_SHIFT, 4, 0),
	ARM64_FTR_END,
};

If we make the CSV2_frac writable, there isn't definition about 
CSV2_frac in the ftr_id_aa64pfr1. And the KVM really depends on the 
ftr_id_aa64pfr1 in the arm64_check_features() function. We're missing 
the CSV2_frac.

So the arm64_check_features() will not check the CSV2_frac if it's 
writable, and the user can write any value into it.

Thanks,
Shaoqin

> 
> Other fields are same with CSV2_frac I think. The KVM don't know the 
> configuration about them. Why we should allow them writable and hidden 
> them right now? Instead of just make them still unwrittable? >
> Thanks,
> Shaoqin
> 
>>
>>     M.
>>
>

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 22b45a15d068..bead81867bce 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2306,7 +2306,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 		   ID_AA64PFR0_EL1_GIC |
 		   ID_AA64PFR0_EL1_AdvSIMD |
 		   ID_AA64PFR0_EL1_FP), },
-	ID_SANITISED(ID_AA64PFR1_EL1),
+	ID_WRITABLE(ID_AA64PFR1_EL1, ~(ID_AA64PFR1_EL1_RAS_frac |
+				       ID_AA64PFR1_EL1_MPAM_frac)),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),