[Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Gabriel Laupre]

Fix pba_offset initialization value for Chelsio T5 devices.  The
hardware doesn't return the correct pba_offset value, so add a
quirk to instead return a hardcoded value of 0x1000 when a Chelsio
T5 device is detected.

Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
---
 hw/vfio/pci.c            | 12 ++++++++++++
 include/hw/pci/pci_ids.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e0e339a..8a4c7cd 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
     uint16_t ctrl;
     uint32_t table, pba;
     int fd = vdev->vbasedev.fd;
+    PCIDevice *pdev = &vdev->pdev;
+    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
+    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
 
     pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
     if (!pos) {
@@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
     vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
     vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
 
+    /* Quirk to set the pba_offset value for Chelsio T5
+     * devices. Since hardware does not return value correctly,
+     * we override with a hardcoded value instead.
+     */
+    if (vendor == PCI_VENDOR_ID_CHELSIO &&
+        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
+        vdev->msix->pba_offset = 0x1000;
+    }
+
     trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
                                 vdev->msix->table_bar,
                                 vdev->msix->table_offset,
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 49c062b..9f649da 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -114,6 +114,9 @@
 #define PCI_VENDOR_ID_ENSONIQ            0x1274
 #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
 
+#define PCI_VENDOR_ID_CHELSIO            0x1425
+#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
+
 #define PCI_VENDOR_ID_FREESCALE          0x1957
 #define PCI_DEVICE_ID_MPC8533E           0x0030
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Bandan Das]

Hi Gabriel,

Glad that you got to the bottom of this! :)

Gabriel Laupre <glaupre@chelsio.com> writes:

> Fix pba_offset initialization value for Chelsio T5 devices.  The
> hardware doesn't return the correct pba_offset value, so add a
> quirk to instead return a hardcoded value of 0x1000 when a Chelsio
> T5 device is detected.
Two questions:
Is the array offset guaranteed to always be the same ?
What are the chances of this getting fixed by a firmware update ? :)

Bandan

> Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> ---
>  hw/vfio/pci.c            | 12 ++++++++++++
>  include/hw/pci/pci_ids.h |  3 +++
>  2 files changed, 15 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e0e339a..8a4c7cd 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      uint16_t ctrl;
>      uint32_t table, pba;
>      int fd = vdev->vbasedev.fd;
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
>  
>      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
>  
> +    /* Quirk to set the pba_offset value for Chelsio T5
> +     * devices. Since hardware does not return value correctly,
> +     * we override with a hardcoded value instead.
> +     */
> +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> +        vdev->msix->pba_offset = 0x1000;
> +    }
> +
>      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
>                                  vdev->msix->table_bar,
>                                  vdev->msix->table_offset,
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 49c062b..9f649da 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -114,6 +114,9 @@
>  #define PCI_VENDOR_ID_ENSONIQ            0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
>  
> +#define PCI_VENDOR_ID_CHELSIO            0x1425
> +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> +
>  #define PCI_VENDOR_ID_FREESCALE          0x1957
>  #define PCI_DEVICE_ID_MPC8533E           0x0030

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Alex Williamson]

On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> Fix pba_offset initialization value for Chelsio T5 devices.  The
> hardware doesn't return the correct pba_offset value, so add a
> quirk to instead return a hardcoded value of 0x1000 when a Chelsio
> T5 device is detected.
> 
> Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> ---
>  hw/vfio/pci.c            | 12 ++++++++++++
>  include/hw/pci/pci_ids.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e0e339a..8a4c7cd 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      uint16_t ctrl;
>      uint32_t table, pba;
>      int fd = vdev->vbasedev.fd;
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
>  
>      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
>  
> +    /* Quirk to set the pba_offset value for Chelsio T5
> +     * devices. Since hardware does not return value correctly,
> +     * we override with a hardcoded value instead.
> +     */
> +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> +        vdev->msix->pba_offset = 0x1000;
> +    }

Wow, so we're writing off a whole 1/16th of the Chelsio device ID space
as broken?  Can we detect that the pba_offset is wrong?  Is it a
consistent and obviously incorrect value?  Thanks,

Alex

> +
>      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
>                                  vdev->msix->table_bar,
>                                  vdev->msix->table_offset,
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 49c062b..9f649da 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -114,6 +114,9 @@
>  #define PCI_VENDOR_ID_ENSONIQ            0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
>  
> +#define PCI_VENDOR_ID_CHELSIO            0x1425
> +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> +
>  #define PCI_VENDOR_ID_FREESCALE          0x1957
>  #define PCI_DEVICE_ID_MPC8533E           0x0030
>  

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Gabriel Laupre]

@Bandan
> Is the array offset guaranteed to always be the same ?
The returned value depends on the physical function and should be 0x1000 for the T5 series. Therefore this offset is guaranteed to always be the same.

> What are the chances of this getting fixed by a firmware update ? :)
It isn't a firmware issue, therefore can't be fixed via firmware update. This will be resolved for the next versions. 

@Alex
> Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?
The pba_offset returned will always be 0x8000 for a virtual function for a T5 device. It is obviously too big. It is an hardware problem in the T5 series. 

Also I will need to correct the patch as the virtual function devices are encoded as 0x58xx . This quirk is only related with the virtual functions.

Gabriel

-----Original Message-----
From: Alex Williamson [mailto:alex.williamson@redhat.com] 
Sent: Thursday, June 25, 2015 7:08 AM
To: Gabriel Laupre
Cc: qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; bsd@makefile.in; Casey Leedom; Michael Boksanyi; Anish Bhatt
Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> Fix pba_offset initialization value for Chelsio T5 devices.  The 
> hardware doesn't return the correct pba_offset value, so add a quirk 
> to instead return a hardcoded value of 0x1000 when a Chelsio
> T5 device is detected.
> 
> Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> ---
>  hw/vfio/pci.c            | 12 ++++++++++++
>  include/hw/pci/pci_ids.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e0e339a..8a4c7cd 
> 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      uint16_t ctrl;
>      uint32_t table, pba;
>      int fd = vdev->vbasedev.fd;
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
>  
>      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
>      if (!pos) {
> @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
>      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
>      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
>  
> +    /* Quirk to set the pba_offset value for Chelsio T5
> +     * devices. Since hardware does not return value correctly,
> +     * we override with a hardcoded value instead.
> +     */
> +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> +        vdev->msix->pba_offset = 0x1000;
> +    }

Wow, so we're writing off a whole 1/16th of the Chelsio device ID space as broken?  Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?  Thanks,

Alex

> +
>      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
>                                  vdev->msix->table_bar,
>                                  vdev->msix->table_offset, diff --git 
> a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index 
> 49c062b..9f649da 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -114,6 +114,9 @@
>  #define PCI_VENDOR_ID_ENSONIQ            0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
>  
> +#define PCI_VENDOR_ID_CHELSIO            0x1425
> +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> +
>  #define PCI_VENDOR_ID_FREESCALE          0x1957
>  #define PCI_DEVICE_ID_MPC8533E           0x0030
>  

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Alex Williamson]

On Thu, 2015-06-25 at 19:00 +0000, Gabriel Laupre wrote:
> @Bandan
> > Is the array offset guaranteed to always be the same ?
> The returned value depends on the physical function and should be 0x1000 for the T5 series. Therefore this offset is guaranteed to always be the same.
> 
> > What are the chances of this getting fixed by a firmware update ? :)
> It isn't a firmware issue, therefore can't be fixed via firmware update. This will be resolved for the next versions. 
> 
> @Alex
> > Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?
> The pba_offset returned will always be 0x8000 for a virtual function for a T5 device. It is obviously too big. It is an hardware problem in the T5 series. 


Is the current failure mode that msix_init() fails because pba_offset
(or pba_offset + pba_size) extends outside of the specified BAR?  If the
problem is going to be resolved in the next version, will that be
different PCI device IDs?  Ideally we'd only enable the quirk if we knew
we were in a bogus situation, so we can let new hardware work as it
describes itself if it gets fixed.  Thanks,

Alex

> Also I will need to correct the patch as the virtual function devices are encoded as 0x58xx . This quirk is only related with the virtual functions.
> 
> Gabriel
> 
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com] 
> Sent: Thursday, June 25, 2015 7:08 AM
> To: Gabriel Laupre
> Cc: qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; bsd@makefile.in; Casey Leedom; Michael Boksanyi; Anish Bhatt
> Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
> 
> On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> > Fix pba_offset initialization value for Chelsio T5 devices.  The 
> > hardware doesn't return the correct pba_offset value, so add a quirk 
> > to instead return a hardcoded value of 0x1000 when a Chelsio
> > T5 device is detected.
> > 
> > Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> > ---
> >  hw/vfio/pci.c            | 12 ++++++++++++
> >  include/hw/pci/pci_ids.h |  3 +++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e0e339a..8a4c7cd 
> > 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      uint16_t ctrl;
> >      uint32_t table, pba;
> >      int fd = vdev->vbasedev.fd;
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> > +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> >  
> >      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> >      if (!pos) {
> > @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> >      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> >  
> > +    /* Quirk to set the pba_offset value for Chelsio T5
> > +     * devices. Since hardware does not return value correctly,
> > +     * we override with a hardcoded value instead.
> > +     */
> > +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> > +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> > +        vdev->msix->pba_offset = 0x1000;
> > +    }
> 
> Wow, so we're writing off a whole 1/16th of the Chelsio device ID space as broken?  Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?  Thanks,
> 
> Alex
> 
> > +
> >      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
> >                                  vdev->msix->table_bar,
> >                                  vdev->msix->table_offset, diff --git 
> > a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index 
> > 49c062b..9f649da 100644
> > --- a/include/hw/pci/pci_ids.h
> > +++ b/include/hw/pci/pci_ids.h
> > @@ -114,6 +114,9 @@
> >  #define PCI_VENDOR_ID_ENSONIQ            0x1274
> >  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
> >  
> > +#define PCI_VENDOR_ID_CHELSIO            0x1425
> > +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> > +
> >  #define PCI_VENDOR_ID_FREESCALE          0x1957
> >  #define PCI_DEVICE_ID_MPC8533E           0x0030
> >  
> 
> 
> 

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Casey Leedom]

  Hi Alex, the issue is that the T5 hardware has a bug in it where it reports a Pending Interrupt Bit Array Offset of 0x8000 for its SR-IOV Virtual Functions instead of the 0x1000 that the hardware actually uses internally.  (There was a mistaken <<3 used in the IP Glue Logic for the PCI Configuration Space of the VFs.)

  This bug has been fixed in our next chip — unimaginatively enough called T6 — but there are no plans to respin the T5 ASIC for this bug.  We've just documented it in our T5 Errata and left it at that.

  Prior to this issue that Gabriel is tackling, the mis-reported PBA Offset wasn't a problem because no Host Software that we were aware of ever used that value.  Apparently the software that Gabriel is using does look at the PBA Value and complains that the 0x8000 is outside the BAR4/5 MSI-X range, which is itself only 8KB in length.  (Although it's still not clear to me that the software in question ever bothers doing anything with the PBA Offset.)

  So I suggested to Gabriel that he follow the Linux kernel model and provide a Device Specific "Quirk" which would detect a T5 VF and hard wire the PBA Offset to 0x1000 for such devices.

  Please let me know if you'd like any more background information.

Casey

________________________________________
From: Alex Williamson [alex.williamson@redhat.com]
Sent: Thursday, June 25, 2015 12:19 PM
To: Gabriel Laupre
Cc: bsd@makefile.in; qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; Casey Leedom; Michael Boksanyi; Anish Bhatt
Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

On Thu, 2015-06-25 at 19:00 +0000, Gabriel Laupre wrote:
> @Bandan
> > Is the array offset guaranteed to always be the same ?
> The returned value depends on the physical function and should be 0x1000 for the T5 series. Therefore this offset is guaranteed to always be the same.
>
> > What are the chances of this getting fixed by a firmware update ? :)
> It isn't a firmware issue, therefore can't be fixed via firmware update. This will be resolved for the next versions.
>
> @Alex
> > Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?
> The pba_offset returned will always be 0x8000 for a virtual function for a T5 device. It is obviously too big. It is an hardware problem in the T5 series.


Is the current failure mode that msix_init() fails because pba_offset
(or pba_offset + pba_size) extends outside of the specified BAR?  If the
problem is going to be resolved in the next version, will that be
different PCI device IDs?  Ideally we'd only enable the quirk if we knew
we were in a bogus situation, so we can let new hardware work as it
describes itself if it gets fixed.  Thanks,

Alex

> Also I will need to correct the patch as the virtual function devices are encoded as 0x58xx . This quirk is only related with the virtual functions.
>
> Gabriel
>
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, June 25, 2015 7:08 AM
> To: Gabriel Laupre
> Cc: qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; bsd@makefile.in; Casey Leedom; Michael Boksanyi; Anish Bhatt
> Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
>
> On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> > Fix pba_offset initialization value for Chelsio T5 devices.  The
> > hardware doesn't return the correct pba_offset value, so add a quirk
> > to instead return a hardcoded value of 0x1000 when a Chelsio
> > T5 device is detected.
> >
> > Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> > ---
> >  hw/vfio/pci.c            | 12 ++++++++++++
> >  include/hw/pci/pci_ids.h |  3 +++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e0e339a..8a4c7cd
> > 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      uint16_t ctrl;
> >      uint32_t table, pba;
> >      int fd = vdev->vbasedev.fd;
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> > +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> >
> >      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> >      if (!pos) {
> > @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> >      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> >
> > +    /* Quirk to set the pba_offset value for Chelsio T5
> > +     * devices. Since hardware does not return value correctly,
> > +     * we override with a hardcoded value instead.
> > +     */
> > +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> > +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> > +        vdev->msix->pba_offset = 0x1000;
> > +    }
>
> Wow, so we're writing off a whole 1/16th of the Chelsio device ID space as broken?  Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?  Thanks,
>
> Alex
>
> > +
> >      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
> >                                  vdev->msix->table_bar,
> >                                  vdev->msix->table_offset, diff --git
> > a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index
> > 49c062b..9f649da 100644
> > --- a/include/hw/pci/pci_ids.h
> > +++ b/include/hw/pci/pci_ids.h
> > @@ -114,6 +114,9 @@
> >  #define PCI_VENDOR_ID_ENSONIQ            0x1274
> >  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
> >
> > +#define PCI_VENDOR_ID_CHELSIO            0x1425
> > +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> > +
> >  #define PCI_VENDOR_ID_FREESCALE          0x1957
> >  #define PCI_DEVICE_ID_MPC8533E           0x0030
> >
>
>
>

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Casey Leedom]

  Oh, and by the way, I've already asked Gabriel to respin the patch because the quirk incorrectly trips for all T5 Functions instead of only for T5 Virtual Functions.  So you should reject the first patch regardless.  Thanks!

Casey

________________________________________
From: Casey Leedom
Sent: Thursday, June 25, 2015 1:21 PM
To: Alex Williamson; Gabriel Laupre
Cc: bsd@makefile.in; qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; Michael Boksanyi; Anish Bhatt
Subject: RE: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

  Hi Alex, the issue is that the T5 hardware has a bug in it where it reports a Pending Interrupt Bit Array Offset of 0x8000 for its SR-IOV Virtual Functions instead of the 0x1000 that the hardware actually uses internally.  (There was a mistaken <<3 used in the IP Glue Logic for the PCI Configuration Space of the VFs.)

  This bug has been fixed in our next chip — unimaginatively enough called T6 — but there are no plans to respin the T5 ASIC for this bug.  We've just documented it in our T5 Errata and left it at that.

  Prior to this issue that Gabriel is tackling, the mis-reported PBA Offset wasn't a problem because no Host Software that we were aware of ever used that value.  Apparently the software that Gabriel is using does look at the PBA Value and complains that the 0x8000 is outside the BAR4/5 MSI-X range, which is itself only 8KB in length.  (Although it's still not clear to me that the software in question ever bothers doing anything with the PBA Offset.)

  So I suggested to Gabriel that he follow the Linux kernel model and provide a Device Specific "Quirk" which would detect a T5 VF and hard wire the PBA Offset to 0x1000 for such devices.

  Please let me know if you'd like any more background information.

Casey

________________________________________
From: Alex Williamson [alex.williamson@redhat.com]
Sent: Thursday, June 25, 2015 12:19 PM
To: Gabriel Laupre
Cc: bsd@makefile.in; qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; Casey Leedom; Michael Boksanyi; Anish Bhatt
Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

On Thu, 2015-06-25 at 19:00 +0000, Gabriel Laupre wrote:
> @Bandan
> > Is the array offset guaranteed to always be the same ?
> The returned value depends on the physical function and should be 0x1000 for the T5 series. Therefore this offset is guaranteed to always be the same.
>
> > What are the chances of this getting fixed by a firmware update ? :)
> It isn't a firmware issue, therefore can't be fixed via firmware update. This will be resolved for the next versions.
>
> @Alex
> > Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?
> The pba_offset returned will always be 0x8000 for a virtual function for a T5 device. It is obviously too big. It is an hardware problem in the T5 series.


Is the current failure mode that msix_init() fails because pba_offset
(or pba_offset + pba_size) extends outside of the specified BAR?  If the
problem is going to be resolved in the next version, will that be
different PCI device IDs?  Ideally we'd only enable the quirk if we knew
we were in a bogus situation, so we can let new hardware work as it
describes itself if it gets fixed.  Thanks,

Alex

> Also I will need to correct the patch as the virtual function devices are encoded as 0x58xx . This quirk is only related with the virtual functions.
>
> Gabriel
>
> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, June 25, 2015 7:08 AM
> To: Gabriel Laupre
> Cc: qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; bsd@makefile.in; Casey Leedom; Michael Boksanyi; Anish Bhatt
> Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
>
> On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> > Fix pba_offset initialization value for Chelsio T5 devices.  The
> > hardware doesn't return the correct pba_offset value, so add a quirk
> > to instead return a hardcoded value of 0x1000 when a Chelsio
> > T5 device is detected.
> >
> > Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> > ---
> >  hw/vfio/pci.c            | 12 ++++++++++++
> >  include/hw/pci/pci_ids.h |  3 +++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e0e339a..8a4c7cd
> > 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      uint16_t ctrl;
> >      uint32_t table, pba;
> >      int fd = vdev->vbasedev.fd;
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> > +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> >
> >      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> >      if (!pos) {
> > @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> >      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> >      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> >
> > +    /* Quirk to set the pba_offset value for Chelsio T5
> > +     * devices. Since hardware does not return value correctly,
> > +     * we override with a hardcoded value instead.
> > +     */
> > +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> > +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> > +        vdev->msix->pba_offset = 0x1000;
> > +    }
>
> Wow, so we're writing off a whole 1/16th of the Chelsio device ID space as broken?  Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?  Thanks,
>
> Alex
>
> > +
> >      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
> >                                  vdev->msix->table_bar,
> >                                  vdev->msix->table_offset, diff --git
> > a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index
> > 49c062b..9f649da 100644
> > --- a/include/hw/pci/pci_ids.h
> > +++ b/include/hw/pci/pci_ids.h
> > @@ -114,6 +114,9 @@
> >  #define PCI_VENDOR_ID_ENSONIQ            0x1274
> >  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
> >
> > +#define PCI_VENDOR_ID_CHELSIO            0x1425
> > +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> > +
> >  #define PCI_VENDOR_ID_FREESCALE          0x1957
> >  #define PCI_DEVICE_ID_MPC8533E           0x0030
> >
>
>
>

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Alex Williamson]

On Thu, 2015-06-25 at 20:22 +0000, Casey Leedom wrote:
>   Oh, and by the way, I've already asked Gabriel to respin the patch
> because the quirk incorrectly trips for all T5 Functions instead of
> only for T5 Virtual Functions.  So you should reject the first patch
> regardless.  Thanks!
> 
> Casey
> 
> ________________________________________
> From: Casey Leedom
> Sent: Thursday, June 25, 2015 1:21 PM
> To: Alex Williamson; Gabriel Laupre
> Cc: bsd@makefile.in; qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; Michael Boksanyi; Anish Bhatt
> Subject: RE: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
> 
>   Hi Alex, the issue is that the T5 hardware has a bug in it where it
> reports a Pending Interrupt Bit Array Offset of 0x8000 for its SR-IOV
> Virtual Functions instead of the 0x1000 that the hardware actually
> uses internally.  (There was a mistaken <<3 used in the IP Glue Logic
> for the PCI Configuration Space of the VFs.)
> 
>   This bug has been fixed in our next chip — unimaginatively enough
> called T6 — but there are no plans to respin the T5 ASIC for this bug.
> We've just documented it in our T5 Errata and left it at that.
> 
>   Prior to this issue that Gabriel is tackling, the mis-reported PBA
> Offset wasn't a problem because no Host Software that we were aware of
> ever used that value.  Apparently the software that Gabriel is using
> does look at the PBA Value and complains that the 0x8000 is outside
> the BAR4/5 MSI-X range, which is itself only 8KB in length.  (Although
> it's still not clear to me that the software in question ever bothers
> doing anything with the PBA Offset.)

We expose it to the guest VM, but the guest probably doesn't consume it,
I don't know of any drivers that do.

It would be really helpful if these kinds of details were in the commit
log, specifically the value we expect to see for pba_offset when it's
wrong, and in this case that it's obviously wrong because it's beyond
the end of the BAR.  Knowing that it's limited to VFs is also helpful if
someone needs to go back and rework the quirk later.

And, because it's obviously wrong, we can further limit the scope of the
quirk by testing those parameters.  That would have automatically
skipped the quirk if we had used the wrong device ID match or when
Chelsio runs out of device IDs and starts using gaps left in the device
ID address space (I only see ~34 device IDs in the 58xx space used so
far according to pci-ids).  Thanks,

Alex

>   So I suggested to Gabriel that he follow the Linux kernel model and
> provide a Device Specific "Quirk" which would detect a T5 VF and hard
> wire the PBA Offset to 0x1000 for such devices.
> 
>   Please let me know if you'd like any more background information.
> 
> Casey
> 
> ________________________________________
> From: Alex Williamson [alex.williamson@redhat.com]
> Sent: Thursday, June 25, 2015 12:19 PM
> To: Gabriel Laupre
> Cc: bsd@makefile.in; qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; Casey Leedom; Michael Boksanyi; Anish Bhatt
> Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
> 
> On Thu, 2015-06-25 at 19:00 +0000, Gabriel Laupre wrote:
> > @Bandan
> > > Is the array offset guaranteed to always be the same ?
> > The returned value depends on the physical function and should be 0x1000 for the T5 series. Therefore this offset is guaranteed to always be the same.
> >
> > > What are the chances of this getting fixed by a firmware update ? :)
> > It isn't a firmware issue, therefore can't be fixed via firmware update. This will be resolved for the next versions.
> >
> > @Alex
> > > Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?
> > The pba_offset returned will always be 0x8000 for a virtual function for a T5 device. It is obviously too big. It is an hardware problem in the T5 series.
> 
> 
> Is the current failure mode that msix_init() fails because pba_offset
> (or pba_offset + pba_size) extends outside of the specified BAR?  If the
> problem is going to be resolved in the next version, will that be
> different PCI device IDs?  Ideally we'd only enable the quirk if we knew
> we were in a bogus situation, so we can let new hardware work as it
> describes itself if it gets fixed.  Thanks,
> 
> Alex
> 
> > Also I will need to correct the patch as the virtual function devices are encoded as 0x58xx . This quirk is only related with the virtual functions.
> >
> > Gabriel
> >
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, June 25, 2015 7:08 AM
> > To: Gabriel Laupre
> > Cc: qemu-devel@nongnu.org; mst@redhat.com; jb-gnumlists@wisemo.com; bsd@makefile.in; Casey Leedom; Michael Boksanyi; Anish Bhatt
> > Subject: Re: [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices
> >
> > On Wed, 2015-06-24 at 19:04 -0700, Gabriel Laupre wrote:
> > > Fix pba_offset initialization value for Chelsio T5 devices.  The
> > > hardware doesn't return the correct pba_offset value, so add a quirk
> > > to instead return a hardcoded value of 0x1000 when a Chelsio
> > > T5 device is detected.
> > >
> > > Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> > > ---
> > >  hw/vfio/pci.c            | 12 ++++++++++++
> > >  include/hw/pci/pci_ids.h |  3 +++
> > >  2 files changed, 15 insertions(+)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e0e339a..8a4c7cd
> > > 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2220,6 +2220,9 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> > >      uint16_t ctrl;
> > >      uint32_t table, pba;
> > >      int fd = vdev->vbasedev.fd;
> > > +    PCIDevice *pdev = &vdev->pdev;
> > > +    uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> > > +    uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> > >
> > >      pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
> > >      if (!pos) {
> > > @@ -2252,6 +2255,15 @@ static int vfio_early_setup_msix(VFIOPCIDevice *vdev)
> > >      vdev->msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
> > >      vdev->msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> > >
> > > +    /* Quirk to set the pba_offset value for Chelsio T5
> > > +     * devices. Since hardware does not return value correctly,
> > > +     * we override with a hardcoded value instead.
> > > +     */
> > > +    if (vendor == PCI_VENDOR_ID_CHELSIO &&
> > > +        (device & 0xf000) == PCI_DEVICE_ID_CHELSIO_T5_SERIES) {
> > > +        vdev->msix->pba_offset = 0x1000;
> > > +    }
> >
> > Wow, so we're writing off a whole 1/16th of the Chelsio device ID space as broken?  Can we detect that the pba_offset is wrong?  Is it a consistent and obviously incorrect value?  Thanks,
> >
> > Alex
> >
> > > +
> > >      trace_vfio_early_setup_msix(vdev->vbasedev.name, pos,
> > >                                  vdev->msix->table_bar,
> > >                                  vdev->msix->table_offset, diff --git
> > > a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h index
> > > 49c062b..9f649da 100644
> > > --- a/include/hw/pci/pci_ids.h
> > > +++ b/include/hw/pci/pci_ids.h
> > > @@ -114,6 +114,9 @@
> > >  #define PCI_VENDOR_ID_ENSONIQ            0x1274
> > >  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
> > >
> > > +#define PCI_VENDOR_ID_CHELSIO            0x1425
> > > +#define PCI_DEVICE_ID_CHELSIO_T5_SERIES  0x5000
> > > +
> > >  #define PCI_VENDOR_ID_FREESCALE          0x1957
> > >  #define PCI_DEVICE_ID_MPC8533E           0x0030
> > >
> >
> >
> >
> 
> 
> 

Re: [Qemu-devel] [PATCH] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Casey Leedom]

| From: Alex Williamson [alex.williamson@redhat.com]
| Sent: Thursday, June 25, 2015 1:57 PM
| 
| We expose [the PBA Offset] to the guest VM, but the guest probably
| doesn't consume it, I don't know of any drivers that do.

  Ah, okay.  You're doing this for the Hypervisor traps of accesses
to the device's Configuration Space.  Makes sense.

| It would be really helpful if these kinds of details were in the commit
| log, specifically the value we expect to see for pba_offset when it's
| wrong, and in this case that it's obviously wrong because it's beyond
| the end of the BAR.  Knowing that it's limited to VFs is also helpful if
| someone needs to go back and rework the quirk later.

  Yeah, sorry about that.  Gabriel sent it around for internal review
but I was overwhelmed with other things and didn't get a chance to
make revision comments.  Gabriel's new to the process and assumed
when someone else said that "it's sort of looks like what you'd need to do ..."
that that was ACK enough ... :-)  I certainly would have noted that he was
applying the quirk to all the T5 PCI-E Functions instead of only the
SR-IOV Virtual Functions.

| And, because it's obviously wrong, we can further limit the scope of the
| quirk by testing those parameters.  That would have automatically
| skipped the quirk if we had used the wrong device ID match or when
| Chelsio runs out of device IDs and starts using gaps left in the device
| ID address space (I only see ~34 device IDs in the 58xx space used so
| far according to pci-ids).  Thanks,

  Sure, if you like.  At our current rate we would end up reusing these
PCI Device IDs in about 50 years though.

Casey