PCI IO regions must be power of two

PATCH Marcelo Tosatti 3 participants 4 messages Mar 11, 2008 – Mar 12, 2008 KVM

lore

Share & tools

lore

Marcelo Tosatti

author

2008-03-11 19:52 UTC #0 lore raw

Anthony,

Both virtio-net and virtio-block currently register PCI IO space regions that are not power of two in size.

The decoding process to discover the size of a PCI resource expects it to be a power of two. The PCI controller masks the size out of what is written into

config_space + 0x10 + (4 * region_num)

The result is that the size is calculated and registered erroneously by the OS:

[root@localhost ~]# cat /proc/ioports | grep virtio
c200-c203 : virtio-pci

This is a virtio-block device whose BAR0 has length (16+20)-1, not 4.

      BAR0: I/O at 0xc200 [0xc223].

I suggest forcing the size to be power of two as follows:

qemu/hw/pci.c +7 -0

Index: kvm-userspace.hotplug2/qemu/hw/pci.c
===================================================================
@@ -236,6 +236,13 @@ void pci_register_io_region(PCIDevice *p
 
     if ((unsigned int)region_num >= PCI_NUM_REGIONS)
         return;
+
+    /* IO region size must be power of two */
+    if (type == PCI_ADDRESS_SPACE_IO && (size & (size-1))) {
+        size = size << 1;
+        size &= size-1;
+    }
+
     r = &pci_dev->io_regions[region_num];
     r->addr = -1;
     r->size = size;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Anthony Liguori

2008-03-11 20:19 UTC #1 lore raw

Quoted 14 lines · Marcelo Tosatti

Anthony,
Both virtio-net and virtio-block currently register PCI IO space regions that are not power of two in size.
The decoding process to discover the size of a PCI resource expects it to be a power of two. The PCI controller masks the size out of what is written into
config_space + 0x10 + (4 * region_num)
The result is that the size is calculated and registered erroneously by the OS:

Quoted 10 lines

[root@localhost ~]# cat /proc/ioports | grep virtio
c200-c203 : virtio-pci
This is a virtio-block device whose BAR0 has length (16+20)-1, not 4.
      BAR0: I/O at 0xc200 [0xc223].
I suggest forcing the size to be power of two as follows:

A quick grep of the source show that virtio seems to be the only device that isn't behaving here. I suggest modifying the virtio.c to always use a power of two and then perhaps adding a check in hw/pci.c to validate that the registered region is a power of two in size.

Regards,

Anthony Liguori

Quoted 10 lines

Index: kvm-userspace.hotplug2/qemu/hw/pci.c
===================================================================
qemu/hw/pci.c +0 -0
@@ -236,6 +236,13 @@ void pci_register_io_region(PCIDevice *p
 
     if ((unsigned int)region_num >= PCI_NUM_REGIONS)
         return;

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 248

Quoted 10 lines

qemu/hw/pci.c +7 -0

+
+    /* IO region size must be power of two */
+    if (type == PCI_ADDRESS_SPACE_IO && (size & (size-1))) {
+        size = size << 1;
+        size &= size-1;
+    }
+
     r = &pci_dev->io_regions[region_num];
     r->addr = -1;
     r->size = size;

Uri Lublin

2008-03-12 15:39 UTC #2 lore raw

In reply to Marcelo Tosatti

From: Marcelo Tosatti

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 241

Quoted 12 lines

Index: kvm-userspace.hotplug2/qemu/hw/pci.c
===================================================================

qemu/hw/pci.c +3 -0

@@ -236,6 +236,13 @@ void pci_register_io_region(PCIDevice *p
 
    if ((unsigned int)region_num >= PCI_NUM_REGIONS)
        return;
+
+    /* IO region size must be power of two */
+    if (type == PCI_ADDRESS_SPACE_IO && (size & (size-1))) {

Why only for PCI IO regions ? Don't PCI memory regions have the same restriction ?

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 243

Quoted 2 lines

qemu/hw/pci.c +2 -0

+        size = size << 1;
+        size &= size-1;

That would not make size a power of 2 (e.g. size=7 --> size=12).

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 245

Quoted 2 lines

qemu/hw/pci.c +2 -0
+    }
+

Regards,
Uri.

_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Marcelo Tosatti

author

2008-03-12 15:59 UTC #3 lore raw

Quoted 8 lines · Uri Lublin

From: Marcelo Tosatti
Quoted 7 lines
Index: kvm-userspace.hotplug2/qemu/hw/pci.c
===================================================================
qemu/hw/pci.c +0 -0
@@ -236,6 +236,13 @@ void pci_register_io_region(PCIDevice *p
 

Quoted 7 lines

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 241
Quoted 5 lines
qemu/hw/pci.c +3 -0
    if ((unsigned int)region_num >= PCI_NUM_REGIONS)
        return;
+
+    /* IO region size must be power of two */
+    if (type == PCI_ADDRESS_SPACE_IO && (size & (size-1))) {
Why only for PCI IO regions ? Don't PCI memory regions have the same restriction ?

Yes, they do.

Quoted 4 lines

qemu/hw/pci.c void pci_register_io_region(PCIDevice *p · line 243
Quoted 2 lines
qemu/hw/pci.c +2 -0
+        size = size << 1;
+        size &= size-1;
That would not make size a power of 2 (e.g. size=7 --> size=12).

Right, sorry.

I'll resend a patch to have virtio allocate IO regions with proper sizes and warn if the IO _or_ memory region are not power of two, as Anthony suggested.

Since this problem is only visible with device hotplug (QEMU will allocate the region offsets properly during initialization) I'll wait for that to hit the tree.

Thanks.