Re: [PATCH v3 06/10] hw/isa/vt82c686: Instantiate USB functions in host device
Am 31. August 2022 17:03:35 MESZ schrieb BALATON Zoltan :
>On Wed, 31 Aug 2022, BB wrote:
>> Am 31. August 2022 15:23:37 MESZ schrieb BALATON Zoltan :
>>> On Wed, 31 Aug 2022, Bernhard Beschow wrote:
>>>> The USB functions can be enabled/disabled through the ISA function. Also
>>>> its interrupt routing can be influenced there.
>>>>
>>>> Signed-off-by: Bernhard Beschow
>>>> ---
>>>> 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 9d946cea54..66a4b9c230 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", >ide, "via-ide");
>>>> +object_initialize_child(obj, "uhci1", >uhci[0],
>>>> "vt82c686b-usb-uhci");
>>>> +object_initialize_child(obj, "uhci2", >uhci[1],
>>>> "vt82c686b-usb-uhci");
>>>
>>> Sorry for not saying this yesterday, this can also be done separately so no
>>> need for another version of this series if not needed for another reason
>>> but could we add a define for vt82c686b-usb-uhci in
>>> include/hw/isa/vt82c686.h and use that here and in
>>> hw/usb/vt82c686-uhci-pci.c ?
>>
>> Would creating a dedicated header work, too? Board code doesn't need to see
>> the define any longer.
>
>I don't think it needs a separate header just for this so I'd put it in
>vt82c686.h but I don't mind either way.
Alright, I'll take the easy route for now. Splitting in dedicated headers (also
for the other devices) could be done in a separate series.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
Re: [PATCH v3 08/10] hw/isa/vt82c686: Instantiate AC97 and MC97 functions in host device
Am 31. August 2022 15:24:28 MESZ schrieb BALATON Zoltan :
>On Wed, 31 Aug 2022, Bernhard Beschow wrote:
>> The AC97 function's wakeup status is wired to the PM function and both
>> the AC97 and MC97 interrupt routing is determined by the ISA function.
>>
>> Signed-off-by: Bernhard Beschow
>> ---
>> hw/isa/vt82c686.c | 16
>> hw/mips/fuloong2e.c | 4
>> hw/ppc/pegasos2.c | 5 -
>> 3 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index fcc9894e8b..691a467b2c 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -553,6 +553,8 @@ struct ViaISAState {
>> PCIIDEState ide;
>> UHCIState uhci[2];
>> ViaPMState pm;
>> +PCIDevice ac97;
>> +PCIDevice mc97;
>> };
>>
>> static const VMStateDescription vmstate_via = {
>> @@ -572,6 +574,8 @@ static void via_isa_init(Object *obj)
>> object_initialize_child(obj, "ide", >ide, "via-ide");
>> object_initialize_child(obj, "uhci1", >uhci[0], "vt82c686b-usb-uhci");
>> object_initialize_child(obj, "uhci2", >uhci[1], "vt82c686b-usb-uhci");
>> +object_initialize_child(obj, "ac97", >ac97, TYPE_VIA_AC97);
>> +object_initialize_child(obj, "mc97", >mc97, TYPE_VIA_MC97);
>> }
>>
>> static const TypeInfo via_isa_info = {
>> @@ -652,6 +656,18 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>> if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
>> return;
>> }
>> +
>> +/* Function 5: AC97 Audio */
>> +qdev_prop_set_int32(DEVICE(>ac97), "addr", d->devfn + 5);
>> +if (!qdev_realize(DEVICE(>ac97), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> +
>> +/* Function 6: AC97 Modem */
>
>Is this MC97 Modem instead?
Yeah, MC97 Modem. I''ll send a v4.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +qdev_prop_set_int32(DEVICE(>mc97), "addr", d->devfn + 6);
>> +if (!qdev_realize(DEVICE(>mc97), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> }
>>
>> /* TYPE_VT82C686B_ISA */
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 377108d313..2d8723ab74 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -210,10 +210,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus,
>> int slot, qemu_irq intc,
>>
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "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 e32944ee2b..09fdb7557f 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -159,7 +159,6 @@ static void pegasos2_init(MachineState *machine)
>> pci_bus = mv64361_get_pci_bus(pm->mv, 1);
>>
>> /* VIA VT8231 South Bridge (multifunction PCI device) */
>> -/* VT8231 function 0: PCI-to-ISA Bridge */
>> via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
>> TYPE_VT8231_ISA);
>> qdev_connect_gpio_out(DEVICE(via), 0,
>> @@ -173,10 +172,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);
>>
>>
Re: [PATCH v3 06/10] hw/isa/vt82c686: Instantiate USB functions in host device
Am 31. August 2022 15:23:37 MESZ schrieb BALATON Zoltan :
>On Wed, 31 Aug 2022, Bernhard Beschow wrote:
>> The USB functions can be enabled/disabled through the ISA function. Also
>> its interrupt routing can be influenced there.
>>
>> Signed-off-by: Bernhard Beschow
>> ---
>> 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 9d946cea54..66a4b9c230 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", >ide, "via-ide");
>> +object_initialize_child(obj, "uhci1", >uhci[0],
>> "vt82c686b-usb-uhci");
>> +object_initialize_child(obj, "uhci2", >uhci[1],
>> "vt82c686b-usb-uhci");
>
>Sorry for not saying this yesterday, this can also be done separately so no
>need for another version of this series if not needed for another reason but
>could we add a define for vt82c686b-usb-uhci in include/hw/isa/vt82c686.h and
>use that here and in hw/usb/vt82c686-uhci-pci.c ?
Would creating a dedicated header work, too? Board code doesn't need to see the
define any longer.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> }
>>
>> static const TypeInfo via_isa_info = {
>> @@ -629,6 +633,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>> if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
>> return;
>> }
>> +
>> +/* Functions 2-3: USB Ports */
>> +for (i = 0; i < ARRAY_SIZE(s->uhci); i++) {
>> +qdev_prop_set_int32(DEVICE(>uhci[i]), "addr", d->devfn + 2 + i);
>> +if (!qdev_realize(DEVICE(>uhci[i]), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> +}
>> }
>>
>> /* TYPE_VT82C686B_ISA */
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 32605901e7..dc92223b76 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -208,9 +208,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus,
>> int slot, qemu_irq intc,
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(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");
>> -
>> 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 8bc528a560..85cca6f7a6 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -168,10 +168,6 @@ static void pegasos2_init(MachineState *machine)
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(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");
>> -
>> /* 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"));
>>
Re: [PATCH v3 05/10] hw/isa/vt82c686: Instantiate IDE function in host device
Am 31. August 2022 15:12:26 MESZ schrieb BALATON Zoltan :
>On Wed, 31 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 south bridge itself.
>>
>> Signed-off-by: Bernhard Beschow
>> ---
>> configs/devices/mips64el-softmmu/default.mak | 1 -
>> hw/isa/Kconfig | 1 +
>> hw/isa/vt82c686.c| 17 +
>> hw/mips/fuloong2e.c | 8
>> hw/ppc/Kconfig | 1 -
>> hw/ppc/pegasos2.c| 9 -
>> 6 files changed, 26 insertions(+), 11 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 37e37b3855..9d946cea54 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", >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,
>
>Did you verify this is actually called? I guess you could add a debug printf
>in the init method above or check the output of info qom-tree to see if ide
>apears below via-isa. I'm not sure because I think QOM does not call
>superclass methods if you override them, that's why the subclass realize
>methods called via_isa_realize before. In this case it may not cause a problem
>because ide-via does not have an init method so it will work with just realize
>called so the only effect may be that qom-tree is not like it should be. Or if
>this is called then I still don't get QOM.
We discussed the semantics of init() vs. realize() when discussing patch 1 in
v1 which consolidates realize() methods. My understanding is that init()
behaves like C++ constructors which are called implicitly parent first, child
next. OTOH realize() methods behave like virtual methods which get replaced by
the most specific one, i.e. one needs to call the parent implementation
explicitly.
Anyway, via_isa_init() must be called, otherwise the realize() method would
abort due to trying to realize the non-initialized ide attribute.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> .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;
>> @@ -612,6 +623,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>> if (!qdev_realize(DEVICE(>via_sio), BUS(isa_bus), errp)) {
>> return;
>> }
>> +
>> +/* Function 1: IDE */
>> +qdev_prop_set_int32(DEVICE(>ide), "addr", d->devfn + 1);
>> +if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> }
>>
>> /* TYPE_VT82C686B_ISA */
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 5ee546f5f6..32605901e7 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -199,13 +199,13 @@ static void main_cpu_reset(void *opaque)
>> static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq
>> intc,
>>I2CBus **i2c_bus)
>> {
>> -PCIDevice *dev;
>> +PCIDevice *dev, *via;
>>
>> -
Re: [PATCH v2 07/10] hw/isa/vt82c686: Instantiate PM function in host device
Am 31. August 2022 00:39:22 MESZ schrieb BALATON Zoltan :
>On Tue, 30 Aug 2022, Bernhard Beschow wrote:
>> The PM controller has activity bits which monitor activity of other
>> built-in devices in the host device.
>>
>> Signed-off-by: Bernhard Beschow
>> ---
>> hw/isa/vt82c686.c | 12
>> hw/mips/fuloong2e.c | 2 +-
>> hw/ppc/pegasos2.c | 3 +--
>> include/hw/isa/vt82c686.h | 2 --
>> 4 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 6aba7f29de..4e66570655 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -40,6 +40,9 @@
>> #define TYPE_VIA_PM "via-pm"
>> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>>
>> +#define TYPE_VT82C686B_PM "vt82c686b-pm"
>> +#define TYPE_VT8231_PM "vt8231-pm"
>
>These defines should be further down before vt82c686b_pm_init_info and
>vt8231_pm_init_info respectively to keep object class definitions together.
>Here the generic abstract superclass is defined, followed be the specific
>chips so it's too early to define these at this point.
Right. Fixed in v3.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +
>> struct ViaPMState {
>> PCIDevice dev;
>> MemoryRegion io;
>> @@ -548,6 +551,7 @@ struct ViaISAState {
>> ViaSuperIOState via_sio;
>> PCIIDEState ide;
>> UHCIState uhci[2];
>> +ViaPMState pm;
>> };
>>
>> static const VMStateDescription vmstate_via = {
>> @@ -641,6 +645,12 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>> return;
>> }
>> }
>> +
>> +/* Function 4: Power Management */
>> +qdev_prop_set_int32(DEVICE(>pm), "addr", d->devfn + 4);
>> +if (!qdev_realize(DEVICE(>pm), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> }
>>
>> /* TYPE_VT82C686B_ISA */
>> @@ -683,6 +693,7 @@ static void vt82c686b_init(Object *obj)
>> ViaISAState *s = VIA_ISA(obj);
>>
>> object_initialize_child(obj, "sio", >via_sio, TYPE_VT82C686B_SUPERIO);
>> +object_initialize_child(obj, "pm", >pm, TYPE_VT82C686B_PM);
>> }
>>
>> static void vt82c686b_class_init(ObjectClass *klass, void *data)
>> @@ -746,6 +757,7 @@ static void vt8231_init(Object *obj)
>> ViaISAState *s = VIA_ISA(obj);
>>
>> object_initialize_child(obj, "sio", >via_sio, TYPE_VT8231_SUPERIO);
>> +object_initialize_child(obj, "pm", >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 dc92223b76..377108d313 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -208,7 +208,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus,
>> int slot, qemu_irq intc,
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
>> pci_ide_create_devs(dev);
>>
>> -dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
>> +dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "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 85cca6f7a6..e32944ee2b 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -168,8 +168,7 @@ static void pegasos2_init(MachineState *machine)
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
>> pci_ide_create_devs(dev);
>>
>> -/* 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(via), "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);
>> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
>> index 56ac141be3..559f7c8926 100644
>> --- a/include/hw/isa/vt82c686.h
>> +++ b/include/hw/isa/vt82c686.h
>> @@ -4,9 +4,7 @@
>> #include "hw/pci/pci.h"
>>
>> #define TYPE_VT82C686B_ISA "vt82c686b-isa"
>> -#define TYPE_VT82C686B_PM "vt82c686b-pm"
>> #define TYPE_VT8231_ISA "vt8231-isa"
>> -#define TYPE_VT8231_PM "vt8231-pm"
>> #define TYPE_VIA_AC97 "via-ac97"
>> #define TYPE_VIA_MC97 "via-mc97"
>>
>>
Re: [PATCH v2 06/10] hw/isa/vt82c686: Instantiate USB functions in host device
Am 31. August 2022 00:33:33 MESZ schrieb BALATON Zoltan :
>On Tue, 30 Aug 2022, Bernhard Beschow wrote:
>> The USB functions can be enabled/disabled through the ISA function. Also
>> its interrupt routing can be influenced there.
>>
>> Signed-off-by: Bernhard Beschow
>> ---
>> 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 9d946cea54..6aba7f29de 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", >ide, "via-ide");
>> +object_initialize_child(obj, "uhci1", >uhci[0],
>> "vt82c686b-usb-uhci");
>> +object_initialize_child(obj, "uhci2", >uhci[1],
>> "vt82c686b-usb-uhci");
>> }
>>
>> static const TypeInfo via_isa_info = {
>> @@ -629,6 +633,14 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>> if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
>> return;
>> }
>> +
>> +/* Functions 2-3: USB Ports */
>> +for (i = 0; i < ARRAY_SIZE(s->uhci); ++i) {
>
>It does not really matter but Usually i++ is used in for loops so seeing ++i
>here is a bit odd but this alone isn't worth a new version.
Fixed in v3.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> +qdev_prop_set_int32(DEVICE(>uhci[i]), "addr", d->devfn + 2 + i);
>> +if (!qdev_realize(DEVICE(>uhci[i]), BUS(pci_bus), errp)) {
>> +return;
>> +}
>> +}
>> }
>>
>> /* TYPE_VT82C686B_ISA */
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 32605901e7..dc92223b76 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -208,9 +208,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus,
>> int slot, qemu_irq intc,
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(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");
>> -
>> 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 8bc528a560..85cca6f7a6 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -168,10 +168,6 @@ static void pegasos2_init(MachineState *machine)
>> dev = PCI_DEVICE(object_resolve_path_component(OBJECT(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");
>> -
>> /* 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"));
>>
Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation
Am 29. August 2022 20:12:21 MESZ schrieb BB :
>
>
>Am 29. August 2022 19:04:06 MESZ schrieb BALATON Zoltan :
>>On Mon, 29 Aug 2022, BB wrote:
>>> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan
>>> :
>>>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>>>>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan 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
>>>>>>> ---
>>>>>>> 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", >ide, "via-ide");
>>>>>>> +}
>>>>>>> +
>>>>>>> static const TypeInfo via_isa_info = {
>>>>>>> .name = TYPE_VIA_ISA,
>>>>>>> .parent= TYPE_PCI_DEVICE,
>>>>>>> .instance_size = siz
Re: [PATCH 4/9] hw/isa/vt82c686: QOM'ify via-ide creation
Am 29. August 2022 19:04:06 MESZ schrieb BALATON Zoltan :
>On Mon, 29 Aug 2022, BB wrote:
>> Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan :
>>> On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>>>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan 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
>>>>>> ---
>>>>>> 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", >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)
>>>>>> {
>
Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation
Am 29. August 2022 19:50:10 MESZ schrieb BALATON Zoltan :
>On Mon, 29 Aug 2022, BB wrote:
>> Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan :
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan wrote:
>>>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>>>>>> Signed-off-by: Bernhard Beschow
>>>>>> ---
>>>>>> 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", >rtc, TYPE_MC146818_RTC);
>>>>>> object_initialize_child(obj, "ide", >ide, "via-ide");
>>>>>> object_initialize_child(obj, "uhci1", >uhci[0],
>>>>> "vt82c686b-usb-uhci");
>>>>>> object_initialize_child(obj, "uhci2", >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(>rtc), "base_year", 2000);
>>>>>> +if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
>>>>>> +return;
>>>>>> +}
>>>>>> +object_property_add_alias(qdev_get_machine(), "rtc-time",
>>>>> OBJECT(>rtc),
>>>>>> + "date");
>>>>>> +isa_connect_gpio_out(ISA_DEVICE(>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 V
Re: [PATCH 8/9] hw/isa/vt82c686: QOM'ify RTC creation
Am 24. August 2022 01:23:14 MESZ schrieb BALATON Zoltan :
>On Tue, 23 Aug 2022, Bernhard Beschow wrote:
>> On Tue, Aug 23, 2022 at 2:20 AM BALATON Zoltan wrote:
>>> On Tue, 23 Aug 2022, Bernhard Beschow wrote:
Signed-off-by: Bernhard Beschow
---
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", >rtc, TYPE_MC146818_RTC);
object_initialize_child(obj, "ide", >ide, "via-ide");
object_initialize_child(obj, "uhci1", >uhci[0],
>>> "vt82c686b-usb-uhci");
object_initialize_child(obj, "uhci2", >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(>rtc), "base_year", 2000);
+if (!qdev_realize(DEVICE(>rtc), BUS(isa_bus), errp)) {
+return;
+}
+object_property_add_alias(qdev_get_machine(), "rtc-time",
>>> OBJECT(>rtc),
+ "date");
+isa_connect_gpio_out(ISA_DEVICE(>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 4/9] hw/isa/vt82c686: QOM'ify via-ide creation
Am 25. August 2022 01:18:56 MESZ schrieb BALATON Zoltan :
>On Thu, 25 Aug 2022, Bernhard Beschow wrote:
>> On Wed, Aug 24, 2022 at 3:54 PM BALATON Zoltan 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
---
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", >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(>via_sio), BUS(isa_bus), errp)) {
return;
}
+
+/* Function 1: IDE */
+qdev_prop_set_int32(DEVICE(>ide), "addr", d->devfn + 1);
+if (!qdev_realize(DEVICE(>ide), BUS(pci_bus), errp)) {
+return;
+}
+pci_ide_create_devs(PCI_DEVICE(>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
Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM
Am 8. August 2022 20:02:50 MESZ schrieb Peter Maydell : >On Mon, 8 Aug 2022 at 18:57, BB wrote: >> Am 8. August 2022 14:15:40 MESZ schrieb Igor Mammedov : >> >On Wed, 3 Aug 2022 19:26:30 +0200 >> >While refactoring we should keep migration stream compatible with older >> >QEMU versions (we must not regress widely x86 code path). Which might be >> >tricky in this case. >> >> Does this particular fix make future compatibility harder or easier or is it >> that hard already? IIUC it omits the hotplug bits in the vm state for Malta >> which is what one would expect there, right? > >This patch's fix only affects Malta. It is (I suspect but have >not tested) a migration compat break on Malta, but we don't >care about cross-version migration compat for that board anyway. >Migration compat matters (to a first approximation) only for >those boards which have versioned machine types (eg pc-7.0, >pc-7.1, etc). For all other machine types we retain compat >only if it's easy. I see. Thanks for the clarification! Best regards, Bernhard > >thanks >-- PMM
Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM
Am 8. August 2022 14:15:40 MESZ schrieb Igor Mammedov :
>On Wed, 3 Aug 2022 19:26:30 +0200
>Bernhard Beschow wrote:
>
>> On Tue, Aug 2, 2022 at 8:37 AM Philippe Mathieu-Daudé via <
>> qemu-devel@nongnu.org> wrote:
>>
>> > On 28/7/22 15:16, Igor Mammedov wrote:
>> > > On Thu, 28 Jul 2022 13:29:07 +0100
>> > > Peter Maydell wrote:
>> > >
>> > >> On Thu, 28 Jul 2022 at 12:50, Igor Mammedov
>> > wrote:
>> > >>>
>> > >>> QEMU crashes trying to save VMSTATE when only MIPS target are compiled
>> > >>>
>> > in
>> > >>>$ qemu-system-mips -monitor stdio
>> > >>>(qemu) migrate "exec:gzip -c > STATEFILE.gz"
>> > >>>Segmentation fault (core dumped)
>> > >>>
>> > >>> It happens due to PIIX4_PM trying to parse hotplug vmstate structures
>> > >>> which are valid only for x86 and not for MIPS (as it requires ACPI
>> > >>> tables support which is not existent for ithe later)
>> >
>> > We already discussed this Frankenstein PIIX4 problem 2 and 4 years ago:
>> >
>> > https://lore.kernel.org/qemu-devel/4d42697e-ba84-e5af-3a17-a2cc52cf0...@redhat.com/
>> >
>> > https://lore.kernel.org/qemu-devel/20190304210359-mutt-send-email-...@kernel.org/
>> >
>>
>>
>> Interesting reads!
>>
>>
>> > >>> Issue was probably exposed by trying to cleanup/compile out unused
>> > >>> ACPI bits from MIPS target (but forgetting about migration bits).
>> > >>>
>> > >>> Disable compiled out features using compat properties as the least
>> > >>> risky way to deal with issue.
>> >
>> > So now MIPS is forced to use meaningless compat[] to satisfy X86.
>> >
>> > Am I wrong seeing this as a dirty hack creeping in, yet another
>> > technical debt that will hit (me...) back in a close future?
>> >
>> > Are we sure there are no better solution (probably more time consuming
>> > and involving refactors) we could do instead?
>> >
>>
>> Working on the consolidation of piix3 and -4 soutbridges [1] I've stumbled
>> over certain design decisions where board/platform specific assumptions are
>> baked into the piix device models. I figure that's the core of the issue.
>>
>> In our case the ACPI functionality is implemented by inheritance while
>> perhaps it should be implemented using composition. With composition, the
>> ACPI functionality could be injected by the caller: The pc board would
>> inject it while the Malta board wouldn't. This would solve both the crash
>> and above design problem.
>
>While refactoring we should keep migration stream compatible with older
>QEMU versions (we must not regress widely x86 code path). Which might be
>tricky in this case.
Does this particular fix make future compatibility harder or easier or is it
that hard already? IIUC it omits the hotplug bits in the vm state for Malta
which is what one would expect there, right?
>Perhaps the best we could do is follow up on Philippe's idea to make
>PIIX4_PM frankenstein x86-specific (the least chance for regressions)
>and create/use clean version for anything else.
Having two implementations of the same device means that we'll end up having
duplicate code with board/platform-specific assumptions baked in. I guess what
Phil cares about is a sustainable solution without hacks that doesn't cause
bloat and/or regressions for MIPS, especially for features where MIPS doesn't
benefit from. I believe that composition could be such a solution.
My consolidation work could actually make PIIX4 an option for the PC machine.
This means that PIIX4_PM wouldn't be Frankenstein any more. This works already
on my branch - for both PC and Malta. Furthermore, it looks like it allowed
Malta to benefit more from KVM virtualization, but that's off-topic in this
discussion.
>> I'd be willing to implement it but can't make any promises about the time
>> frame since I'm currently doing this in my free time. Any hints regarding
>> the implementation would be welcome, though.
>>
>> Best regards,
>> Bernhard
>>
>> [1] https://github.com/shentok/qemu/commits/piix-consolidate
>>
>>
>> > Thanks,
>> >
>> > Phil.
>> >
>> > >>> Signed-off-by: Igor Mammedov
>> > >>
>> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/995
>> > >>
>> > >>> ---
>> > >>> PS:
>> > >>> another approach could be setting defaults to disabled state and
>> > >>> enabling them using compat props on PC machines (which is more
>> > >>> code to deal with => more risky) or continue with PIIX4_PM
>> > >>> refactoring to split x86-shism out (which I'm not really
>> > >>> interested in due to risk of regressions for not much of
>> > >>> benefit)
>> > >>> ---
>> > >>> hw/mips/malta.c | 9 +
>> > >>> 1 file changed, 9 insertions(+)
>> > >>>
>> > >>> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
>> > >>> index 7a0ec513b0..0e932988e0 100644
>> > >>> --- a/hw/mips/malta.c
>> > >>> +++ b/hw/mips/malta.c
>> > >>> @@ -1442,6 +1442,14 @@ static const TypeInfo mips_malta_device = {
>> > >>> .instance_init = mips_malta_instance_init,
>> > >>> };
>> > >>>
>> > >>>
Re: [PATCH for-7.1] hw/mips/malta: turn off x86 specific features of PIIX4_PM
Am 3. August 2022 20:00:18 MESZ schrieb Peter Maydell : >On Wed, 3 Aug 2022 at 18:26, Bernhard Beschow wrote: >> >> On Tue, Aug 2, 2022 at 8:37 AM Philippe Mathieu-Daudé via >> wrote: >>> >>> On 28/7/22 15:16, Igor Mammedov wrote: >>> > On Thu, 28 Jul 2022 13:29:07 +0100 >>> > Peter Maydell wrote: >>> > >>> >> On Thu, 28 Jul 2022 at 12:50, Igor Mammedov wrote: >>> >>> Disable compiled out features using compat properties as the least >>> >>> risky way to deal with issue. >>> >>> So now MIPS is forced to use meaningless compat[] to satisfy X86. >>> >>> Am I wrong seeing this as a dirty hack creeping in, yet another >>> technical debt that will hit (me...) back in a close future? >>> >>> Are we sure there are no better solution (probably more time consuming >>> and involving refactors) we could do instead? >> >> >> Working on the consolidation of piix3 and -4 soutbridges [1] I've stumbled >> over certain design decisions where board/platform specific assumptions are >> baked into the piix device models. I figure that's the core of the issue. >> >> In our case the ACPI functionality is implemented by inheritance while >> perhaps it should be implemented using composition. With composition, the >> ACPI functionality could be injected by the caller: The pc board would >> inject it while the Malta board wouldn't. This would solve both the crash >> and above design problem. >> >> I'd be willing to implement it but can't make any promises about the time >> frame since I'm currently doing this in my free time. Any hints regarding >> the implementation would be welcome, though. > > >For the 7.1 release (coming up real soon now) can we get consensus >on this patch from Igor as the least risky way to at least fix >the segfault ? We can look at better approaches for 7.2. Hi, My proposal isn't 7.1 material. I merily intended to start a design discussion how to proceed after 7.1 that would make Phil's maintainer life easier and provide further insights for my consolidation work. I don't feel qualified enough to judge the impact of Igor's patch, so I'd leave that for the competent. Best regards, Bernhard > >thanks >-- PMM
Re: [PATCH 00/11] QOM'ify PIIX3 southbridge
Am 26. Juli 2022 16:53:03 MESZ schrieb "Michael S. Tsirkin" : >On Wed, Jul 13, 2022 at 10:17:24AM +0200, Bernhard Beschow wrote: >> Similar to PIIX4 this series QOM'ifies internal device creation for PIIX3. >> This reduces the delta between the implementations of PIIX3 and PIIX4 and >> therefore might allow to merge both implementations in the future. >> >> There were two challenges in this series: >> >> First, QEMU considers the ACPI and USB functions to be optional in PIIX3. >> When instantiating those with object_initialize_child(), they need to be >> unparented in the realize function to prevent an assertion (see respective >> commit messages). >> >> Second, the PIC used to be instantiated outside of the southbridge while >> some sub functions require a PIC with populated qemu_irqs. This has been >> solved by introducing a proxy PIC which furthermore allows PIIX3 to be >> agnostic towards the virtualization technology used (KVM, TCG, Xen). > >Thanks! >I think it's best to merge this after the 7.1 release. >I'll tag this but if possible pls also ping me after the release >to make sure I don't forget. Thanks! Sure! I'm extending the scope of this series to go all the way to consolidate the piix 3 + 4 southbridges which is why I didn't post a v2 yet. The extended series will also address Peter's comments. Thanks, Bernhard P.S.: I've got a working POC where PIIX4 rather than PIIX3 is used in the "pc" machine which also supports KVM accelleration: https://github.com/shentok/qemu/commits/pc-piix4 > >> Testing done: >> * make check >> * make check-avocado >> * Boot live CD: >> * qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom >> manjaro-kde-21.3.2-220704-linux515.iso >> * qemu-system-x86_64 -M q35 -m 2G -accel kvm -cpu host -cdrom >> manjaro-kde-21.3.2-220704-linux515.iso >> >> Bernhard Beschow (11): >> hw/i386/pc: QOM'ify DMA creation >> hw/i386/pc_piix: Allow for setting properties before realizing PIIX3 >> southbridge >> hw/isa/piix3: QOM'ify USB controller creation >> hw/isa/piix3: QOM'ify ACPI controller creation >> hw/i386/pc: QOM'ify RTC creation >> hw/i386/pc: No need for rtc_state to be an out-parameter >> hw/intc/i8259: Introduce i8259 proxy "isa-pic" >> hw/isa/piix3: QOM'ify ISA PIC creation >> hw/isa/piix3: QOM'ify IDE controller creation >> hw/isa/piix3: Wire up ACPI interrupt internally >> hw/isa/piix3: Remove extra ';' outside of functions >> >> hw/i386/Kconfig | 1 - >> hw/i386/pc.c | 17 --- >> hw/i386/pc_piix.c | 70 - >> hw/i386/pc_q35.c | 3 +- >> hw/intc/i8259.c | 27 +++ >> hw/isa/Kconfig| 1 + >> hw/isa/lpc_ich9.c | 11 + >> hw/isa/piix3.c| 84 --- >> include/hw/i386/ich9.h| 2 + >> include/hw/i386/pc.h | 2 +- >> include/hw/intc/i8259.h | 14 ++ >> include/hw/southbridge/piix.h | 16 ++- >> 12 files changed, 201 insertions(+), 47 deletions(-) >> >> -- >> 2.37.1 >> >
Re: [PULL 06/35] hw/acpi: refactor acpi hp modules so that targets can just use what they need
On July 21, 2022 12:13:06 AM GMT+02:00, Ani Sinha wrote: > > >On Wed, 20 Jul 2022, Peter Maydell wrote: > >> On Wed, 20 Jul 2022 at 19:37, Ani Sinha wrote: >> > >> > >> > >> > On Tue, 19 Jul 2022, Peter Maydell wrote: >> > >> > > On Sat, 4 Sept 2021 at 22:36, Michael S. Tsirkin wrote: >> > > How is this intended to work? The obvious fix from my point >> > > of view would just be to say "piix4.c requires pcihp.c" >> > > and cause CONFIG_ACPI_PIIX4 to pull in CONFIG_ACPI_PCIHP, >> > > but that seems like it would be rather undoing the point >> > > of this change. >> > >> > Yes. From the commit log and the vague recollection I have about this >> > change : >> > >> > > For example, mips only needs support for PIIX4 and does not >> > > need acpi pci hotplug support or cpu hotplug support or memory hotplug >> > support >> > > etc >> > >> > So does malta really need acpi hotplug? If not, then the stubbing out of >> > the vmstate struct is correct. >> >> It's not, because the vmstate struct is actually used when you >> savevm/loadvm a malta machine. If the malta shouldn't have >> acpi hotplug then we need to arrange for the hotplug code >> to be avoided at an earlier point, not just stub in the >> vmstate struct field. > >yes I think that would be more appropriate fix, I agree. Since mst added >that vmstate, maybe he can comment on this. > Despite its name, ACPI_PIIX4 is also used by PIIX3, which is required by the "pc" machines. These machines support migration etc. which may explain why ACPI_PIIX4 has the vmstate struct field. To me it just looks like an oversight that the Malta board doesn't support all ACPI_PIIX4 features. Best regards, Bernhard
Re: [PATCH v2 05/22] hw/ppc/pnv: Determine ns16550's IRQ number from QOM property
Hi Cédric,
Am 26. Februar 2022 11:24:03 UTC schrieb "Cédric Le Goater" :
>Hello,
>
>On 2/22/22 20:34, Bernhard Beschow wrote:
>> Determine the IRQ number in the same way as for isa-ipmi-bt. This resolves
>> the last usage of ISADevice::isairq[] which allows it to be removed.
>>
>> Signed-off-by: Bernhard Beschow
>
>I can take this patch in the ppc stream if you prefer.
Good idea. I think that's going to be faster.
Thanks,
Bernhard
> Anyhow,
>
>Reviewed-by: Cédric Le Goater
>
>Thanks,
>
>C.
>
>
>> ---
>> hw/ppc/pnv.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 837146a2fb..1e9f6b0690 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -380,9 +380,12 @@ static void pnv_dt_serial(ISADevice *d, void *fdt, int
>> lpc_off)
>> cpu_to_be32(io_base),
>> cpu_to_be32(8)
>> };
>> +uint32_t irq;
>> char *name;
>> int node;
>>
>> +irq = object_property_get_int(OBJECT(d), "irq", _fatal);
>> +
>> name = g_strdup_printf("%s@i%x", qdev_fw_name(DEVICE(d)), io_base);
>> node = fdt_add_subnode(fdt, lpc_off, name);
>> _FDT(node);
>> @@ -394,7 +397,7 @@ static void pnv_dt_serial(ISADevice *d, void *fdt, int
>> lpc_off)
>>
>> _FDT((fdt_setprop_cell(fdt, node, "clock-frequency", 1843200)));
>> _FDT((fdt_setprop_cell(fdt, node, "current-speed", 115200)));
>> -_FDT((fdt_setprop_cell(fdt, node, "interrupts", d->isairq[0])));
>> +_FDT((fdt_setprop_cell(fdt, node, "interrupts", irq)));
>> _FDT((fdt_setprop_cell(fdt, node, "interrupt-parent",
>> fdt_get_phandle(fdt, lpc_off;
>>
>
Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState
Am 12. Februar 2022 19:30:43 MEZ schrieb Peter Maydell
:
>On Sat, 12 Feb 2022 at 17:02, BALATON Zoltan wrote:
>>
>> On Sat, 12 Feb 2022, Peter Maydell wrote:
>> > On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan wrote:
>> >> By the way the corresponding member in struct PIIXState in
>> >> include/hw/southbridge/piix.h has a comment saying:
>> >>
>> >> /* This member isn't used. Just for save/load compatibility */
>> >> int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>> >>
>> >> and only seems to be filled in piix3_pre_save() but never used. So what's
>> >> the point of this then? Maybe piix3 also uses a bitmap to store these
>> >> levels instead? There's a uint64_t pic_levels member above the unused
>> >> array but I haven't checked the implementation.
>> >
>> > I think what has happened here is that originally piix3 used
>> > the same implementation piix4 currently does -- where it stores
>> > locally the value of the (incoming) PCI IRQ levels, and then when it wants
>> > to know the value of the (outgoing) PIC IRQ levels it loops round
>> > to effectively OR together all the PCI IRQ levels for those PCI
>> > IRQs which are configured to that particular PIC interrupt.
>> >
>> > Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
>> > pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
>> > looking at its local cache of that information in the pci_irq_levels[]
>> > array. This is the source of the "save/load compatibility" thing --
>> > before doing a vmsave piix3_pre_save() fills in that field so that
>> > it appears in the outbound data stream and thus a migration from
>> > a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
>> > was important at the time, but could probably be cleaned up now.)
>> >
>> > The next commit after that one is ab431c283e7055bcd, which
>> > is an optimization that fixes the equivalent of the "XXX: optimize"
>> > marker in the gt64120_pci_set_irq()/piix4 code -- this does
>> > something slightly more complicated involving the pic_levels
>> > field, in order to avoid having to do the "loop over all the
>> > PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.
>> >
>> > We could pick up one or both (or none!) of these two changes
>> > for the piix4 code. (If we're breaking migration compat anyway
>> > we wouldn't need to include the save-load compat part of
>> > the first change.)
>>
>> I'm not sure I fully get this. Currently (before this series) PIIX4State
>> does not seem to have any fields to store irq state (in hw/isa/piix4.c):
>>
>> struct PIIX4State {
>> PCIDevice dev;
>> qemu_irq cpu_intr;
>> qemu_irq *isa;
>>
>> RTCState rtc;
>> /* Reset Control Register */
>> MemoryRegion rcr_mem;
>> uint8_t rcr;
>> };
>>
>> Patch 1 in this series introduces that by moving it from MaltaState. Then
>> we could have a patch 2 to clean it up and change to the way piix3 does it
>> and skip introducing the saving of this array into the migration state. It
>> may still break migration but not sure if MaltaState was saved already so
>> this may be already missing from migration anyway and if we do the same as
>> piix3 then maybe we don't need to change the piix4 state either (as this
>> is saved as part of the PCIHost state?) but I don't know much about this
>> so maybe I'm wrong.
>
>Yeah, that would work -- we weren't saving the old gt64xxx_pci.c
>pci_irq_levels[] global, so we don't break anything that wasn't
>already broken by not putting the newly-introduced PIIX4State
>array into migration state. Then when we do the equivalent of
>e735b55a8c11 the array can just be deleted again. (We can't
>conveniently switch to using pci_bus_get_irq_level() before doing
>patch 1 of this series, because we need the pointer to the
>piix4 device object and gt64120_pci_set_irq() is only passed
>a pointer directly to a qemu_irq array.)
>
>> In any case PIIX3 and PIIX4 state seem to be different so there's no
>> reason to worry aobut compatibility between them.
>
>Yep, that's right. The only reasons to copy changes from piix3
>are (a) because they're worth having in themselves and (b)
>because having the two devices be the same is maybe less
>confusing. (b)'s a pretty weak reason, and (a) depends on
>the individual change. In this case I think making the equivalent
>of e735b55a8c11 is worthwhile because it saves us having an
>extra array field and migrating it, and the change is pretty
>small. For ab431c283e7055bcd you could argue either way -- it's
>clearly a better way to structure the irq handling, but it's only
>an optimisation, not a bug fix.
e735b55a8c11 seems like a very elegant way for fixing migration of the IRQ
levels. I'll have a look.
Regards,
Bernhard
>
>-- PMM
Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute
Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan :
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>> gt64xxx to piix4" where i8259[] was moved from MaltaState to PIIX4State
>> to make the code movement more obvious. However, i8259[] seems redundant
>> to *isa, so remove it.
>
>Should this be squashed in patch 1 or at least come after it? Or are there
>some other changes needed before this patch?
I didn't want to change the patch order since they've been reviewed already.
But yeah, you're right: It makes sense to get this out of the way early in the
patch series. I will do a v3.
Regards,
Bernhard
>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: Bernhard Beschow
>> ---
>> hw/isa/piix4.c | 7 +--
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index a9322851db..692fa8d1f9 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -43,7 +43,6 @@ struct PIIX4State {
>> PCIDevice dev;
>> qemu_irq cpu_intr;
>> qemu_irq *isa;
>> -qemu_irq i8259[ISA_NUM_IRQS];
>>
>> int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>
>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int
>> level)
>> pic_level |= s->pci_irq_levels[i];
>> }
>> }
>> -qemu_set_irq(s->i8259[pic_irq], pic_level);
>> +qemu_set_irq(s->isa[pic_irq], pic_level);
>> }
>> }
>>
>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus
>> **isa_bus, I2CBus **smbus)
>>
>> pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s,
>> PIIX_NUM_PIRQS);
>>
>> -for (int i = 0; i < ISA_NUM_IRQS; i++) {
>> -s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
>> -}
>> -
>> return dev;
>> }
>>
Re: [PATCH 3/3] isa/piix4: Resolve global variables
Am 30. Januar 2022 23:53:42 MEZ schrieb "Philippe Mathieu-Daudé"
:
>On 14/1/22 14:36, Peter Maydell wrote:
>> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow wrote:
>>>
>>> Now that piix4_set_irq's opaque parameter references own PIIX4State,
>>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
>>>
>>> Signed-off-by: Bernhard Beschow
>>> ---
>>> hw/isa/piix4.c| 22 +-
>>> include/hw/southbridge/piix.h | 2 --
>>> 2 files changed, 9 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>> index a31e9714cf..964e09cf7f 100644
>>> --- a/hw/isa/piix4.c
>>> +++ b/hw/isa/piix4.c
>>> @@ -39,14 +39,14 @@
>>> #include "sysemu/runstate.h"
>>> #include "qom/object.h"
>>>
>>> -PCIDevice *piix4_dev;
>>> -
>>> struct PIIX4State {
>>> PCIDevice dev;
>>> qemu_irq cpu_intr;
>>> qemu_irq *isa;
>>> qemu_irq i8259[ISA_NUM_IRQS];
>>>
>>> +int pci_irq_levels[PIIX_NUM_PIRQS];
>>> +
>>
>> I wondered how we were migrating this state, and the answer
>> seems to be that we aren't (and weren't before, when it was
>> a global variable, so this is a pre-existing bug).
>
>Indeed the migrated VM starts with PCI IRQ levels zeroed.
>
>> Does the malta platform support migration save/load?
>
>Maybe a "best effort" support, but not versioned machines.
>
>> We should probably add this field to the vmstate struct
>> (which will be a migration compatibility break, which is OK
>> as the malta board isn't versioned.)
>
>Yeah good catch.
>
>Bernhard, do you mind adding it?
Sure, I'll give it a try. Shall I submit a v2 of this patch series then? If so,
would it be ok to change the topic of the cover letter or would that be
confusing?
Last but not least: How to treat the version_id and the version parameters of
the new and existing fields?
Regards,
Bernhard.
