Re: [PATCH 10/13] hw/ide/piix: Reuse PCIIDEState::{cmd,data}_ops

[Mark Cave-Ayland]

On 26/04/2023 21:14, Bernhard Beschow wrote:

> Am 26. April 2023 18:18:35 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland 
> > <mark.cave-ayland@ilande.co.uk>:
> > > On 22/04/2023 16:07, Bernhard Beschow wrote:
> > >
> > > > Now that PCIIDEState::{cmd,data}_ops are initialized in the base class
> > > > constructor there is an opportunity for PIIX to reuse these attributes. This
> > > > resolves usage of ide_init_ioport() which would fall back internally to using
> > > > the isabus global due to NULL being passed as ISADevice by PIIX.
> > > >
> > > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > > ---
> > > >    hw/ide/piix.c | 30 +++++++++++++-----------------
> > > >    1 file changed, 13 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > > index a3a15dc7db..406a67fa0f 100644
> > > > --- a/hw/ide/piix.c
> > > > +++ b/hw/ide/piix.c
> > > > @@ -104,34 +104,32 @@ static void piix_ide_reset(DeviceState *dev)
> > > >        pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
> > > >    }
> > > >    -static bool pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus,
> > > > -                              Error **errp)
> > > > +static void pci_piix_init_bus(PCIIDEState *d, unsigned i, ISABus *isa_bus)
> > > >    {
> > > >        static const struct {
> > > >            int iobase;
> > > >            int iobase2;
> > > >            int isairq;
> > > >        } port_info[] = {
> > > > -        {0x1f0, 0x3f6, 14},
> > > > -        {0x170, 0x376, 15},
> > > > +        {0x1f0, 0x3f4, 14},
> > > > +        {0x170, 0x374, 15},
> > > >        };
> > > > -    int ret;
> > > > +    MemoryRegion *address_space_io = pci_address_space_io(PCI_DEVICE(d));
> > > >          ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> > > > -    ret = ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> > > > -                          port_info[i].iobase2);
> > > > -    if (ret) {
> > > > -        error_setg_errno(errp, -ret, "Failed to realize %s port %u",
> > > > -                         object_get_typename(OBJECT(d)), i);
> > > > -        return false;
> > > > -    }
> > > > +    memory_region_add_subregion(address_space_io, port_info[i].iobase,
> > > > +                                &d->data_ops[i]);
> > > > +    /*
> > > > +     * PIIX forwards the last byte of cmd_ops to ISA. Model this using a low
> > > > +     * prio so competing memory regions take precedence.
> > > > +     */
> > > > +    memory_region_add_subregion_overlap(address_space_io, port_info[i].iobase2,
> > > > +                                        &d->cmd_ops[i], -1);
> > >
> > > Interesting. Is this behaviour documented somewhere and/or used in one of your 
> > > test images at all? If I'd have seen this myself, I probably thought that the 
> > > addresses were a typo...
> >
> > I first  stumbled upon this and wondered why this code was working with VIA_IDE (through 
> > my pc-via branch). Then I found the correct offsets there which are confirmed in the piix 
> > datasheet, e.g.: "Secondary Control Block Offset: 0374h"
>
> In case you were wondering about the forwarding of the last byte the datasheet says: 
> "Accesses to byte 3 of the Control Block are forwarded to ISA where the floppy disk 
> controller responds."

Ahhh okay okay I see what's happening here: the PIIX IDE is assuming that the legacy 
ioport semantics are in operation here, which as you note above is where the FDC 
controller is also accessed via the above byte in the IDE control block. This is also 
why you need to change the address above from 0x3f6/0x376 to 0x3f4/0x374 when trying 
to use the MemoryRegions used for the PCI BARs since the PCI IDE controller 
specification requires a 4 byte allocation for the Control Block - see sections 2.0 
and 2.2.

And that's fine, because the portio_lists used in ide_init_ioport() set up the legacy 
IDE ioports so that FDC accesses done in this way can succeed, and the PIIX IDE is 
hard-coded to legacy mode. So in fact PIIX IDE should keep using ide_init_ioport() 
rather than trying to re-use the BAR MemoryRegions so I think this patch should just 
be dropped.

> > > >        ide_bus_init_output_irq(&d->bus[i],
> > > >                                isa_bus_get_irq(isa_bus, port_info[i].isairq));
> > > >          bmdma_init(&d->bus[i], &d->bmdma[i], d);
> > > >        ide_bus_register_restart_cb(&d->bus[i]);
> > > > -
> > > > -    return true;
> > > >    }
> > > >      static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
> > > > @@ -160,9 +158,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error 
> > > > **errp)
> > > >        }
> > > >          for (unsigned i = 0; i < 2; i++) {
> > > > -        if (!pci_piix_init_bus(d, i, isa_bus, errp)) {
> > > > -            return;
> > > > -        }
> > > > +        pci_piix_init_bus(d, i, isa_bus);
> > > >        }
> > > >    }


ATB,

Mark.