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

Submitted by Ulrich Obergfell on April 28, 2011, 2:24 p.m.

Details

Message ID 1304000700-3640-2-git-send-email-uobergfe@redhat.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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 {