[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T5 devices |
Date: |
Wed, 01 Jul 2015 12:18:19 -0600 |
On Wed, 2015-07-01 at 18:10 +0000, Gabriel Laupre wrote:
> > What you are suggesting is:
> > If table_offset is not as expected, then check if it's a chelsio device.
> > If it's not, then print a message. On the other hand, if it's a chelsio
> > device, then let msix_init() catch the error. Why ? And if we are sure that
> > msix_init will error out, what's the purpose of the table_offset check ?
>
> The test needs only to check the pba_offset and reduced as following is enough
>
> ...
> /*
> * Test the size of the pba_offset variable and catch if it extends
> outside
> * of the specified BAR. If it is the case, we need to apply a hardware
> * specific quirk if the device is known or we have a broken
> configuration.
> */
> if (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 while the BAR itself is only 8k. 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;
> } else {
> error_report("vfio: Hardware reports invalid configuration, "
> "MSIX data outside of specified BAR");
> return -EINVAL;
> }
> }
> ...
>
> As the hardware problem is only related with the pba_offset and the purpose
> of the quirk is to correct the known hardware error. The table_offset has
> never been seen as wrong. Therefore the msix_init() sanity check should take
> care of a "rare" potential error as you mentioned.
>
> This time I'll wait for ACKs from your side before submitting a new version :)
I would s/data/PBA/ in the error_report text for this version. Thanks,
Alex
> -----Original Message-----
> From: Bandan Das [mailto:address@hidden
> Sent: Tuesday, June 30, 2015 6:48 PM
> To: Gabriel Laupre
> Cc: address@hidden; Casey Leedom; address@hidden; address@hidden; Anish
> Bhatt; Michael Boksanyi; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for
> Chelsio T5 devices
>
> Gabriel Laupre <address@hidden> writes:
>
> > @ Bandan ...
> >> > + + /* 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 ?
> >
> > I don't know if it is relevant to do all the tests here because in the
> > function msix_init() all size are checked. I would prefer keeping this
> > test as this to simplify the quirk, i.e. just testing the device
> > first, and if another size than the pba_offset is wrong, then the
> > sanity check in the function msix_init() will catch the error.
>
> Ok, here's the excerpt:
>
> + /* 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;
> + } else {
> + error_report("vfio: Hardware reports invalid configuration, "
> + "MSIX data outside of specified BAR");
> + return -EINVAL;
> + }
> + }
>
>
> What you are suggesting is:
> If table_offset is not as expected, then check if it's a chelsio device.
> If it's not, then print a message. On the other hand, if it's a chelsio
> device, then let msix_init() catch the error. Why ? And if we are sure that
> msix_init will error out, what's the purpose of the table_offset check ?
>
> > @ Alex I corrected what you pointed out. I will send the patch v4 in a
> > minute.
> >
> > Thanks you
> >
> > Gabriel