| Message ID | 1447437247-19512-2-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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; };
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(-)