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