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.