On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, BALATON Zoltan wrote:
>> Instead of passing unneeded enclosing objects to the config direct
>> access ops that only need the bus we can pass that directly thus
>> simplifying the functions.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/pci-host/raven.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index d7a0bde382..2057a1869f 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -65,16 +65,16 @@ static inline uint32_t raven_idsel_to_addr(hwaddr addr)
>> static void raven_mmcfg_write(void *opaque, hwaddr addr, uint64_t val,
>> unsigned int size)
>> {
>> - PREPPCIState *s = opaque;
>> - PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> - pci_data_write(phb->bus, raven_idsel_to_addr(addr), val, size);
>> + PCIBus *hbus = opaque;
>> +
>> + pci_data_write(hbus, raven_idsel_to_addr(addr), val, size);
>> }
>> static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned
>> int size)
>> {
>> - PREPPCIState *s = opaque;
>> - PCIHostState *phb = PCI_HOST_BRIDGE(s);
>> - return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
>> + PCIBus *hbus = opaque;
>> +
>> + return pci_data_read(hbus, raven_idsel_to_addr(addr), size);
>> }
>> static const MemoryRegionOps raven_mmcfg_ops = {
>> @@ -233,7 +233,7 @@ static void raven_pcihost_realizefn(DeviceState *d,
>> Error **errp)
>> "pci-conf-data", 4);
>> memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>> - memory_region_init_io(&h->mmcfg, OBJECT(s), &raven_mmcfg_ops, s,
>> + memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
>> "pci-mmcfg", 0x00400000);
>> memory_region_add_subregion(address_space_mem, 0x80800000,
>> &h->mmcfg);
>
> I find this confusing as a reviewer since the general expectation is that the
> device is passed as the opaque for the memory region rather than the bus.
> What is the reason for trying to change an existing convention here?
I don't think there is such convention that it must be the device state,
that's just a common thing and obvious choice in a lot of cases but the
opaque parameter is defined to take a pointer to some data the callback
needs and what it is is defined by the callback. As this callback only
needs the bus it's simplest to pass that as the opaque data.
> You could simplify what is there by dropping the PREPPCIState reference and
> simply doing:
>
> static uint64_t raven_mmcfg_read(void *opaque, hwaddr addr, unsigned int
> size)
> {
> PCIHostState *phb = PCI_HOST_BRIDGE(opaque);
> return pci_data_read(phb->bus, raven_idsel_to_addr(addr), size);
> }
>
> etc.
So you broke your own convention here already by passing the parent object
and not the PREPPCI. Then we can go further the same way and pass only
what the callback needs which is what my patch does. (There is also no
convention that opaque must be an object or device so no need to QOM cast
it either. The type is verified when registered the callback.)
Regards,
BALATON Zoltan