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-ayl...@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));
> }
>
>
