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

[Mark Cave-Ayland]

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.