kernel vfio: PCI ROM size calculation

PATCH In review Alexey Kardashevskiy 2 participants 4 messages May 12, 2012 – May 14, 2012

Subsystems (2)

KVM VFIO driver

lore

Share & tools

lore

Alexey Kardashevskiy

author

2012-05-12 06:50 UTC #0 lore raw

PATCH 1/1 kernel vfio: PCI ROM size calculation

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 <[email protected]>

---
 drivers/vfio/pci/vfio_pci.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

drivers/vfio/pci/vfio_pci.c +3 -6

index b2f1f3a..6d4a783 100644
@@ -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. */
 			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 |

Signature · 1 line

Alexey

Alex Williamson

2012-05-14 02:55 UTC #1 lore raw

Quoted 18 lines · Alexey Kardashevskiy

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 <[email protected]>
---
 drivers/vfio/pci/vfio_pci.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
drivers/vfio/pci/vfio_pci.c +0 -0
index b2f1f3a..6d4a783 100644

drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 235

Quoted 2 lines

drivers/vfio/pci/vfio_pci.c +0 -0

@@ -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,

Alex

Quoted 14 lines

drivers/vfio/pci/vfio_pci.c +3 -6

 			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 Kardashevskiy

author

2012-05-14 03:24 UTC #2 lore raw

Quoted 25 lines · Alex Williamson

Quoted 18 lines · Alexey Kardashevskiy
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 <[email protected]>
---
 drivers/vfio/pci/vfio_pci.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
drivers/vfio/pci/vfio_pci.c +0 -0
index b2f1f3a..6d4a783 100644
drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 235
Quoted 2 lines
drivers/vfio/pci/vfio_pci.c +0 -0
@@ -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:

Quoted 8 lines

/* 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?

Quoted 20 lines

Alex

Quoted 14 lines

drivers/vfio/pci/vfio_pci.c +3 -6

 			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 |

Signature · 1 line

Alexey

Alex Williamson

2012-05-14 04:34 UTC #3 lore raw

Quoted 30 lines · Alexey Kardashevskiy

Quoted 29 lines · Alex Williamson
Quoted 18 lines · Alexey Kardashevskiy
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 <[email protected]>
---
 drivers/vfio/pci/vfio_pci.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
drivers/vfio/pci/vfio_pci.c +0 -0
index b2f1f3a..6d4a783 100644
drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 235
Quoted 2 lines
drivers/vfio/pci/vfio_pci.c +0 -0
@@ -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. */

Quoted 8 lines

Quoted 4 lines
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?

Yes.

Quoted 19 lines

Quoted 17 lines

Quoted 14 lines

drivers/vfio/pci/vfio_pci.c +3 -6

 			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 |