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

[Kevin Wolf <kwolf@redhat.com>]

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-ayland@ilande.co.uk>
> Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Tested-by: Bernhard Beschow <shentey@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.

> +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.

> +
> +        /* 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?

> +        /* 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.

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.

> +        /* 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.

> +    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