| Message ID | 20220528192057.30910-7-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOM'ify PIIX southbridge creation | expand |
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 <shentey@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. > /* Interrupt controller */ > qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq); > diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h > index 0a2ef0c7b6..e1f5d6d5c8 100644 > --- a/include/hw/southbridge/piix.h > +++ b/include/hw/southbridge/piix.h > @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE, > > PIIX3State *piix3_create(PCIBus *pci_bus); > > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus); > +DeviceState *piix4_create(PCIBus *pci_bus); > > #endif ATB, Mark.
On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland < mark.cave-ayland@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 <shentey@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. 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? Best regards, Bernhard > > /* Interrupt controller */ > > qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq); > > diff --git a/include/hw/southbridge/piix.h > b/include/hw/southbridge/piix.h > > index 0a2ef0c7b6..e1f5d6d5c8 100644 > > --- a/include/hw/southbridge/piix.h > > +++ b/include/hw/southbridge/piix.h > > @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE, > > > > PIIX3State *piix3_create(PCIBus *pci_bus); > > > > -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus); > > +DeviceState *piix4_create(PCIBus *pci_bus); > > > > #endif > > > ATB, > > Mark. >
On 29/05/2022 19:05, Bernhard Beschow wrote: > On Sun, May 29, 2022 at 11:25 AM Mark Cave-Ayland < > mark.cave-ayland@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 <shentey@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). > 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.
Am 30. Mai 2022 19:58:37 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@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-ayland@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 <shentey@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.
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"); + 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")); /* Interrupt controller */ qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq); diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h index 0a2ef0c7b6..e1f5d6d5c8 100644 --- a/include/hw/southbridge/piix.h +++ b/include/hw/southbridge/piix.h @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE, PIIX3State *piix3_create(PCIBus *pci_bus); -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus); +DeviceState *piix4_create(PCIBus *pci_bus); #endif
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 <shentey@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(-)