Patchwork [v3,1/5] hpet 'driftfix': add hooks required to detect coalesced interrupts (x86 apic only)

login
register
mail settings
Submitter Ulrich Obergfell
Date April 28, 2011, 2:24 p.m.
Message ID <1304000700-3640-2-git-send-email-uobergfe@redhat.com>
Download mbox | patch
Permalink /patch/93223/
State New
Headers show

Comments

Ulrich Obergfell - April 28, 2011, 2:24 p.m.
'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
entry addresses of functions that are utilized by update_irq() to
detect coalesced interrupts. apic code loads these pointers during
initialization.

This change can be replaced if a generic feedback infrastructure to
track coalesced IRQs for periodic, clock providing devices becomes
available.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 hw/apic.c |    4 ++++
 sysemu.h  |    3 +++
 vl.c      |    3 +++
 3 files changed, 10 insertions(+), 0 deletions(-)
Blue Swirl - April 28, 2011, 6:51 p.m.
On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
> entry addresses of functions that are utilized by update_irq() to
> detect coalesced interrupts. apic code loads these pointers during
> initialization.

I'm not so happy with this approach, but probably then the i386
dependencies can be removed from RTC and it can be compiled only once
for all targets.

> This change can be replaced if a generic feedback infrastructure to
> track coalesced IRQs for periodic, clock providing devices becomes
> available.
>
> Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
> ---
>  hw/apic.c |    4 ++++
>  sysemu.h  |    3 +++
>  vl.c      |    3 +++
>  3 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index a45b57f..eb0f6d9 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -24,6 +24,7 @@
>  #include "sysbus.h"
>  #include "trace.h"
>  #include "kvm.h"
> +#include "sysemu.h"
>
>  /* APIC Local Vector Table */
>  #define APIC_LVT_TIMER   0
> @@ -1143,6 +1144,9 @@ static SysBusDeviceInfo apic_info = {
>
>  static void apic_register_devices(void)
>  {
> +    target_get_irq_delivered = apic_get_irq_delivered;
> +    target_reset_irq_delivered = apic_reset_irq_delivered;
> +
>     sysbus_register_withprop(&apic_info);
>  }
>
> diff --git a/sysemu.h b/sysemu.h
> index 07d85cd..75b0139 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -98,6 +98,9 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
>  void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
>  int qemu_loadvm_state(QEMUFile *f);
>
> +extern int (*target_get_irq_delivered)(void);
> +extern void (*target_reset_irq_delivered)(void);
> +
>  /* SLIRP */
>  void do_info_slirp(Monitor *mon);
>
> diff --git a/vl.c b/vl.c
> index 0c24e07..7bab315 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -233,6 +233,9 @@ const char *prom_envs[MAX_PROM_ENVS];
>  const char *nvram = NULL;
>  int boot_menu;
>
> +int (*target_get_irq_delivered)(void) = 0;
> +void (*target_reset_irq_delivered)(void) = 0;

Instead of initializing with 0 (should be actually NULL in C), please
define stub functions, which are used by default. Then the users don't
need to check for NULL pointers.
Jan Kiszka - April 28, 2011, 10:50 p.m.
On 2011-04-28 20:51, Blue Swirl wrote:
> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>> entry addresses of functions that are utilized by update_irq() to
>> detect coalesced interrupts. apic code loads these pointers during
>> initialization.
> 
> I'm not so happy with this approach, but probably then the i386
> dependencies can be removed from RTC and it can be compiled only once
> for all targets.

This whole series is really the minimalistic approach. The callbacks
defined here must remain a temporary "shortcut". Just like proper
abstraction of periodic tick compensation for reuse in other timers has
to be added later on. And the limitation to edge-triggered legacy HPET
INTs has to be removed.

Jan
Ulrich Obergfell - April 29, 2011, 9:45 a.m.
> On 2011-04-28 20:51, Blue Swirl wrote:
>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>> entry addresses of functions that are utilized by update_irq() to
>>> detect coalesced interrupts. apic code loads these pointers during
>>> initialization.
>>
>> I'm not so happy with this approach, but probably then the i386
>> dependencies can be removed from RTC and it can be compiled only once
>> for all targets.
> 
> This whole series is really the minimalistic approach. The callbacks
> defined here must remain a temporary "shortcut". Just like proper
> abstraction of periodic tick compensation for reuse in other timers has
> to be added later on. And the limitation to edge-triggered legacy HPET
> INTs has to be removed.

Since QEMU doesn't have a generic infrastructure to track interrupt
delivery, I decided to reuse something that is currently available.
The only mechanism that I'm aware of is the one that is utilized by
RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.

The changes that would be introduced by part 1/5 and part 4/5 of
this patch series could be replaced if a generic infrastructure
to track interrupt delivery becomes available.

Regards,

Uli
Jan Kiszka - April 29, 2011, 10:15 a.m.
On 2011-04-29 11:45, Ulrich Obergfell wrote:
> 
>> On 2011-04-28 20:51, Blue Swirl wrote:
>>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>>> entry addresses of functions that are utilized by update_irq() to
>>>> detect coalesced interrupts. apic code loads these pointers during
>>>> initialization.
>>>
>>> I'm not so happy with this approach, but probably then the i386
>>> dependencies can be removed from RTC and it can be compiled only once
>>> for all targets.
>>
>> This whole series is really the minimalistic approach. The callbacks
>> defined here must remain a temporary "shortcut". Just like proper
>> abstraction of periodic tick compensation for reuse in other timers has
>> to be added later on. And the limitation to edge-triggered legacy HPET
>> INTs has to be removed.
> 
> Since QEMU doesn't have a generic infrastructure to track interrupt
> delivery, I decided to reuse something that is currently available.
> The only mechanism that I'm aware of is the one that is utilized by
> RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.
> 
> The changes that would be introduced by part 1/5 and part 4/5 of
> this patch series could be replaced if a generic infrastructure
> to track interrupt delivery becomes available.

Right, and I hope you have the resources to continue working on this
topic in the foreseeable future.

Jan
Blue Swirl - April 29, 2011, 9:44 p.m.
On Fri, Apr 29, 2011 at 12:45 PM, Ulrich Obergfell <uobergfe@redhat.com> wrote:
>
>> On 2011-04-28 20:51, Blue Swirl wrote:
>>> On Thu, Apr 28, 2011 at 5:24 PM, Ulrich Obergfell wrote:
>>>> 'target_get_irq_delivered' and 'target_reset_irq_delivered' contain
>>>> entry addresses of functions that are utilized by update_irq() to
>>>> detect coalesced interrupts. apic code loads these pointers during
>>>> initialization.
>>>
>>> I'm not so happy with this approach, but probably then the i386
>>> dependencies can be removed from RTC and it can be compiled only once
>>> for all targets.
>>
>> This whole series is really the minimalistic approach. The callbacks
>> defined here must remain a temporary "shortcut". Just like proper
>> abstraction of periodic tick compensation for reuse in other timers has
>> to be added later on. And the limitation to edge-triggered legacy HPET
>> INTs has to be removed.
>
> Since QEMU doesn't have a generic infrastructure to track interrupt
> delivery, I decided to reuse something that is currently available.
> The only mechanism that I'm aware of is the one that is utilized by
> RTC code ('rtc_td_hack'), i.e. apic_get_irq_delivered() etc.

I think your approach is slightly better (while not the best possible
one), so it should be used also for RTC.

Some more comments:
- instead of global variables, there could be a setter function (maybe inline)
- pc.h would be a better place instead of sysemu.h

> The changes that would be introduced by part 1/5 and part 4/5 of
> this patch series could be replaced if a generic infrastructure
> to track interrupt delivery becomes available.

OK.

Patch

diff --git a/hw/apic.c b/hw/apic.c
index a45b57f..eb0f6d9 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -24,6 +24,7 @@ 
 #include "sysbus.h"
 #include "trace.h"
 #include "kvm.h"
+#include "sysemu.h"
 
 /* APIC Local Vector Table */
 #define APIC_LVT_TIMER   0
@@ -1143,6 +1144,9 @@  static SysBusDeviceInfo apic_info = {
 
 static void apic_register_devices(void)
 {
+    target_get_irq_delivered = apic_get_irq_delivered;
+    target_reset_irq_delivered = apic_reset_irq_delivered;
+
     sysbus_register_withprop(&apic_info);
 }
 
diff --git a/sysemu.h b/sysemu.h
index 07d85cd..75b0139 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -98,6 +98,9 @@  int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
 void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
 int qemu_loadvm_state(QEMUFile *f);
 
+extern int (*target_get_irq_delivered)(void);
+extern void (*target_reset_irq_delivered)(void);
+
 /* SLIRP */
 void do_info_slirp(Monitor *mon);
 
diff --git a/vl.c b/vl.c
index 0c24e07..7bab315 100644
--- a/vl.c
+++ b/vl.c
@@ -233,6 +233,9 @@  const char *prom_envs[MAX_PROM_ENVS];
 const char *nvram = NULL;
 int boot_menu;
 
+int (*target_get_irq_delivered)(void) = 0;
+void (*target_reset_irq_delivered)(void) = 0;
+
 typedef struct FWBootEntry FWBootEntry;
 
 struct FWBootEntry {