[1/2] sysbus: always allow explicit_ofw_unit_address() to override address generation

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

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

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.

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.