Or you end up formalizing the concept of PCI Quirks as the kernel does rather
than scattering special exception code through the source ...
Casey
________________________________________
From: Alex Williamson [alex.william...@redhat.com]
Sent: Tuesday, June 30, 2015 3:28 PM
To: Bandan Das
Cc: Gabriel Laupre; qemu-devel@nongnu.org; jb-gnumli...@wisemo.com; Casey
Leedom; m...@redhat.com; Anish Bhatt; Michael Boksanyi; b...@makefile.in
Subject: Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio
T5 devices
On Tue, 2015-06-30 at 17:58 -0400, Bandan Das wrote:
> Gabriel Laupre <glau...@chelsio.com> writes:
> ...
> > + /* 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. */
> > + 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) {
> > +
> > + 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. */
> > + if (vendor == PCI_VENDOR_ID_CHELSIO && (device & 0xff00) ==
> > 0x5800) {
> > + vdev->msix->pba_offset = 0x1000;
>
> For the rare case where table_offset is wrong for the device being checked for
> above and pba_offset is actually correct, shouldn't we fail ?
>
> > + } else {
> > + error_report("vfio: Hardware reports invalid configuration, "
> > + "MSIX data outside of specified BAR");
>
> Since we are printing anyway, and we have already made the check above, why
> not print exactly what's wrong instead of "MSIX data" ?
Probably diminishing returns to get too specific, we just need to know
that it's a hardware bug. If we want the test to be more thorough, it
should be extracted from msix_init() so we're not duplicating code.
Thanks,
Alex
> > + 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
