Am 27. April 2023 10:52:17 UTC schrieb Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk>:
>On 26/04/2023 21:14, Bernhard Beschow wrote:
>
>> Am 26. April 2023 18:18:35 UTC schrieb Bernhard Beschow <shen...@gmail.com>:
>>>
>>>
>>> Am 26. April 2023 11:37:48 UTC schrieb Mark Cave-Ayland
>>> <mark.cave-ayl...@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 <shen...@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.
