[PATCH v4 0/3] Allow userspace to change ID_AA64PFR1_EL1

[PATCH v4 0/3] Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

Hi guys,

This is another try to allow userspace to change ID_AA64PFR1_EL1, and we want to
give userspace the ability to control the visible feature set for a VM, which
could be used by userspace in such a way to transparently migrate VMs.

The patch series have three part:

The first patch disable those fields which KVM doesn't know how to handle, so
KVM will only expose value 0 of those fields to the guest.

The second patch allow userspace to change ID_AA64PFR1_EL1, it allow as much as
possible fields to be writable, except some special fields which is still not
writable.

The third patch adds the kselftest to test if userspace can change the
ID_AA64PFR1_EL1.

Besides, I also noticed there is another patch [1] which try to make the
ID_AA64PFR1_EL1 writable. This patch [1] is try to enable GCS on baremental, and
add GCS support for the guest. What I understand is if we have GCS support on
baremental, it will be clear to how to handle them in KVM. And same for other
fields like NMI, THE, DF2, MTEX..

I'm still not confident about the correctness of this patch series, but I've try
my best to understand each of the fields. And follow Marc's comments to tweak
this patch series.

The question confuse me a lot is that should we allow those fields (NMI, GCS,
THE, DF2, MTEX..) which KVM doesn't know how to handle writable? Baremental
doesn't know about them, and the ftr_id_aa64pfr1[] doesn't know about them. I
follow the comment "I should handle all 15 fields", so I allow them writable
because they're disabled in the register read accessor, and their value will
alwyas be 0, the userspace can write to it but only value 0.

If I did anything wrong, please point me out. Thanks a lot.

[1] [PATCH v9 13/39] KVM: arm64: Manage GCS registers for guests
    https://lore.kernel.org/all/20240625-arm64-gcs-v9-13-0f634469b8f0@kernel.org/

Changelog:
----------
v3 -> v4:
  * Add a new patch to disable some feature which KVM doesn't know how to
    handle in the register accessor.
  * Handle all the fields in the register.
  * Fixes a small cnt issue in kselftest.

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/
v3: https://lore.kernel.org/all/20240628060454.1936886-2-shahuang@redhat.com/

Shaoqin Huang (3):
  KVM: arm64: Disable fields that KVM doesn't know how to handle in
    ID_AA64PFR1_EL1
  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                     | 13 ++++++++++-
 .../selftests/kvm/aarch64/set_id_regs.c       | 23 ++++++++++++++++---
 2 files changed, 32 insertions(+), 4 deletions(-)

-- 
2.40.1

[PATCH v4 1/3] KVM: arm64: Disable fields that KVM doesn't know how to handle in ID_AA64PFR1_EL1

[Shaoqin Huang]

For some of the fields in the ID_AA64PFR1_EL1 register, KVM doesn't know
how to handle them right now. So explicitly disable them in the register
accessor, then those fields value will be masked to 0 even if on the
hardware the field value is 1.

This will benifit the migration if the host and VM have different values
when restoring a VM.

Those fields include RNDR_trap, NMI, MTE_frac, GCS, THE, MTEX, DF2, PFAR.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 22b45a15d068..4508288b9d38 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1531,6 +1531,14 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
 			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE);
 
 		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_SME);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_RNDR_trap);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_NMI);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTE_frac);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_GCS);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_THE);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_MTEX);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_DF2);
+		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_EL1_PFAR);
 		break;
 	case SYS_ID_AA64ISAR1_EL1:
 		if (!vcpu_has_ptrauth(vcpu))
-- 
2.40.1

Re: [PATCH v4 1/3] KVM: arm64: Disable fields that KVM doesn't know how to handle in ID_AA64PFR1_EL1

[Oliver Upton]

Hi Shaoqin,

On Wed, Jul 17, 2024 at 11:50:14PM -0400, Shaoqin Huang wrote:
> For some of the fields in the ID_AA64PFR1_EL1 register, KVM doesn't know
> how to handle them right now. So explicitly disable them in the register
> accessor, then those fields value will be masked to 0 even if on the
> hardware the field value is 1.

It is probably important to note that the only reason this is safe to do
from a UAPI POV is that read_sanitised_ftr_reg() doesn't yet return a
nonzero value for any of these fields.

-- 
Thanks,
Oliver

Re: [PATCH v4 1/3] KVM: arm64: Disable fields that KVM doesn't know how to handle in ID_AA64PFR1_EL1

[Shaoqin Huang]

Hi Oliver,

On 7/18/24 14:09, Oliver Upton wrote:
> Hi Shaoqin,
> 
> On Wed, Jul 17, 2024 at 11:50:14PM -0400, Shaoqin Huang wrote:
>> For some of the fields in the ID_AA64PFR1_EL1 register, KVM doesn't know
>> how to handle them right now. So explicitly disable them in the register
>> accessor, then those fields value will be masked to 0 even if on the
>> hardware the field value is 1.
> 
> It is probably important to note that the only reason this is safe to do
> from a UAPI POV is that read_sanitised_ftr_reg() doesn't yet return a
> nonzero value for any of these fields.

(Reply again by the plain text)

Yeah. That would be more clear if I tell the reader this information. 
Will add this when updating.

Thanks,
Shaoqin

> 

-- 
Shaoqin

[PATCH v4 2/3] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

Allow userspace to change the guest-visible value of the register with
different way of handling:

  - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1
    register, RAS_frac and MPAM_frac are also not writable in the
    ID_AA64PFR1_EL1 register.

  - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED),
    so it's not writable.

  - For those fields which KVM doesn't know how to handle, they have
    are not exposed to the guest (being disabled in the register read
    accessor), those fields value will always be 0. Allow those fields
    writable is fine, since the userspace can only write 0 into those
    fields. Maybe in the future KVM know how to handle some of the
    fields, then they can be written into other value.
    So let them writable.
    Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR,
    MTE_frac, MTEX.

  - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM
    doesn't know how to handle, they can be written without ill effect.
    So let them writable.

Besides, we don't do the crosscheck in KVM about the CSV2_frac even if
it depends on the value of CSV2, it should be made sure by the VMM
instead of KVM.

Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 4508288b9d38..26b38165a8be 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2314,7 +2314,10 @@ 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_RES0 |
+				       ID_AA64PFR1_EL1_MPAM_frac |
+				       ID_AA64PFR1_EL1_RAS_frac |
+				       ID_AA64PFR1_EL1_MTE)),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
 	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
-- 
2.40.1

Re: [PATCH v4 2/3] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Oliver Upton]

On Wed, Jul 17, 2024 at 11:50:15PM -0400, Shaoqin Huang wrote:
> Allow userspace to change the guest-visible value of the register with
> different way of handling:
> 
>   - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1
>     register, RAS_frac and MPAM_frac are also not writable in the
>     ID_AA64PFR1_EL1 register.
> 
>   - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED),
>     so it's not writable.

The flag isn't the relevant part, what's important about MTE is that it
already has a separate UAPI for controlling it (KVM_CAP_ARM_MTE).

>   - For those fields which KVM doesn't know how to handle, they have
>     are not exposed to the guest (being disabled in the register read
>     accessor), those fields value will always be 0. Allow those fields
>     writable is fine, since the userspace can only write 0 into those
>     fields. Maybe in the future KVM know how to handle some of the
>     fields, then they can be written into other value.
>     So let them writable.
>     Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR,
>     MTE_frac, MTEX.

This doesn't seem right. We're committing to a UAPI behavior the moment
these fields are advertised to userspace, which is rather difficult to
do for features that we don't even implement.

Please only advertise the fields known to KVM and leave the others
unadvertised.

>   - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM
>     doesn't know how to handle, they can be written without ill effect.
>     So let them writable.

I think the handling of ARM_SMCCC_ARCH_WORKAROUND_2 needs to be updated
to consider the presence of FEAT_SSBS in the guest's ID registers.
Otherwise we'll wind up returning NOT_SUPPORTED and the guest will
conclude it is in a vulnerable state.

-- 
Thanks,
Oliver

Re: [PATCH v4 2/3] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

Hi Oliver,

On 7/18/24 14:35, Oliver Upton wrote:
> On Wed, Jul 17, 2024 at 11:50:15PM -0400, Shaoqin Huang wrote:
>> Allow userspace to change the guest-visible value of the register with
>> different way of handling:
>>
>>    - Since the RAS and MPAM is not writable in the ID_AA64PFR0_EL1
>>      register, RAS_frac and MPAM_frac are also not writable in the
>>      ID_AA64PFR1_EL1 register.
>>
>>    - The MTE is controlled by an internal flag (KVM_ARCH_FLAG_MTE_ENABLED),
>>      so it's not writable.
> 
> The flag isn't the relevant part, what's important about MTE is that it
> already has a separate UAPI for controlling it (KVM_CAP_ARM_MTE).

I'm not quite understand why KVM_ARCH_FLAG_MTE_ENABLED isn't the 
relevant part. I see this capability, when enable the KVM_CAP_ARM_MTE, 
it set the KVM_ARCH_FLAG_MTE_ENABLED in the kvm->arch.flags.

And do you mean we should update it like "The MTE is controlled by a 
UAPI (KVM_CAP_ARM_MTE)"?

> 
>>    - For those fields which KVM doesn't know how to handle, they have
>>      are not exposed to the guest (being disabled in the register read
>>      accessor), those fields value will always be 0. Allow those fields
>>      writable is fine, since the userspace can only write 0 into those
>>      fields. Maybe in the future KVM know how to handle some of the
>>      fields, then they can be written into other value.
>>      So let them writable.
>>      Those fields include SME, RNDR_trap, NMI, GCS, THE, DF2, PFAR,
>>      MTE_frac, MTEX.
> 
> This doesn't seem right. We're committing to a UAPI behavior the moment
> these fields are advertised to userspace, which is rather difficult to
> do for features that we don't even implement.
> 
> Please only advertise the fields known to KVM and leave the others
> unadvertised.

Thanks a lot for pointing this out. Now I get the point, for those not 
implemented feature, they should not writable, so they're not advertised 
to userspace.

> 
>>    - The BT, SSBS, CSV2_frac don't introduce any new registers which KVM
>>      doesn't know how to handle, they can be written without ill effect.
>>      So let them writable.
> 
> I think the handling of ARM_SMCCC_ARCH_WORKAROUND_2 needs to be updated
> to consider the presence of FEAT_SSBS in the guest's ID registers.
> Otherwise we'll wind up returning NOT_SUPPORTED and the guest will
> conclude it is in a vulnerable state.

I see that line of code, in the case ARM_SMCCC_ARCH_WORKAROUND_2:

if (cpus_have_final_cap(ARM64_SSBS))
	break;

I guess we should update it with something like

if (SYS_FIELD_GET(ID_AA64PFR1_EL1, SSBS, IDREG(kvm,
     SYS_ID_AA64PFR1_EL1)) != 0)

Oliver, Is there any proper function or macro implement the checking 
like above? I don't find the similar checking in the code.

Thanks,
Shaoqin

> 

-- 
Shaoqin

[PATCH v4 3/3] 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>
---
 .../selftests/kvm/aarch64/set_id_regs.c       | 23 ++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
index a7de39fa2a0a..836dfb17d322 100644
--- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -133,6 +133,22 @@ 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, PFAR, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, DF2, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, MTEX, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, THE, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, GCS, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, MTE_frac, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, NMI, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, CSV2_frac, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, RNDR_trap, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64PFR1_EL1, SME, 0),
+	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 +215,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),
@@ -551,9 +568,9 @@ int main(void)
 	test_cnt = ARRAY_SIZE(ftr_id_aa64dfr0_el1) + ARRAY_SIZE(ftr_id_dfr0_el1) +
 		   ARRAY_SIZE(ftr_id_aa64isar0_el1) + ARRAY_SIZE(ftr_id_aa64isar1_el1) +
 		   ARRAY_SIZE(ftr_id_aa64isar2_el1) + ARRAY_SIZE(ftr_id_aa64pfr0_el1) +
-		   ARRAY_SIZE(ftr_id_aa64mmfr0_el1) + ARRAY_SIZE(ftr_id_aa64mmfr1_el1) +
-		   ARRAY_SIZE(ftr_id_aa64mmfr2_el1) + ARRAY_SIZE(ftr_id_aa64zfr0_el1) -
-		   ARRAY_SIZE(test_regs) + 2;
+		   ARRAY_SIZE(ftr_id_aa64pfr1_el1) + ARRAY_SIZE(ftr_id_aa64mmfr0_el1) +
+		   ARRAY_SIZE(ftr_id_aa64mmfr1_el1) + ARRAY_SIZE(ftr_id_aa64mmfr2_el1) +
+		   ARRAY_SIZE(ftr_id_aa64zfr0_el1) - ARRAY_SIZE(test_regs) + 2;
 
 	ksft_set_plan(test_cnt);
 
-- 
2.40.1