|
From: | BALATON Zoltan |
Subject: | Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode |
Date: | Fri, 6 Mar 2020 13:40:26 +0100 (CET) |
User-agent: | Alpine 2.22 (BSF 395 2020-01-19) |
On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
On 06/03/2020 00:21, BALATON Zoltan wrote:On Fri, 6 Mar 2020, BALATON Zoltan wrote:On Thu, 5 Mar 2020, Mark Cave-Ayland wrote:On 04/03/2020 22:33, BALATON Zoltan wrote: another possibility: PCI configuration space register 0x3d (Interrupt pin) is documented as having value 0 == Legacy IRQ routing which should be the initial value on reset, but QEMU incorrectly sets it to 1 which indicates PCI IRQ routing.The VT8231 docs say this should always read 1 but may be this is somehow set to 0 on the Pegasos2. What does that mean? Should we use this value instead of the feature bit to force using legacy interrupts? We'd still need a property in via-ide to set this reg or is it possible to set it from board code overriding the default after device is created? That would allow to drop patch 1. I can try this.This seemed like it could simplify patches a bit but it does not work. Setting this reg to 0 breaks Linux and MorphOS which then think the device does not have an interrupt at all and fail as before waiting for the irq. So we still need the feature bit, cant use this reg to force legacy interrupts. I've spent considerable time testing different OSes before I've ended up with this patch series I've submitted and I could not find a simpler way that works with everything.I appreciate that testing these things can take a lot of time, but what is important thing to ask here is whether these hacks are attempting to work around something in QEMU that doesn't match the hardware specification, and to me it feels that this is what is happening here.
It may be we need to work around some incomplete modelling of devices in QEMU, e.g. we only model the native mode of these IDE interfaces so anything involving legacy mode is out of scope. To also emulate legacy mode we'd need changing common ISA code and maybe PIC code as well. As those parts are also used by other more commonly used machine models I'd avoid breaking those and rather implement it confined to these machines that are not yet finished or complete anyway than try to change all dependent devices that would need even more testing. These "hacks" could be cleaned up later and this would not be the only hack in QEMU, I don't have time to fix everything and it's unreasonable to demand it I think. I'd suggest to take this patch as it is now and if you don't like it you can submit patches that clean it up the way you think is correct or submit an alternative patch now that shows how do you think it can be done in a cleaner way because I don't see it and don't have more time for it now.
Obviously this thread has become quite long (and even I'm struggling to find previous discussions) but here is my summary below: - I don't think the patch in its current form is the right way to do this. Instead of adding a feature bit to fudge the existing IRQ routing when the existing IRQ routing is wrong, let's fix the existing IRQ routing instead.
I think that would involve changing parts which could break other machines so I'd rather go with a featute bit only affecting pegasos2 and fulonge2 than touch i8259 or ISA emulation basing that on some guess how the real chip may be implemented. Is it possible to implement what you propose without changing common IDE, ISA and PIC emulation only in via-ide and fulong2e code?
- There is no mention of "non-100%" native mode in the 8231 or 686B datasheet: this is simply a term used within the Linux patches. The controller is either in native mode, or legacy mode. It may be that guests are making use of some undefined behaviour here.
Yes, this is a Linux term and Linux also uses a feature bit to enable this workaround. If that's good enough for Linux why isn't it good enough for you?
- The code that uses the value of PCI_INTERRUPT_LINE in via-ide is incorrect (as your patch comment points out, some guests ignore it anyway).
You're misunderstanding the comment. The via_ide_config_read function is needed to restore value in interrupt line that common PCI reset code deletes. Linux depends on this value to be the same as on real hardware so this is needed to work around QEMU and Linux pecularities.
I've tried using PCI_INTERRUPT_PIN in place of the feature bit but setting that to 0 breaks Linux and MorphOS on pegasos2 because these apparently expect this to be set to 1 corresponding to native mode. (Firmware only sets native mode enable bits in prog-if but datasheet says this reg should be 1 by default and other PCI docs say 0 here means no interrupt used so maybe that's why Linux and MorphOS don't like it.)
- There is different behaviour here between the 8231 and 686B in this area, despite having the same vendor/device id. The first thing you need to fix is the PCI_INTERRUPT_LINE part; I would start by removing the existing code and instead expose a qdev named gpio "legacy-irq" and wiring that up to your interrupt controller. Note you'd have to do the same for fulong2e, but that is reasonably trivial.
Please go ahead and do it but if you don't submit an alternative patch before the freeze I'd ask John and Peter to make a judgement here and tell if my series is acceptable or not in its current form and if it is then please merge it and leave clean ups for subsequent patches. This is blocking my further patches to implement pegasos2 emulation.
Regards, BALATON Zoltan
[Prev in Thread] | Current Thread | [Next in Thread] |