This series implements support for the 2023 dpISA extensions in KVM
guests, it was previously posted as part of a series with the host
support but that has now been merged so only the KVM portions remain.
Most of these extensions add only new instructions so the guest support
consists of adding the relevant ID registers, masking out other features
like the 2023 MTE extensions.
FEAT_FPMR introduces a new system register FPMR to the floating point
state which we enable guest access to and context switch when the ID
registers indicate that it is supported. Currently we implement
visibility for FPMR with a fpmr_visibility() function as for other
system registers, I will separately look into adding support for
specifying this in the struct sys_reg_desc.
Signed-off-by: Mark Brown <[email protected]>
---
Changes in v6:
- Rebase onto v6.9-rc1.
- The host portions of the series were merged so only the KVM guest
support remains.
- Link to v5: https://lore.kernel.org/r/[email protected]
Changes in v5:
- Rebase onto v6.8-rc3.
- Use u64 rather than unsigned long for storing FPMR.
- Temporarily drop KVM guest support due to issues with KVM being a
moving target.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- Rebase onto v6.8-rc1.
- Move KVM support to the end of the series.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Rebase onto v6.7-rc3.
- Hook up traps for FPMR in emulate-nested.c.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Rebase onto v6.7-rc1.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Mark Brown (5):
KVM: arm64: Share all userspace hardened thread data with the hypervisor
KVM: arm64: Add newly allocated ID registers to register descriptions
KVM: arm64: Support FEAT_FPMR for guests
KVM: arm64: selftests: Document feature registers added in 2023 extensions
KVM: arm64: selftests: Teach get-reg-list about FPMR
arch/arm64/include/asm/kvm_host.h | 6 ++++--
arch/arm64/include/asm/processor.h | 2 +-
arch/arm64/kvm/emulate-nested.c | 9 ++++++++
arch/arm64/kvm/fpsimd.c | 15 +++++++-------
arch/arm64/kvm/hyp/include/hyp/switch.h | 9 ++++++--
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++--
arch/arm64/kvm/sys_regs.c | 24 +++++++++++++++++++---
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 11 ++++++++--
8 files changed, 60 insertions(+), 20 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20231003-arm64-2023-dpisa-2f3d25746474
Best regards,
--
Mark Brown <[email protected]>
As part of the lazy FPSIMD state transitioning done by the hypervisor we
currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with
the host. Since this struct is non-extensible userspace ABI we have to keep
the definition as is but the addition of FPMR in the 2023 dpISA means that
we will want to share more storage with the host. To facilitate this
refactor the current code to share the entire thread->uw rather than just
the one field.
The large number of references to fpsimd_state make it very inconvenient
to add an additional wrapper struct.
Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/kvm_host.h | 3 ++-
arch/arm64/include/asm/processor.h | 2 +-
arch/arm64/kvm/fpsimd.c | 13 ++++++-------
arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++--
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 9e8a496fb284..8a251f0da900 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -27,6 +27,7 @@
#include <asm/fpsimd.h>
#include <asm/kvm.h>
#include <asm/kvm_asm.h>
+#include <asm/processor.h>
#include <asm/vncr_mapping.h>
#define __KVM_HAVE_ARCH_INTC_INITIALIZED
@@ -640,7 +641,7 @@ struct kvm_vcpu_arch {
struct kvm_guest_debug_arch vcpu_debug_state;
struct kvm_guest_debug_arch external_debug_state;
- struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */
+ struct thread_struct_uw *host_uw; /* hyp VA */
struct task_struct *parent_task;
struct {
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index f77371232d8c..78781333ee26 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -152,7 +152,7 @@ struct thread_struct {
* Maintainers must ensure manually that this contains no
* implicit padding.
*/
- struct {
+ struct thread_struct_uw {
unsigned long tp_value; /* TLS register */
unsigned long tp2_value;
u64 fpmr;
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 826307e19e3a..8a0fedbb6f39 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -17,13 +17,13 @@
void kvm_vcpu_unshare_task_fp(struct kvm_vcpu *vcpu)
{
struct task_struct *p = vcpu->arch.parent_task;
- struct user_fpsimd_state *fpsimd;
+ struct thread_struct_uw *uw;
if (!is_protected_kvm_enabled() || !p)
return;
- fpsimd = &p->thread.uw.fpsimd_state;
- kvm_unshare_hyp(fpsimd, fpsimd + 1);
+ uw = &p->thread.uw;
+ kvm_unshare_hyp(uw, uw + 1);
put_task_struct(p);
}
@@ -39,17 +39,16 @@ void kvm_vcpu_unshare_task_fp(struct kvm_vcpu *vcpu)
int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
{
int ret;
-
- struct user_fpsimd_state *fpsimd = ¤t->thread.uw.fpsimd_state;
+ struct thread_struct_uw *uw = ¤t->thread.uw;
kvm_vcpu_unshare_task_fp(vcpu);
/* Make sure the host task fpsimd state is visible to hyp: */
- ret = kvm_share_hyp(fpsimd, fpsimd + 1);
+ ret = kvm_share_hyp(uw, uw + 1);
if (ret)
return ret;
- vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd);
+ vcpu->arch.host_uw = kern_hyp_va(uw);
/*
* We need to keep current's task_struct pinned until its data has been
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index e3fcf8c4d5b4..a9a11893c191 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -377,7 +377,7 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code)
/* Write out the host state if it's in the registers */
if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED)
- __fpsimd_save_state(vcpu->arch.host_fpsimd_state);
+ __fpsimd_save_state(&(vcpu->arch.host_uw->fpsimd_state));
/* Restore the guest state */
if (sve_guest)
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 2385fd03ed87..eb2208009875 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -42,7 +42,7 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
hyp_vcpu->vcpu.arch.fp_state = host_vcpu->arch.fp_state;
hyp_vcpu->vcpu.arch.debug_ptr = kern_hyp_va(host_vcpu->arch.debug_ptr);
- hyp_vcpu->vcpu.arch.host_fpsimd_state = host_vcpu->arch.host_fpsimd_state;
+ hyp_vcpu->vcpu.arch.host_uw = host_vcpu->arch.host_uw;
hyp_vcpu->vcpu.arch.vsesr_el2 = host_vcpu->arch.vsesr_el2;
@@ -64,7 +64,7 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
host_vcpu->arch.fault = hyp_vcpu->vcpu.arch.fault;
host_vcpu->arch.iflags = hyp_vcpu->vcpu.arch.iflags;
- host_vcpu->arch.fp_state = hyp_vcpu->vcpu.arch.fp_state;
+ host_vcpu->arch.host_uw = hyp_vcpu->vcpu.arch.host_uw;
host_cpu_if->vgic_hcr = hyp_cpu_if->vgic_hcr;
for (i = 0; i < hyp_cpu_if->used_lrs; ++i)
--
2.30.2
The 2023 architecture extensions have allocated some new ID registers, add
them to the KVM system register descriptions so that they are visible to
guests.
We make the newly introduced dpISA features writeable, as well as
allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
introduces straigforward new instructions with no additional
architectural state or traps.
Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c9f4f387155f..a3c20d1a36aa 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2293,12 +2293,15 @@ static const struct sys_reg_desc sys_reg_descs[] = {
ID_AA64PFR0_EL1_AdvSIMD |
ID_AA64PFR0_EL1_FP), },
ID_SANITISED(ID_AA64PFR1_EL1),
- ID_UNALLOCATED(4,2),
+ ID_WRITABLE(ID_AA64PFR2_EL1, ~(ID_AA64PFR2_EL1_RES0 |
+ ID_AA64PFR2_EL1_MTEFAR |
+ ID_AA64PFR2_EL1_MTESTOREONLY |
+ ID_AA64PFR2_EL1_MTEPERM)),
ID_UNALLOCATED(4,3),
ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
ID_HIDDEN(ID_AA64SMFR0_EL1),
ID_UNALLOCATED(4,6),
- ID_UNALLOCATED(4,7),
+ ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
/* CRm=5 */
{ SYS_DESC(SYS_ID_AA64DFR0_EL1),
@@ -2325,7 +2328,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
ID_WRITABLE(ID_AA64ISAR2_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
ID_AA64ISAR2_EL1_APA3 |
ID_AA64ISAR2_EL1_GPA3)),
- ID_UNALLOCATED(6,3),
+ ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
+ ID_AA64ISAR3_EL1_PACM |
+ ID_AA64ISAR3_EL1_TLBIW)),
ID_UNALLOCATED(6,4),
ID_UNALLOCATED(6,5),
ID_UNALLOCATED(6,6),
--
2.30.2
FEAT_FPMR introduces a new system register FPMR which allows configuration
of floating point behaviour, currently for FP8 specific features. Allow use
of this in guests, disabling the trap while guests are running and saving
and restoring the value along with the rest of the floating point state
if the ID registers indicate that the feature is present. Since FPMR is
stored immediately after the main floating point state we share it with
the hypervisor by adjusting the size of the shared region.
Access to FPMR is covered by both a register specific trap HCRX_EL2.EnFPM
and the overall floating point access trap so we just unconditionally
enable the FPMR specific trap if the guest has FPMR exposed in the ID
registers and rely on the floating point access trap to detect guest
floating point usage when FPMR is enabled for the guest.
Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/include/asm/kvm_host.h | 3 ++-
arch/arm64/kvm/emulate-nested.c | 9 +++++++++
arch/arm64/kvm/fpsimd.c | 2 +-
arch/arm64/kvm/hyp/include/hyp/switch.h | 7 ++++++-
arch/arm64/kvm/sys_regs.c | 13 +++++++++++++
5 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 8a251f0da900..3f0f31b17d96 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -415,6 +415,8 @@ enum vcpu_sysreg {
APGAKEYLO_EL1,
APGAKEYHI_EL1,
+ FPMR,
+
/* Memory Tagging Extension registers */
RGSR_EL1, /* Random Allocation Tag Seed Register */
GCR_EL1, /* Tag Control Register */
@@ -582,7 +584,6 @@ struct kvm_vcpu_arch {
enum fp_type fp_type;
unsigned int sve_max_vl;
u64 svcr;
- u64 fpmr;
/* Stage 2 paging state used by the hardware on next switch */
struct kvm_s2_mmu *hw_mmu;
diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index 4697ba41b3a9..0289882713f9 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -67,6 +67,8 @@ enum cgt_group_id {
CGT_HCR_TTLBIS,
CGT_HCR_TTLBOS,
+ CGT_HCRX_EnFPM,
+
CGT_MDCR_TPMCR,
CGT_MDCR_TPM,
CGT_MDCR_TDE,
@@ -279,6 +281,12 @@ static const struct trap_bits coarse_trap_bits[] = {
.mask = HCR_TTLBOS,
.behaviour = BEHAVE_FORWARD_ANY,
},
+ [CGT_HCRX_EnFPM] = {
+ .index = HCRX_EL2,
+ .value = 0,
+ .mask = HCRX_EL2_EnFPM,
+ .behaviour = BEHAVE_HANDLE_LOCALLY,
+ },
[CGT_MDCR_TPMCR] = {
.index = MDCR_EL2,
.value = MDCR_EL2_TPMCR,
@@ -481,6 +489,7 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
SR_TRAP(SYS_AIDR_EL1, CGT_HCR_TID1),
SR_TRAP(SYS_SMIDR_EL1, CGT_HCR_TID1),
SR_TRAP(SYS_CTR_EL0, CGT_HCR_TID2),
+ SR_TRAP(SYS_FPMR, CGT_HCRX_EnFPM),
SR_TRAP(SYS_CCSIDR_EL1, CGT_HCR_TID2_TID4),
SR_TRAP(SYS_CCSIDR2_EL1, CGT_HCR_TID2_TID4),
SR_TRAP(SYS_CLIDR_EL1, CGT_HCR_TID2_TID4),
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 8a0fedbb6f39..2f625410c1b7 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -152,7 +152,7 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
fp_state.sve_vl = vcpu->arch.sve_max_vl;
fp_state.sme_state = NULL;
fp_state.svcr = &vcpu->arch.svcr;
- fp_state.fpmr = &vcpu->arch.fpmr;
+ fp_state.fpmr = &__vcpu_sys_reg(vcpu, FPMR);
fp_state.fp_type = &vcpu->arch.fp_type;
if (vcpu_has_sve(vcpu))
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index a9a11893c191..3d78ab164bab 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -376,10 +376,15 @@ static bool kvm_hyp_handle_fpsimd(struct kvm_vcpu *vcpu, u64 *exit_code)
isb();
/* Write out the host state if it's in the registers */
- if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED)
+ if (vcpu->arch.fp_state == FP_STATE_HOST_OWNED) {
__fpsimd_save_state(&(vcpu->arch.host_uw->fpsimd_state));
+ if (cpus_have_final_cap(ARM64_HAS_FPMR))
+ vcpu->arch.host_uw->fpmr = read_sysreg_s(SYS_FPMR);
+ }
/* Restore the guest state */
+ if (kvm_has_feat(kern_hyp_va(vcpu->kvm), ID_AA64PFR2_EL1, FPMR, IMP))
+ write_sysreg_s(__vcpu_sys_reg(vcpu, FPMR), SYS_FPMR);
if (sve_guest)
__hyp_sve_restore_guest(vcpu);
else
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a3c20d1a36aa..941ad700d0ab 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2068,6 +2068,15 @@ static unsigned int hidden_user_visibility(const struct kvm_vcpu *vcpu,
.visibility = hidden_user_visibility, \
}
+static unsigned int fpmr_visibility(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd)
+{
+ if (kvm_has_feat(vcpu->kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+ return 0;
+
+ return REG_HIDDEN;
+}
+
/*
* Since reset() callback and field val are not used for idregs, they will be
* used for specific purposes for idregs.
@@ -2469,6 +2478,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
{ SYS_DESC(SYS_CTR_EL0), access_ctr },
{ SYS_DESC(SYS_SVCR), undef_access },
+ { SYS_DESC(SYS_FPMR), access_rw, reset_unknown, FPMR,
+ .visibility = fpmr_visibility },
{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr, .reset = reset_pmcr,
.reg = PMCR_EL0, .get_user = get_pmcr, .set_user = set_pmcr },
@@ -4054,6 +4065,8 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
if (kvm_has_feat(kvm, ID_AA64ISAR2_EL1, MOPS, IMP))
vcpu->arch.hcrx_el2 |= (HCRX_EL2_MSCEn | HCRX_EL2_MCE2);
+ if (kvm_has_feat(kvm, ID_AA64PFR2_EL1, FPMR, IMP))
+ vcpu->arch.hcrx_el2 |= (HCRX_EL2_EnFPM);
}
if (test_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags))
--
2.30.2
The 2023 architecture extensions allocated some previously usused feature
registers, add comments mapping the names in get-reg-list as we do for the
other allocated registers.
Signed-off-by: Mark Brown <[email protected]>
---
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index 709d7d721760..71ea6ecec7ce 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -428,7 +428,7 @@ static __u64 base_regs[] = {
ARM64_SYS_REG(3, 0, 0, 4, 4), /* ID_AA64ZFR0_EL1 */
ARM64_SYS_REG(3, 0, 0, 4, 5), /* ID_AA64SMFR0_EL1 */
ARM64_SYS_REG(3, 0, 0, 4, 6),
- ARM64_SYS_REG(3, 0, 0, 4, 7),
+ ARM64_SYS_REG(3, 0, 0, 4, 7), /* ID_AA64FPFR_EL1 */
ARM64_SYS_REG(3, 0, 0, 5, 0), /* ID_AA64DFR0_EL1 */
ARM64_SYS_REG(3, 0, 0, 5, 1), /* ID_AA64DFR1_EL1 */
ARM64_SYS_REG(3, 0, 0, 5, 2),
@@ -440,7 +440,7 @@ static __u64 base_regs[] = {
ARM64_SYS_REG(3, 0, 0, 6, 0), /* ID_AA64ISAR0_EL1 */
ARM64_SYS_REG(3, 0, 0, 6, 1), /* ID_AA64ISAR1_EL1 */
ARM64_SYS_REG(3, 0, 0, 6, 2), /* ID_AA64ISAR2_EL1 */
- ARM64_SYS_REG(3, 0, 0, 6, 3),
+ ARM64_SYS_REG(3, 0, 0, 6, 3), /* ID_AA64ISAR3_EL1 */
ARM64_SYS_REG(3, 0, 0, 6, 4),
ARM64_SYS_REG(3, 0, 0, 6, 5),
ARM64_SYS_REG(3, 0, 0, 6, 6),
--
2.30.2
FEAT_FPMR defines a new register FMPR which is available at all ELs and is
discovered via ID_AA64PFR2_EL1.FPMR, add this to the set of registers that
get-reg-list knows to check for with the required identification register
depdendency.
Signed-off-by: Mark Brown <[email protected]>
---
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index 71ea6ecec7ce..1e43511d1440 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -40,6 +40,12 @@ static struct feature_id_reg feat_id_regs[] = {
ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */
4,
1
+ },
+ {
+ ARM64_SYS_REG(3, 3, 4, 4, 2), /* FPMR */
+ ARM64_SYS_REG(3, 0, 0, 4, 2), /* ID_AA64PFR2_EL1 */
+ 32,
+ 1
}
};
@@ -481,6 +487,7 @@ static __u64 base_regs[] = {
ARM64_SYS_REG(3, 3, 14, 2, 1), /* CNTP_CTL_EL0 */
ARM64_SYS_REG(3, 3, 14, 2, 2), /* CNTP_CVAL_EL0 */
ARM64_SYS_REG(3, 4, 3, 0, 0), /* DACR32_EL2 */
+ ARM64_SYS_REG(3, 3, 4, 4, 2), /* FPMR */
ARM64_SYS_REG(3, 4, 5, 0, 1), /* IFSR32_EL2 */
ARM64_SYS_REG(3, 4, 5, 3, 0), /* FPEXC32_EL2 */
};
--
2.30.2
On Fri, 29 Mar 2024 00:13:42 +0000,
Mark Brown <[email protected]> wrote:
>
> As part of the lazy FPSIMD state transitioning done by the hypervisor we
> currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with
> the host. Since this struct is non-extensible userspace ABI we have to keep
Using the same representation is just pure convenience, and nothing
requires us to use the it in the kernel/hypervisor.
> the definition as is but the addition of FPMR in the 2023 dpISA means that
> we will want to share more storage with the host. To facilitate this
> refactor the current code to share the entire thread->uw rather than just
> the one field.
So this increase the required sharing with EL2 from 528 bytes to
560. Not a huge deal, but definitely moving in the wrong direction. Is
there any plans to add more stuff to this structure that wouldn't be
*directly* relevant to the hypervisor?
>
> The large number of references to fpsimd_state make it very inconvenient
> to add an additional wrapper struct.
>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> arch/arm64/include/asm/kvm_host.h | 3 ++-
> arch/arm64/include/asm/processor.h | 2 +-
> arch/arm64/kvm/fpsimd.c | 13 ++++++-------
> arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++--
> 5 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 9e8a496fb284..8a251f0da900 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -27,6 +27,7 @@
> #include <asm/fpsimd.h>
> #include <asm/kvm.h>
> #include <asm/kvm_asm.h>
> +#include <asm/processor.h>
> #include <asm/vncr_mapping.h>
>
> #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> @@ -640,7 +641,7 @@ struct kvm_vcpu_arch {
> struct kvm_guest_debug_arch vcpu_debug_state;
> struct kvm_guest_debug_arch external_debug_state;
>
> - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */
> + struct thread_struct_uw *host_uw; /* hyp VA */
> struct task_struct *parent_task;
Well, this is going away, and you know it.
M.
--
Without deviation from the norm, progress is not possible.
On Fri, 29 Mar 2024 00:13:43 +0000,
Mark Brown <[email protected]> wrote:
>
> The 2023 architecture extensions have allocated some new ID registers, add
> them to the KVM system register descriptions so that they are visible to
> guests.
>
> We make the newly introduced dpISA features writeable, as well as
> allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
> introduces straigforward new instructions with no additional
> architectural state or traps.
FPMR actively gets trapped by HCRX_EL2.
>
> Signed-off-by: Mark Brown <[email protected]>
> ---
> arch/arm64/kvm/sys_regs.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c9f4f387155f..a3c20d1a36aa 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2293,12 +2293,15 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> ID_AA64PFR0_EL1_AdvSIMD |
> ID_AA64PFR0_EL1_FP), },
> ID_SANITISED(ID_AA64PFR1_EL1),
> - ID_UNALLOCATED(4,2),
> + ID_WRITABLE(ID_AA64PFR2_EL1, ~(ID_AA64PFR2_EL1_RES0 |
> + ID_AA64PFR2_EL1_MTEFAR |
> + ID_AA64PFR2_EL1_MTESTOREONLY |
> + ID_AA64PFR2_EL1_MTEPERM)),
> ID_UNALLOCATED(4,3),
> ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> ID_HIDDEN(ID_AA64SMFR0_EL1),
> ID_UNALLOCATED(4,6),
> - ID_UNALLOCATED(4,7),
> + ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
>
> /* CRm=5 */
> { SYS_DESC(SYS_ID_AA64DFR0_EL1),
> @@ -2325,7 +2328,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> ID_WRITABLE(ID_AA64ISAR2_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> ID_AA64ISAR2_EL1_APA3 |
> ID_AA64ISAR2_EL1_GPA3)),
> - ID_UNALLOCATED(6,3),
> + ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> + ID_AA64ISAR3_EL1_PACM |
> + ID_AA64ISAR3_EL1_TLBIW)),
> ID_UNALLOCATED(6,4),
> ID_UNALLOCATED(6,5),
> ID_UNALLOCATED(6,6),
>
Where is the code that enforces the lack of support for MTEFAR,
MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
And I haven't checked whether TLBI VMALLWS2 can be trapped.
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 02 Apr 2024 15:34:27 +0100,
Mark Brown <[email protected]> wrote:
>
> [1 <text/plain; us-ascii (quoted-printable)>]
> On Sun, Mar 31, 2024 at 11:00:41AM +0100, Marc Zyngier wrote:
> > Mark Brown <[email protected]> wrote:
>
> > > As part of the lazy FPSIMD state transitioning done by the hypervisor we
> > > currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with
> > > the host. Since this struct is non-extensible userspace ABI we have to keep
>
> > Using the same representation is just pure convenience, and nothing
> > requires us to use the it in the kernel/hypervisor.
>
> Indeed, the additional data seemed contained enough that it was a
> reasonable tradeoff.
>
> > > the definition as is but the addition of FPMR in the 2023 dpISA means that
> > > we will want to share more storage with the host. To facilitate this
> > > refactor the current code to share the entire thread->uw rather than just
> > > the one field.
>
> > So this increase the required sharing with EL2 from 528 bytes to
> > 560. Not a huge deal, but definitely moving in the wrong direction. Is
> > there any plans to add more stuff to this structure that wouldn't be
> > *directly* relevant to the hypervisor?
>
> I'm not aware of any current plans to extend this.
>
> > > @@ -640,7 +641,7 @@ struct kvm_vcpu_arch {
> > > struct kvm_guest_debug_arch vcpu_debug_state;
> > > struct kvm_guest_debug_arch external_debug_state;
> > >
> > > - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */
> > > + struct thread_struct_uw *host_uw; /* hyp VA */
> > > struct task_struct *parent_task;
>
> > Well, this is going away, and you know it.
>
> Sure, those patches are still in flight though. It does seem reasonable
> to target the current code.
Sure, if your intent is for this code not to be merged.
Because it means this series assumes a different data life cycle, and
the review effort spent on it will be invalidated once you move to the
per-CPU state.
M.
--
Without deviation from the norm, progress is not possible.
On Sun, Mar 31, 2024 at 11:00:41AM +0100, Marc Zyngier wrote:
> Mark Brown <[email protected]> wrote:
> > As part of the lazy FPSIMD state transitioning done by the hypervisor we
> > currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with
> > the host. Since this struct is non-extensible userspace ABI we have to keep
> Using the same representation is just pure convenience, and nothing
> requires us to use the it in the kernel/hypervisor.
Indeed, the additional data seemed contained enough that it was a
reasonable tradeoff.
> > the definition as is but the addition of FPMR in the 2023 dpISA means that
> > we will want to share more storage with the host. To facilitate this
> > refactor the current code to share the entire thread->uw rather than just
> > the one field.
> So this increase the required sharing with EL2 from 528 bytes to
> 560. Not a huge deal, but definitely moving in the wrong direction. Is
> there any plans to add more stuff to this structure that wouldn't be
> *directly* relevant to the hypervisor?
I'm not aware of any current plans to extend this.
> > @@ -640,7 +641,7 @@ struct kvm_vcpu_arch {
> > struct kvm_guest_debug_arch vcpu_debug_state;
> > struct kvm_guest_debug_arch external_debug_state;
> >
> > - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */
> > + struct thread_struct_uw *host_uw; /* hyp VA */
> > struct task_struct *parent_task;
> Well, this is going away, and you know it.
Sure, those patches are still in flight though. It does seem reasonable
to target the current code.
On Tue, Apr 02, 2024 at 03:53:33PM +0100, Marc Zyngier wrote:
> Mark Brown <[email protected]> wrote:
> > Sure, those patches are still in flight though. It does seem reasonable
> > to target the current code.
> Sure, if your intent is for this code not to be merged.
> Because it means this series assumes a different data life cycle, and
> the review effort spent on it will be invalidated once you move to the
> per-CPU state.
I don't have any visibility on when those patches are likely to get
merged or the general practices with in flight serieses here, last time
around with some of the serieses that were in flight it was quite late
which did make it unclear if things would go in during that release
cycle at all.
The amount of churn in KVM recently and long periods where the relevant
patches are apparently pre accepted but for various not always clear
reasons not actually merged is making it quite hard to target, you're
obviously going to be a lot more in the loop so this is doubtless
clearer to you than to me. It's also been a little unclear what the
expectations are for basing things on - some people do prefer to do
their own merging for example, and while you have mentioned your in
flight serieses your communication style means that it's not been
entirely clear if you're just noting the overlap. Is it just that
refactoring series you want taking into account here or are there other
in flight serieses that should be rolled into a base?
On Sun, Mar 31, 2024 at 11:59:06AM +0100, Marc Zyngier wrote:
> Mark Brown <[email protected]> wrote:
> > The 2023 architecture extensions have allocated some new ID registers, add
> > them to the KVM system register descriptions so that they are visible to
> > guests.
> > We make the newly introduced dpISA features writeable, as well as
> > allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
> > introduces straigforward new instructions with no additional
> > architectural state or traps.
> FPMR actively gets trapped by HCRX_EL2.
Sure, I'm not clear what you're trying to say here? The "no additional"
bit is referring to FEAT_CPA.
> > - ID_UNALLOCATED(6,3),
> > + ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> > + ID_AA64ISAR3_EL1_PACM |
> > + ID_AA64ISAR3_EL1_TLBIW)),
> > ID_UNALLOCATED(6,4),
> > ID_UNALLOCATED(6,5),
> > ID_UNALLOCATED(6,6),
> Where is the code that enforces the lack of support for MTEFAR,
> MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
Could you please be more explicit regarding what you're expecting to see
here? Other than the writeability mask for the ID register I would have
expected to need explicit code to enable new features rather than
explicit code to keep currently unsupported features unsupported. I'm
sure what you're referencing will be obvious once I see it but I'm
drawing a blank.
> And I haven't checked whether TLBI VMALLWS2 can be trapped.
I didn't see anything but I might not be aware of where to look, there
doesn't seem to be anything for that specifically in HFGITR_EL2 or
HFGITR2_EL2 which would be the main places I'd expect to find something.
On Tue, 02 Apr 2024 17:20:36 +0100,
Mark Brown <[email protected]> wrote:
>
> On Tue, Apr 02, 2024 at 03:53:33PM +0100, Marc Zyngier wrote:
> > Mark Brown <[email protected]> wrote:
>
> > > Sure, those patches are still in flight though. It does seem reasonable
> > > to target the current code.
>
> > Sure, if your intent is for this code not to be merged.
>
> > Because it means this series assumes a different data life cycle, and
> > the review effort spent on it will be invalidated once you move to the
> > per-CPU state.
>
> I don't have any visibility on when those patches are likely to get
> merged or the general practices with in flight serieses here, last time
> around with some of the serieses that were in flight it was quite late
> which did make it unclear if things would go in during that release
> cycle at all.
Here's a trick: you could ask. Other people do.
> The amount of churn in KVM recently and long periods where the relevant
> patches are apparently pre accepted but for various not always clear
Nothing is "pre accepted". Everything gets discussed and reviewed.
Specially when it comes to what you call "churn", which I call "crap
removal".
> reasons not actually merged is making it quite hard to target, you're
Things get merged when they are reviewed and ready. Not before.
> obviously going to be a lot more in the loop so this is doubtless
> clearer to you than to me. It's also been a little unclear what the
> expectations are for basing things on - some people do prefer to do
> their own merging for example, and while you have mentioned your in
This isn't about resolving a simple conflict. This is a fundamental
change in the way the state is tracked. We have argued about this for
months now, you were Cc'd on the patches addressing this problem, and
you even reviewed them. What other hint do you need?
> flight serieses your communication style means that it's not been
> entirely clear if you're just noting the overlap.
Not clear? That's a first. I'm usually seen as "blunt and assertive".
But I'll keep that in mind and aspire to greater clarity in the future.
> Is it just that
> refactoring series you want taking into account here or are there other
> in flight serieses that should be rolled into a base?
That, and the already merged feature enforcement framework which you
keep ignoring. I'll push out a rc3-based branch in to -next shortly so
that it is crystal clear what you need to base things on.
M.
--
Without deviation from the norm, progress is not possible.
On Tue, 02 Apr 2024 18:21:55 +0100,
Mark Brown <[email protected]> wrote:
>
> On Sun, Mar 31, 2024 at 11:59:06AM +0100, Marc Zyngier wrote:
> > Mark Brown <[email protected]> wrote:
>
> > > The 2023 architecture extensions have allocated some new ID registers, add
> > > them to the KVM system register descriptions so that they are visible to
> > > guests.
>
> > > We make the newly introduced dpISA features writeable, as well as
> > > allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
> > > introduces straigforward new instructions with no additional
> > > architectural state or traps.
>
> > FPMR actively gets trapped by HCRX_EL2.
>
> Sure, I'm not clear what you're trying to say here?
I'm saying (and not trying to say) that there are traps implied by the
features that you are adding.
> The "no additional" bit is referring to FEAT_CPA.
Well, that wasn't clear to me.
And when it comes to CPA, there are additional controls in SCTLR2_ELx,
which doesn't even gets context switched for EL1. What could possibly
go wrong?
>
> > > - ID_UNALLOCATED(6,3),
> > > + ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> > > + ID_AA64ISAR3_EL1_PACM |
> > > + ID_AA64ISAR3_EL1_TLBIW)),
> > > ID_UNALLOCATED(6,4),
> > > ID_UNALLOCATED(6,5),
> > > ID_UNALLOCATED(6,6),
>
> > Where is the code that enforces the lack of support for MTEFAR,
> > MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
>
> Could you please be more explicit regarding what you're expecting to see
> here?
I'm expecting you to add all the required masking and fine-grained
disabling of features that are not explicitly advertised to the guest.
This should translate into additional init code in kvm_init_sysreg(),
kvm_init_nv_sysregs() and limit_nv_id_reg(). You also should update
the exception triaging infrastructure in emulate-nested.c.
> Other than the writeability mask for the ID register I would have
> expected to need explicit code to enable new features rather than
> explicit code to keep currently unsupported features unsupported. I'm
> sure what you're referencing will be obvious once I see it but I'm
> drawing a blank.
>
> > And I haven't checked whether TLBI VMALLWS2 can be trapped.
>
> I didn't see anything but I might not be aware of where to look, there
> doesn't seem to be anything for that specifically in HFGITR_EL2 or
> HFGITR2_EL2 which would be the main places I'd expect to find
> something.
That's a really odd place to look. This is a S2 invalidation
primitive, which by definition is under the sole control of EL2, and
therefore cannot be trapped by any of the FGT registers, as they only
affect lesser-privileged ELs.
The instruction is described in the XML:
https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Instructions/TLBI-VMALLWS2E1--TLB-Invalidate-stage-2-dirty-state-by-VMID--EL1-0
M.
--
Without deviation from the norm, progress is not possible.
On Wed, Apr 10, 2024 at 11:32:02AM +0100, Marc Zyngier wrote:
> Mark Brown <[email protected]> wrote:
> > On Sun, Mar 31, 2024 at 11:59:06AM +0100, Marc Zyngier wrote:
> > > Mark Brown <[email protected]> wrote:
> And when it comes to CPA, there are additional controls in SCTLR2_ELx,
> which doesn't even gets context switched for EL1. What could possibly
> go wrong?
Yes, I'd missed those - they were rather buried in the XML
unfortunately. I'd already disabled CPA in my local code, and I've also
refactored the exposure of FPMR to be in the patch that context switches
it.
> > > > - ID_UNALLOCATED(6,3),
> > > > + ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> > > > + ID_AA64ISAR3_EL1_PACM |
> > > > ID_UNALLOCATED(6,4),
> > > > ID_UNALLOCATED(6,5),
> > > > ID_UNALLOCATED(6,6),
> > > Where is the code that enforces the lack of support for MTEFAR,
> > > MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
> > Could you please be more explicit regarding what you're expecting to see
> > here?
> I'm expecting you to add all the required masking and fine-grained
> disabling of features that are not explicitly advertised to the guest.
> This should translate into additional init code in kvm_init_sysreg(),
> kvm_init_nv_sysregs() and limit_nv_id_reg(). You also should update
I see that in limit_nv_id_reg() I am missing updates to expose the new
dpISA features to nested guests. However from a first pass it looks
like kvm_init_nv_sysregs() already handles everything I'd expect it to,
AFAICT it's handling all known trap bits? For kvm_init_sysreg() with
HCRX AFAICT we default to having all bits 0 with explicit relaxations
for supported features (currently FEAT_MOPS, also FEAT_FPMR with this
series) meaning that I'm still unclear what exactly the updates you're
looking for are. For SCTLR unless I'm misunderstanding things we've got
an existing issue with not initialising the res0 and res1 fields in
kvm_init_nv_sysregs() but that doesn't seem right...
> the exception triaging infrastructure in emulate-nested.c.
Again I am really struggling to identify which specific updates you are
looking for here.
> > > And I haven't checked whether TLBI VMALLWS2 can be trapped.
> > I didn't see anything but I might not be aware of where to look, there
> > doesn't seem to be anything for that specifically in HFGITR_EL2 or
> > HFGITR2_EL2 which would be the main places I'd expect to find
> > something.
> That's a really odd place to look. This is a S2 invalidation
> primitive, which by definition is under the sole control of EL2, and
> therefore cannot be trapped by any of the FGT registers, as they only
> affect lesser-privileged ELs.
> The instruction is described in the XML:
> https://developer.arm.com/documentation/ddi0601/2024-03/AArch64-Instructions/TLBI-VMALLWS2E1--TLB-Invalidate-stage-2-dirty-state-by-VMID--EL1-0
That's TLBI VMALLWSE1 which is a more specific instruction. TBH I can't
remember exactly what I was looking for, I did go into the instruction
pseudocode a bit (I was going in via SYS at one point) but didn't find
anything so was also trawling sysregs looking for something. If I'm
reading this right there are no traps?
On Wed, Apr 10, 2024 at 08:27:07AM +0100, Marc Zyngier wrote:
> Mark Brown <[email protected]> wrote:
> > On Tue, Apr 02, 2024 at 03:53:33PM +0100, Marc Zyngier wrote:
> > > Mark Brown <[email protected]> wrote:
> > > Because it means this series assumes a different data life cycle, and
> > > the review effort spent on it will be invalidated once you move to the
> > > per-CPU state.
> > I don't have any visibility on when those patches are likely to get
> > merged or the general practices with in flight serieses here, last time
> > around with some of the serieses that were in flight it was quite late
> > which did make it unclear if things would go in during that release
> > cycle at all.
> Here's a trick: you could ask. Other people do.
I do ask questions from time to time, and you do respond to some of
them, but in particular with "what's the status" type questions you have
given the impression that they're not super welcome. TBH my thinking
was just the standard submit against whatever is merged, rebase as
needed approach - I was of course aware of your series but also aware
that it was still in review.
> > The amount of churn in KVM recently and long periods where the relevant
> > patches are apparently pre accepted but for various not always clear
> Nothing is "pre accepted". Everything gets discussed and reviewed.
Sure, but then there's some things like your cleanup series which are
still in the discussion and review phase (or at least not applied) but
where you are saying you've got a strong expectation that other work
should be based on them. Most if not all of those are clearly going to
go in (as you indicate below with this one) but for whatever reason
haven't actually done so yet, it's them that I'm referring to here.
> Specially when it comes to what you call "churn", which I call "crap
> removal".
Indeed, a lot of things get churned for good reasons - the point here is
not that there aren't valuable improvements being made (there definitely
are here) but rather that it does make for a bit of a moving target.
> > obviously going to be a lot more in the loop so this is doubtless
> > clearer to you than to me. It's also been a little unclear what the
> > expectations are for basing things on - some people do prefer to do
> > their own merging for example, and while you have mentioned your in
> This isn't about resolving a simple conflict. This is a fundamental
> change in the way the state is tracked. We have argued about this for
I was on balance expecting to need to do the rebase at some point but
unclear what approach you and your comaintainers would want. TBH I
don't really see it as a huge update, refactoring with the current
approach in this series should be a *broadly* mechanical operation.
> months now, you were Cc'd on the patches addressing this problem, and
> you even reviewed them. What other hint do you need?
Honestly, them actually landing in git or a positive statement to the
effect of "these will go in after whatever, please base your work on
it" or whatever. It's not that I can't tell they're likely to go in at
some point, it's just that I'm targetting the current tree. This is a
fairly standard approach which many maintainers prefer.
> > flight serieses your communication style means that it's not been
> > entirely clear if you're just noting the overlap.
> Not clear? That's a first. I'm usually seen as "blunt and assertive".
> But I'll keep that in mind and aspire to greater clarity in the future.
Those aren't quite the words I would use though I do understand the
sentiment. One consequence of this style is that while it's obvious
that you don't like something it's not always quite so obvious what the
practical suggestion is, and since the general affect is so strong any
emphasis is not really visible.
More generally to this particular issue with rebasing on your series you
will have noticed that I have submitted a number of serieses which aimed
to address some of your review comments which you've rejected as not in
fact being things you were looking for.
> > Is it just that
> > refactoring series you want taking into account here or are there other
> > in flight serieses that should be rolled into a base?
> That, and the already merged feature enforcement framework which you
> keep ignoring. I'll push out a rc3-based branch in to -next shortly so
> that it is crystal clear what you need to base things on.
I really thought when I submitted this that I'd updated everything for
the feature enforcement framework now that it has actually landed, and
as I said in reply to the other patch apart from the nested idregs
(which I had missed) and the exposure of CPA (which did actually
introduce traps that I'd missed so I've updated to not expose CPA) I'm
still unclear what exactly it is that I have missed there.
Prior to that work landing this was all the same thing as I was
mentioning above with submitting against what's actually applied,
especially for serieses that aren't KVM specific where there's a higher
cost for pulling in extra dependencies early. I was aware that there
were changes coming but also aware that they were still not there yet.