On 09/09/2011 12:55, Alexander Graf wrote:
>
> On 09.09.2011, at 12:36, Fabien Chouteau wrote:
>
>> On 07/09/2011 21:59, Alexander Graf wrote:
>>>
>>> On 07.09.2011, at 16:41, Fabien Chouteau wrote:
>>>
>>>> On 06/09/2011 21:33, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> Am 01.09.2011 um 10:20 schrieb Fabien Chouteau <chout...@adacore.com
>>>>> <mailto:chout...@adacore.com>>:
>>>>>
>>>>>> While working on the emulation of the freescale p2010 (e500v2) I
>>>>>> realized that
>>>>>> there's no implementation of booke's timers features. Currently mpc8544
>>>>>> uses
>>>>>> ppc_emb (ppc_emb_timers_init) which is close but not exactly like booke
>>>>>> (for
>>>>>> example booke uses different SPR).
>>>>>>
>>>>>> Signed-off-by: Fabien Chouteau <chout...@adacore.com
>>>>>> <mailto:chout...@adacore.com>>
>>>>>> ---
>>>>>> Makefile.target | 2 +-
>>>>>> hw/ppc.c | 133 ++++++++--------------
>>>>>> hw/ppc.h | 25 ++++-
>>>>>> hw/ppc4xx_devs.c | 2 +-
>>>>>> hw/ppc_booke.c | 263
>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>> hw/ppce500_mpc8544ds.c | 4 +-
>>>>>> hw/virtex_ml507.c | 11 +--
>>>>>> target-ppc/cpu.h | 29 +++++
>>>>>> target-ppc/translate_init.c | 43 +++++++-
>>>>>> 9 files changed, 412 insertions(+), 100 deletions(-)
>>>>>> create mode 100644 hw/ppc_booke.c
>>>>>>
>>>>>> diff --git a/Makefile.target b/Makefile.target
>>>>>> index 07af4d4..c8704cd 100644
>>>>>> --- a/Makefile.target
>>>>>> +++ b/Makefile.target
>>>>>> @@ -234,7 +234,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
>>>>>> obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>>>>
>>>>>> # shared objects
>>>>>> -obj-ppc-y = ppc.o
>>>>>> +obj-ppc-y = ppc.o ppc_booke.o
>>>>>> obj-ppc-y += vga.o
>>>>>> # PREP target
>>>>>> obj-ppc-y += i8259.o mc146818rtc.o
>>>>>> diff --git a/hw/ppc.c b/hw/ppc.c
>>>>>> index 8870748..bcb1e91 100644
>>>>>> --- a/hw/ppc.c
>>>>>> +++ b/hw/ppc.c
>>>>>> @@ -50,7 +50,7 @@
>>>>>> static void cpu_ppc_tb_stop (CPUState *env);
>>>>>> static void cpu_ppc_tb_start (CPUState *env);
>>>>>>
>>>>>> -static void ppc_set_irq (CPUState *env, int n_IRQ, int level)
>>>>>> +void ppc_set_irq(CPUState *env, int n_IRQ, int level)
>>>>>> {
>>>>>> unsigned int old_pending = env->pending_interrupts;
>>>>>>
>>>>>> @@ -423,25 +423,8 @@ void ppce500_irq_init (CPUState *env)
>>>>>> }
>>>>>> /*****************************************************************************/
>>>>>> /* PowerPC time base and decrementer emulation */
>>>>>> -struct ppc_tb_t {
>>>>>> - /* Time base management */
>>>>>> - int64_t tb_offset; /* Compensation */
>>>>>> - int64_t atb_offset; /* Compensation */
>>>>>> - uint32_t tb_freq; /* TB frequency */
>>>>>> - /* Decrementer management */
>>>>>> - uint64_t decr_next; /* Tick for next decr interrupt */
>>>>>> - uint32_t decr_freq; /* decrementer frequency */
>>>>>> - struct QEMUTimer *decr_timer;
>>>>>> - /* Hypervisor decrementer management */
>>>>>> - uint64_t hdecr_next; /* Tick for next hdecr interrupt */
>>>>>> - struct QEMUTimer *hdecr_timer;
>>>>>> - uint64_t purr_load;
>>>>>> - uint64_t purr_start;
>>>>>> - void *opaque;
>>>>>> -};
>>>>>>
>>>>>> -static inline uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk,
>>>>>> - int64_t tb_offset)
>>>>>> +uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t
>>>>>> tb_offset)
>>>>>> {
>>>>>> /* TB time in tb periods */
>>>>>> return muldiv64(vmclk, tb_env->tb_freq, get_ticks_per_sec()) +
>>>>>> tb_offset;
>>>>>> @@ -611,10 +594,13 @@ static inline uint32_t _cpu_ppc_load_decr(CPUState
>>>>>> *env, uint64_t next)
>>>>>> int64_t diff;
>>>>>>
>>>>>> diff = next - qemu_get_clock_ns(vm_clock);
>>>>>> - if (diff >= 0)
>>>>>> + if (diff >= 0) {
>>>>>> decr = muldiv64(diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>>> - else
>>>>>> + } else if (env->insns_flags & PPC_BOOKE) {
>>>>>> + decr = 0;
>>>>>
>>>>> I don't think we should expose instruction interpreter details into the
>>>>> timing code. It'd be better to have a separate flag that gets set on the
>>>>> booke
>>>>> timer init function which would be stored in tb_env. Keeps things better
>>>>> separated :)
>>>>
>>>> Good idea, but how do we set the flags? ppc_booke_timers_init doesn't know
>>>> which processor is emulated.
>>>
>>> We could have two different init functions. One for normal booke and one for
>>> e500. Or we pass in timer flags to the init function.
>>
>> How can we handle the -cpu option? For example if I want to test a ppc604 on
>> my
>> p2010 board? I'm not sure if it really makes sense...
>
> I guess at the end of the day we need to have the CPU initialize its timers.
> They are tightly coupled with the CPU anyways. For now, we can leave the
> instantiation as is though.
>
>>
>>
>>>>
>>>>
>>>>>
>>>>>> + } else {
>>>>>> decr = -muldiv64(-diff, tb_env->decr_freq, get_ticks_per_sec());
>>>>>> + }
>>>>>> LOG_TB("%s: %08" PRIx32 "\n", __func__, decr);
>>>>>>
>>>>>> return decr;
>>>>>> @@ -678,18 +664,23 @@ static void __cpu_ppc_store_decr (CPUState *env,
>>>>>> uint64_t *nextp,
>>>>>> decr, value);
>>>>>> now = qemu_get_clock_ns(vm_clock);
>>>>>> next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
>>>>>> - if (is_excp)
>>>>>> + if (is_excp) {
>>>>>> next += *nextp - now;
>>>>>> - if (next == now)
>>>>>> + }
>>>>>> + if (next == now) {
>>>>>> next++;
>>>>>> + }
>>>>>> *nextp = next;
>>>>>> /* Adjust timer */
>>>>>> qemu_mod_timer(timer, next);
>>>>>> /* If we set a negative value and the decrementer was positive,
>>>>>> - * raise an exception.
>>>>>> + * raise an exception (not for booke).
>>>>>> */
>>>>>> - if ((value & 0x80000000) && !(decr & 0x80000000))
>>>>>> + if (!(env->insns_flags & PPC_BOOKE)
>>>>>> + && (value & 0x80000000)
>>>>>> + && !(decr & 0x80000000)) {
>>>>>> (*raise_excp)(env);
>>>>>
>>>>> Please make this a separate flag too. IIRC this is not unified behavior
>>>>> on all ppc CPUs, not even all server type ones.
>>>>>
>>>>
>>>> This come from a comment by Scott (CC'd), I don't know much about it. Can
>>>> you
>>>> help me to find a good name for this feature?
>>>
>>> PPC_DECR_TRIGGER_ON_NEGATIVE? :)
>>>
>>
>> After a quick check, PPC_DECR_UNDERFLOW_TRIGGERED seems more appropriate.
>
> Good :).
>
> [...]
>
>>>>>> +/* Return the location of the bit of time base at which the FIT will
>>>>>> raise an
>>>>>> + interrupt */
>>>>>> +static uint8_t booke_get_fit_target(CPUState *env)
>>>>>> +{
>>>>>> + uint8_t fp = (env->spr[SPR_BOOKE_TCR] & TCR_FP_MASK) >>
>>>>>> TCR_FP_SHIFT;
>>>>>> +
>>>>>> + /* Only for e500 */
>>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>>
>>>>> Same here again :). Do you have test cases for fit? IIRC Linux doesn't
>>>>> use it.
>>>>
>>>> VxWorks uses it.
>>>
>>> If even remotely possible, I'd really like to have something in my portfolio
>>> of guest code to test this case - especially given that KVM implements its
>>> own timers. I assume your binary is non-redistributable?
>>
>> No I can't give you the binary. Maybe the best solution is to write a simple
>> test case from scratch.
>
> Yeah, I'll poke and see if someone already has something there.
>
>>
>>
>>>>
>>>>>
>>>>>> + uint32_t fpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_FPEXT_MASK)
>>>>>> + >> TCR_E500_FPEXT_SHIFT;
>>>>>> + fp = 63 - (fp | fpext << 2);
>>>>>> + } else {
>>>>>> + fp = env->fit_period[fp];
>>>>>> + }
>>>>>> +
>>>>>> + return fp;
>>>>>> +}
>>>>>> +
>>>>>> +/* Return the location of the bit of time base at which the WDT will
>>>>>> raise an
>>>>>> + interrupt */
>>>>>> +static uint8_t booke_get_wdt_target(CPUState *env)
>>>>>> +{
>>>>>> + uint8_t wp = (env->spr[SPR_BOOKE_TCR] & TCR_WP_MASK) >>
>>>>>> TCR_WP_SHIFT;
>>>>>> +
>>>>>> + /* Only for e500 */
>>>>>> + if (env->insns_flags2 & PPC2_E500) {
>>>>>> + uint32_t wpext = (env->spr[SPR_BOOKE_TCR] & TCR_E500_WPEXT_MASK)
>>>>>> + >> TCR_E500_WPEXT_SHIFT;
>>>>>> + wp = 63 - (wp | wpext << 2);
>>>>>> + } else {
>>>>>> + wp = env->wdt_period[wp];
>>>>>> + }
>>>>>> +
>>>>>> + return wp;
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_update_fixed_timer(CPUState *env,
>>>>>> + uint8_t target_bit,
>>>>>> + uint64_t *next,
>>>>>> + struct QEMUTimer *timer)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + uint64_t lapse;
>>>>>> + uint64_t tb;
>>>>>> + uint64_t period = 1 << (target_bit + 1);
>>>>>> + uint64_t now;
>>>>>> +
>>>>>> + now = qemu_get_clock_ns(vm_clock);
>>>>>> + tb = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
>>>>>> +
>>>>>> + lapse = period - ((tb - (1 << target_bit)) & (period - 1));
>>>>>> +
>>>>>> + *next = now + muldiv64(lapse, get_ticks_per_sec(), tb_env->tb_freq);
>>>>>> +
>>>>>> + if (*next == now) {
>>>>>> + (*next)++;
>>>>>
>>>>> Huh? If we hit the timer we don't fire it?
>>>>
>>>> Yes we do, but one nanosecond later :)
>>>
>>> Well, why trigger a timer when we can just as well call the callback
>>> immediately? :)
>>>
>>
>> This come from ppc.c, QEMUTimer is kind of a private type so we don't have
>> access to the callback.
>
> Oh, I see. Please just add that as an XXX comment so we can resolve it when
> we get around to it.
>
>>
>>>>
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> + qemu_mod_timer(timer, *next);
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_decr_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_DIS;
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_DIE) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>> + }
>>>>>
>>>>> You will need this more often - also for the TCR write case - so please
>>>>> put
>>>>> the 3 lines above into a separate function and just call that here :)
>>>>
>>>> Actually the checks are never exactly the same, here we test DIE in TCR...
>>>
>>> Sure, we have 3 different tests for the 3 different bits we can potentially
>>> set in TCR. The check always ends up being the same though:
>>>
>>> if (TSR & bit && TCR & bit)
>>> set_irq(irq_for_bit);
>>>
>>> Most device emulators have a simple function for this called "update_irq"
>>> that checks for all the bits and sets the respective irq lines.
>>>
>>
>> I know but we have two cases:
>>
>> - Timer hit: we check DIE in TCR
>> - Rising edge of DIE in TCR (from user): check if DIS is set
>>
>> I don't think we can have a good generic function for this, and I don't
>> forecast any improvement in code readability.
>
> update_decr_irq() {
> if (TSR.DIS && TCR.DIE) {
> set_irq(DECR);
> } else {
> unset_irq(DECR);
> }
> }
>
> Timer hit:
>
> TSR |= DIS;
> update_decr_irq();
>
> Setting TCR:
>
> TCR |= DIE;
> update_decr_irq();
>
> Or am I misunderstanding the conditions under which the irq actually
> triggers? TCR.DIE is only the interrupt enabled flag - the timer can still
> hit nevertheless. The level of the interrupt is determined by TSR.DIR which
> is what the timer sets when it hits. Unless I completely misread the spec, an
> interrupt occurs when both of them are true. So all we need to do is have
> that check and run it every time we change a value in TSR or TCR.
Well OK, this can work to trigger the interrupts, not to clear them though.
And it will call ppc_set_irq when it's not required.
>>
>>>>
>>>>
>>>>>> +
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
>>>>>> + /* Auto Reload */
>>>>>> + cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
>>>>>> + }
>>>>>
>>>>> Ah nice - never seen this one used. Do you have a test case?
>>>>>
>>>>
>>>> VxWorks :)
>>>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_fit_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> + env->spr[SPR_BOOKE_TSR] |= TSR_FIS;
>>>>>> + if (env->spr[SPR_BOOKE_TCR] & TCR_FIE) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> + }
>>>>>
>>>>> Same as above :)
>>>>>
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_fit_target(env),
>>>>>> + &booke_timer->fit_next,
>>>>>> + booke_timer->fit_timer);
>>>>>> +}
>>>>>> +
>>>>>> +static void booke_wdt_cb(void *opaque)
>>>>>> +{
>>>>>> + CPUState *env;
>>>>>> + ppc_tb_t *tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + env = opaque;
>>>>>> + tb_env = env->tb_env;
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + /* TODO: There's lots of complicated stuff to do here */
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_wdt_target(env),
>>>>>> + &booke_timer->wdt_next,
>>>>>> + booke_timer->wdt_timer);
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tsr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + booke_timer_t *booke_timer;
>>>>>> +
>>>>>> + booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + env->spr[SPR_BOOKE_TSR] &= ~val;
>>>>>> +
>>>>>> + if (val & TSR_DIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 0);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TSR_FIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 0);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TSR_WIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 0);
>>>>>> + }
>>>>>
>>>>> Why do you need the above? Shouldn't they still be active if the guest
>>>>> didn't
>>>>> reset the bit? This is probably hiding a bug in the interrupt delivery
>>>>> mechanism automatically unmasking an irq bit when it's delivered, right?
>>>>
>>>> They are active until the bit is cleared by user, and TSR is a
>>>> write-1-to-clear
>>>> register.
>>>
>>> Yes, that's what I mean. There shouldn't be a need to set the irq again
>>> because it should still be active. These interrupts are level based.
>>>
>>
>> I feel like there's a misunderstanding here. I do not set the interrupts I
>> clear them.
>
> Oh. Yes, I misread that. Sorry :). That indeed does make a lot of sense. I
> can however be folded in with the normal "is DECR active right now" check.
>
>>
>>
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +void store_booke_tcr(CPUState *env, target_ulong val)
>>>>>> +{
>>>>>> + ppc_tb_t *tb_env = env->tb_env;
>>>>>> + booke_timer_t *booke_timer = tb_env->opaque;
>>>>>> +
>>>>>> + tb_env = env->tb_env;
>>>>>> + env->spr[SPR_BOOKE_TCR] = val;
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_fit_target(env),
>>>>>> + &booke_timer->fit_next,
>>>>>> + booke_timer->fit_timer);
>>>>>> +
>>>>>> + booke_update_fixed_timer(env,
>>>>>> + booke_get_wdt_target(env),
>>>>>> + &booke_timer->wdt_next,
>>>>>> + booke_timer->wdt_timer);
>>>>>> +
>>>>>> + if (val & TCR_DIE && env->spr[SPR_BOOKE_TSR] & TSR_DIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_DECR, 1);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TCR_FIE && env->spr[SPR_BOOKE_TSR] & TSR_FIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_FIT, 1);
>>>>>> + }
>>>>>> +
>>>>>> + if (val & TCR_WIE && env->spr[SPR_BOOKE_TSR] & TSR_WIS) {
>>>>>> + ppc_set_irq(env, PPC_INTERRUPT_WDT, 1);
>>>>>> + }
>>>>>
>>>>> Here is the second user of the checks. It really does make sense to only
>>>>> have
>>>>> them in a single spot, so that ever ppc_set_irq(DECR) always goes through
>>>>> the
>>>>> TSR and TCR checks for example :).
>>>>
>>>> ...here we test DIE in TCR and DIS in TSR.
>>>
>>> Yes, both of which are basically the prerequisites for actually triggering
>>> the internal DECR interrupt line.
>>>
>>
>> Not exactly, see above.
>
> [...]
>
>>>>
>>>>> Very nice patch however! Thanks a lot for sitting down and fixing the
>>>>> timer
>>>>> mess on booke that we currently have.
>>>>
>>>> You are welcome, It's my thank you gift for the MMU ;)
>>>
>>> Haha thanks :). So you're going to fix the MPIC mess for me implementing
>>> SMP support? :D
>>>
>>
>> I'll see, but I'm not sure you deserve it :)
>
> Probably not :). What else is on your todo list to get your awesome guest
> running? :)
>
Actually it works pretty well with VxWorks already, I still have to clean up
few things and play with this new memory API to implement the CCSR
reallocation.
I've tried to look at Liu's patch, but I really don't know nothing about KVM so
I won't be able to make any valuable comment...
Regards,
--
Fabien Chouteau
