[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode
Date: Sat, 7 Mar 2020 16:56:14 +0100 (CET)
User-agent: Alpine 2.22 (BSF 395 2020-01-19)

On Sat, 7 Mar 2020, Mark Cave-Ayland wrote:
On 06/03/2020 22:59, BALATON Zoltan wrote:
I've done a quick grep of the source tree and AFAICT the only IDE controller 
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 
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

2) Add via_ide_initfn() to hw/ide/via.c and add qdev_init_gpio_out_named() with 
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 
clean than your proposal.
It's not VIA-specific though: the ISA legacy and PCI buses have different 
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 
those pins are not modelled so not needed now.

Correct. It seems to me that having it there in PCIIDEState provides a 
place for all controllers that support legacy mode to store the IRQ in case 
would like to add legacy support to the device at a later date.

Why introduce it now and not only when someone would like to add legacy mode? I guess nobody needs legacy mode so we can avoid complexity modelling that unused detail now and only add it when needed.

If you really object to this then as I mentioned above, you'll need to add a
VIAIDEState or similar and have the legacy IRQs there for the qdev gpio 

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 
embedded in southbridge/superio chips where they connect to the also embedded 
PICs so even in native mode it should raise one of the ISA IRQs.

I certainly agree with your analysis here, and that is borne out by the fact 
that you
are able to boot your OSs using your current hack with no PCI interrupts.

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.

This isn't complexity though, it is just normal qdev usage. In fact if you look 

Then qdev is probably overly complex for simple things.

isa_get_irq() it contains this large comment above the function itself:

* isa_get_irq() returns the corresponding qemu_irq entry for the i8259.
* This function is only for special cases such as the 'ferr', and
* temporary use for normal devices until they are converted to qdev.

It was a temporary hack to allow old devices to access the 8259 IRQs before 
were converted to qdev. And via-ide is a qdev device, so that's how you need to 
it up.

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 
this as this does not seem to belong to PCI IDE.

Like I said above: if you really don't want to put the legacy IRQs there then 
need to create a VIAIDEState for the qdev gpio connector.

I still think what you propose is more complex than my patch and does not achieve any cleaner model. If you object to using isa_get_irq here I can go the piix way and embed the PIC array as well (or set a pointer to that with a property somehow) so I can use isa_irq[n] rather than adding non-existing pins for this. I won't do anything though until IDE and QEMU maintainers indicate what their preference is.

3) Inline via_ide_init() into hw/mips/mips_fulong2e.c, changing 
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 

How do I connect gpios to 8259 interrupts? That seems to be internal state of 
that I'm not sure how to access cleanly from code instantiating it. Is this 
than my patch? It seems it achieves the same via-ide specific behaviour just in 
more complicated way and would still need the feature bit to know when to use

We know from the PCI specification that the existing code for via-ide is 
and given that none of the other IDE controllers in QEMU use PCI_INTERRUPT_LINE 
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

But it's not done that way on real hardware and via-ide is not a PCI device but 
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 
to model the internal of the chip down to the smallest detail.

That's not what I'm suggesting though.

I think you did not like emulating the interrupt selection in via-ide that decides which irq to raise (which is really just one if() ) and would like to push it out to board code instead connecting it via gpios. But that's also not quite correct and would not even get rid of the feature bit you also disliked. In fact we don't actually have a qdev model for the VT82C686B where all this probably belongs but I don't plan to make a detailed qdevified model for that now (could be done later in a clean up patch maybe when I'll clean up pegasos2 patches for inclusion) and since via-ide is considered part of that southbridge (and reused in similar VT8231 model) we can include the interrupt controller emulation (this one if() ) as well for now and not worry about PCI specs that don't really apply to a "chip" that's not a standalone PCI IDE controller but part of a bigger southbridge chip. It's just reuses PCI IDE emulation from QEMU where appropriate.

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

What evidence do you have for this?

The datasheet (of the IDE function) says that PCI_INTERRUPT_LINE selects the ISA interrupt that's raised in native mode for both channels and guests work if we emulate that. Since via-ide is part of the southbridge that also includes the PICs they are somehow connected inside and something selects the interrupt line within the chip but I don't know how exactly that's implemented in hardware. If not the ide part directly as you propose then some other interrupt controller part has to do this and we need to do the same somewhere in our model. Since we don't emulate the southbridge in more detail I've just put it in via-ide which seems to be a good place as it's only used in these southbridges and this could be changed later if via-ide were used elsewhere. No need to be more complex now than that I think.

we don't have a separate model of that in QEMU so we can just emulate this 
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 
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 
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 
just be a case of returning the nth entry and using that with

5) Remove the PCI_INTERRUPT_LINE logic from via_ide_set_irq() and instead just 
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 
the interrupts should be 14 and 15 so legacy_irq[n] with your way but in 100% 
mode (used on the fulong2e) it should be the one set in PCI_INTERRUPT_LINE. The 
datasheet does not detail this but I believe it works the same. Since we 
fixed the native mode interrupt to 14 to work around pegasos2 firmware and QEMU 
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 
PCI specification itself that defines PCI_INTERRUPT_LINE has *no effect* upon 
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 
to get guests running we need to emulate this.

This may be a separate issue. As you mentioned in your email most guest OSs on
Pegasos blindly ignore the PCI_INTERRUPT_LINE register since they know that the 
are hardwired. However you may also be getting caught out by the bug that the 
implementation will try and use the PCI_INTERRUPT_LINE IRQ for routing which is

It's not incorrect. It follows what the datasheet says at least for VT8231, the 686B does not detail this but the Linux driver also works on fulong2e with this so I think it's correct for that as well.

Both the 8231 and 636B datasheets mark PCI_INTERRUPT_LINE as read-only with a 
value of 0xe, so I believe that once you've done the qdev gpio part, keeping 
part of your patchset as a separate patch should fix this.

I don't get this. My patch emulates that already, what do you want to be changed here?

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

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

I don't know what else to say except other than what I have already said: both 
current via-ide IRQ implementation and the changes in your patchset are 

Why do you think so? You base this on PCI docs but those don't necessarily apply to via-ide which is not a standalone PCI device but embedded in integrated southbridges whose datasheets say what I've implemented and guests are happy with that. So I don't think it's incorrect maybe just not to your taste.

I've given you the outline of what you need to do to switch the device over to 
which agrees with the datasheet, the PCI specification, comments within the 
QEMU code
itself and the results of your experiments. The proposed change is also simple 
implement (less than an hour's work) using standard APIs.

Then please do that and submit alternative patches. I don't understand all of your proposals so it's easier if you can do it in less than an hour. It would probably take me more time. I'll test them with the clients if you provide patches or I've pushed my working version now to


so you can try as well.

At this time I don't see any point in continuing to repeat myself over and over
again: if you can make the qdev change and post an updated patchset then I'm 
happy to
review and continue this discussion, but otherwise I don't see any purpose in
continuing this thread.

I also think we're not getting anywhere so I hope John and Peter can give some advice here what is needed to get these patches merged. I've run out of time for now so I won't have time to make extensive changes to these patches and can only make reasonable changes or test what you provide.

You seem to want to force me again to qdevify everything now which I don't have time for. You did the same to my OpenBIOS patches too where you wanted me to clean up PCI code and implement handling of multiple PCI buses when I just wanted to include a simple workaround to get MorphOS running until the bigger clean up is eventually done. Even though some years have passed you did still not come around to do the OpenBIOS PCI clean up but also not accepted my workaround in the meantime so I need to use patched OpenBIOS for MorphOS ever since. I think we'd end up in the same situation with these patches. (It would be OK if OpenBIOS were otherwise very clean but it's full of other workarounds like patching device tree and installing QEMU VGA driver to any VGA card it sees and others that are much worse than what I've proposed and you don't seem to mind those which come from you just those that someone else propose.) Don't get this wrong, we don't have any personal problem with each other, at least I don't have anything against you but your habit of forcing people to do much more additional work just to satisfy your pedantry is not something I have time to comply with. If you propose changes that make the patches simpler or in some way better I'd do that but otherwise I'll only do what QEMU maintainers also think is necessary.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]