Re: [PATCH v3 01/23] KVM: arm64: Refactor idreg caching into dedicated structure
From: Marc Zyngier
Date: Sat Nov 14 2026 - 12:24:14 EST
On Fri, 06 Nov 2026 16:07:34 +0000,
Steffen Eiden <seiden@xxxxxxxxxxxxx> wrote:
>
> Move VM-wide ID register emulation fields from struct kvm_arch into a
> new struct kvm_vm_id_regs. The new struct is placed in a shared region.
> Update all users to use the new structure.
>
> No functional changes.
>
> Co-developed-by: Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx>
> Co-developed-by: Andreas Grapentin <gra@xxxxxxxxxxxxx>
> Signed-off-by: Andreas Grapentin <gra@xxxxxxxxxxxxx>
> Signed-off-by: Steffen Eiden <seiden@xxxxxxxxxxxxx>
> ---
> arch/arm64/include/asm/kvm_host.h | 50 ++++++++++++++++++++------------------
> arch/arm64/kvm/config.c | 10 ++++----
> arch/arm64/kvm/hyp/nvhe/pkvm.c | 7 +++---
> arch/arm64/kvm/hyp/nvhe/sys_regs.c | 4 +--
> arch/arm64/kvm/sys_regs.c | 2 +-
> 5 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 97332ffcb130..7f051dad79c7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -105,6 +105,25 @@ struct vcpu_reset_state {
> bool reset;
> };
>
> +struct kvm_vm_id_regs {
> + /*
> + * Emulated CPU ID registers per VM
> + * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> + * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> + *
> + * These emulated idregs are VM-wide, but accessed from the context of a vCPU.
> + * Atomic access to multiple idregs are guarded by kvm_arch.config_lock.
> + */
> +#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> +#define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> + u64 ftr_reg[KVM_ARM_ID_REG_NUM];
I'm OK with changing the name of the array, but then you need to be
consistent and change the name of the indexing macros throughout the
code. This should be a preliminary change.
> +
> + u64 midr_el1;
> + u64 revidr_el1;
> + u64 aidr_el1;
> + u64 ctr_el0;
> +};
> +
> #endif /* ARM64_S390_COMMON == 1 */
>
> #define KVM_DIRTY_LOG_MANUAL_CAPS (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
> @@ -478,22 +497,7 @@ struct kvm_arch {
> struct kvm_smccc_features smccc_feat;
> struct maple_tree smccc_filter;
>
> - /*
> - * Emulated CPU ID registers per VM
> - * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> - * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> - *
> - * These emulated idregs are VM-wide, but accessed from the context of a vCPU.
> - * Atomic access to multiple idregs are guarded by kvm_arch.config_lock.
> - */
> -#define IDREG_IDX(id) (((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> -#define KVM_ARM_ID_REG_NUM (IDREG_IDX(sys_reg(3, 0, 0, 7, 7)) + 1)
> - u64 id_regs[KVM_ARM_ID_REG_NUM];
> -
> - u64 midr_el1;
> - u64 revidr_el1;
> - u64 aidr_el1;
> - u64 ctr_el0;
> + struct kvm_vm_id_regs id_regs;
>
> /* Masks for VNCR-backed and general EL2 sysregs */
> struct kvm_sysreg_masks *sysreg_masks;
> @@ -1634,19 +1638,19 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
> bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
> bool kvm_pkvm_vcpu_is_powered_off(struct kvm_vcpu *vcpu);
>
> -static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
> +static inline u64 *__vm_id_reg(struct kvm_vm_id_regs *id_regs, u32 reg)
> {
> switch (reg) {
> case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
> - return &ka->id_regs[IDREG_IDX(reg)];
> + return &id_regs->ftr_reg[IDREG_IDX(reg)];
> case SYS_CTR_EL0:
> - return &ka->ctr_el0;
> + return &id_regs->ctr_el0;
> case SYS_MIDR_EL1:
> - return &ka->midr_el1;
> + return &id_regs->midr_el1;
> case SYS_REVIDR_EL1:
> - return &ka->revidr_el1;
> + return &id_regs->revidr_el1;
> case SYS_AIDR_EL1:
> - return &ka->aidr_el1;
> + return &id_regs->aidr_el1;
> default:
> WARN_ON_ONCE(1);
> return NULL;
> @@ -1654,7 +1658,7 @@ static inline u64 *__vm_id_reg(struct kvm_arch *ka, u32 reg)
> }
>
> #define kvm_read_vm_id_reg(kvm, reg) \
> - ({ u64 __val = *__vm_id_reg(&(kvm)->arch, reg); __val; })
> + ({ u64 __val = *__vm_id_reg(&(kvm)->arch.id_regs, reg); __val; })
>
> void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);
>
> diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
> index adaf26ab93b8..49bbe68c0b9e 100644
> --- a/arch/arm64/kvm/config.c
> +++ b/arch/arm64/kvm/config.c
> @@ -1467,11 +1467,11 @@ static bool idreg_apply_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
> resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);
> }
>
> - val = kvm->arch.id_regs[r->idreg_idx];
> - kvm->arch.id_regs[r->idreg_idx] &= ~resx.res0;
> - kvm->arch.id_regs[r->idreg_idx] |= resx.res1;
> + val = kvm->arch.id_regs.ftr_reg[r->idreg_idx];
> + kvm->arch.id_regs.ftr_reg[r->idreg_idx] &= ~resx.res0;
> + kvm->arch.id_regs.ftr_reg[r->idreg_idx] |= resx.res1;
>
> - return val != kvm->arch.id_regs[r->idreg_idx];
> + return val != kvm->arch.id_regs.ftr_reg[r->idreg_idx];
> }
>
> void kvm_idregs_enforce_dependencies(struct kvm *kvm)
> @@ -1488,7 +1488,7 @@ void kvm_idregs_enforce_dependencies(struct kvm *kvm)
>
> static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
> {
> - u64 regval = kvm->arch.id_regs[map->regidx];
> + u64 regval = kvm->arch.id_regs.ftr_reg[map->regidx];
> u64 regfld = (regval >> map->shift) & GENMASK(map->width - 1, 0);
>
> if (map->sign) {
All this stuff should really be cleaned up by introducing the correct
level of abstraction. I have a local untested hack that does this:
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index ed00637f1d7a8..65d34e2da17d2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1639,11 +1639,16 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
bool kvm_pkvm_vcpu_is_powered_off(struct kvm_vcpu *vcpu);
+static inline u64 *__vm_ftr_reg_by_idx(struct kvm_vm_id_regs *id_regs, u32 idx)
+{
+ return &id_regs->ftr_reg[idx];
+}
+
static inline u64 *__vm_id_reg(struct kvm_vm_id_regs *id_regs, u32 reg)
{
switch (reg) {
case sys_reg(3, 0, 0, 1, 0) ... sys_reg(3, 0, 0, 7, 7):
- return &id_regs->ftr_reg[IDREG_IDX(reg)];
+ return __vm_ftr_reg_by_idx(id_regs, IDREG_IDX(reg));
case SYS_CTR_EL0:
return &id_regs->ctr_el0;
case SYS_MIDR_EL1:
@@ -1658,8 +1663,14 @@ static inline u64 *__vm_id_reg(struct kvm_vm_id_regs *id_regs, u32 reg)
}
}
-#define kvm_read_vm_id_reg(kvm, reg) \
- ({ u64 __val = *__vm_id_reg(&(kvm)->arch.id_regs, reg); __val; })
+#define kvm_get_vm_ftr_reg_by_idx(kvm, idx) \
+ ({ u64 *__valp = __vm_ftr_reg_by_idx(&(kvm)->arch.id_regs, idx); __valp; })
+
+#define kvm_get_vm_id_reg(kvm, reg) \
+ ({ u64 *__valp = __vm_id_reg(&(kvm)->arch.id_regs, reg); __valp; })
+
+#define kvm_read_vm_id_reg(kvm, reg) \
+ ({ u64 __val = *kvm_get_vm_id_reg(kvm, reg); __val; })
void kvm_set_vm_id_reg(struct kvm *kvm, u32 reg, u64 val);
diff --git a/arch/arm64/kvm/config.c b/arch/arm64/kvm/config.c
index 399693203e934..37eae14c3924a 100644
--- a/arch/arm64/kvm/config.c
+++ b/arch/arm64/kvm/config.c
@@ -1565,7 +1565,7 @@ static const struct reg_feat_map_desc *idreg_feat_maps[] = {
static bool idreg_apply_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
{
struct resx resx = {};
- u64 val;
+ u64 val, *valp;
for (int i = 0; i < r->bit_feat_map_sz; i++) {
if ((r->bit_feat_map[i].flags & CALL_FUNC) &&
@@ -1573,11 +1573,12 @@ static bool idreg_apply_deps(struct kvm *kvm, const struct reg_feat_map_desc *r)
resx.res0 |= reg_feat_map_bits(&r->bit_feat_map[i]);
}
- val = kvm->arch.id_regs.ftr_reg[r->idreg_idx];
- kvm->arch.id_regs.ftr_reg[r->idreg_idx] &= ~resx.res0;
- kvm->arch.id_regs.ftr_reg[r->idreg_idx] |= resx.res1;
+ valp = kvm_get_vm_ftr_reg_by_idx(kvm, r->idreg_idx);
+ val = *valp;
+ *valp &= ~resx.res0;
+ *valp |= resx.res1;
- return val != kvm->arch.id_regs.ftr_reg[r->idreg_idx];
+ return val != *valp;
}
void kvm_idregs_enforce_dependencies(struct kvm *kvm)
@@ -1594,7 +1595,7 @@ void kvm_idregs_enforce_dependencies(struct kvm *kvm)
static bool idreg_feat_match(struct kvm *kvm, const struct reg_bits_to_feat_map *map)
{
- u64 regval = kvm->arch.id_regs.ftr_reg[map->regidx];
+ u64 regval = *kvm_get_vm_ftr_reg_by_idx(kvm, map->regidx);
u64 regfld = (regval >> map->shift) & GENMASK(map->width - 1, 0);
if (map->sign) {
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index fc0475f0877d..41ee83290697 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -399,7 +399,7 @@ static void pkvm_init_features_from_host(struct pkvm_hyp_vm *hyp_vm, const struc
> DECLARE_BITMAP(allowed_features, KVM_VCPU_MAX_FEATURES);
>
> /* CTR_EL0 is always under host control, even for protected VMs. */
> - hyp_vm->kvm.arch.ctr_el0 = host_kvm->arch.ctr_el0;
> + hyp_vm->kvm.arch.id_regs.ctr_el0 = host_kvm->arch.id_regs.ctr_el0;
This stuff is just terrible code, and needs to be cleaned up. It
should read something like:
diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
index ef936dee20ad3..ad100df6a8b0d 100644
--- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
+++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
@@ -413,7 +413,7 @@ static void pkvm_init_features_from_host(struct pkvm_hyp_vm *hyp_vm, const struc
DECLARE_BITMAP(allowed_features, KVM_VCPU_MAX_FEATURES);
/* CTR_EL0 is always under host control, even for protected VMs. */
- hyp_vm->kvm.arch.id_regs.ctr_el0 = host_kvm->arch.id_regs.ctr_el0;
+ *kvm_get_vm_id_reg(kvm, SYS_CTR_EL0) = kvm_read_vm_id_reg((struct kvm *)host_kvm, SYS_CTR_EL0);
/*
* pKVM runs only GICv3. Anything else becomes 0, the model of a VM
Overall, I'd like to see a first patch introducing helpers that the
code uses everywhere possible, then another patch that introduces the
structure, the renaming, and changes the accessors without touching
the rest of the code.
Thanks,
M.
--
Jazz isn't dead. It just smells funny.