[Qemu-devel] [RFC PATCH 0/1] ppc: loadvm corrupts excp_prefix

[Qemu-devel] [RFC PATCH 0/1] ppc: loadvm corrupts excp_prefix

[Kurban Mallachiev]

On processors which don't support MSR_EP bit, loadvm command set exception prefix to an incorrect value and so guest OS freezes.

In cpu_post_load() there is:
	/* Invalidate all msr bits except MSR_TGPR/MSR_HVB before restoring */
	msr = env->msr;
	env->msr ^= ~((1ULL << MSR_TGPR) | MSR_HVB);
	ppc_store_msr(env, msr);

While hreg_store_msr() (called by ppc_store_msr) contains:
	value &= env->msr_mask;
	...
	if (unlikely((value >> MSR_EP) & 1) != msr_ep) {
	/* Change the exception prefix on PowerPC 601 */
	...

where msr_ep is ((env->msr >> MSR_EP) & 1).

If MSR_EP bit in msr_mask is zero, then MSR_EP in 'value' bit is zero, and MSR_EP bit in env->msr is 1. Condition '(value >> MSR_EP) & 1) != msr_ep' is true and so qemu changes exception prefix.

AFAIU we should multiply env->msr by msr_mask, but I am not sure where we should do it: inside hreg_store_msr or outside. This is why this patch is RFC.

Current version of the patch adds msr_mask multiplication before the hreg_store_msr call.

Kurban

Kurban Mallachiev (1):
  target-ppc: Don't invalidate non-supported msr bits

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

-- 
2.15.0

[Qemu-devel] [RFC PATCH 1/1] target-ppc: Don't invalidate non-supported msr bits

[Kurban Mallachiev]

The msr invalidation code (commits 993eb and 2360b) inverts all
bits except MSR_TGPR and MSR_HVB. On non PowerPC 601 processors
this leads to incorrect change of excp_prefix in hreg_store_msr()
function. The problem is that new msr value get multiplied by msr_mask
and inverted msr does not, thus values of MSR_EP bit in new msr value
and inverted msr are distinct, so that excp_prefix changes but should
not.

Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>
---
 target/ppc/machine.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 384caee800..96113ee881 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -298,9 +298,9 @@ static int cpu_post_load(void *opaque, int version_id)
         ppc_store_sdr1(env, env->spr[SPR_SDR1]);
     }
 
-    /* Invalidate all msr bits except MSR_TGPR/MSR_HVB before restoring */
+    /* Invalidate all supported msr bits except MSR_TGPR/MSR_HVB before restoring */
     msr = env->msr;
-    env->msr ^= ~((1ULL << MSR_TGPR) | MSR_HVB);
+    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
     ppc_store_msr(env, msr);
 
     hreg_compute_mem_idx(env);
-- 
2.15.0

Re: [Qemu-devel] [RFC PATCH 1/1] target-ppc: Don't invalidate non-supported msr bits

[David Gibson]

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

On Wed, Nov 29, 2017 at 07:22:19PM +0300, Kurban Mallachiev wrote:
> The msr invalidation code (commits 993eb and 2360b) inverts all
> bits except MSR_TGPR and MSR_HVB. On non PowerPC 601 processors
> this leads to incorrect change of excp_prefix in hreg_store_msr()
> function. The problem is that new msr value get multiplied by msr_mask
> and inverted msr does not, thus values of MSR_EP bit in new msr value
> and inverted msr are distinct, so that excp_prefix changes but should
> not.
> 
> Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>

So, the whole logic of ppc_store_msr() / hreg_store_msr() looks much
harier than it should be to me.  Nonetheless, this definitely looks
like an improvement over the current code.

Applied to ppc-for-2.11.

Laurent, could this be related to the loadvm state problems you were
seeing in several BZs?

> ---
>  target/ppc/machine.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 384caee800..96113ee881 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -298,9 +298,9 @@ static int cpu_post_load(void *opaque, int version_id)
>          ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>      }
>  
> -    /* Invalidate all msr bits except MSR_TGPR/MSR_HVB before restoring */
> +    /* Invalidate all supported msr bits except MSR_TGPR/MSR_HVB before restoring */
>      msr = env->msr;
> -    env->msr ^= ~((1ULL << MSR_TGPR) | MSR_HVB);
> +    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
>      ppc_store_msr(env, msr);
>  
>      hreg_compute_mem_idx(env);

-- 
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: 833 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/1] target-ppc: Don't invalidate non-supported msr bits

[Laurent Vivier]

On 30/11/2017 04:58, David Gibson wrote:
> On Wed, Nov 29, 2017 at 07:22:19PM +0300, Kurban Mallachiev wrote:
>> The msr invalidation code (commits 993eb and 2360b) inverts all
>> bits except MSR_TGPR and MSR_HVB. On non PowerPC 601 processors
>> this leads to incorrect change of excp_prefix in hreg_store_msr()
>> function. The problem is that new msr value get multiplied by msr_mask
>> and inverted msr does not, thus values of MSR_EP bit in new msr value
>> and inverted msr are distinct, so that excp_prefix changes but should
>> not.
>>
>> Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>
> 
> So, the whole logic of ppc_store_msr() / hreg_store_msr() looks much
> harier than it should be to me.  Nonetheless, this definitely looks
> like an improvement over the current code.
> 
> Applied to ppc-for-2.11.
> 
> Laurent, could this be related to the loadvm state problems you were
> seeing in several BZs?

Thank you David, I've tried and this doesn't solve my problems.

Laurent