[Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Stefan Weil]

cirrus_reset is also called by the pci framework,
so there is no need to call it in cirrus_init_common.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/cirrus_vga.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 2724f7b..bdf4c8b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3024,7 +3024,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
     s->vga.cursor_draw_line = cirrus_cursor_draw_line;
 
     qemu_register_reset(cirrus_reset, s);
-    cirrus_reset(s);
 }
 
 /***************************************
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Isaku Yamahata]

Hi. cirrus_init_common() is used by both isa and pci cirrus.
and isa cirrus isn't qdevfied yet.
So what you want is
- remove qemu_register_reset() and cirrus_reset() from cirrus_init_common()

- add to PCIDeviceInfo cirrus_vga_info
  .qdev.reset = cirrus_reset()
  in order to use pci reset framework.

- add qemu_register_reset() and cirrus_reset() to isa_cirrus_vga_init()
  (Hopefully convert isa cirrus to qdev. and use .qdev.reset at best.
   But it's up to you)

thanks,

On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
> cirrus_reset is also called by the pci framework,
> so there is no need to call it in cirrus_init_common.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/cirrus_vga.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 2724f7b..bdf4c8b 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -3024,7 +3024,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
>      s->vga.cursor_draw_line = cirrus_cursor_draw_line;
>  
>      qemu_register_reset(cirrus_reset, s);
> -    cirrus_reset(s);
>  }
>  
>  /***************************************
> -- 
> 1.7.2.5
> 
> 

-- 
yamahata

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Stefan Weil]

Am 28.03.2011 04:17, schrieb Isaku Yamahata:
> Hi. cirrus_init_common() is used by both isa and pci cirrus.
> and isa cirrus isn't qdevfied yet.
> So what you want is
> - remove qemu_register_reset() and cirrus_reset() from cirrus_init_common()
>
> - add to PCIDeviceInfo cirrus_vga_info
>    .qdev.reset = cirrus_reset()
>    in order to use pci reset framework.
>
> - add qemu_register_reset() and cirrus_reset() to isa_cirrus_vga_init()
>    (Hopefully convert isa cirrus to qdev. and use .qdev.reset at best.
>     But it's up to you)
>
> thanks,
>
> On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
>> cirrus_reset is also called by the pci framework,
>> so there is no need to call it in cirrus_init_common.
>>
>> Cc: Michael S. Tsirkin<mst@redhat.com>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> ---
>>   hw/cirrus_vga.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>> index 2724f7b..bdf4c8b 100644
>> --- a/hw/cirrus_vga.c
>> +++ b/hw/cirrus_vga.c
>> @@ -3024,7 +3024,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
>>       s->vga.cursor_draw_line = cirrus_cursor_draw_line;
>>
>>       qemu_register_reset(cirrus_reset, s);
>> -    cirrus_reset(s);
>>   }
>>
>>   /***************************************
>> -- 
>> 1.7.2.5
>


I tested the new code with isa pc, too. In gdb, I could see that it also 
calls
cirrus_reset twice. But isa pc is broken since the switch to sea bios, so
obviously isa is an unmaintained part of qemu. Even with bochs bios,
it no longer works, so it is broken at least twice.

Stefan

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Isaku Yamahata]

On Mon, Mar 28, 2011 at 07:18:04AM +0200, Stefan Weil wrote:
> Am 28.03.2011 04:17, schrieb Isaku Yamahata:
>> Hi. cirrus_init_common() is used by both isa and pci cirrus.
>> and isa cirrus isn't qdevfied yet.
>> So what you want is
>> - remove qemu_register_reset() and cirrus_reset() from cirrus_init_common()
>>
>> - add to PCIDeviceInfo cirrus_vga_info
>>    .qdev.reset = cirrus_reset()
>>    in order to use pci reset framework.
>>
>> - add qemu_register_reset() and cirrus_reset() to isa_cirrus_vga_init()
>>    (Hopefully convert isa cirrus to qdev. and use .qdev.reset at best.
>>     But it's up to you)
>>
>> thanks,
>>
>> On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
>>> cirrus_reset is also called by the pci framework,
>>> so there is no need to call it in cirrus_init_common.
>>>
>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>> ---
>>>   hw/cirrus_vga.c |    1 -
>>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>>> index 2724f7b..bdf4c8b 100644
>>> --- a/hw/cirrus_vga.c
>>> +++ b/hw/cirrus_vga.c
>>> @@ -3024,7 +3024,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
>>>       s->vga.cursor_draw_line = cirrus_cursor_draw_line;
>>>
>>>       qemu_register_reset(cirrus_reset, s);
>>> -    cirrus_reset(s);
>>>   }
>>>
>>>   /***************************************
>>> -- 
>>> 1.7.2.5
>>
>
>
> I tested the new code with isa pc, too. In gdb, I could see that it also  
> calls
> cirrus_reset twice. But isa pc is broken since the switch to sea bios, so
> obviously isa is an unmaintained part of qemu. Even with bochs bios,
> it no longer works, so it is broken at least twice.

Ah, I see. The the second reset is called not via pci reset framework,
but qemu reset framework. So removing the above reset call makes sense.
It would be another patch to make use of pci reset framework.
-- 
yamahata

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Markus Armbruster]

Isaku Yamahata <yamahata@valinux.co.jp> writes:

> On Mon, Mar 28, 2011 at 07:18:04AM +0200, Stefan Weil wrote:
>> Am 28.03.2011 04:17, schrieb Isaku Yamahata:
[...]
>>> On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
>>>> cirrus_reset is also called by the pci framework,
>>>> so there is no need to call it in cirrus_init_common.
>>>>
>>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
[...]
>> I tested the new code with isa pc, too. In gdb, I could see that it also  
>> calls
>> cirrus_reset twice. But isa pc is broken since the switch to sea bios, so
>> obviously isa is an unmaintained part of qemu. Even with bochs bios,
>> it no longer works, so it is broken at least twice.
>
> Ah, I see. The the second reset is called not via pci reset framework,
> but qemu reset framework. So removing the above reset call makes sense.
> It would be another patch to make use of pci reset framework.

Then the proposed commit message's claim cirrus_reset() is "called by
the pci framework" is incorrect, isn't it?

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Isaku Yamahata]

On Mon, Mar 28, 2011 at 11:21:23AM +0200, Markus Armbruster wrote:
> Isaku Yamahata <yamahata@valinux.co.jp> writes:
> 
> > On Mon, Mar 28, 2011 at 07:18:04AM +0200, Stefan Weil wrote:
> >> Am 28.03.2011 04:17, schrieb Isaku Yamahata:
> [...]
> >>> On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
> >>>> cirrus_reset is also called by the pci framework,
> >>>> so there is no need to call it in cirrus_init_common.
> >>>>
> >>>> Cc: Michael S. Tsirkin<mst@redhat.com>
> >>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
> [...]
> >> I tested the new code with isa pc, too. In gdb, I could see that it also  
> >> calls
> >> cirrus_reset twice. But isa pc is broken since the switch to sea bios, so
> >> obviously isa is an unmaintained part of qemu. Even with bochs bios,
> >> it no longer works, so it is broken at least twice.
> >
> > Ah, I see. The the second reset is called not via pci reset framework,
> > but qemu reset framework. So removing the above reset call makes sense.
> > It would be another patch to make use of pci reset framework.
> 
> Then the proposed commit message's claim cirrus_reset() is "called by
> the pci framework" is incorrect, isn't it?

Yes, incorrect. The commit message should be fixed.
The code change itself looks correct.
-- 
yamahata

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Stefan Weil]

Am 28.03.2011 11:24, schrieb Isaku Yamahata:
> On Mon, Mar 28, 2011 at 11:21:23AM +0200, Markus Armbruster wrote:
>> Isaku Yamahata <yamahata@valinux.co.jp> writes:
>>
>>> On Mon, Mar 28, 2011 at 07:18:04AM +0200, Stefan Weil wrote:
>>>> Am 28.03.2011 04:17, schrieb Isaku Yamahata:
>> [...]
>>>>> On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
>>>>>> cirrus_reset is also called by the pci framework,
>>>>>> so there is no need to call it in cirrus_init_common.
>>>>>>
>>>>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>>>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> [...]
>>>> I tested the new code with isa pc, too. In gdb, I could see that it 
>>>> also
>>>> calls
>>>> cirrus_reset twice. But isa pc is broken since the switch to sea 
>>>> bios, so
>>>> obviously isa is an unmaintained part of qemu. Even with bochs bios,
>>>> it no longer works, so it is broken at least twice.
>>>
>>> Ah, I see. The the second reset is called not via pci reset framework,
>>> but qemu reset framework. So removing the above reset call makes sense.
>>> It would be another patch to make use of pci reset framework.
>>
>> Then the proposed commit message's claim cirrus_reset() is "called by
>> the pci framework" is incorrect, isn't it?
>
> Yes, incorrect. The commit message should be fixed.
> The code change itself looks correct.

For current qemu it is correct, or is there a working configuration
with isa cirrus? I asked that question on #qemu but did not get
an answer (Anthony replied that isa was broken long ago).

This was the reason why I wrote the commit text as it is.
I don't mind if the committer adds more descriptive text,
but the main focus should be fixing isa emulation.
I also noticed that some more emulations obviously also
include redundant reset calls. These should be fixed, too.

Re: [Qemu-devel] [PATCH] cirrus_vga: Remove unneeded reset

[Michael S. Tsirkin]

On Mon, Mar 28, 2011 at 06:20:15PM +0200, Stefan Weil wrote:
> Am 28.03.2011 11:24, schrieb Isaku Yamahata:
> >On Mon, Mar 28, 2011 at 11:21:23AM +0200, Markus Armbruster wrote:
> >>Isaku Yamahata <yamahata@valinux.co.jp> writes:
> >>
> >>>On Mon, Mar 28, 2011 at 07:18:04AM +0200, Stefan Weil wrote:
> >>>>Am 28.03.2011 04:17, schrieb Isaku Yamahata:
> >>[...]
> >>>>>On Sat, Mar 26, 2011 at 10:53:09PM +0100, Stefan Weil wrote:
> >>>>>>cirrus_reset is also called by the pci framework,
> >>>>>>so there is no need to call it in cirrus_init_common.
> >>>>>>
> >>>>>>Cc: Michael S. Tsirkin<mst@redhat.com>
> >>>>>>Signed-off-by: Stefan Weil<weil@mail.berlios.de>
> >>[...]
> >>>>I tested the new code with isa pc, too. In gdb, I could see
> >>>>that it also
> >>>>calls
> >>>>cirrus_reset twice. But isa pc is broken since the switch to
> >>>>sea bios, so
> >>>>obviously isa is an unmaintained part of qemu. Even with bochs bios,
> >>>>it no longer works, so it is broken at least twice.
> >>>
> >>>Ah, I see. The the second reset is called not via pci reset framework,
> >>>but qemu reset framework. So removing the above reset call makes sense.
> >>>It would be another patch to make use of pci reset framework.
> >>
> >>Then the proposed commit message's claim cirrus_reset() is "called by
> >>the pci framework" is incorrect, isn't it?
> >
> >Yes, incorrect. The commit message should be fixed.
> >The code change itself looks correct.
> 
> For current qemu it is correct, or is there a working configuration
> with isa cirrus? I asked that question on #qemu but did not get
> an answer (Anthony replied that isa was broken long ago).
> 
> This was the reason why I wrote the commit text as it is.
> I don't mind if the committer adds more descriptive text,
> but the main focus should be fixing isa emulation.
> I also noticed that some more emulations obviously also
> include redundant reset calls. These should be fixed, too.

*I tweaked the commit log a bit to make everyone happy
and applied that.
Thanks!