Re: [PATCH] kvm: Check irqchip mode before assign irqfd

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/05/19 10:56, Peter Xu wrote:
> When assigning kvm irqfd we didn't check the irqchip mode but we allow
> KVM_IRQFD to succeed with all the irqchip modes.  However it does not
> make much sense to create irqfd even without the kernel chips.  Let's
> provide a arch-dependent helper to check whether a specific irqfd is
> allowed by the arch.  At least for x86, it should make sense to check:
> 
> - when irqchip mode is NONE, all irqfds should be disallowed, and,
> 
> - when irqchip mode is SPLIT, irqfds that are with resamplefd should
>   be disallowed.
> 
> For either of the case, previously we'll silently ignore the irq or
> the irq ack event if the irqchip mode is incorrect.  However that can
> cause misterious guest behaviors and it can be hard to triage.  Let's
> fail KVM_IRQFD even earlier to detect these incorrect configurations.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Radim Krčmář <rkrcmar@redhat.com>
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  arch/x86/kvm/irq.c | 7 +++++++
>  arch/x86/kvm/irq.h | 1 +
>  virt/kvm/eventfd.c | 9 +++++++++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index faa264822cee..007bc654f928 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -172,3 +172,10 @@ void __kvm_migrate_timers(struct kvm_vcpu *vcpu)
>  	__kvm_migrate_apic_timer(vcpu);
>  	__kvm_migrate_pit_timer(vcpu);
>  }
> +
> +bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
> +{
> +	bool resample = args->flags & KVM_IRQFD_FLAG_RESAMPLE;
> +
> +	return resample ? irqchip_kernel(kvm) : irqchip_in_kernel(kvm);
> +}
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index d5005cc26521..fd210cdd4983 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -114,6 +114,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return mode != KVM_IRQCHIP_NONE;
>  }
>  
> +bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args);
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
>  void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu);
>  void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 001aeda4c154..3972a9564c76 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -44,6 +44,12 @@
>  
>  static struct workqueue_struct *irqfd_cleanup_wq;
>  
> +bool __attribute__((weak))
> +kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args)
> +{
> +	return true;
> +}
> +
>  static void
>  irqfd_inject(struct work_struct *work)
>  {
> @@ -297,6 +303,9 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
>  	if (!kvm_arch_intc_initialized(kvm))
>  		return -EAGAIN;
>  
> +	if (!kvm_arch_irqfd_allowed(kvm, args))
> +		return -EINVAL;
> +
>  	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL_ACCOUNT);
>  	if (!irqfd)
>  		return -ENOMEM;
> 

Queued, thanks.

Paolo