| Message ID | 1315832873-18976-1-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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.
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~
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.
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~
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
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.
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.
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);
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.
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);
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(-)