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

[Dor Laor <dor.laor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- 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