Re: [Qemu-devel] Re: IRQ handling

[J. Mayer]

On Mon, 2007-04-09 at 01:41 +0100, Paul Brook wrote:
> [replying to a couple of different mails]
> 
> > What do you need to route an IRQ ?
> > -> A peripheral destination
> 
> Agreed.
> 
> > What we got now ?
> > -> a callback with 3 parameters: an opaque, a PIN (the n_IRQ) and a
> > state
> 
> We have this in some places. Other places only have some parts.

So, here was the first point to fix. Imho, no need to change all the
code for this.

> > Is more needed to have a generic routing approach ?
> > -> no. This can work to route any signal
> 
> Agreed.
> 
> > Can we do with less ?
> > -> no. You need the 3 parameters.
> 
> Agreed.
> 
> 
> In summary the IRQ source (ie. device raising the IRQ) needs to keep track of 
> 4 values:
> 1) Callback
> 2) Opaque callback argument
> 3) PIN number
> 4) IRQ state.
> 
> In most cases we get (4) for free because it's a direct function of device 
> state, so I'll ignore it for now.

I don't agree. There are a lot of cases where the hardware state is not
equivalent to the device internal state. If an IRQ line is not driven,
then you cannot predict what the hardware state is, from the source. It
seems to me this is the most usual case for IRQs lines.

> I believe (1) and (2) are inherently linked, and it makes no sense to 
> set/change them individually.

OK.

> In some cases code has multiple instances of (3) sharing a single (2). This 
> is, in general, incorrect as a device may have outputs connected to different 
> interrupt controllers. I think there are examples of this on the ARM boards.
> 
> Thus an IRQ source can treat (1), (2) and (3) as a single block of 
> information, with no loss of flexibility.
> 
> > The problem is also: what does this patch adds, but complexity 
> 
> I believe my patch concentrates the (necessary) complexity in a single place. 
> For the record, the net effect of my patch was to remove approximately 32 
> lines of code (71 files changed, 594 insertions(+), 626 deletions(-))
> 
> > and arbitrary limitations ?
> 
> You have stated several times that my patch adds arbitrary limitations.
> I refute this assertion absolutely.
> 
> There are no limits on the number of IRQs, or the topology of entities 
> (devices, interrupt handlers, and CPUs) that can be supported. 

OK, then I'm wrong on this point.

> Hotplugging is not a problem, neither are systems with thousands of IRQs. I 
> have local patches for an arm-based core with several hundred IRQ lines. 
> Convention is that IRQ objects are created at the same time as the rest of 
> the device state for the IRQ sink (ie. interrupt controller or CPU).
> 
> In practice this means a particular device needs to know how many IRQ inputs 
> it has when it is instantiated. I believe this is entirely reasonable. Last 
> time I checked it wasn't feasible to dynamically solder new pins onto an IC 
> while it was running [*1].

Quite hard to achieve, I got to admit it.

>  Note that this is the number of IRQs*per instance 
> of a device. It's entirely possible to have different instances of the "same" 
> device with arbitrarily different numbers of IRQs, and an arbitrary number of 
> devices/IRQs in a system. [*2]
> 
> If you want to do anything other that simple 1-1 connections (eg. shared IRQ 
> lines) you can create a fake device to perform the appropriate multiplexing. 
> This is what the PCI code does. It creates a "PCI bus" interrupt controller 
> that maps individual device IRQ pins onto the host interface IRQs.

So, this does not change anything from the previous situation.

> > What can be done (but once again, it changes nothing, just hide the
> > "complexity"), is to replace the  { callback, n_IRQ } in devices
> > structure by a IRQState structure and have the inline functions.
> 
> This is what I did, except I chose to make it an opaque structure, to prevent 
> devices for meddling with it directly. I'd be amazed if inlining qemu_set_irq 
> made any measurable difference to execution speed.

> You seem to be saying that making this change has no benefit. I disagree 
> quite 
> strongly. 
> 
> Having each device keep track of 3 values (callback, opaque and nIRQ; see 
> earlier) is a real mess, evidenced by the fact that devices don't do this 
> consistently, the PCI code has grown its own slightly different mechanisms 
> for signalling IRQs, and the ARM boards had their own partially generic 
> implementation. Adding simple and consistent infrastructure for signalling 
> interrupts is IMHO a worthwhile change in its own right.

So, you wanted just to hide all the mess in an opaque ?
But I still cannot see what functionality this could add, how this could
make SMP feasible, for example (I do think there was no problem for this
before), ...

> >..
> > To achieve this, you have to have a structure:
> > struct PINState {
> >     qemu_pin_handler handler;
> >     void *opaque;
> >     int n;
> >     int level;
> > };
> 
> Yes, and the existing code can be extended to implement this without wasting 
> any of the current changes.
> 
> What you're talking about (and in later emails with tristate pins) is a 
> generic mechanism for emulating single-bit buses. I don't claim that my 
> implementation can do this as-is.
> 
> I have implemented sufficient infrastructure for a single-master single-slave 
> bus. The most common example of which is a IRQ line. I believe it also covers 
> a usefully large subset GPIO pin uses.

I don't think so. For most hardware, you need to emulate the actual
hardware behavior if you want to emulate what happens on the GPIO pins.
And I maintain you need to read back the hardware state if you want to
know the logical level of an IO. Think about implementing IIC,
SPI,  .... using GPIO, which is not efficient but quite a usual
situation. You absolutely need to handle multiple sources - multiple
destinations cases to achieve this. And knowing the state of an IO
source is not sufficient to know the state of the IO line.
Even for IRQ, I think it should be done: this would avoid the kind of
local mess we have to handle PCI shared IRQ (I just did look the PREP
PCI 1.2 patch to say this, it may be done in a better way for other PCI
controllers).  And PCI IRQ is exactly a case where you cannot read back
the IRQ line state from the state of the IRQ source. And it's not rare
that an IRQ source has a register where you can read back the hardware
state of its IRQ line(s).
And you also have many cases you need to know the level of an IO at some
precise points but you do not want to track all the IO changes (the IO
line may be floating most of the time, for example), then not define any
callback but just read the actual IO value when you need to know it.

> I say that my changes are a necessary first step in implementing a fully 
> generic single-bit bus framework. My implementation adds infrastructure and 
> abstraction for the "master" device (IRQ source), while leaving the "slave" 
> (IRQ sink) device code largely unchanged.

I will try to sanitize the openpic, as you did suggest. I have to admit
what I did is not so good as I initially though it was.
As I already told here, I have added a fake internal IRQ controller in
the PowerPC core just to use the same API (the SetIRQ callback / nIRQ /
level) as the rest of the code to easily plug other IRQ controllers to a
PowerPC core. This fake IRQ controller is not in the PowerPC
specification and does not have any counterpart in actual
implementations:
"While the specific definition of an external input exception is
implementation dependant, it would typically be caused by the activation
of an asynchronous signal that is part of the processing system".
The same definition is used for other exception sources (critical
interrupt, ...) not generated by the PowerPC core itself. One should
take care that the word "interrupt" in the PowerPC specification is not
related to hardware IRQ (IBM likes to use standard vocabulary with
different meanings), which is quite confusing: "an interrupt is the
action in which the processor saves its context and begins execution at
a pre-determined interrupt-handler address. Exception are the events
that will, if enabled, cause the processor to take an interrupt".
This fake interrupt controller is just an IRQ sink, not a source, so I
maintain the presence of IRQ structure in the PowerPC CPU structure is
not to be. In addition of using the API of an IRQ controller, it also
cover a problem in the CPU emulation which is Qemu does not handle
exception queues and/or has not any "clean" support for multiple
exception sources raising at the same time. As suggested by Fabrice,
instead of adding mess in the Qemu core code, as I needed to add more
asynchronous exception sources, I decided to hide this using a bitmask
in the CPU structure, which seemed to be cleaner to me.

> [*1] I guess you could theoretically do this with a self-modifying FPGA SoC. 
> It's not impossible to model, just a bit hairy. Effectively an extreme case 
> of 
> hotplugging.

Quite extreme, true.

> [*2] Technically you're limited by available memory on the host. However 
> struct IRQState is very small, so you have other much larger problems before 
> you even come close to that limit.
;-)

-- 
J. Mayer <address@hidden>
Never organized