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

[Mark Cave-Ayland]

On 22/10/2023 23:06, Bernhard Beschow wrote:

> 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[] = {
> > +    { 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).

I can certainly do that for now, although my latest experiments suggest that the BM 
DMA BAR will need to be disabled in future when using legacy mode.

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

Well as mentioned in the cover letter this is an extract from a future series which 
should work for all PCI IDE controllers, but for now I've kept it limited to the 
via-ide device. This allows the basic concept to be tested (particularly for Zoltan's 
PPC machines) without having to worry about introducing regressions that can affect 
other controllers.

In future the aim is to allow it to be used for all PCI IDE controllers, but looking 
at what I have now I don't think I'll be able to get everything ready and tested for 
8.2. So watch this space :)

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

I'd be fairly confident this is the case, since the PIIX-IDE device is hard-coded to 
legacy mode which means that the PCI interrupt pins aren't used (the values 1-4 
represent INTA-INTD here).

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