Re: [PATCH 11/16] KVM: arm64: nv: Add Maintenance Interrupt emulation

[Marc Zyngier <maz@kernel.org>]

On Tue, 17 Dec 2024 15:13:26 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> Emulating the vGIC means emulating the dreaded Maintenance Interrupt.
> 
> This is a two-pronged problem:
> 
> - while running L2, getting an MI translates into an MI injected
>   in the L1 based on the state of the HW.
> 
> - while running L1, we must accurately reflect the state of the
>   MI line, based on the in-memory state.
> 
> The MI INTID is added to the distributor, as expected on any
> virtualisation-capable implementation, and further patches
> will allow its configuration.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arm.c                 |  6 ++++
>  arch/arm64/kvm/vgic/vgic-init.c      | 22 ++++++++++++++
>  arch/arm64/kvm/vgic/vgic-v3-nested.c | 45 ++++++++++++++++++++++++++++
>  arch/arm64/kvm/vgic/vgic.c           |  9 ++++++
>  arch/arm64/kvm/vgic/vgic.h           |  2 ++
>  include/kvm/arm_vgic.h               |  4 +++
>  6 files changed, 88 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 5e353b2c225b4..756cc4e74e10f 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -824,6 +824,12 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  	if (ret)
>  		return ret;
>  
> +	if (vcpu_has_nv(vcpu)) {
> +		ret = kvm_vgic_vcpu_nv_init(vcpu);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * This needs to happen after any restriction has been applied
>  	 * to the feature set.
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index bc7e22ab5d812..d2724315a70e9 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -180,6 +180,20 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
>  	return 0;
>  }
>  
> +int kvm_vgic_vcpu_nv_init(struct kvm_vcpu *vcpu)
> +{
> +	int ret;
> +
> +	guard(mutex)(&vcpu->kvm->arch.config_lock);
> +
> +	/* Cope with vintage userspace. Maybe we should fail instead */
> +	if (vcpu->kvm->arch.vgic.maint_irq == 0)
> +		vcpu->kvm->arch.vgic.maint_irq = kvm_vgic_global_state.maint_irq;

Well, that didn't take too long, and Joey did hit a bug here. Although
the two fields have the same name, they represent something totally
different:

- kvm_vgic_global_state.maint_irq is the interrupt number Linux uses
  for the host. It's just a number, without any signification other
  than for the kernel's own stuff.

- vcpu->kvm->arch.vgic.maint_irq is an GIC INTID. Really not the same
  thing as the above.

What this should read is something like:

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index d2724315a70e9..8d92b05639588 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -188,7 +188,7 @@ int kvm_vgic_vcpu_nv_init(struct kvm_vcpu *vcpu)
 
 	/* Cope with vintage userspace. Maybe we should fail instead */
 	if (vcpu->kvm->arch.vgic.maint_irq == 0)
-		vcpu->kvm->arch.vgic.maint_irq = kvm_vgic_global_state.maint_irq;
+		vcpu->kvm->arch.vgic.maint_irq = irq_get_irq_data(kvm_vgic_global_state.maint_irq)->hwirq;
 	ret = kvm_vgic_set_owner(vcpu, vcpu->kvm->arch.vgic.maint_irq, vcpu);
 
 	return ret;

So I guess there are two things that need to happen:

- rename vcpu->kvm->arch.vgic.maint_irq to something like "mi_intid"

- make the damn thing fail, as the comment suggests. We don't have any
  legacy worth speaking of anyway.

Another thing is that Joey (who really didn't waste any time finding
issues) pointed out that we happily allow NV without GICv3. Clearly,
this should also be prevented.

Thanks Joey!

	M.

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