qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-create


From: Mark Cave-Ayland
Subject: Re: [PATCH v2 07/10] hw/ide/piix: Require an ISABus only for user-created instances
Date: Wed, 1 Mar 2023 16:42:16 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0

On 23/02/2023 20:46, Bernhard Beschow wrote:


Am 7. Februar 2023 20:52:02 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
On 06/02/2023 23:40, Bernhard Beschow wrote:

Am 5. Februar 2023 22:32:03 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
On 05/02/2023 22:21, BALATON Zoltan wrote:

On Sun, 5 Feb 2023, Mark Cave-Ayland wrote:
On 26/01/2023 21:17, Bernhard Beschow wrote:
Internal instances now defer interrupt wiring to the caller which
decouples them from the ISABus. User-created devices still fish out the
ISABus from the QOM tree and the interrupt wiring remains in PIIX IDE.
The latter mechanism is considered a workaround and intended to be
removed once a deprecation period for user-created PIIX IDE devices is
over.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
    include/hw/ide/pci.h |  1 +
    hw/ide/piix.c        | 64 ++++++++++++++++++++++++++++++++++----------
    hw/isa/piix.c        |  5 ++++
    3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 24c0b7a2dd..ee2c8781b7 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -54,6 +54,7 @@ struct PCIIDEState {
        MemoryRegion bmdma_bar;
        MemoryRegion cmd_bar[2];
        MemoryRegion data_bar[2];
+    bool user_created;
    };
      static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 5980045db0..f0d95761ac 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -108,6 +108,13 @@ static void bmdma_setup_bar(PCIIDEState *d)
        }
    }
    +static void piix_ide_set_irq(void *opaque, int n, int level)
+{
+    PCIIDEState *d = opaque;
+
+    qemu_set_irq(d->isa_irqs[n], level);
+}
+
    static void piix_ide_reset(DeviceState *dev)
    {
        PCIIDEState *d = PCI_IDE(dev);
@@ -138,11 +145,18 @@ static void pci_piix_init_ports(PCIIDEState *d, ISABus 
*isa_bus)
        };
        int i;
    +    if (isa_bus) {
+        d->isa_irqs[0] = isa_bus->irqs[port_info[0].isairq];
+        d->isa_irqs[1] = isa_bus->irqs[port_info[1].isairq];
+    } else {
+        qdev_init_gpio_out(DEVICE(d), d->isa_irqs, 2);
+    }
+
        for (i = 0; i < 2; i++) {
            ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
            ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
                            port_info[i].iobase2);
-        ide_init2(&d->bus[i], isa_bus->irqs[port_info[i].isairq]);
+        ide_init2(&d->bus[i], qdev_get_gpio_in(DEVICE(d), i));
              bmdma_init(&d->bus[i], &d->bmdma[i], d);
            d->bmdma[i].bus = &d->bus[i];
@@ -154,8 +168,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
**errp)
    {
        PCIIDEState *d = PCI_IDE(dev);
        uint8_t *pci_conf = dev->config;
-    ISABus *isa_bus;
-    bool ambiguous;
+    ISABus *isa_bus = NULL;
          pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
    @@ -164,22 +177,36 @@ static void pci_piix_ide_realize(PCIDevice *dev, 
Error **errp)
          vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
    -    isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS, 
&ambiguous));
-    if (ambiguous) {
-        error_setg(errp,
-                   "More than one ISA bus found while %s supports only one",
-                   object_get_typename(OBJECT(dev)));
-        return;
-    }
-    if (!isa_bus) {
-        error_setg(errp, "No ISA bus found while %s requires one",
-                   object_get_typename(OBJECT(dev)));
-        return;
+    if (d->user_created) {
+        bool ambiguous;
+
+        isa_bus = ISA_BUS(object_resolve_path_type("", TYPE_ISA_BUS,
+                                                   &ambiguous));
+
+        if (ambiguous) {
+            error_setg(errp,
+                       "More than one ISA bus found while %s supports only 
one",
+                       object_get_typename(OBJECT(dev)));
+            return;
+        }
+
+        if (!isa_bus) {
+            error_setg(errp, "No ISA bus found while %s requires one",
+                       object_get_typename(OBJECT(dev)));
+            return;
+        }
        }
          pci_piix_init_ports(d, isa_bus);
    }
    +static void pci_piix_ide_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in(dev, piix_ide_set_irq, 2);
+}
+
    static void pci_piix_ide_exitfn(PCIDevice *dev)
    {
        PCIIDEState *d = PCI_IDE(dev);
@@ -191,6 +218,11 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
        }
    }
    +static Property piix_ide_properties[] = {
+    DEFINE_PROP_BOOL("user-created", PCIIDEState, user_created, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
    /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
    static void piix3_ide_class_init(ObjectClass *klass, void *data)
    {
@@ -205,11 +237,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void 
*data)
        k->class_id = PCI_CLASS_STORAGE_IDE;
        set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
        dc->hotpluggable = false;
+    device_class_set_props(dc, piix_ide_properties);
    }
      static const TypeInfo piix3_ide_info = {
        .name          = TYPE_PIIX3_IDE,
        .parent        = TYPE_PCI_IDE,
+    .instance_init = pci_piix_ide_init,
        .class_init    = piix3_ide_class_init,
    };
    @@ -227,11 +261,13 @@ static void piix4_ide_class_init(ObjectClass *klass, 
void *data)
        k->class_id = PCI_CLASS_STORAGE_IDE;
        set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
        dc->hotpluggable = false;
+    device_class_set_props(dc, piix_ide_properties);
    }
      static const TypeInfo piix4_ide_info = {
        .name          = TYPE_PIIX4_IDE,
        .parent        = TYPE_PCI_IDE,
+    .instance_init = pci_piix_ide_init,
        .class_init    = piix4_ide_class_init,
    };
    diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 54a1246a9d..f9974c2a77 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -345,9 +345,14 @@ static void pci_piix_realize(PCIDevice *dev, const char 
*uhci_type,
          /* IDE */
        qdev_prop_set_int32(DEVICE(&d->ide), "addr", dev->devfn + 1);
+    qdev_prop_set_bit(DEVICE(&d->ide), "user-created", false);
        if (!qdev_realize(DEVICE(&d->ide), BUS(pci_bus), errp)) {
            return;
        }
+    qdev_connect_gpio_out(DEVICE(&d->ide), 0,
+                          qdev_get_gpio_in(DEVICE(&d->pic), 14));
+    qdev_connect_gpio_out(DEVICE(&d->ide), 1,
+                          qdev_get_gpio_in(DEVICE(&d->pic), 15));
          /* USB */
        if (d->has_usb) {

I haven't checked the datasheet, but I suspect this will be similar to the 
cmd646/via PCI-IDE interfaces in that there will be a PCI configuration 
register that will switch between ISA compatibility mode (and ISA irqs) and PCI 
mode (with PCI IRQs). So it would be the device configuration that would 
specify PCI or ISA mode, rather than the presence of an ISABus.

I forgot about this topic already and haven't follwed this series either so 
what I say may not fully make sense but I think CMD646 and via-ide are 
different. CMD646 is a PCI device and should use PCI interrupts while via-ide 
is part of a southbridge/superio complex and connected to the ISA PICs within 
that southbride, so I think via-ide always uses ISA IRQs and the ISA btidge 
within the same chip may convert that to PCI IRQs or not (that part is where 
I'm lost also because we may not actually model it that way). After a long 
debate we managed to find a solution back then that works for every guest we 
use it for now so I think we don't want to touch it now until some real need 
arises. It does not worth the trouble and added complexity to model something 
that is not used just for the sake of correctness. By the time we find a use 
for that, the ISA emulation may evolve so it's easier to implement the missing 
switching between isa and native mode or we may want to do it differently (such 
as we do things differently now compared to what we did years ago). So I think 
it does not worth keeping the ISA model from being simplified for some 
theoretical uses in the future which we may not actually do any time soon. But 
I don't want to get into this again so just shared my thoughts and feel free to 
ignore it. I don't care where these patches go as long as the VIA model keeps 
working for me.

I have a vague memory that ISA compatibility mode was part of the original 
PCI-BMDMA specification, but it has been a while since I last looked.

Bernhard, is there any mention of this in the PIIX datasheet(s)? For reference 
the cmd646 datasheet specifies that ISA mode or PCI mode is determined by 
register PROG_IF (0x9) in PCI configuration space.

I've found the following:

    "Only PCI masters have access to the IDE port. ISA Bus masters cannot access the 
IDE I/O port addresses. Memory targeted by the IDE interface acting as a PCI Bus master 
on behalf of IDE DMA slaves must reside on PCI, usually main memory implemented by the 
host-to-PCI bridge."

And:

    "PIIX4 can act as a PCI Bus master on behalf of an IDE slave device."

Does this perhaps mean that piix-ide does indeed have no ISA bus?

I'd be amazed if that were the case: certainly when the first motherboards came 
out with PCI and ISA slots, I'd expect the IDE legacy mode to be enabled by 
default since BIOSes and OSs such as DOS wouldn't have been PCI aware and would 
access the ISA ioports directly. From memory the OF PCI specification has 
mention of workarounds such as mapping the old VGA memory to PCI MMIO space for 
compatibility reasons, so I'd be surprised if there wasn't something similar 
for IDE.

The wording above is a bit ambiguous because I can see the above statements 
would be true if the PCI-IDE device were already switched to PCI mode, and what 
we're looking for is whether a switch between the two is supported or possible.

A switch is definitely impossible: The PIIX IDE function has the 0x3c 
(interrupt line) and 0x3d (interrupt pin) registers reserved. It's fixed in 
legacy mode (prog-if is 0x80).

Best regards,
Bernhard

Thanks for checking this. If you have any upcoming patches that touch this file, it's worth adding a comment explaining this with a reference to the relevant part of the datasheet, and also update wmask accordingly.


ATB,

Mark.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]