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

[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.

Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan