[Qemu-devel] [RFC PATCH 0/2] ppc: fix spapr migration (TCG)

[Qemu-devel] [RFC PATCH 0/2] ppc: fix spapr migration (TCG)

[Cédric Le Goater]

Hello,

Here a couple of patches to fix the migration of spapr guests running
in TCG. This is not of the highest priority but it should work.

Waiting for your comments. Thanks !

C.



Cédric Le Goater (2):
  target-ppc: migrate interrupt vectors address for spapr VM
  target-ppc: fix interrupt vectors address migration

 target-ppc/helper_regs.h | 8 +++++---
 target-ppc/machine.c     | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.1.4

[Qemu-devel] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Cédric Le Goater]

This address is changed by the linux kernel using the H_SET_MODE hcall
and needs to be migrated in order to restart a spapr VM running in
TCG. Other platforms should not be affected.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 target-ppc/machine.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index 692121e98319..a418d463db83 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
         VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
         /* FIXME: access_type? */
 
+        /* Effective Address of interrupt vectors */
+        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
+
         /* Sanity checking */
         VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
-- 
2.1.4

Re: [Qemu-devel] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Thomas Huth]

On 21.03.2016 15:02, Cédric Le Goater wrote:
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be migrated in order to restart a spapr VM running in
> TCG. Other platforms should not be affected.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>  target-ppc/machine.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index 692121e98319..a418d463db83 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>          /* FIXME: access_type? */
>  
> +        /* Effective Address of interrupt vectors */
> +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
> +
>          /* Sanity checking */
>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),

I'm really no expert with all this migration stuff, but don't you have
to bump the version_id when you add new fields to the vmstate?
... and/or use VMSTATE_UINTTL_V() so that migration from older versions
of QEMU to the current one also still works with KVM? For example, is it
still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
use VMSTATE_UINTTL without the _V suffix?

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Greg Kurz]

On Mon, 21 Mar 2016 17:18:17 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 21.03.2016 15:02, Cédric Le Goater wrote:
> > This address is changed by the linux kernel using the H_SET_MODE hcall
> > and needs to be migrated in order to restart a spapr VM running in
> > TCG. Other platforms should not be affected.
> > 
> > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> > ---
> >  target-ppc/machine.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > index 692121e98319..a418d463db83 100644
> > --- a/target-ppc/machine.c
> > +++ b/target-ppc/machine.c
> > @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
> >          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> >          /* FIXME: access_type? */
> >  
> > +        /* Effective Address of interrupt vectors */
> > +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
> > +
> >          /* Sanity checking */
> >          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> >          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),  
> 
> I'm really no expert with all this migration stuff, but don't you have
> to bump the version_id when you add new fields to the vmstate?

Yes...

> ... and/or use VMSTATE_UINTTL_V() so that migration from older versions

... and also yes.

> of QEMU to the current one also still works with KVM? For example, is it
> still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
> use VMSTATE_UINTTL without the _V suffix?
> 

No we really want to use the _V version here: it indicates that starting with
this version (v6), cpu objects stream env.excp_prefix.

>  Thomas
> 
> 

--
Greg

Re: [Qemu-devel] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Cédric Le Goater]

On 03/21/2016 05:18 PM, Thomas Huth wrote:
> On 21.03.2016 15:02, Cédric Le Goater wrote:
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be migrated in order to restart a spapr VM running in
>> TCG. Other platforms should not be affected.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>  target-ppc/machine.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index 692121e98319..a418d463db83 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
>>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>>          /* FIXME: access_type? */
>>  
>> +        /* Effective Address of interrupt vectors */
>> +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
>> +
>>          /* Sanity checking */
>>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> 
> I'm really no expert with all this migration stuff, but don't you have
> to bump the version_id when you add new fields to the vmstate?
> ... and/or use VMSTATE_UINTTL_V() so that migration from older versions
> of QEMU to the current one also still works with KVM? For example, is it
> still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
> use VMSTATE_UINTTL without the _V suffix?

Yes. You are right. I think we need something like below.

Thanks,

C.


target-ppc: migrate interrupt vectors address for spapr VM

This address is changed by the linux kernel using the H_SET_MODE hcall
and needs to be migrated in order to restart a spapr VM running in
TCG. Other platforms should not be affected.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
 target-ppc/machine.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
+++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
@@ -522,7 +522,7 @@ static const VMStateDescription vmstate_
 
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
-    .version_id = 5,
+    .version_id = 6,
     .minimum_version_id = 5,
     .minimum_version_id_old = 4,
     .load_state_old = cpu_load_old,
@@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu
         VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
         /* FIXME: access_type? */
 
+        /* Effective Address of interrupt vectors */
+        VMSTATE_UINTTL_V(env.excp_prefix, PowerPCCPU, 6),
+
         /* Sanity checking */
         VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
         VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),


 
>  Thomas
> 

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Greg Kurz]

On Mon, 21 Mar 2016 17:51:22 +0100
Cédric Le Goater <clg@fr.ibm.com> wrote:

> On 03/21/2016 05:18 PM, Thomas Huth wrote:
> > On 21.03.2016 15:02, Cédric Le Goater wrote:  
> >> This address is changed by the linux kernel using the H_SET_MODE hcall
> >> and needs to be migrated in order to restart a spapr VM running in
> >> TCG. Other platforms should not be affected.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >> ---
> >>  target-ppc/machine.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> >> index 692121e98319..a418d463db83 100644
> >> --- a/target-ppc/machine.c
> >> +++ b/target-ppc/machine.c
> >> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
> >>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> >>          /* FIXME: access_type? */
> >>  
> >> +        /* Effective Address of interrupt vectors */
> >> +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
> >> +
> >>          /* Sanity checking */
> >>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> >>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),  
> > 
> > I'm really no expert with all this migration stuff, but don't you have
> > to bump the version_id when you add new fields to the vmstate?
> > ... and/or use VMSTATE_UINTTL_V() so that migration from older versions
> > of QEMU to the current one also still works with KVM? For example, is it
> > still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
> > use VMSTATE_UINTTL without the _V suffix?  
> 
> Yes. You are right. I think we need something like below.
> 

It looks much better !

And just to be sure: this only affects TCG because excp_prefix isn't accessed by
QEMU when running with KVM, right ?

If the answer is as simple as yes, then:

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

> Thanks,
> 
> C.
> 
> 
> target-ppc: migrate interrupt vectors address for spapr VM
> 
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be migrated in order to restart a spapr VM running in
> TCG. Other platforms should not be affected.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>  target-ppc/machine.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> @@ -522,7 +522,7 @@ static const VMStateDescription vmstate_
> 
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
> -    .version_id = 5,
> +    .version_id = 6,
>      .minimum_version_id = 5,
>      .minimum_version_id_old = 4,
>      .load_state_old = cpu_load_old,
> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu
>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>          /* FIXME: access_type? */
> 
> +        /* Effective Address of interrupt vectors */
> +        VMSTATE_UINTTL_V(env.excp_prefix, PowerPCCPU, 6),
> +
>          /* Sanity checking */
>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> 
> 
> 
> >  Thomas
> >   
> 
> 

Re: [Qemu-devel] [RFC PATCH 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[David Gibson]

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

On Mon, Mar 21, 2016 at 05:51:22PM +0100, Cédric Le Goater wrote:
> On 03/21/2016 05:18 PM, Thomas Huth wrote:
> > On 21.03.2016 15:02, Cédric Le Goater wrote:
> >> This address is changed by the linux kernel using the H_SET_MODE hcall
> >> and needs to be migrated in order to restart a spapr VM running in
> >> TCG. Other platforms should not be affected.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >> ---
> >>  target-ppc/machine.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> >> index 692121e98319..a418d463db83 100644
> >> --- a/target-ppc/machine.c
> >> +++ b/target-ppc/machine.c
> >> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
> >>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
> >>          /* FIXME: access_type? */
> >>  
> >> +        /* Effective Address of interrupt vectors */
> >> +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
> >> +
> >>          /* Sanity checking */
> >>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
> >>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> > 
> > I'm really no expert with all this migration stuff, but don't you have
> > to bump the version_id when you add new fields to the vmstate?
> > ... and/or use VMSTATE_UINTTL_V() so that migration from older versions
> > of QEMU to the current one also still works with KVM? For example, is it
> > still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
> > use VMSTATE_UINTTL without the _V suffix?
> 
> Yes. You are right. I think we need something like below.
> 
> Thanks,
> 
> C.
> 
> 
> target-ppc: migrate interrupt vectors address for spapr VM
> 
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be migrated in order to restart a spapr VM running in
> TCG. Other platforms should not be affected.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>  target-ppc/machine.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> @@ -522,7 +522,7 @@ static const VMStateDescription vmstate_
>  
>  const VMStateDescription vmstate_ppc_cpu = {
>      .name = "cpu",
> -    .version_id = 5,
> +    .version_id = 6,
>      .minimum_version_id = 5,
>      .minimum_version_id_old = 4,
>      .load_state_old = cpu_load_old,
> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu
>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>          /* FIXME: access_type? */
>  
> +        /* Effective Address of interrupt vectors */
> +        VMSTATE_UINTTL_V(env.excp_prefix, PowerPCCPU, 6),


So, I dislike putting what's essentially emulator internal state (as
opposed to architected state) into the migration stream if we can
possibly avoid it.

I think recalculating excp_prefix from the MSR on incoming migration
is the correct approach here - I see that there are bugs with that in
the other patch, but so far I'm not seeing a reason to migrate
excp_prefix itself.

>          /* Sanity checking */
>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
> 
> 
>  
> >  Thomas
> > 
> 

-- 
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 1/2] target-ppc: migrate interrupt vectors address for spapr VM

[Cédric Le Goater]

On 03/22/2016 01:15 AM, David Gibson wrote:
> On Mon, Mar 21, 2016 at 05:51:22PM +0100, Cédric Le Goater wrote:
>> On 03/21/2016 05:18 PM, Thomas Huth wrote:
>>> On 21.03.2016 15:02, Cédric Le Goater wrote:
>>>> This address is changed by the linux kernel using the H_SET_MODE hcall
>>>> and needs to be migrated in order to restart a spapr VM running in
>>>> TCG. Other platforms should not be affected.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>> ---
>>>>  target-ppc/machine.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>>> index 692121e98319..a418d463db83 100644
>>>> --- a/target-ppc/machine.c
>>>> +++ b/target-ppc/machine.c
>>>> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu = {
>>>>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>>>>          /* FIXME: access_type? */
>>>>  
>>>> +        /* Effective Address of interrupt vectors */
>>>> +        VMSTATE_UINTTL(env.excp_prefix, PowerPCCPU),
>>>> +
>>>>          /* Sanity checking */
>>>>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>>>>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>>>
>>> I'm really no expert with all this migration stuff, but don't you have
>>> to bump the version_id when you add new fields to the vmstate?
>>> ... and/or use VMSTATE_UINTTL_V() so that migration from older versions
>>> of QEMU to the current one also still works with KVM? For example, is it
>>> still possible to migrate from QEMU 2.5 to QEMU 2.6 in KVM if you only
>>> use VMSTATE_UINTTL without the _V suffix?
>>
>> Yes. You are right. I think we need something like below.
>>
>> Thanks,
>>
>> C.
>>
>>
>> target-ppc: migrate interrupt vectors address for spapr VM
>>
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be migrated in order to restart a spapr VM running in
>> TCG. Other platforms should not be affected.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>  target-ppc/machine.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> @@ -522,7 +522,7 @@ static const VMStateDescription vmstate_
>>  
>>  const VMStateDescription vmstate_ppc_cpu = {
>>      .name = "cpu",
>> -    .version_id = 5,
>> +    .version_id = 6,
>>      .minimum_version_id = 5,
>>      .minimum_version_id_old = 4,
>>      .load_state_old = cpu_load_old,
>> @@ -553,6 +553,9 @@ const VMStateDescription vmstate_ppc_cpu
>>          VMSTATE_UINTTL(env.hflags_nmsr, PowerPCCPU),
>>          /* FIXME: access_type? */
>>  
>> +        /* Effective Address of interrupt vectors */
>> +        VMSTATE_UINTTL_V(env.excp_prefix, PowerPCCPU, 6),
> 
> 
> So, I dislike putting what's essentially emulator internal state (as
> opposed to architected state) into the migration stream if we can
> possibly avoid it.
> 
> I think recalculating excp_prefix from the MSR on incoming migration
> is the correct approach here - I see that there are bugs with that in
> the other patch, but so far I'm not seeing a reason to migrate
> excp_prefix itself.

OK. It seems quite feasible as we can compute the value from the LPCR_AIL 
bits in SPR_LPCR. I will give it a try.

Thanks,

C.



>>          /* Sanity checking */
>>          VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU),
>>          VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU),
>>
>>
>>  
>>>  Thomas
>>>
>>
> 

[Qemu-devel] [RFC PATCH 2/2] target-ppc: fix interrupt vectors address migration

[Cédric Le Goater]

commit 2360b6e84f78 ("target-ppc: force update of msr bits in
cpu_post_load") introduced a change to restore env->excp_prefix of a
guest which could have altered its MSR_EP. To do this, cpu_post_load()
invalidates msr and then calls ppc_store_msr() with the expected value
in argument.

The problem is that ppc_store_msr() relies on a 'valid' current msr
before changing its value. The MSR_HVB and MSR_TGPR bits are excluded
from the msr reset to keep the checks valid but the MSR_IR, MSR_DR,
MSR_EP bits which are also used through the msr_{ir,dr,ep} macros, are
reseted.

This is an issue for CPUs not using MSR_EP, on the spapr platform for
instance but all book3s are impacted. If excp_prefix is restored to
some value, it will be reseted by this call, causing an ISEG exception
on spapr guests.

This patch proposal is to test the msr_mask before actually testing
the MSR_EP bit and protect excp_prefix.

Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---

 Should we just move the test in cpu_post_load() and not reset MSR_EP
 if it is not present in msr_mask ? like this is done for MSR_HVB and
 MSR_TGPR. I think this is making assumptions on what ppc_store_msr()
 is up to though.

 Maybe we could add a POWERPC_FLAGS_ for this purpose ? or test the
 excp_model ?
 
 There is room for improvement in ppc_store_msr(). It might need a new
 helper like ppc_restore_msr() ?

 Suggestions welcomed.
 
 target-ppc/helper_regs.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 271fddf17f0a..2a72e000ed83 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -92,9 +92,11 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
         /* Swap temporary saved registers with GPRs */
         hreg_swap_gpr_tgpr(env);
     }
-    if (unlikely((value >> MSR_EP) & 1) != msr_ep) {
-        /* Change the exception prefix on PowerPC 601 */
-        env->excp_prefix = ((value >> MSR_EP) & 1) * 0xFFF00000;
+    if ((env->msr_mask >> MSR_EP) & 1) {
+        if (unlikely((value >> MSR_EP) & 1) != msr_ep) {
+            /* Change the exception prefix on PowerPC 601 */
+            env->excp_prefix = ((value >> MSR_EP) & 1) * 0xFFF00000;
+        }
     }
 #endif
     env->msr = value;
-- 
2.1.4

Re: [Qemu-devel] [RFC PATCH 2/2] target-ppc: fix interrupt vectors address migration

[David Gibson]

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

On Mon, Mar 21, 2016 at 03:02:08PM +0100, Cédric Le Goater wrote:
> commit 2360b6e84f78 ("target-ppc: force update of msr bits in
> cpu_post_load") introduced a change to restore env->excp_prefix of a
> guest which could have altered its MSR_EP. To do this, cpu_post_load()
> invalidates msr and then calls ppc_store_msr() with the expected value
> in argument.
> 
> The problem is that ppc_store_msr() relies on a 'valid' current msr
> before changing its value. The MSR_HVB and MSR_TGPR bits are excluded
> from the msr reset to keep the checks valid but the MSR_IR, MSR_DR,
> MSR_EP bits which are also used through the msr_{ir,dr,ep} macros, are
> reseted.
> 
> This is an issue for CPUs not using MSR_EP, on the spapr platform for
> instance but all book3s are impacted. If excp_prefix is restored to
> some value, it will be reseted by this call, causing an ISEG exception
> on spapr guests.
> 
> This patch proposal is to test the msr_mask before actually testing
> the MSR_EP bit and protect excp_prefix.
> 
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
> 
>  Should we just move the test in cpu_post_load() and not reset MSR_EP
>  if it is not present in msr_mask ? like this is done for MSR_HVB and
>  MSR_TGPR. I think this is making assumptions on what ppc_store_msr()
>  is up to though.
> 
>  Maybe we could add a POWERPC_FLAGS_ for this purpose ? or test the
>  excp_model ?
>  
>  There is room for improvement in ppc_store_msr(). It might need a new
>  helper like ppc_restore_msr() ?
> 
>  Suggestions welcomed.

So, IIUC, in spapr MSR[EP] can't be set with mtmsr or rfid, but can be
set with H_SET_MODE?

I think what we need to do is to make sure the full MSR value is
migrated then, even if EP is not in the msr_mask.  Once that's done,
we should be able to correctly calculate excp_prefix from the MSR
value after migration.

Or am I missing something?

>  target-ppc/helper_regs.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 271fddf17f0a..2a72e000ed83 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -92,9 +92,11 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
>          /* Swap temporary saved registers with GPRs */
>          hreg_swap_gpr_tgpr(env);
>      }
> -    if (unlikely((value >> MSR_EP) & 1) != msr_ep) {
> -        /* Change the exception prefix on PowerPC 601 */
> -        env->excp_prefix = ((value >> MSR_EP) & 1) * 0xFFF00000;
> +    if ((env->msr_mask >> MSR_EP) & 1) {
> +        if (unlikely((value >> MSR_EP) & 1) != msr_ep) {
> +            /* Change the exception prefix on PowerPC 601 */
> +            env->excp_prefix = ((value >> MSR_EP) & 1) * 0xFFF00000;
> +        }
>      }
>  #endif
>      env->msr = value;

-- 
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 --]