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