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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Apr 12, 2012 at 11:28:09AM +0200, Jan Kiszka wrote:
> On 2012-04-12 00:10, Marcelo Tosatti wrote:
> > 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).
> 
> The point is that you need to track those changes and/or provide
> mechanism to cache routing information related to a specific MSI source.
> That either means patching the full path from the source to the sink or
> paying some price on injection.
> 
> > 
> > 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?
> 
> For sure, more IRQ lines will also contribute to the shortage of GSIs.
> Yet another reason to avoid wasting them on userspace generated MSIs.
> 
> > 
> >> 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? 
> 
> See kvm_set_msi: The routing structure is only read in the context of
> that function, no reference is kept.

I was thinking more along the lines of inconsistent state
due to races.

- set MSI for IOAPIC handled vector before kvm->irq_routing 
is assigned.
- IOAPIC EOI for that vector.
- EOI handler expects kvm->irq_routing present.

> > 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).
> 
> See [1] for an implementation of one of Avi's proposals.

Just now that i start to appreciate KVM_SIGNAL_MSI.

"so we have a single ioctl for all interrupt handling.  This allows
eventual removal of the line-oriented ioctls." 

So you move from one interface that handles both MSI/INTx, to _another_
interface that handles both. KVM_SIGNAL_MSI with address/data is clean
and obvious.