On 06/11/2023 14:12, Kevin Wolf wrote:
Hi Kevin,
Thanks for taking the time to review this. I'll reply inline below.
Am 25.10.2023 um 00:40 hat Mark Cave-Ayland geschrieben:
This function reads the value of the PCI_CLASS_PROG register for PCI IDE
controllers and configures the PCI BARs and/or IDE ioports accordingly.
In the case where we switch to legacy mode, the PCI BARs are set to return zero
(as suggested in the "PCI IDE Controller" specification), the legacy IDE ioports
are enabled, and the PCI interrupt pin cleared to indicate legacy IRQ routing.
Conversely when we switch to native mode, the legacy IDE ioports are disabled
and the PCI interrupt pin set to indicate native IRQ routing. The contents of
the PCI BARs are unspecified, but this is not an issue since if a PCI IDE
controller has been switched to native mode then its BARs will need to be
programmed.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Tested-by: BALATON Zoltan <bala...@eik.bme.hu>
Tested-by: Bernhard Beschow <shen...@gmail.com>
---
hw/ide/pci.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/ide/pci.h | 1 +
2 files changed, 91 insertions(+)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a25b352537..5be643b460 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -104,6 +104,96 @@ const MemoryRegionOps pci_ide_data_le_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
};
+static const MemoryRegionPortio ide_portio_list[] = {
+ { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
+ { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
+ { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
+ PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionPortio ide_portio2_list[] = {
+ { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
+ PORTIO_END_OF_LIST(),
+};
This is duplicated from hw/ide/ioport.c. I think it would be better to
use the arrays already defined there, ideally by calling ioport.c
functions to setup and release the I/O ports.
The tricky part here is that hw/ide/ioport.c is defined for CONFIG_ISA, and so if we
did that then all PCI IDE controllers would become dependent upon ISA too, regardless
of whether they implement compatibility mode or not. What do you think is the best
solution here? Perhaps moving ide_init_ioport() to a more ISA-specific place? I know
that both myself and Phil have considered whether ide_init_ioport() should be
replaced by something else further down the line.
+void pci_ide_update_mode(PCIIDEState *s)
+{
+ PCIDevice *d = PCI_DEVICE(s);
+ uint8_t mode = d->config[PCI_CLASS_PROG];
+
+ switch (mode & 0xf) {
+ case 0xa:
+ /* Both channels legacy mode */
Why is it ok to handle only the case where both channels are set to the
same mode? The spec describes mixed-mode setups, too, and doesn't seem
to allow ignoring a mode change if it's only for one of the channels.
Certainly that can be done: only both channels were implemented initially because
that was the test case immediately available using the VIA. I can have a look at
implementing both channels separately in v2.
+
+ /* Zero BARs */
+ pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x0);
+ pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x0);
+ pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x0);
+ pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x0);
Here I'm not sure what the spec really implies. Disabling the BAR (i.e.
making it read-only and returning 0) while in compatibility mode doesn't
necessarily mean that the value of the register is lost. In other words,
are we sure that a driver can't expect that the old value is still there
when it re-enables native mode?
The specification is definitely a bit vague on the details here. In the testing I've
done with the VIA, there is only ever a switch from native to legacy mode, and not
the other way around. I can see the logic that once you've gone from the native to
legacy mode, the memory allocated for the BARs is already reserved by the OS so in
theory it could be possible to switch back to native mode again... and that would
work if the BARs are preserved.
Would it happen in practice? I'm not really sure, but I can try to implement this if
you think it makes sense to take a safer approach.
+ /* Clear interrupt pin */
+ pci_config_set_interrupt_pin(d->config, 0);
Unlike for the BARs, I don't see anything in the spec that allows
disabling this byte. I doubt it hurts in practice, but did you see any
drivers requiring this? According to the spec, we just must not use the
PCI interrupt in compatbility mode, but the registers stay accessible.
The PCI config dumps taken from a real VIA indicate that this byte is cleared in
legacy mode, and that appears to make sense here. If you imagine an early PCI IDE
controller, it will always start up in legacy mode and so you don't want to indicate
to the guest OS that PCI IRQ routing is required unless it has been switched to
native mode first.
As far as I can see, the whole PCI interrupt configuration is currently
unused anyway, and nothing in this series seems to change it. So won't
we incorrectly continue to use the legacy interrupt even in native mode?
(Actually, cmd646 seems to get it wrong the other way around and uses
the PCI interrupt even in compatibility mode.)
I think this means that BMDMAState needs to have two irq lines (a legacy
and a PCI one) and select the right one in bmdma_irq() depending on
which mode we're in currently.
I need to flesh out the details a bit more (in particular testing with more than just
the VIA PCI IDE controller), but yes the eventual aim is to consolidate the majority
of the BMDMA and mode switching code into hw/ide/pci.c so the individual controllers
don't need to worry about this, and everything "just works".
+ /* Add legacy IDE ports */
+ if (!s->bus[0].portio_list.owner) {
+ portio_list_init(&s->bus[0].portio_list, OBJECT(d),
+ ide_portio_list, &s->bus[0], "ide");
+ portio_list_add(&s->bus[0].portio_list,
+ pci_address_space_io(d), 0x1f0);
+ }
+
+ if (!s->bus[0].portio2_list.owner) {
+ portio_list_init(&s->bus[0].portio2_list, OBJECT(d),
+ ide_portio2_list, &s->bus[0], "ide");
+ portio_list_add(&s->bus[0].portio2_list,
+ pci_address_space_io(d), 0x3f6);
+ }
+
+ if (!s->bus[1].portio_list.owner) {
+ portio_list_init(&s->bus[1].portio_list, OBJECT(d),
+ ide_portio_list, &s->bus[1], "ide");
+ portio_list_add(&s->bus[1].portio_list,
+ pci_address_space_io(d), 0x170);
+ }
+
+ if (!s->bus[1].portio2_list.owner) {
+ portio_list_init(&s->bus[1].portio2_list, OBJECT(d),
+ ide_portio2_list, &s->bus[1], "ide");
+ portio_list_add(&s->bus[1].portio2_list,
+ pci_address_space_io(d), 0x376);
+ }
+ break;
This is essentially ide_init_ioport(), except that it handles the case
where it is already initialised. Let's reuse it.
This still has the dependency issue with CONFIG_ISA, but I'm happy to implement what
you think is the best solution (as discussed above).
+ case 0xf:
+ /* Both channels native mode */
+
+ /* Set interrupt pin */
+ pci_config_set_interrupt_pin(d->config, 1);
+
+ /* Remove legacy IDE ports */
+ if (s->bus[0].portio_list.owner) {
+ portio_list_del(&s->bus[0].portio_list);
+ portio_list_destroy(&s->bus[0].portio_list);
+ }
+
+ if (s->bus[0].portio2_list.owner) {
+ portio_list_del(&s->bus[0].portio2_list);
+ portio_list_destroy(&s->bus[0].portio2_list);
+ }
+
+ if (s->bus[1].portio_list.owner) {
+ portio_list_del(&s->bus[1].portio_list);
+ portio_list_destroy(&s->bus[1].portio_list);
+ }
+
+ if (s->bus[1].portio2_list.owner) {
+ portio_list_del(&s->bus[1].portio2_list);
+ portio_list_destroy(&s->bus[1].portio2_list);
+ }
+ break;
And this part could be an ioport.c function as well.
+ }
+}
Kevin
ATB,
Mark.
