[PATCH] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[PATCH] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[Carlo Marcelo Arenas Belon]

The following patch fixes 1a483ef4040ed380bf69d684783d06a617073256 so that the
parent PIC pointer is used to send the edge irq0 instead of the PIC pair and
that is an incompatible pointer type as reported in :

/var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c: In function
`
pic_read_irq':
/var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:248:
warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
/var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:249:
warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type

Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
---
 qemu/hw/i8259.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
index 01447d7..60063d4 100644
--- a/qemu/hw/i8259.c
+++ b/qemu/hw/i8259.c
@@ -245,8 +245,8 @@ int pic_read_irq(PicState2 *s)
 	    if (timer_ints_to_push > 0) {
 		timer_ints_to_push--;
                 /* simulate an edge irq0, like the one generated by i8254 */
-                pic_set_irq1(s, 0, 0);
-                pic_set_irq1(s, 0, 1);
+                pic_set_irq1(&s->pics[0], 0, 0);
+                pic_set_irq1(&s->pics[0], 0, 1);
 	    }
 	}
 
-- 
1.5.2.5


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[Carlo Marcelo Arenas Belon]

wrong patch, this was meant to go to the slave PIC, so it will use s->pics[1]
instead of s->pics[0].

also from my tests it might seem that tdf is irrelevant anyway with the new
clock work and haven't been able to find a case where enabling it (so
triggering this buggy code path) migh be needed.

any one care to comment on any current users of tdf? and if there are none in
the viability for removing it?

Carlo

On Fri, Nov 09, 2007 at 11:22:10AM -0600, Carlo Marcelo Arenas Belon wrote:
> The following patch fixes 1a483ef4040ed380bf69d684783d06a617073256 so that the
> parent PIC pointer is used to send the edge irq0 instead of the PIC pair and
> that is an incompatible pointer type as reported in :
> 
> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c: In function
> `
> pic_read_irq':
> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:248:
> warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:249:
> warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
> 
> Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
> ---
>  qemu/hw/i8259.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
> index 01447d7..60063d4 100644
> --- a/qemu/hw/i8259.c
> +++ b/qemu/hw/i8259.c
> @@ -245,8 +245,8 @@ int pic_read_irq(PicState2 *s)
>  	    if (timer_ints_to_push > 0) {
>  		timer_ints_to_push--;
>                  /* simulate an edge irq0, like the one generated by i8254 */
> -                pic_set_irq1(s, 0, 0);
> -                pic_set_irq1(s, 0, 1);
> +                pic_set_irq1(&s->pics[0], 0, 0);
> +                pic_set_irq1(&s->pics[0], 0, 1);
>  	    }
>  	}
>  
> -- 
> 1.5.2.5
> 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[Dor Laor]

[-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --]

Carlo Marcelo Arenas Belon wrote:
> wrong patch, this was meant to go to the slave PIC, so it will use s->pics[1]
> instead of s->pics[0].
>
>   
Why to the slave pic? Isn't the pit connected to line 0 of the master?
> also from my tests it might seem that tdf is irrelevant anyway with the new
> clock work and haven't been able to find a case where enabling it (so
> triggering this buggy code path) migh be needed.
>
> any one care to comment on any current users of tdf? and if there are none in
> the viability for removing it?
>
>   
It does work but only for non-acpi guest that has the -no-kvm-irqchip 
parameter.
To test it you can load your host and see what happens to the clock when 
you run
a 1000HZ guest (use taskset to pin the guest with other cpu intensive 
tasks.)
We decided not to fix it in the in-kernel pic since once the tpr 
optimization enable
running acpi/apic guests and thus the pic is not used as time source.
Dor.
> Carlo
>
> On Fri, Nov 09, 2007 at 11:22:10AM -0600, Carlo Marcelo Arenas Belon wrote:
>   
>> The following patch fixes 1a483ef4040ed380bf69d684783d06a617073256 so that the
>> parent PIC pointer is used to send the edge irq0 instead of the PIC pair and
>> that is an incompatible pointer type as reported in :
>>
>> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c: In function
>> `
>> pic_read_irq':
>> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:248:
>> warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
>> /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:249:
>> warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
>>
>> Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
>> ---
>>  qemu/hw/i8259.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
>> index 01447d7..60063d4 100644
>> --- a/qemu/hw/i8259.c
>> +++ b/qemu/hw/i8259.c
>> @@ -245,8 +245,8 @@ int pic_read_irq(PicState2 *s)
>>  	    if (timer_ints_to_push > 0) {
>>  		timer_ints_to_push--;
>>                  /* simulate an edge irq0, like the one generated by i8254 */
>> -                pic_set_irq1(s, 0, 0);
>> -                pic_set_irq1(s, 0, 1);
>> +                pic_set_irq1(&s->pics[0], 0, 0);
>> +                pic_set_irq1(&s->pics[0], 0, 1);
>>  	    }
>>  	}
>>  
>> -- 
>> 1.5.2.5
>>
>>     
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   


[-- Attachment #1.2: Type: text/html, Size: 3858 bytes --]

[-- Attachment #2: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [RFC] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[Carlo Marcelo Arenas Belon]

On Sat, Nov 10, 2007 at 12:35:34AM +0200, Dor Laor wrote:
>    Carlo Marcelo Arenas Belon wrote:
> 
>  wrong patch, this was meant to go to the slave PIC, so it will use s->pics[1]
>  instead of s->pics[0].
> 
>    Why to the slave pic? Isn't the pit connected to line 0 of the master?

yes, you are right; 8254 should be connected to line 0 of the master 8259 and
using the slave PIC is not correct.

sadly, with the patch and while trying to test if it make a difference or not,
I found that when using -no-kvm-irqchip -tdf with an ACPI enabled guest
(Fedora 7 x86_64) it was dying with the following kernel panic :

MP-BIOS bug: 8254 timer not connected to IO-APIC
Kernel panic - not syncing: IO-APIC + timer doesn't work! Try using the
'noapic' kernel parameter.

and pressummed I misread the code and got the wrong PIC; when that problem was
already happenning even without the patch.

so to resume, the original patch should be right, and probably closer to what
the original implementation of tdf was doing; but that original implementation
has a bug with ACPI and the current code works eventhough is obviously broken,
just because the compiler is packing the PicState2 struct in a way that makes
"s" and "&s->pics[0]" point to the same address.

>  also from my tests it might seem that tdf is irrelevant anyway with the new
>  clock work and haven't been able to find a case where enabling it (so
>  triggering this buggy code path) migh be needed.
> 
>  any one care to comment on any current users of tdf? and if there are none in
>  the viability for removing it?
> 
>   
> 
>    It does work but only for non-acpi guest that has the -no-kvm-irqchip
>    parameter.
>    To test it you can load your host and see what happens to the clock when
>    you run
>    a 1000HZ guest (use taskset to pin the guest with other cpu intensive
>    tasks.)
>    We decided not to fix it in the in-kernel pic since once the tpr
>    optimization enable
>    running acpi/apic guests and thus the pic is not used as time source.
>    Dor.

not sure if I follow what you meant here, but I had been able to reproduce the
time drifts at least when using -no-acpi

Carlo
> 
>  On Fri, Nov 09, 2007 at 11:22:10AM -0600, Carlo Marcelo Arenas Belon wrote:
>   
> 
>  The following patch fixes 1a483ef4040ed380bf69d684783d06a617073256 so that the
>  parent PIC pointer is used to send the edge irq0 instead of the PIC pair and
>  that is an incompatible pointer type as reported in :
> 
>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c: In function
>  `
>  pic_read_irq':
>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:248:
>  warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:249:
>  warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
> 
>  Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
>  ---
>   qemu/hw/i8259.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
> 
>  diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
>  index 01447d7..60063d4 100644
>  --- a/qemu/hw/i8259.c
>  +++ b/qemu/hw/i8259.c
>  @@ -245,8 +245,8 @@ int pic_read_irq(PicState2 *s)
>              if (timer_ints_to_push > 0) {
>                  timer_ints_to_push--;
>                   /* simulate an edge irq0, like the one generated by i8254 */
>  -                pic_set_irq1(s, 0, 0);
>  -                pic_set_irq1(s, 0, 1);
>  +                pic_set_irq1(&s->pics[0], 0, 0);
>  +                pic_set_irq1(&s->pics[0], 0, 1);
>              }
>          }
>  
>  --
>  1.5.2.5
> 
>     
> 
>  -------------------------------------------------------------------------
>  This SF.net email is sponsored by: Splunk Inc.
>  Still grepping through log files to find problems?  Stop.
>  Now Search log events and configuration files using AJAX and a browser.
>  Download your FREE copy of Splunk now >> http://get.splunk.com/
>  _______________________________________________
>  kvm-devel mailing list
>  kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>  https://lists.sourceforge.net/lists/listinfo/kvm-devel
> 
>   

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [RFC] qemu: simulate edge-triggered interrupt in master PIC for time-drift-fix

[Dor Laor]

[-- Attachment #1.1: Type: text/plain, Size: 4728 bytes --]

Carlo Marcelo Arenas Belon wrote:
> On Sat, Nov 10, 2007 at 12:35:34AM +0200, Dor Laor wrote:
>   
>>    Carlo Marcelo Arenas Belon wrote:
>>
>>  wrong patch, this was meant to go to the slave PIC, so it will use s->pics[1]
>>  instead of s->pics[0].
>>
>>    Why to the slave pic? Isn't the pit connected to line 0 of the master?
>>     
>
> yes, you are right; 8254 should be connected to line 0 of the master 8259 and
> using the slave PIC is not correct.
>
> sadly, with the patch and while trying to test if it make a difference or not,
> I found that when using -no-kvm-irqchip -tdf with an ACPI enabled guest
> (Fedora 7 x86_64) it was dying with the following kernel panic :
>
> MP-BIOS bug: 8254 timer not connected to IO-APIC
> Kernel panic - not syncing: IO-APIC + timer doesn't work! Try using the
> 'noapic' kernel parameter.
>
>   
It was only tested with non acpi/apic guests, mainly with windows.
If you'll use windows with stadard hal as a guest you'll see the difference.
For acpi/apic guest the -tdf is not relevant because they dont use the 
pit as timesource and use apic/acpi timers.
> and pressummed I misread the code and got the wrong PIC; when that problem was
> already happenning even without the patch.
>
> so to resume, the original patch should be right, and probably closer to what
> the original implementation of tdf was doing; but that original implementation
> has a bug with ACPI and the current code works eventhough is obviously broken,
> just because the compiler is packing the PicState2 struct in a way that makes
> "s" and "&s->pics[0]" point to the same address.
>
>   
You're right about this and we should apply the patch.
>>  also from my tests it might seem that tdf is irrelevant anyway with the new
>>  clock work and haven't been able to find a case where enabling it (so
>>  triggering this buggy code path) migh be needed.
>>
>>  any one care to comment on any current users of tdf? and if there are none in
>>  the viability for removing it?
>>
>>   
>>
>>    It does work but only for non-acpi guest that has the -no-kvm-irqchip
>>    parameter.
>>    To test it you can load your host and see what happens to the clock when
>>    you run
>>    a 1000HZ guest (use taskset to pin the guest with other cpu intensive
>>    tasks.)
>>    We decided not to fix it in the in-kernel pic since once the tpr
>>    optimization enable
>>    running acpi/apic guests and thus the pic is not used as time source.
>>    Dor.
>>     
>
> not sure if I follow what you meant here, but I had been able to reproduce the
> time drifts at least when using -no-acpi
>
> Carlo
>   
>>  On Fri, Nov 09, 2007 at 11:22:10AM -0600, Carlo Marcelo Arenas Belon wrote:
>>   
>>
>>  The following patch fixes 1a483ef4040ed380bf69d684783d06a617073256 so that the
>>  parent PIC pointer is used to send the edge irq0 instead of the PIC pair and
>>  that is an incompatible pointer type as reported in :
>>
>>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c: In function
>>  `
>>  pic_read_irq':
>>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:248:
>>  warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
>>  /var/tmp/portage/app-emulation/kvm-51/work/kvm-51/qemu/hw/i8259.c:249:
>>  warning: passing arg 1 of `pic_set_irq1' from incompatible pointer type
>>
>>  Signed-off-by: Carlo Marcelo Arenas Belon <carenas-kLeDWSohozoJb6fo7hG9ng@public.gmane.org>
>>  ---
>>   qemu/hw/i8259.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>>  diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
>>  index 01447d7..60063d4 100644
>>  --- a/qemu/hw/i8259.c
>>  +++ b/qemu/hw/i8259.c
>>  @@ -245,8 +245,8 @@ int pic_read_irq(PicState2 *s)
>>              if (timer_ints_to_push > 0) {
>>                  timer_ints_to_push--;
>>                   /* simulate an edge irq0, like the one generated by i8254 */
>>  -                pic_set_irq1(s, 0, 0);
>>  -                pic_set_irq1(s, 0, 1);
>>  +                pic_set_irq1(&s->pics[0], 0, 0);
>>  +                pic_set_irq1(&s->pics[0], 0, 1);
>>              }
>>          }
>>  
>>  --
>>  1.5.2.5
>>
>>     
>>
>>  -------------------------------------------------------------------------
>>  This SF.net email is sponsored by: Splunk Inc.
>>  Still grepping through log files to find problems?  Stop.
>>  Now Search log events and configuration files using AJAX and a browser.
>>  Download your FREE copy of Splunk now >> http://get.splunk.com/
>>  _______________________________________________
>>  kvm-devel mailing list
>>  kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>>  https://lists.sourceforge.net/lists/listinfo/kvm-devel
>>
>>   
>>     
>
>   


[-- Attachment #1.2: Type: text/html, Size: 5660 bytes --]

[-- Attachment #2: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel