[Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes

[Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes

[Mark Cave-Ayland]

Whilst trying to fix migration of g3beige/mac99 images I came up with the
following patchset. The first patch is really cosmetic, while the second patch
alters the migration stream to include internal CPU IRQ state which appears
to fix an issue where images randomly fail to resume after migration.

As the second patch would need more work if deemed correct (the change in 
migration stream would require a bump in version number), it seemed worth
putting this out for review in case this is actually the symptom of another
bug.

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

Mark Cave-Ayland (2):
  target-ppc: remove hreg_compute_mem_idx() from cpu_post_load
  target-ppc: add CPU IRQ state to PPC VMStateDescription

 target-ppc/machine.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [RFC PATCH 1/2] target-ppc: remove hreg_compute_mem_idx() from cpu_post_load

[Mark Cave-Ayland]

hreg_compute_mem_idx() has already been called previously by ppc_store_msr()
via hreg_store_msr() and hreg_compute_hflags(). Drop the duplicate function
call as it is no longer needed.

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

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index f4ac761..bd99844 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -197,8 +197,6 @@ static int cpu_post_load(void *opaque, int version_id)
     env->msr ^= ~((1ULL << MSR_TGPR) | MSR_HVB);
     ppc_store_msr(env, msr);
 
-    hreg_compute_mem_idx(env);
-
     return 0;
 }
 
-- 
1.7.10.4

[Qemu-devel] [RFC PATCH 2/2] target-ppc: add CPU IRQ state to PPC VMStateDescription

[Mark Cave-Ayland]

Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription"
appears to drop the internal CPU IRQ state from the migration stream. Whilst
testing migration on g3beige/mac99 machines, test images would randomly fail to
resume unless a key was pressed on the VGA console.

Further investigation suggests that internal CPU IRQ state isn't being
preserved and so interrupts asserted at the time of migration are lost. Adding
the pending_interrupts and irq_input_state fields back into the migration
stream appears to fix the problem here during local tests.

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

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index bd99844..968a7d6 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = {
 
         /* Internal state */
         VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
+        VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU),
+        VMSTATE_UINT32(env.irq_input_state, PowerPCCPU),
         /* FIXME: access_type? */
 
         /* Sanity checking */
-- 
1.7.10.4

Re: [Qemu-devel] [RFC PATCH 2/2] target-ppc: add CPU IRQ state to PPC VMStateDescription

[Alexey Kardashevskiy]

On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote:
> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription"
> appears to drop the internal CPU IRQ state from the migration stream. Whilst
> testing migration on g3beige/mac99 machines, test images would randomly fail to
> resume unless a key was pressed on the VGA console.
>
> Further investigation suggests that internal CPU IRQ state isn't being
> preserved and so interrupts asserted at the time of migration are lost. Adding
> the pending_interrupts and irq_input_state fields back into the migration
> stream appears to fix the problem here during local tests.


On spapr, interrupt state migrates with XICS interrupt controller and it 
resets the CPU bits you are adding to the migration descriptor. I'd expect 
openpic (this one is used for mac99?) to do the same.

>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   target-ppc/machine.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index bd99844..968a7d6 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>
>           /* Internal state */
>           VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> +        VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU),
> +        VMSTATE_UINT32(env.irq_input_state, PowerPCCPU),

This update requires a "version" increment for vmstate_ppc_cpu and
VMSTATE_UINT32_V instead of VMSTATE_UINT32.


>           /* FIXME: access_type? */
>
>           /* Sanity checking */
>


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH 1/2] target-ppc: remove hreg_compute_mem_idx() from cpu_post_load

[Alexey Kardashevskiy]

On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote:
> hreg_compute_mem_idx() has already been called previously by ppc_store_msr()
> via hreg_store_msr() and hreg_compute_hflags(). Drop the duplicate function
> call as it is no longer needed.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>	

> ---
>   target-ppc/machine.c |    2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index f4ac761..bd99844 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -197,8 +197,6 @@ static int cpu_post_load(void *opaque, int version_id)
>       env->msr ^= ~((1ULL << MSR_TGPR) | MSR_HVB);
>       ppc_store_msr(env, msr);
>
> -    hreg_compute_mem_idx(env);
> -
>       return 0;
>   }
>
>


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH 2/2] target-ppc: add CPU IRQ state to PPC VMStateDescription

[Mark Cave-Ayland]

On 15/09/15 00:10, Alexey Kardashevskiy wrote:

> On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote:
>> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription"
>> appears to drop the internal CPU IRQ state from the migration stream.
>> Whilst
>> testing migration on g3beige/mac99 machines, test images would
>> randomly fail to
>> resume unless a key was pressed on the VGA console.
>>
>> Further investigation suggests that internal CPU IRQ state isn't being
>> preserved and so interrupts asserted at the time of migration are
>> lost. Adding
>> the pending_interrupts and irq_input_state fields back into the migration
>> stream appears to fix the problem here during local tests.
>  
> On spapr, interrupt state migrates with XICS interrupt controller and it
> resets the CPU bits you are adding to the migration descriptor. I'd
> expect openpic (this one is used for mac99?) to do the same.

Interesting. I wrote the patch that converted openpic to
VMStateDescription at the end of last year, and my understanding from
the feedback was that ideally interrupt state should be maintained so
that no post_load function was required. I guess spapr is very different
from the basic Mac machines though.

Also I see that you also removed the reference to cpu_write_xer() which
appears to set some related internal state variables. Is this now not
necessary either?

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   target-ppc/machine.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index bd99844..968a7d6 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>>
>>           /* Internal state */
>>           VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>> +        VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU),
>> +        VMSTATE_UINT32(env.irq_input_state, PowerPCCPU),
> 
> This update requires a "version" increment for vmstate_ppc_cpu and
> VMSTATE_UINT32_V instead of VMSTATE_UINT32.

So this means you're happy with the basic patch if I go ahead and make
the version changes too?


ATB,

Mark.

Re: [Qemu-devel] [RFC PATCH 2/2] target-ppc: add CPU IRQ state to PPC VMStateDescription

[Alexey Kardashevskiy]

On 09/16/2015 07:09 AM, Mark Cave-Ayland wrote:
> On 15/09/15 00:10, Alexey Kardashevskiy wrote:
>
>> On 09/15/2015 05:30 AM, Mark Cave-Ayland wrote:
>>> Commit a90db15 "target-ppc: Convert ppc cpu savevm to VMStateDescription"
>>> appears to drop the internal CPU IRQ state from the migration stream.
>>> Whilst
>>> testing migration on g3beige/mac99 machines, test images would
>>> randomly fail to
>>> resume unless a key was pressed on the VGA console.
>>>
>>> Further investigation suggests that internal CPU IRQ state isn't being
>>> preserved and so interrupts asserted at the time of migration are
>>> lost. Adding
>>> the pending_interrupts and irq_input_state fields back into the migration
>>> stream appears to fix the problem here during local tests.
>>
>> On spapr, interrupt state migrates with XICS interrupt controller and it
>> resets the CPU bits you are adding to the migration descriptor. I'd
>> expect openpic (this one is used for mac99?) to do the same.
>
> Interesting. I wrote the patch that converted openpic to
> VMStateDescription at the end of last year, and my understanding from
> the feedback was that ideally interrupt state should be maintained so
> that no post_load function was required. I guess spapr is very different
> from the basic Mac machines though.
>
> Also I see that you also removed the reference to cpu_write_xer() which
> appears to set some related internal state variables. Is this now not
> necessary either?


Not sure here, looks like a bug actually, cpu_post_load() should call it. 
But it should only affect TCG migration (which we have not extensively 
tested :) ).



>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>    target-ppc/machine.c |    2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>> index bd99844..968a7d6 100644
>>> --- a/target-ppc/machine.c
>>> +++ b/target-ppc/machine.c
>>> @@ -528,6 +528,8 @@ const VMStateDescription vmstate_ppc_cpu = {
>>>
>>>            /* Internal state */
>>>            VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>>> +        VMSTATE_UINT32(env.pending_interrupts, PowerPCCPU),
>>> +        VMSTATE_UINT32(env.irq_input_state, PowerPCCPU),
>>
>> This update requires a "version" increment for vmstate_ppc_cpu and
>> VMSTATE_UINT32_V instead of VMSTATE_UINT32.
>
> So this means you're happy with the basic patch if I go ahead and make
> the version changes too?

Yes, I suppose.


-- 
Alexey

Re: [Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes

[Alexander Graf]

On 14.09.15 21:30, Mark Cave-Ayland wrote:
> Whilst trying to fix migration of g3beige/mac99 images I came up with the
> following patchset. The first patch is really cosmetic, while the second patch
> alters the migration stream to include internal CPU IRQ state which appears
> to fix an issue where images randomly fail to resume after migration.
> 
> As the second patch would need more work if deemed correct (the change in 
> migration stream would require a bump in version number), it seemed worth
> putting this out for review in case this is actually the symptom of another
> bug.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

David, when a non-RFC version of this patch comes around, could you
please review and if good apply it to the tree via your branch?

Thanks a bunch!

Alex

Re: [Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes

[david]

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

On Sun, Sep 20, 2015 at 10:31:01PM +0200, Alexander Graf wrote:
> 
> 
> On 14.09.15 21:30, Mark Cave-Ayland wrote:
> > Whilst trying to fix migration of g3beige/mac99 images I came up with the
> > following patchset. The first patch is really cosmetic, while the second patch
> > alters the migration stream to include internal CPU IRQ state which appears
> > to fix an issue where images randomly fail to resume after migration.
> > 
> > As the second patch would need more work if deemed correct (the change in 
> > migration stream would require a bump in version number), it seemed worth
> > putting this out for review in case this is actually the symptom of another
> > bug.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> David, when a non-RFC version of this patch comes around, could you
> please review and if good apply it to the tree via your branch?
> 
> Thanks a bunch!

Mark,

I haven't seen a revised version of this.  Is that because it hasn't
been posted, or just because I've missed it somehow?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/2] target-ppc migration fixes

[Mark Cave-Ayland]

On 23/10/15 02:46, david@gibson.dropbear.id.au wrote:

> On Sun, Sep 20, 2015 at 10:31:01PM +0200, Alexander Graf wrote:
>>
>>
>> On 14.09.15 21:30, Mark Cave-Ayland wrote:
>>> Whilst trying to fix migration of g3beige/mac99 images I came up with the
>>> following patchset. The first patch is really cosmetic, while the second patch
>>> alters the migration stream to include internal CPU IRQ state which appears
>>> to fix an issue where images randomly fail to resume after migration.
>>>
>>> As the second patch would need more work if deemed correct (the change in 
>>> migration stream would require a bump in version number), it seemed worth
>>> putting this out for review in case this is actually the symptom of another
>>> bug.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> David, when a non-RFC version of this patch comes around, could you
>> please review and if good apply it to the tree via your branch?
>>
>> Thanks a bunch!
> 
> Mark,
> 
> I haven't seen a revised version of this.  Is that because it hasn't
> been posted, or just because I've missed it somehow?

Hi David,

I've spent a bit more time on this, and even with the patch I'm still
seeing migration issues which need further investigation so I'll have to
pick this up again after the upcoming release. On the plus side I've
been able to spend some time this week updating Cormac's OS9 GSoC
patches for upstream, so I'd be grateful if you could take a look at
those in the meantime.


ATB,

Mark.