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 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.

For CMD646 we only use PCI interrupt which is in PCIDevice. Its legacy 
mode and thus those pins are not modelled so not needed now. For via-ide 
we only use ISA interrupts because even if we don't model legacy mode, 
boards expect ISA interrupts also in native mode maybe because this 
controller is not a separate PCI device only found embedded in 
southbridge/superio chips where they connect to the also embedded ISA PICs 
so even in native mode it should raise one of the ISA IRQs. My patch 
accesses ISA irqs with isa_get_irq() so no gpios and legacy irqs in 
PCIIDEState is neeeded and I don't see the need to introduce this 
complexity here. Also newer PCI ATA and SATA controllers such as sii3112 
do not have a legacy mode so I'd keep things related to that out of common 
PCI IDE code and model it instead in the controllers that have this as 
this does not seem to belong to PCI IDE.

> > > 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.

But it's not done that way on real hardware and via-ide is not a PCI 
device but all this is internal to the VT8231 chip, the PICs, via-ide and 
a lot of other things which are modelled in QEMU by reusing existing 
components but I think we don't want to model the internal of the chip 
down to the smallest detail. In via-ide's case the PCI_INTERRUPT_LINE is 
probably not used by the IDE controller function but is used by some other 
function of the southbridge chip that implements interrupt controller but 
we don't have a separate model of that in QEMU so we can just emulate this 
function within via-ide which I think is OK as this IDE controller is part 
of these southbridges and not used anywhere else. This partly mixes IDE 
controller function and interrupt controller function but probably OK 
until we want to model this southbridge in more detail which could be done 
in later clean up patches. The piix model seems to do the same embedding a 
8259 which even less belongs to an IDE controller and acceses irqs array 
directly.

> 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.

Correct or not or follows the spec or not this is how it works on real 
hardware and to get guests running we need to emulate this. Again, this 
controller is embedded in the 868B and 8231 southbridge chips so they may 
not completely confirm to PCI spec but their own datasheets. We can argue 
in how much detail do we want to model the internals of these chips (which 
we don't know for sure) but I think this patch is good enough for now and 
could be refined later or we likely won't be able to finish this before 
the freeze.

Another way to look at it is that this patch gets some guests running and 
does not break anything as far as I know so is there anything in it that's 
unacceptable now and needs to be changed for it to be merged? Unless you 
can propose a way to achieve the same in a simpler way now I think we 
could go with this and you can submit follow up patches to clean it up as 
you like later.

Regards,
BALATON Zoltan