| Message ID | 54945D7E.3090409@suse.de |
|---|---|
| State | New |
| Headers | show |
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
On 19.12.14 18:29, Paolo Bonzini wrote: > > > 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, g. 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). Even better, yes. 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); }