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");
>>>> 
>>> 
>>