[Qemu-devel] [PATCH 0/3] sun4u: timer fixups

[Qemu-devel] [PATCH 0/3] sun4u: timer fixups

[Mark Cave-Ayland]

Whilst trying to boot FreeBSD SPARC64, it became apparent that the existing
timer code doesn't behave correctly with respect to the NPT and INT_DIS
bits. This patchset corrects this behaviour and allows FreeBSD SPARC64 boot
to progress further.

Many thanks to Marius Strobl <marius@alchemy.franken.de> for pointing me in
the right direction.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (3):
  sun4u: split out NPT and INT_DIS into separate CPUTimer fields
  sun4u: split NPT and INT_DIS accesses between timer and compare
    registers
  target-sparc: implement NPT timer bit

 hw/sparc64/sun4u.c       |   36 +++++++++++++++++++++++-------------
 target-sparc/cpu.h       |    2 ++
 target-sparc/helper.c    |   10 ++++++++--
 target-sparc/helper.h    |    2 +-
 target-sparc/translate.c |   18 +++++++++++++++---
 5 files changed, 49 insertions(+), 19 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] sun4u: split out NPT and INT_DIS into separate CPUTimer fields

[Mark Cave-Ayland]

Currently there is confusion between use of these bits for the timer and timer
compare registers (while they both have the same value, the behaviour is
different). Split into two separate CPUTimer fields so we can always reference
the correct value.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc64/sun4u.c |   17 +++++++++++++----
 target-sparc/cpu.h |    2 ++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d6b929c..7153638 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -363,6 +363,8 @@ void cpu_put_timer(QEMUFile *f, CPUTimer *s)
     qemu_put_be32s(f, &s->frequency);
     qemu_put_be32s(f, &s->disabled);
     qemu_put_be64s(f, &s->disabled_mask);
+    qemu_put_be32s(f, &s->npt);
+    qemu_put_be64s(f, &s->npt_mask);
     qemu_put_sbe64s(f, &s->clock_offset);
 
     timer_put(f, s->qtimer);
@@ -373,6 +375,8 @@ void cpu_get_timer(QEMUFile *f, CPUTimer *s)
     qemu_get_be32s(f, &s->frequency);
     qemu_get_be32s(f, &s->disabled);
     qemu_get_be64s(f, &s->disabled_mask);
+    qemu_get_be32s(f, &s->npt);
+    qemu_get_be64s(f, &s->npt_mask);
     qemu_get_sbe64s(f, &s->clock_offset);
 
     timer_get(f, s->qtimer);
@@ -380,15 +384,17 @@ void cpu_get_timer(QEMUFile *f, CPUTimer *s)
 
 static CPUTimer *cpu_timer_create(const char *name, SPARCCPU *cpu,
                                   QEMUBHFunc *cb, uint32_t frequency,
-                                  uint64_t disabled_mask)
+                                  uint64_t disabled_mask, uint64_t npt_mask)
 {
     CPUTimer *timer = g_malloc0(sizeof (CPUTimer));
 
     timer->name = name;
     timer->frequency = frequency;
     timer->disabled_mask = disabled_mask;
+    timer->npt_mask = npt_mask;
 
     timer->disabled = 1;
+    timer->npt = 1;
     timer->clock_offset = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     timer->qtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cb, cpu);
@@ -799,13 +805,16 @@ static SPARCCPU *cpu_devinit(const char *cpu_model, const struct hwdef *hwdef)
     env = &cpu->env;
 
     env->tick = cpu_timer_create("tick", cpu, tick_irq,
-                                  tick_frequency, TICK_NPT_MASK);
+                                  tick_frequency, TICK_INT_DIS,
+                                  TICK_NPT_MASK);
 
     env->stick = cpu_timer_create("stick", cpu, stick_irq,
-                                   stick_frequency, TICK_INT_DIS);
+                                   stick_frequency, TICK_INT_DIS,
+                                   TICK_NPT_MASK);
 
     env->hstick = cpu_timer_create("hstick", cpu, hstick_irq,
-                                    hstick_frequency, TICK_INT_DIS);
+                                    hstick_frequency, TICK_INT_DIS,
+                                    TICK_NPT_MASK);
 
     reset_info = g_malloc0(sizeof(ResetData));
     reset_info->cpu = cpu;
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 9fa770b..4aa689e 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -366,6 +366,8 @@ struct CPUTimer
     uint32_t    frequency;
     uint32_t    disabled;
     uint64_t    disabled_mask;
+    uint32_t    npt;
+    uint64_t    npt_mask;
     int64_t     clock_offset;
     QEMUTimer  *qtimer;
 };
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/3] sun4u: split out NPT and INT_DIS into separate CPUTimer fields

[Peter Maydell]

On 13 November 2015 at 17:54, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Currently there is confusion between use of these bits for the timer and timer
> compare registers (while they both have the same value, the behaviour is
> different). Split into two separate CPUTimer fields so we can always reference
> the correct value.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/sparc64/sun4u.c |   17 +++++++++++++----
>  target-sparc/cpu.h |    2 ++
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index d6b929c..7153638 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -363,6 +363,8 @@ void cpu_put_timer(QEMUFile *f, CPUTimer *s)
>      qemu_put_be32s(f, &s->frequency);
>      qemu_put_be32s(f, &s->disabled);
>      qemu_put_be64s(f, &s->disabled_mask);
> +    qemu_put_be32s(f, &s->npt);
> +    qemu_put_be64s(f, &s->npt_mask);
>      qemu_put_sbe64s(f, &s->clock_offset);
>
>      timer_put(f, s->qtimer);
> @@ -373,6 +375,8 @@ void cpu_get_timer(QEMUFile *f, CPUTimer *s)
>      qemu_get_be32s(f, &s->frequency);
>      qemu_get_be32s(f, &s->disabled);
>      qemu_get_be64s(f, &s->disabled_mask);
> +    qemu_get_be32s(f, &s->npt);
> +    qemu_get_be64s(f, &s->npt_mask);
>      qemu_get_sbe64s(f, &s->clock_offset);
>
>      timer_get(f, s->qtimer);

Hi. I was just rebasing my sparc convert-to-vmstate patchset, and I
noticed this patch due to a conflict. This change breaks migration
and vmstate save/restore compatibility for these boards.
Making the field version-dependent is probably a bit awkward at
this point because these are just subfields in the overall CPU
state and share its version number (which in turn is shared with
the 32-bit CPUs).

Not sure what you want to do here?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] sun4u: split out NPT and INT_DIS into separate CPUTimer fields

[Mark Cave-Ayland]

On 08/01/16 14:05, Peter Maydell wrote:

> On 13 November 2015 at 17:54, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> Currently there is confusion between use of these bits for the timer and timer
>> compare registers (while they both have the same value, the behaviour is
>> different). Split into two separate CPUTimer fields so we can always reference
>> the correct value.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/sparc64/sun4u.c |   17 +++++++++++++----
>>  target-sparc/cpu.h |    2 ++
>>  2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index d6b929c..7153638 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -363,6 +363,8 @@ void cpu_put_timer(QEMUFile *f, CPUTimer *s)
>>      qemu_put_be32s(f, &s->frequency);
>>      qemu_put_be32s(f, &s->disabled);
>>      qemu_put_be64s(f, &s->disabled_mask);
>> +    qemu_put_be32s(f, &s->npt);
>> +    qemu_put_be64s(f, &s->npt_mask);
>>      qemu_put_sbe64s(f, &s->clock_offset);
>>
>>      timer_put(f, s->qtimer);
>> @@ -373,6 +375,8 @@ void cpu_get_timer(QEMUFile *f, CPUTimer *s)
>>      qemu_get_be32s(f, &s->frequency);
>>      qemu_get_be32s(f, &s->disabled);
>>      qemu_get_be64s(f, &s->disabled_mask);
>> +    qemu_get_be32s(f, &s->npt);
>> +    qemu_get_be64s(f, &s->npt_mask);
>>      qemu_get_sbe64s(f, &s->clock_offset);
>>
>>      timer_get(f, s->qtimer);
> 
> Hi. I was just rebasing my sparc convert-to-vmstate patchset, and I
> noticed this patch due to a conflict. This change breaks migration
> and vmstate save/restore compatibility for these boards.
> Making the field version-dependent is probably a bit awkward at
> this point because these are just subfields in the overall CPU
> state and share its version number (which in turn is shared with
> the 32-bit CPUs).
> 
> Not sure what you want to do here?

Hi Peter,

I'm not particularly worried about sun4u for the moment as there are
already other reasons why migration would fail, e.g. no
VMStateDescription for storing PCI interrupt state in the apb host bridge.

Last time I checked sun4m migration appeared to work under some very
light testing, so as long as this behaviour is preserved then I don't
see a problem.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/3] sun4u: split out NPT and INT_DIS into separate CPUTimer fields

[Peter Maydell]

On 8 January 2016 at 14:34, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> I'm not particularly worried about sun4u for the moment as there are
> already other reasons why migration would fail, e.g. no
> VMStateDescription for storing PCI interrupt state in the apb host bridge.
>
> Last time I checked sun4m migration appeared to work under some very
> light testing, so as long as this behaviour is preserved then I don't
> see a problem.

OK. Does this apply to all 64-bit SPARC CPUs? (There are some
things I can simplify in the CPU migration code if we can break
64-bit migration.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] sun4u: split out NPT and INT_DIS into separate CPUTimer fields

[Mark Cave-Ayland]

On 08/01/16 14:55, Peter Maydell wrote:

> On 8 January 2016 at 14:34, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> I'm not particularly worried about sun4u for the moment as there are
>> already other reasons why migration would fail, e.g. no
>> VMStateDescription for storing PCI interrupt state in the apb host bridge.
>>
>> Last time I checked sun4m migration appeared to work under some very
>> light testing, so as long as this behaviour is preserved then I don't
>> see a problem.
> 
> OK. Does this apply to all 64-bit SPARC CPUs? (There are some
> things I can simplify in the CPU migration code if we can break
> 64-bit migration.)

Yes, seems reasonable to me - I'm fairly sure that sun4u migration is
incomplete so I'd be amazed if anyone were successfully using this out
in the field.


ATB,

Mark.

[Qemu-devel] [PATCH 2/3] sun4u: split NPT and INT_DIS accesses between timer and compare registers

[Mark Cave-Ayland]

Accesses to the timer register high bit should only set NPT, whilst accesses
to the timer compare register high bit should only set INT_DIS. This fixes
issues with the timer being unexpectedly disabled whilst trying to boot
FreeBSD SPARC64.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/sparc64/sun4u.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 7153638..7a433d3 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -500,17 +500,17 @@ static uint64_t timer_to_cpu_ticks(int64_t timer_ticks, uint32_t frequency)
 
 void cpu_tick_set_count(CPUTimer *timer, uint64_t count)
 {
-    uint64_t real_count = count & ~timer->disabled_mask;
-    uint64_t disabled_bit = count & timer->disabled_mask;
+    uint64_t real_count = count & ~timer->npt_mask;
+    uint64_t npt_bit = count & timer->npt_mask;
 
     int64_t vm_clock_offset = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
                     cpu_to_timer_ticks(real_count, timer->frequency);
 
-    TIMER_DPRINTF("%s set_count count=0x%016lx (%s) p=%p\n",
+    TIMER_DPRINTF("%s set_count count=0x%016lx (npt %s) p=%p\n",
                   timer->name, real_count,
-                  timer->disabled?"disabled":"enabled", timer);
+                  timer->npt ? "disabled" : "enabled", timer);
 
-    timer->disabled = disabled_bit ? 1 : 0;
+    timer->npt = npt_bit ? 1 : 0;
     timer->clock_offset = vm_clock_offset;
 }
 
@@ -520,12 +520,13 @@ uint64_t cpu_tick_get_count(CPUTimer *timer)
                     qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - timer->clock_offset,
                     timer->frequency);
 
-    TIMER_DPRINTF("%s get_count count=0x%016lx (%s) p=%p\n",
+    TIMER_DPRINTF("%s get_count count=0x%016lx (npt %s) p=%p\n",
            timer->name, real_count,
-           timer->disabled?"disabled":"enabled", timer);
+           timer->npt ? "disabled" : "enabled", timer);
 
-    if (timer->disabled)
-        real_count |= timer->disabled_mask;
+    if (timer->npt) {
+        real_count |= timer->npt_mask;
+    }
 
     return real_count;
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] target-sparc: implement NPT timer bit

[Mark Cave-Ayland]

If the NPT bit is set in the timer register, all non-supervisor read accesses
to the register should fail with a privilege exception.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-sparc/helper.c    |   10 ++++++++--
 target-sparc/helper.h    |    2 +-
 target-sparc/translate.c |   18 +++++++++++++++---
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index 4850c7c..6600834 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -51,10 +51,16 @@ void helper_tick_set_count(void *opaque, uint64_t count)
 #endif
 }
 
-uint64_t helper_tick_get_count(void *opaque)
+uint64_t helper_tick_get_count(CPUSPARCState *env, void *opaque, int mem_idx)
 {
 #if !defined(CONFIG_USER_ONLY)
-    return cpu_tick_get_count(opaque);
+    CPUTimer *timer = opaque;
+
+    if (timer->npt && mem_idx < MMU_KERNEL_IDX) {
+        helper_raise_exception(env, TT_PRIV_INSN);
+    }
+
+    return cpu_tick_get_count(timer);
 #else
     return 0;
 #endif
diff --git a/target-sparc/helper.h b/target-sparc/helper.h
index 1ad23e8..4374f0d 100644
--- a/target-sparc/helper.h
+++ b/target-sparc/helper.h
@@ -25,7 +25,7 @@ DEF_HELPER_2(set_softint, void, env, i64)
 DEF_HELPER_2(clear_softint, void, env, i64)
 DEF_HELPER_2(write_softint, void, env, i64)
 DEF_HELPER_2(tick_set_count, void, ptr, i64)
-DEF_HELPER_1(tick_get_count, i64, ptr)
+DEF_HELPER_3(tick_get_count, i64, env, ptr, int)
 DEF_HELPER_2(tick_set_limit, void, ptr, i64)
 #endif
 #if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 41a3319..a3f1edb 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -2708,12 +2708,16 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                 case 0x4: /* V9 rdtick */
                     {
                         TCGv_ptr r_tickptr;
+                        TCGv_i32 r_const;
 
                         r_tickptr = tcg_temp_new_ptr();
+                        r_const = tcg_const_i32(dc->mem_idx);
                         tcg_gen_ld_ptr(r_tickptr, cpu_env,
                                        offsetof(CPUSPARCState, tick));
-                        gen_helper_tick_get_count(cpu_dst, r_tickptr);
+                        gen_helper_tick_get_count(cpu_dst, cpu_env, r_tickptr,
+                                                  r_const);
                         tcg_temp_free_ptr(r_tickptr);
+                        tcg_temp_free_i32(r_const);
                         gen_store_gpr(dc, rd, cpu_dst);
                     }
                     break;
@@ -2750,12 +2754,16 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                 case 0x18: /* System tick */
                     {
                         TCGv_ptr r_tickptr;
+                        TCGv_i32 r_const;
 
                         r_tickptr = tcg_temp_new_ptr();
+                        r_const = tcg_const_i32(dc->mem_idx);
                         tcg_gen_ld_ptr(r_tickptr, cpu_env,
                                        offsetof(CPUSPARCState, stick));
-                        gen_helper_tick_get_count(cpu_dst, r_tickptr);
+                        gen_helper_tick_get_count(cpu_dst, cpu_env, r_tickptr,
+                                                  r_const);
                         tcg_temp_free_ptr(r_tickptr);
+                        tcg_temp_free_i32(r_const);
                         gen_store_gpr(dc, rd, cpu_dst);
                     }
                     break;
@@ -2863,12 +2871,16 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
                 case 4: // tick
                     {
                         TCGv_ptr r_tickptr;
+                        TCGv_i32 r_const;
 
                         r_tickptr = tcg_temp_new_ptr();
+                        r_const = tcg_const_i32(dc->mem_idx);
                         tcg_gen_ld_ptr(r_tickptr, cpu_env,
                                        offsetof(CPUSPARCState, tick));
-                        gen_helper_tick_get_count(cpu_tmp0, r_tickptr);
+                        gen_helper_tick_get_count(cpu_tmp0, cpu_env,
+                                                  r_tickptr, r_const);
                         tcg_temp_free_ptr(r_tickptr);
+                        tcg_temp_free_i32(r_const);
                     }
                     break;
                 case 5: // tba
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/3] sun4u: timer fixups

[Artyom Tarasenko]

Looks good.

Reviewed-By: Artyom Tarasenko <atar4qemu@gmail.com>


On Fri, Nov 13, 2015 at 6:54 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Whilst trying to boot FreeBSD SPARC64, it became apparent that the existing
> timer code doesn't behave correctly with respect to the NPT and INT_DIS
> bits. This patchset corrects this behaviour and allows FreeBSD SPARC64 boot
> to progress further.
>
> Many thanks to Marius Strobl <marius@alchemy.franken.de> for pointing me in
> the right direction.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Mark Cave-Ayland (3):
>   sun4u: split out NPT and INT_DIS into separate CPUTimer fields
>   sun4u: split NPT and INT_DIS accesses between timer and compare
>     registers
>   target-sparc: implement NPT timer bit
>
>  hw/sparc64/sun4u.c       |   36 +++++++++++++++++++++++-------------
>  target-sparc/cpu.h       |    2 ++
>  target-sparc/helper.c    |   10 ++++++++--
>  target-sparc/helper.h    |    2 +-
>  target-sparc/translate.c |   18 +++++++++++++++---
>  5 files changed, 49 insertions(+), 19 deletions(-)
>
> --
> 1.7.10.4
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu