On Sat, 19 Feb 2022, Liav Albani wrote:
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.
I think we don't actually have ich9-ide, we only have piix3, piix4 and
ahci, the latter is used by ich9. I just said that calling this new device
ich7-ide instead of ich6-ide would make it more clear it has nothing to do
with ich9. It's already clear with calling it ich6 unless somebody
misreads that which is easier with 6 than 7. Also according to
https://en.wikipedia.org/wiki/I/O_Controller_Hub the native mode PATA
controller first appeared in ICH3 but that says limited so maybe it was
changed later. I don't know anthing about these so can't tell which are
similar. Also if your reference system you're emulating is actually an
ICH7 then calling it that would make sense to me.
+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).
I still don't get what this tries to achieve but I think it's very likely
wrong so if it's not needed it's better to just drop it for now. It can
also be addressed in a follow up patch later.
+}
+
+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,
The real question if this controller is similiar enough to piix or it's
different just for this initial version it's reusing most code but this
will change in the future. If the latter then putting it in a separate
file is better, if it will stay like it then putting it in piix to share
code already there is better.
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.
I just wanted to avoid more code duplication by adding new files that will
look like the devices that's already there. But if this is just a first
step and you'll send more patches to implement more features that will
make it more different, then having it in a separate file is better. But I
could only judge from the patch so far which is almost identical to piix,
only differentce is irq and port handling so this could be a variant of
piix then as well if that's all you want for this device.
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 :)
It can be in the same series but still good to have as separate patch so
it can be reviewed independently and maybe also merged independently. The
maintainer can always squash patches together later but separating them is
usually only the original contributor can easily and best do.
+
+ 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.
The cmd646 is similar as it's a PCI device and shares the same PCIIDEState
so I think doing the same is best for cnosistency and to avoid needing to
change the shared PCIIDEState that should be a generic PCI IDE controller
model but still has one cmd646 specific field. Using qdev_init_gpio_in
attaches the irq to the device so it will be kept track of without needing
to store it anywhere so I think it's not a bad way to do it for this case.
AHCI has its own device state and stores the irq there, I don't know what
rtl8139 does. There may be multiple correct ways to do it but following
similar examples is probably a good idea.
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
OK so we probably again have a case we had with via-ide which also has
legacy mode where it behaves like an ISA controller using legacy ports and
irq 14-15 or a native mode where ports can be set by PCI BARs and irqs may
or may not use PCI interrupt. Due to how ISA code is organised, which is
an old part of QEMU we can't easily implement this (the main problem is
once ISA ports are created we can't delete them so we're stuck in legacy
mode) so what we have is only emulating one mode for these controller
models. I think the cmd646 is completely PCI device (not sure it had a
legacy mode), the via-ide is native mode but with ISA interrupts and piix
is probably only legacy mode but not sure if it had a native mode. Maybe
this ich-ide is the native mode of piix? In that case could it be modelled
that way adding an option to piix so it can be set up in native mode or
legacy mode? We would still not be able to switch between them from the
guest as on real hardware but machines or users could decide when creating
the device what mode they want. Or is the ich ide completely different
from piix?
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.
OK then it will probably be different from piix so maybe this justifies it
putting it in a different file and code that is similar now will become
more different in the future.
I'll wait a few more days to let other people send comments on this before
sending v3. Thanks again for the valuable comments :)
That's probably a good idea, I hope others will have some time to comment
as well. I'm trying to help but I can only give ideas, not definitive
answers for your questions.
Regards.
BALATON Zoltan
