| Message ID | 20230422150728.176512-11-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up PCI IDE device models | expand |
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... > 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.
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" > >> 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.
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." > >> >>> 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.
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.
Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >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. Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. > >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. I was hoping to keep that patch... Best regards, Bernhard > >>>> >>>>> 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.
Am 27. April 2023 18:15:24 UTC schrieb Bernhard Beschow <shentey@gmail.com>: > > >Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>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. > >Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. > >> >>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. > >I was hoping to keep that patch... The whole paragraph reads: "PIIX4 claims all accesses to these ranges, if enabled. The byte enables do not have to be externally decoded to assert DEVSEL#. Accesses to byte 3 of the Control Block are forwarded to ISA where the floppy disk controller responds." So PIIX doesn't look at the individual io ports but rather at the whole blocks covering them. To me, this sounds like PIIX is applying the PCI IDE controller specification without the native option. In QEMU the block part of the specification is implemented by cmd_bar and data_bar. I think that reusing the blocks here in fact models the PIIX datasheet closer than the current implementation. I'd therefore keep this patch. Best regards, Bernhard > >Best regards, >Bernhard > >> >>>>> >>>>>> 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.
On Fri, 28 Apr 2023, Bernhard Beschow wrote: > Am 27. April 2023 18:15:24 UTC schrieb Bernhard Beschow <shentey@gmail.com>: >> Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>> 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. >> >> Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. >> >>> >>> 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. >> >> I was hoping to keep that patch... > > The whole paragraph reads: "PIIX4 claims all accesses to these ranges, > if enabled. The byte enables do not have to be externally decoded to > assert DEVSEL#. Accesses to byte 3 of the Control Block are forwarded to > ISA where the floppy disk controller responds." So PIIX doesn't look at > the individual io ports but rather at the whole blocks covering them. I don't mind either way if this patch is dropped or not, I could imagine there could be reasons for both decisions. Also this is changing piix which I'm not concerned with but still commenting on this hoping to help to reach a decision. > To me, this sounds like PIIX is applying the PCI IDE controller > specification without the native option. What is that? Isn't "PCI IDE without native option" just legacy IDE i.e. fixed io regions and 14/15 IRQs? If so then it could be either modelled with ISA functions as it is now or you could reuse code from PCI IDE which would normally allow to set these io regions via BARs but not using thar here and just hard coding it to legacy ports. > In QEMU the block part of the > specification is implemented by cmd_bar and data_bar. I think that > reusing the blocks here in fact models the PIIX datasheet closer than > the current implementation. I'd therefore keep this patch. I'm not sure PIIX follows the PCI IDE spec and you may just try too hard to reuse code from an unrelated model here that happens to match as both are following legacy ISA IDE for compatibility. That's not necessarily a problem, but just call it what it is and keep the naming consistent to PCI IDE where these are BARs but are not used as such here. You can add a comment to note this if it disturbs you to explain why these are hardcoded here. On the other hand if we consider this a legacy device that should be modelled accordingly then this patch may not be needed which is I think what Mark pointed out. Now the question is if we want to reduce the use of legacy ISA IDE functions hoping we could get rid of it eventually so we go with reusing a slightly unrelated PCI IDE model which would work here and would allow more code sharing; or we want to reduce code churn and keep the model as it is which is already modelling the device as legacy IDE. I can't decide on that. Dropping the patch reduces code churn and may be slighly cleaner but keeping it may allow more code reuse and potentially removing some legacy ISA stuff which may allow simplifying that part, even if it may need a comment here to explain that more here. Both sides have merrits and don't know who could be the one to make a decison. Maybe the maintainer of the code part? (Oops, it does not really have one... Then no idea.) I don't know this well and did not follow it too deeply so in case I've completely misunderstood it or missed an important detail then just disregard all of the above. Regards, BALATON Zoltan
On 27/04/2023 19:15, Bernhard Beschow wrote: > Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >> 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. > > Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. Can you explain a bit more about where you see the contradiction? At first glance it looks okay to me. >> 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. > > I was hoping to keep that patch... Perhaps a different way to think about it is that from QEMU's perspective a BAR is a MemoryRegion that can be dynamically assigned/updated and cannot overlap, whereas the portio_list implementation also handles unaligned accesses and overlapping sparse accesses. Since the latter is the exact case here with the IDE/FDC then it seems the existing portio_list solution already does the "right thing" instead of having to manually emulate the overlapping dispatch. ATB, Mark.
Am 3. Mai 2023 19:52:41 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 27/04/2023 19:15, Bernhard Beschow wrote: > >> Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>> 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. >> >> Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. > >Can you explain a bit more about where you see the contradiction? At first glance it looks okay to me. > >>> 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. >> >> I was hoping to keep that patch... > >Perhaps a different way to think about it is that from QEMU's perspective a BAR is a MemoryRegion that can be dynamically assigned/updated and cannot overlap, whereas the portio_list implementation also handles unaligned accesses and overlapping sparse accesses. Since the latter is the exact case here with the IDE/FDC then it seems the existing portio_list solution already does the "right thing" instead of having to manually emulate the overlapping dispatch. I've had another look into the "PCI IDE Controller Specification Revision 1.0" which says: "The Control Block registers consist of two bytes used for control/status of the IDE device. The second byte of this pair is read-only and has the interesting quirk where the top bit of this byte is shared with the floppy controller when the IDE device is mapped at 'compatibility' locations. It turns out that software controlling IDE devices (BIOS, drivers, etc.) does not use this register at all. The exception for PCI IDE controllers to the ATA Standard is that only the first of the two bytes defined in the Control Block registers is implemented. This byte provides Alternate Status on reads and Device Control on writes. Accesses to the second byte of the Control Block registers (Drive Address) should be ignored by the PCI IDE controller." So in fact the real PIIX does adhere to this standard and there is no reason to reject the idea behind this patch -- which is to make our PIIX device model implement this standard. It's just that all our other PCI-IDE implementations need to implement this quirk as long as they implement the standard. And according to the Linux kernel they all do -- see its CONFIG_ATA_SFF. Since this patch actually uncovered a small bug in the other device models I'd rather fix those, too. One way I could do this is to decrease the size of the memory region or to map with lower priority. What is the preferred fix? Any other ideas? Note that this and the next patch resolve the last dependencies on the "isabus" global. So after this series we could apply some small patches posted before and get rid of the global entirely... And have as many ISA and LPC buses as we want! Best regards, Bernhard > > >ATB, > >Mark. >
On 13/05/2023 13:21, Bernhard Beschow wrote: > Am 3. Mai 2023 19:52:41 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >> On 27/04/2023 19:15, Bernhard Beschow wrote: >> >>> Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>>> 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. >>> >>> Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. >> >> Can you explain a bit more about where you see the contradiction? At first glance it looks okay to me. >> >>>> 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. >>> >>> I was hoping to keep that patch... >> >> Perhaps a different way to think about it is that from QEMU's perspective a BAR is a MemoryRegion that can be dynamically assigned/updated and cannot overlap, whereas the portio_list implementation also handles unaligned accesses and overlapping sparse accesses. Since the latter is the exact case here with the IDE/FDC then it seems the existing portio_list solution already does the "right thing" instead of having to manually emulate the overlapping dispatch. > > I've had another look into the "PCI IDE Controller Specification Revision 1.0" which says: Interesting: it looks as if we are getting different conclusions from the same document. > "The Control Block registers consist of two bytes used for control/status of the IDE device. The second byte of this pair is read-only and has the interesting quirk where the top bit of this byte is shared with the floppy controller when the IDE device is mapped at 'compatibility' locations. It turns out that software controlling IDE devices (BIOS, drivers, etc.) does not use this register at all. Just before this section the start of the paragraph reads "The ATA Standard defines two sets of registers known as Control Block Registers and Command Block Registers." which reads to me that the paragraph quoted above is describing the original ATA Standard behaviour, i.e. the expected behaviour for pre-PCI controllers or PCI IDE controllers in compatibility mode. > The exception for PCI IDE controllers to the ATA Standard is that only the first of the two bytes defined in the Control Block registers is implemented. This byte provides Alternate Status on reads and Device Control on writes. Accesses to the second byte of the Control Block registers (Drive Address) should be ignored by the PCI IDE controller." And this paragraph then leads onto the differences for PCI IDE controllers which are that the second (shared) byte in the Control Block is ignored, which again makes sense from a PCI perspective since PCI BARs cannot overlap. But that doesn't matter in PCI native mode because the BIOS/OS will have moved the BAR to a suitable memory address that doesn't clash with the floppy drive. > So in fact the real PIIX does adhere to this standard and there is no reason to reject the idea behind this patch -- which is to make our PIIX device model implement this standard. > > It's just that all our other PCI-IDE implementations need to implement this quirk as long as they implement the standard. And according to the Linux kernel they all do -- see its CONFIG_ATA_SFF. Another couple of hints that the registers in PCI IDE controllers in compatibility mode aren't accessed through PCI BARs can also be found: i) the table in section 2.1 for compatibility mode uses fixed addresses whilst the table in section 2.2 references BAs and ii) section 2.4 suggests that PCI controllers in compatibility mode always ignore the BARs. Now it could be that the description in the PIIX datasheet indicates that the PCI IO address is hardcoded and then the second byte (re)dispatched to the ISA bus, but then I would argue that this is an implementation detail: from QEMU's perspective there is zero difference between this and the existing IDE portio_list, and as a bonus the existing compatibility behaviour is completely unaffected by any PCI BARs. > Since this patch actually uncovered a small bug in the other device models I'd rather fix those, too. One way I could do this is to decrease the size of the memory region or to map with lower priority. What is the preferred fix? Any other ideas? > > Note that this and the next patch resolve the last dependencies on the "isabus" global. So after this series we could apply some small patches posted before and get rid of the global entirely... And have as many ISA and LPC buses as we want! This is the part I think we can do better with: both Phil and I have patches that remove the isabus reference from the IDE ioports e.g. https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-9-philmd@linaro.org/ so dropping this patch shouldn't affect our ability to remove the isabus global. Do you have an example of a use-case you have for multiple ISA buses? I'm fairly sure that this wouldn't work on x86 PC machines with a single PCI root bus for example. ATB, Mark.
Am 18. Mai 2023 14:53:26 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >On 13/05/2023 13:21, Bernhard Beschow wrote: > >> Am 3. Mai 2023 19:52:41 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>> On 27/04/2023 19:15, Bernhard Beschow wrote: >>> >>>> Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >>>>> 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. >>>> >>>> Yes, PIIX assuming that might be the case. Why does it contradict the PCI IDE specification? PIIX seems to apply the apprppriate "workarounds" here. >>> >>> Can you explain a bit more about where you see the contradiction? At first glance it looks okay to me. >>> >>>>> 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. >>>> >>>> I was hoping to keep that patch... >>> >>> Perhaps a different way to think about it is that from QEMU's perspective a BAR is a MemoryRegion that can be dynamically assigned/updated and cannot overlap, whereas the portio_list implementation also handles unaligned accesses and overlapping sparse accesses. Since the latter is the exact case here with the IDE/FDC then it seems the existing portio_list solution already does the "right thing" instead of having to manually emulate the overlapping dispatch. >> >> I've had another look into the "PCI IDE Controller Specification Revision 1.0" which says: > >Interesting: it looks as if we are getting different conclusions from the same document. > >> "The Control Block registers consist of two bytes used for control/status of the IDE device. The second byte of this pair is read-only and has the interesting quirk where the top bit of this byte is shared with the floppy controller when the IDE device is mapped at 'compatibility' locations. It turns out that software controlling IDE devices (BIOS, drivers, etc.) does not use this register at all. > >Just before this section the start of the paragraph reads "The ATA Standard defines two sets of registers known as Control Block Registers and Command Block Registers." which reads to me that the paragraph quoted above is describing the original ATA Standard behaviour, i.e. the expected behaviour for pre-PCI controllers or PCI IDE controllers in compatibility mode. > >> The exception for PCI IDE controllers to the ATA Standard is that only the first of the two bytes defined in the Control Block registers is implemented. This byte provides Alternate Status on reads and Device Control on writes. Accesses to the second byte of the Control Block registers (Drive Address) should be ignored by the PCI IDE controller." > >And this paragraph then leads onto the differences for PCI IDE controllers which are that the second (shared) byte in the Control Block is ignored, which again makes sense from a PCI perspective since PCI BARs cannot overlap. But that doesn't matter in PCI native mode because the BIOS/OS will have moved the BAR to a suitable memory address that doesn't clash with the floppy drive. > >> So in fact the real PIIX does adhere to this standard and there is no reason to reject the idea behind this patch -- which is to make our PIIX device model implement this standard. >> >> It's just that all our other PCI-IDE implementations need to implement this quirk as long as they implement the standard. And according to the Linux kernel they all do -- see its CONFIG_ATA_SFF. > >Another couple of hints that the registers in PCI IDE controllers in compatibility mode aren't accessed through PCI BARs can also be found: i) the table in section 2.1 for compatibility mode uses fixed addresses whilst the table in section 2.2 references BAs and ii) section 2.4 suggests that PCI controllers in compatibility mode always ignore the BARs. > >Now it could be that the description in the PIIX datasheet indicates that the PCI IO address is hardcoded and then the second byte (re)dispatched to the ISA bus, but then I would argue that this is an implementation detail: from QEMU's perspective there is zero difference between this and the existing IDE portio_list, and as a bonus the existing compatibility behaviour is completely unaffected by any PCI BARs. Right, that was my idea: Trading one implementation detail in PIIX with another to have a common "theory" accross all our TYPE_PCI_IDE devices which all implement the PCI IDE controller standard. At the same time quite a bit of redundant code could be removed. We could of course extend this theory to consider compatibility and PCI native modes to have different implementations. That is, compatibility mode would use portio_list semantics while PCI native woulde use the BARs semantics. Then we'd have to make at least TYPE_VIA_IDE use portio_list since it is currently hardcoded to operate in compatibility mode. Same for cmd646 if it can switch modes. And then there is also sil3112... I guess I'll split this series and only ship the first four patches in v2. Meanwhile we can discuss here further on the topic of the PCI IDE controller specification which the rest of this series addresses. > >> Since this patch actually uncovered a small bug in the other device models I'd rather fix those, too. One way I could do this is to decrease the size of the memory region or to map with lower priority. What is the preferred fix? Any other ideas? >> >> Note that this and the next patch resolve the last dependencies on the "isabus" global. So after this series we could apply some small patches posted before and get rid of the global entirely... And have as many ISA and LPC buses as we want! > >This is the part I think we can do better with: both Phil and I have patches that remove the isabus reference from the IDE ioports e.g. https://patchew.org/QEMU/20230302224058.43315-1-philmd@linaro.org/20230302224058.43315-9-philmd@linaro.org/ so dropping this patch shouldn't affect our ability to remove the isabus global. > >Do you have an example of a use-case you have for multiple ISA buses? I'm fairly sure that this wouldn't work on x86 PC machines with a single PCI root bus for example. The ICH0 (82801AA & 82801AB) datasheet depicts in the "system block diagram" an LPC bus for the Super I/O devices and an optional PCI-ISA bridge connected to the PCI bus. I guess the use case is for connecting ISA cards while the ICH made the switch to the more modern LPC bus. Of course one could attach an ISA "card" to the LPC bus in QEMU since they look the same from software. I'd assume though that above two buses would be visible somehow. Other than that I don't see a real use case. It's just a property of a clean software design to avoid imposing any artificial limits onto callers and instead give them maximum control. ISABus currently assumes that there can only be one ISA bus in a QEMU process and above example seems like a counter example. Moreover, ISABus not being a singleton may make its testing more convenient and who knows, maybe it helps in moving towards heterogenious architectures. Best regards, Bernhard > > >ATB, > >Mark. >
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); 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); } }
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(-)