[Qemu-devel] [PATCH v2 0/2] sysbus/pci: allow better customisation of firmware device paths

[Qemu-devel] [PATCH v2 0/2] sysbus/pci: allow better customisation of firmware device paths

[Mark Cave-Ayland]

Here are a couple of patches coming out of my work to add bootindex support
to OpenBIOS.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v2:
- Rebase onto master
- Reword patch 1 as suggested by Laszlo (and double-check affected devices)
- Add Philippe's R-B for patch 2


Mark Cave-Ayland (2):
  sysbus: always allow explicit_ofw_unit_address() to override address
    generation
  pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

 hw/core/sysbus.c | 15 +++++++--------
 hw/pci/pci.c     |  4 +++-
 2 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Mark Cave-Ayland]

Some SysBusDevices either use sysbus_init_mmio() without
sysbus_mmio_map() or the first MMIO memory region doesn't represent the
bus address, causing a firmware device path with an invalid address to
be generated.

SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
method that can be used to override this process, but it was originally intended
only as as a fallback option meaning that any existing MMIO memory regions still
take priority whilst determining the firmware device address.

There is currently only one user of explicit_ofw_unit_address() and that
is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
defined. This enables us to allow explicit_ofw_unit_address() to take
priority without affecting backwards compatibility, allowing the address
to be customised as required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/core/sysbus.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index ecfb0cfc0e..1ee0c162f4 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -293,16 +293,8 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
 {
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
-    /* for the explicit unit address fallback case: */
     char *addr, *fw_dev_path;
 
-    if (s->num_mmio) {
-        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
-                               s->mmio[0].addr);
-    }
-    if (s->num_pio) {
-        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
-    }
     if (sbc->explicit_ofw_unit_address) {
         addr = sbc->explicit_ofw_unit_address(s);
         if (addr) {
@@ -311,6 +303,13 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
             return fw_dev_path;
         }
     }
+    if (s->num_mmio) {
+        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
+                               s->mmio[0].addr);
+    }
+    if (s->num_pio) {
+        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
+    }
     return g_strdup(qdev_fw_name(dev));
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Laszlo Ersek]

On 06/29/18 15:56, Mark Cave-Ayland wrote:
> Some SysBusDevices either use sysbus_init_mmio() without
> sysbus_mmio_map() or the first MMIO memory region doesn't represent the
> bus address, causing a firmware device path with an invalid address to
> be generated.
> 
> SysBusDeviceClass does provide a virtual explicit_ofw_unit_address()
> method that can be used to override this process, but it was originally intended
> only as as a fallback option meaning that any existing MMIO memory regions still
> take priority whilst determining the firmware device address.
> 
> There is currently only one user of explicit_ofw_unit_address() and that
> is the PCI expander bridge (PXB) device which has no MMIO/PIO resources
> defined. This enables us to allow explicit_ofw_unit_address() to take
> priority without affecting backwards compatibility, allowing the address
> to be customised as required.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/core/sysbus.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index ecfb0cfc0e..1ee0c162f4 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -293,16 +293,8 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
>  {
>      SysBusDevice *s = SYS_BUS_DEVICE(dev);
>      SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
> -    /* for the explicit unit address fallback case: */
>      char *addr, *fw_dev_path;
>  
> -    if (s->num_mmio) {
> -        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
> -                               s->mmio[0].addr);
> -    }
> -    if (s->num_pio) {
> -        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
> -    }
>      if (sbc->explicit_ofw_unit_address) {
>          addr = sbc->explicit_ofw_unit_address(s);
>          if (addr) {
> @@ -311,6 +303,13 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
>              return fw_dev_path;
>          }
>      }
> +    if (s->num_mmio) {
> +        return g_strdup_printf("%s@" TARGET_FMT_plx, qdev_fw_name(dev),
> +                               s->mmio[0].addr);
> +    }
> +    if (s->num_pio) {
> +        return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
> +    }
>      return g_strdup(qdev_fw_name(dev));
>  }
>  
> 


Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [PATCH v2 2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

[Mark Cave-Ayland]

The current implementation of pci_dev_fw_name() scans through a fixed
set of PCI class descriptions to determine the firmware device name
but in some cases this isn't always appropriate, for example with
the macio device which uses a class code of 0xff (unassigned).

Rather than add a new entry for the macio device and risk a potential
clash with another unassigned device later, add a check to
pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
preference to pci_dev_fw_name().

This enables PCI devices such as macio to set dc->fw_name as required
to match the name specified in the firmware.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 80bc45930d..126dd683dc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2411,12 +2411,14 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
 
 static char *pcibus_get_fw_dev_path(DeviceState *dev)
 {
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
     PCIDevice *d = (PCIDevice *)dev;
     char path[50], name[33];
     int off;
 
     off = snprintf(path, sizeof(path), "%s@%x",
-                   pci_dev_fw_name(dev, name, sizeof name),
+                   dc->fw_name ? dc->fw_name :
+                       pci_dev_fw_name(dev, name, sizeof name),
                    PCI_SLOT(d->devfn));
     if (PCI_FUNC(d->devfn))
         snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

[Michael S. Tsirkin]

On Fri, Jun 29, 2018 at 02:56:16PM +0100, Mark Cave-Ayland wrote:
> The current implementation of pci_dev_fw_name() scans through a fixed
> set of PCI class descriptions to determine the firmware device name
> but in some cases this isn't always appropriate, for example with
> the macio device which uses a class code of 0xff (unassigned).
> 
> Rather than add a new entry for the macio device and risk a potential
> clash with another unassigned device later, add a check to
> pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
> preference to pci_dev_fw_name().
> 
> This enables PCI devices such as macio to set dc->fw_name as required
> to match the name specified in the firmware.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pci/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 80bc45930d..126dd683dc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2411,12 +2411,14 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
>  
>  static char *pcibus_get_fw_dev_path(DeviceState *dev)
>  {
> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>      PCIDevice *d = (PCIDevice *)dev;
>      char path[50], name[33];
>      int off;
>  
>      off = snprintf(path, sizeof(path), "%s@%x",
> -                   pci_dev_fw_name(dev, name, sizeof name),
> +                   dc->fw_name ? dc->fw_name :
> +                       pci_dev_fw_name(dev, name, sizeof name),
>                     PCI_SLOT(d->devfn));

So why not put the logic into pci_dev_fw_name then?



>      if (PCI_FUNC(d->devfn))
>          snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> -- 
> 2.11.0

Re: [Qemu-devel] [PATCH v2 2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

[Mark Cave-Ayland]

On 29/06/18 18:04, Michael S. Tsirkin wrote:

> On Fri, Jun 29, 2018 at 02:56:16PM +0100, Mark Cave-Ayland wrote:
>> The current implementation of pci_dev_fw_name() scans through a fixed
>> set of PCI class descriptions to determine the firmware device name
>> but in some cases this isn't always appropriate, for example with
>> the macio device which uses a class code of 0xff (unassigned).
>>
>> Rather than add a new entry for the macio device and risk a potential
>> clash with another unassigned device later, add a check to
>> pcibus_get_fw_dev_path() that will use DeviceClass fw_name if set in
>> preference to pci_dev_fw_name().
>>
>> This enables PCI devices such as macio to set dc->fw_name as required
>> to match the name specified in the firmware.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/pci/pci.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 80bc45930d..126dd683dc 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2411,12 +2411,14 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
>>   
>>   static char *pcibus_get_fw_dev_path(DeviceState *dev)
>>   {
>> +    DeviceClass *dc = DEVICE_GET_CLASS(dev);
>>       PCIDevice *d = (PCIDevice *)dev;
>>       char path[50], name[33];
>>       int off;
>>   
>>       off = snprintf(path, sizeof(path), "%s@%x",
>> -                   pci_dev_fw_name(dev, name, sizeof name),
>> +                   dc->fw_name ? dc->fw_name :
>> +                       pci_dev_fw_name(dev, name, sizeof name),
>>                      PCI_SLOT(d->devfn));
> 
> So why not put the logic into pci_dev_fw_name then?

To me it felt like pci_dev_fw_name() had already made the assumption 
that it's going to start interrogating PCI config space to determine the 
fw_name (the initialisers at the top of the function have already 
started pulling information from out of there) so it seemed a little odd 
to bury the logic in there.

However if you feel it should still belong in pci_dev_fw_name() then I'm 
happy to resubmit.


ATB,

Mark.