On Mon, Jan 11, 2016 at 07:43:54AM +0000, Mark Cave-Ayland wrote: > On 11/01/16 04:55, David Gibson wrote: > > > On Mon, Jan 11, 2016 at 12:18:31PM +1100, Alexey Kardashevskiy wrote: > >> On 01/09/2016 01:21 AM, Mark Cave-Ayland wrote: > >>> On 08/01/16 02:47, Alexey Kardashevskiy wrote: > >>> > >>>> On 01/07/2016 05:22 AM, Mark Cave-Ayland wrote: > >>>>> During local testing with TCG, intermittent errors were found when > >>>>> trying to > >>>>> migrate Darwin OS images. > >>>>> > >>>>> The underlying cause was that Darwin resets the decrementer value to > >>>>> fairly > >>>>> small values on each interrupt. cpu_ppc_set_tb_clk() sets the default > >>>>> value > >>>>> of the decrementer to 0xffffffff during initialisation which typically > >>>>> corresponds to several seconds. Hence when restoring the image, the > >>>>> guest > >>>>> would effectively "lose" decrementer interrupts during this time causing > >>>>> confusion in the guest. > >>>>> > >>>>> NOTE: there does seem to be some overlap here with the > >>>>> vmstate_ppc_timebase > >>>>> code, however it doesn't seem to handle multiple CPUs which is why > >>>>> I've gone > >>>>> for an independent implementation. > >>>> > >>>> It does handle multiple CPUs: > >>>> > >>>> static int timebase_post_load(void *opaque, int version_id) > >>>> { > >>>> ... > >>>> /* Set new offset to all CPUs */ > >>>> CPU_FOREACH(cpu) { > >>>> PowerPCCPU *pcpu = POWERPC_CPU(cpu); > >>>> pcpu->env.tb_env->tb_offset = tb_off_adj; > >>>> } > >>>> > >>>> > >>>> It does not transfer DECR though, it transfers the offset instead, one > >>>> for all CPUs. > >>>> > >>>> > >>>> The easier solution would be just adding this instead of the whole patch: > >>>> > >>>> spr_register(env, SPR_DECR, "DECR", > >>>> SPR_NOACCESS, SPR_NOACCESS, > >>>> &spr_read_decr, &spr_write_decr, > >>>> 0x00000000); > >>>> > >>>> somewhere in target-ppc/translate_init.c for CPUs you want to support, > >>>> gen_tbl() seems to be the right place for this. As long as it is just > >>>> spr_register() and not spr_register_kvm(), I suppose it should not break > >>>> KVM and pseries. > >>> > >>> I've just tried adding that but it then gives the following error on > >>> startup: > >>> > >>> Error: Trying to register SPR 22 (016) twice ! > >>> > >>> Based upon this, the existing registration seems fine. I think the > >>> problem is that I can't see anything in __cpu_ppc_store_decr() that > >>> updates the spr[SPR_DECR] value when the decrementer register is > >>> changed, so it needs to be explicitly added to > >>> cpu_pre_save()/cpu_post_load(): > >>> > >>> > >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c > >>> index 251a84b..495e58d 100644 > >>> --- a/target-ppc/machine.c > >>> +++ b/target-ppc/machine.c > >>> @@ -141,6 +141,7 @@ static void cpu_pre_save(void *opaque) > >>> env->spr[SPR_CFAR] = env->cfar; > >>> #endif > >>> env->spr[SPR_BOOKE_SPEFSCR] = env->spe_fscr; > >>> + env->spr[SPR_DECR] = cpu_ppc_load_decr(env); > >>> > >>> for (i = 0; (i < 4) && (i < env->nb_BATs); i++) { > >>> env->spr[SPR_DBAT0U + 2*i] = env->DBAT[0][i]; > >>> @@ -175,6 +176,7 @@ static int cpu_post_load(void *opaque, int version_id) > >>> env->cfar = env->spr[SPR_CFAR]; > >>> #endif > >>> env->spe_fscr = env->spr[SPR_BOOKE_SPEFSCR]; > >>> + cpu_ppc_store_decr(env, env->spr[SPR_DECR]); > >>> > >>> for (i = 0; (i < 4) && (i < env->nb_BATs); i++) { > >>> env->DBAT[0][i] = env->spr[SPR_DBAT0U + 2*i]; > >>> > >>> > >>> I've just tried the diff above instead of my original version and > >>> repeated my savevm/loadvm pair test with a Darwin installation and it > >>> also fixes the random hang I was seeing on loadvm. > >>> > >>> Seemingly this should work on KVM in that cpu_ppc_load_decr() and > >>> cpu_ppc_store_decr() become no-ops as long as KVM maintains > >>> env->spr[SPR_DECR], but a second set of eyeballs would be useful here. > >>> > >>> If you can let me know if this is suitable then I'll update the patchset > >>> based upon your feedback and send out a v2. > >> > >> > >> Looks good to me, I'd just wait a day or two in the case if David wants to > >> comment. > > > > I was on holiday and missed the start of this thread, I'm afraid, so I > > don't fully understand the context here. > > It's part of a patchset I posted which fixes up problems I had with > migrating g3beige/mac99 machines under TCG: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00544.html. > > Apologies for not adding you as CC directly - are you still helping to > cover ppc-next for Alex?
Yes, I am. > > Am I right in thinking that this change will essentially freeze the > > decrementer across the migration downtime? That doesn't seem right, > > since the decrementer is supposed to be linked to the timebase and > > represent real time passing. > > Yes, that's correct. > > > In other words, isn't this just skipping the decrementer interrupts at > > the qemu level rather than the guest level? > > > > It seems that instead we should be reconstructing the decrementer on > > the destination based on an offset from the timebase. > > Well I haven't really looked at how time warping works during in > migration for QEMU, however this seems to be the method used by > hw/ppc/ppc.c's timebase_post_load() function but my understanding is > that this isn't currently available for the g3beige/mac99 machines? Ah.. yes, it looks like the timebase migration stuff is only hooked in on the pseries machine type. As far as I can tell it should be trivial to add it to other machines though - it doesn't appear to rely on anything outside the common ppc timebase stuff. > Should the patch in fact do this but also add decrementer support? And > if it did, would this have a negative effect on pseries? Yes, I think that's the right approach. Note that rather than duplicating the logic to adjust the decrementer over migration, it should be possible to encode the decrementer as a diff from the timebase across the migration. In fact.. I'm not sure it ever makes sense to store the decrementer value as a direct value, since it's constantly changing - probably makes more sense to derive it from the timebase whenever it is needed. As far as I know that should be fine for pseries. I think the current behaviour is probably technically wrong for pseries as well, but the timing code of our Linux guests is robust enough to handle a small displacement to the time of the next decrementer interrupt. > But yes, assuming that the guest time warp is handled correctly (which I > assume is handled correctly elsewhere since this would also be required > for KVM) then I think that this should work. > > > ATB, > > Mark. > -- 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
signature.asc
Description: PGP signature