[v2,06/13] mac_newworld: Simplify creation of Uninorth devices

Avoid open coding sysbus_mmio_map() and map regions in ascending
otder. Reorganise code a bit to avoid some casts.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/mac_newworld.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

On 25/09/2022 13:38, BALATON Zoltan wrote:

> Avoid open coding sysbus_mmio_map() and map regions in ascending
> otder. Reorganise code a bit to avoid some casts.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/mac_newworld.c | 42 +++++++++++++++++-------------------------
>   1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6bc3bd19be..b4ad43cc05 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -228,13 +228,6 @@ static void ppc_core99_init(MachineState *machine)
>           }
>       }
>   
> -    /* UniN init */
> -    dev = qdev_new(TYPE_UNI_NORTH);
> -    s = SYS_BUS_DEVICE(dev);
> -    sysbus_realize_and_unref(s, &error_fatal);
> -    memory_region_add_subregion(get_system_memory(), 0xf8000000,
> -                                sysbus_mmio_get_region(s, 0));
> -
>       openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
>       for (i = 0; i < machine->smp.cpus; i++) {
>           /* Mac99 IRQ connection between OpenPIC outputs pins
> @@ -275,24 +268,27 @@ static void ppc_core99_init(MachineState *machine)
>           }
>       }
>   
> +    /* UniN init */
> +    s = SYS_BUS_DEVICE(qdev_new(TYPE_UNI_NORTH));
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    sysbus_mmio_map(s, 0, 0xf8000000);
> +
>       if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
> +        machine_arch = ARCH_MAC99_U3;
>           /* 970 gets a U3 bus */
>           /* Uninorth AGP bus */
>           dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>           uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
>           s = SYS_BUS_DEVICE(dev);
> -        /* PCI hole */
> -        memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> -                                    sysbus_mmio_get_region(s, 2));
> -        /* Register 8 MB of ISA IO space */
> -        memory_region_add_subregion(get_system_memory(), 0xf2000000,
> -                                    sysbus_mmio_get_region(s, 3));
> +        sysbus_realize_and_unref(s, &error_fatal);
>           sysbus_mmio_map(s, 0, 0xf0800000);
>           sysbus_mmio_map(s, 1, 0xf0c00000);
> -
> -        machine_arch = ARCH_MAC99_U3;
> +        /* PCI hole */
> +        sysbus_mmio_map(s, 2, 0x80000000);
> +        /* Register 8 MB of ISA IO space */
> +        sysbus_mmio_map(s, 3, 0xf2000000);
>       } else {
> +        machine_arch = ARCH_MAC99;
>           /* Use values found on a real PowerMac */
>           /* Uninorth AGP bus */
>           uninorth_agp_dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
> @@ -312,19 +308,15 @@ static void ppc_core99_init(MachineState *machine)
>           /* Uninorth main bus */
>           dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
>           qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
> -        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>           uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
>           s = SYS_BUS_DEVICE(dev);
> -        /* PCI hole */
> -        memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
> -                                    sysbus_mmio_get_region(s, 2));
> -        /* Register 8 MB of ISA IO space */
> -        memory_region_add_subregion(get_system_memory(), 0xf2000000,
> -                                    sysbus_mmio_get_region(s, 3));
> +        sysbus_realize_and_unref(s, &error_fatal);
>           sysbus_mmio_map(s, 0, 0xf2800000);
>           sysbus_mmio_map(s, 1, 0xf2c00000);
> -
> -        machine_arch = ARCH_MAC99;
> +        /* PCI hole */
> +        sysbus_mmio_map(s, 2, 0x80000000);
> +        /* Register 8 MB of ISA IO space */
> +        sysbus_mmio_map(s, 3, 0xf2000000);
>       }
>   
>       machine->usb |= defaults_enabled() && !machine->usb_disabled;

Same comment here re: sysbus. Also the patch seems correct here, but it is worth 
noting that the PCI bus initialisation is order sensitive: the last bus created is 
the one that becomes the default PCI bus for -device, so changing this would break 
quite a few command lines...


ATB,

Mark.

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Cc'ing CLI refactor team.
>
> On 29/9/22 09:39, Mark Cave-Ayland wrote:
>> On 25/09/2022 13:38, BALATON Zoltan wrote:
>> 
>>> Avoid open coding sysbus_mmio_map() and map regions in ascending
>>> otder. Reorganise code a bit to avoid some casts.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/ppc/mac_newworld.c | 42 +++++++++++++++++-------------------------
>>>   1 file changed, 17 insertions(+), 25 deletions(-)
>
>> Same comment here re: sysbus. Also the patch seems correct here, but it is worth noting that the PCI bus initialisation is order sensitive: the 
>> last bus created is the one that becomes the default PCI bus for -device, so changing this would break quite a few command lines...
>
> Eh, I was not aware of this API fragility. So when using -device without
> expliciting the 'bus' key, the default is the latest bus created... OK.

Yes, our external interface is in part defined implicitly by the order
in board code execution.  It goes deeper than just CLI, I'm afraid.

Omitting bus= is a convenience feature for users.  It's clearly useful.
But what's the default?  We walk the qdev tree rooted at the main system
bus looking for a bus of suitable bus type that is not full, or else of
suitable type that is full.  See qdev_find_recursive().  The tree walk
visits children in creation order.  Therefore, bus creation order is
ABI.

Related: bus names.

For user-created buses, the bus name depends on the owning device's qdev
ID (specified with id=ID).  If it has none, the system picks a bus name;
more on that below.  Else, it is ID.N, where N counts from zero.  When a
device creates multiple buses of the same type, its bus creation order
is ABI.

For onboard devices, the bus name can be specified by board code.  If
board code elects not to, the system picks a name.

System-picked names are TYPE.N, where TYPE is the bus type name such as
"pci" or "isa", and N counts from zero separately for each type.  Again,
bus creation order is ABI.

This is qbus_init_internal().

Letting users omit qdev IDs is a convenience feature.

Letting developers delegate bus name picking to the system is also a
convenience feature.  Without it, we'd need bus name logic in board code
and not just qbus_init_internal().

I dislike implicit ABI definitions.

Rarely met a QEMU convenience feature that didn't bite us in the
posterior later (the sensitivity of my posterior may well cloud my
memory, though).

On Tue, Oct 04, 2022 at 08:39:57AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
> > Cc'ing CLI refactor team.
> >
> > On 29/9/22 09:39, Mark Cave-Ayland wrote:
> >> On 25/09/2022 13:38, BALATON Zoltan wrote:
> >> 
> >>> Avoid open coding sysbus_mmio_map() and map regions in ascending
> >>> otder. Reorganise code a bit to avoid some casts.
> >>>
> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >>> ---
> >>>   hw/ppc/mac_newworld.c | 42 +++++++++++++++++-------------------------
> >>>   1 file changed, 17 insertions(+), 25 deletions(-)
> >
> >> Same comment here re: sysbus. Also the patch seems correct here, but it is worth noting that the PCI bus initialisation is order sensitive: the 
> >> last bus created is the one that becomes the default PCI bus for -device, so changing this would break quite a few command lines...
> >
> > Eh, I was not aware of this API fragility. So when using -device without
> > expliciting the 'bus' key, the default is the latest bus created... OK.
> 
> Yes, our external interface is in part defined implicitly by the order
> in board code execution.  It goes deeper than just CLI, I'm afraid.
> 
> Omitting bus= is a convenience feature for users.  It's clearly useful.
> But what's the default?  We walk the qdev tree rooted at the main system
> bus looking for a bus of suitable bus type that is not full, or else of
> suitable type that is full.  See qdev_find_recursive().  The tree walk
> visits children in creation order.  Therefore, bus creation order is
> ABI.

There are different levels of ABI importance, however, depending on
which target/machine type we're talking about.

The critically important ABI is for anything that is part of, or used
by, machine types which are versioned. These are our top tier platforms,
typically associated with KVM and mgmt apps expect continuity from us,
and will usually be fully explicit in their configuration.

The less important ABI is for the non-versioned machine types. These
are typically associated with emulating legacy hardware platforms.
Users are more likely to use the convenience syntax, leaving out as
many implicit properties as possible.

ppc/mac_newworld is one of the latter class.

It is still desirable to avoid gratuitous changes that are likely
to break existing user's configurations of QEMU, but it is not
quite so critical. If someone can make a good case for why it is
better, it can be considered

With regards,
Daniel