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

[Bernhard Beschow]

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.

Best regards,
Bernhard

>>+    { 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? 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;