Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

[BALATON Zoltan]

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