[Qemu-devel] [PATCH 0/2] pci_add_capability_at_offset

[Qemu-devel] [PATCH 0/2] pci_add_capability_at_offset

[Michael S. Tsirkin]

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(-)

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

[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 <mst@redhat.com>
---
 hw/pci.c |   16 ++++++++++++----
 hw/pci.h |    2 ++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 0dbca17..df03149 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -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)
 {
diff --git a/hw/pci.h b/hw/pci.h
index 20c670e..625188c 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -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);
 
-- 
1.7.0.2.280.gc6f05

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

[Michael S. Tsirkin]

Using new pci_add_capability_at_offset makes
eepro100 code cleaner.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/eepro100.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 785a7da..a74d834 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -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
-- 
1.7.0.2.280.gc6f05

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

[Stefan Weil]

Michael S. Tsirkin schrieb:
> 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 <mst@redhat.com>
> ---
>  hw/pci.c |   16 ++++++++++++----
>  hw/pci.h |    2 ++
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 0dbca17..df03149 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -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;
>   

{} Coding style? The rest is ok.

> +    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)
>  {
> diff --git a/hw/pci.h b/hw/pci.h
> index 20c670e..625188c 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -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);
>  
>   

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

[Stefan Weil]

Michael S. Tsirkin schrieb:
> Using new pci_add_capability_at_offset makes
> eepro100 code cleaner.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> hw/eepro100.c | 22 +++++++++-------------
> 1 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 785a7da..a74d834 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -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;

Suggestion:

uint8_t 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

Acked-by: Stefan Weil <weil@mail.berlios.de>

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

[Michael S. Tsirkin]

On Wed, Apr 07, 2010 at 10:42:07AM +0200, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > Using new pci_add_capability_at_offset makes
> > eepro100 code cleaner.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > hw/eepro100.c | 22 +++++++++-------------
> > 1 files changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index 785a7da..a74d834 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -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;
> 
> Suggestion:
> 
> 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.

> > + 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
> 
> Acked-by: Stefan Weil <weil@mail.berlios.de>