On 31.03.2011, at 11:02, Alexander Graf wrote: > > On 31.03.2011, at 11:01, Liu Yu-B13201 wrote: > >> >> >>> -----Original Message----- >>> From: Alexander Graf [mailto:ag...@suse.de] >>> Sent: Thursday, March 31, 2011 4:43 PM >>> To: Liu Yu-B13201 >>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org >>> Subject: Re: [PATCH v5 4/4] KVM: PPC: e500: Save/restore SPE state >>> >>> >>> On 31.03.2011, at 10:40, Liu Yu-B13201 wrote: >>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: Alexander Graf [mailto:ag...@suse.de] >>>>> Sent: Thursday, March 31, 2011 4:15 PM >>>>> To: Liu Yu-B13201 >>>>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org >>>>> Subject: Re: [PATCH v5 4/4] KVM: PPC: e500: Save/restore SPE state >>>>> >>>>> >>>>> On 31.03.2011, at 10:06, Liu Yu-B13201 wrote: >>>>> >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: kvm-ppc-ow...@vger.kernel.org >>>>>>> [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of >>> Alexander Graf >>>>>>> Sent: Thursday, March 31, 2011 3:51 PM >>>>>>> To: Liu Yu-B13201 >>>>>>> Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org >>>>>>> Subject: Re: [PATCH v5 4/4] KVM: PPC: e500: Save/restore >>> SPE state >>>>>>> >>>>>>> >>>>>>> On 31.03.2011, at 05:21, Liu Yu-B13201 wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: kvm-ppc-ow...@vger.kernel.org >>>>>>>>> [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Scott Wood >>>>>>>>> Sent: Thursday, March 31, 2011 7:35 AM >>>>>>>>> To: ag...@suse.de >>>>>>>>> Cc: kvm-ppc@vger.kernel.org >>>>>>>>> Subject: [PATCH v5 4/4] KVM: PPC: e500: Save/restore SPE state >>>>>>>>> >>>>>>>>> This is done lazily. The SPE save will be done only if >>>>>>> the guest has >>>>>>>>> used SPE since the last preemption or heavyweight exit. >>>>>>>>> Restore will be >>>>>>>>> done only on demand, when enabling MSR_SPE in the shadow MSR, >>>>>>>>> in response >>>>>>>>> to an SPE fault or mtmsr emulation. >>>>>>>>> >>>>>>>>> For SPEFSCR, Linux already switches it on context switch >>>>>>>>> (non-lazily), so >>>>>>>>> the only remaining bit is to save it between qemu and >>> the guest. >>>>>>>>> >>>>>>>>> Signed-off-by: Liu Yu <yu....@freescale.com> >>>>>>>>> Signed-off-by: Scott Wood <scottw...@freescale.com> >>>>>>>>> --- >>>>>>>>> v5: disable preemption when restoring SPE state >>>>>>>>> >>>>>>>>> Saving SPE state is only done from a preempt notifier or >>>>>>> vcpu_put(), >>>>>>>>> where preemption is already disabled. >>>>>>>>> >>>>>>>>> The other patches in this series are the same as v4. >>>>>>>>> >>>>>>>>> arch/powerpc/include/asm/kvm_host.h | 6 +++ >>>>>>>>> arch/powerpc/include/asm/reg_booke.h | 1 + >>>>>>>>> arch/powerpc/kernel/asm-offsets.c | 6 +++ >>>>>>>>> arch/powerpc/kvm/booke.c | 72 >>>>>>>>> +++++++++++++++++++++++++++++++++- >>>>>>>>> arch/powerpc/kvm/booke.h | 18 ++------- >>>>>>>>> arch/powerpc/kvm/booke_interrupts.S | 40 +++++++++++++++++++ >>>>>>>>> arch/powerpc/kvm/e500.c | 19 ++++++++- >>>>>>>>> 7 files changed, 145 insertions(+), 17 deletions(-) >>>>>>>>> >>>>>>>> >>>>>>>> I think the patch miss the bit to handle the case that >>>>>>>> if guest clear the MSR_SPE. >>>>>>> >>>>>>> Right. On set_msr the shadow_msr should get MSR_SPE removed. >>>>>>> Thanks for the catch! >>>>>>> >>>>>> >>>>>> BTW, >>>>>> looks like guest trap on MSR[SPE] in para virt mode for >>> now, is it? >>>>>> If we don't want to trap on that, then shadow_msr doesn't >>> work here. >>>>>> That's the intend to use msr_block.. >>>>> >>>>> I see. I still like it better when we explicitly set the MSR >>>>> value that's used while running the guest. Makes it harder >>>>> for things to slip through. Also, so far paravirt is disabled >>>>> for BookE. >>>> >>>> We already use paravirt in internal tree. >>>> >>>>> >>>>> So what happens is that since MSR_SPE is disabled, we get a >>>>> BOOKE_INTERRUPT_SPE_UNAVAIL. At that point, we need to check >>>>> if it's enabled inside the guest already or if we merely need >>>>> to activate the flag. That's what the patch does. We'd have >>>>> to do the exact same when using msr_block, no? The only >>>>> difference is that we'd set msr_block instead of shadow_msr >>>>> when we are actually able to use the SPE inside the guest. >>>>> >>>> >>>> Hmm. I think you're right. >>>> I thought that in paravirt mode, if we don't trap on MSR[SPE], >>>> the shadow_msr cannot get notified timely (we don't have >>> chance to call kvmppc_set_msr()). >>>> But looks like msr_block has the same issue. >>>> So that MSR[SPE] should always be traped. >>> >>> It doesn't have to be trapped - we can enable it lazily on >>> SPE_UNAVAIL :). Of course we should also explicitly enable it >>> on set_msr. >>> >> >> It works on set MSR[SPE], but doesn't work on clear MSR[SPE]. >> Guest may also do lazy SPE, if guest clear MSR[SPE], >> but kvm don't know and still keep it. >> Then guest app may get SPE state clobbered. > > Ah, yes. On SPE clear we definitely have to trap :).
Scott, to verify that I don't completely screw up the lazy FPU work back when I did it, I wrote some small test program that would just initialize an fpu register with a value and then constantly loop to verify if it's still the same. I ran that program with different values on the host and guest sides. That exposed quite a lot of glitches. It would be nice if you could provide something similar for the SPE patches, so we have some more faith in it :). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
