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.