[Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Avi Kivity]

i8259 is an ISA device (or at least, depends on the ISA infrastructure to
register its ioport); and the ISA bus is supplied by piix4.  Later patches
make this dependency explicit.

Move the i8259 initialization until after the ISA bus is created; and supply
a new qemu_irq to PCI initialization, since the i8259 isn't ready yet.  Later
wire the new qemu_irq to the i8259.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

Part of batch 7, but nasty, so sending it by itself.

Not sure this is the right approach - the i8259 is not really an ISA device.
However, disentangling it from ISA is hard.

 hw/mips_malta.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 0110daa..f7297e7 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -72,6 +72,10 @@
     SerialState *uart;
 } MaltaFPGAState;
 
+typedef struct MaltaISAState {
+    qemu_irq *i8259;
+} MaltaISAState;
+
 static ISADevice *pit;
 
 static struct _loaderparams {
@@ -763,6 +767,15 @@ static void cpu_request_exit(void *opaque, int irq, int level)
     }
 }
 
+static void malta_isa_irq_handler(void *opaque, int n, int level)
+{
+    MaltaISAState *s = opaque;
+
+    if (s->i8259) {
+        qemu_set_irq(s->i8259[n], level);
+    }
+}
+
 static
 void mips_malta_init (ram_addr_t ram_size,
                       const char *boot_device,
@@ -778,7 +791,8 @@ void mips_malta_init (ram_addr_t ram_size,
     int64_t kernel_entry;
     PCIBus *pci_bus;
     CPUState *env;
-    qemu_irq *i8259;
+    qemu_irq *i8259, *isa_irq;
+    MaltaISAState *malta_isa = g_new0(MaltaISAState, 1);
     qemu_irq *cpu_exit_irq;
     int piix4_devfn;
     i2c_bus *smbus;
@@ -928,17 +942,20 @@ void mips_malta_init (ram_addr_t ram_size,
     cpu_mips_irq_init_cpu(env);
     cpu_mips_clock_init(env);
 
-    /* Interrupt controller */
-    /* The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2 */
-    i8259 = i8259_init(env->irq[2]);
+    isa_irq = qemu_allocate_irqs(malta_isa_irq_handler, malta_isa, 16);
 
     /* Northbridge */
-    pci_bus = gt64120_register(i8259);
+    pci_bus = gt64120_register(isa_irq);
 
     /* Southbridge */
     ide_drive_get(hd, MAX_IDE_BUS);
 
     piix4_devfn = piix4_init(pci_bus, 80);
+
+    /* Interrupt controller */
+    /* The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2 */
+    malta_isa->i8259 = i8259 = i8259_init(env->irq[2]);
+
     isa_bus_irqs(i8259);
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     usb_uhci_piix4_init(pci_bus, piix4_devfn + 2);
-- 
1.7.6.1

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Avi Kivity]

On 09/12/2011 04:07 PM, Avi Kivity wrote:
> i8259 is an ISA device (or at least, depends on the ISA infrastructure to
> register its ioport); and the ISA bus is supplied by piix4.  Later patches
> make this dependency explicit.
>
> Move the i8259 initialization until after the ISA bus is created; and supply
> a new qemu_irq to PCI initialization, since the i8259 isn't ready yet.  Later
> wire the new qemu_irq to the i8259.
>

Can this please be reviewed?  It's in the front of the memory queue logjam.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Andreas Färber]

Am 14.09.2011 14:00, schrieb Avi Kivity:
> On 09/12/2011 04:07 PM, Avi Kivity wrote:
>> i8259 is an ISA device (or at least, depends on the ISA
>> infrastructure to
>> register its ioport); and the ISA bus is supplied by piix4.  Later
>> patches
>> make this dependency explicit.
>>
>> Move the i8259 initialization until after the ISA bus is created; and
>> supply
>> a new qemu_irq to PCI initialization, since the i8259 isn't ready
>> yet.  Later
>> wire the new qemu_irq to the i8259.
>>
>
> Can this please be reviewed?  It's in the front of the memory queue
> logjam.

With or without this patch, the mips and mipsel test images on qemu.org
produce no output at all, on x64 host.

Andreas

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Stefan Weil]

Am 14.09.2011 18:19, schrieb Andreas Färber:
> Am 14.09.2011 14:00, schrieb Avi Kivity:
>> On 09/12/2011 04:07 PM, Avi Kivity wrote:
>>> i8259 is an ISA device (or at least, depends on the ISA
>>> infrastructure to
>>> register its ioport); and the ISA bus is supplied by piix4. Later
>>> patches
>>> make this dependency explicit.
>>>
>>> Move the i8259 initialization until after the ISA bus is created; and
>>> supply
>>> a new qemu_irq to PCI initialization, since the i8259 isn't ready
>>> yet. Later
>>> wire the new qemu_irq to the i8259.
>>>
>>
>> Can this please be reviewed? It's in the front of the memory queue
>> logjam.
>
> With or without this patch, the mips and mipsel test images on qemu.org
> produce no output at all, on x64 host.
>
> Andreas

I just tested the mipsel test image on i386 and x64 hosts with the
same result. Adding -d in_asm shows that the linux kernel is started,
but runs in a loop at some point (timer initialization?). Timer interrupts
are still served (attach cross gdb to running emulation).

Could you already bisect this problem? I did not run these tests for
years.

My standard mips tests use debian mips kernels and show no problems.

Stefan

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Avi Kivity]

On 09/14/2011 07:19 PM, Andreas Färber wrote:
> >
> >  Can this please be reviewed?  It's in the front of the memory queue
> >  logjam.
>
> With or without this patch, the mips and mipsel test images on qemu.org
> produce no output at all, on x64 host.
>

AFAICT this problem predates the memory API.  I'm looking more for 
criticism on the approach.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Edgar E. Iglesias]

On Wed, Sep 14, 2011 at 11:17:54PM +0300, Avi Kivity wrote:
> On 09/14/2011 07:19 PM, Andreas Färber wrote:
> >>
> >>  Can this please be reviewed?  It's in the front of the memory queue
> >>  logjam.
> >
> >With or without this patch, the mips and mipsel test images on qemu.org
> >produce no output at all, on x64 host.
> >
> 
> AFAICT this problem predates the memory API.  I'm looking more for
> criticism on the approach.

Hi, i think the images on the wiki are meant to run on a -M mips board.
Maybe the scripts predate a change to Malta as default board, don't know.

The patch looks OK to me to, but you could maybe put a comment in the code
to explain the indirection.

Cheers

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Stefan Weil]

Am 14.09.2011 22:52, schrieb Edgar E. Iglesias:
> On Wed, Sep 14, 2011 at 11:17:54PM +0300, Avi Kivity wrote:
>> On 09/14/2011 07:19 PM, Andreas Färber wrote:
>>>>   Can this please be reviewed?  It's in the front of the memory queue
>>>>   logjam.
>>> With or without this patch, the mips and mipsel test images on qemu.org
>>> produce no output at all, on x64 host.
>>>
>> AFAICT this problem predates the memory API.  I'm looking more for
>> criticism on the approach.
> Hi, i think the images on the wiki are meant to run on a -M mips board.
> Maybe the scripts predate a change to Malta as default board, don't know.

That's a very good hint. Yes, with -M mips it works. See
commit 63742cf887a161aba6cbe53a60161aa47db9a4bb
(Sat Jul 5 21:51:47 2008 +0000):
     Change MIPS machine default to Malta.

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Avi Kivity]

On 09/14/2011 11:52 PM, Edgar E. Iglesias wrote:
> On Wed, Sep 14, 2011 at 11:17:54PM +0300, Avi Kivity wrote:
> >  On 09/14/2011 07:19 PM, Andreas Färber wrote:
> >  >>
> >  >>   Can this please be reviewed?  It's in the front of the memory queue
> >  >>   logjam.
> >  >
> >  >With or without this patch, the mips and mipsel test images on qemu.org
> >  >produce no output at all, on x64 host.
> >  >
> >
> >  AFAICT this problem predates the memory API.  I'm looking more for
> >  criticism on the approach.
>
> Hi, i think the images on the wiki are meant to run on a -M mips board.
> Maybe the scripts predate a change to Malta as default board, don't know.
>
> The patch looks OK to me to, but you could maybe put a comment in the code
> to explain the indirection.
>

Thanks, added a comment and your acked-by.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Stefan Weil]

Am 14.09.2011 14:00, schrieb Avi Kivity:
> On 09/12/2011 04:07 PM, Avi Kivity wrote:
>> i8259 is an ISA device (or at least, depends on the ISA 
>> infrastructure to
>> register its ioport); and the ISA bus is supplied by piix4.  Later 
>> patches
>> make this dependency explicit.
>>
>> Move the i8259 initialization until after the ISA bus is created; and 
>> supply
>> a new qemu_irq to PCI initialization, since the i8259 isn't ready 
>> yet.  Later
>> wire the new qemu_irq to the i8259.
>>
>
> Can this please be reviewed?  It's in the front of the memory queue 
> logjam.
>

Tested-by: Stefan Weil <weil@mail.berlios.de>

Test configurations:

1. Debian lenny, qemu-system-mipsel (32 bit little endian), MIPS Malta
2. Debian lenny, qemu-system-mips64 (64 bit big endian), MIPS Malta

Both work on a i386 linux host with the patch applied.

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Richard Henderson]

On 09/12/2011 06:07 AM, Avi Kivity wrote:
> +static void malta_isa_irq_handler(void *opaque, int n, int level)
> +{
> +    MaltaISAState *s = opaque;
> +
> +    if (s->i8259) {
> +        qemu_set_irq(s->i8259[n], level);
> +    }
> +}

Is there any point in the IF?  I realize that there's an ordering
problem that requires the use of the memory indirection in order
to be able to provide *some* opaque value at the proper time, but
AFAICT the ->i8259 value will *always* be non-null at the point
this function is called.  Am I wrong here?

Otherwise the approach looks correct.


r~

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Avi Kivity]

On 09/14/2011 11:32 PM, Richard Henderson wrote:
> On 09/12/2011 06:07 AM, Avi Kivity wrote:
> >  +static void malta_isa_irq_handler(void *opaque, int n, int level)
> >  +{
> >  +    MaltaISAState *s = opaque;
> >  +
> >  +    if (s->i8259) {
> >  +        qemu_set_irq(s->i8259[n], level);
> >  +    }
> >  +}
>
> Is there any point in the IF?  I realize that there's an ordering
> problem that requires the use of the memory indirection in order
> to be able to provide *some* opaque value at the proper time, but
> AFAICT the ->i8259 value will *always* be non-null at the point
> this function is called.  Am I wrong here?

The pci bridge may toggle the irq line as part of its initialization.

> Otherwise the approach looks correct.

Yeah, but it would have been even more correct to make i8259 not an isa 
device.  Something for later on.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Richard Henderson]

On 09/14/2011 01:37 PM, Avi Kivity wrote:
> On 09/14/2011 11:32 PM, Richard Henderson wrote:
>> On 09/12/2011 06:07 AM, Avi Kivity wrote:
>> >  +static void malta_isa_irq_handler(void *opaque, int n, int level)
>> >  +{
>> >  +    MaltaISAState *s = opaque;
>> >  +
>> >  +    if (s->i8259) {
>> >  +        qemu_set_irq(s->i8259[n], level);
>> >  +    }
>> >  +}
>>
>> Is there any point in the IF?  I realize that there's an ordering
>> problem that requires the use of the memory indirection in order
>> to be able to provide *some* opaque value at the proper time, but
>> AFAICT the ->i8259 value will *always* be non-null at the point
>> this function is called.  Am I wrong here?
> 
> The pci bridge may toggle the irq line as part of its initialization.

Ah.  Right.

> Yeah, but it would have been even more correct to make i8259 not an
> isa device.  Something for later on.

Yeah, there's a lot of that going around.  In any case:

Reviewed-by: Richard Henderson  <rth@twiddle.net>



r~

Re: [Qemu-devel] [PATCH] mips_malta: move i8259 initialization after piix4 initialization

[Anthony Liguori]

On 09/12/2011 08:07 AM, Avi Kivity wrote:
> i8259 is an ISA device (or at least, depends on the ISA infrastructure to
> register its ioport); and the ISA bus is supplied by piix4.  Later patches
> make this dependency explicit.
>
> Move the i8259 initialization until after the ISA bus is created; and supply
> a new qemu_irq to PCI initialization, since the i8259 isn't ready yet.  Later
> wire the new qemu_irq to the i8259.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>
> ---
>
> Part of batch 7, but nasty, so sending it by itself.
>
> Not sure this is the right approach - the i8259 is not really an ISA device.
> However, disentangling it from ISA is hard.

ISA is a train wreck today so we just have to do what we need to for now until 
it can be properly fixed.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
>   hw/mips_malta.c |   27 ++++++++++++++++++++++-----
>   1 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index 0110daa..f7297e7 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -72,6 +72,10 @@
>       SerialState *uart;
>   } MaltaFPGAState;
>
> +typedef struct MaltaISAState {
> +    qemu_irq *i8259;
> +} MaltaISAState;
> +
>   static ISADevice *pit;
>
>   static struct _loaderparams {
> @@ -763,6 +767,15 @@ static void cpu_request_exit(void *opaque, int irq, int level)
>       }
>   }
>
> +static void malta_isa_irq_handler(void *opaque, int n, int level)
> +{
> +    MaltaISAState *s = opaque;
> +
> +    if (s->i8259) {
> +        qemu_set_irq(s->i8259[n], level);
> +    }
> +}
> +
>   static
>   void mips_malta_init (ram_addr_t ram_size,
>                         const char *boot_device,
> @@ -778,7 +791,8 @@ void mips_malta_init (ram_addr_t ram_size,
>       int64_t kernel_entry;
>       PCIBus *pci_bus;
>       CPUState *env;
> -    qemu_irq *i8259;
> +    qemu_irq *i8259, *isa_irq;
> +    MaltaISAState *malta_isa = g_new0(MaltaISAState, 1);
>       qemu_irq *cpu_exit_irq;
>       int piix4_devfn;
>       i2c_bus *smbus;
> @@ -928,17 +942,20 @@ void mips_malta_init (ram_addr_t ram_size,
>       cpu_mips_irq_init_cpu(env);
>       cpu_mips_clock_init(env);
>
> -    /* Interrupt controller */
> -    /* The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2 */
> -    i8259 = i8259_init(env->irq[2]);
> +    isa_irq = qemu_allocate_irqs(malta_isa_irq_handler, malta_isa, 16);
>
>       /* Northbridge */
> -    pci_bus = gt64120_register(i8259);
> +    pci_bus = gt64120_register(isa_irq);
>
>       /* Southbridge */
>       ide_drive_get(hd, MAX_IDE_BUS);
>
>       piix4_devfn = piix4_init(pci_bus, 80);
> +
> +    /* Interrupt controller */
> +    /* The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2 */
> +    malta_isa->i8259 = i8259 = i8259_init(env->irq[2]);
> +
>       isa_bus_irqs(i8259);
>       pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
>       usb_uhci_piix4_init(pci_bus, piix4_devfn + 2);