Indefinite recursion in pci_default_read_config

Hi all,

I just triggered a nasty indefinite recursion in pci_default_read_config:

uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)

{
     uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);

     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }

     len = MIN(len, pci_config_size(d) - address);
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
}

And d->cap.config_read is pointing to pci_default_read_config:

(gdb) print *d
$3 = {qdev = {id = 0xc99b10 "01:10.0", state = DEV_STATE_INITIALIZED,

     opts = 0xc99ad0, hotplugged = 0, info = 0x837e60, parent_bus = 0xc71710,
     num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0,
     child_bus = {lh_first = 0x0}, num_child_bus = 0, sibling = {
       le_next = 0xc99c30, le_prev = 0xc71730}},
   config = 0xca3010 "\206\200\312\020\003",
   cmask = 0xca3120 "\377\377\377\377", wmask = 0xca3230 "",
   used = 0xca3340 "", bus = 0xc71710, devfn = 32,
   name = "pci-assign", '\000'<repeats 53 times>, io_regions = {{
       addr = 4060102656, size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 4060119040,
       size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}},

config_read = 0x46a050<assigned_dev_pci_read_config>,

config_write = 0x469f30<assigned_dev_pci_write_config>, irq = 0xca3450, irq_state = 0 '\000', cap_present = 0, msix_cap = 0 '\000', msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, msix_page_size = 0, msix_irq_entries = 0x0, cap = {supported = 1,

     start = 64, length = 16,
     config_read = 0x416770<pci_default_cap_read_config>,
     config_write = 0x46b750<assigned_device_pci_cap_write_config>}}

Quoted 41 lines · Hannes Reinecke

Hi all,

I just triggered a nasty indefinite recursion in pci_default_read_config:

uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)

{
     uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);

     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }

     len = MIN(len, pci_config_size(d) - address);
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
}

And d->cap.config_read is pointing to pci_default_read_config:

(gdb) print *d
$3 = {qdev = {id = 0xc99b10 "01:10.0", state = DEV_STATE_INITIALIZED,

     opts = 0xc99ad0, hotplugged = 0, info = 0x837e60, parent_bus = 0xc71710,
     num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0,
     child_bus = {lh_first = 0x0}, num_child_bus = 0, sibling = {
       le_next = 0xc99c30, le_prev = 0xc71730}},
   config = 0xca3010 "\206\200\312\020\003",
   cmask = 0xca3120 "\377\377\377\377", wmask = 0xca3230 "",
   used = 0xca3340 "", bus = 0xc71710, devfn = 32,
   name = "pci-assign", '\000'<repeats 53 times>, io_regions = {{
       addr = 4060102656, size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 4060119040,
       size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}},

config_read = 0x46a050<assigned_dev_pci_read_config>,

Quoted 9 lines

config_write = 0x469f30<assigned_dev_pci_write_config>, irq = 0xca3450, irq_state = 0 '\000', cap_present = 0, msix_cap = 0 '\000', msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, msix_page_size = 0, msix_irq_entries = 0x0, cap = {supported = 1,

     start = 64, length = 16,
     config_read = 0x416770<pci_default_cap_read_config>,
     config_write = 0x46b750<assigned_device_pci_cap_write_config>}}

Michael? This is likely a bad merge on my part. Can you help?

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 51 lines · Avi Kivity

Quoted 41 lines · Hannes Reinecke

Hi all,

I just triggered a nasty indefinite recursion in pci_default_read_config:

uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)

{
     uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);

     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }

     len = MIN(len, pci_config_size(d) - address);
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
}

And d->cap.config_read is pointing to pci_default_read_config:

(gdb) print *d
$3 = {qdev = {id = 0xc99b10 "01:10.0", state = DEV_STATE_INITIALIZED,

     opts = 0xc99ad0, hotplugged = 0, info = 0x837e60, parent_bus = 0xc71710,
     num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0,
     child_bus = {lh_first = 0x0}, num_child_bus = 0, sibling = {
       le_next = 0xc99c30, le_prev = 0xc71730}},
   config = 0xca3010 "\206\200\312\020\003",
   cmask = 0xca3120 "\377\377\377\377", wmask = 0xca3230 "",
   used = 0xca3340 "", bus = 0xc71710, devfn = 32,
   name = "pci-assign", '\000'<repeats 53 times>, io_regions = {{
       addr = 4060102656, size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 4060119040,
       size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}},

config_read = 0x46a050<assigned_dev_pci_read_config>,

Quoted 9 lines

config_write = 0x469f30<assigned_dev_pci_write_config>, irq = 0xca3450, irq_state = 0 '\000', cap_present = 0, msix_cap = 0 '\000', msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, msix_page_size = 0, msix_irq_entries = 0x0, cap = {supported = 1,

     start = 64, length = 16,
     config_read = 0x416770<pci_default_cap_read_config>,
     config_write = 0x46b750<assigned_device_pci_cap_write_config>}}

Quoted 4 lines

Michael? This is likely a bad merge on my part. Can you help?

Signature · 1 line

error compiling committee.c: too many arguments to function

Um, yes. I think the following is the right way to do this. As a side note, we really should work to remove all these hacks and make assignment use capability support in upstream qemu.

--

hw/pci.c +11 -4

index 110a5fc..a74d3d4 100644
@@ -1016,19 +1016,26 @@ static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
     }
 }
 
+uint32_t pci_read_config(PCIDevice *d,
+                         uint32_t address, int len)
+{
+    uint32_t val = 0;
+
+    len = MIN(len, pci_config_size(d) - address);
+    memcpy(&val, d->config + address, len);
+    return le32_to_cpu(val);
+}
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
-    uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);
 
     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }
 
-    len = MIN(len, pci_config_size(d) - address);
-    memcpy(&val, d->config + address, len);
-    return le32_to_cpu(val);
+    return pci_read_config(d, address, len);
 }
 
 static void pci_write_config(PCIDevice *pci_dev,

Quoted 108 lines · Michael S. Tsirkin

Quoted 51 lines · Avi Kivity

Quoted 41 lines · Hannes Reinecke

Hi all,

I just triggered a nasty indefinite recursion in pci_default_read_config:

uint32_t pci_default_read_config(PCIDevice *d,
uint32_t address, int len)

{
     uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);

     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }

     len = MIN(len, pci_config_size(d) - address);
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
}

And d->cap.config_read is pointing to pci_default_read_config:

(gdb) print *d
$3 = {qdev = {id = 0xc99b10 "01:10.0", state = DEV_STATE_INITIALIZED,

     opts = 0xc99ad0, hotplugged = 0, info = 0x837e60, parent_bus = 0xc71710,
     num_gpio_out = 0, gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0,
     child_bus = {lh_first = 0x0}, num_child_bus = 0, sibling = {
       le_next = 0xc99c30, le_prev = 0xc71730}},
   config = 0xca3010 "\206\200\312\020\003",
   cmask = 0xca3120 "\377\377\377\377", wmask = 0xca3230 "",
   used = 0xca3340 "", bus = 0xc71710, devfn = 32,
   name = "pci-assign", '\000'<repeats 53 times>, io_regions = {{
       addr = 4060102656, size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 4060119040,
       size = 16384, filtered_size = 16384, type = 0 '\000',
       map_func = 0x46a5f0<assigned_dev_iomem_map>}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}, {addr = 0, size = 0,
       filtered_size = 0, type = 0 '\000', map_func = 0}},

config_read = 0x46a050<assigned_dev_pci_read_config>,

Quoted 9 lines

config_write = 0x469f30<assigned_dev_pci_write_config>, irq = 0xca3450, irq_state = 0 '\000', cap_present = 0, msix_cap = 0 '\000', msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, msix_page_size = 0, msix_irq_entries = 0x0, cap = {supported = 1,

     start = 64, length = 16,
     config_read = 0x416770<pci_default_cap_read_config>,
     config_write = 0x46b750<assigned_device_pci_cap_write_config>}}

Quoted 4 lines

Michael? This is likely a bad merge on my part. Can you help?

Signature · 1 line

error compiling committee.c: too many arguments to function

Um, yes. I think the following is the right way to do this. As a side note, we really should work to remove all these hacks and make assignment use capability support in upstream qemu.

--

hw/pci.c +12 -5

index 110a5fc..a74d3d4 100644
@@ -1016,19 +1016,26 @@ static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
     }
 }
 
+uint32_t pci_read_config(PCIDevice *d,
+                         uint32_t address, int len)
+{
+    uint32_t val = 0;
+
+    len = MIN(len, pci_config_size(d) - address);
+    memcpy(&val, d->config + address, len);
+    return le32_to_cpu(val);
+}
+
 uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
-    uint32_t val = 0;
     assert(len == 1 || len == 2 || len == 4);
 
     if (pci_access_cap_config(d, address, len)) {
         return d->cap.config_read(d, address, len);
     }
 
-    len = MIN(len, pci_config_size(d) - address);
-    memcpy(&val, d->config + address, len);
-    return le32_to_cpu(val);
+    return pci_read_config(d, address, len);
 }
 
 static void pci_write_config(PCIDevice *pci_dev,

@@ -1052,7 +1059,7 @@ int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len)
 uint32_t pci_default_cap_read_config(PCIDevice *pci_dev,
                                      uint32_t address, int len)
 {
-    return pci_default_read_config(pci_dev, address, len);
+    return pci_read_config(pci_dev, address, len);
 }
 

hw/pci.c int pci_access_cap_config(PCIDevice *pci_dev, uint32_t address, int len) · line 1065

Quoted 1 line

hw/pci.c +0 -0

 void pci_default_cap_write_config(PCIDevice *pci_dev,

Ok, works. Except for a missing prototype in hw/pci.h :-)

Should just be static in fact. Here's a better one: