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

[Qemu-devel] [PATCH 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>


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 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 an invalid
firmware device path to be generated.

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

As any class defining explicit_ofw_unit_address() has explicitly requested a
specialised behaviour then it should be used in preference to the default
implementation rather than being used as a fallback.

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 1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

[Laszlo Ersek]

Hi Mark,

On 06/23/18 10:50, 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 an invalid
> firmware device path to be generated.
> 
> SysBusDeviceClass does provide a virtual explicit_ofw_unit_address() method that
> can be used to override this process, but it is only considered as a fallback
> option meaning that any existing MMIO memory regions still take priority whilst
> determining the firmware device address.
> 
> As any class defining explicit_ofw_unit_address() has explicitly requested a
> specialised behaviour then it should be used in preference to the default
> implementation rather than being used as a fallback.

I disagree about the last paragraph, when put like this. I don't
disagree with the *goal* of the patch, however the original
justification for explicit_ofw_unit_address() was different.

It was meant as a fallback for distinguishing sysbus devices when those
sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
MMIO/PIO-based identification was not "right", the issue was that unique
identification was impossible in the absence of such resources. Please
see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
for SysBusDeviceClass", 2015-06-23).

I don't have anything against repurposing explicit_ofw_unit_address()
like this -- as long as you check that it doesn't change behavior for
existing devices -- it's just that we shouldn't justify the new purpose
with the original intent. The original intent was different.

I suggest stating, "we can have explicit_ofw_unit_address() take
priority in a backwards-compatible manner, because no sysbus device
currently has both explicit_ofw_unit_address() and MMIO/PIO resources".

(Obviously checking the validity of this statement is up to you; I'm
just suggesting what I'd see as one more precise explanation.)

Thanks,
Laszlo

> 
> 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));
>  }
>  
> 

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

[Mark Cave-Ayland]

On 25/06/18 08:32, Laszlo Ersek wrote:

Hi Laszlo,

>> As any class defining explicit_ofw_unit_address() has explicitly requested a
>> specialised behaviour then it should be used in preference to the default
>> implementation rather than being used as a fallback.
> 
> I disagree about the last paragraph, when put like this. I don't
> disagree with the *goal* of the patch, however the original
> justification for explicit_ofw_unit_address() was different.
> 
> It was meant as a fallback for distinguishing sysbus devices when those
> sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
> MMIO/PIO-based identification was not "right", the issue was that unique
> identification was impossible in the absence of such resources. Please
> see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
> for SysBusDeviceClass", 2015-06-23).
> 
> I don't have anything against repurposing explicit_ofw_unit_address()
> like this -- as long as you check that it doesn't change behavior for
> existing devices -- it's just that we shouldn't justify the new purpose
> with the original intent. The original intent was different.
> 
> I suggest stating, "we can have explicit_ofw_unit_address() take
> priority in a backwards-compatible manner, because no sysbus device
> currently has both explicit_ofw_unit_address() and MMIO/PIO resources".

Thanks for the feedback, I'm more than happy to update the commit 
message to better describe the original intent of the patch. How does 
the following sound to you?


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 is only 
considered 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.

> (Obviously checking the validity of this statement is up to you; I'm
> just suggesting what I'd see as one more precise explanation.)
  Yes, it seems correct to me - grep tells me the PXB device is the only 
user of explicit_ofw_unit_address() in the whole code base, and there 
are no sysbus_init_*() functions anywhere within pci_expander_bridge.c.


ATB,

Mark.

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

[Laszlo Ersek]

On 06/27/18 21:59, Mark Cave-Ayland wrote:
> On 25/06/18 08:32, Laszlo Ersek wrote:
> 
> Hi Laszlo,
> 
>>> As any class defining explicit_ofw_unit_address() has explicitly
>>> requested a
>>> specialised behaviour then it should be used in preference to the
>>> default
>>> implementation rather than being used as a fallback.
>>
>> I disagree about the last paragraph, when put like this. I don't
>> disagree with the *goal* of the patch, however the original
>> justification for explicit_ofw_unit_address() was different.
>>
>> It was meant as a fallback for distinguishing sysbus devices when those
>> sysbus devices had neither MMIO nor PIO resources. The issue wasn't that
>> MMIO/PIO-based identification was not "right", the issue was that unique
>> identification was impossible in the absence of such resources. Please
>> see commit 0b336b3b98d8 ("hw/core: explicit OFW unit address callback
>> for SysBusDeviceClass", 2015-06-23).
>>
>> I don't have anything against repurposing explicit_ofw_unit_address()
>> like this -- as long as you check that it doesn't change behavior for
>> existing devices -- it's just that we shouldn't justify the new purpose
>> with the original intent. The original intent was different.
>>
>> I suggest stating, "we can have explicit_ofw_unit_address() take
>> priority in a backwards-compatible manner, because no sysbus device
>> currently has both explicit_ofw_unit_address() and MMIO/PIO resources".
> 
> Thanks for the feedback, I'm more than happy to update the commit
> message to better describe the original intent of the patch. How does
> the following sound to you?
> 
> 
> 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 is only
> considered as a fallback option meaning that any existing MMIO memory
> regions still take priority whilst determining the firmware device address.

s/is only considered as/was originally intended only as/

and then it looks great to me.

Thank you!
Laszlo

> 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.
> 
>> (Obviously checking the validity of this statement is up to you; I'm
>> just suggesting what I'd see as one more precise explanation.)
>  Yes, it seems correct to me - grep tells me the PXB device is the only
> user of explicit_ofw_unit_address() in the whole code base, and there
> are no sysbus_init_*() functions anywhere within pci_expander_bridge.c.
> 
> 
> ATB,
> 
> Mark.

[Qemu-devel] [PATCH 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>
---
 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 2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

[Philippe Mathieu-Daudé]

On 06/23/2018 05:50 AM, 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));
>      if (PCI_FUNC(d->devfn))
>          snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> 

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

[Michael S. Tsirkin]

On Sat, Jun 23, 2018 at 09:50:28AM +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>


Do we know no existing pci device sets this?

> ---
>  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 2/2] pci: allow DeviceClass fw_name to override pci_dev_fw_name() if set

[Mark Cave-Ayland]

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

> On Sat, Jun 23, 2018 at 09:50:28AM +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>
> 
> 
> Do we know no existing pci device sets this?

Here was my basic check:

# Find all files containing PCIDevice
find . -type f -exec grep -l "PCIDevice" {} \; > /tmp/pcidevices.txt
# Of these return files containing 'dc->fw_name'
for FILE in `cat /tmp/pcidevices.txt`; do grep -H 'dc->fw_name' $FILE; done;

./hw/ppc/spapr.c:        snprintf(cpu_model, 32, "%s@%x", dc->fw_name, 
index);
./hw/ppc/spapr.c:        nodename = g_strdup_printf("%s@%x", 
dc->fw_name, index);
./hw/ppc/spapr.c:    nodename = g_strdup_printf("%s@%x", dc->fw_name, id);
./hw/pci-bridge/pci_expander_bridge.c:    dc->fw_name = "pci";
./hw/pci-host/designware.c:    dc->fw_name = "pci";
./hw/pci-host/piix.c:    dc->fw_name = "pci";
./hw/pci-host/gpex.c:    dc->fw_name = "pci";
./hw/pci-host/xilinx-pcie.c:    dc->fw_name = "pci";
./hw/pci-host/q35.c:    dc->fw_name = "pci";
./hw/pci-host/prep.c:    dc->fw_name = "pci";

 From what I can see the usage in spapr.c is for setting the DT CPU name 
whilst all of the others are derived from 
PCIE_HOST_BRIDGE/PCI_HOST_BRIDGE which are ultimately sysbus devices.


ATB,

Mark.