Re: [PATCH v3] KVM: Introduce direct MSI message injection for in-kernel irqchips

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Mar 29, 2012 at 06:15:41PM +0200, Jan Kiszka wrote:
> Currently, MSI messages can only be injected to in-kernel irqchips by
> defining a corresponding IRQ route for each message. This is not only
> unhandy if the MSI messages are generated "on the fly" by user space,

The MSI message format is configured on device configuration, and once
its settled, does not change. This should be an unfrequent operation,
no? (i am trying to understand what you mean by "on the fly" here).

If that is the case, the real problem is that irq routing tables do not
handle large numbers of vectors? And isnt that limitation also an issue
if you'd like to add more IOAPICs, for example?

> IRQ routes are a limited resource that user space as to manage
> carefully.
> 
> By providing a direct injection path, we can both avoid using up limited
> resources and simplify the necessary steps for user land.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v3:
>  - align return code doc to reality
>  - rename SET_MSI -> SIGNAL_MSI
> 
>  Documentation/virtual/kvm/api.txt |   21 +++++++++++++++++++++
>  arch/x86/kvm/Kconfig              |    1 +
>  include/linux/kvm.h               |   11 +++++++++++
>  virt/kvm/Kconfig                  |    3 +++
>  virt/kvm/kvm_main.c               |   21 +++++++++++++++++++++
>  5 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 81ff39f..ed27d1b 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1482,6 +1482,27 @@ See KVM_ASSIGN_DEV_IRQ for the data structure.  The target device is specified
>  by assigned_dev_id.  In the flags field, only KVM_DEV_ASSIGN_MASK_INTX is
>  evaluated.
>  
> +4.61 KVM_SIGNAL_MSI
> +
> +Capability: KVM_CAP_SIGNAL_MSI
> +Architectures: x86
> +Type: vm ioctl
> +Parameters: struct kvm_msi (in)
> +Returns: >0 on delivery, 0 if guest blocked the MSI, and -1 on error
> +
> +Directly inject a MSI message. Only valid with in-kernel irqchip that handles
> +MSI messages.
> +
> +struct kvm_msi {
> +	__u32 address_lo;
> +	__u32 address_hi;
> +	__u32 data;
> +	__u32 flags;
> +	__u8  pad[16];
> +};
> +
> +No flags are defined so far. The corresponding field must be 0.
> +
>  4.62 KVM_CREATE_SPAPR_TCE
>  
>  Capability: KVM_CAP_SPAPR_TCE
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index 1a7fe86..a28f338 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -36,6 +36,7 @@ config KVM
>  	select TASKSTATS
>  	select TASK_DELAY_ACCT
>  	select PERF_EVENTS
> +	select HAVE_KVM_MSI
>  	---help---
>  	  Support hosting fully virtualized guest machines using hardware
>  	  virtualization extensions.  You will need a fairly recent
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 7a9dd4b..225b452 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -590,6 +590,7 @@ struct kvm_ppc_pvinfo {
>  #define KVM_CAP_SYNC_REGS 74
>  #define KVM_CAP_PCI_2_3 75
>  #define KVM_CAP_KVMCLOCK_CTRL 76
> +#define KVM_CAP_SIGNAL_MSI 77
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -715,6 +716,14 @@ struct kvm_one_reg {
>  	__u64 addr;
>  };
>  
> +struct kvm_msi {
> +	__u32 address_lo;
> +	__u32 address_hi;
> +	__u32 data;
> +	__u32 flags;
> +	__u8  pad[16];
> +};
> +
>  /*
>   * ioctls for VM fds
>   */
> @@ -789,6 +798,8 @@ struct kvm_s390_ucas_mapping {
>  /* Available with KVM_CAP_PCI_2_3 */
>  #define KVM_ASSIGN_SET_INTX_MASK  _IOW(KVMIO,  0xa4, \
>  				       struct kvm_assigned_pci_dev)
> +/* Available with KVM_CAP_SIGNAL_MSI */
> +#define KVM_SIGNAL_MSI            _IOW(KVMIO,  0xa5, struct kvm_msi)
>  
>  /*
>   * ioctls for vcpu fds
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index f63ccb0..28694f4 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -18,3 +18,6 @@ config KVM_MMIO
>  
>  config KVM_ASYNC_PF
>         bool
> +
> +config HAVE_KVM_MSI
> +       bool
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a612bc8..3aeb7ab 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2063,6 +2063,24 @@ static long kvm_vm_ioctl(struct file *filp,
>  		mutex_unlock(&kvm->lock);
>  		break;
>  #endif
> +#ifdef CONFIG_HAVE_KVM_MSI
> +	case KVM_SIGNAL_MSI: {
> +		struct kvm_kernel_irq_routing_entry route;
> +		struct kvm_msi msi;

Zero them (future proof).

> +
> +		r = -EFAULT;
> +		if (copy_from_user(&msi, argp, sizeof msi))
> +			goto out;
> +		r = -EINVAL;
> +		if (!irqchip_in_kernel(kvm) || msi.flags != 0)
> +			goto out;
> +		route.msi.address_lo = msi.address_lo;
> +		route.msi.address_hi = msi.address_hi;
> +		route.msi.data = msi.data;
> +		r =  kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);

extra whitespace.

> +		break;
> +	}

Have you checked that direct MSI injection does not make use of 
IRQ routing data structures, such as for acking? 

irqchip_in_kernel(kvm) returns true before kvm->irq_routing is 
actually in place. With kvm_set_irq there is no problem, but now
there is another path into injection.

The real purpose of this is not entirely clear (and as Avi mentioned two
interfaces should be avoided if possible).