[PATCH v3 0/2] Allow userspace to change ID_AA64PFR1_EL1

[PATCH v3 0/2] Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

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,
    SME, RNDP_trap).

  - No changes to features (CSV2_frac, NMI, MTE_frac, GCS, THE, MTEX,
    DF2, PFAR) which haven't been added into the ftr_id_aa64pfr1[].
    Because the struct arm64_ftr_bits definition for each feature in the
    ftr_id_aa64pfr1[] is used by arm64_check_features. If they're not
    existing in the ftr_id_aa64pfr1[], the for loop won't check the if
    the new_val is safe for those features.

For the question why can't those fields be hidden depending on the VM
configuration? I don't find there is the related VM configuration, maybe we
should add the new VM configuration?

I'm not sure I'm right, so if there're any problems please help to point out and
I will fix them.

Also add the selftest for it.

Changelog:
----------
v2 -> v3:
  * Give more description about why only part of the fields can be writable.
  * Updated the writable mask by referring the latest ARM spec.

v1 -> v2:
  * Tackling the full register instead of single field.
  * Changing the patch title and commit message.

RFCv1 -> v1:
  * Fix the compilation error.
  * Delete the machine specific information and make the description more
    generable.

RFCv1: https://lore.kernel.org/all/20240612023553.127813-1-shahuang@redhat.com/
v1: https://lore.kernel.org/all/20240617075131.1006173-1-shahuang@redhat.com/
v2: https://lore.kernel.org/all/20240618063808.1040085-1-shahuang@redhat.com/

Shaoqin Huang (2):
  KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1
  KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1

 arch/arm64/kvm/sys_regs.c                         | 4 +++-
 tools/testing/selftests/kvm/aarch64/set_id_regs.c | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH v3 1/2] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

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,
    SME, RNDP_trap).

  - No changes to features (CSV2_frac, NMI, MTE_frac, GCS, THE, MTEX,
    DF2, PFAR) which haven't been added into the ftr_id_aa64pfr1[].
    Because the struct arm64_ftr_bits definition for each feature in the
    ftr_id_aa64pfr1[] is used by arm64_check_features. If they're not
    existing in the ftr_id_aa64pfr1[], the for loop won't check the if
    the new_val is safe for those features.
---
 arch/arm64/kvm/sys_regs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 22b45a15d068..159cded22357 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2306,7 +2306,9 @@ 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_MTE |
+				     ID_AA64PFR1_EL1_SSBS |
+				     ID_AA64PFR1_EL1_BT),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
-- 
2.40.1

[PATCH v3 2/2] KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1

[Shaoqin Huang]

Add writable test for the ID_AA64PFR1_EL1 register.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 tools/testing/selftests/kvm/aarch64/set_id_regs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
index a7de39fa2a0a..7fd4d6f26456 100644
--- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -133,6 +133,13 @@ static const struct reg_ftr_bits ftr_id_aa64pfr0_el1[] = {
 	REG_FTR_END,
 };
 
+static const struct reg_ftr_bits ftr_id_aa64pfr1_el1[] = {
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, MTE, ID_AA64PFR1_EL1_MTE_NI),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, SSBS, ID_AA64PFR1_EL1_SSBS_NI),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, BT, 0),
+	REG_FTR_END,
+};
+
 static const struct reg_ftr_bits ftr_id_aa64mmfr0_el1[] = {
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, ECV, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, EXS, 0),
@@ -199,6 +206,7 @@ static struct test_feature_reg test_regs[] = {
 	TEST_REG(SYS_ID_AA64ISAR1_EL1, ftr_id_aa64isar1_el1),
 	TEST_REG(SYS_ID_AA64ISAR2_EL1, ftr_id_aa64isar2_el1),
 	TEST_REG(SYS_ID_AA64PFR0_EL1, ftr_id_aa64pfr0_el1),
+	TEST_REG(SYS_ID_AA64PFR1_EL1, ftr_id_aa64pfr1_el1),
 	TEST_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0_el1),
 	TEST_REG(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1_el1),
 	TEST_REG(SYS_ID_AA64MMFR2_EL1, ftr_id_aa64mmfr2_el1),
-- 
2.40.1

Re: [PATCH v3 1/2] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Marc Zyngier]

On Fri, 28 Jun 2024 07:04:51 +0100,
Shaoqin Huang <shahuang@redhat.com> 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,
>     SME, RNDP_trap).
> 
>   - No changes to features (CSV2_frac, NMI, MTE_frac, GCS, THE, MTEX,
>     DF2, PFAR) which haven't been added into the ftr_id_aa64pfr1[].
>     Because the struct arm64_ftr_bits definition for each feature in the
>     ftr_id_aa64pfr1[] is used by arm64_check_features. If they're not
>     existing in the ftr_id_aa64pfr1[], the for loop won't check the if
>     the new_val is safe for those features.
> ---
>  arch/arm64/kvm/sys_regs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

This is getting very tiring:

- this isn't a valid patch, as it doesn't carry a proper SoB. You did
  it last time, I pointed it out, and you ignored me.

- this is *wrong*. The moment the kernel publishes any of the fields
  you have decided to ignore, the restoring of a VM on a new kernel
  will fail if the host and the VM have different values.

>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22b45a15d068..159cded22357 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2306,7 +2306,9 @@ 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_MTE |

Why? If the VM has been created with MTE support, this must be obeyed.

> +				     ID_AA64PFR1_EL1_SSBS |
> +				     ID_AA64PFR1_EL1_BT),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
>  	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),

So let me be very blunt:

- you *must* handle *all* the fields described in that register. There
  are 15 valid fields there, and I want to see all 15 fields being
  explicitly dealt with.

- fields that can be changed without ill effect must be enabled for
  write, irrespective of what the cpufeature code does (CSV2_frac, for
  example).

- fields that have a fixed value because KVM doesn't handle the
  corresponding feature must be explicitly disabled in the register
  accessor (MPAM, RNDR, MTEX, THE...). Just like we do for SME.

- fields that correspond to a feature that is controlled by an
  internal flag (MTE) must not be writable. Just like we do for PAuth
  in ID_AA64ISAR1_EL1.

- you *must* handle *all* the fields described in that register.

Until I see all of the above, I will not take this patch. If you don't
want to do this, that's absolutely fine by me. Just don't post another
patch. But if you do, this is the deal.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 1/2] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

Hi Marc,

On 6/29/24 16:55, Marc Zyngier wrote:
> On Fri, 28 Jun 2024 07:04:51 +0100,
> Shaoqin Huang <shahuang@redhat.com> 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,
>>      SME, RNDP_trap).
>>
>>    - No changes to features (CSV2_frac, NMI, MTE_frac, GCS, THE, MTEX,
>>      DF2, PFAR) which haven't been added into the ftr_id_aa64pfr1[].
>>      Because the struct arm64_ftr_bits definition for each feature in the
>>      ftr_id_aa64pfr1[] is used by arm64_check_features. If they're not
>>      existing in the ftr_id_aa64pfr1[], the for loop won't check the if
>>      the new_val is safe for those features.
>> ---
>>   arch/arm64/kvm/sys_regs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> This is getting very tiring:
> 
> - this isn't a valid patch, as it doesn't carry a proper SoB. You did
>    it last time, I pointed it out, and you ignored me.
> 
> - this is *wrong*. The moment the kernel publishes any of the fields
>    you have decided to ignore, the restoring of a VM on a new kernel
>    will fail if the host and the VM have different values.
> 

Thanks for giving me the detail about why it's wrong, and thanks to your 
patience to tolerate my mistakes.

This time I understand what you mean and it's a good opportunity for me 
to learn professional knowledge from you.

>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 22b45a15d068..159cded22357 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2306,7 +2306,9 @@ 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_MTE |
> 
> Why? If the VM has been created with MTE support, this must be obeyed.

I misunderstood the MTE support in KVM, and thanks your explanation 
below. This is absolutely wrong.

> 
>> +				     ID_AA64PFR1_EL1_SSBS |
>> +				     ID_AA64PFR1_EL1_BT),
>>   	ID_UNALLOCATED(4,2),
>>   	ID_UNALLOCATED(4,3),
>>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> 
> So let me be very blunt:
> 
> - you *must* handle *all* the fields described in that register. There
>    are 15 valid fields there, and I want to see all 15 fields being
>    explicitly dealt with.

Yes, I totally understand that.

> 
> - fields that can be changed without ill effect must be enabled for
>    write, irrespective of what the cpufeature code does (CSV2_frac, for
>    example).

Ok, I remember that.

> 
> - fields that have a fixed value because KVM doesn't handle the
>    corresponding feature must be explicitly disabled in the register
>    accessor (MPAM, RNDR, MTEX, THE...). Just like we do for SME.

Ok, I think this is the point I misunderstood. Thus cause some unhappy 
to you. I want to say sorry to you and I really appreciate your 
expertise and patience to explain this to me.

> 
> - fields that correspond to a feature that is controlled by an
>    internal flag (MTE) must not be writable. Just like we do for PAuth
>    in ID_AA64ISAR1_EL1.

Got that.

> 
> - you *must* handle *all* the fields described in that register.
> 
> Until I see all of the above, I will not take this patch. If you don't
> want to do this, that's absolutely fine by me. Just don't post another
> patch. But if you do, this is the deal.

Thanks a lot you provided so much useful information about the writable 
register. With those standard, I won't post the invalid patch like this.

Thanks,
Shaoqin

> 
> 	M.
> 

-- 
Shaoqin