Skip to contentSkip to conversation

pci_add_capability_at_offset

PATCH In review Michael S. Tsirkin 2 participants 6 messages Apr 7, 2010

lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

cover pci_add_capability_at_offset

eepro100 has to jump through hoops to add
capability at a specific offset. It's cleaner
to simply add an API for that.

Michael S. Tsirkin (2):
pci: add API to add capability at a known offset
eepro100: convert to new capability API

 hw/eepro100.c |   22 +++++++++-------------
 hw/pci.c      |   16 ++++++++++++----
 hw/pci.h      |    2 ++
 3 files changed, 23 insertions(+), 17 deletions(-)
author
#1 lore raw

PATCH 1/2 pci: add API to add capability at a known offset

Unlike virtio, device emulations need to add pci capabilities at known offsets to match real hardware. Make this possible by adding an appropriate API.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/pci.c |   16 ++++++++++++----
 hw/pci.h |    2 ++
 2 files changed, 14 insertions(+), 4 deletions(-)
hw/pci.c +12 -4
index 0dbca17..df03149 100644
@@ -1789,12 +1789,10 @@ static int pci_add_option_rom(PCIDevice *pdev)
 }
 
 /* Reserve space and add capability to the linked list in pci config space */
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
+                                 uint8_t offset, uint8_t size)
 {
-    uint8_t offset = pci_find_space(pdev, size);
     uint8_t *config = pdev->config + offset;
-    if (!offset)
-        return -ENOSPC;
     config[PCI_CAP_LIST_ID] = cap_id;
     config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
     pdev->config[PCI_CAPABILITY_LIST] = offset;

@@ -1807,6 +1805,16 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
     return offset;
 }
 
+/* Find and reserve space and add capability to the linked list
+ * in pci config space */
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t offset = pci_find_space(pdev, size);
+    if (!offset)
+        return -ENOSPC;
+    return pci_add_capability_at_offset(pdev, cap_id, offset, size);
+}
+
 /* Unlink capability from the pci config space. */
 void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
 {
hw/pci.h +2 -0
index 20c670e..625188c 100644
@@ -190,6 +190,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
                             PCIMapIORegionFunc *map_func);
 
 int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+int pci_add_capability_at_offset(PCIDevice *pci_dev, uint8_t cap_id,
+                                 uint8_t cap_offset, uint8_t cap_size);
 
 void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
Signature · 1 line
1.7.0.2.280.gc6f05
author
#3 lore raw

PATCH 2/2 eepro100: convert to new capability API

In reply to Michael S. Tsirkin

Using new pci_add_capability_at_offset makes
eepro100 code cleaner.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/eepro100.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)
hw/eepro100.c +9 -13
index 785a7da..a74d834 100644
@@ -539,21 +539,17 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
 
     if (e100_device->power_management) {
         /* Power Management Capabilities */
-        int cfg_offset;
-        pci_reserve_capability(&s->dev, PCI_CONFIG_HEADER_SIZE,
-                               0xdc - PCI_CONFIG_HEADER_SIZE);
-        cfg_offset = pci_add_capability(&s->dev, PCI_CAP_ID_PM, PCI_PM_SIZEOF);
-        assert(cfg_offset == 0xdc);
-        if (cfg_offset > 0) {
-            /* Power Management Capabilities */
-            pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+        int cfg_offset = 0xdc;
+        int r = pci_add_capability_at_offset(&s->dev, PCI_CAP_ID_PM,
+                                             cfg_offset, PCI_PM_SIZEOF);
+        assert(r >= 0);
+        pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
 #if 0 /* TODO: replace dummy code for power management emulation. */
-            /* TODO: Power Management Control / Status. */
-            pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
-            /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
-            pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
+        /* TODO: Power Management Control / Status. */
+        pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
+        /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
+        pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
 #endif
-        }
     }
 
 #if EEPROM_SIZE > 0
Signature · 1 line
1.7.0.2.280.gc6f05
#2 lore raw

[Qemu-devel] Re: [PATCH 1/2] pci: add API to add capability at a known offset

In reply to Michael S. Tsirkin
Quoted 31 lines · Michael S. Tsirkin

Unlike virtio, device emulations need to add pci capabilities at known offsets to match real hardware. Make this possible by adding an appropriate API.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 hw/pci.c |   16 ++++++++++++----
 hw/pci.h |    2 ++
 2 files changed, 14 insertions(+), 4 deletions(-)
hw/pci.c +2 -4
index 0dbca17..df03149 100644
@@ -1789,12 +1789,10 @@ static int pci_add_option_rom(PCIDevice *pdev)
 }
 
 /* Reserve space and add capability to the linked list in pci config space */
-int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+int pci_add_capability_at_offset(PCIDevice *pdev, uint8_t cap_id,
+                                 uint8_t offset, uint8_t size)
 {
-    uint8_t offset = pci_find_space(pdev, size);
     uint8_t *config = pdev->config + offset;
-    if (!offset)
-        return -ENOSPC;
     config[PCI_CAP_LIST_ID] = cap_id;
     config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
     pdev->config[PCI_CAPABILITY_LIST] = offset;

@@ -1807,6 +1805,16 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
     return offset;

hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size) · line 1815

Quoted 10 lines
hw/pci.c +7 -0
 }
 
+/* Find and reserve space and add capability to the linked list
+ * in pci config space */
+int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
+{
+    uint8_t offset = pci_find_space(pdev, size);
+    if (!offset)
+        return -ENOSPC;

{} Coding style? The rest is ok.

Quoted 20 lines
hw/pci.c +3 -0
+    return pci_add_capability_at_offset(pdev, cap_id, offset, size);
+}
+
 /* Unlink capability from the pci config space. */
 void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)

{

hw/pci.h +2 -0
index 20c670e..625188c 100644
@@ -190,6 +190,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
                             PCIMapIORegionFunc *map_func);
 
 int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
+int pci_add_capability_at_offset(PCIDevice *pci_dev, uint8_t cap_id,
+                                 uint8_t cap_offset, uint8_t cap_size);
 
 void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
Acked-by on v1 2/2
#4 lore raw

[Qemu-devel] Re: [PATCH 2/2] eepro100: convert to new capability API

In reply to Michael S. Tsirkin
Quoted 16 lines · Michael S. Tsirkin

Using new pci_add_capability_at_offset makes
eepro100 code cleaner.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
hw/eepro100.c | 22 +++++++++-------------

1 files changed, 9 insertions(+), 13 deletions(-)
hw/eepro100.c +0 -0
index 785a7da..a74d834 100644
@@ -539,21 +539,17 @@ static void e100_pci_reset(EEPRO100State * s,
 E100PCIDeviceInfo *e100_device)
 
 if (e100_device->power_management) {

hw/eepro100.c static void e100_pci_reset(EEPRO100State * s, · line 543

Quoted 10 lines
hw/eepro100.c +1 -8
 /* Power Management Capabilities */
- int cfg_offset;
- pci_reserve_capability(&s->dev, PCI_CONFIG_HEADER_SIZE,
- 0xdc - PCI_CONFIG_HEADER_SIZE);
- cfg_offset = pci_add_capability(&s->dev, PCI_CAP_ID_PM, PCI_PM_SIZEOF);
- assert(cfg_offset == 0xdc);
- if (cfg_offset > 0) {
- /* Power Management Capabilities */
- pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ int cfg_offset = 0xdc;

Suggestion:

uint8_t cfg_offset = 0xdc;

Quoted 17 lines
hw/eepro100.c +8 -5
+ int r = pci_add_capability_at_offset(&s->dev, PCI_CAP_ID_PM,
+ cfg_offset, PCI_PM_SIZEOF);
+ assert(r >= 0);
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
 #if 0 /* TODO: replace dummy code for power management emulation. */
- /* TODO: Power Management Control / Status. */
- pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
- /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
- pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
+ /* TODO: Power Management Control / Status. */
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
+ /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
+ pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
 #endif
- }
 }
Quoted 1 line

#if EEPROM_SIZE > 0

Acked-by Stefan Weil <[email protected]>
author
#5 lore raw

[Qemu-devel] Re: [PATCH 2/2] eepro100: convert to new capability API

Quoted 30 lines · Stefan Weil
Quoted 16 lines · Michael S. Tsirkin

Using new pci_add_capability_at_offset makes
eepro100 code cleaner.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
hw/eepro100.c | 22 +++++++++-------------

1 files changed, 9 insertions(+), 13 deletions(-)
hw/eepro100.c +0 -0
index 785a7da..a74d834 100644
@@ -539,21 +539,17 @@ static void e100_pci_reset(EEPRO100State * s,
 E100PCIDeviceInfo *e100_device)
 
 if (e100_device->power_management) {

hw/eepro100.c static void e100_pci_reset(EEPRO100State * s, · line 543

Quoted 10 lines
hw/eepro100.c +1 -8
 /* Power Management Capabilities */
- int cfg_offset;
- pci_reserve_capability(&s->dev, PCI_CONFIG_HEADER_SIZE,
- 0xdc - PCI_CONFIG_HEADER_SIZE);
- cfg_offset = pci_add_capability(&s->dev, PCI_CAP_ID_PM, PCI_PM_SIZEOF);
- assert(cfg_offset == 0xdc);
- if (cfg_offset > 0) {
- /* Power Management Capabilities */
- pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+ int cfg_offset = 0xdc;

Suggestion:

Quoted 1 line

uint8_t cfg_offset = 0xdc;

I don't think it matters as there's no chance of overflow or anything, all math is on small constants.

Quoted 20 lines
Quoted 17 lines
hw/eepro100.c +8 -5
+ int r = pci_add_capability_at_offset(&s->dev, PCI_CAP_ID_PM,
+ cfg_offset, PCI_PM_SIZEOF);
+ assert(r >= 0);
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
 #if 0 /* TODO: replace dummy code for power management emulation. */
- /* TODO: Power Management Control / Status. */
- pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
- /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
- pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
+ /* TODO: Power Management Control / Status. */
+ pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
+ /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
+ pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
 #endif
- }
 }
Quoted 1 line

#if EEPROM_SIZE > 0

Acked-by Stefan Weil <[email protected]>