[Qemu-devel] Re: [PATCH 11/16] hpet/rtc: Rework RTC IRQ replacement by HPET

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 4694 bytes --]

Blue Swirl wrote:
> On Sun, Jun 6, 2010 at 8:11 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Allow the intercept the RTC IRQ for the HPET legacy mode. Then push
>> routing to IRQ8 completely into the HPET. This allows to turn
>> hpet_in_legacy_mode() into a private function. Furthermore, this stops
>> the RTC from clearing IRQ8 even if the HPET is in control.
>>
>> This patch comes with a side effect: The RTC timers will no longer be
>> stoppend when there is no IRQ consumer, possibly causing a minor
>> performance degration. But as the guest may want to redirect the RTC to
>> the SCI in that mode, it should normally disable unused IRQ source
>> anyway.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/hpet.c        |   42 +++++++++++++++++++++++++++++++++++-------
>>  hw/hpet_emul.h   |    4 ----
>>  hw/mc146818rtc.c |   54 +++++++++++++++---------------------------------------
>>  hw/mc146818rtc.h |    4 +++-
>>  hw/mips_jazz.c   |    2 +-
>>  hw/mips_malta.c  |    2 +-
>>  hw/mips_r4k.c    |    2 +-
>>  hw/pc.c          |   14 ++++++++------
>>  hw/ppc_prep.c    |    2 +-
>>  9 files changed, 65 insertions(+), 61 deletions(-)
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index 041dd84..d26cad5 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -30,6 +30,7 @@
>>  #include "qemu-timer.h"
>>  #include "hpet_emul.h"
>>  #include "sysbus.h"
>> +#include "mc146818rtc.h"
>>
>>  //#define HPET_DEBUG
>>  #ifdef HPET_DEBUG
>> @@ -58,6 +59,7 @@ typedef struct HPETState {
>>     SysBusDevice busdev;
>>     uint64_t hpet_offset;
>>     qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
>> +    uint8_t rtc_irq_level;
>>     HPETTimer timer[HPET_NUM_TIMERS];
>>
>>     /* Memory-mapped, software visible registers */
>> @@ -69,12 +71,9 @@ typedef struct HPETState {
>>
>>  static HPETState *hpet_statep;
>>
>> -uint32_t hpet_in_legacy_mode(void)
>> +static uint32_t hpet_in_legacy_mode(HPETState *s)
>>  {
>> -    if (!hpet_statep) {
>> -        return 0;
>> -    }
>> -    return hpet_statep->config & HPET_CFG_LEGACY;
>> +    return s->config & HPET_CFG_LEGACY;
>>  }
>>
>>  static uint32_t timer_int_route(struct HPETTimer *timer)
>> @@ -166,12 +165,12 @@ static void update_irq(struct HPETTimer *timer)
>>  {
>>     int route;
>>
>> -    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
>> +    if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
>>         /* if LegacyReplacementRoute bit is set, HPET specification requires
>>          * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
>>          * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
>>          */
>> -        route = (timer->tn == 0) ? 0 : 8;
>> +        route = (timer->tn == 0) ? 0 : RTC_ISA_IRQ;
>>     } else {
>>         route = timer_int_route(timer);
>>     }
>> @@ -515,8 +514,10 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
>>             /* i8254 and RTC are disabled when HPET is in legacy mode */
>>             if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
>>                 hpet_pit_disable();
>> +                qemu_irq_lower(s->irqs[RTC_ISA_IRQ]);
>>             } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
>>                 hpet_pit_enable();
>> +                qemu_set_irq(s->irqs[RTC_ISA_IRQ], s->rtc_irq_level);
>>             }
>>             break;
>>         case HPET_CFG + 4:
>> @@ -607,6 +608,30 @@ static void hpet_reset(DeviceState *d)
>>     count = 1;
>>  }
>>
>> +static void hpet_rtc_delivery_cb(qemu_irq irq, void *opaque, int n, int level,
>> +                                 int result)
>> +{
>> +    qemu_irq orig_irq = opaque;
>> +
>> +    qemu_irq_fire_delivery_cb(orig_irq, level, result);
>> +}
>> +
>> +static void hpet_handle_rtc_irq(qemu_irq irq, void *opaque, int n, int level)
>> +{
>> +    HPETState *s = FROM_SYSBUS(HPETState, opaque);
>> +    IRQMsg msg = {
>> +        .delivery_cb = hpet_rtc_delivery_cb,
>> +        .delivery_opaque = irq,
>> +    };
>> +
>> +    s->rtc_irq_level = level;
>> +    if (hpet_in_legacy_mode(s)) {
>> +        qemu_irq_fire_delivery_cb(irq, level, QEMU_IRQ_MASKED);
>> +    } else {
>> +        qemu_set_irq_msg(s->irqs[RTC_ISA_IRQ], level, &msg);
> 
> This is the problem with passing around stack allocated objects: after
> this function finishes, s->irqs[RTC_ISA_IRQ].msg is a dangling pointer
> to some stack space.

s->irqs[RTC_ISA_IRQ].msg is NULL when qemu_set_irq_msg returned, msg
itself will not "leak" out of the qemu_irq subsystem.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]