[PATCH v2 0/4] VFIO Misc fixes

[PATCH v2 0/4] VFIO Misc fixes

[Gavin Shan]

Changelog
=========
v1 -> v2:
	* Change the comments and commit log in PATCH[4/4] (Alex).
	* Export 2 MSI relevant functions (Alex).

Gavin Shan (4):
  PCI: Export MSI message relevant functions
  drivers/vfio: Rework offsetofend()
  drivers/vfio/pci: Fix wrong MSI interrupt count
  vfio/pci: Restore MSIx message prior to enabling

 drivers/pci/msi.c                 |  2 ++
 drivers/vfio/pci/vfio_pci.c       |  3 +--
 drivers/vfio/pci/vfio_pci_intrs.c | 14 ++++++++++++++
 include/linux/vfio.h              |  5 ++---
 4 files changed, 19 insertions(+), 5 deletions(-)

Thanks,
Gavin

[PATCH 1/4] PCI: Export MSI message relevant functions

[Gavin Shan]

The patch exports 2 MSI message relevant functions, which will be
used by VFIO PCI driver. The VFIO PCI driver would be built as
a module.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 955ab79..2350271 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -324,6 +324,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
 
 	__get_cached_msi_msg(entry, msg);
 }
+EXPORT_SYMBOL_GPL(get_cached_msi_msg);
 
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
@@ -368,6 +369,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
 
 	__write_msi_msg(entry, msg);
 }
+EXPORT_SYMBOL_GPL(write_msi_msg);
 
 static void free_msi_irqs(struct pci_dev *dev)
 {
-- 
1.8.3.2

[PATCH 2/4] drivers/vfio: Rework offsetofend()

[Gavin Shan]

The macro offsetofend() introduces unnecessary temporary variable
"tmp". The patch avoids that and saves a bit memory in stack.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/linux/vfio.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 81022a52..8ec980b 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -86,9 +86,8 @@ extern void vfio_unregister_iommu_driver(
  * from user space.  This allows us to easily determine if the provided
  * structure is sized to include various fields.
  */
-#define offsetofend(TYPE, MEMBER) ({				\
-	TYPE tmp;						\
-	offsetof(TYPE, MEMBER) + sizeof(tmp.MEMBER); })		\
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER)	+ sizeof(((TYPE *)0)->MEMBER))
 
 /*
  * External user API
-- 
1.8.3.2

[PATCH 3/4] drivers/vfio/pci: Fix wrong MSI interrupt count

[Gavin Shan]

According PCI local bus specification, the register of Message
Control for MSI (offset: 2, length: 2) has bit#0 to enable or
disable MSI logic and it shouldn't be part contributing to the
calculation of MSI interrupt count. The patch fixes the issue.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/vfio/pci/vfio_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 7ba0424..6b8cd07 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -196,8 +196,7 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
 		if (pos) {
 			pci_read_config_word(vdev->pdev,
 					     pos + PCI_MSI_FLAGS, &flags);
-
-			return 1 << (flags & PCI_MSI_FLAGS_QMASK);
+			return 1 << ((flags & PCI_MSI_FLAGS_QMASK) >> 1);
 		}
 	} else if (irq_type == VFIO_PCI_MSIX_IRQ_INDEX) {
 		u8 pos;
-- 
1.8.3.2

[PATCH 4/4] vfio/pci: Restore MSIx message prior to enabling

[Gavin Shan]

The MSIx vector table lives in device memory, which may be cleared as
part of a backdoor device reset. This is the case on the IBM IPR HBA
when the BIST is run on the device. When assigned to a QEMU guest,
the guest driver does a pci_save_state(), issues a BIST, then does a
pci_restore_state(). The BIST clears the MSIx vector table, but due
to the way interrupts are configured the pci_restore_state() does not
restore the vector table as expected. Eventually this results in an
EEH error on Power platforms when the device attempts to signal an
interrupt with the zero'd table entry.

Fix the problem by restoring the host cached MSI message prior to
enabling each vector.

Reported-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 9dd49c9..409346f 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -548,6 +548,20 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
 		return PTR_ERR(trigger);
 	}
 
+	/*
+	 * The MSIx vector table resides in device memory which may be cleared
+	 * via backdoor resets. We don't allow direct access to the vector
+	 * table so even if a userspace driver attempts to save/restore around
+	 * such a reset it would be unsuccessful. To avoid this, restore the
+	 * cached value of the message prior to enabling.
+	 */
+	if (msix) {
+		struct msi_msg msg;
+
+		get_cached_msi_msg(irq, &msg);
+		write_msi_msg(irq, &msg);
+	}
+
 	ret = request_irq(irq, vfio_msihandler, 0,
 			  vdev->ctx[vector].name, trigger);
 	if (ret) {
-- 
1.8.3.2

Re: [PATCH 4/4] vfio/pci: Restore MSIx message prior to enabling

[Gavin Shan]

On Tue, May 13, 2014 at 11:35:24AM +1000, Gavin Shan wrote:
>The MSIx vector table lives in device memory, which may be cleared as
>part of a backdoor device reset. This is the case on the IBM IPR HBA
>when the BIST is run on the device. When assigned to a QEMU guest,
>the guest driver does a pci_save_state(), issues a BIST, then does a
>pci_restore_state(). The BIST clears the MSIx vector table, but due
>to the way interrupts are configured the pci_restore_state() does not
>restore the vector table as expected. Eventually this results in an
>EEH error on Power platforms when the device attempts to signal an
>interrupt with the zero'd table entry.
>
>Fix the problem by restoring the host cached MSI message prior to
>enabling each vector.
>
>Reported-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>---

Sorry about that the following line was missed. I'll add that and send
"v3" out.

#include <linux/msi.h>

> drivers/vfio/pci/vfio_pci_intrs.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
>index 9dd49c9..409346f 100644
>--- a/drivers/vfio/pci/vfio_pci_intrs.c
>+++ b/drivers/vfio/pci/vfio_pci_intrs.c
>@@ -548,6 +548,20 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> 		return PTR_ERR(trigger);
> 	}
>
>+	/*
>+	 * The MSIx vector table resides in device memory which may be cleared
>+	 * via backdoor resets. We don't allow direct access to the vector
>+	 * table so even if a userspace driver attempts to save/restore around
>+	 * such a reset it would be unsuccessful. To avoid this, restore the
>+	 * cached value of the message prior to enabling.
>+	 */
>+	if (msix) {
>+		struct msi_msg msg;
>+
>+		get_cached_msi_msg(irq, &msg);
>+		write_msi_msg(irq, &msg);
>+	}
>+
> 	ret = request_irq(irq, vfio_msihandler, 0,
> 			  vdev->ctx[vector].name, trigger);
> 	if (ret) {

Thanks,
Gavin

[PATCH 1/4] PCI: Export MSI message relevant functions

[Gavin Shan]

The patch exports 2 MSI message relevant functions, which will be
used by VFIO PCI driver. The VFIO PCI driver would be built as
a module.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/msi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 955ab79..2350271 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -324,6 +324,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
 
 	__get_cached_msi_msg(entry, msg);
 }
+EXPORT_SYMBOL_GPL(get_cached_msi_msg);
 
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
@@ -368,6 +369,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
 
 	__write_msi_msg(entry, msg);
 }
+EXPORT_SYMBOL_GPL(write_msi_msg);
 
 static void free_msi_irqs(struct pci_dev *dev)
 {
-- 
1.8.3.2

Re: [PATCH 1/4] PCI: Export MSI message relevant functions

[Gavin Shan]

On Mon, May 19, 2014 at 01:01:07PM +1000, Gavin Shan wrote:
>The patch exports 2 MSI message relevant functions, which will be
>used by VFIO PCI driver. The VFIO PCI driver would be built as
>a module.
>
>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Bjorn, could you help ack it if you don't have objection to it?
I guess Alex is probably waiting to merge the subsequent patch,
which depends on this one.

>---
> drivers/pci/msi.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>index 955ab79..2350271 100644
>--- a/drivers/pci/msi.c
>+++ b/drivers/pci/msi.c
>@@ -324,6 +324,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>
> 	__get_cached_msi_msg(entry, msg);
> }
>+EXPORT_SYMBOL_GPL(get_cached_msi_msg);
>
> void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
> {
>@@ -368,6 +369,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>
> 	__write_msi_msg(entry, msg);
> }
>+EXPORT_SYMBOL_GPL(write_msi_msg);
>
> static void free_msi_irqs(struct pci_dev *dev)
> {

Thanks,
Gavin

Re: [PATCH 1/4] PCI: Export MSI message relevant functions

[Bjorn Helgaas]

On Mon, May 19, 2014 at 01:01:07PM +1000, Gavin Shan wrote:
> The patch exports 2 MSI message relevant functions, which will be
> used by VFIO PCI driver. The VFIO PCI driver would be built as
> a module.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I think Alex will merge this along with the other ones.  Sorry this
took so long.  I don't really like this, but I just can't figure out
any solution that's better.

> ---
>  drivers/pci/msi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 955ab79..2350271 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -324,6 +324,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>  
>  	__get_cached_msi_msg(entry, msg);
>  }
> +EXPORT_SYMBOL_GPL(get_cached_msi_msg);
>  
>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
> @@ -368,6 +369,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>  
>  	__write_msi_msg(entry, msg);
>  }
> +EXPORT_SYMBOL_GPL(write_msi_msg);
>  
>  static void free_msi_irqs(struct pci_dev *dev)
>  {
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/4] PCI: Export MSI message relevant functions

[Gavin Shan]

On Thu, Sep 04, 2014 at 04:57:36PM -0600, Bjorn Helgaas wrote:
>On Mon, May 19, 2014 at 01:01:07PM +1000, Gavin Shan wrote:
>> The patch exports 2 MSI message relevant functions, which will be
>> used by VFIO PCI driver. The VFIO PCI driver would be built as
>> a module.
>> 
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
>I think Alex will merge this along with the other ones.  Sorry this
>took so long.  I don't really like this, but I just can't figure out
>any solution that's better.
>

Thanks, Bjorn. I thought you must forget this. Lets get it in firstly
and I'll do more investigation later to see if I can figure out something
better.

Thanks,
Gavin

>> ---
>>  drivers/pci/msi.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 955ab79..2350271 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -324,6 +324,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>>  
>>  	__get_cached_msi_msg(entry, msg);
>>  }
>> +EXPORT_SYMBOL_GPL(get_cached_msi_msg);
>>  
>>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>>  {
>> @@ -368,6 +369,7 @@ void write_msi_msg(unsigned int irq, struct msi_msg *msg)
>>  
>>  	__write_msi_msg(entry, msg);
>>  }
>> +EXPORT_SYMBOL_GPL(write_msi_msg);
>>  
>>  static void free_msi_irqs(struct pci_dev *dev)
>>  {
>> -- 
>> 1.8.3.2
>> 
>