From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Subject: Re: [Qemu-devel] [PATCH] kernel vfio: PCI ROM size calculation Date: Mon, 14 May 2012 13:24:22 +1000 Message-ID: <4FB07AE6.5080009@ozlabs.ru> References: <4FAE0824.4050209@ozlabs.ru> <1336964103.6954.12.camel@bling.home> Mime-Version: 1.0 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, Alex Graf , David Gibson To: Alex Williamson Return-path: Received: from mail-pb0-f46.google.com ([209.85.160.46]:50484 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753162Ab2ENDY2 (ORCPT ); Sun, 13 May 2012 23:24:28 -0400 Received: by pbbrp8 with SMTP id rp8so5541828pbb.19 for ; Sun, 13 May 2012 20:24:28 -0700 (PDT) In-Reply-To: <1336964103.6954.12.camel@bling.home> Sender: kvm-owner@vger.kernel.org List-ID: On 14/05/12 12:55, Alex Williamson wrote: > On Sat, 2012-05-12 at 16:50 +1000, Alexey Kardashevskiy wrote: >> The original code calculated the real size of the ROM and passed it to >> QEMU which tried to do pci_register_bar and failed with error message >> "ERROR: PCI region size must be pow2". >> >> The example card is PCIe Intel E1000E with the ROM size 0x9c00. >> >> pci_resource_len seems to be a proper solution. >> >> Signed-off-by: Alexey Kardashevskiy >> --- >> drivers/vfio/pci/vfio_pci.c | 9 +++------ >> 1 files changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index b2f1f3a..6d4a783 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -234,12 +234,9 @@ static long vfio_pci_ioctl(void *device_data, >> * this gives the user an easy way to determine whether >> * there's anything here w/o trying to read it. */ > > This is actually intentional, but maybe isn't the best way to handle it > anymore. The cut-off comment above reads: > > /* Report the actual ROM size instead of the BAR size, > * this gives the user an easy way to determine whether > * there's anything here w/o trying to read it. */ > > At the time I added that though, I think we still had a separate ioctl > to get the BAR size, so one gave the real size, the other the BAR size. > Do we care to expose that ROMs have real size, maybe zero, that doesn't > necessarily match pci_resource_len()? Thanks, The guest will still see aligned ROM BAR (not real one) and may try to read behind the real end of ROM, and it is allowed while it is still within the BAR, right? > > Alex > >> if (info.index == VFIO_PCI_ROM_REGION_INDEX) { >> - void __iomem *io; >> - size_t size; >> - >> - io = pci_map_rom(pdev, &size); >> - info.size = io ? size : 0; >> - pci_unmap_rom(pdev, io); >> + info.size = pci_resource_len(pdev, >> + PCI_ROM_RESOURCE); >> + info.flags |= VFIO_PCI_ROM_REGION_INDEX; >> } else if (flags & IORESOURCE_MEM) { >> info.size = pci_resource_len(pdev, info.index); >> info.flags |= (VFIO_REGION_INFO_FLAG_WRITE | >> > > > -- Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57187) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STlti-0005GR-Sf for qemu-devel@nongnu.org; Sun, 13 May 2012 23:24:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1STltg-0007ub-5T for qemu-devel@nongnu.org; Sun, 13 May 2012 23:24:34 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:41989) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STltf-0007u7-Uz for qemu-devel@nongnu.org; Sun, 13 May 2012 23:24:32 -0400 Received: by pbbro12 with SMTP id ro12so7862168pbb.4 for ; Sun, 13 May 2012 20:24:28 -0700 (PDT) Message-ID: <4FB07AE6.5080009@ozlabs.ru> Date: Mon, 14 May 2012 13:24:22 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <4FAE0824.4050209@ozlabs.ru> <1336964103.6954.12.camel@bling.home> In-Reply-To: <1336964103.6954.12.camel@bling.home> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] kernel vfio: PCI ROM size calculation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: David Gibson , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alex Graf On 14/05/12 12:55, Alex Williamson wrote: > On Sat, 2012-05-12 at 16:50 +1000, Alexey Kardashevskiy wrote: >> The original code calculated the real size of the ROM and passed it to >> QEMU which tried to do pci_register_bar and failed with error message >> "ERROR: PCI region size must be pow2". >> >> The example card is PCIe Intel E1000E with the ROM size 0x9c00. >> >> pci_resource_len seems to be a proper solution. >> >> Signed-off-by: Alexey Kardashevskiy >> --- >> drivers/vfio/pci/vfio_pci.c | 9 +++------ >> 1 files changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index b2f1f3a..6d4a783 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -234,12 +234,9 @@ static long vfio_pci_ioctl(void *device_data, >> * this gives the user an easy way to determine whether >> * there's anything here w/o trying to read it. */ > > This is actually intentional, but maybe isn't the best way to handle it > anymore. The cut-off comment above reads: > > /* Report the actual ROM size instead of the BAR size, > * this gives the user an easy way to determine whether > * there's anything here w/o trying to read it. */ > > At the time I added that though, I think we still had a separate ioctl > to get the BAR size, so one gave the real size, the other the BAR size. > Do we care to expose that ROMs have real size, maybe zero, that doesn't > necessarily match pci_resource_len()? Thanks, The guest will still see aligned ROM BAR (not real one) and may try to read behind the real end of ROM, and it is allowed while it is still within the BAR, right? > > Alex > >> if (info.index == VFIO_PCI_ROM_REGION_INDEX) { >> - void __iomem *io; >> - size_t size; >> - >> - io = pci_map_rom(pdev, &size); >> - info.size = io ? size : 0; >> - pci_unmap_rom(pdev, io); >> + info.size = pci_resource_len(pdev, >> + PCI_ROM_RESOURCE); >> + info.flags |= VFIO_PCI_ROM_REGION_INDEX; >> } else if (flags & IORESOURCE_MEM) { >> info.size = pci_resource_len(pdev, info.index); >> info.flags |= (VFIO_REGION_INFO_FLAG_WRITE | >> > > > -- Alexey