diff mbox

[v9,12/18] vfio: Register/unregister irq_bypass_producer

Message ID 1442586596-5920-13-git-send-email-feng.wu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wu, Feng Sept. 18, 2015, 2:29 p.m. UTC
This patch adds the registration/unregistration of an
irq_bypass_producer for MSI/MSIx on vfio pci devices.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v8:
- Merge "[PATCH v7 08/17] vfio: Select IRQ_BYPASS_MANAGER for vfio PCI devices"
  into this patch.

v6:
- Make the add_consumer and del_consumer callbacks static
- Remove pointless INIT_LIST_HEAD to 'vdev->ctx[vector].producer.node)'
- Use dev_info instead of WARN_ON() when irq_bypass_register_producer fails
- Remove optional dummy callbacks for irq producer

 drivers/vfio/pci/Kconfig            | 1 +
 drivers/vfio/pci/vfio_pci_intrs.c   | 9 +++++++++
 drivers/vfio/pci/vfio_pci_private.h | 2 ++
 3 files changed, 12 insertions(+)

Comments

Alex Williamson Sept. 18, 2015, 5:19 p.m. UTC | #1
On Fri, 2015-09-18 at 22:29 +0800, Feng Wu wrote:
> This patch adds the registration/unregistration of an
> irq_bypass_producer for MSI/MSIx on vfio pci devices.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>

On nit, Paolo could you please fix the spelling of "registration" in the
dev_info, otherwise:

Acked-by: Alex Williamson <alex.williamson@redhat.com>


> ---
> v8:
> - Merge "[PATCH v7 08/17] vfio: Select IRQ_BYPASS_MANAGER for vfio PCI devices"
>   into this patch.
> 
> v6:
> - Make the add_consumer and del_consumer callbacks static
> - Remove pointless INIT_LIST_HEAD to 'vdev->ctx[vector].producer.node)'
> - Use dev_info instead of WARN_ON() when irq_bypass_register_producer fails
> - Remove optional dummy callbacks for irq producer
> 
>  drivers/vfio/pci/Kconfig            | 1 +
>  drivers/vfio/pci/vfio_pci_intrs.c   | 9 +++++++++
>  drivers/vfio/pci/vfio_pci_private.h | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index 579d83b..02912f1 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -2,6 +2,7 @@ config VFIO_PCI
>  	tristate "VFIO support for PCI devices"
>  	depends on VFIO && PCI && EVENTFD
>  	select VFIO_VIRQFD
> +	select IRQ_BYPASS_MANAGER
>  	help
>  	  Support for the PCI VFIO bus driver.  This is required to make
>  	  use of PCI drivers using the VFIO framework.
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 1f577b4..c65299d 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -319,6 +319,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
>  
>  	if (vdev->ctx[vector].trigger) {
>  		free_irq(irq, vdev->ctx[vector].trigger);
> +		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
>  		kfree(vdev->ctx[vector].name);
>  		eventfd_ctx_put(vdev->ctx[vector].trigger);
>  		vdev->ctx[vector].trigger = NULL;
> @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
>  		return ret;
>  	}
>  
> +	vdev->ctx[vector].producer.token = trigger;
> +	vdev->ctx[vector].producer.irq = irq;
> +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> +	if (unlikely(ret))
> +		dev_info(&pdev->dev,
> +		"irq bypass producer (token %p) registeration fails: %d\n",
> +		vdev->ctx[vector].producer.token, ret);
> +
>  	vdev->ctx[vector].trigger = trigger;
>  
>  	return 0;
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index ae0e1b4..0e7394f 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -13,6 +13,7 @@
>  
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
> +#include <linux/irqbypass.h>
>  
>  #ifndef VFIO_PCI_PRIVATE_H
>  #define VFIO_PCI_PRIVATE_H
> @@ -29,6 +30,7 @@ struct vfio_pci_irq_ctx {
>  	struct virqfd		*mask;
>  	char			*name;
>  	bool			masked;
> +	struct irq_bypass_producer	producer;
>  };
>  
>  struct vfio_pci_device {



--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu, Feng Sept. 21, 2015, 8:56 a.m. UTC | #2
Hi Paolo & Alex,

I find that there is a build error in the following two cases:
- KVM is configured as 'M' and VFIO as 'Y'
The reason is the build of irqbypass manager is triggered in
arch/x86/kvm/Makefile, and VFIO is built before KVM, hence
it cannot find the symbols in irqbypass manager.

- Disable KVM and enable VFIO in .config
The reason is similar with the above one, the irqbypass manager
is not built since KVM is not configured.

I think the point is that we cannot trigger the build of irqbypass
manager inside KVM or VFIO, we need trigger the build at a high
level and it should be built before VFIO and KVM. Any ideas?

Thanks,
Feng

> -----Original Message-----
> From: Wu, Feng
> Sent: Friday, September 18, 2015 10:30 PM
> To: pbonzini@redhat.com; alex.williamson@redhat.com; joro@8bytes.org;
> mtosatti@redhat.com
> Cc: eric.auger@linaro.org; kvm@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org; Wu, Feng
> Subject: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> 
> This patch adds the registration/unregistration of an
> irq_bypass_producer for MSI/MSIx on vfio pci devices.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> v8:
> - Merge "[PATCH v7 08/17] vfio: Select IRQ_BYPASS_MANAGER for vfio PCI
> devices"
>   into this patch.
> 
> v6:
> - Make the add_consumer and del_consumer callbacks static
> - Remove pointless INIT_LIST_HEAD to 'vdev->ctx[vector].producer.node)'
> - Use dev_info instead of WARN_ON() when irq_bypass_register_producer fails
> - Remove optional dummy callbacks for irq producer
> 
>  drivers/vfio/pci/Kconfig            | 1 +
>  drivers/vfio/pci/vfio_pci_intrs.c   | 9 +++++++++
>  drivers/vfio/pci/vfio_pci_private.h | 2 ++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
> index 579d83b..02912f1 100644
> --- a/drivers/vfio/pci/Kconfig
> +++ b/drivers/vfio/pci/Kconfig
> @@ -2,6 +2,7 @@ config VFIO_PCI
>  	tristate "VFIO support for PCI devices"
>  	depends on VFIO && PCI && EVENTFD
>  	select VFIO_VIRQFD
> +	select IRQ_BYPASS_MANAGER
>  	help
>  	  Support for the PCI VFIO bus driver.  This is required to make
>  	  use of PCI drivers using the VFIO framework.
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 1f577b4..c65299d 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -319,6 +319,7 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_device *vdev,
> 
>  	if (vdev->ctx[vector].trigger) {
>  		free_irq(irq, vdev->ctx[vector].trigger);
> +		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
>  		kfree(vdev->ctx[vector].name);
>  		eventfd_ctx_put(vdev->ctx[vector].trigger);
>  		vdev->ctx[vector].trigger = NULL;
> @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_device *vdev,
>  		return ret;
>  	}
> 
> +	vdev->ctx[vector].producer.token = trigger;
> +	vdev->ctx[vector].producer.irq = irq;
> +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> +	if (unlikely(ret))
> +		dev_info(&pdev->dev,
> +		"irq bypass producer (token %p) registeration fails: %d\n",
> +		vdev->ctx[vector].producer.token, ret);
> +
>  	vdev->ctx[vector].trigger = trigger;
> 
>  	return 0;
> diff --git a/drivers/vfio/pci/vfio_pci_private.h
> b/drivers/vfio/pci/vfio_pci_private.h
> index ae0e1b4..0e7394f 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -13,6 +13,7 @@
> 
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
> +#include <linux/irqbypass.h>
> 
>  #ifndef VFIO_PCI_PRIVATE_H
>  #define VFIO_PCI_PRIVATE_H
> @@ -29,6 +30,7 @@ struct vfio_pci_irq_ctx {
>  	struct virqfd		*mask;
>  	char			*name;
>  	bool			masked;
> +	struct irq_bypass_producer	producer;
>  };
> 
>  struct vfio_pci_device {
> --
> 2.1.0

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Sept. 21, 2015, 9:32 a.m. UTC | #3
On 21/09/2015 10:56, Wu, Feng wrote:
> Hi Paolo & Alex,
> 
> I find that there is a build error in the following two cases:
> - KVM is configured as 'M' and VFIO as 'Y'
> The reason is the build of irqbypass manager is triggered in
> arch/x86/kvm/Makefile, and VFIO is built before KVM, hence
> it cannot find the symbols in irqbypass manager.
> 
> - Disable KVM and enable VFIO in .config
> The reason is similar with the above one, the irqbypass manager
> is not built since KVM is not configured.
> 
> I think the point is that we cannot trigger the build of irqbypass
> manager inside KVM or VFIO, we need trigger the build at a high
> level and it should be built before VFIO and KVM. Any ideas?

We can add virt/Makefile and build virt/lib/ directly, not through
arch/x86/kvm.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu, Feng Sept. 21, 2015, 11:35 a.m. UTC | #4
> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, September 21, 2015 5:32 PM
> To: Wu, Feng; alex.williamson@redhat.com; joro@8bytes.org;
> mtosatti@redhat.com
> Cc: eric.auger@linaro.org; kvm@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> 
> 
> 
> On 21/09/2015 10:56, Wu, Feng wrote:
> > Hi Paolo & Alex,
> >
> > I find that there is a build error in the following two cases:
> > - KVM is configured as 'M' and VFIO as 'Y'
> > The reason is the build of irqbypass manager is triggered in
> > arch/x86/kvm/Makefile, and VFIO is built before KVM, hence
> > it cannot find the symbols in irqbypass manager.
> >
> > - Disable KVM and enable VFIO in .config
> > The reason is similar with the above one, the irqbypass manager
> > is not built since KVM is not configured.
> >
> > I think the point is that we cannot trigger the build of irqbypass
> > manager inside KVM or VFIO, we need trigger the build at a high
> > level and it should be built before VFIO and KVM. Any ideas?
> 
> We can add virt/Makefile and build virt/lib/ directly, not through
> arch/x86/kvm.

Yes, that can solve the build error. Should I send a new version?

Thanks,
Feng

> 
> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Sept. 21, 2015, 12:06 p.m. UTC | #5
On 21/09/2015 13:35, Wu, Feng wrote:
>>> > > I think the point is that we cannot trigger the build of irqbypass
>>> > > manager inside KVM or VFIO, we need trigger the build at a high
>>> > > level and it should be built before VFIO and KVM. Any ideas?
>> > 
>> > We can add virt/Makefile and build virt/lib/ directly, not through
>> > arch/x86/kvm.
> Yes, that can solve the build error. Should I send a new version?

You can send a separate patch on top of this v9.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu, Feng Sept. 21, 2015, 12:08 p.m. UTC | #6
> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, September 21, 2015 8:07 PM
> To: Wu, Feng; alex.williamson@redhat.com; joro@8bytes.org;
> mtosatti@redhat.com
> Cc: eric.auger@linaro.org; kvm@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> 
> 
> 
> On 21/09/2015 13:35, Wu, Feng wrote:
> >>> > > I think the point is that we cannot trigger the build of irqbypass
> >>> > > manager inside KVM or VFIO, we need trigger the build at a high
> >>> > > level and it should be built before VFIO and KVM. Any ideas?
> >> >
> >> > We can add virt/Makefile and build virt/lib/ directly, not through
> >> > arch/x86/kvm.
> > Yes, that can solve the build error. Should I send a new version?
> 
> You can send a separate patch on top of this v9.

Sure, will do this soon!

Thanks,
Feng

> 
> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu, Feng Sept. 21, 2015, 12:53 p.m. UTC | #7
> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, September 21, 2015 5:32 PM
> To: Wu, Feng; alex.williamson@redhat.com; joro@8bytes.org;
> mtosatti@redhat.com
> Cc: eric.auger@linaro.org; kvm@vger.kernel.org;
> iommu@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> 
> 
> 
> On 21/09/2015 10:56, Wu, Feng wrote:
> > Hi Paolo & Alex,
> >
> > I find that there is a build error in the following two cases:
> > - KVM is configured as 'M' and VFIO as 'Y'
> > The reason is the build of irqbypass manager is triggered in
> > arch/x86/kvm/Makefile, and VFIO is built before KVM, hence
> > it cannot find the symbols in irqbypass manager.
> >
> > - Disable KVM and enable VFIO in .config
> > The reason is similar with the above one, the irqbypass manager
> > is not built since KVM is not configured.
> >
> > I think the point is that we cannot trigger the build of irqbypass
> > manager inside KVM or VFIO, we need trigger the build at a high
> > level and it should be built before VFIO and KVM. Any ideas?
> 
> We can add virt/Makefile and build virt/lib/ directly, not through
> arch/x86/kvm.

Thinking about this more, does that mean we need to add the virt directory
in the top Makefile in Linux tree?

Thanks,
Feng

> 
> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Sept. 21, 2015, 1:02 p.m. UTC | #8
On 21/09/2015 14:53, Wu, Feng wrote:
>>> > > I think the point is that we cannot trigger the build of irqbypass
>>> > > manager inside KVM or VFIO, we need trigger the build at a high
>>> > > level and it should be built before VFIO and KVM. Any ideas?
>> > 
>> > We can add virt/Makefile and build virt/lib/ directly, not through
>> > arch/x86/kvm.
> Thinking about this more, does that mean we need to add the virt directory
> in the top Makefile in Linux tree?

Yes, it does.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Auger Sept. 21, 2015, 7:46 p.m. UTC | #9
Hi,
On 09/21/2015 03:02 PM, Paolo Bonzini wrote:
> 
> 
> On 21/09/2015 14:53, Wu, Feng wrote:
>>>>>> I think the point is that we cannot trigger the build of irqbypass
>>>>>> manager inside KVM or VFIO, we need trigger the build at a high
>>>>>> level and it should be built before VFIO and KVM. Any ideas?
>>>>
>>>> We can add virt/Makefile and build virt/lib/ directly, not through
>>>> arch/x86/kvm.
>> Thinking about this more, does that mean we need to add the virt directory
>> in the top Makefile in Linux tree?
> 
> Yes, it does.
So I understand this will replace patches 2 & 3 then and will fix the
arm64 issue then.

Thanks

Eric

> 
> Paolo
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson April 26, 2016, 8:08 p.m. UTC | #10
On Fri, 18 Sep 2015 22:29:50 +0800
Feng Wu <feng.wu@intel.com> wrote:

 @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct
 vfio_pci_device *vdev,
>  		return ret;
>  	}
>  
> +	vdev->ctx[vector].producer.token = trigger;
> +	vdev->ctx[vector].producer.irq = irq;
> +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> +	if (unlikely(ret))
> +		dev_info(&pdev->dev,
> +		"irq bypass producer (token %p) registeration fails: %d\n",
> +		vdev->ctx[vector].producer.token, ret);
> +
>  	vdev->ctx[vector].trigger = trigger;
>  
>  	return 0;

Digging back into the IRQ producer/consumer thing, I'm not sure how we
should be handling a failure here, but it turns out that what we have
is pretty sub-optimal.  Any sort of testing on AMD hits this dev_info
because kvm_arch_irq_bypass_add_producer() returns -EINVAL without
kvm_x86_ops->update_pi_irte which is only implemented for vmx.  Clearly
we don't want to spew confusing error messages for a feature that does
not exist.

The easiest option is to simply make this error silent, but should
registering a producer/consumer really fail due to a mismatch on the
other end or should the __connect sequence fail silently, which both
ends would know about (if they care) due to the add/del handshake
between them?  Perhaps for now we simply need a stable suitable fix to
silence the dev_info above, but longer term, registration shouldn't
fail for mismatches like this.  Thoughts?  Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wu, Feng April 27, 2016, 1:32 a.m. UTC | #11
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, April 27, 2016 4:08 AM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: pbonzini@redhat.com; joro@8bytes.org; mtosatti@redhat.com;
> eric.auger@linaro.org; kvm@vger.kernel.org; iommu@lists.linux-
> foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> 
> On Fri, 18 Sep 2015 22:29:50 +0800
> Feng Wu <feng.wu@intel.com> wrote:
> 
>  @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct
>  vfio_pci_device *vdev,
> >  		return ret;
> >  	}
> >
> > +	vdev->ctx[vector].producer.token = trigger;
> > +	vdev->ctx[vector].producer.irq = irq;
> > +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > +	if (unlikely(ret))
> > +		dev_info(&pdev->dev,
> > +		"irq bypass producer (token %p) registeration fails: %d\n",
> > +		vdev->ctx[vector].producer.token, ret);
> > +
> >  	vdev->ctx[vector].trigger = trigger;
> >
> >  	return 0;
> 
> Digging back into the IRQ producer/consumer thing, I'm not sure how we
> should be handling a failure here, but it turns out that what we have
> is pretty sub-optimal.  Any sort of testing on AMD hits this dev_info
> because kvm_arch_irq_bypass_add_producer() returns -EINVAL without
> kvm_x86_ops->update_pi_irte which is only implemented for vmx.  Clearly
> we don't want to spew confusing error messages for a feature that does
> not exist.
> 
> The easiest option is to simply make this error silent, but should
> registering a producer/consumer really fail due to a mismatch on the
> other end or should the __connect sequence fail silently, which both
> ends would know about (if they care) due to the add/del handshake
> between them?  Perhaps for now we simply need a stable suitable fix to
> silence the dev_info above, but longer term, registration shouldn't
> fail for mismatches like this.  Thoughts?  Thanks,

Can we just return 0 when kvm_x86_ops->update_pi_irte is NULL in
kvm_arch_irq_bypass_add_producer?

Thanks,
Feng

> 
> Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Auger April 28, 2016, 3:35 p.m. UTC | #12
Hi Alex,
On 04/26/2016 10:08 PM, Alex Williamson wrote:
> On Fri, 18 Sep 2015 22:29:50 +0800
> Feng Wu <feng.wu@intel.com> wrote:
> 
>  @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct
>  vfio_pci_device *vdev,
>>  		return ret;
>>  	}
>>  
>> +	vdev->ctx[vector].producer.token = trigger;
>> +	vdev->ctx[vector].producer.irq = irq;
>> +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
>> +	if (unlikely(ret))
>> +		dev_info(&pdev->dev,
>> +		"irq bypass producer (token %p) registeration fails: %d\n",
>> +		vdev->ctx[vector].producer.token, ret);
>> +
>>  	vdev->ctx[vector].trigger = trigger;
>>  
>>  	return 0;
> 
> Digging back into the IRQ producer/consumer thing, I'm not sure how we
> should be handling a failure here, but it turns out that what we have
> is pretty sub-optimal.  Any sort of testing on AMD hits this dev_info
> because kvm_arch_irq_bypass_add_producer() returns -EINVAL without
> kvm_x86_ops->update_pi_irte which is only implemented for vmx.  Clearly
> we don't want to spew confusing error messages for a feature that does
> not exist.
> 
> The easiest option is to simply make this error silent, but should
> registering a producer/consumer really fail due to a mismatch on the
> other end or should the __connect sequence fail silently, which both
> ends would know about (if they care) due to the add/del handshake
> between them?  Perhaps for now we simply need a stable suitable fix to
> silence the dev_info above, but longer term, registration shouldn't
> fail for mismatches like this.  Thoughts?  Thanks,

Regarding the ARM IRQ forwarding use case, I think it is OK to fail
silently. We would fall back to the irqfd standard mechanism. Anyway
this series still is waiting for ARM new-vgic dependency to be resolved,
as discussed with Christoffer and Marc.

Best Regards

Eric
> 
> Alex
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson April 28, 2016, 4:40 p.m. UTC | #13
On Wed, 27 Apr 2016 01:32:32 +0000
"Wu, Feng" <feng.wu@intel.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Wednesday, April 27, 2016 4:08 AM
> > To: Wu, Feng <feng.wu@intel.com>
> > Cc: pbonzini@redhat.com; joro@8bytes.org; mtosatti@redhat.com;
> > eric.auger@linaro.org; kvm@vger.kernel.org; iommu@lists.linux-
> > foundation.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v9 12/18] vfio: Register/unregister irq_bypass_producer
> > 
> > On Fri, 18 Sep 2015 22:29:50 +0800
> > Feng Wu <feng.wu@intel.com> wrote:
> > 
> >  @@ -360,6 +361,14 @@ static int vfio_msi_set_vector_signal(struct
> >  vfio_pci_device *vdev,  
> > >  		return ret;
> > >  	}
> > >
> > > +	vdev->ctx[vector].producer.token = trigger;
> > > +	vdev->ctx[vector].producer.irq = irq;
> > > +	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > > +	if (unlikely(ret))
> > > +		dev_info(&pdev->dev,
> > > +		"irq bypass producer (token %p) registeration fails: %d\n",
> > > +		vdev->ctx[vector].producer.token, ret);
> > > +
> > >  	vdev->ctx[vector].trigger = trigger;
> > >
> > >  	return 0;  
> > 
> > Digging back into the IRQ producer/consumer thing, I'm not sure how we
> > should be handling a failure here, but it turns out that what we have
> > is pretty sub-optimal.  Any sort of testing on AMD hits this dev_info
> > because kvm_arch_irq_bypass_add_producer() returns -EINVAL without
> > kvm_x86_ops->update_pi_irte which is only implemented for vmx.  Clearly
> > we don't want to spew confusing error messages for a feature that does
> > not exist.
> > 
> > The easiest option is to simply make this error silent, but should
> > registering a producer/consumer really fail due to a mismatch on the
> > other end or should the __connect sequence fail silently, which both
> > ends would know about (if they care) due to the add/del handshake
> > between them?  Perhaps for now we simply need a stable suitable fix to
> > silence the dev_info above, but longer term, registration shouldn't
> > fail for mismatches like this.  Thoughts?  Thanks,  
> 
> Can we just return 0 when kvm_x86_ops->update_pi_irte is NULL in
> kvm_arch_irq_bypass_add_producer?

Yeah, that may be the best way to go, only return error for actual
failures, not for simple lack of a bypass mechanism.  This is
consistent with what update_pi_irte does when running on hardware
or configurations without PI.  Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig
index 579d83b..02912f1 100644
--- a/drivers/vfio/pci/Kconfig
+++ b/drivers/vfio/pci/Kconfig
@@ -2,6 +2,7 @@  config VFIO_PCI
 	tristate "VFIO support for PCI devices"
 	depends on VFIO && PCI && EVENTFD
 	select VFIO_VIRQFD
+	select IRQ_BYPASS_MANAGER
 	help
 	  Support for the PCI VFIO bus driver.  This is required to make
 	  use of PCI drivers using the VFIO framework.
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 1f577b4..c65299d 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -319,6 +319,7 @@  static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 
 	if (vdev->ctx[vector].trigger) {
 		free_irq(irq, vdev->ctx[vector].trigger);
+		irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
 		kfree(vdev->ctx[vector].name);
 		eventfd_ctx_put(vdev->ctx[vector].trigger);
 		vdev->ctx[vector].trigger = NULL;
@@ -360,6 +361,14 @@  static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 		return ret;
 	}
 
+	vdev->ctx[vector].producer.token = trigger;
+	vdev->ctx[vector].producer.irq = irq;
+	ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
+	if (unlikely(ret))
+		dev_info(&pdev->dev,
+		"irq bypass producer (token %p) registeration fails: %d\n",
+		vdev->ctx[vector].producer.token, ret);
+
 	vdev->ctx[vector].trigger = trigger;
 
 	return 0;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index ae0e1b4..0e7394f 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -13,6 +13,7 @@ 
 
 #include <linux/mutex.h>
 #include <linux/pci.h>
+#include <linux/irqbypass.h>
 
 #ifndef VFIO_PCI_PRIVATE_H
 #define VFIO_PCI_PRIVATE_H
@@ -29,6 +30,7 @@  struct vfio_pci_irq_ctx {
 	struct virqfd		*mask;
 	char			*name;
 	bool			masked;
+	struct irq_bypass_producer	producer;
 };
 
 struct vfio_pci_device {