From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZA41L-0007A4-SL for qemu-devel@nongnu.org; Tue, 30 Jun 2015 18:28:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZA41E-0004EM-L7 for qemu-devel@nongnu.org; Tue, 30 Jun 2015 18:28:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZA41E-0004E8-Fc for qemu-devel@nongnu.org; Tue, 30 Jun 2015 18:28:44 -0400 Message-ID: <1435703321.3700.544.camel@redhat.com> From: Alex Williamson Date: Tue, 30 Jun 2015 16:28:41 -0600 In-Reply-To: References: <1435698210-15999-1-git-send-email-glaupre@chelsio.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T5 devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bandan Das Cc: jb-gnumlists@wisemo.com, leedom@chelsio.com, mst@redhat.com, qemu-devel@nongnu.org, anish@chelsio.com, mboksanyi@chelsio.com, Gabriel Laupre , bsd@makefile.in On Tue, 2015-06-30 at 17:58 -0400, Bandan Das wrote: > Gabriel Laupre 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