Re: [Qemu-devel] [Qemu-ppc] Unable to loadvm on qemu-system-ppc -M g3beige (keyboard freeze)

[Paolo Bonzini <pbonzini@redhat.com>]

On 19/12/2014 18:16, Alexander Graf wrote:
> 
> 
> On 19.12.14 17:51, Mark Cave-Ayland wrote:
>> On 19/12/14 16:35, Peter Maydell wrote:
>>
>>> On 19 December 2014 at 16:15, Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>> It looks like after restoring the CPU state with loadvm env->htab_base
>>>> is still at 0x0 rather than its pre-migration value of 0x7e000000.
>>>
>>> Odd -- there is code that is at least trying to do that:
>>> machine.c calls ppc_store_sdr1() which should set htab_base/mask
>>> according to the migrated value of the register...
>>
>> Stepping through ppc_store_sdr1() I see the problem is the if() statement:
>>
>> if (env->spr[SPR_SDR1] != value) {
>>    ....
>> }
>>
>> It looks like env->spr[SPR_SDR1] has already been set to 0x7e000000
>> before the call to ppc_store_sdr1() and so as the values are already
>> equal then the code to setup env->htab_mask and env->htab_base is bypassed.
>>
>> The quick fix is to comment out the above if() statement which allows me
>> to restore my OpenBIOS image successfully with -loadvm but sadly not my
>> OS image (I guess there are probably a few more state bugs still lying
>> around). Does this seem the right thing to do or is there a better way?
> 
> How about something like this instead? Then we at least don't lose the
> acceleration we get from not invalidating the TLB on identical SDR1 writes.
> 
> 
> Alex
> 
> 
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index c801b82..6f347bf 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -71,6 +71,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int
> version_id)
>      for (i = 0; i < 1024; i++)
>          qemu_get_betls(f, &env->spr[i]);
>      if (!env->external_htab) {
> +        env->spr[SPR_SDR1] = 0;
>          ppc_store_sdr1(env, sdr1);
>      }
>      qemu_get_be32s(f, &env->vscr);

Or even move the tlb_flush and if to the caller, helper_store_sdr1.  It
is only needed there, and would get in the way if one day PPC were to
support --disable-tcg (which removes cputlb.c from the executable).

Paolo