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

[Bernhard Beschow]

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