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

[Mark Cave-Ayland]

On 06/03/2020 19:38, BALATON Zoltan wrote:

> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>> On 06/03/2020 12:06, BALATON Zoltan wrote:
>>> On Fri, 6 Mar 2020, Mark Cave-Ayland wrote:
>>>> On 05/03/2020 23:35, BALATON Zoltan wrote:
>>>>> On real hardware this may be true but in QEMU how would it otherwise 
>>>>> raise the
>>>>> correct interrupt line the guest expects? This probably does not matter 
>>>>> for
>>>>> pegasos2
>>>>> but I think is needed for 100% native mode used with the fulong2e so it 
>>>>> gets the
>>>>> IRQ
>>>>> it expects.
>>>>
>>>> That's easy - remember that both the PCI and IRQ interrupts are separate 
>>>> pins on the
>>>> chip, so all that needs to be done is expose the legacy IRQ via qdev and 
>>>> use that to
>>>> wire it up to your interrupt controller.
>>>
>>> This "chip" is part of an integrated southbridge/superio/everything chip 
>>> the also
>>> includes the two PICs and how they are internally connected is not known so 
>>> we would
>>> be guessing here anyway. I don't see a need to make it more complicated 
>>> than it is
>>> now by modeling internal pins but how would I wire up gpio to the i8259 
>>> model and
>>> where should I connect the PCI irq?
>>
>> For now I would say not to worry about the PCI IRQ: the reason for 
>> discussing this
>> before was because we believed that if the controller was in native mode it 
>> must be
>> using the IRQ in PCI_INTERRUPT_LINE. But from yesterday's read of the 
>> specification
>> we know that PCI_INTERRUPT_LINE is never used by the device itself, and so 
>> given that
>> the existing via-ide device doesn't currently attempt to use the PCI IRQ in
>> via_ide_set_irq() then we should be good.
>>
>> If someone had a machine somewhere that did use the PCI IRQ then it would 
>> need
>> investigation, but since there isn't then I don't see any need to do this 
>> now.
>>
>>>> Okay so this is interesting: I've been switching between the VT8231 and the
>>>> VT82C686B
>>>> datasheets, and there is a difference here. You are correct in what you 
>>>> say above in
>>>> that the 8231 docs specify that this is set to 1, but on the 686B this is 
>>>> clearly
>>>> not
>>>> the case.
>>>
>>> The 82C686B says this reg can be 0 or 1, where 0 is legacy interrupt 
>>> routing and 1 is
>>> native mode. Given that we only model native mode of the chip it does not 
>>> make sense
>>> to set it to anything else than 1 and setting it to 0 confuses MorphOS and 
>>> Linux on
>>> pegasos2 while setting it to 1 works with everything I've tried both on 
>>> pegasos2 and
>>> fulong2e even if that may not completely match how it's implemented in 
>>> hardware.
>>>
>>>> What is rather unusual here is that both the 8231 and 686B have exactly 
>>>> the same
>>>> device and vendor ids, so I'm not sure how you'd distinguish between them?
>>>
>>> Guests distinguish by looking at the parent device (function 0) which is 
>>> the chip
>>> this IDE device is part of (on function 1).
>>
>> Okay thanks, that's useful to know.
>>
>> I've done a quick grep of the source tree and AFAICT the only IDE controller 
>> that
>> tries to use the PCI_INTERRUPT_LINE register is via-ide, which means this 
>> should be
>> fairly easy. In short:
>>
>> 1) Add qemu_irq legacy_irqs[2] into PCIIDEState
>>
>> (You could argue that it should belong in a separate VIAIDEState, however 
>> quite a few
>> of the BMDMA controllers in QEMU don't have their own device state and just 
>> use
>> PCIIDEState. And whilst via-ide is the only one that currently needs support 
>> for
>> legacy IRQs, I think it's good to put it there in case other controllers 
>> need it in
>> future)
>>
>> 2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() 
>> with a
>> name of "legacy-irq" to it
> 
> I don't like this. This adds two via-ide specific data to to common PCI IDE 
> code
> where it does not belong and subclassing it just for this also seems to be 
> more
> changes than really needed. Reusing the existing CMD646 field and 
> generalising it to
> allow implementation specific feature flags seems much less intrusive and not 
> less
> clean than your proposal.

It's not VIA-specific though: the ISA legacy and PCI buses have different 
electrical
characteristics and so by definition their signals must be driven by separate
physical pins. Have a look at the CMD646 datasheet for example, and you will 
see that
separate pins exist for legacy and PCI native IRQs.

>> 3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing 
>> pci_create_simple()
>> to pci_create() because the device shouldn't be realised immediately
>>
>> 4) In vt82c686b_southbridge_init use qdev_connect_gpio_out_named() to connect
>> legacy_irq[0] to 8259 IRQ 14 and legacy_irq[1] to 8259 IRQ 15, and then 
>> realise the
>> device
> 
> How do I connect gpios to 8259 interrupts? That seems to be internal state of 
> 8259
> that I'm not sure how to access cleanly from code instantiating it. Is this 
> better
> than my patch? It seems it achieves the same via-ide specific behaviour just 
> in a
> more complicated way and would still need the feature bit to know when to use
> legacy_irq[1].

We know from the PCI specification that the existing code for via-ide is 
incorrect,
and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE 
in
this way then both of these strongly suggest that current via-ide 
implementation is
wrong. Rather than add a hack on top of a hack then the simplest solution is to
physically wire the IRQ pin using qdev at the board level, as is done on real 
hardware.

Looking at the code it seems that i8259_init() returns the PIC IRQ array so it 
should
just be a case of returning the nth entry and using that with 
qdev_init_gpio_out_named().

>> 5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead 
>> just do
>> qemu_set_irq() on legacy_irq[0] (in theory I guess it should be 
>> legacy_irq[n] but it
>> seems that both drives on MIPS and Pegasos both use IRQ 14).
> 
> According to the 8231 datasheet in legacy mode (and on pegasos2's half-native 
> mode)
> the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% 
> native
> mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. 
> The 686B
> datasheet does not detail this but I believe it works the same. Since we 
> currently
> fixed the native mode interrupt to 14 to work around pegasos2 firmware and 
> QEMU PCI
> reset interactions using legacy_irq[0] might work but is not correct, the 
> current way
> using PCI_INTERRUPT_LINE is better modelling what hardware does in my opinion.

This is not correct though: please re-read my previous email which quotes from 
the
PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon 
the
device itself, and is merely advisory to the OS. Possibly the MIPS BIOS could be
placing a value other than 14 there, but if it does then that suggests the 
board IRQs
are physically wired differently which again should be handled at board level 
by qdev.


ATB,

Mark.