Re: [v2 17/25] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Alex Williamson]

On Thu, 2014-12-11 at 05:55 +0000, Wu, Feng wrote:
> 
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> > Sent: Monday, December 08, 2014 1:21 PM
> > To: Wu, Feng
> > Cc: Eric Auger; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx;
> > x86@xxxxxxxxxx; gleb@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> > dwmw2@xxxxxxxxxxxxx; joro@xxxxxxxxxx; jiang.liu@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> > kvm@xxxxxxxxxxxxxxx
> > Subject: Re: [v2 17/25] KVM: kvm-vfio: User API for VT-d Posted-Interrupts
> > 
> > On Mon, 2014-12-08 at 04:58 +0000, Wu, Feng wrote:
> > >
> > > > -----Original Message-----
> > > > From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx]
> > On
> > > > Behalf Of Eric Auger
> > > > Sent: Thursday, December 04, 2014 10:05 PM
> > > > To: Wu, Feng; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx;
> > > > x86@xxxxxxxxxx; gleb@xxxxxxxxxx; pbonzini@xxxxxxxxxx;
> > > > dwmw2@xxxxxxxxxxxxx; joro@xxxxxxxxxx; alex.williamson@xxxxxxxxxx;
> > > > jiang.liu@xxxxxxxxxxxxxxx
> > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> > > > kvm@xxxxxxxxxxxxxxx
> > > > Subject: Re: [v2 17/25] KVM: kvm-vfio: User API for VT-d Posted-Interrupts
> > > >
> > > > Hi Feng,
> > > > On 12/03/2014 08:39 AM, Feng Wu wrote:
> > > > > This patch adds and documents a new attribute
> > > > > KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
> > group.
> > > > > This new attribute is used for VT-d Posted-Interrupts.
> > > > >
> > > > > When guest OS changes the interrupt configuration for an
> > > > > assigned device, such as, MSI/MSIx data/address fields,
> > > > > QEMU will use this IRQ attribute to tell KVM to update the
> > > > > related IRTE according the VT-d Posted-Interrrupts Specification,
> > > > > such as, the guest vector should be updated in the related IRTE.
> > > > >
> > > > > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > > > > ---
> > > > >  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> > > > >  include/uapi/linux/kvm.h                   |   10 ++++++++++
> > > > >  2 files changed, 19 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> > > > b/Documentation/virtual/kvm/devices/vfio.txt
> > > > > index f7aff29..41e12b7 100644
> > > > > --- a/Documentation/virtual/kvm/devices/vfio.txt
> > > > > +++ b/Documentation/virtual/kvm/devices/vfio.txt
> > > > > @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has
> > been
> > > > called to trigger the IRQ
> > > > >  or associate an eventfd to it. Unforwarding can only be called while the
> > > > >  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
> > condition
> > > > is
> > > > >  not satisfied, the command returns an -EBUSY.
> > > > > +
> > > > > +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> > > > mechanism to post
> > > > typo
> > > > > +                                   the IRQ to guests.
> > > > > +For this attribute, kvm_device_attr.addr points to a kvm_vfio_dev_irq
> > > > struct.
> > > > > +
> > > > > +When guest OS changes the interrupt configuration for an assigned
> > device,
> > > > > +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> > > > > +to tell KVM to update the related IRTE according the VT-d
> > > > Posted-Interrrupts
> > > > > +Specification, such as, the guest vector should be updated in the related
> > > > IRTE.
> > > > For my curiosity are there any restrictions about the instant at which
> > > > the change can be done?
> > > > I do not get here how you deactivate the posting?
> > >
> > > The current method is if the hardware supports interrupts posting, we will
> > > use it instead of interrupts remapping, since it has good performance. Why
> > > do I need deactivate interrupts posting?
> > >
> > > Here is the reply to Alex for the same question:
> > > "In fact, I don't think we need to stop the posted-interrupts. For setting
> > > posted interrupts, we update the related IRTE according to the new
> > > format. If the guest reboots, or unload the drivers, or some other
> > > operations, the msi/msix will be disabled first, in this path, the irq
> > > will be disabled the related IRTE is not used anymore."
> > 
> > Right, and I'm still not sure I agree with that reasoning.  We need to
> > build the kernel interface to be generic, not tailored for a specific
> > userspace.  I don't really feel comfortable having something that can't
> > be disabled via a similar path to it being enabled.  For instance, what
> > about a dynamic debug interface where we want to enable tracing and see
> > each interrupt injected into the guest.  At that point we'd want to
> > disabled posted interrupts and direct KVM injection and route via QEMU.
> > Thanks,
> > 
> > Alex
> 
> I am not quite understand why we need to debug the software
> delivery path for interrupt when PI is used, in this case, the software
> injection code will have no chance to execute. If we don't want the use
> PI, we can disable it from kernel command line.

Well, first off, I think it's just good interface design that if we
introduce the ability to enable something we also provide the ability to
disable it without making assumption about how a specific userspace is
expected to make use of the interface.  In this specific case, you're
missing the "dynamic" aspect of the switch.  What if we want to attach
gdb to the guest and at that point disable all KVM accelerations for an
assigned device so that we can see every register access and every
interrupt?  I don't think the interface proposed here allows for that
because it expects the guest to switch to a different interrupt mode for
the device in order to disable PI.  Don't design the interface tailored
only for a specific use case.  Thanks,

Alex

> > > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > > > index a269a42..7d98650 100644
> > > > > --- a/include/uapi/linux/kvm.h
> > > > > +++ b/include/uapi/linux/kvm.h
> > > > > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > > > >  #define  KVM_DEV_VFIO_DEVICE			2
> > > > >  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> > > > >  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> > > > > +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> > > > Maybe we should align our naming verb vs verbing here?
> > > > Best Regards
> > > > Eric
> > >
> > > No problem, I will align my patch in the next version. Thanks!
> > >
> > > Thanks,
> > > Feng
> > >
> > > > >
> > > > >  enum kvm_device_type {
> > > > >  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > > > > @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> > > > >  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> > > > >  };
> > > > >
> > > > > +struct kvm_vfio_dev_irq {
> > > > > +	__u32	argsz;
> > > > > +	__u32	fd;		/* file descriptor of the VFIO device */
> > > > > +	__u32	index;		/* VFIO device IRQ index */
> > > > > +	__u32	start;
> > > > > +	__u32	count;
> > > > > +	__u32	gsi[];		/* gsi, ie. virtual IRQ number */
> > > > > +};
> > > > > +
> > > > >  /*
> > > > >   * ioctls for VM fds
> > > > >   */
> > > > >
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/