From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60130) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZA786-0007sc-8B for qemu-devel@nongnu.org; Tue, 30 Jun 2015 21:48:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZA785-0002K3-3J for qemu-devel@nongnu.org; Tue, 30 Jun 2015 21:48:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38167) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZA784-0002Jx-TV for qemu-devel@nongnu.org; Tue, 30 Jun 2015 21:48:01 -0400 From: Bandan Das References: <1435698210-15999-1-git-send-email-glaupre@chelsio.com> <8BEEA230E0FD5945BB88FC15B2A54777D0A957@nice.asicdesigners.com> Date: Tue, 30 Jun 2015 21:47:58 -0400 Message-ID: MIME-Version: 1.0 Content-Type: text/plain 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 , "alex.williamson@redhat.com" , "bsd@makefile.in" 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