[Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Stefan Weil]

Commit 5632ae46d5bda798e971dae48ebb318ac2c3686a passes the address
of i8259 to qemu_irq_proxy. i8259 was an auto variable with undefined
value outside of mips_malta_init. This made the proxy unusable.

Ethernet for example no longer worked with MIPS Malta.

There is only one Malta device with one i8259, so using a static
variable for i8259 is the simplest solution which fixes the problem.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/mips_malta.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index bb49749..e4dc7fb 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -775,7 +775,10 @@ void mips_malta_init (ram_addr_t ram_size,
     int64_t kernel_entry;
     PCIBus *pci_bus;
     CPUState *env;
-    qemu_irq *i8259 = NULL, *isa_irq;
+    /* The address of i8259 is passed to qemu_irq_proxy and saved there, but
+       its value is set later, so it must have a fixed reserved address. */
+    static qemu_irq *i8259;
+    qemu_irq *isa_irq;
     qemu_irq *cpu_exit_irq;
     int piix4_devfn;
     i2c_bus *smbus;
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Peter Maydell]

On 24 November 2011 22:07, Stefan Weil <sw@weilnetz.de> wrote:
> Commit 5632ae46d5bda798e971dae48ebb318ac2c3686a passes the address
> of i8259 to qemu_irq_proxy. i8259 was an auto variable with undefined
> value outside of mips_malta_init. This made the proxy unusable.
>
> Ethernet for example no longer worked with MIPS Malta.
>
> There is only one Malta device with one i8259, so using a static
> variable for i8259 is the simplest solution which fixes the problem.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/mips_malta.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index bb49749..e4dc7fb 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -775,7 +775,10 @@ void mips_malta_init (ram_addr_t ram_size,
>     int64_t kernel_entry;
>     PCIBus *pci_bus;
>     CPUState *env;
> -    qemu_irq *i8259 = NULL, *isa_irq;
> +    /* The address of i8259 is passed to qemu_irq_proxy and saved there, but
> +       its value is set later, so it must have a fixed reserved address. */
> +    static qemu_irq *i8259;
> +    qemu_irq *isa_irq;

Yuck. (If boards had state structures the way devices do we'd have
somewhere to put this rather than a local static. More generally
if we have to do this kind of trick then either our device model
is wrong or we're not using it right in this specific situation...)

-- PMM

Re: [Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Stefan Weil]

Am 25.11.2011 00:21, schrieb Peter Maydell:
> On 24 November 2011 22:07, Stefan Weil <sw@weilnetz.de> wrote:
>> Commit 5632ae46d5bda798e971dae48ebb318ac2c3686a passes the address
>> of i8259 to qemu_irq_proxy. i8259 was an auto variable with undefined
>> value outside of mips_malta_init. This made the proxy unusable.
>>
>> Ethernet for example no longer worked with MIPS Malta.
>>
>> There is only one Malta device with one i8259, so using a static
>> variable for i8259 is the simplest solution which fixes the problem.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  hw/mips_malta.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
>> index bb49749..e4dc7fb 100644
>> --- a/hw/mips_malta.c
>> +++ b/hw/mips_malta.c
>> @@ -775,7 +775,10 @@ void mips_malta_init (ram_addr_t ram_size,
>>     int64_t kernel_entry;
>>     PCIBus *pci_bus;
>>     CPUState *env;
>> -    qemu_irq *i8259 = NULL, *isa_irq;
>> +    /* The address of i8259 is passed to qemu_irq_proxy and saved 
>> there, but
>> +       its value is set later, so it must have a fixed reserved 
>> address. */
>> +    static qemu_irq *i8259;
>> +    qemu_irq *isa_irq;
>
> Yuck. (If boards had state structures the way devices do we'd have
> somewhere to put this rather than a local static. More generally
> if we have to do this kind of trick then either our device model
> is wrong or we're not using it right in this specific situation...)
>
> -- PMM

Boards normally have state structures, the device model
supports this and is not wrong here.

The Malta implementation simply needs conversion to qdev.
This is something which will be done after QEMU 1.0.

My patch is not the final solution. It's simply a hack to get
a working Malta in QEMU 1.0. If the qdev solution is preferred
for 1.0, I can also send a Malta qdev patch.

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Avi Kivity]

On 11/25/2011 09:01 AM, Stefan Weil wrote:
>
> Boards normally have state structures, the device model
> supports this and is not wrong here.
>
> The Malta implementation simply needs conversion to qdev.
> This is something which will be done after QEMU 1.0.
>
> My patch is not the final solution. It's simply a hack to get
> a working Malta in QEMU 1.0. If the qdev solution is preferred
> for 1.0, I can also send a Malta qdev patch.
>

For 1.0, the simpler patch is better IMO (though I prefer g_new()
instead of static; g_new() is reentrant and catches the eye, while
static will hide from sight).

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

Re: [Qemu-devel] [PATCH 1.0] malta: Fix regression (i8259 interrupts did not work)

[Anthony Liguori]

On 11/24/2011 05:21 PM, Peter Maydell wrote:
> On 24 November 2011 22:07, Stefan Weil<sw@weilnetz.de>  wrote:
>> Commit 5632ae46d5bda798e971dae48ebb318ac2c3686a passes the address
>> of i8259 to qemu_irq_proxy. i8259 was an auto variable with undefined
>> value outside of mips_malta_init. This made the proxy unusable.
>>
>> Ethernet for example no longer worked with MIPS Malta.
>>
>> There is only one Malta device with one i8259, so using a static
>> variable for i8259 is the simplest solution which fixes the problem.
>>
>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>> ---
>>   hw/mips_malta.c |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
>> index bb49749..e4dc7fb 100644
>> --- a/hw/mips_malta.c
>> +++ b/hw/mips_malta.c
>> @@ -775,7 +775,10 @@ void mips_malta_init (ram_addr_t ram_size,
>>      int64_t kernel_entry;
>>      PCIBus *pci_bus;
>>      CPUState *env;
>> -    qemu_irq *i8259 = NULL, *isa_irq;
>> +    /* The address of i8259 is passed to qemu_irq_proxy and saved there, but
>> +       its value is set later, so it must have a fixed reserved address. */
>> +    static qemu_irq *i8259;
>> +    qemu_irq *isa_irq;
>
> Yuck. (If boards had state structures the way devices do we'd have
> somewhere to put this rather than a local static. More generally
> if we have to do this kind of trick then either our device model
> is wrong or we're not using it right in this specific situation...)

Boards should be modeled in qdev as first class devices.  I have a series ready 
to go that I'll send out once 1.1 opens that gets us started in that direction.

Regards,

Anthony Liguori

>
> -- PMM
>
>