From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAMaZ-00006v-H5 for qemu-devel@nongnu.org; Wed, 01 Jul 2015 14:18:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAMaU-0005Hu-3v for qemu-devel@nongnu.org; Wed, 01 Jul 2015 14:18:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAMaT-0005Hi-SN for qemu-devel@nongnu.org; Wed, 01 Jul 2015 14:18:22 -0400 Message-ID: <1435774699.3909.18.camel@redhat.com> From: Alex Williamson Date: Wed, 01 Jul 2015 12:18:19 -0600 In-Reply-To: <8BEEA230E0FD5945BB88FC15B2A54777D0B00D@nice.asicdesigners.com> References: <1435698210-15999-1-git-send-email-glaupre@chelsio.com> <8BEEA230E0FD5945BB88FC15B2A54777D0A957@nice.asicdesigners.com> <8BEEA230E0FD5945BB88FC15B2A54777D0B00D@nice.asicdesigners.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: Gabriel Laupre Cc: "jb-gnumlists@wisemo.com" , Casey Leedom , "mst@redhat.com" , "qemu-devel@nongnu.org" , Anish Bhatt , Michael Boksanyi , Bandan Das , "bsd@makefile.in" 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:bsd@redhat.com] > Sent: Tuesday, June 30, 2015 6:48 PM > To: Gabriel Laupre > Cc: jb-gnumlists@wisemo.com; Casey Leedom; mst@redhat.com; qemu-devel@nongnu.org; Anish Bhatt; Michael Boksanyi; alex.williamson@redhat.com; bsd@makefile.in > Subject: Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T5 devices > > Gabriel Laupre 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