Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Liav Albani]

On 2/19/22 02:50, BALATON Zoltan wrote:
> > +/*
> > + * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
>
> This is a small thing, but if these two are the same maybe keeping 
> this comment but using the ich7 name everywhere else would make it 
> less likely to get it confused with ich9. I mean ich6 and ich9 is 
> easily confused, while ich7 is clearly distinct. But maybe it's just 
> me, calling it ich6 is also correct and can be preferred by someone else.
ICH6 and ICH7 IDE controllers are quite the same as far as I know. I 
could change it, but then one could argue that the name ich6-ide seems 
like "ich9-ide", so not sure if we can really go on this path.
> > +static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, 
> > uint32_t val,
> > +                                    int l)
> > +{
> > +    uint32_t i;
> > +
> > +    pci_default_write_config(d, addr, val, l);
> > +
> > +    for (i = addr; i < addr + l; i++) {
> > +        switch (i) {
> > +        case 0x40:
> > +            pci_default_write_config(d, i, 0x8000, 2);
> > +            continue;
> > +        case 0x42:
> > +            pci_default_write_config(d, i, 0x8000, 2);
> > +            continue;
> > +        }
> > +    }
>
> I'm not sure what this tries to do but this for cycle looks suspicious 
> here. It's also only calls pci_default_write_config() so why didn't 
> the default worked and why was this override needed?
It's just a loop to ensure that if the guest OS tries to disable an IDE 
channel from the PCI config space, it seems "stuck" on enabled. I should 
probably put a comment on this to explain this, but I definitely don't 
want the guest OS to be able to disable any IDE channel for now (on real 
hardware, it does that, but I think we can either skip this feature or 
implement in a future patch, as Linux deals with the controller just fine).
> > +}
> > +
> > +static void ich6_ide_reset(DeviceState *dev)
> > +{
> > +    PCIIDEState *d = PCI_IDE(dev);
> > +    PCIDevice *pd = PCI_DEVICE(d);
> > +    uint8_t *pci_conf = pd->config;
> > +    int i;
> > +
> > +    for (i = 0; i < 2; i++) {
> > +        ide_bus_reset(&d->bus[i]);
> > +    }
> > +
> > +    /* TODO: this is the default. do not override. */
> > +    pci_conf[PCI_COMMAND] = 0x00;
> > +    /* TODO: this is the default. do not override. */
> > +    pci_conf[PCI_COMMAND + 1] = 0x00;
> > +    /* TODO: use pci_set_word */
> > +    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
> > +    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
> > +    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
> > +}
> > +
> > +static int pci_ich6_init_ports(PCIIDEState *d)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < 2; i++) {
> > +        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> > +        ide_init2(&d->bus[i], d->native_irq);
> > +
> > +        bmdma_init(&d->bus[i], &d->bmdma[i], d);
> > +        d->bmdma[i].bus = &d->bus[i];
> > +        ide_register_restart_cb(&d->bus[i]);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
> > +{
> > +    PCIIDEState *d = PCI_IDE(dev);
> > +    uint8_t *pci_conf = dev->config;
> > +    int rc;
>
> All in all this device looks very similar to piix ide devices with 
> only some differentces so I wonder if ir could be implemented as 
> another device in piix.c. We already have 3 variants there: piix3-ide, 
> piix3-ide-xen, and piix4-ide so maybe putting this device in piix.c 
> could avoid some code duplication. In that case moving out 
> bmdma_{read,write} were not needed either although maybe that's a good 
> idea anyway to share it with other devices.
As for putting the ich6-ide code in piix.c  - I'm not against it. 
Although, in the long run, if I send more patches to QEMU, I rather 
split the files a bit more in the /hw/ide directory as there's a lot of 
code duplication to solve.
So, what we could do instead, is to share more code between the devices 
and still keep them in separate files.

As for moving out the bmdma_{write,read} functions - I'm still in the 
previous mind that we need to move them out as via.c shares the same 
code but currently has code duplications for it, and that I want to 
solve as part of making the IDE code more clean and to add more features.

However, if it seems necessary to do this cleanup before implementing 
this ich6-ide controller, I'm more than happy to wait on this, send a 
patch to solve and clean up some issues in the IDE code, and then 
re-send this as v3.
I might even consider doing that now if nobody rejects this idea :)

> > +
> > +    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
> > +
> > +    /* PCI native mode-only controller, supports bus mastering */
> > +    pci_conf[PCI_CLASS_PROG] = 0x85;
> > +
> > +    bmdma_setup_bar(d);
> > +    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
> > +
> > +    d->native_irq = pci_allocate_irq(&d->parent_obj);
>
> Is this irq and the new field in PCIIDEState really needed? If this 
> device is using PCI interrupts could it do the same as CMD646 ide does 
> instead?
I looked into how cmd646.c does that, but it creates that with the 
qdev_init_gpio_in function. The AHCI controller seems to use 
pci_allocate_irq function (in ich.c), as well as other hardware devices 
in QEMU, but the rtl8139 device doesn't have such field and still works, 
so not sure what to do and how to make it to work in the best, simple 
way here.
> That's all for now, I haven't checked the docs of these ide 
> controllers so I'm not sure if these have switchable native and legacy 
> modes like via has and we again getting the problem that we can't 
> model that easily or these are really different with one having only 
> legacy and the ich6/7 only native modes.

If I recall correctly, on my ICH7 test machine you can switch between 
native and legacy mode, but it doesn't mean that there couldn't be a 
device on some motherboard in 2009 that doesn't allow you to switch 
between legacy and native mode. However, I rather not put the option 
because I want software to deal with a PCI device that is free from 
legacy IO ports completely. Also, I am pretty sure that if I change the 
settings in the BIOS menu then it could be stuck to native mode (if for 
example, I enable both the PATA connector and the 4 SATA ports), but 
still not sure about this. This is a feature I'll look into it in the 
future, as I definitely want to improve on this and maybe allow the user 
to configure the ich6-ide device to have a different MAP register to 
simulate 4 SATA ports and 2 PATA connections.

I'll wait a few more days to let other people send comments on this 
before sending v3. Thanks again for the valuable comments :)

Best regards,
Liav

> Regards.
> BALATON Zoltan