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:address@hidden 
Sent: Thursday, June 25, 2015 7:08 AM
To: Gabriel Laupre
Cc: address@hidden; address@hidden; address@hidden; address@hidden; 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 <address@hidden>
> ---
>  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
>