[PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[PATCH v5 0/4] 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 four 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 check the FEAT_SSBS in guest IDREG instead of the cpu
capability.

The third patch allow userspace to change ID_AA64PFR1_EL1, it only advertise the
fields known to KVM and leave others unadvertise.

The fourth 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.. At that time, they can be writable.

[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:
----------
v4 -> v5:
  * Only advertise fields which KVM know how to handle to userspace, leave
    others unadvertised.
  * Add a new patch to check FEAT_SSBS in IDREG instead of cpu capability.
  * Tweak the kselftest writable fields.
  * Improve the commit message.

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/
v4: https://lore.kernel.org/all/20240718035017.434996-1-shahuang@redhat.com/

Shaoqin Huang (4):
  KVM: arm64: Disable fields that KVM doesn't know how to handle in
    ID_AA64PFR1_EL1
  KVM: arm64: Use kvm_has_feat() to check if FEAT_SSBS is advertised to
    the guest
  KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1
  KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1

 arch/arm64/kvm/hypercalls.c                   | 12 +++++-----
 arch/arm64/kvm/sys_regs.c                     | 22 ++++++++++++++++++-
 .../selftests/kvm/aarch64/set_id_regs.c       | 14 +++++++++---
 3 files changed, 38 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH v5 1/4] 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 is safe because from a UAPI point of
view that read_sanitised_ftr_reg() doesn't yet return a nonzero value
for any of those fields.

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

[PATCH v5 2/4] KVM: arm64: Use kvm_has_feat() to check if FEAT_SSBS is advertised to the guest

[Shaoqin Huang]

Currently KVM use cpus_have_final_cap() to check if FEAT_SSBS is
advertised to the guest. But if FEAT_SSBS is writable and isn't
advertised to the guest, this is wrong.

Update it to use kvm_has_feat() to check if FEAT_SSBS is advertised
to the guest, thus the KVM can do the right thing if FEAT_SSBS isn't
advertised to the guest.

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

diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 5763d979d8ca..ee6573befb81 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -317,7 +317,7 @@ int kvm_smccc_call_handler(struct kvm_vcpu *vcpu)
 				 * to the guest, and hide SSBS so that the
 				 * guest stays protected.
 				 */
-				if (cpus_have_final_cap(ARM64_SSBS))
+				if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, SSBS, IMP))
 					break;
 				fallthrough;
 			case SPECTRE_UNAFFECTED:
@@ -428,7 +428,7 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
  * Convert the workaround level into an easy-to-compare number, where higher
  * values mean better protection.
  */
-static int get_kernel_wa_level(u64 regid)
+static int get_kernel_wa_level(struct kvm_vcpu *vcpu, u64 regid)
 {
 	switch (regid) {
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
@@ -449,7 +449,7 @@ static int get_kernel_wa_level(u64 regid)
 			 * don't have any FW mitigation if SSBS is there at
 			 * all times.
 			 */
-			if (cpus_have_final_cap(ARM64_SSBS))
+			if (kvm_has_feat(vcpu->kvm, ID_AA64PFR1_EL1, SSBS, IMP))
 				return KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_AVAIL;
 			fallthrough;
 		case SPECTRE_UNAFFECTED:
@@ -486,7 +486,7 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_1:
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
 	case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_3:
-		val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
+		val = get_kernel_wa_level(vcpu, reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
 		break;
 	case KVM_REG_ARM_STD_BMAP:
 		val = READ_ONCE(smccc_feat->std_bmap);
@@ -588,7 +588,7 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		if (val & ~KVM_REG_FEATURE_LEVEL_MASK)
 			return -EINVAL;
 
-		if (get_kernel_wa_level(reg->id) < val)
+		if (get_kernel_wa_level(vcpu, reg->id) < val)
 			return -EINVAL;
 
 		return 0;
@@ -624,7 +624,7 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		 * We can deal with NOT_AVAIL on NOT_REQUIRED, but not the
 		 * other way around.
 		 */
-		if (get_kernel_wa_level(reg->id) < wa_level)
+		if (get_kernel_wa_level(vcpu, reg->id) < wa_level)
 			return -EINVAL;
 
 		return 0;
-- 
2.40.1

[PATCH v5 3/4] 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 a separate UAPI (KVM_CAP_ARM_MTE) with 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 are not
    exposed to the guest (being disabled in the register read accessor),
    those fields value will always be 0.
    Those fields don't have a known behavior now, so don't advertise
    them to the userspace. Thus still not 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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 4508288b9d38..0f2c2a1182cc 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2314,7 +2314,19 @@ 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_PFAR |
+				       ID_AA64PFR1_EL1_DF2 |
+				       ID_AA64PFR1_EL1_MTEX |
+				       ID_AA64PFR1_EL1_THE |
+				       ID_AA64PFR1_EL1_GCS |
+				       ID_AA64PFR1_EL1_MTE_frac |
+				       ID_AA64PFR1_EL1_NMI |
+				       ID_AA64PFR1_EL1_RNDR_trap |
+				       ID_AA64PFR1_EL1_SME |
+				       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

[PATCH v5 4/4] 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 | 14 +++++++++++---
 1 file changed, 11 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..04e237a371f2 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, CSV2_frac, 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 +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),
@@ -551,9 +559,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

Re: [PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang]

Hi Marc, Oliver,

Kindly ping for this series. Welcome your feedbacks. I wonder if this 
implementation satisfy your expectation?

Thanks,
Shaoqin

On 7/23/24 15:19, Shaoqin Huang wrote:
> 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 four 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 check the FEAT_SSBS in guest IDREG instead of the cpu
> capability.
> 
> The third patch allow userspace to change ID_AA64PFR1_EL1, it only advertise the
> fields known to KVM and leave others unadvertise.
> 
> The fourth 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.. At that time, they can be writable.
> 
> [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:
> ----------
> v4 -> v5:
>    * Only advertise fields which KVM know how to handle to userspace, leave
>      others unadvertised.
>    * Add a new patch to check FEAT_SSBS in IDREG instead of cpu capability.
>    * Tweak the kselftest writable fields.
>    * Improve the commit message.
> 
> 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/
> v4: https://lore.kernel.org/all/20240718035017.434996-1-shahuang@redhat.com/
> 
> Shaoqin Huang (4):
>    KVM: arm64: Disable fields that KVM doesn't know how to handle in
>      ID_AA64PFR1_EL1
>    KVM: arm64: Use kvm_has_feat() to check if FEAT_SSBS is advertised to
>      the guest
>    KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1
>    KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1
> 
>   arch/arm64/kvm/hypercalls.c                   | 12 +++++-----
>   arch/arm64/kvm/sys_regs.c                     | 22 ++++++++++++++++++-
>   .../selftests/kvm/aarch64/set_id_regs.c       | 14 +++++++++---
>   3 files changed, 38 insertions(+), 10 deletions(-)
> 

-- 
Shaoqin

Re: [PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[Marc Zyngier]

On Tue, 23 Jul 2024 08:19:59 +0100,
Shaoqin Huang <shahuang@redhat.com> wrote:
> 
> 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.


I think this looks OK now, thanks for going through the motions and
doing the right thing.

What is missing is similar handling for 32bit ID registers, but I'm
not sure we keen on going down that road -- machines capable of
running those are on their way out. This can be done later anyway,
should anyone care.

>
> The patch series have four 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 check the FEAT_SSBS in guest IDREG instead of the cpu
> capability.
> 
> The third patch allow userspace to change ID_AA64PFR1_EL1, it only advertise the
> fields known to KVM and leave others unadvertise.
> 
> The fourth 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.. At that time, they can be
> writable.

I expect that Broonie would look into this as part of his series.

Thanks,

	M.

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

Re: [PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[Russell King (Oracle)]

On Sun, Aug 25, 2024 at 05:46:36PM +0100, Marc Zyngier wrote:
> On Tue, 23 Jul 2024 08:19:59 +0100,
> Shaoqin Huang <shahuang@redhat.com> wrote:
> > 
> > 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.
> 
> 
> I think this looks OK now, thanks for going through the motions and
> doing the right thing.
> 
> What is missing is similar handling for 32bit ID registers, but I'm
> not sure we keen on going down that road -- machines capable of
> running those are on their way out. This can be done later anyway,
> should anyone care.

The Aarch32 ID registers need doing - we've already established that
fact. Sadly, you decided you wouldn't respond to my patch addressing
one of the Aarch32 ID registers despite me sending follow-ups to nicely
ask you about this - you seemed to go utterly silent on it.

The Aarch32 ID registers have changed value between different kernel
versions, and given that QEMU saves and restores _all_ ID registers,
changes to these ID registers cause a regression if one attempts to
migrate VMs between one kernel version and the next. It doesn't even
have to be between two physical machines. Libvirt supports managed-
saving on reboot, where it saves an image of a VM at shutdown, and
restores it at the next reboot. These changes in ID registers render
effectively data loss in VMs that have been managed-saved - the
saved state of the VM has to either be destroyed, or the host kernel
reverted back and _never_ moved forward.

As you don't seem to be keen to address this (by ignoring my emails
on the topic, and now suggesting in your response above that you're
not keen to do anything with the Aarch32 ID registers, I guess this
just means that KVM on Aarch64 is going to forever suck.

I'm sure Oliver will recall my emails on this which you've decided to
ignore... he was supportive of my efforts to address this.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[Marc Zyngier]

On Sun, 25 Aug 2024 18:09:48 +0100,
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> On Sun, Aug 25, 2024 at 05:46:36PM +0100, Marc Zyngier wrote:
> > On Tue, 23 Jul 2024 08:19:59 +0100,
> > Shaoqin Huang <shahuang@redhat.com> wrote:
> > > 
> > > 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.
> > 
> > 
> > I think this looks OK now, thanks for going through the motions and
> > doing the right thing.
> > 
> > What is missing is similar handling for 32bit ID registers, but I'm
> > not sure we keen on going down that road -- machines capable of
> > running those are on their way out. This can be done later anyway,
> > should anyone care.
> 
> The Aarch32 ID registers need doing - we've already established that
> fact. Sadly, you decided you wouldn't respond to my patch addressing
> one of the Aarch32 ID registers despite me sending follow-ups to nicely
> ask you about this - you seemed to go utterly silent on it.

No, Russell. *you* went utterly silent after your May patch. You sent
an RFC, to which people responded. Given that your last email on the
subject was almost 4 months ago and that you never brought the subject
up again, it can't be that big a deal.

To me, an RFC means "I have this idea, and I'm not sure how to do it".
An RFC is usually only a proof of concept that has no purpose being
taken at face value. If you want a patch taken seriously, don't send
it as an RFC. And send it again if nobody replies. It's not that this
is anything new.

> The Aarch32 ID registers have changed value between different kernel
> versions, and given that QEMU saves and restores _all_ ID registers,
> changes to these ID registers cause a regression if one attempts to
> migrate VMs between one kernel version and the next. It doesn't even
> have to be between two physical machines. Libvirt supports managed-
> saving on reboot, where it saves an image of a VM at shutdown, and
> restores it at the next reboot. These changes in ID registers render
> effectively data loss in VMs that have been managed-saved - the
> saved state of the VM has to either be destroyed, or the host kernel
> reverted back and _never_ moved forward.
>
> As you don't seem to be keen to address this (by ignoring my emails
> on the topic, and now suggesting in your response above that you're
> not keen to do anything with the Aarch32 ID registers, I guess this
> just means that KVM on Aarch64 is going to forever suck.

I'm fine with that. Nobody is forced to use it, and I don't feel the
need to put extra effort on things I don't care about any more.
AArch32 support is one of these things, amongst many others.

If you want the support to improve, I suggest you send patches. And
send them again if no reply shows up in a timely manner.  Because
you're probably the last person who gives a damn about the AArch32
support in KVM. And if not even you can be bothered to fix it, then
support for AArch32 EL1 should probably be removed altogether (I'm all
for deleting unused code).

> I'm sure Oliver will recall my emails on this which you've decided to
> ignore... he was supportive of my efforts to address this.

I'm supportive as well. I'm just not going to fix it for you.

	M.

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

Re: [PATCH v5 0/4] Allow userspace to change ID_AA64PFR1_EL1

[Marc Zyngier]

On Tue, 23 Jul 2024 03:19:59 -0400, Shaoqin Huang wrote:
> 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 four 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.
> 
> [...]

Applied to next, thanks!

[1/4] KVM: arm64: Disable fields that KVM doesn't know how to handle in ID_AA64PFR1_EL1
      commit: ffe68b2d19a5a84440fea99a732cfc3b157559eb
[2/4] KVM: arm64: Use kvm_has_feat() to check if FEAT_SSBS is advertised to the guest
      commit: e8d164974cfa46fe5ec87869c8a7113641f322d5
[3/4] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1
      commit: 78c4446b5f957fb14737582e503b1b25f66edc45
[4/4] KVM: selftests: aarch64: Add writable test for ID_AA64PFR1_EL1
      commit: dc9b5d7e0bd40e68a94013766b27be3dda10c006

Cheers,

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