qemu-block
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function

From: Mark Cave-Ayland
Subject: Re: [PATCH 1/2] ide/pci.c: introduce pci_ide_update_mode() function
Date: Tue, 24 Oct 2023 21:52:17 +0100
User-agent: Mozilla Thunderbird 
On 24/10/2023 08:08, Bernhard Beschow wrote:


Am 23. Oktober 2023 21:06:11 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:

On 23/10/2023 18:19, Bernhard Beschow wrote:


Am 22. Oktober 2023 22:06:30 UTC schrieb Bernhard Beschow <shentey@gmail.com>:



Am 19. Oktober 2023 13:04:51 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:

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-ayland@ilande.co.uk>
---
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..9eb30af632 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[] = {


Although the naming seems familiar: What about renaming both lists to something 
like ide_portio_primary_list resp. ide_portio_secondary_list? Having one list 
carrying a number in its name while omitting it for the other I find rather 
confusing.


The two different portio_lists don't represent the primary and secondary 
interfaces though: they represent the command and data ports for a single 
interface.


Ah, right.


I've left the naming as-is (at least for now) so that all of the IDEBus fields, 
ISA IDE ioports and PCI IDE ioports all share the same naming convention.


Okay. At some point we should really harmonize the names to avoid above 
confusion. The PCI IDE BAR code does a much better job at naming and could 
serve as a template. Then all IDE code would clearly communicate that these are 
all the same concepts. I could send a patch for it once this series is in.
I agree that would be useful: and also same for the memory region names which could be updated so it's possible to tell the difference between the command and data ports (or indeed base/control as indicated in other online references). 




+    { 0, 1, 1, .read = ide_status_read, .write = ide_ctrl_write },
+    PORTIO_END_OF_LIST(),
+};
+
+void pci_ide_update_mode(PCIIDEState *s)
+{
+    PCIDevice *d = PCI_DEVICE(s);
+    uint8_t mode = d->config[PCI_CLASS_PROG];
+
+    switch (mode) {


Maybe

   switch (mode & 0xf) {

here such that only the bits relevant to the PCI IDE controller specification 
are analyzed?


Due to the above conversation I realize that s->bus[] could be iterated over 
such that only the two bits of each bus could be switch()ed over. This would avoid 
some duplicate code, model the specification closer and allow for catching illegal 
states. Illegal states could be logged as guest errors. But it would also 
complicate dealing with the interrupt pin. So this might be a future extension.
Agreed. That particular logic will also change as a result of adding support for switching individual buses on the PCI IDE controller: not that I've seen anything do this to date, but it makes sense to implement what the documentation says where possible. 

I didn't manage to send out the v2 yesterday evening, but I'll do so now.


Best regards,
Bernhard


Then we can omit the high '8' nibble in the case labels which indicate bus 
master capability which is obviously out of scope of the switch statement 
(since you're not touching the BM DMA BAR).


+    case 0x8a:


Perhaps we could add a

   case 0x0:

in front of the above label for compatibility with PIIX-IDE? That way, this 
function could be reused in the future for resetting PIIX-IDE.


+        /* Both channels legacy mode */
+
+        /* 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);
+
+        /* Clear interrupt pin */
+        pci_config_set_interrupt_pin(d->config, 0);


Do we really need to toggle the interrupt pin in this function? Or is this 
VIA-specific? This byte isn't even defined for PIIX-IDE.

Best regards,
Bernhard


+
+        /* 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;
+
+    case 0x8f:
+        /* 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;
+    }
+}
+
static IDEState *bmdma_active_if(BMDMAState *bmdma)
{
      assert(bmdma->bus->retry_unit != (uint8_t)-1);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 1ff469de87..a814a0a7c3 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -61,6 +61,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
void bmdma_status_writeb(BMDMAState *bm, uint32_t val);
extern MemoryRegionOps bmdma_addr_ioport_ops;
void pci_ide_create_devs(PCIDevice *dev);
+void pci_ide_update_mode(PCIIDEState *s);

extern const VMStateDescription vmstate_ide_pci;
extern const MemoryRegionOps pci_ide_cmd_le_ops;



ATB,

Mark.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]