lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Oct 2021 15:45:13 +0200 From: Laurent Vivier <lvivier@...hat.com> To: Greg Kurz <groug@...d.org> Cc: kvm-ppc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, Benjamin Herrenschmidt <benh@...nel.crashing.org>, Michael Ellerman <mpe@...erman.id.au>, linux-kernel@...r.kernel.org, Nicholas Piggin <npiggin@...il.com>, Paul Mackerras <paulus@...abs.org> Subject: Re: [PATCH] KVM: PPC: Defer vtime accounting 'til after IRQ handling On 06/10/2021 12:42, Greg Kurz wrote: > On Wed, 6 Oct 2021 09:37:45 +0200 > Laurent Vivier <lvivier@...hat.com> wrote: > >> Commit 61bd0f66ff92 has moved guest_enter() out of the interrupt >> protected area to be able to have updated tick counters, but >> commit 112665286d08 moved back to this area to avoid wrong >> context warning (or worse). >> >> None of them are correct, to fix the problem port to POWER >> the x86 fix 160457140187 ("KVM: x86: Defer vtime accounting 'til >> after IRQ handling"): >> >> "Defer the call to account guest time until after servicing any IRQ(s) >> that happened in the guest or immediately after VM-Exit. Tick-based >> accounting of vCPU time relies on PF_VCPU being set when the tick IRQ >> handler runs, and IRQs are blocked throughout the main sequence of >> vcpu_enter_guest(), including the call into vendor code to actually >> enter and exit the guest." >> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2009312 >> Fixes: 61bd0f66ff92 ("KVM: PPC: Book3S HV: Fix guest time accounting with VIRT_CPU_ACCOUNTING_GEN") > > This patch was merged in linux 4.16 and thus is on the 4.16.y > stable branch and it was backported to stable 4.14.y. It would > make sense to Cc: stable # v4.14 also, but... > >> Fixes: 112665286d08 ("KVM: PPC: Book3S HV: Context tracking exit guest context before enabling irqs") > > ... this one, which was merged in linux 5.12, was never backported > anywhere because it wasn't considered as a fix, as commented here: > > https://lore.kernel.org/linuxppc-dev/1610793296.fjhomer31g.astroid@bobo.none/ > > AFAICT commit 61bd0f66ff92 was never mentioned anywhere in a bug. The > first Fixes: tag thus looks a bit misleading. I'd personally drop it > and Cc: stable # v5.12. > Ok, I update the comment. >> Cc: npiggin@...il.com >> >> Signed-off-by: Laurent Vivier <lvivier@...hat.com> >> --- >> arch/powerpc/kvm/book3s_hv.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c >> index 2acb1c96cfaf..43e1ce853785 100644 >> --- a/arch/powerpc/kvm/book3s_hv.c >> +++ b/arch/powerpc/kvm/book3s_hv.c >> @@ -3695,6 +3695,8 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc) >> >> srcu_read_unlock(&vc->kvm->srcu, srcu_idx); >> >> + context_tracking_guest_exit(); >> + >> set_irq_happened(trap); >> >> spin_lock(&vc->lock); >> @@ -3726,9 +3728,8 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc) >> >> kvmppc_set_host_core(pcpu); >> >> - guest_exit_irqoff(); >> - > > > Change looks ok but it might be a bit confusing for the > occasional reader that guest_enter_irqoff() isn't matched > by a guest_exit_irqoff(). > >> local_irq_enable(); >> + vtime_account_guest_exit(); >> > > Maybe add a comment like in x86 ? > done >> /* Let secondaries go back to the offline loop */ >> for (i = 0; i < controlled_threads; ++i) { >> @@ -4506,13 +4507,14 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit, >> >> srcu_read_unlock(&kvm->srcu, srcu_idx); >> >> + context_tracking_guest_exit(); >> + >> set_irq_happened(trap); >> >> kvmppc_set_host_core(pcpu); >> >> - guest_exit_irqoff(); >> - >> local_irq_enable(); >> + vtime_account_guest_exit(); >> >> cpumask_clear_cpu(pcpu, &kvm->arch.cpu_in_guest); >> > > Same remarks. FWIW a followup was immediately added to x86 to > encapsulate the enter/exit logic in common helpers : > > ommit bc908e091b3264672889162733020048901021fb > Author: Sean Christopherson <seanjc@...gle.com> > Date: Tue May 4 17:27:35 2021 -0700 > > KVM: x86: Consolidate guest enter/exit logic to common helpers > > This makes the code nicer. Maybe do something similar for POWER ? I don't like to modify kernel code when it's not needed. I just want to fix a bug, if someone wants this nicer I let this to him... Thanks, Laurent
Powered by blists - more mailing lists