[PATCH 0/9] QOM'ify VT82xx devices

[PATCH 0/9] QOM'ify VT82xx devices

[Bernhard Beschow]

This series instantiates all PCI functions of the VT82xx southbridges in the southbridges themselves.
For the IDE function this is especially important since its interrupt routing is configured in the
ISA function, hence doesn't make sense to instantiate it as a "Frankenstein" device. The interrupt
routing is currently hardcoded and changing that is currently not in the scope of this series.

Testing done:
* `qemu-system-ppc -machine pegasos2 -rtc base=localtime -device ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel morphos-3.17/boot.img`
  Boots successfully and it is possible to open games and tools.

* I was unable to test the fuloong2e board even before this series since it seems to be unfinished [1].
  A buildroot-baked kernel [2] booted but doesn't find its root partition, though the issues could be in the buildroot receipt I created.

[1] https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2
[2] https://github.com/shentok/buildroot/commits/fuloong2e

Bernhard Beschow (9):
  hw/isa/vt82c686: QOM'ify Super I/O creation
  hw/isa/vt82c686: Resolve unneeded attribute
  hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory()
  hw/isa/vt82c686: QOM'ify via-ide creation
  hw/isa/vt82c686: QOM'ify vt82c686b-usb-uhci creation
  hw/isa/vt82c686: QOM'ify pm creation
  hw/isa/vt82c686: QOM'ify ac97 and mc97 creation
  hw/isa/vt82c686: QOM'ify RTC creation
  hw/isa/vt82c686: Reuse errp

 configs/devices/mips64el-softmmu/default.mak |   1 -
 hw/isa/Kconfig                               |   1 +
 hw/isa/vt82c686.c                            | 119 +++++++++++++++----
 hw/mips/fuloong2e.c                          |  12 +-
 hw/ppc/Kconfig                               |   1 -
 hw/ppc/pegasos2.c                            |  14 +--
 6 files changed, 99 insertions(+), 49 deletions(-)

-- 
2.37.2

[PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[Bernhard Beschow]

The object creation now happens in chip-specific init methods which
allows the realize methods to be consolidated into one method. Shifting
the logic into the init methods has the addidional advantage that the
parent object's init methods are called implicitly.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8f656251b8..0217c98fe4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -544,7 +544,7 @@ struct ViaISAState {
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs;
     ISABus *isa_bus;
-    ViaSuperIOState *via_sio;
+    ViaSuperIOState via_sio;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
             d->wmask[i] = 0;
         }
     }
+
+    /* Super I/O */
+    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
+        return;
+    }
 }
 
 /* TYPE_VT82C686B_ISA */
@@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
     pci_default_write_config(d, addr, val, len);
     if (addr == 0x85) {
         /* BIT(1): enable or disable superio config io ports */
-        via_superio_io_enable(s->via_sio, val & BIT(1));
+        via_superio_io_enable(&s->via_sio, val & BIT(1));
     }
 }
 
@@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
 }
 
-static void vt82c686b_realize(PCIDevice *d, Error **errp)
+static void vt82c686b_init(Object *obj)
 {
-    ViaISAState *s = VIA_ISA(d);
+    ViaISAState *s = VIA_ISA(obj);
 
-    via_isa_realize(d, errp);
-    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
-                                               TYPE_VT82C686B_SUPERIO));
+    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT82C686B_SUPERIO);
 }
 
 static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->realize = vt82c686b_realize;
+    k->realize = via_isa_realize;
     k->config_write = vt82c686b_write_config;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
@@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
     .name          = TYPE_VT82C686B_ISA,
     .parent        = TYPE_VIA_ISA,
     .instance_size = sizeof(ViaISAState),
+    .instance_init = vt82c686b_init,
     .class_init    = vt82c686b_class_init,
 };
 
@@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d, uint32_t addr,
     pci_default_write_config(d, addr, val, len);
     if (addr == 0x50) {
         /* BIT(2): enable or disable superio config io ports */
-        via_superio_io_enable(s->via_sio, val & BIT(2));
+        via_superio_io_enable(&s->via_sio, val & BIT(2));
     }
 }
 
@@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
 }
 
-static void vt8231_realize(PCIDevice *d, Error **errp)
+static void vt8231_init(Object *obj)
 {
-    ViaISAState *s = VIA_ISA(d);
+    ViaISAState *s = VIA_ISA(obj);
 
-    via_isa_realize(d, errp);
-    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
-                                               TYPE_VT8231_SUPERIO));
+    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT8231_SUPERIO);
 }
 
 static void vt8231_class_init(ObjectClass *klass, void *data)
@@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->realize = vt8231_realize;
+    k->realize = via_isa_realize;
     k->config_write = vt8231_write_config;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
@@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
     .name          = TYPE_VT8231_ISA,
     .parent        = TYPE_VIA_ISA,
     .instance_size = sizeof(ViaISAState),
+    .instance_init = vt8231_init,
     .class_init    = vt8231_class_init,
 };
 
-- 
2.37.2

Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> The object creation now happens in chip-specific init methods which
> allows the realize methods to be consolidated into one method. Shifting
> the logic into the init methods has the addidional advantage that the
> parent object's init methods are called implicitly.

This and later patches titled "QOM'ify" don't do what I understand on 
QOMifying which is turining an old device model into a QOM object. These 
devices are already QOMified, what's really done here is that they are 
moved to the ViaISAState or embedded into it and created as part of the 
south bridge rather then individually by the boards. Either my 
understanding of what QOMify means is wrong or these patches are misnamed.

Technically via_isa_realize() is the realize method of the abstract 
TYPE_VIA_ISA class which were overriden by the subclasses but since QOM 
does not call overridden methods implicitly this had to be explicitly 
called in the overriding realize methods of the subclasses. Now that we 
don't have to ovverride the method maybe it could be set once on the 
VIA_ISA class then it may work that way but as it's done here is also OK 
maybe as a reminder that this super class method should be called by any 
overriding method if one's added in the future for some reason.

Regards,
BALATON Zoltan

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8f656251b8..0217c98fe4 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -544,7 +544,7 @@ struct ViaISAState {
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs;
>     ISABus *isa_bus;
> -    ViaSuperIOState *via_sio;
> +    ViaSuperIOState via_sio;
> };
>
> static const VMStateDescription vmstate_via = {
> @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>             d->wmask[i] = 0;
>         }
>     }
> +
> +    /* Super I/O */
> +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
> +        return;
> +    }
> }
>
> /* TYPE_VT82C686B_ISA */
> @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>     pci_default_write_config(d, addr, val, len);
>     if (addr == 0x85) {
>         /* BIT(1): enable or disable superio config io ports */
> -        via_superio_io_enable(s->via_sio, val & BIT(1));
> +        via_superio_io_enable(&s->via_sio, val & BIT(1));
>     }
> }
>
> @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
> }
>
> -static void vt82c686b_realize(PCIDevice *d, Error **errp)
> +static void vt82c686b_init(Object *obj)
> {
> -    ViaISAState *s = VIA_ISA(d);
> +    ViaISAState *s = VIA_ISA(obj);
>
> -    via_isa_realize(d, errp);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> -                                               TYPE_VT82C686B_SUPERIO));
> +    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT82C686B_SUPERIO);
> }
>
> static void vt82c686b_class_init(ObjectClass *klass, void *data)
> @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass, void *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->realize = vt82c686b_realize;
> +    k->realize = via_isa_realize;
>     k->config_write = vt82c686b_write_config;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
> @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
>     .name          = TYPE_VT82C686B_ISA,
>     .parent        = TYPE_VIA_ISA,
>     .instance_size = sizeof(ViaISAState),
> +    .instance_init = vt82c686b_init,
>     .class_init    = vt82c686b_class_init,
> };
>
> @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d, uint32_t addr,
>     pci_default_write_config(d, addr, val, len);
>     if (addr == 0x50) {
>         /* BIT(2): enable or disable superio config io ports */
> -        via_superio_io_enable(s->via_sio, val & BIT(2));
> +        via_superio_io_enable(&s->via_sio, val & BIT(2));
>     }
> }
>
> @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
> }
>
> -static void vt8231_realize(PCIDevice *d, Error **errp)
> +static void vt8231_init(Object *obj)
> {
> -    ViaISAState *s = VIA_ISA(d);
> +    ViaISAState *s = VIA_ISA(obj);
>
> -    via_isa_realize(d, errp);
> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> -                                               TYPE_VT8231_SUPERIO));
> +    object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT8231_SUPERIO);
> }
>
> static void vt8231_class_init(ObjectClass *klass, void *data)
> @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass, void *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->realize = vt8231_realize;
> +    k->realize = via_isa_realize;
>     k->config_write = vt8231_write_config;
>     k->vendor_id = PCI_VENDOR_ID_VIA;
>     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
> @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
>     .name          = TYPE_VT8231_ISA,
>     .parent        = TYPE_VIA_ISA,
>     .instance_size = sizeof(ViaISAState),
> +    .instance_init = vt8231_init,
>     .class_init    = vt8231_class_init,
> };
>
>

Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 6277 bytes --]

On Tue, Aug 23, 2022 at 2:35 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > The object creation now happens in chip-specific init methods which
> > allows the realize methods to be consolidated into one method. Shifting
> > the logic into the init methods has the addidional advantage that the
> > parent object's init methods are called implicitly.
>
> This and later patches titled "QOM'ify" don't do what I understand on
> QOMifying which is turining an old device model into a QOM object. These
> devices are already QOMified, what's really done here is that they are
> moved to the ViaISAState or embedded into it and created as part of the
> south bridge rather then individually by the boards. Either my
> understanding of what QOMify means is wrong or these patches are misnamed.
>

I think your understanding is correct. Peter refers to it as the
embed-the-device-struct style [1] which I can take as inspiration for
renaming my patches in v2.

Technically via_isa_realize() is the realize method of the abstract
> TYPE_VIA_ISA class which were overriden by the subclasses but since QOM
> does not call overridden methods implicitly this had to be explicitly
> called in the overriding realize methods of the subclasses. Now that we
> don't have to ovverride the method maybe it could be set once on the
> VIA_ISA class then it may work that way but as it's done here is also OK
> maybe as a reminder that this super class method should be called by any
> overriding method if one's added in the future for some reason.
>

Right. This would involve moving some code around and creating a class
struct. Do you think it's worth it?

Best regards,
Bernhard

[1]
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/

Regards,
> BALATON Zoltan
>
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
> > 1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 8f656251b8..0217c98fe4 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -544,7 +544,7 @@ struct ViaISAState {
> >     qemu_irq cpu_intr;
> >     qemu_irq *isa_irqs;
> >     ISABus *isa_bus;
> > -    ViaSuperIOState *via_sio;
> > +    ViaSuperIOState via_sio;
> > };
> >
> > static const VMStateDescription vmstate_via = {
> > @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
> >             d->wmask[i] = 0;
> >         }
> >     }
> > +
> > +    /* Super I/O */
> > +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
> > +        return;
> > +    }
> > }
> >
> > /* TYPE_VT82C686B_ISA */
> > @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d,
> uint32_t addr,
> >     pci_default_write_config(d, addr, val, len);
> >     if (addr == 0x85) {
> >         /* BIT(1): enable or disable superio config io ports */
> > -        via_superio_io_enable(s->via_sio, val & BIT(1));
> > +        via_superio_io_enable(&s->via_sio, val & BIT(1));
> >     }
> > }
> >
> > @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
> >     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
> > }
> >
> > -static void vt82c686b_realize(PCIDevice *d, Error **errp)
> > +static void vt82c686b_init(Object *obj)
> > {
> > -    ViaISAState *s = VIA_ISA(d);
> > +    ViaISAState *s = VIA_ISA(obj);
> >
> > -    via_isa_realize(d, errp);
> > -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> > -                                               TYPE_VT82C686B_SUPERIO));
> > +    object_initialize_child(obj, "sio", &s->via_sio,
> TYPE_VT82C686B_SUPERIO);
> > }
> >
> > static void vt82c686b_class_init(ObjectClass *klass, void *data)
> > @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass,
> void *data)
> >     DeviceClass *dc = DEVICE_CLASS(klass);
> >     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >
> > -    k->realize = vt82c686b_realize;
> > +    k->realize = via_isa_realize;
> >     k->config_write = vt82c686b_write_config;
> >     k->vendor_id = PCI_VENDOR_ID_VIA;
> >     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
> > @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
> >     .name          = TYPE_VT82C686B_ISA,
> >     .parent        = TYPE_VIA_ISA,
> >     .instance_size = sizeof(ViaISAState),
> > +    .instance_init = vt82c686b_init,
> >     .class_init    = vt82c686b_class_init,
> > };
> >
> > @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d,
> uint32_t addr,
> >     pci_default_write_config(d, addr, val, len);
> >     if (addr == 0x50) {
> >         /* BIT(2): enable or disable superio config io ports */
> > -        via_superio_io_enable(s->via_sio, val & BIT(2));
> > +        via_superio_io_enable(&s->via_sio, val & BIT(2));
> >     }
> > }
> >
> > @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
> >     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
> > }
> >
> > -static void vt8231_realize(PCIDevice *d, Error **errp)
> > +static void vt8231_init(Object *obj)
> > {
> > -    ViaISAState *s = VIA_ISA(d);
> > +    ViaISAState *s = VIA_ISA(obj);
> >
> > -    via_isa_realize(d, errp);
> > -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> > -                                               TYPE_VT8231_SUPERIO));
> > +    object_initialize_child(obj, "sio", &s->via_sio,
> TYPE_VT8231_SUPERIO);
> > }
> >
> > static void vt8231_class_init(ObjectClass *klass, void *data)
> > @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass,
> void *data)
> >     DeviceClass *dc = DEVICE_CLASS(klass);
> >     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >
> > -    k->realize = vt8231_realize;
> > +    k->realize = via_isa_realize;
> >     k->config_write = vt8231_write_config;
> >     k->vendor_id = PCI_VENDOR_ID_VIA;
> >     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
> > @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
> >     .name          = TYPE_VT8231_ISA,
> >     .parent        = TYPE_VIA_ISA,
> >     .instance_size = sizeof(ViaISAState),
> > +    .instance_init = vt8231_init,
> >     .class_init    = vt8231_class_init,
> > };
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 8304 bytes --]

Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> On Tue, Aug 23, 2022 at 2:35 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> The object creation now happens in chip-specific init methods which
>>> allows the realize methods to be consolidated into one method. Shifting
>>> the logic into the init methods has the addidional advantage that the
>>> parent object's init methods are called implicitly.
>>
>> This and later patches titled "QOM'ify" don't do what I understand on
>> QOMifying which is turining an old device model into a QOM object. These
>> devices are already QOMified, what's really done here is that they are
>> moved to the ViaISAState or embedded into it and created as part of the
>> south bridge rather then individually by the boards. Either my
>> understanding of what QOMify means is wrong or these patches are misnamed.
>>
>
> I think your understanding is correct. Peter refers to it as the
> embed-the-device-struct style [1] which I can take as inspiration for
> renaming my patches in v2.
>
> Technically via_isa_realize() is the realize method of the abstract
>> TYPE_VIA_ISA class which were overriden by the subclasses but since QOM
>> does not call overridden methods implicitly this had to be explicitly
>> called in the overriding realize methods of the subclasses. Now that we
>> don't have to ovverride the method maybe it could be set once on the
>> VIA_ISA class then it may work that way but as it's done here is also OK
>> maybe as a reminder that this super class method should be called by any
>> overriding method if one's added in the future for some reason.
>>
>
> Right. This would involve moving some code around and creating a class
> struct. Do you think it's worth it?

You mean a class_init func to assign realize as the class struct 
via_isa_info already exists. But yes this would need to be moved after 
via_isa_realize then. As I wrote above I don't think this is worth it, 
especially becuase if in the future a realize method was re-added to the 
subclasses then it's easy to forget to revert this and call the superclass 
method so assigning via_isa_realize to the subclass as done here is OK and 
I can think of it as a reminder to call this method when overriding it. 
Also with the added class_init func it's shorter this way even if slightly 
mixing different objects. So in the end I'm OK with this as it is.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
>
> [1]
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/
>
> Regards,
>> BALATON Zoltan
>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
>>> 1 file changed, 18 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8f656251b8..0217c98fe4 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -544,7 +544,7 @@ struct ViaISAState {
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa_irqs;
>>>     ISABus *isa_bus;
>>> -    ViaSuperIOState *via_sio;
>>> +    ViaSuperIOState via_sio;
>>> };
>>>
>>> static const VMStateDescription vmstate_via = {
>>> @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>>             d->wmask[i] = 0;
>>>         }
>>>     }
>>> +
>>> +    /* Super I/O */
>>> +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
>>> +        return;
>>> +    }
>>> }
>>>
>>> /* TYPE_VT82C686B_ISA */
>>> @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d,
>> uint32_t addr,
>>>     pci_default_write_config(d, addr, val, len);
>>>     if (addr == 0x85) {
>>>         /* BIT(1): enable or disable superio config io ports */
>>> -        via_superio_io_enable(s->via_sio, val & BIT(1));
>>> +        via_superio_io_enable(&s->via_sio, val & BIT(1));
>>>     }
>>> }
>>>
>>> @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
>>>     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
>>> }
>>>
>>> -static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>> +static void vt82c686b_init(Object *obj)
>>> {
>>> -    ViaISAState *s = VIA_ISA(d);
>>> +    ViaISAState *s = VIA_ISA(obj);
>>>
>>> -    via_isa_realize(d, errp);
>>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
>>> -                                               TYPE_VT82C686B_SUPERIO));
>>> +    object_initialize_child(obj, "sio", &s->via_sio,
>> TYPE_VT82C686B_SUPERIO);
>>> }
>>>
>>> static void vt82c686b_class_init(ObjectClass *klass, void *data)
>>> @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass *klass,
>> void *data)
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->realize = vt82c686b_realize;
>>> +    k->realize = via_isa_realize;
>>>     k->config_write = vt82c686b_write_config;
>>>     k->vendor_id = PCI_VENDOR_ID_VIA;
>>>     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
>>> @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
>>>     .name          = TYPE_VT82C686B_ISA,
>>>     .parent        = TYPE_VIA_ISA,
>>>     .instance_size = sizeof(ViaISAState),
>>> +    .instance_init = vt82c686b_init,
>>>     .class_init    = vt82c686b_class_init,
>>> };
>>>
>>> @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d,
>> uint32_t addr,
>>>     pci_default_write_config(d, addr, val, len);
>>>     if (addr == 0x50) {
>>>         /* BIT(2): enable or disable superio config io ports */
>>> -        via_superio_io_enable(s->via_sio, val & BIT(2));
>>> +        via_superio_io_enable(&s->via_sio, val & BIT(2));
>>>     }
>>> }
>>>
>>> @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
>>>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
>>> }
>>>
>>> -static void vt8231_realize(PCIDevice *d, Error **errp)
>>> +static void vt8231_init(Object *obj)
>>> {
>>> -    ViaISAState *s = VIA_ISA(d);
>>> +    ViaISAState *s = VIA_ISA(obj);
>>>
>>> -    via_isa_realize(d, errp);
>>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
>>> -                                               TYPE_VT8231_SUPERIO));
>>> +    object_initialize_child(obj, "sio", &s->via_sio,
>> TYPE_VT8231_SUPERIO);
>>> }
>>>
>>> static void vt8231_class_init(ObjectClass *klass, void *data)
>>> @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass,
>> void *data)
>>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>
>>> -    k->realize = vt8231_realize;
>>> +    k->realize = via_isa_realize;
>>>     k->config_write = vt8231_write_config;
>>>     k->vendor_id = PCI_VENDOR_ID_VIA;
>>>     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
>>> @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
>>>     .name          = TYPE_VT8231_ISA,
>>>     .parent        = TYPE_VIA_ISA,
>>>     .instance_size = sizeof(ViaISAState),
>>> +    .instance_init = vt8231_init,
>>>     .class_init    = vt8231_class_init,
>>> };
>>>
>>>
>>
>

Re: [PATCH 1/9] hw/isa/vt82c686: QOM'ify Super I/O creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 7531 bytes --]

On Wed, Aug 24, 2022 at 1:36 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > On Tue, Aug 23, 2022 at 2:35 AM BALATON Zoltan <balaton@eik.bme.hu>
> wrote:
> >
> >> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> >>> The object creation now happens in chip-specific init methods which
> >>> allows the realize methods to be consolidated into one method. Shifting
> >>> the logic into the init methods has the addidional advantage that the
> >>> parent object's init methods are called implicitly.
> >>
> >> This and later patches titled "QOM'ify" don't do what I understand on
> >> QOMifying which is turining an old device model into a QOM object. These
> >> devices are already QOMified, what's really done here is that they are
> >> moved to the ViaISAState or embedded into it and created as part of the
> >> south bridge rather then individually by the boards. Either my
> >> understanding of what QOMify means is wrong or these patches are
> misnamed.
> >>
> >
> > I think your understanding is correct. Peter refers to it as the
> > embed-the-device-struct style [1] which I can take as inspiration for
> > renaming my patches in v2.
> >
> > Technically via_isa_realize() is the realize method of the abstract
> >> TYPE_VIA_ISA class which were overriden by the subclasses but since QOM
> >> does not call overridden methods implicitly this had to be explicitly
> >> called in the overriding realize methods of the subclasses. Now that we
> >> don't have to ovverride the method maybe it could be set once on the
> >> VIA_ISA class then it may work that way but as it's done here is also OK
> >> maybe as a reminder that this super class method should be called by any
> >> overriding method if one's added in the future for some reason.
> >>
> >
> > Right. This would involve moving some code around and creating a class
> > struct. Do you think it's worth it?
>
> You mean a class_init func to assign realize as the class struct
> via_isa_info already exists.


Ah yes, of course.


> But yes this would need to be moved after
> via_isa_realize then. As I wrote above I don't think this is worth it,
> especially becuase if in the future a realize method was re-added to the
> subclasses then it's easy to forget to revert this and call the superclass
> method so assigning via_isa_realize to the subclass as done here is OK and
> I can think of it as a reminder to call this method when overriding it.
> Also with the added class_init func it's shorter this way even if slightly
> mixing different objects. So in the end I'm OK with this as it is.
>

Okay, I'll keep it as it is.

Thanks,
Bernhard

>
> Regards,
> BALATON Zoltan
>
> > Best regards,
> > Bernhard
> >
> > [1]
> >
> http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/
> >
> > Regards,
> >> BALATON Zoltan
> >>
> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>> ---
> >>> hw/isa/vt82c686.c | 33 ++++++++++++++++++---------------
> >>> 1 file changed, 18 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> >>> index 8f656251b8..0217c98fe4 100644
> >>> --- a/hw/isa/vt82c686.c
> >>> +++ b/hw/isa/vt82c686.c
> >>> @@ -544,7 +544,7 @@ struct ViaISAState {
> >>>     qemu_irq cpu_intr;
> >>>     qemu_irq *isa_irqs;
> >>>     ISABus *isa_bus;
> >>> -    ViaSuperIOState *via_sio;
> >>> +    ViaSuperIOState via_sio;
> >>> };
> >>>
> >>> static const VMStateDescription vmstate_via = {
> >>> @@ -602,6 +602,11 @@ static void via_isa_realize(PCIDevice *d, Error
> >> **errp)
> >>>             d->wmask[i] = 0;
> >>>         }
> >>>     }
> >>> +
> >>> +    /* Super I/O */
> >>> +    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
> >>> +        return;
> >>> +    }
> >>> }
> >>>
> >>> /* TYPE_VT82C686B_ISA */
> >>> @@ -615,7 +620,7 @@ static void vt82c686b_write_config(PCIDevice *d,
> >> uint32_t addr,
> >>>     pci_default_write_config(d, addr, val, len);
> >>>     if (addr == 0x85) {
> >>>         /* BIT(1): enable or disable superio config io ports */
> >>> -        via_superio_io_enable(s->via_sio, val & BIT(1));
> >>> +        via_superio_io_enable(&s->via_sio, val & BIT(1));
> >>>     }
> >>> }
> >>>
> >>> @@ -639,13 +644,11 @@ static void vt82c686b_isa_reset(DeviceState *dev)
> >>>     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
> >>> }
> >>>
> >>> -static void vt82c686b_realize(PCIDevice *d, Error **errp)
> >>> +static void vt82c686b_init(Object *obj)
> >>> {
> >>> -    ViaISAState *s = VIA_ISA(d);
> >>> +    ViaISAState *s = VIA_ISA(obj);
> >>>
> >>> -    via_isa_realize(d, errp);
> >>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> >>> -
>  TYPE_VT82C686B_SUPERIO));
> >>> +    object_initialize_child(obj, "sio", &s->via_sio,
> >> TYPE_VT82C686B_SUPERIO);
> >>> }
> >>>
> >>> static void vt82c686b_class_init(ObjectClass *klass, void *data)
> >>> @@ -653,7 +656,7 @@ static void vt82c686b_class_init(ObjectClass
> *klass,
> >> void *data)
> >>>     DeviceClass *dc = DEVICE_CLASS(klass);
> >>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>>
> >>> -    k->realize = vt82c686b_realize;
> >>> +    k->realize = via_isa_realize;
> >>>     k->config_write = vt82c686b_write_config;
> >>>     k->vendor_id = PCI_VENDOR_ID_VIA;
> >>>     k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
> >>> @@ -670,6 +673,7 @@ static const TypeInfo vt82c686b_isa_info = {
> >>>     .name          = TYPE_VT82C686B_ISA,
> >>>     .parent        = TYPE_VIA_ISA,
> >>>     .instance_size = sizeof(ViaISAState),
> >>> +    .instance_init = vt82c686b_init,
> >>>     .class_init    = vt82c686b_class_init,
> >>> };
> >>>
> >>> @@ -684,7 +688,7 @@ static void vt8231_write_config(PCIDevice *d,
> >> uint32_t addr,
> >>>     pci_default_write_config(d, addr, val, len);
> >>>     if (addr == 0x50) {
> >>>         /* BIT(2): enable or disable superio config io ports */
> >>> -        via_superio_io_enable(s->via_sio, val & BIT(2));
> >>> +        via_superio_io_enable(&s->via_sio, val & BIT(2));
> >>>     }
> >>> }
> >>>
> >>> @@ -703,13 +707,11 @@ static void vt8231_isa_reset(DeviceState *dev)
> >>>     pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
> >>> }
> >>>
> >>> -static void vt8231_realize(PCIDevice *d, Error **errp)
> >>> +static void vt8231_init(Object *obj)
> >>> {
> >>> -    ViaISAState *s = VIA_ISA(d);
> >>> +    ViaISAState *s = VIA_ISA(obj);
> >>>
> >>> -    via_isa_realize(d, errp);
> >>> -    s->via_sio = VIA_SUPERIO(isa_create_simple(s->isa_bus,
> >>> -                                               TYPE_VT8231_SUPERIO));
> >>> +    object_initialize_child(obj, "sio", &s->via_sio,
> >> TYPE_VT8231_SUPERIO);
> >>> }
> >>>
> >>> static void vt8231_class_init(ObjectClass *klass, void *data)
> >>> @@ -717,7 +719,7 @@ static void vt8231_class_init(ObjectClass *klass,
> >> void *data)
> >>>     DeviceClass *dc = DEVICE_CLASS(klass);
> >>>     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>>
> >>> -    k->realize = vt8231_realize;
> >>> +    k->realize = via_isa_realize;
> >>>     k->config_write = vt8231_write_config;
> >>>     k->vendor_id = PCI_VENDOR_ID_VIA;
> >>>     k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
> >>> @@ -734,6 +736,7 @@ static const TypeInfo vt8231_isa_info = {
> >>>     .name          = TYPE_VT8231_ISA,
> >>>     .parent        = TYPE_VIA_ISA,
> >>>     .instance_size = sizeof(ViaISAState),
> >>> +    .instance_init = vt8231_init,
> >>>     .class_init    = vt8231_class_init,
> >>> };
> >>>
> >>>
> >>
> >
>

[-- Attachment #2: Type: text/html, Size: 10745 bytes --]

[PATCH 2/9] hw/isa/vt82c686: Resolve unneeded attribute

[Bernhard Beschow]

Now that also the super io device is realized in the common realize method,
the isa_bus attribute can be turned into a temporary.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 0217c98fe4..9d12e1cae4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -543,7 +543,6 @@ struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs;
-    ISABus *isa_bus;
     ViaSuperIOState via_sio;
 };
 
@@ -585,17 +584,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
     qemu_irq *isa_irq;
+    ISABus *isa_bus;
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-    s->isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
-    s->isa_irqs = i8259_init(s->isa_bus, *isa_irq);
-    isa_bus_irqs(s->isa_bus, s->isa_irqs);
-    i8254_pit_init(s->isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(s->isa_bus, 0);
-    mc146818_rtc_init(s->isa_bus, 2000, NULL);
+    s->isa_irqs = i8259_init(isa_bus, *isa_irq);
+    isa_bus_irqs(isa_bus, s->isa_irqs);
+    i8254_pit_init(isa_bus, 0x40, 0, NULL);
+    i8257_dma_init(isa_bus, 0);
+    mc146818_rtc_init(isa_bus, 2000, NULL);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
@@ -604,7 +604,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     }
 
     /* Super I/O */
-    if (!qdev_realize(DEVICE(&s->via_sio), BUS(s->isa_bus), errp)) {
+    if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
         return;
     }
 }
-- 
2.37.2

[PATCH 3/9] hw/isa/vt82c686: Prefer pci_address_space() over get_system_memory()

[Bernhard Beschow]

Unlike get_system_memory(), pci_address_space() respects the memory tree
available to the parent device.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9d12e1cae4..5582c0b179 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -589,7 +589,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
-    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+    isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           &error_fatal);
     s->isa_irqs = i8259_init(isa_bus, *isa_irq);
     isa_bus_irqs(isa_bus, s->isa_irqs);
-- 
2.37.2

[PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[Bernhard Beschow]

The IDE function is closely tied to the ISA function (e.g. the IDE
interrupt routing happens there), so it makes sense that the IDE
function is instantiated within the southbridge itself. As a side effect,
duplicated code in the boards is resolved.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 configs/devices/mips64el-softmmu/default.mak |  1 -
 hw/isa/Kconfig                               |  1 +
 hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
 hw/mips/fuloong2e.c                          |  3 ---
 hw/ppc/Kconfig                               |  1 -
 hw/ppc/pegasos2.c                            |  4 ----
 6 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/configs/devices/mips64el-softmmu/default.mak b/configs/devices/mips64el-softmmu/default.mak
index c610749ac1..d5188f7ea5 100644
--- a/configs/devices/mips64el-softmmu/default.mak
+++ b/configs/devices/mips64el-softmmu/default.mak
@@ -1,7 +1,6 @@
 # Default configuration for mips64el-softmmu
 
 include ../mips-softmmu/common.mak
-CONFIG_IDE_VIA=y
 CONFIG_FULOONG=y
 CONFIG_LOONGSON3V=y
 CONFIG_ATI_VGA=y
diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index d42143a991..20de7e9294 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -53,6 +53,7 @@ config VT82C686
     select I8254
     select I8257
     select I8259
+    select IDE_VIA
     select MC146818RTC
     select PARALLEL
 
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 5582c0b179..37d9ed635d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -17,6 +17,7 @@
 #include "hw/isa/vt82c686.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
+#include "hw/ide/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/isa/superio.h"
 #include "hw/intc/i8259.h"
@@ -544,6 +545,7 @@ struct ViaISAState {
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs;
     ViaSuperIOState via_sio;
+    PCIIDEState ide;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
     }
 };
 
+static void via_isa_init(Object *obj)
+{
+    ViaISAState *s = VIA_ISA(obj);
+
+    object_initialize_child(obj, "ide", &s->ide, "via-ide");
+}
+
 static const TypeInfo via_isa_info = {
     .name          = TYPE_VIA_ISA,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(ViaISAState),
+    .instance_init = via_isa_init,
     .abstract      = true,
     .interfaces    = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
+    PCIBus *pci_bus = pci_get_bus(d);
     qemu_irq *isa_irq;
     ISABus *isa_bus;
     int i;
@@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
         return;
     }
+
+    /* Function 1: IDE */
+    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
+    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
+        return;
+    }
+    pci_ide_create_devs(PCI_DEVICE(&s->ide));
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 5ee546f5f6..dae263c1e3 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                           TYPE_VT82C686B_ISA);
     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
-    pci_ide_create_devs(dev);
-
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
 
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 400511c6b7..18565e966b 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -74,7 +74,6 @@ config PEGASOS2
     bool
     select MV64361
     select VT82C686
-    select IDE_VIA
     select SMBUS_EEPROM
     select VOF
 # This should come with VT82C686
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 61f4263953..2f59d08ad1 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
     qdev_connect_gpio_out(DEVICE(dev), 0,
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
-    /* VT8231 function 1: IDE Controller */
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
-    pci_ide_create_devs(dev);
-
     /* VT8231 function 2-3: USB Ports */
     pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
-- 
2.37.2

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> The IDE function is closely tied to the ISA function (e.g. the IDE
> interrupt routing happens there), so it makes sense that the IDE
> function is instantiated within the southbridge itself. As a side effect,
> duplicated code in the boards is resolved.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> configs/devices/mips64el-softmmu/default.mak |  1 -
> hw/isa/Kconfig                               |  1 +
> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
> hw/mips/fuloong2e.c                          |  3 ---
> hw/ppc/Kconfig                               |  1 -
> hw/ppc/pegasos2.c                            |  4 ----
> 6 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/configs/devices/mips64el-softmmu/default.mak b/configs/devices/mips64el-softmmu/default.mak
> index c610749ac1..d5188f7ea5 100644
> --- a/configs/devices/mips64el-softmmu/default.mak
> +++ b/configs/devices/mips64el-softmmu/default.mak
> @@ -1,7 +1,6 @@
> # Default configuration for mips64el-softmmu
>
> include ../mips-softmmu/common.mak
> -CONFIG_IDE_VIA=y
> CONFIG_FULOONG=y
> CONFIG_LOONGSON3V=y
> CONFIG_ATI_VGA=y
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index d42143a991..20de7e9294 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -53,6 +53,7 @@ config VT82C686
>     select I8254
>     select I8257
>     select I8259
> +    select IDE_VIA
>     select MC146818RTC
>     select PARALLEL
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 5582c0b179..37d9ed635d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -17,6 +17,7 @@
> #include "hw/isa/vt82c686.h"
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> +#include "hw/ide/pci.h"
> #include "hw/isa/isa.h"
> #include "hw/isa/superio.h"
> #include "hw/intc/i8259.h"
> @@ -544,6 +545,7 @@ struct ViaISAState {
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs;
>     ViaSuperIOState via_sio;
> +    PCIIDEState ide;
> };
>
> static const VMStateDescription vmstate_via = {
> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>     }
> };
>
> +static void via_isa_init(Object *obj)
> +{
> +    ViaISAState *s = VIA_ISA(obj);
> +
> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
> +}
> +
> static const TypeInfo via_isa_info = {
>     .name          = TYPE_VIA_ISA,
>     .parent        = TYPE_PCI_DEVICE,
>     .instance_size = sizeof(ViaISAState),
> +    .instance_init = via_isa_init,
>     .abstract      = true,
>     .interfaces    = (InterfaceInfo[]) {
>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
> {
>     ViaISAState *s = VIA_ISA(d);
>     DeviceState *dev = DEVICE(d);
> +    PCIBus *pci_bus = pci_get_bus(d);
>     qemu_irq *isa_irq;
>     ISABus *isa_bus;
>     int i;
> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>         return;
>     }
> +
> +    /* Function 1: IDE */
> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
> +        return;
> +    }
> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));

I'm not sure about moving pci_ide_create_devs() here. This is usally 
called from board code and only piix4 seems to do this. Maybe that's wrong 
because if all IDE devices did this then one machine could not have more 
than one different ide devices (like having an on-board ide and adding a 
pci ide controoler with -device) so this probably belongs to the board 
code to add devices to its default ide controller only as this is machine 
specific. Unless I'm wrong in which case somebody will correct me.

Regards,
BALATON Zoltan

> }
>
> /* TYPE_VT82C686B_ISA */
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 5ee546f5f6..dae263c1e3 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>                                           TYPE_VT82C686B_ISA);
>     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>
> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> -    pci_ide_create_devs(dev);
> -
>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
>
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 400511c6b7..18565e966b 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -74,7 +74,6 @@ config PEGASOS2
>     bool
>     select MV64361
>     select VT82C686
> -    select IDE_VIA
>     select SMBUS_EEPROM
>     select VOF
> # This should come with VT82C686
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 61f4263953..2f59d08ad1 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
>     qdev_connect_gpio_out(DEVICE(dev), 0,
>                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
> -    /* VT8231 function 1: IDE Controller */
> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
> -    pci_ide_create_devs(dev);
> -
>     /* VT8231 function 2-3: USB Ports */
>     pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
>     pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
>

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 6500 bytes --]

On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > The IDE function is closely tied to the ISA function (e.g. the IDE
> > interrupt routing happens there), so it makes sense that the IDE
> > function is instantiated within the southbridge itself. As a side effect,
> > duplicated code in the boards is resolved.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > configs/devices/mips64el-softmmu/default.mak |  1 -
> > hw/isa/Kconfig                               |  1 +
> > hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
> > hw/mips/fuloong2e.c                          |  3 ---
> > hw/ppc/Kconfig                               |  1 -
> > hw/ppc/pegasos2.c                            |  4 ----
> > 6 files changed, 19 insertions(+), 9 deletions(-)
> >
> > diff --git a/configs/devices/mips64el-softmmu/default.mak
> b/configs/devices/mips64el-softmmu/default.mak
> > index c610749ac1..d5188f7ea5 100644
> > --- a/configs/devices/mips64el-softmmu/default.mak
> > +++ b/configs/devices/mips64el-softmmu/default.mak
> > @@ -1,7 +1,6 @@
> > # Default configuration for mips64el-softmmu
> >
> > include ../mips-softmmu/common.mak
> > -CONFIG_IDE_VIA=y
> > CONFIG_FULOONG=y
> > CONFIG_LOONGSON3V=y
> > CONFIG_ATI_VGA=y
> > diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> > index d42143a991..20de7e9294 100644
> > --- a/hw/isa/Kconfig
> > +++ b/hw/isa/Kconfig
> > @@ -53,6 +53,7 @@ config VT82C686
> >     select I8254
> >     select I8257
> >     select I8259
> > +    select IDE_VIA
> >     select MC146818RTC
> >     select PARALLEL
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 5582c0b179..37d9ed635d 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -17,6 +17,7 @@
> > #include "hw/isa/vt82c686.h"
> > #include "hw/pci/pci.h"
> > #include "hw/qdev-properties.h"
> > +#include "hw/ide/pci.h"
> > #include "hw/isa/isa.h"
> > #include "hw/isa/superio.h"
> > #include "hw/intc/i8259.h"
> > @@ -544,6 +545,7 @@ struct ViaISAState {
> >     qemu_irq cpu_intr;
> >     qemu_irq *isa_irqs;
> >     ViaSuperIOState via_sio;
> > +    PCIIDEState ide;
> > };
> >
> > static const VMStateDescription vmstate_via = {
> > @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
> >     }
> > };
> >
> > +static void via_isa_init(Object *obj)
> > +{
> > +    ViaISAState *s = VIA_ISA(obj);
> > +
> > +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
> > +}
> > +
> > static const TypeInfo via_isa_info = {
> >     .name          = TYPE_VIA_ISA,
> >     .parent        = TYPE_PCI_DEVICE,
> >     .instance_size = sizeof(ViaISAState),
> > +    .instance_init = via_isa_init,
> >     .abstract      = true,
> >     .interfaces    = (InterfaceInfo[]) {
> >         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
> > {
> >     ViaISAState *s = VIA_ISA(d);
> >     DeviceState *dev = DEVICE(d);
> > +    PCIBus *pci_bus = pci_get_bus(d);
> >     qemu_irq *isa_irq;
> >     ISABus *isa_bus;
> >     int i;
> > @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
> >     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
> >         return;
> >     }
> > +
> > +    /* Function 1: IDE */
> > +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
> > +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
> > +        return;
> > +    }
> > +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>
> I'm not sure about moving pci_ide_create_devs() here. This is usally
> called from board code and only piix4 seems to do this. Maybe that's wrong
> because if all IDE devices did this then one machine could not have more
> than one different ide devices (like having an on-board ide and adding a
> pci ide controoler with -device) so this probably belongs to the board
> code to add devices to its default ide controller only as this is machine
> specific. Unless I'm wrong in which case somebody will correct me.
>

Grepping the code it can be seen that it's always called right after
creating the IDE controllers. The only notable exception is the "sii3112"
device in the sam460ex board which is not emulated yet. Since the IDE
controllers are often created in board code this means
pci_ide_create_devs() is called there as well.

Grouping these calls together does make sense since it keeps the logic
together. Otherwise it could happen all too easily that code becomes
inconsistent such that pci_ide_create_devs() could be called without an IDE
controller actually being available. Right?

Best regards,
Bernhard

>
> Regards,
> BALATON Zoltan
>
> > }
> >
> > /* TYPE_VT82C686B_ISA */
> > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> > index 5ee546f5f6..dae263c1e3 100644
> > --- a/hw/mips/fuloong2e.c
> > +++ b/hw/mips/fuloong2e.c
> > @@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus
> *pci_bus, int slot, qemu_irq intc,
> >                                           TYPE_VT82C686B_ISA);
> >     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
> >
> > -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> > -    pci_ide_create_devs(dev);
> > -
> >     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
> >     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
> >
> > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> > index 400511c6b7..18565e966b 100644
> > --- a/hw/ppc/Kconfig
> > +++ b/hw/ppc/Kconfig
> > @@ -74,7 +74,6 @@ config PEGASOS2
> >     bool
> >     select MV64361
> >     select VT82C686
> > -    select IDE_VIA
> >     select SMBUS_EEPROM
> >     select VOF
> > # This should come with VT82C686
> > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > index 61f4263953..2f59d08ad1 100644
> > --- a/hw/ppc/pegasos2.c
> > +++ b/hw/ppc/pegasos2.c
> > @@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
> >     qdev_connect_gpio_out(DEVICE(dev), 0,
> >                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
> >
> > -    /* VT8231 function 1: IDE Controller */
> > -    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
> > -    pci_ide_create_devs(dev);
> > -
> >     /* VT8231 function 2-3: USB Ports */
> >     pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
> >     pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
> >
>

[-- Attachment #2: Type: text/html, Size: 8438 bytes --]

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BALATON Zoltan]

On Thu, 25 Aug 2022, Bernhard Beschow wrote:
> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> The IDE function is closely tied to the ISA function (e.g. the IDE
>>> interrupt routing happens there), so it makes sense that the IDE
>>> function is instantiated within the southbridge itself. As a side effect,
>>> duplicated code in the boards is resolved.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> configs/devices/mips64el-softmmu/default.mak |  1 -
>>> hw/isa/Kconfig                               |  1 +
>>> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
>>> hw/mips/fuloong2e.c                          |  3 ---
>>> hw/ppc/Kconfig                               |  1 -
>>> hw/ppc/pegasos2.c                            |  4 ----
>>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/configs/devices/mips64el-softmmu/default.mak
>> b/configs/devices/mips64el-softmmu/default.mak
>>> index c610749ac1..d5188f7ea5 100644
>>> --- a/configs/devices/mips64el-softmmu/default.mak
>>> +++ b/configs/devices/mips64el-softmmu/default.mak
>>> @@ -1,7 +1,6 @@
>>> # Default configuration for mips64el-softmmu
>>>
>>> include ../mips-softmmu/common.mak
>>> -CONFIG_IDE_VIA=y
>>> CONFIG_FULOONG=y
>>> CONFIG_LOONGSON3V=y
>>> CONFIG_ATI_VGA=y
>>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>>> index d42143a991..20de7e9294 100644
>>> --- a/hw/isa/Kconfig
>>> +++ b/hw/isa/Kconfig
>>> @@ -53,6 +53,7 @@ config VT82C686
>>>     select I8254
>>>     select I8257
>>>     select I8259
>>> +    select IDE_VIA
>>>     select MC146818RTC
>>>     select PARALLEL
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 5582c0b179..37d9ed635d 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -17,6 +17,7 @@
>>> #include "hw/isa/vt82c686.h"
>>> #include "hw/pci/pci.h"
>>> #include "hw/qdev-properties.h"
>>> +#include "hw/ide/pci.h"
>>> #include "hw/isa/isa.h"
>>> #include "hw/isa/superio.h"
>>> #include "hw/intc/i8259.h"
>>> @@ -544,6 +545,7 @@ struct ViaISAState {
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa_irqs;
>>>     ViaSuperIOState via_sio;
>>> +    PCIIDEState ide;
>>> };
>>>
>>> static const VMStateDescription vmstate_via = {
>>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>>>     }
>>> };
>>>
>>> +static void via_isa_init(Object *obj)
>>> +{
>>> +    ViaISAState *s = VIA_ISA(obj);
>>> +
>>> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>> +}
>>> +
>>> static const TypeInfo via_isa_info = {
>>>     .name          = TYPE_VIA_ISA,
>>>     .parent        = TYPE_PCI_DEVICE,
>>>     .instance_size = sizeof(ViaISAState),
>>> +    .instance_init = via_isa_init,
>>>     .abstract      = true,
>>>     .interfaces    = (InterfaceInfo[]) {
>>>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>> {
>>>     ViaISAState *s = VIA_ISA(d);
>>>     DeviceState *dev = DEVICE(d);
>>> +    PCIBus *pci_bus = pci_get_bus(d);
>>>     qemu_irq *isa_irq;
>>>     ISABus *isa_bus;
>>>     int i;
>>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>>>         return;
>>>     }
>>> +
>>> +    /* Function 1: IDE */
>>> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
>>> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>>> +        return;
>>> +    }
>>> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>>
>> I'm not sure about moving pci_ide_create_devs() here. This is usally
>> called from board code and only piix4 seems to do this. Maybe that's wrong
>> because if all IDE devices did this then one machine could not have more
>> than one different ide devices (like having an on-board ide and adding a
>> pci ide controoler with -device) so this probably belongs to the board
>> code to add devices to its default ide controller only as this is machine
>> specific. Unless I'm wrong in which case somebody will correct me.
>>
>
> Grepping the code it can be seen that it's always called right after
> creating the IDE controllers. The only notable exception is the "sii3112"
> device in the sam460ex board which is not emulated yet. Since the IDE

The problem with sii3112 is that it only has 2 channels becuase I did not 
bother to implement more so pci_ide_create_devs() probably would not work 
as it assumes 4 channels. AFAIK this means that the short -hda, -cdrom, 
etc. convenience options don't work with sam460ex but yhou have to use the 
long way of creating ide-hd and ide-cd devices on the command line. I 
think there's a version of this controller with 4 channels, maybe called 
sii3114 or similar and it would be easy to enhance the current model for 
that but I haven't done that. What's not emulated on sam460ex is the 
on-board SATA ports of the SoC because it's too complex and all guest OSes 
have sii31xx drivers so it was simpler to implement that instead. The 
network port is the same as we already have working PCI network cards so 
I did not try to implement the 460EX network ports.

> controllers are often created in board code this means
> pci_ide_create_devs() is called there as well.
>
> Grouping these calls together does make sense since it keeps the logic
> together. Otherwise it could happen all too easily that code becomes
> inconsistent such that pci_ide_create_devs() could be called without an IDE
> controller actually being available. Right?

I don't know for sure but I think you cannot assign the devices to more 
than one controller and if this was called by every ide model then adding 
two of such ide controllers would call pci_ide_create_devs() twice which I 
don't think is correct and may cause problems. So I think it belongs to 
the board code even if the ide controller is created within another device 
instantiated by the board so it's only called by once.

Regards,
BALATON Zoltan

>>> }
>>>
>>> /* TYPE_VT82C686B_ISA */
>>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>>> index 5ee546f5f6..dae263c1e3 100644
>>> --- a/hw/mips/fuloong2e.c
>>> +++ b/hw/mips/fuloong2e.c
>>> @@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus
>> *pci_bus, int slot, qemu_irq intc,
>>>                                           TYPE_VT82C686B_ISA);
>>>     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>>>
>>> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>>> -    pci_ide_create_devs(dev);
>>> -
>>>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>>>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
>>>
>>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>>> index 400511c6b7..18565e966b 100644
>>> --- a/hw/ppc/Kconfig
>>> +++ b/hw/ppc/Kconfig
>>> @@ -74,7 +74,6 @@ config PEGASOS2
>>>     bool
>>>     select MV64361
>>>     select VT82C686
>>> -    select IDE_VIA
>>>     select SMBUS_EEPROM
>>>     select VOF
>>> # This should come with VT82C686
>>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>>> index 61f4263953..2f59d08ad1 100644
>>> --- a/hw/ppc/pegasos2.c
>>> +++ b/hw/ppc/pegasos2.c
>>> @@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
>>>     qdev_connect_gpio_out(DEVICE(dev), 0,
>>>                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>>>
>>> -    /* VT8231 function 1: IDE Controller */
>>> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
>>> -    pci_ide_create_devs(dev);
>>> -
>>>     /* VT8231 function 2-3: USB Ports */
>>>     pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
>>>     pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
>>>
>>
>

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BB]

Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> The IDE function is closely tied to the ISA function (e.g. the IDE
>>>> interrupt routing happens there), so it makes sense that the IDE
>>>> function is instantiated within the southbridge itself. As a side effect,
>>>> duplicated code in the boards is resolved.
>>>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> configs/devices/mips64el-softmmu/default.mak |  1 -
>>>> hw/isa/Kconfig                               |  1 +
>>>> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
>>>> hw/mips/fuloong2e.c                          |  3 ---
>>>> hw/ppc/Kconfig                               |  1 -
>>>> hw/ppc/pegasos2.c                            |  4 ----
>>>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>>> 
>>>> diff --git a/configs/devices/mips64el-softmmu/default.mak
>>> b/configs/devices/mips64el-softmmu/default.mak
>>>> index c610749ac1..d5188f7ea5 100644
>>>> --- a/configs/devices/mips64el-softmmu/default.mak
>>>> +++ b/configs/devices/mips64el-softmmu/default.mak
>>>> @@ -1,7 +1,6 @@
>>>> # Default configuration for mips64el-softmmu
>>>> 
>>>> include ../mips-softmmu/common.mak
>>>> -CONFIG_IDE_VIA=y
>>>> CONFIG_FULOONG=y
>>>> CONFIG_LOONGSON3V=y
>>>> CONFIG_ATI_VGA=y
>>>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>>>> index d42143a991..20de7e9294 100644
>>>> --- a/hw/isa/Kconfig
>>>> +++ b/hw/isa/Kconfig
>>>> @@ -53,6 +53,7 @@ config VT82C686
>>>>     select I8254
>>>>     select I8257
>>>>     select I8259
>>>> +    select IDE_VIA
>>>>     select MC146818RTC
>>>>     select PARALLEL
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 5582c0b179..37d9ed635d 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -17,6 +17,7 @@
>>>> #include "hw/isa/vt82c686.h"
>>>> #include "hw/pci/pci.h"
>>>> #include "hw/qdev-properties.h"
>>>> +#include "hw/ide/pci.h"
>>>> #include "hw/isa/isa.h"
>>>> #include "hw/isa/superio.h"
>>>> #include "hw/intc/i8259.h"
>>>> @@ -544,6 +545,7 @@ struct ViaISAState {
>>>>     qemu_irq cpu_intr;
>>>>     qemu_irq *isa_irqs;
>>>>     ViaSuperIOState via_sio;
>>>> +    PCIIDEState ide;
>>>> };
>>>> 
>>>> static const VMStateDescription vmstate_via = {
>>>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>>>>     }
>>>> };
>>>> 
>>>> +static void via_isa_init(Object *obj)
>>>> +{
>>>> +    ViaISAState *s = VIA_ISA(obj);
>>>> +
>>>> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>> +}
>>>> +
>>>> static const TypeInfo via_isa_info = {
>>>>     .name          = TYPE_VIA_ISA,
>>>>     .parent        = TYPE_PCI_DEVICE,
>>>>     .instance_size = sizeof(ViaISAState),
>>>> +    .instance_init = via_isa_init,
>>>>     .abstract      = true,
>>>>     .interfaces    = (InterfaceInfo[]) {
>>>>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
>>> **errp)
>>>> {
>>>>     ViaISAState *s = VIA_ISA(d);
>>>>     DeviceState *dev = DEVICE(d);
>>>> +    PCIBus *pci_bus = pci_get_bus(d);
>>>>     qemu_irq *isa_irq;
>>>>     ISABus *isa_bus;
>>>>     int i;
>>>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
>>> **errp)
>>>>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>>>>         return;
>>>>     }
>>>> +
>>>> +    /* Function 1: IDE */
>>>> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
>>>> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>>>> +        return;
>>>> +    }
>>>> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>>> 
>>> I'm not sure about moving pci_ide_create_devs() here. This is usally
>>> called from board code and only piix4 seems to do this. Maybe that's wrong
>>> because if all IDE devices did this then one machine could not have more
>>> than one different ide devices (like having an on-board ide and adding a
>>> pci ide controoler with -device) so this probably belongs to the board
>>> code to add devices to its default ide controller only as this is machine
>>> specific. Unless I'm wrong in which case somebody will correct me.
>>> 
>> 
>> Grepping the code it can be seen that it's always called right after
>> creating the IDE controllers. The only notable exception is the "sii3112"
>> device in the sam460ex board which is not emulated yet. Since the IDE
>
>The problem with sii3112 is that it only has 2 channels becuase I did not bother to implement more so pci_ide_create_devs() probably would not work as it assumes 4 channels. AFAIK this means that the short -hda, -cdrom, etc. convenience options don't work with sam460ex but yhou have to use the long way of creating ide-hd and ide-cd devices on the command line. I think there's a version of this controller with 4 channels, maybe called sii3114 or similar and it would be easy to enhance the current model for that but I haven't done that. What's not emulated on sam460ex is the on-board SATA ports of the SoC because it's too complex and all guest OSes have sii31xx drivers so it was simpler to implement that instead. The network port is the same as we already have working PCI network cards so I did not try to implement the 460EX network ports.
>
>> controllers are often created in board code this means
>> pci_ide_create_devs() is called there as well.
>> 
>> Grouping these calls together does make sense since it keeps the logic
>> together. Otherwise it could happen all too easily that code becomes
>> inconsistent such that pci_ide_create_devs() could be called without an IDE
>> controller actually being available. Right?
>
>I don't know for sure but I think you cannot assign the devices to more than one controller and if this was called by every ide model then adding two of such ide controllers would call pci_ide_create_devs() twice which I don't think is correct and may cause problems.

This sounds reasonable.

> So I think it belongs to the board code even if the ide controller is created within another device instantiated by the board so it's only called by once.

pci_ide_create_devs() isn't called by the VIA IDE controller. Instead, it gets called by the VIA south bridges, of which there should only be one per board. Do you still see a risk of pci_ide_create_devs() being called multiple times? If so, I'd need to change the piix4 south bridge as well for consistency.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>>>> }
>>>> 
>>>> /* TYPE_VT82C686B_ISA */
>>>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>>>> index 5ee546f5f6..dae263c1e3 100644
>>>> --- a/hw/mips/fuloong2e.c
>>>> +++ b/hw/mips/fuloong2e.c
>>>> @@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus
>>> *pci_bus, int slot, qemu_irq intc,
>>>>                                           TYPE_VT82C686B_ISA);
>>>>     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>>>> 
>>>> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>>>> -    pci_ide_create_devs(dev);
>>>> -
>>>>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>>>>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
>>>> 
>>>> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
>>>> index 400511c6b7..18565e966b 100644
>>>> --- a/hw/ppc/Kconfig
>>>> +++ b/hw/ppc/Kconfig
>>>> @@ -74,7 +74,6 @@ config PEGASOS2
>>>>     bool
>>>>     select MV64361
>>>>     select VT82C686
>>>> -    select IDE_VIA
>>>>     select SMBUS_EEPROM
>>>>     select VOF
>>>> # This should come with VT82C686
>>>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>>>> index 61f4263953..2f59d08ad1 100644
>>>> --- a/hw/ppc/pegasos2.c
>>>> +++ b/hw/ppc/pegasos2.c
>>>> @@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
>>>>     qdev_connect_gpio_out(DEVICE(dev), 0,
>>>>                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>>>> 
>>>> -    /* VT8231 function 1: IDE Controller */
>>>> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 1), "via-ide");
>>>> -    pci_ide_create_devs(dev);
>>>> -
>>>>     /* VT8231 function 2-3: USB Ports */
>>>>     pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
>>>>     pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
>>>> 
>>> 
>> 

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BALATON Zoltan]

On Mon, 29 Aug 2022, BB wrote:
> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>> The IDE function is closely tied to the ISA function (e.g. the IDE
>>>>> interrupt routing happens there), so it makes sense that the IDE
>>>>> function is instantiated within the southbridge itself. As a side effect,
>>>>> duplicated code in the boards is resolved.
>>>>>
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> configs/devices/mips64el-softmmu/default.mak |  1 -
>>>>> hw/isa/Kconfig                               |  1 +
>>>>> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
>>>>> hw/mips/fuloong2e.c                          |  3 ---
>>>>> hw/ppc/Kconfig                               |  1 -
>>>>> hw/ppc/pegasos2.c                            |  4 ----
>>>>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/configs/devices/mips64el-softmmu/default.mak
>>>> b/configs/devices/mips64el-softmmu/default.mak
>>>>> index c610749ac1..d5188f7ea5 100644
>>>>> --- a/configs/devices/mips64el-softmmu/default.mak
>>>>> +++ b/configs/devices/mips64el-softmmu/default.mak
>>>>> @@ -1,7 +1,6 @@
>>>>> # Default configuration for mips64el-softmmu
>>>>>
>>>>> include ../mips-softmmu/common.mak
>>>>> -CONFIG_IDE_VIA=y
>>>>> CONFIG_FULOONG=y
>>>>> CONFIG_LOONGSON3V=y
>>>>> CONFIG_ATI_VGA=y
>>>>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>>>>> index d42143a991..20de7e9294 100644
>>>>> --- a/hw/isa/Kconfig
>>>>> +++ b/hw/isa/Kconfig
>>>>> @@ -53,6 +53,7 @@ config VT82C686
>>>>>     select I8254
>>>>>     select I8257
>>>>>     select I8259
>>>>> +    select IDE_VIA
>>>>>     select MC146818RTC
>>>>>     select PARALLEL
>>>>>
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 5582c0b179..37d9ed635d 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -17,6 +17,7 @@
>>>>> #include "hw/isa/vt82c686.h"
>>>>> #include "hw/pci/pci.h"
>>>>> #include "hw/qdev-properties.h"
>>>>> +#include "hw/ide/pci.h"
>>>>> #include "hw/isa/isa.h"
>>>>> #include "hw/isa/superio.h"
>>>>> #include "hw/intc/i8259.h"
>>>>> @@ -544,6 +545,7 @@ struct ViaISAState {
>>>>>     qemu_irq cpu_intr;
>>>>>     qemu_irq *isa_irqs;
>>>>>     ViaSuperIOState via_sio;
>>>>> +    PCIIDEState ide;
>>>>> };
>>>>>
>>>>> static const VMStateDescription vmstate_via = {
>>>>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>>>>>     }
>>>>> };
>>>>>
>>>>> +static void via_isa_init(Object *obj)
>>>>> +{
>>>>> +    ViaISAState *s = VIA_ISA(obj);
>>>>> +
>>>>> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>> +}
>>>>> +
>>>>> static const TypeInfo via_isa_info = {
>>>>>     .name          = TYPE_VIA_ISA,
>>>>>     .parent        = TYPE_PCI_DEVICE,
>>>>>     .instance_size = sizeof(ViaISAState),
>>>>> +    .instance_init = via_isa_init,
>>>>>     .abstract      = true,
>>>>>     .interfaces    = (InterfaceInfo[]) {
>>>>>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>>>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
>>>> **errp)
>>>>> {
>>>>>     ViaISAState *s = VIA_ISA(d);
>>>>>     DeviceState *dev = DEVICE(d);
>>>>> +    PCIBus *pci_bus = pci_get_bus(d);
>>>>>     qemu_irq *isa_irq;
>>>>>     ISABus *isa_bus;
>>>>>     int i;
>>>>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
>>>> **errp)
>>>>>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>>>>>         return;
>>>>>     }
>>>>> +
>>>>> +    /* Function 1: IDE */
>>>>> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
>>>>> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>>>>> +        return;
>>>>> +    }
>>>>> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>>>>
>>>> I'm not sure about moving pci_ide_create_devs() here. This is usally
>>>> called from board code and only piix4 seems to do this. Maybe that's wrong
>>>> because if all IDE devices did this then one machine could not have more
>>>> than one different ide devices (like having an on-board ide and adding a
>>>> pci ide controoler with -device) so this probably belongs to the board
>>>> code to add devices to its default ide controller only as this is machine
>>>> specific. Unless I'm wrong in which case somebody will correct me.
>>>>
>>>
>>> Grepping the code it can be seen that it's always called right after
>>> creating the IDE controllers. The only notable exception is the "sii3112"
>>> device in the sam460ex board which is not emulated yet. Since the IDE
>>
>> The problem with sii3112 is that it only has 2 channels becuase I did not bother to implement more so pci_ide_create_devs() probably would not work as it assumes 4 channels. AFAIK this means that the short -hda, -cdrom, etc. convenience options don't work with sam460ex but yhou have to use the long way of creating ide-hd and ide-cd devices on the command line. I think there's a version of this controller with 4 channels, maybe called sii3114 or similar and it would be easy to enhance the current model for that but I haven't done that. What's not emulated on sam460ex is the on-board SATA ports of the SoC because it's too complex and all guest OSes have sii31xx drivers so it was simpler to implement that instead. The network port is the same as we already have working PCI network cards so I did not try to implement the 460EX network ports.
>>
>>> controllers are often created in board code this means
>>> pci_ide_create_devs() is called there as well.
>>>
>>> Grouping these calls together does make sense since it keeps the logic
>>> together. Otherwise it could happen all too easily that code becomes
>>> inconsistent such that pci_ide_create_devs() could be called without an IDE
>>> controller actually being available. Right?
>>
>> I don't know for sure but I think you cannot assign the devices to more than one controller and if this was called by every ide model then adding two of such ide controllers would call pci_ide_create_devs() twice which I don't think is correct and may cause problems.
>
> This sounds reasonable.
>
>> So I think it belongs to the board code even if the ide controller is created within another device instantiated by the board so it's only called by once.
>
> pci_ide_create_devs() isn't called by the VIA IDE controller. Instead, 
> it gets called by the VIA south bridges, of which there should only be 
> one per board. Do you still see a risk of pci_ide_create_devs() being 
> called multiple times? If so, I'd need to change the piix4 south bridge 
> as well for consistency.

Since the vt8231 is user_creatable = false there's probably no way to add 
a second one accidentally so in this case there's no direct risk. Yet for 
consistency I'd keep the call to pci_ide_create_devs() in board code as 
done by all other machines just to avoid any misunderstanding and keep it 
consistent accross the board(s) :-)

Regards,
BALATON Zoltan

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BB]

Am 29. August 2022 19:04:06 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 29 Aug 2022, BB wrote:
>> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>>>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>>> The IDE function is closely tied to the ISA function (e.g. the IDE
>>>>>> interrupt routing happens there), so it makes sense that the IDE
>>>>>> function is instantiated within the southbridge itself. As a side effect,
>>>>>> duplicated code in the boards is resolved.
>>>>>> 
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> configs/devices/mips64el-softmmu/default.mak |  1 -
>>>>>> hw/isa/Kconfig                               |  1 +
>>>>>> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
>>>>>> hw/mips/fuloong2e.c                          |  3 ---
>>>>>> hw/ppc/Kconfig                               |  1 -
>>>>>> hw/ppc/pegasos2.c                            |  4 ----
>>>>>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>>>>> 
>>>>>> diff --git a/configs/devices/mips64el-softmmu/default.mak
>>>>> b/configs/devices/mips64el-softmmu/default.mak
>>>>>> index c610749ac1..d5188f7ea5 100644
>>>>>> --- a/configs/devices/mips64el-softmmu/default.mak
>>>>>> +++ b/configs/devices/mips64el-softmmu/default.mak
>>>>>> @@ -1,7 +1,6 @@
>>>>>> # Default configuration for mips64el-softmmu
>>>>>> 
>>>>>> include ../mips-softmmu/common.mak
>>>>>> -CONFIG_IDE_VIA=y
>>>>>> CONFIG_FULOONG=y
>>>>>> CONFIG_LOONGSON3V=y
>>>>>> CONFIG_ATI_VGA=y
>>>>>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>>>>>> index d42143a991..20de7e9294 100644
>>>>>> --- a/hw/isa/Kconfig
>>>>>> +++ b/hw/isa/Kconfig
>>>>>> @@ -53,6 +53,7 @@ config VT82C686
>>>>>>     select I8254
>>>>>>     select I8257
>>>>>>     select I8259
>>>>>> +    select IDE_VIA
>>>>>>     select MC146818RTC
>>>>>>     select PARALLEL
>>>>>> 
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 5582c0b179..37d9ed635d 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>> #include "hw/isa/vt82c686.h"
>>>>>> #include "hw/pci/pci.h"
>>>>>> #include "hw/qdev-properties.h"
>>>>>> +#include "hw/ide/pci.h"
>>>>>> #include "hw/isa/isa.h"
>>>>>> #include "hw/isa/superio.h"
>>>>>> #include "hw/intc/i8259.h"
>>>>>> @@ -544,6 +545,7 @@ struct ViaISAState {
>>>>>>     qemu_irq cpu_intr;
>>>>>>     qemu_irq *isa_irqs;
>>>>>>     ViaSuperIOState via_sio;
>>>>>> +    PCIIDEState ide;
>>>>>> };
>>>>>> 
>>>>>> static const VMStateDescription vmstate_via = {
>>>>>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>>>>>>     }
>>>>>> };
>>>>>> 
>>>>>> +static void via_isa_init(Object *obj)
>>>>>> +{
>>>>>> +    ViaISAState *s = VIA_ISA(obj);
>>>>>> +
>>>>>> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>>> +}
>>>>>> +
>>>>>> static const TypeInfo via_isa_info = {
>>>>>>     .name          = TYPE_VIA_ISA,
>>>>>>     .parent        = TYPE_PCI_DEVICE,
>>>>>>     .instance_size = sizeof(ViaISAState),
>>>>>> +    .instance_init = via_isa_init,
>>>>>>     .abstract      = true,
>>>>>>     .interfaces    = (InterfaceInfo[]) {
>>>>>>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>>>>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>> **errp)
>>>>>> {
>>>>>>     ViaISAState *s = VIA_ISA(d);
>>>>>>     DeviceState *dev = DEVICE(d);
>>>>>> +    PCIBus *pci_bus = pci_get_bus(d);
>>>>>>     qemu_irq *isa_irq;
>>>>>>     ISABus *isa_bus;
>>>>>>     int i;
>>>>>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>> **errp)
>>>>>>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>>>>>>         return;
>>>>>>     }
>>>>>> +
>>>>>> +    /* Function 1: IDE */
>>>>>> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
>>>>>> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>>>>> 
>>>>> I'm not sure about moving pci_ide_create_devs() here. This is usally
>>>>> called from board code and only piix4 seems to do this. Maybe that's wrong
>>>>> because if all IDE devices did this then one machine could not have more
>>>>> than one different ide devices (like having an on-board ide and adding a
>>>>> pci ide controoler with -device) so this probably belongs to the board
>>>>> code to add devices to its default ide controller only as this is machine
>>>>> specific. Unless I'm wrong in which case somebody will correct me.
>>>>> 
>>>> 
>>>> Grepping the code it can be seen that it's always called right after
>>>> creating the IDE controllers. The only notable exception is the "sii3112"
>>>> device in the sam460ex board which is not emulated yet. Since the IDE
>>> 
>>> The problem with sii3112 is that it only has 2 channels becuase I did not bother to implement more so pci_ide_create_devs() probably would not work as it assumes 4 channels. AFAIK this means that the short -hda, -cdrom, etc. convenience options don't work with sam460ex but yhou have to use the long way of creating ide-hd and ide-cd devices on the command line. I think there's a version of this controller with 4 channels, maybe called sii3114 or similar and it would be easy to enhance the current model for that but I haven't done that. What's not emulated on sam460ex is the on-board SATA ports of the SoC because it's too complex and all guest OSes have sii31xx drivers so it was simpler to implement that instead. The network port is the same as we already have working PCI network cards so I did not try to implement the 460EX network ports.
>>> 
>>>> controllers are often created in board code this means
>>>> pci_ide_create_devs() is called there as well.
>>>> 
>>>> Grouping these calls together does make sense since it keeps the logic
>>>> together. Otherwise it could happen all too easily that code becomes
>>>> inconsistent such that pci_ide_create_devs() could be called without an IDE
>>>> controller actually being available. Right?
>>> 
>>> I don't know for sure but I think you cannot assign the devices to more than one controller and if this was called by every ide model then adding two of such ide controllers would call pci_ide_create_devs() twice which I don't think is correct and may cause problems.
>> 
>> This sounds reasonable.
>> 
>>> So I think it belongs to the board code even if the ide controller is created within another device instantiated by the board so it's only called by once.
>> 
>> pci_ide_create_devs() isn't called by the VIA IDE controller. Instead, it gets called by the VIA south bridges, of which there should only be one per board. Do you still see a risk of pci_ide_create_devs() being called multiple times? If so, I'd need to change the piix4 south bridge as well for consistency.
>
>Since the vt8231 is user_creatable = false there's probably no way to add a second one accidentally so in this case there's no direct risk. Yet for consistency I'd keep the call to pci_ide_create_devs() in board code as done by all other machines just to avoid any misunderstanding and keep it consistent accross the board(s) :-)

:-)

I'll add a patch for Malta/Piix4 to the series then.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan

Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation

[BB]

Am 29. August 2022 20:12:21 MESZ schrieb BB <shentey@gmail.com>:
>
>
>Am 29. August 2022 19:04:06 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>On Mon, 29 Aug 2022, BB wrote:
>>> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>>>>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>>>> The IDE function is closely tied to the ISA function (e.g. the IDE
>>>>>>> interrupt routing happens there), so it makes sense that the IDE
>>>>>>> function is instantiated within the southbridge itself. As a side effect,
>>>>>>> duplicated code in the boards is resolved.
>>>>>>> 
>>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>>> ---
>>>>>>> configs/devices/mips64el-softmmu/default.mak |  1 -
>>>>>>> hw/isa/Kconfig                               |  1 +
>>>>>>> hw/isa/vt82c686.c                            | 18 ++++++++++++++++++
>>>>>>> hw/mips/fuloong2e.c                          |  3 ---
>>>>>>> hw/ppc/Kconfig                               |  1 -
>>>>>>> hw/ppc/pegasos2.c                            |  4 ----
>>>>>>> 6 files changed, 19 insertions(+), 9 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/configs/devices/mips64el-softmmu/default.mak
>>>>>> b/configs/devices/mips64el-softmmu/default.mak
>>>>>>> index c610749ac1..d5188f7ea5 100644
>>>>>>> --- a/configs/devices/mips64el-softmmu/default.mak
>>>>>>> +++ b/configs/devices/mips64el-softmmu/default.mak
>>>>>>> @@ -1,7 +1,6 @@
>>>>>>> # Default configuration for mips64el-softmmu
>>>>>>> 
>>>>>>> include ../mips-softmmu/common.mak
>>>>>>> -CONFIG_IDE_VIA=y
>>>>>>> CONFIG_FULOONG=y
>>>>>>> CONFIG_LOONGSON3V=y
>>>>>>> CONFIG_ATI_VGA=y
>>>>>>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>>>>>>> index d42143a991..20de7e9294 100644
>>>>>>> --- a/hw/isa/Kconfig
>>>>>>> +++ b/hw/isa/Kconfig
>>>>>>> @@ -53,6 +53,7 @@ config VT82C686
>>>>>>>     select I8254
>>>>>>>     select I8257
>>>>>>>     select I8259
>>>>>>> +    select IDE_VIA
>>>>>>>     select MC146818RTC
>>>>>>>     select PARALLEL
>>>>>>> 
>>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>>> index 5582c0b179..37d9ed635d 100644
>>>>>>> --- a/hw/isa/vt82c686.c
>>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>>> @@ -17,6 +17,7 @@
>>>>>>> #include "hw/isa/vt82c686.h"
>>>>>>> #include "hw/pci/pci.h"
>>>>>>> #include "hw/qdev-properties.h"
>>>>>>> +#include "hw/ide/pci.h"
>>>>>>> #include "hw/isa/isa.h"
>>>>>>> #include "hw/isa/superio.h"
>>>>>>> #include "hw/intc/i8259.h"
>>>>>>> @@ -544,6 +545,7 @@ struct ViaISAState {
>>>>>>>     qemu_irq cpu_intr;
>>>>>>>     qemu_irq *isa_irqs;
>>>>>>>     ViaSuperIOState via_sio;
>>>>>>> +    PCIIDEState ide;
>>>>>>> };
>>>>>>> 
>>>>>>> static const VMStateDescription vmstate_via = {
>>>>>>> @@ -556,10 +558,18 @@ static const VMStateDescription vmstate_via = {
>>>>>>>     }
>>>>>>> };
>>>>>>> 
>>>>>>> +static void via_isa_init(Object *obj)
>>>>>>> +{
>>>>>>> +    ViaISAState *s = VIA_ISA(obj);
>>>>>>> +
>>>>>>> +    object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>>>> +}
>>>>>>> +
>>>>>>> static const TypeInfo via_isa_info = {
>>>>>>>     .name          = TYPE_VIA_ISA,
>>>>>>>     .parent        = TYPE_PCI_DEVICE,
>>>>>>>     .instance_size = sizeof(ViaISAState),
>>>>>>> +    .instance_init = via_isa_init,
>>>>>>>     .abstract      = true,
>>>>>>>     .interfaces    = (InterfaceInfo[]) {
>>>>>>>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>>>>>>> @@ -583,6 +593,7 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>>> **errp)
>>>>>>> {
>>>>>>>     ViaISAState *s = VIA_ISA(d);
>>>>>>>     DeviceState *dev = DEVICE(d);
>>>>>>> +    PCIBus *pci_bus = pci_get_bus(d);
>>>>>>>     qemu_irq *isa_irq;
>>>>>>>     ISABus *isa_bus;
>>>>>>>     int i;
>>>>>>> @@ -607,6 +618,13 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>>> **errp)
>>>>>>>     if (!qdev_realize(DEVICE(&s->via_sio), BUS(isa_bus), errp)) {
>>>>>>>         return;
>>>>>>>     }
>>>>>>> +
>>>>>>> +    /* Function 1: IDE */
>>>>>>> +    qdev_prop_set_int32(DEVICE(&s->ide), "addr", d->devfn + 1);
>>>>>>> +    if (!qdev_realize(DEVICE(&s->ide), BUS(pci_bus), errp)) {
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +    pci_ide_create_devs(PCI_DEVICE(&s->ide));
>>>>>> 
>>>>>> I'm not sure about moving pci_ide_create_devs() here. This is usally
>>>>>> called from board code and only piix4 seems to do this. Maybe that's wrong
>>>>>> because if all IDE devices did this then one machine could not have more
>>>>>> than one different ide devices (like having an on-board ide and adding a
>>>>>> pci ide controoler with -device) so this probably belongs to the board
>>>>>> code to add devices to its default ide controller only as this is machine
>>>>>> specific. Unless I'm wrong in which case somebody will correct me.
>>>>>> 
>>>>> 
>>>>> Grepping the code it can be seen that it's always called right after
>>>>> creating the IDE controllers. The only notable exception is the "sii3112"
>>>>> device in the sam460ex board which is not emulated yet. Since the IDE
>>>> 
>>>> The problem with sii3112 is that it only has 2 channels becuase I did not bother to implement more so pci_ide_create_devs() probably would not work as it assumes 4 channels. AFAIK this means that the short -hda, -cdrom, etc. convenience options don't work with sam460ex but yhou have to use the long way of creating ide-hd and ide-cd devices on the command line. I think there's a version of this controller with 4 channels, maybe called sii3114 or similar and it would be easy to enhance the current model for that but I haven't done that. What's not emulated on sam460ex is the on-board SATA ports of the SoC because it's too complex and all guest OSes have sii31xx drivers so it was simpler to implement that instead. The network port is the same as we already have working PCI network cards so I did not try to implement the 460EX network ports.
>>>> 
>>>>> controllers are often created in board code this means
>>>>> pci_ide_create_devs() is called there as well.
>>>>> 
>>>>> Grouping these calls together does make sense since it keeps the logic
>>>>> together. Otherwise it could happen all too easily that code becomes
>>>>> inconsistent such that pci_ide_create_devs() could be called without an IDE
>>>>> controller actually being available. Right?
>>>> 
>>>> I don't know for sure but I think you cannot assign the devices to more than one controller and if this was called by every ide model then adding two of such ide controllers would call pci_ide_create_devs() twice which I don't think is correct and may cause problems.
>>> 
>>> This sounds reasonable.
>>> 
>>>> So I think it belongs to the board code even if the ide controller is created within another device instantiated by the board so it's only called by once.
>>> 
>>> pci_ide_create_devs() isn't called by the VIA IDE controller. Instead, it gets called by the VIA south bridges, of which there should only be one per board. Do you still see a risk of pci_ide_create_devs() being called multiple times? If so, I'd need to change the piix4 south bridge as well for consistency.
>>
>>Since the vt8231 is user_creatable = false there's probably no way to add a second one accidentally so in this case there's no direct risk. Yet for consistency I'd keep the call to pci_ide_create_devs() in board code as done by all other machines just to avoid any misunderstanding and keep it consistent accross the board(s) :-)
>
>:-)
>
>I'll add a patch for Malta/Piix4 to the series then.

I'll actually add this patch to my piix consolidation series. Otherwise this patch would interfere with it.

>
>Best regards,
>Bernhard
>>
>>Regards,
>>BALATON Zoltan

[PATCH 5/9] hw/isa/vt82c686: QOM'ify vt82c686b-usb-uhci creation

[Bernhard Beschow]

Resolves duplicate code in the boards.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 12 ++++++++++++
 hw/mips/fuloong2e.c |  3 ---
 hw/ppc/pegasos2.c   |  4 ----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 37d9ed635d..c2f2e0039a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -23,6 +23,7 @@
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
 #include "hw/dma/i8257.h"
+#include "hw/usb/hcd-uhci.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "migration/vmstate.h"
@@ -546,6 +547,7 @@ struct ViaISAState {
     qemu_irq *isa_irqs;
     ViaSuperIOState via_sio;
     PCIIDEState ide;
+    UHCIState uhci[2];
 };
 
 static const VMStateDescription vmstate_via = {
@@ -563,6 +565,8 @@ static void via_isa_init(Object *obj)
     ViaISAState *s = VIA_ISA(obj);
 
     object_initialize_child(obj, "ide", &s->ide, "via-ide");
+    object_initialize_child(obj, "uhci1", &s->uhci[0], "vt82c686b-usb-uhci");
+    object_initialize_child(obj, "uhci2", &s->uhci[1], "vt82c686b-usb-uhci");
 }
 
 static const TypeInfo via_isa_info = {
@@ -625,6 +629,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
         return;
     }
     pci_ide_create_devs(PCI_DEVICE(&s->ide));
+
+    /* Functions 2-3: USB Ports */
+    for (i = 0; i < ARRAY_SIZE(s->uhci); ++i) {
+        qdev_prop_set_int32(DEVICE(&s->uhci[i]), "addr", d->devfn + 2 + i);
+        if (!qdev_realize(DEVICE(&s->uhci[i]), BUS(pci_bus), errp)) {
+            return;
+        }
+    }
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index dae263c1e3..c375107c53 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -205,9 +205,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                           TYPE_VT82C686B_ISA);
     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
-
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 2f59d08ad1..a1b851638a 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -165,10 +165,6 @@ static void pegasos2_init(MachineState *machine)
     qdev_connect_gpio_out(DEVICE(dev), 0,
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
-    /* VT8231 function 2-3: USB Ports */
-    pci_create_simple(pci_bus, PCI_DEVFN(12, 2), "vt82c686b-usb-uhci");
-    pci_create_simple(pci_bus, PCI_DEVFN(12, 3), "vt82c686b-usb-uhci");
-
     /* VT8231 function 4: Power Management Controller */
     dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
     i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-- 
2.37.2

[PATCH 6/9] hw/isa/vt82c686: QOM'ify pm creation

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 9 +++++++++
 hw/mips/fuloong2e.c | 2 +-
 hw/ppc/pegasos2.c   | 2 +-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c2f2e0039a..b964d1a760 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -548,6 +548,7 @@ struct ViaISAState {
     ViaSuperIOState via_sio;
     PCIIDEState ide;
     UHCIState uhci[2];
+    ViaPMState pm;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -637,6 +638,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
             return;
         }
     }
+
+    /* Function 4: Power Management */
+    qdev_prop_set_int32(DEVICE(&s->pm), "addr", d->devfn + 4);
+    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
+        return;
+    }
 }
 
 /* TYPE_VT82C686B_ISA */
@@ -679,6 +686,7 @@ static void vt82c686b_init(Object *obj)
     ViaISAState *s = VIA_ISA(obj);
 
     object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT82C686B_SUPERIO);
+    object_initialize_child(obj, "pm", &s->pm, TYPE_VT82C686B_PM);
 }
 
 static void vt82c686b_class_init(ObjectClass *klass, void *data)
@@ -742,6 +750,7 @@ static void vt8231_init(Object *obj)
     ViaISAState *s = VIA_ISA(obj);
 
     object_initialize_child(obj, "sio", &s->via_sio, TYPE_VT8231_SUPERIO);
+    object_initialize_child(obj, "pm", &s->pm, TYPE_VT8231_PM);
 }
 
 static void vt8231_class_init(ObjectClass *klass, void *data)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index c375107c53..f05474348f 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -205,7 +205,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
                                           TYPE_VT82C686B_ISA);
     qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
+    dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
     /* Audio support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index a1b851638a..4e29e42fba 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -166,7 +166,7 @@ static void pegasos2_init(MachineState *machine)
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
     /* VT8231 function 4: Power Management Controller */
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(12, 4), TYPE_VT8231_PM);
+    dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
     i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
     spd_data = spd_data_generate(DDR, machine->ram_size);
     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
-- 
2.37.2

[PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation

[Bernhard Beschow]

Resolves duplicate code in the boards.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 16 ++++++++++++++++
 hw/mips/fuloong2e.c |  4 ----
 hw/ppc/pegasos2.c   |  4 ----
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b964d1a760..47f2fd2669 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -549,6 +549,8 @@ struct ViaISAState {
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
+    PCIDevice ac97;
+    PCIDevice mc97;
 };
 
 static const VMStateDescription vmstate_via = {
@@ -568,6 +570,8 @@ static void via_isa_init(Object *obj)
     object_initialize_child(obj, "ide", &s->ide, "via-ide");
     object_initialize_child(obj, "uhci1", &s->uhci[0], "vt82c686b-usb-uhci");
     object_initialize_child(obj, "uhci2", &s->uhci[1], "vt82c686b-usb-uhci");
+    object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
+    object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
 }
 
 static const TypeInfo via_isa_info = {
@@ -644,6 +648,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
         return;
     }
+
+    /* Function 5: AC97 Audio */
+    qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
+    if (!qdev_realize(DEVICE(&s->ac97), BUS(pci_bus), errp)) {
+        return;
+    }
+
+    /* Function 6: AC97 Modem */
+    qdev_prop_set_int32(DEVICE(&s->mc97), "addr", d->devfn + 6);
+    if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
+        return;
+    }
 }
 
 /* TYPE_VT82C686B_ISA */
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index f05474348f..ea1aef3049 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -207,10 +207,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 
     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
-
-    /* Audio support */
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
-    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
 }
 
 /* Network support */
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 4e29e42fba..89ef4aed8b 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -171,10 +171,6 @@ static void pegasos2_init(MachineState *machine)
     spd_data = spd_data_generate(DDR, machine->ram_size);
     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
 
-    /* VT8231 function 5-6: AC97 Audio & Modem */
-    pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
-    pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
-
     /* other PC hardware */
     pci_vga_init(pci_bus);
 
-- 
2.37.2

Re: [PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> Resolves duplicate code in the boards.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c   | 16 ++++++++++++++++
> hw/mips/fuloong2e.c |  4 ----
> hw/ppc/pegasos2.c   |  4 ----
> 3 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index b964d1a760..47f2fd2669 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -549,6 +549,8 @@ struct ViaISAState {
>     PCIIDEState ide;
>     UHCIState uhci[2];
>     ViaPMState pm;
> +    PCIDevice ac97;
> +    PCIDevice mc97;
> };
>
> static const VMStateDescription vmstate_via = {
> @@ -568,6 +570,8 @@ static void via_isa_init(Object *obj)
>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>     object_initialize_child(obj, "uhci1", &s->uhci[0], "vt82c686b-usb-uhci");
>     object_initialize_child(obj, "uhci2", &s->uhci[1], "vt82c686b-usb-uhci");
> +    object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
> +    object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> }
>
> static const TypeInfo via_isa_info = {
> @@ -644,6 +648,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>         return;
>     }
> +
> +    /* Function 5: AC97 Audio */
> +    qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
> +    if (!qdev_realize(DEVICE(&s->ac97), BUS(pci_bus), errp)) {
> +        return;
> +    }
> +
> +    /* Function 6: AC97 Modem */
> +    qdev_prop_set_int32(DEVICE(&s->mc97), "addr", d->devfn + 6);
> +    if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
> +        return;
> +    }
> }
>
> /* TYPE_VT82C686B_ISA */
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index f05474348f..ea1aef3049 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -207,10 +207,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>
>     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
> -
> -    /* Audio support */
> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
> }
>
> /* Network support */
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 4e29e42fba..89ef4aed8b 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -171,10 +171,6 @@ static void pegasos2_init(MachineState *machine)
>     spd_data = spd_data_generate(DDR, machine->ram_size);
>     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
>
> -    /* VT8231 function 5-6: AC97 Audio & Modem */
> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
> -

This removes the last function created here so the comment above saying:
/* VT8231 function 0: PCI-to-ISA Bridge */
is now stale and may be removed as well.

Regards,
BALATON Zoltan

>     /* other PC hardware */
>     pci_vga_init(pci_bus);
>
>

Re: [PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

On Tue, Aug 23, 2022 at 2:44 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > Resolves duplicate code in the boards.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > hw/isa/vt82c686.c   | 16 ++++++++++++++++
> > hw/mips/fuloong2e.c |  4 ----
> > hw/ppc/pegasos2.c   |  4 ----
> > 3 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index b964d1a760..47f2fd2669 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -549,6 +549,8 @@ struct ViaISAState {
> >     PCIIDEState ide;
> >     UHCIState uhci[2];
> >     ViaPMState pm;
> > +    PCIDevice ac97;
> > +    PCIDevice mc97;
> > };
> >
> > static const VMStateDescription vmstate_via = {
> > @@ -568,6 +570,8 @@ static void via_isa_init(Object *obj)
> >     object_initialize_child(obj, "ide", &s->ide, "via-ide");
> >     object_initialize_child(obj, "uhci1", &s->uhci[0],
> "vt82c686b-usb-uhci");
> >     object_initialize_child(obj, "uhci2", &s->uhci[1],
> "vt82c686b-usb-uhci");
> > +    object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
> > +    object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> > }
> >
> > static const TypeInfo via_isa_info = {
> > @@ -644,6 +648,18 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
> >     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> >         return;
> >     }
> > +
> > +    /* Function 5: AC97 Audio */
> > +    qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
> > +    if (!qdev_realize(DEVICE(&s->ac97), BUS(pci_bus), errp)) {
> > +        return;
> > +    }
> > +
> > +    /* Function 6: AC97 Modem */
> > +    qdev_prop_set_int32(DEVICE(&s->mc97), "addr", d->devfn + 6);
> > +    if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
> > +        return;
> > +    }
> > }
> >
> > /* TYPE_VT82C686B_ISA */
> > diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> > index f05474348f..ea1aef3049 100644
> > --- a/hw/mips/fuloong2e.c
> > +++ b/hw/mips/fuloong2e.c
> > @@ -207,10 +207,6 @@ static void vt82c686b_southbridge_init(PCIBus
> *pci_bus, int slot, qemu_irq intc,
> >
> >     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
> >     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
> > -
> > -    /* Audio support */
> > -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
> > -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
> > }
> >
> > /* Network support */
> > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > index 4e29e42fba..89ef4aed8b 100644
> > --- a/hw/ppc/pegasos2.c
> > +++ b/hw/ppc/pegasos2.c
> > @@ -171,10 +171,6 @@ static void pegasos2_init(MachineState *machine)
> >     spd_data = spd_data_generate(DDR, machine->ram_size);
> >     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
> >
> > -    /* VT8231 function 5-6: AC97 Audio & Modem */
> > -    pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
> > -    pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
> > -
>
> This removes the last function created here so the comment above saying:
> /* VT8231 function 0: PCI-to-ISA Bridge */
> is now stale and may be removed as well.
>

Sure, I'll remove it in v2. What about the comment saying:
/* VT8231 function 4: Power Management Controller */
?

Thanks,
Bernhard

>
> Regards,
> BALATON Zoltan
>
> >     /* other PC hardware */
> >     pci_vga_init(pci_bus);
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 4918 bytes --]

Re: [PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> On Tue, Aug 23, 2022 at 2:44 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> Resolves duplicate code in the boards.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c   | 16 ++++++++++++++++
>>> hw/mips/fuloong2e.c |  4 ----
>>> hw/ppc/pegasos2.c   |  4 ----
>>> 3 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index b964d1a760..47f2fd2669 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -549,6 +549,8 @@ struct ViaISAState {
>>>     PCIIDEState ide;
>>>     UHCIState uhci[2];
>>>     ViaPMState pm;
>>> +    PCIDevice ac97;
>>> +    PCIDevice mc97;
>>> };
>>>
>>> static const VMStateDescription vmstate_via = {
>>> @@ -568,6 +570,8 @@ static void via_isa_init(Object *obj)
>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>> "vt82c686b-usb-uhci");
>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>> "vt82c686b-usb-uhci");
>>> +    object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
>>> +    object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
>>> }
>>>
>>> static const TypeInfo via_isa_info = {
>>> @@ -644,6 +648,18 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>>     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
>>>         return;
>>>     }
>>> +
>>> +    /* Function 5: AC97 Audio */
>>> +    qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
>>> +    if (!qdev_realize(DEVICE(&s->ac97), BUS(pci_bus), errp)) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* Function 6: AC97 Modem */
>>> +    qdev_prop_set_int32(DEVICE(&s->mc97), "addr", d->devfn + 6);
>>> +    if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
>>> +        return;
>>> +    }
>>> }
>>>
>>> /* TYPE_VT82C686B_ISA */
>>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>>> index f05474348f..ea1aef3049 100644
>>> --- a/hw/mips/fuloong2e.c
>>> +++ b/hw/mips/fuloong2e.c
>>> @@ -207,10 +207,6 @@ static void vt82c686b_southbridge_init(PCIBus
>> *pci_bus, int slot, qemu_irq intc,
>>>
>>>     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
>>>     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
>>> -
>>> -    /* Audio support */
>>> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
>>> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
>>> }
>>>
>>> /* Network support */
>>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>>> index 4e29e42fba..89ef4aed8b 100644
>>> --- a/hw/ppc/pegasos2.c
>>> +++ b/hw/ppc/pegasos2.c
>>> @@ -171,10 +171,6 @@ static void pegasos2_init(MachineState *machine)
>>>     spd_data = spd_data_generate(DDR, machine->ram_size);
>>>     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
>>>
>>> -    /* VT8231 function 5-6: AC97 Audio & Modem */
>>> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
>>> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
>>> -
>>
>> This removes the last function created here so the comment above saying:
>> /* VT8231 function 0: PCI-to-ISA Bridge */
>> is now stale and may be removed as well.
>>
>
> Sure, I'll remove it in v2. What about the comment saying:
> /* VT8231 function 4: Power Management Controller */
> ?

I thought that was removed by patch 6 but indeed it wasn't. I think that's 
now also stale and can be dropped (or replapced by something saying SPD 
EEPROM but the remaining code is fairly clear without a comment so jist 
removing it is fine).

Regards,
BALATON Zoltan

Re: [PATCH 7/9] hw/isa/vt82c686: QOM'ify ac97 and mc97 creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 4043 bytes --]

On Wed, Aug 24, 2022 at 12:54 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > On Tue, Aug 23, 2022 at 2:44 AM BALATON Zoltan <balaton@eik.bme.hu>
> wrote:
> >
> >> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> >>> Resolves duplicate code in the boards.
> >>>
> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>> ---
> >>> hw/isa/vt82c686.c   | 16 ++++++++++++++++
> >>> hw/mips/fuloong2e.c |  4 ----
> >>> hw/ppc/pegasos2.c   |  4 ----
> >>> 3 files changed, 16 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> >>> index b964d1a760..47f2fd2669 100644
> >>> --- a/hw/isa/vt82c686.c
> >>> +++ b/hw/isa/vt82c686.c
> >>> @@ -549,6 +549,8 @@ struct ViaISAState {
> >>>     PCIIDEState ide;
> >>>     UHCIState uhci[2];
> >>>     ViaPMState pm;
> >>> +    PCIDevice ac97;
> >>> +    PCIDevice mc97;
> >>> };
> >>>
> >>> static const VMStateDescription vmstate_via = {
> >>> @@ -568,6 +570,8 @@ static void via_isa_init(Object *obj)
> >>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
> >>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
> >> "vt82c686b-usb-uhci");
> >>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
> >> "vt82c686b-usb-uhci");
> >>> +    object_initialize_child(obj, "ac97", &s->ac97, TYPE_VIA_AC97);
> >>> +    object_initialize_child(obj, "mc97", &s->mc97, TYPE_VIA_MC97);
> >>> }
> >>>
> >>> static const TypeInfo via_isa_info = {
> >>> @@ -644,6 +648,18 @@ static void via_isa_realize(PCIDevice *d, Error
> >> **errp)
> >>>     if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> >>>         return;
> >>>     }
> >>> +
> >>> +    /* Function 5: AC97 Audio */
> >>> +    qdev_prop_set_int32(DEVICE(&s->ac97), "addr", d->devfn + 5);
> >>> +    if (!qdev_realize(DEVICE(&s->ac97), BUS(pci_bus), errp)) {
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    /* Function 6: AC97 Modem */
> >>> +    qdev_prop_set_int32(DEVICE(&s->mc97), "addr", d->devfn + 6);
> >>> +    if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
> >>> +        return;
> >>> +    }
> >>> }
> >>>
> >>> /* TYPE_VT82C686B_ISA */
> >>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> >>> index f05474348f..ea1aef3049 100644
> >>> --- a/hw/mips/fuloong2e.c
> >>> +++ b/hw/mips/fuloong2e.c
> >>> @@ -207,10 +207,6 @@ static void vt82c686b_southbridge_init(PCIBus
> >> *pci_bus, int slot, qemu_irq intc,
> >>>
> >>>     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
> >>>     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
> >>> -
> >>> -    /* Audio support */
> >>> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
> >>> -    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
> >>> }
> >>>
> >>> /* Network support */
> >>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> >>> index 4e29e42fba..89ef4aed8b 100644
> >>> --- a/hw/ppc/pegasos2.c
> >>> +++ b/hw/ppc/pegasos2.c
> >>> @@ -171,10 +171,6 @@ static void pegasos2_init(MachineState *machine)
> >>>     spd_data = spd_data_generate(DDR, machine->ram_size);
> >>>     smbus_eeprom_init_one(i2c_bus, 0x57, spd_data);
> >>>
> >>> -    /* VT8231 function 5-6: AC97 Audio & Modem */
> >>> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 5), TYPE_VIA_AC97);
> >>> -    pci_create_simple(pci_bus, PCI_DEVFN(12, 6), TYPE_VIA_MC97);
> >>> -
> >>
> >> This removes the last function created here so the comment above saying:
> >> /* VT8231 function 0: PCI-to-ISA Bridge */
> >> is now stale and may be removed as well.
> >>
> >
> > Sure, I'll remove it in v2. What about the comment saying:
> > /* VT8231 function 4: Power Management Controller */
> > ?
>
> I thought that was removed by patch 6 but indeed it wasn't. I think that's
> now also stale and can be dropped (or replapced by something saying SPD
> EEPROM but the remaining code is fairly clear without a comment so jist
> removing it is fine).
>

I'll omit it then.

Thanks,
Bernhard

>
> Regards,
> BALATON Zoltan
>

[-- Attachment #2: Type: text/html, Size: 6005 bytes --]

[PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 47f2fd2669..ee745d5d49 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -546,6 +546,7 @@ struct ViaISAState {
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs;
     ViaSuperIOState via_sio;
+    RTCState rtc;
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
@@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
 {
     ViaISAState *s = VIA_ISA(obj);
 
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
     object_initialize_child(obj, "ide", &s->ide, "via-ide");
     object_initialize_child(obj, "uhci1", &s->uhci[0], "vt82c686b-usb-uhci");
     object_initialize_child(obj, "uhci2", &s->uhci[1], "vt82c686b-usb-uhci");
@@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, s->isa_irqs);
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-    mc146818_rtc_init(isa_bus, 2000, NULL);
+
+    /* RTC */
+    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
+    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
+        return;
+    }
+    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(&s->rtc),
+                              "date");
+    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
-- 
2.37.2

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 47f2fd2669..ee745d5d49 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -546,6 +546,7 @@ struct ViaISAState {
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs;
>     ViaSuperIOState via_sio;
> +    RTCState rtc;
>     PCIIDEState ide;
>     UHCIState uhci[2];
>     ViaPMState pm;
> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
> {
>     ViaISAState *s = VIA_ISA(obj);
>
> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>     object_initialize_child(obj, "uhci1", &s->uhci[0], "vt82c686b-usb-uhci");
>     object_initialize_child(obj, "uhci2", &s->uhci[1], "vt82c686b-usb-uhci");
> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     isa_bus_irqs(isa_bus, s->isa_irqs);
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(isa_bus, 0);
> -    mc146818_rtc_init(isa_bus, 2000, NULL);
> +
> +    /* RTC */
> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
> +        return;
> +    }
> +    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(&s->rtc),
> +                              "date");
> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>
>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>

This actually introduces code duplication as all other places except piix4 
seem to still use the init function (probably to ensure that the rtc-rime 
alias on the machine is properly set) so I'd keep this the same as 
everything else and drop this patch until this init function is removed 
from all other places as well.

Regards,
BALATON Zoltan

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[Bernhard Beschow]

[-- Attachment #1: Type: text/plain, Size: 3484 bytes --]

On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > hw/isa/vt82c686.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 47f2fd2669..ee745d5d49 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -546,6 +546,7 @@ struct ViaISAState {
> >     qemu_irq cpu_intr;
> >     qemu_irq *isa_irqs;
> >     ViaSuperIOState via_sio;
> > +    RTCState rtc;
> >     PCIIDEState ide;
> >     UHCIState uhci[2];
> >     ViaPMState pm;
> > @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
> > {
> >     ViaISAState *s = VIA_ISA(obj);
> >
> > +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
> >     object_initialize_child(obj, "ide", &s->ide, "via-ide");
> >     object_initialize_child(obj, "uhci1", &s->uhci[0],
> "vt82c686b-usb-uhci");
> >     object_initialize_child(obj, "uhci2", &s->uhci[1],
> "vt82c686b-usb-uhci");
> > @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
> **errp)
> >     isa_bus_irqs(isa_bus, s->isa_irqs);
> >     i8254_pit_init(isa_bus, 0x40, 0, NULL);
> >     i8257_dma_init(isa_bus, 0);
> > -    mc146818_rtc_init(isa_bus, 2000, NULL);
> > +
> > +    /* RTC */
> > +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
> > +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
> > +        return;
> > +    }
> > +    object_property_add_alias(qdev_get_machine(), "rtc-time",
> OBJECT(&s->rtc),
> > +                              "date");
> > +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
> >
> >     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> >         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> >
>
> This actually introduces code duplication as all other places except piix4
> seem to still use the init function (probably to ensure that the rtc-rime
> alias on the machine is properly set) so I'd keep this the same as
> everything else and drop this patch until this init function is removed
> from all other places as well.
>

Hi Zoltan,

Thanks for the fast reply! Regarding code homogeneity and duplication I've
made a similar argument for mc146818_rtc_init() in the past [1] and I've
learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
as a candidate for the embed-the-device-struct style which - again
incidentally - I've now done.

The rtc-time alias is actually only used by a couple of PPC machines where
Pegasos II is one of them. So the alias actually needs to be created only
for these machines, and identifying the cases where it has to be preserved
requires a lot of careful investigation. In the Pegasos II case this seems
especially complicated since one needs to look through several layers of
devices. During my work on the VT82xx south bridges I've gained some
knowledge such that I'd like to make this simplifying contribution.

Our discussion makes me realize that the creation of the alias could now
actually be moved to the Pegasos II board. This way, the Pegasos II board
would both create and consume that alias, which seems to remove quite some
cognitive load. Do you agree? Would moving the alias to the board work for
you?

Thanks,
Bernhard

[1]
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220205175913.31738-2-shentey@gmail.com/

>
> Regards,
> BALATON Zoltan
>

[-- Attachment #2: Type: text/html, Size: 4782 bytes --]

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BALATON Zoltan]

On Tue, 23 Aug 2022, Bernhard Beschow wrote:
> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 47f2fd2669..ee745d5d49 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa_irqs;
>>>     ViaSuperIOState via_sio;
>>> +    RTCState rtc;
>>>     PCIIDEState ide;
>>>     UHCIState uhci[2];
>>>     ViaPMState pm;
>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>> {
>>>     ViaISAState *s = VIA_ISA(obj);
>>>
>>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>> "vt82c686b-usb-uhci");
>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>> "vt82c686b-usb-uhci");
>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>> **errp)
>>>     isa_bus_irqs(isa_bus, s->isa_irqs);
>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>     i8257_dma_init(isa_bus, 0);
>>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>>> +
>>> +    /* RTC */
>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>> +        return;
>>> +    }
>>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>> OBJECT(&s->rtc),
>>> +                              "date");
>>> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>
>>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>
>>
>> This actually introduces code duplication as all other places except piix4
>> seem to still use the init function (probably to ensure that the rtc-rime
>> alias on the machine is properly set) so I'd keep this the same as
>> everything else and drop this patch until this init function is removed
>> from all other places as well.
>>
>
> Hi Zoltan,
>
> Thanks for the fast reply! Regarding code homogeneity and duplication I've
> made a similar argument for mc146818_rtc_init() in the past [1] and I've
> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
> as a candidate for the embed-the-device-struct style which - again
> incidentally - I've now done.

I've seen patches embedding devices recently but in this case it looked 
not that simple because of the rtc-time alias.

> The rtc-time alias is actually only used by a couple of PPC machines where
> Pegasos II is one of them. So the alias actually needs to be created only
> for these machines, and identifying the cases where it has to be preserved
> requires a lot of careful investigation. In the Pegasos II case this seems
> especially complicated since one needs to look through several layers of
> devices. During my work on the VT82xx south bridges I've gained some
> knowledge such that I'd like to make this simplifying contribution.

I've used it to implement the get-time-of-day rtas call with VOF in 
pegasos2 because otherwise it would need to access internals of the RTC 
model and/or duplicate some code. Here's the message discussing this:

https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html

so this alias still seems to be the simplest way.

I think the primary function of this alias is not these ppc machines but 
some QMP/HMP command or to make the guest time available from the monitor 
or something like that so it's probably also used from there and therefore 
all rtc probably should have it but I'm not sure about it.

> Our discussion makes me realize that the creation of the alias could now
> actually be moved to the Pegasos II board. This way, the Pegasos II board
> would both create and consume that alias, which seems to remove quite some
> cognitive load. Do you agree? Would moving the alias to the board work for
> you?

Yes I think that would be better. This way the vt82xx and piix4 would be 
similar and the alias would also be clear within the pegasos2 code and it 
also has the machine directly at that point so it's clearer that way.

Regards,
BALATON Zoltan

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BB]

Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 47f2fd2669..ee745d5d49 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>>     qemu_irq cpu_intr;
>>>>     qemu_irq *isa_irqs;
>>>>     ViaSuperIOState via_sio;
>>>> +    RTCState rtc;
>>>>     PCIIDEState ide;
>>>>     UHCIState uhci[2];
>>>>     ViaPMState pm;
>>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>>> {
>>>>     ViaISAState *s = VIA_ISA(obj);
>>>> 
>>>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>>> "vt82c686b-usb-uhci");
>>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>>> "vt82c686b-usb-uhci");
>>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>>> **errp)
>>>>     isa_bus_irqs(isa_bus, s->isa_irqs);
>>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>     i8257_dma_init(isa_bus, 0);
>>>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>>>> +
>>>> +    /* RTC */
>>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>>> +        return;
>>>> +    }
>>>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>>> OBJECT(&s->rtc),
>>>> +                              "date");
>>>> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>> 
>>>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>> 
>>> 
>>> This actually introduces code duplication as all other places except piix4
>>> seem to still use the init function (probably to ensure that the rtc-rime
>>> alias on the machine is properly set) so I'd keep this the same as
>>> everything else and drop this patch until this init function is removed
>>> from all other places as well.
>>> 
>> 
>> Hi Zoltan,
>> 
>> Thanks for the fast reply! Regarding code homogeneity and duplication I've
>> made a similar argument for mc146818_rtc_init() in the past [1] and I've
>> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
>> as a candidate for the embed-the-device-struct style which - again
>> incidentally - I've now done.
>
>I've seen patches embedding devices recently but in this case it looked not that simple because of the rtc-time alias.
>
>> The rtc-time alias is actually only used by a couple of PPC machines where
>> Pegasos II is one of them. So the alias actually needs to be created only
>> for these machines, and identifying the cases where it has to be preserved
>> requires a lot of careful investigation. In the Pegasos II case this seems
>> especially complicated since one needs to look through several layers of
>> devices. During my work on the VT82xx south bridges I've gained some
>> knowledge such that I'd like to make this simplifying contribution.
>
>I've used it to implement the get-time-of-day rtas call with VOF in pegasos2 because otherwise it would need to access internals of the RTC model and/or duplicate some code. Here's the message discussing this:
>
>https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>
>so this alias still seems to be the simplest way.
>
>I think the primary function of this alias is not these ppc machines but some QMP/HMP command or to make the guest time available from the monitor or something like that so it's probably also used from there and therefore all rtc probably should have it but I'm not sure about it.

Indeed, the alias seems to be a convenience for some QMP/HMP commands. AFAICS only the mc146818 sets the alias while it is probably not the only RTC modelled in QEMU. So I wonder why boards using another RTC don't need it and whether removing the alias constitutes a compatibility break. 

>> Our discussion makes me realize that the creation of the alias could now
>> actually be moved to the Pegasos II board. This way, the Pegasos II board
>> would both create and consume that alias, which seems to remove quite some
>> cognitive load. Do you agree? Would moving the alias to the board work for
>> you?
>
>Yes I think that would be better. This way the vt82xx and piix4 would be similar and the alias would also be clear within the pegasos2 code and it also has the machine directly at that point so it's clearer that way.

All in all I wonder if we need to preserve the alias for the fuloong2e board?

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BALATON Zoltan]

On Mon, 29 Aug 2022, BB wrote:
> Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 47f2fd2669..ee745d5d49 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>>>     qemu_irq cpu_intr;
>>>>>     qemu_irq *isa_irqs;
>>>>>     ViaSuperIOState via_sio;
>>>>> +    RTCState rtc;
>>>>>     PCIIDEState ide;
>>>>>     UHCIState uhci[2];
>>>>>     ViaPMState pm;
>>>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>>>> {
>>>>>     ViaISAState *s = VIA_ISA(obj);
>>>>>
>>>>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>>>> "vt82c686b-usb-uhci");
>>>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>>>> "vt82c686b-usb-uhci");
>>>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>>>> **errp)
>>>>>     isa_bus_irqs(isa_bus, s->isa_irqs);
>>>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>>     i8257_dma_init(isa_bus, 0);
>>>>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>>>>> +
>>>>> +    /* RTC */
>>>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>>>> +        return;
>>>>> +    }
>>>>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>>>> OBJECT(&s->rtc),
>>>>> +                              "date");
>>>>> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>>>
>>>>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>>>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>>>
>>>>
>>>> This actually introduces code duplication as all other places except piix4
>>>> seem to still use the init function (probably to ensure that the rtc-rime
>>>> alias on the machine is properly set) so I'd keep this the same as
>>>> everything else and drop this patch until this init function is removed
>>>> from all other places as well.
>>>>
>>>
>>> Hi Zoltan,
>>>
>>> Thanks for the fast reply! Regarding code homogeneity and duplication I've
>>> made a similar argument for mc146818_rtc_init() in the past [1] and I've
>>> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
>>> as a candidate for the embed-the-device-struct style which - again
>>> incidentally - I've now done.
>>
>> I've seen patches embedding devices recently but in this case it looked not that simple because of the rtc-time alias.
>>
>>> The rtc-time alias is actually only used by a couple of PPC machines where
>>> Pegasos II is one of them. So the alias actually needs to be created only
>>> for these machines, and identifying the cases where it has to be preserved
>>> requires a lot of careful investigation. In the Pegasos II case this seems
>>> especially complicated since one needs to look through several layers of
>>> devices. During my work on the VT82xx south bridges I've gained some
>>> knowledge such that I'd like to make this simplifying contribution.
>>
>> I've used it to implement the get-time-of-day rtas call with VOF in 
>> pegasos2 because otherwise it would need to access internals of the RTC 
>> model and/or duplicate some code. Here's the message discussing this:
>>
>> https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>>
>> so this alias still seems to be the simplest way.
>>
>> I think the primary function of this alias is not these ppc machines 
>> but some QMP/HMP command or to make the guest time available from the 
>> monitor or something like that so it's probably also used from there 
>> and therefore all rtc probably should have it but I'm not sure about 
>> it.
>
> Indeed, the alias seems to be a convenience for some QMP/HMP commands. 
> AFAICS only the mc146818 sets the alias while it is probably not the 
> only RTC modelled in QEMU. So I wonder why boards using another RTC 
> don't need it and whether removing the alias constitutes a compatibility 
> break.
>
>>> Our discussion makes me realize that the creation of the alias could now
>>> actually be moved to the Pegasos II board. This way, the Pegasos II board
>>> would both create and consume that alias, which seems to remove quite some
>>> cognitive load. Do you agree? Would moving the alias to the board work for
>>> you?
>>
>> Yes I think that would be better. This way the vt82xx and piix4 would 
>> be similar and the alias would also be clear within the pegasos2 code 
>> and it also has the machine directly at that point so it's clearer that 
>> way.
>
> All in all I wonder if we need to preserve the alias for the fuloong2e board?

I don't know. A quick investigation shows that it seems to be added by 
commit 654a36d857ff94 which suggests something may use it (or was intended 
to use it back then, but not sure if things have changed in the meantime). 
I don't think any management app cares about fuloong2e but if this should 
be a generic thing then all machine may need it. Then it's also mentioned 
in commit 29551fdcf4d99 that suggests one ought to be careful moving this 
around as it may cause unexpected problems. But doing it from the machine 
init should be OK.

Regards,
BALATON Zoltan

Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation

[BB]

Am 29. August 2022 19:50:10 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 29 Aug 2022, BB wrote:
>> Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>>> ---
>>>>>> hw/isa/vt82c686.c | 12 +++++++++++-
>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 47f2fd2669..ee745d5d49 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -546,6 +546,7 @@ struct ViaISAState {
>>>>>>     qemu_irq cpu_intr;
>>>>>>     qemu_irq *isa_irqs;
>>>>>>     ViaSuperIOState via_sio;
>>>>>> +    RTCState rtc;
>>>>>>     PCIIDEState ide;
>>>>>>     UHCIState uhci[2];
>>>>>>     ViaPMState pm;
>>>>>> @@ -567,6 +568,7 @@ static void via_isa_init(Object *obj)
>>>>>> {
>>>>>>     ViaISAState *s = VIA_ISA(obj);
>>>>>> 
>>>>>> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>>>>>>     object_initialize_child(obj, "ide", &s->ide, "via-ide");
>>>>>>     object_initialize_child(obj, "uhci1", &s->uhci[0],
>>>>> "vt82c686b-usb-uhci");
>>>>>>     object_initialize_child(obj, "uhci2", &s->uhci[1],
>>>>> "vt82c686b-usb-uhci");
>>>>>> @@ -615,7 +617,15 @@ static void via_isa_realize(PCIDevice *d, Error
>>>>> **errp)
>>>>>>     isa_bus_irqs(isa_bus, s->isa_irqs);
>>>>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>>>     i8257_dma_init(isa_bus, 0);
>>>>>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>>>>>> +
>>>>>> +    /* RTC */
>>>>>> +    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>>>> +    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>>>>> OBJECT(&s->rtc),
>>>>>> +                              "date");
>>>>>> +    isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>>>>>> 
>>>>>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>>>>>         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>>>>>> 
>>>>> 
>>>>> This actually introduces code duplication as all other places except piix4
>>>>> seem to still use the init function (probably to ensure that the rtc-rime
>>>>> alias on the machine is properly set) so I'd keep this the same as
>>>>> everything else and drop this patch until this init function is removed
>>>>> from all other places as well.
>>>>> 
>>>> 
>>>> Hi Zoltan,
>>>> 
>>>> Thanks for the fast reply! Regarding code homogeneity and duplication I've
>>>> made a similar argument for mc146818_rtc_init() in the past [1] and I've
>>>> learnt that my patch went backwards. Incidentally, Peter mentioned vt686c
>>>> as a candidate for the embed-the-device-struct style which - again
>>>> incidentally - I've now done.
>>> 
>>> I've seen patches embedding devices recently but in this case it looked not that simple because of the rtc-time alias.
>>> 
>>>> The rtc-time alias is actually only used by a couple of PPC machines where
>>>> Pegasos II is one of them. So the alias actually needs to be created only
>>>> for these machines, and identifying the cases where it has to be preserved
>>>> requires a lot of careful investigation. In the Pegasos II case this seems
>>>> especially complicated since one needs to look through several layers of
>>>> devices. During my work on the VT82xx south bridges I've gained some
>>>> knowledge such that I'd like to make this simplifying contribution.
>>> 
>>> I've used it to implement the get-time-of-day rtas call with VOF in pegasos2 because otherwise it would need to access internals of the RTC model and/or duplicate some code. Here's the message discussing this:
>>> 
>>> https://lists.nongnu.org/archive/html/qemu-ppc/2021-10/msg00170.html
>>> 
>>> so this alias still seems to be the simplest way.
>>> 
>>> I think the primary function of this alias is not these ppc machines but some QMP/HMP command or to make the guest time available from the monitor or something like that so it's probably also used from there and therefore all rtc probably should have it but I'm not sure about it.
>> 
>> Indeed, the alias seems to be a convenience for some QMP/HMP commands. AFAICS only the mc146818 sets the alias while it is probably not the only RTC modelled in QEMU. So I wonder why boards using another RTC don't need it and whether removing the alias constitutes a compatibility break.
>> 
>>>> Our discussion makes me realize that the creation of the alias could now
>>>> actually be moved to the Pegasos II board. This way, the Pegasos II board
>>>> would both create and consume that alias, which seems to remove quite some
>>>> cognitive load. Do you agree? Would moving the alias to the board work for
>>>> you?
>>> 
>>> Yes I think that would be better. This way the vt82xx and piix4 would be similar and the alias would also be clear within the pegasos2 code and it also has the machine directly at that point so it's clearer that way.
>> 
>> All in all I wonder if we need to preserve the alias for the fuloong2e board?
>
>I don't know. A quick investigation shows that it seems to be added by commit 654a36d857ff94 which suggests something may use it (or was intended to use it back then, but not sure if things have changed in the meantime). I don't think any management app cares about fuloong2e but if this should be a generic thing then all machine may need it. Then it's also mentioned in commit 29551fdcf4d99 that suggests one ought to be careful moving this around as it may cause unexpected problems. But doing it from the machine init should be OK.

Then I'll create the alias in fuloong2e, too.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan

[PATCH 9/9] hw/isa/vt82c686: Reuse errp

[Bernhard Beschow]

Rather than terminating abruptly, make use of the already present errp and
propagate the error to the caller.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index ee745d5d49..15aa5b39f1 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -612,7 +612,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
-                          &error_fatal);
+                          errp);
+
+    if (!isa_bus) {
+        return;
+    }
+
     s->isa_irqs = i8259_init(isa_bus, *isa_irq);
     isa_bus_irqs(isa_bus, s->isa_irqs);
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
-- 
2.37.2