Re: [PATCH v3 1/3] KVM: arm64: Finalize guest-wide sysregs prior to per-vCPU sysregs

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Sep 01, 2026 at 07:18:48PM +0100, Mark Brown wrote:
> In commit d82d09d5ba4b ("KVM: arm64: Don't skip per-vcpu NV
> initialisation") the NV register sanitisation was moved earlier in
> kvm_finalize_sys_regs() so that it runs for each vCPU rather than only
> once per guest. This means that for the first vCPU it runs prior to vGIC
> finalization, but the vGIC finalization updates the ID registers which
> the NV initialization uses so we may end up with a mismatch. For
> example, HFGRTR_EL2.ICC_IGRPENn_EL1 depends on GICv3 being enabled in
> ID_AA64PFR0_EL1.GIC so may be mistakenly marked or not marked as RES0.

Hand me some rope here, but I'm thinking this GIC case is because of:

kvm_init_nv_sysregs()
  -> get_reg_fixed_bits(kvm, HFGRTR_EL2)
    -> compute_reg_resx_bits()
      -> compute_resx_bits()
        [ does the dependency checks ]

?

Generally speaking it seems like a good idea that the broader system
registers are set up prior to nested in any case.

>
> Split the initialization which runs once per guest into a separate
> function and run that before the per-vCPU initialisation for NV,
> renaming the per-vCPU function to make it clear that it does per-vCPU
> setup.
>
> Fixes: d82d09d5ba4b ("KVM: arm64: Don't skip per-vcpu NV initialisation")
> Reviewed-by: Fuad Tabba <fuad.tabba@linux.dev>
> Tested-by: Fuad Tabba <fuad.tabba@linux.dev>
> Signed-off-by: Mark Brown <broonie@kernel.org>

One nit below but LGTM, so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  arch/arm64/kvm/arm.c      |  2 +-
>  arch/arm64/kvm/sys_regs.c | 40 ++++++++++++++++++++++++++--------------
>  arch/arm64/kvm/sys_regs.h |  2 +-
>  3 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 8b080804bc90..4f044280dec0 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -949,7 +949,7 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  			return ret;
>  	}
>
> -	ret = kvm_finalize_sys_regs(vcpu);
> +	ret = kvm_vcpu_finalize_sys_regs(vcpu);
>  	if (ret)
>  		return ret;
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 44aae52c473d..880f84248427 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -5861,25 +5861,14 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
>  }
>
>  /*
> - * Perform last adjustments to the ID registers that are implied by the
> + * Do system register finalization that is shared by the whole guest. This
> + * includes last adjustments to the ID registers that are implied by the
>   * configuration outside of the ID regs themselves, as well as any
>   * initialisation that directly depend on these ID registers (such as
>   * RES0/RES1 behaviours). This is not the place to configure traps though.
> - *
> - * Because this can be called once per CPU, changes must be idempotent.
>   */
> -int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
> +static int kvm_vm_finalize_sys_regs(struct kvm *kvm)
>  {
> -	struct kvm *kvm = vcpu->kvm;
> -
> -	guard(mutex)(&kvm->arch.config_lock);

NIT: Maybe worth a comment or an assert that the config_lock is held here?

> -
> -	if (vcpu_has_nv(vcpu)) {
> -		int ret = kvm_init_nv_sysregs(vcpu);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	if (kvm_vm_has_ran_once(kvm))
>  		return 0;
>
> @@ -5931,6 +5920,29 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>
> +/*
> + * Because this can be called once per CPU, changes must be idempotent.
> + */
> +int kvm_vcpu_finalize_sys_regs(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	int ret;
> +
> +	guard(mutex)(&kvm->arch.config_lock);
> +
> +	ret = kvm_vm_finalize_sys_regs(kvm);
> +	if (ret)
> +		return ret;
> +
> +	if (vcpu_has_nv(vcpu)) {
> +		ret = kvm_init_nv_sysregs(vcpu);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int __init kvm_sys_reg_table_init(void)
>  {
>  	const struct sys_reg_desc *gicv3_regs;
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index bd56a45abbf9..a3cccad2766f 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -254,7 +254,7 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
>
>  bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index);
>
> -int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu);
> +int kvm_vcpu_finalize_sys_regs(struct kvm_vcpu *vcpu);
>
>  #define AA32(_x)	.aarch32_map = AA32_##_x
>  #define Op0(_x) 	.Op0 = _x
>
> --
> 2.47.3
>
>

--
Cheers, Lorenzo