Am 30. Mai 2022 19:58:37 UTC schrieb Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>: >On 29/05/2022 19:05, Bernhard Beschow wrote: > >> On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland < >> mark.cave-ayl...@ilande.co.uk> wrote: >> >>> On 28/05/2022 20:20, Bernhard Beschow wrote: >>> >>>> Just like the real hardware, create the PIIX4 ACPI controller as part of >>>> the PIIX4 southbridge. This also mirrors how the IDE and USB functions >>>> are already created. >>>> >>>> Signed-off-by: Bernhard Beschow <shen...@gmail.com> >>>> --- >>>> hw/isa/piix4.c | 25 ++++++++++++++----------- >>>> hw/mips/malta.c | 3 ++- >>>> include/hw/southbridge/piix.h | 2 +- >>>> 3 files changed, 17 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c >>>> index 96df21a610..217989227d 100644 >>>> --- a/hw/isa/piix4.c >>>> +++ b/hw/isa/piix4.c >>>> @@ -49,6 +49,7 @@ struct PIIX4State { >>>> RTCState rtc; >>>> PCIIDEState ide; >>>> UHCIState uhci; >>>> + PIIX4PMState pm; >>>> /* Reset Control Register */ >>>> MemoryRegion rcr_mem; >>>> uint8_t rcr; >>>> @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error >>> **errp) >>>> return; >>>> } >>>> >>>> + /* ACPI controller */ >>>> + qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3); >>>> + if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) { >>>> + return; >>>> + } >>>> + qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]); >>>> + object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm), >>> "i2c"); >>> >>> Now that the PM device is QOMified you don't actually need this alias >>> anymore (see >>> below). In general aliasing parts of contained devices onto the container >>> isn't >>> recommended, since it starts to blur the line between individual devices >>> and then you >>> find some wiring gets done to the container, some directly to the >>> contained device >>> and then it starts to get confusing quite quickly. >>> >>>> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, >>> PIIX_NUM_PIRQS); >>>> } >>>> >>>> @@ -271,6 +280,10 @@ static void piix4_init(Object *obj) >>>> object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC); >>>> object_initialize_child(obj, "ide", &s->ide, "piix4-ide"); >>>> object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci"); >>>> + >>>> + object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM); >>>> + qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100); >>>> + qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0); >>>> } >>>> >>>> static void piix4_class_init(ObjectClass *klass, void *data) >>>> @@ -312,7 +325,7 @@ static void piix4_register_types(void) >>>> >>>> type_init(piix4_register_types) >>>> >>>> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus) >>>> +DeviceState *piix4_create(PCIBus *pci_bus) >>>> { >>>> PCIDevice *pci; >>>> DeviceState *dev; >>>> @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus >>> **smbus) >>>> TYPE_PIIX4_PCI_DEVICE); >>>> dev = DEVICE(pci); >>>> >>>> - if (smbus) { >>>> - pci = pci_new(devfn + 3, TYPE_PIIX4_PM); >>>> - qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100); >>>> - qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0); >>>> - pci_realize_and_unref(pci, pci_bus, &error_fatal); >>>> - qdev_connect_gpio_out(DEVICE(pci), 0, >>>> - qdev_get_gpio_in_named(dev, "isa", 9)); >>>> - *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c")); >>>> - } >>>> - >>>> return dev; >>>> } >>>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c >>>> index e446b25ad0..b0fc84ccbb 100644 >>>> --- a/hw/mips/malta.c >>>> +++ b/hw/mips/malta.c >>>> @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine) >>>> empty_slot_init("GT64120", 0, 0x20000000); >>>> >>>> /* Southbridge */ >>>> - dev = piix4_create(pci_bus, &smbus); >>>> + dev = piix4_create(pci_bus); >>>> isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0")); >>>> + smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus")); >>> >>> It should now be possible to do something like this: >>> >>> pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm")); >>> smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c")); >>> >>> whereby we grab the reference to the PIIX4_PM device by resolving the "pm" >>> child >>> object and then use that to obtain the reference to smbus. >>> >> >> Imagining the container device to be an abstraction layer for the >> contained functionality I actually prefer the alias. Having it one >> doesn't need to know that there is an internal device named "pm" and >> one doesn't need to care about how many levels deep it is buried >> inside the implementation. This allows for further refactoring the >> PIIX4 without breaking its contract to its users. >> >> Also, this reflects how the real hardware works: The Malta board can >> wire up the PIIX4 southbridge without worrying about its inner >> details. One can look into the datasheets, see the exposed interfaces >> and pins, and connect them. By QOM'ifying PIIX4 we now have the >> opportunity to mirror the real hardware by exposing it to the Malta >> board as a black box which exposes dedicated pins and buses. > >It's a bit trickier in this particular case because here the PIIX4 device acts >as both a container and PCI device instance - and certainly the distinction >can be blurred when modelling integrated devices. > >The reason for leaning towards referencing the "pm" child object directly is >because a quick glance at the datasheet suggests that the PM functions and >smbus are exposed via function 3, so it feels that referencing that PCIDevice >instance to find the smbus is intuitive. > >For me using the "pm" child object is a shorter alternative to using >pci_find_device() to locate the function 3 PCIDevice function on the bus, >which is effectively what a guest OS would do. Since the PM device sits >directly on the PCI bus it would never change depth within the PIIX4 container >device, and renaming child object properties is an extremely rare event (which >would also be easily fixable with grep).
Alright, I'll implement it like this in v4. Best regards, Bernhard >> Looking further into the QEMU code, there is even the following >> comment in sabrelite.c: >> /* >> * TODO: Ideally we would expose the chip select and spi bus on the >> * SoC object using alias properties; then we would not need to >> * directly access the underlying spi device object. >> */ >> To me this comment seems that the authors think in a similar way. >> >> What do you think? > >It's difficult for me to know without having much knowledge of ARM devices, >but I can see how this might be useful given that SoCs can integrate a lot of >different devices into a single address space. Another thing to note is that >the comment above was originally written in 2016, and the qdev/QOM APIs (and >indeed our knowledge as to how best to use them) have improved considerably >since then. > > >ATB, > >Mark.