diff mbox

[4/4] target-ppc: ensure we include the decrementer value during migration

Message ID 1452104533-8516-5-git-send-email-mark.cave-ayland@ilande.co.uk
State New
Headers show

Commit Message

Mark Cave-Ayland Jan. 6, 2016, 6:22 p.m. UTC
During local testing with TCG, intermittent errors were found when trying to
migrate Darwin OS images.

The underlying cause was that Darwin resets the decrementer value to fairly
small values on each interrupt. cpu_ppc_set_tb_clk() sets the default value
of the decrementer to 0xffffffff during initialisation which typically
corresponds to several seconds. Hence when restoring the image, the guest
would effectively "lose" decrementer interrupts during this time causing
confusion in the guest.

NOTE: there does seem to be some overlap here with the vmstate_ppc_timebase
code, however it doesn't seem to handle multiple CPUs which is why I've gone
for an independent implementation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-ppc/machine.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Alexey Kardashevskiy Jan. 8, 2016, 2:47 a.m. UTC | #1
On 01/07/2016 05:22 AM, Mark Cave-Ayland wrote:
> During local testing with TCG, intermittent errors were found when trying to
> migrate Darwin OS images.
>
> The underlying cause was that Darwin resets the decrementer value to fairly
> small values on each interrupt. cpu_ppc_set_tb_clk() sets the default value
> of the decrementer to 0xffffffff during initialisation which typically
> corresponds to several seconds. Hence when restoring the image, the guest
> would effectively "lose" decrementer interrupts during this time causing
> confusion in the guest.
>
> NOTE: there does seem to be some overlap here with the vmstate_ppc_timebase
> code, however it doesn't seem to handle multiple CPUs which is why I've gone
> for an independent implementation.

It does handle multiple CPUs:

static int timebase_post_load(void *opaque, int version_id)
{
...
      /* Set new offset to all CPUs */
      CPU_FOREACH(cpu) {
          PowerPCCPU *pcpu = POWERPC_CPU(cpu);
          pcpu->env.tb_env->tb_offset = tb_off_adj;
      }


It does not transfer DECR though, it transfers the offset instead, one for 
all CPUs.


The easier solution would be just adding this instead of the whole patch:

spr_register(env, SPR_DECR, "DECR",
              SPR_NOACCESS, SPR_NOACCESS,
              &spr_read_decr, &spr_write_decr,
              0x00000000);

somewhere in target-ppc/translate_init.c for CPUs you want to support, 
gen_tbl() seems to be the right place for this. As long as it is just 
spr_register() and not spr_register_kvm(), I suppose it should not break 
KVM and pseries.





> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target-ppc/machine.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index cb56423..5ee6269 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -499,6 +499,54 @@ static const VMStateDescription vmstate_tlbmas = {
>       }
>   };
>
> +static bool decr_needed(void *opaque)
> +{
> +    return true;
> +}
> +
> +static int get_decr_state(QEMUFile *f, void *opaque, size_t size)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    cpu_ppc_store_decr(env, qemu_get_be32(f));
> +
> +    return 0;
> +}
> +
> +static void put_decr_state(QEMUFile *f, void *opaque, size_t size)
> +{
> +    PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
> +
> +    qemu_put_be32(f, cpu_ppc_load_decr(env));
> +}
> +
> +static const VMStateInfo vmstate_info_decr = {
> +    .name = "decr_state",
> +    .get = get_decr_state,
> +    .put = put_decr_state
> +};
> +
> +static const VMStateDescription vmstate_decr = {
> +    .name = "cpu/decr",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = decr_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "cpu/decr",
> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_decr,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   const VMStateDescription vmstate_ppc_cpu = {
>       .name = "cpu",
>       .version_id = 5,
> @@ -555,6 +603,7 @@ const VMStateDescription vmstate_ppc_cpu = {
>           &vmstate_tlb6xx,
>           &vmstate_tlbemb,
>           &vmstate_tlbmas,
> +        &vmstate_decr,
>           NULL
>       }
>   };
>
diff mbox

Patch

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index cb56423..5ee6269 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -499,6 +499,54 @@  static const VMStateDescription vmstate_tlbmas = {
     }
 };
 
+static bool decr_needed(void *opaque)
+{
+    return true;
+}
+
+static int get_decr_state(QEMUFile *f, void *opaque, size_t size)
+{
+    PowerPCCPU *cpu = opaque;
+    CPUPPCState *env = &cpu->env;
+
+    cpu_ppc_store_decr(env, qemu_get_be32(f));
+
+    return 0;
+}
+
+static void put_decr_state(QEMUFile *f, void *opaque, size_t size)
+{
+    PowerPCCPU *cpu = opaque;
+    CPUPPCState *env = &cpu->env;
+
+    qemu_put_be32(f, cpu_ppc_load_decr(env));
+}
+
+static const VMStateInfo vmstate_info_decr = {
+    .name = "decr_state",
+    .get = get_decr_state,
+    .put = put_decr_state
+};
+
+static const VMStateDescription vmstate_decr = {
+    .name = "cpu/decr",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .needed = decr_needed,
+    .fields = (VMStateField[]) {
+        {
+            .name         = "cpu/decr",
+            .version_id   = 0,
+            .field_exists = NULL,
+            .size         = 0,
+            .info         = &vmstate_info_decr,
+            .flags        = VMS_SINGLE,
+            .offset       = 0,
+        },
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
     .version_id = 5,
@@ -555,6 +603,7 @@  const VMStateDescription vmstate_ppc_cpu = {
         &vmstate_tlb6xx,
         &vmstate_tlbemb,
         &vmstate_tlbmas,
+        &vmstate_decr,
         NULL
     }
 };