Re: [Qemu-devel] qemu Makefile.target vl.h hw/acpi.c hw/adlib.c ...

[J. Mayer]

On Sun, 2007-04-08 at 00:13 +0100, Paul Brook wrote:
> On Saturday 07 April 2007 23:18, J. Mayer wrote:
> > It seems that you don't figure out how real hardware works AT ALL.
> > IRQ callbacks and private data CANNOT BE SHARED as they are INTERNAL TO
> > ONE IRQ CONTROLLER. IRQ management, once again, is completelly specific
> > to an IRQ controller technology and absolutely not related to the CPU
> > you may use on the same board. So it's a complete nonsense to try to
> > share IRQ notion in the CPU. IRQ notions and management is completely
> > different to one IRQ controller technology to another.
> <snip>
> 
> You appear to have misunderstood what my patch does. The arguments you use in 
> the part of the email I snipped are the same resons why I created this patch.

So your patch is not useful for the PowerPC target as cascading could
already works and does not need anything hardcoded in the CPU (once
again, IRQ sources is no way related to the CPU).

> The most important thing to understand is that the "qemu_irq" object is 
> simply 
> an abstract interface for connecting two devices. One of which produces a 
> single-bit output, and the other does something in response to that signal. 
> qemu_irq isn't an ideal name because it's used for things other than physical 
> IRQ pins. However it's the best we (Me, Theimo, or anyone else on #qemu about 
> 24 hours ago) could come up with.
> 
> The purpose of this abstraction is precisely so that interrupt sources and 
> sinks can be chained and rearranged in arbitrary fashion.

Which was already done. Why change what works, so ? Why BREAK ?

> In the specific case of PPC a typical system, as you say, has several 
> cascaded 
> levels of interrupt handling. You have multiple external interrupt 
> controllers, including interrupts from PCI bus based devices. 
> 
> Contrary to your assertion, a PPC cpu does contains an internal vectored 
> interrupt controller. This handles the external IRQ pin, plus various 
> internal interrupt sources (as defined by the PPC_INTERRUPT_* constants). 
> This is what the IRQ array in the cpu state structure is for. I see no 
> fundamental reason why this should be treated any differently to an external 
> interrupt controller.

I guess you make a confusion between IRQ, which are external and managed
with an IRQ controller and exceptions, which are handled inside the
PowerPC core. There is no IRQ controller inside the PowerPC. You can
read the specification in any way you want, you will find none.
IRQ controllers are peripheral devices. So, nothing about them can be
managed in the CPU structure. What when there is more than one CPU ? IRQ
number is a notion internal to one IRQ controller and has no meaning
outside of it. How can you even think of using it to reference 

> Because these are currently implemented in the same file I didn't bother 
> converting them fully and they mostly still use ppc_set_irq dircetly instead 
> of going via qemu_set_irq. If/when ppc.c is split up and/or cpus with more 
> than one external IRQ pin are added the generic IRQ routing mechanisms allow 
> this to happen without having to export ppc_set_irq.
> 
> PPC is a slightly bad example because it still has ppc_openpic_irq. This is 
> exactly the sort of horrible system specific hack I'm trying to replace.

Once again, I guess you don't understand what this code does. Openpic,
as many IRQ controllers, has a lot of inputs and not only a single
output.This code is there to connect external IRQ sources comming from
an OpenPIC to internal exceptions of the PowerPC. As this is hardwired
in the PowerPC core and cannot be changed, it's not especially a hack.
To be _really_ generic and do exactly what is done is real hardware
devices, it would need another cascaded stage. I did not add this stage
because it would have just add performance penalties but would not have
added any real advantage, imho.

> Before my patch chained interrupts were handled with a messy ad-hoc 
> combination of callbacks and opaque parameters. Some devices were properly 
> parameterized, others assumed that their IRQ output went to a specific device 
> or CPU.

So, you just have to fix the messy devices. Not to break the CPU
emulation code or structure.

> My patch introduces a generic mechanism for devices to talk to an interrupt 
> controller. It means that the code signalling the interrupt doesn't need to 
> know or care where it ends up. 

That's exactly why some IRQ callbacks have been added in some devices,
just not to have to care where the interrupt ends up or even if the IRQ
is actually raised. No need to break anything. Just fix the old devices
that did not use this logic.

> The logic for implementing a particular 
> interrupt controller is still private to that device.

Your patch is even worse than what already exists and work, add
confusion by breaking concepts (the IRQ controller is an external
device, whatever the CPU you use), add garbage in the CPU structure, add
a mess to handle multi-cpu/multi PIC systems (which are more than
common, these days), add arbitrary limits with no justifications, ....
So, no new features, broken concepts, new mess, new limitations and more
complicated code...
It's unacceptable.

To give you an real example why arbitrary limits are not acceptable AT
ALL: I know an embedded Mips device (widely used !) with 2 CPU, 8 PIC
and about 500 IRQ sources. How can you even pretend add a limited
structure in the CPUState structure when this is exactly the kind of
device some people want to emulate in Qemu ? Your concept is completely
broken, you have to admit it. You can never put peripheral informations
in the CPUState structure. What if I decide to put the NIC registers in
the CPU structure ? It's exactly the same as what you did...


The funny thing is that you even did not fix the only bug that makes the
OpenPic not really cascadable: the last argument of the openpic_init has
to be changed to be void ** instead of CPUState ** (same in
the .IRQ_dst_t structure). No need for more (but this patch is needed, I
did not commit it because I don't need it just now...)

[...]

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