Re: Problems with irq mapping in qemu v5.2

[Guenter Roeck]

On 12/23/20 8:09 AM, Mark Cave-Ayland wrote:
> On 23/12/2020 15:21, Philippe Mathieu-Daudé wrote:
> 
>> On 12/22/20 5:16 PM, Guenter Roeck wrote:
>>> Hi,
>>>
>>> commit 459ca8bfa41 ("pci: Assert irqnum is between 0 and bus->nirqs in
>>> pci_bus_change_irq_level") added sanity checks to the interrupt number 
>>> passed
>>> to pci_bus_change_irq_level(). That makes sense, given that bus->irq_count
>>> is indexed and sized by the number of interrupts.
>>>
>>> However, as it turns out, the interrupt number passed to this function
>>> is the _mapped_ interrupt number. The result in assertion failures for 
>>> various
>>> emulations.
>>>
>>> Examples (I don't know if there are others):
>>>
>>> - ppc4xx_pci_map_irq() maps the interrupt number to "slot - 1". Obviously
>>>    that isn't a good thing to do for slot 0, and indeed results in an
>>>    assertion as soon as slot 0 is initialized (presumably that is the root
>>>    bridge). Changing the mapping to "slot" doesn't help because valid slots
>>>    are 0..4, and only four interrupts are allocated.
>>> - pci_bonito_map_irq() changes the mapping all over the place. Whatever
>>>    it does, it returns numbers starting with 32 for slots 5..12. With
>>>    a total number of 32 interrupts, this again results in an assertion
>>>    failure.
>>>
>>> ppc4xx_pci_map_irq() is definitely buggy. I just don't know what the
>>> correct mapping should be. slot  & 3, maybe ?
>>>
>>> I don't really have a good solution for pci_bonito_map_irq(). It may not
>>> matter much - I have not been able to boot fuloong_2e since qemu v4.0,
>>> and afaics that is the only platform using it. Maybe it is just completely
>>> broken ?
>>
>> FWIW bisecting Fuloong2E starts failing here:
>>
>> 4ea98d317eb442c738f898f16cfdd47a18b7ca49 is the first bad commit
>> commit 4ea98d317eb442c738f898f16cfdd47a18b7ca49
>> Author: BALATON Zoltan <balaton@eik.bme.hu>
>> Date:   Fri Jan 25 14:52:12 2019 -0500
>>
>>      ide/via: Implement and use native PCI IDE mode
>>
>>      This device only implemented ISA compatibility mode and native PCI IDE
>>      mode was missing but no clients actually need ISA mode but to the
>>      contrary, they usually want to switch to and use device in native
>>      PCI IDE mode. Therefore implement native PCI mode and switch default
>>      to that.
>>
>>      Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>      Message-id:
>> c323f08c59b9931310c5d92503d370f77ce3a557.1548160772.git.balaton@eik.bme.hu
>>      Signed-off-by: John Snow <jsnow@redhat.com>
>>
>>   hw/ide/via.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 38 insertions(+), 14 deletions(-)
> 
> I think the original version of the patch broke fuloong2e, however that 
> should have been fixed by my patchset here: 
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03936.html. It might 
> be that there are multiple regressions located during a full bisect :/
> 

Not really. The following patch on top of qemu 5.2 results in the ide drive
being detected and working.

diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b33..1bfdc422ee 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -186,11 +186,14 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);

     bmdma_setup_bar(d);
+#if 0
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+#endif

     qdev_init_gpio_in(ds, via_ide_set_irq, 2);
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+        ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0, i ? 0x376 : 
0x3f6);
         ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));

         bmdma_init(&d->bus[i], &d->bmdma[i], d);

With the added ide_init_ioport(), the drive is detected. With the #if 0,
it actually starts working. So there are two problems: 1) The qemu ide
subsystem isn't informed about the io addresses, and 2) bmdma isn't working.

Guenter