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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2015-06-30 at 14:03 -0700, Gabriel Laupre wrote:
> Fix pba_offset initialization value for Chelsio T5 Virtual Function
> device. 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. As the hardware
> doesn't return the correct pba_offset value, add a quirk to instead
> return a hardcoded value of 0x1000 when a Chelsio T5 VF device is
> detected.
> 
> This bug has been fixed in the Chelsio's next chip series T6 but there are
> no plans to respin the T5 ASIC for this bug. It is just documented in the
> T5 Errata and left it at that.
> 
> v3: Test the correctness of MSIX data compare to the specified BAR and apply a
>   quirk if it comes from a Chelsio T5 Virtual Function, otherwise raise a
>   config error.
> 
> v2: Replace and PCI_DEVICE_ID_CHELSIO_T5_SERIES_VF macro definition with
>   the Chelsio's T5 VF devices identifier schema of 0x58xx
> 
> Signed-off-by: Gabriel Laupre <glaupre@chelsio.com>
> ---
>  hw/vfio/pci.c            | 25 +++++++++++++++++++++++++
>  include/hw/pci/pci_ids.h |  2 ++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e0e339a..797fedb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2252,6 +2252,31 @@ 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;
>  
> +    /* Test the size of the pba variables and catch if they extend outside of
> +     * the specified BAR. If it is the case, we have a broken configuration or
> +     * we need to apply a hardware specific quirk. */

Please don't introduce new comment styles, this file is very consistent
in using the following format for multi-line comments:

/*
 * Line 1
 * Line 2
 */

> +    if (vdev->msix->table_offset >=
> +        vdev->bars[vdev->msix->table_bar].region.size ||
> +        vdev->msix->pba_offset >=
> +        vdev->bars[vdev->msix->pba_bar].region.size) {

We're testing both the vector table and PBA offsets relative to the BAR
size, so the comment is slightly off in mentioning only the PBA.

> +
> +        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);
> +
> +        /* Chelsio T5 Virtual Function devices are encoded as 0x58xx for T5
> +         * adapters. The T5 hardware returns an incorrect value of 0x8000 for
> +         * the VF PBA offset. The correct value is 0x1000, so we hard code that
> +         * here. */

Comment style...

Notice that in my example I included that the VF BAR was only 8K, which
I think is a useful data point.

> +        if (vendor == PCI_VENDOR_ID_CHELSIO && (device & 0xff00) == 0x5800) {
> +            vdev->msix->pba_offset = 0x1000;
> +        } else {
> +            error_report("vfio: Hardware reports invalid configuration, "
> +            "MSIX data outside of specified BAR");

Please look at how the rest of file wraps long lines.  The standard I
try to use is to place additional lines justified to the right of the
open paren of the function, ex:

foo("blah blah... "
    "blah blah");

When that's not possible, additional lines should at least be indented
deeper than the first line.  Always try to be consistent with the file
you're contributing to.

> +            return -EINVAL;
> +        }
> +    }
> +
>      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..d98e6c9 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -114,6 +114,8 @@
>  #define PCI_VENDOR_ID_ENSONIQ            0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370     0x5000
>  
> +#define PCI_VENDOR_ID_CHELSIO            0x1425
> +
>  #define PCI_VENDOR_ID_FREESCALE          0x1957
>  #define PCI_DEVICE_ID_MPC8533E           0x0030
>