On 06/03/2020 12:40, BALATON Zoltan wrote: > 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.
I believe I've answered this in detail in my previous email, so I suggest we keep the discussion there so it's all in one place. ATB, Mark.
