| Message ID | 1372855359-13452-4-git-send-email-mihai.caraman@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 03.07.2013, at 14:42, Mihai Caraman wrote: > Increase FPU laziness by calling kvmppc_load_guest_fp() just before > returning to guest instead of each sched in. Without this improvement > an interrupt may also claim floting point corrupting guest state. Not sure I follow. Could you please describe exactly what's happening? Alex > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > --- > arch/powerpc/kvm/booke.c | 1 + > arch/powerpc/kvm/e500mc.c | 2 -- > 2 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index 113961f..3cae2e3 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, > r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); > } else { > kvmppc_lazy_ee_enable(); > + kvmppc_load_guest_fp(vcpu); > } > } > > diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c > index 19c8379..09da1ac 100644 > --- a/arch/powerpc/kvm/e500mc.c > +++ b/arch/powerpc/kvm/e500mc.c > @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > kvmppc_e500_tlbil_all(vcpu_e500); > __get_cpu_var(last_vcpu_on_cpu) = vcpu; > } > - > - kvmppc_load_guest_fp(vcpu); > } > > void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu) > -- > 1.7.3.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Alexander Graf [mailto:agraf@suse.de] > Sent: Wednesday, July 03, 2013 4:45 PM > To: Caraman Mihai Claudiu-B02008 > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org > Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness > > > On 03.07.2013, at 14:42, Mihai Caraman wrote: > > > Increase FPU laziness by calling kvmppc_load_guest_fp() just before > > returning to guest instead of each sched in. Without this improvement > > an interrupt may also claim floting point corrupting guest state. > > Not sure I follow. Could you please describe exactly what's happening? This was already discussed on the list, I will forward you the thread. -Mike
On 03.07.2013, at 15:55, Caraman Mihai Claudiu-B02008 wrote: >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@suse.de] >> Sent: Wednesday, July 03, 2013 4:45 PM >> To: Caraman Mihai Claudiu-B02008 >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc- >> dev@lists.ozlabs.org >> Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness >> >> >> On 03.07.2013, at 14:42, Mihai Caraman wrote: >> >>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before >>> returning to guest instead of each sched in. Without this improvement >>> an interrupt may also claim floting point corrupting guest state. >> >> Not sure I follow. Could you please describe exactly what's happening? > > This was already discussed on the list, I will forward you the thread. The only thing I've seen in that thread was some pathetic theoretical case where an interrupt handler would enable fp and clobber state carelessly. That's not something I'm worried about. I really don't see where this patch improves anything tbh. It certainly makes the code flow more awkward. Alex
> >>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before > >>> returning to guest instead of each sched in. Without this improvement > >>> an interrupt may also claim floting point corrupting guest state. > >> > >> Not sure I follow. Could you please describe exactly what's happening? > > > > This was already discussed on the list, I will forward you the thread. > > The only thing I've seen in that thread was some pathetic theoretical > case where an interrupt handler would enable fp and clobber state > carelessly. That's not something I'm worried about. Neither me though I don't find it pathetic. Please refer it to Scott. > > I really don't see where this patch improves anything tbh. It certainly > makes the code flow more awkward. I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel is struggling to achieve is to reduce the number of store/restore operations. Without this improvement we restore the unit each time we are sched it. If an other process take the ownership of the unit (on SMP it's even worse but don't bother with this) the kernel store the unit state to qemu task. This can happen multiple times during handle_exit(). Do you see it now? -Mike
On 03.07.2013, at 17:41, Caraman Mihai Claudiu-B02008 wrote: >>>>> Increase FPU laziness by calling kvmppc_load_guest_fp() just before >>>>> returning to guest instead of each sched in. Without this improvement >>>>> an interrupt may also claim floting point corrupting guest state. >>>> >>>> Not sure I follow. Could you please describe exactly what's happening? >>> >>> This was already discussed on the list, I will forward you the thread. >> >> The only thing I've seen in that thread was some pathetic theoretical >> case where an interrupt handler would enable fp and clobber state >> carelessly. That's not something I'm worried about. > > Neither me though I don't find it pathetic. Please refer it to Scott. If from Linux's point of view we look like a user space program with active floating point registers, we don't have to worry about this case. Kernel code that would clobber that fp state would clobber random user space's fp state too. > >> >> I really don't see where this patch improves anything tbh. It certainly >> makes the code flow more awkward. > > I was pointing you to this: The idea of FPU/AltiVec laziness that the kernel > is struggling to achieve is to reduce the number of store/restore operations. > Without this improvement we restore the unit each time we are sched it. If an > other process take the ownership of the unit (on SMP it's even worse but don't > bother with this) the kernel store the unit state to qemu task. This can happen > multiple times during handle_exit(). > > Do you see it now? Yup. Looks good. The code flow is very hard to follow though - there are a lot of implicit assumptions that don't get documented anywhere. For example the fact that we rely on giveup_fpu() to remove MSR_FP from our thread. Alex
On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: > Increase FPU laziness by calling kvmppc_load_guest_fp() just before > returning to guest instead of each sched in. Without this improvement > an interrupt may also claim floting point corrupting guest state. > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > --- > arch/powerpc/kvm/booke.c | 1 + > arch/powerpc/kvm/e500mc.c | 2 -- > 2 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index 113961f..3cae2e3 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > r = (s << 2) | RESUME_HOST | (r & > RESUME_FLAG_NV); > } else { > kvmppc_lazy_ee_enable(); > + kvmppc_load_guest_fp(vcpu); > } This should go before the kvmppc_lazy_ee_enable(). -Scott
On 03.07.2013, at 19:18, Scott Wood wrote: > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before >> returning to guest instead of each sched in. Without this improvement >> an interrupt may also claim floting point corrupting guest state. >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> >> --- >> arch/powerpc/kvm/booke.c | 1 + >> arch/powerpc/kvm/e500mc.c | 2 -- >> 2 files changed, 1 insertions(+), 2 deletions(-) >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >> index 113961f..3cae2e3 100644 >> --- a/arch/powerpc/kvm/booke.c >> +++ b/arch/powerpc/kvm/booke.c >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, >> r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); >> } else { >> kvmppc_lazy_ee_enable(); >> + kvmppc_load_guest_fp(vcpu); >> } > > This should go before the kvmppc_lazy_ee_enable(). Why? What difference does that make? We're running with interrupts disabled here, right? Alex
On 07/03/2013 12:23:16 PM, Alexander Graf wrote: > > On 03.07.2013, at 19:18, Scott Wood wrote: > > > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: > >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before > >> returning to guest instead of each sched in. Without this > improvement > >> an interrupt may also claim floting point corrupting guest state. > >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > >> --- > >> arch/powerpc/kvm/booke.c | 1 + > >> arch/powerpc/kvm/e500mc.c | 2 -- > >> 2 files changed, 1 insertions(+), 2 deletions(-) > >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > >> index 113961f..3cae2e3 100644 > >> --- a/arch/powerpc/kvm/booke.c > >> +++ b/arch/powerpc/kvm/booke.c > >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > >> r = (s << 2) | RESUME_HOST | (r & > RESUME_FLAG_NV); > >> } else { > >> kvmppc_lazy_ee_enable(); > >> + kvmppc_load_guest_fp(vcpu); > >> } > > > > This should go before the kvmppc_lazy_ee_enable(). > > Why? What difference does that make? We're running with interrupts > disabled here, right? Yes, and we want to minimize the code we run where we have interrupts disabled but the lazy ee state says they're enabled. So kvmppc_lazy_ee_enable() should be the last thing we do before entering asm code. See http://patchwork.ozlabs.org/patch/249565/ -Scott
On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: > Increase FPU laziness by calling kvmppc_load_guest_fp() just before > returning to guest instead of each sched in. Without this improvement > an interrupt may also claim floting point corrupting guest state. > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > --- > arch/powerpc/kvm/booke.c | 1 + > arch/powerpc/kvm/e500mc.c | 2 -- > 2 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index 113961f..3cae2e3 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > r = (s << 2) | RESUME_HOST | (r & > RESUME_FLAG_NV); > } else { > kvmppc_lazy_ee_enable(); > + kvmppc_load_guest_fp(vcpu); > } > } > > diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c > index 19c8379..09da1ac 100644 > --- a/arch/powerpc/kvm/e500mc.c > +++ b/arch/powerpc/kvm/e500mc.c > @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, > int cpu) > kvmppc_e500_tlbil_all(vcpu_e500); > __get_cpu_var(last_vcpu_on_cpu) = vcpu; > } > - > - kvmppc_load_guest_fp(vcpu); > } > > void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu) Can we now remove vcpu->fpu_active, and the comment that says "Kernel usage of FP (via enable_kernel_fp()) in this thread must not occur while vcpu->fpu_active is set."? -Scott
On 03.07.2013, at 19:44, Scott Wood wrote: > On 07/03/2013 12:23:16 PM, Alexander Graf wrote: >> On 03.07.2013, at 19:18, Scott Wood wrote: >> > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: >> >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before >> >> returning to guest instead of each sched in. Without this improvement >> >> an interrupt may also claim floting point corrupting guest state. >> >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> >> >> --- >> >> arch/powerpc/kvm/booke.c | 1 + >> >> arch/powerpc/kvm/e500mc.c | 2 -- >> >> 2 files changed, 1 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >> >> index 113961f..3cae2e3 100644 >> >> --- a/arch/powerpc/kvm/booke.c >> >> +++ b/arch/powerpc/kvm/booke.c >> >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, >> >> r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); >> >> } else { >> >> kvmppc_lazy_ee_enable(); >> >> + kvmppc_load_guest_fp(vcpu); >> >> } >> > >> > This should go before the kvmppc_lazy_ee_enable(). >> Why? What difference does that make? We're running with interrupts disabled here, right? > > Yes, and we want to minimize the code we run where we have interrupts disabled but the lazy ee state says they're enabled. So kvmppc_lazy_ee_enable() should be the last thing we do before entering asm code. > > See http://patchwork.ozlabs.org/patch/249565/ Ah, cool. So we should add a comment saying that this should be the last thing before entering asm code then :). That way we make sure nobody else repeats the same mistake. Alex
On 03.07.2013, at 20:37, Scott Wood wrote: > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before >> returning to guest instead of each sched in. Without this improvement >> an interrupt may also claim floting point corrupting guest state. >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> >> --- >> arch/powerpc/kvm/booke.c | 1 + >> arch/powerpc/kvm/e500mc.c | 2 -- >> 2 files changed, 1 insertions(+), 2 deletions(-) >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >> index 113961f..3cae2e3 100644 >> --- a/arch/powerpc/kvm/booke.c >> +++ b/arch/powerpc/kvm/booke.c >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, >> r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); >> } else { >> kvmppc_lazy_ee_enable(); >> + kvmppc_load_guest_fp(vcpu); >> } >> } >> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c >> index 19c8379..09da1ac 100644 >> --- a/arch/powerpc/kvm/e500mc.c >> +++ b/arch/powerpc/kvm/e500mc.c >> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >> kvmppc_e500_tlbil_all(vcpu_e500); >> __get_cpu_var(last_vcpu_on_cpu) = vcpu; >> } >> - >> - kvmppc_load_guest_fp(vcpu); >> } >> void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu) > > Can we now remove vcpu->fpu_active, and the comment that says "Kernel usage of FP (via > enable_kernel_fp()) in this thread must not occur while vcpu->fpu_active is set."? I think so, yes. Alex
> -----Original Message----- > From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc- > owner@vger.kernel.org] On Behalf Of Alexander Graf > Sent: Wednesday, July 03, 2013 9:40 PM > To: Wood Scott-B07421 > Cc: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org; > kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH 3/6] KVM: PPC: Book3E: Increase FPU laziness > > > On 03.07.2013, at 20:37, Scott Wood wrote: > > > On 07/03/2013 07:42:36 AM, Mihai Caraman wrote: > >> Increase FPU laziness by calling kvmppc_load_guest_fp() just before > >> returning to guest instead of each sched in. Without this improvement > >> an interrupt may also claim floting point corrupting guest state. > >> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> > >> --- > >> arch/powerpc/kvm/booke.c | 1 + > >> arch/powerpc/kvm/e500mc.c | 2 -- > >> 2 files changed, 1 insertions(+), 2 deletions(-) > >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > >> index 113961f..3cae2e3 100644 > >> --- a/arch/powerpc/kvm/booke.c > >> +++ b/arch/powerpc/kvm/booke.c > >> @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, > struct kvm_vcpu *vcpu, > >> r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); > >> } else { > >> kvmppc_lazy_ee_enable(); > >> + kvmppc_load_guest_fp(vcpu); > >> } > >> } > >> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c > >> index 19c8379..09da1ac 100644 > >> --- a/arch/powerpc/kvm/e500mc.c > >> +++ b/arch/powerpc/kvm/e500mc.c > >> @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, > int cpu) > >> kvmppc_e500_tlbil_all(vcpu_e500); > >> __get_cpu_var(last_vcpu_on_cpu) = vcpu; > >> } > >> - > >> - kvmppc_load_guest_fp(vcpu); > >> } > >> void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu) > > > > Can we now remove vcpu->fpu_active, and the comment that says "Kernel > usage of FP (via > > enable_kernel_fp()) in this thread must not occur while vcpu- > >fpu_active is set."? > > I think so, yes. Yes, as I already did this for AltiVec. -Mike
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 113961f..3cae2e3 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -1204,6 +1204,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu, r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV); } else { kvmppc_lazy_ee_enable(); + kvmppc_load_guest_fp(vcpu); } } diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c index 19c8379..09da1ac 100644 --- a/arch/powerpc/kvm/e500mc.c +++ b/arch/powerpc/kvm/e500mc.c @@ -143,8 +143,6 @@ void kvmppc_core_vcpu_load(struct kvm_vcpu *vcpu, int cpu) kvmppc_e500_tlbil_all(vcpu_e500); __get_cpu_var(last_vcpu_on_cpu) = vcpu; } - - kvmppc_load_guest_fp(vcpu); } void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
Increase FPU laziness by calling kvmppc_load_guest_fp() just before returning to guest instead of each sched in. Without this improvement an interrupt may also claim floting point corrupting guest state. Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com> --- arch/powerpc/kvm/booke.c | 1 + arch/powerpc/kvm/e500mc.c | 2 -- 2 files changed, 1 insertions(+), 2 deletions(-)