kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: linux-kernel@vger.kernel.org, aleksandar.qemu.devel@gmail.com,
	alexandru.elisei@arm.com, anup.patel@wdc.com,
	aou@eecs.berkeley.edu, atish.patra@wdc.com,
	benh@kernel.crashing.org, bp@alien8.de, catalin.marinas@arm.com,
	chenhuacai@kernel.org, dave.hansen@linux.intel.com,
	david@redhat.com, frankja@linux.ibm.com, frederic@kernel.org,
	gor@linux.ibm.com, hca@linux.ibm.com, imbrenda@linux.ibm.com,
	james.morse@arm.com, jmattson@google.com, joro@8bytes.org,
	kvm@vger.kernel.org, maz@kernel.org, mingo@redhat.com,
	mpe@ellerman.id.au, nsaenzju@redhat.com, palmer@dabbelt.com,
	paulmck@kernel.org, paulus@samba.org, paul.walmsley@sifive.com,
	pbonzini@redhat.com, seanjc@google.com, suzuki.poulose@arm.com,
	tglx@linutronix.de, tsbogend@alpha.franken.de,
	vkuznets@redhat.com, wanpengli@tencent.com, will@kernel.org
Subject: Re: [PATCH 0/5] kvm: fix latent guest entry/exit bugs
Date: Fri, 14 Jan 2022 12:19:30 +0000	[thread overview]
Message-ID: <YeFqUlhqY+7uzUT1@FVFF77S0Q05N> (raw)
In-Reply-To: <127a6117-85fb-7477-983c-daf09e91349d@linux.ibm.com>

On Thu, Jan 13, 2022 at 04:20:07PM +0100, Christian Borntraeger wrote:
> 
> 
> Am 11.01.22 um 16:35 schrieb Mark Rutland:
> > Several architectures have latent bugs around guest entry/exit, most
> > notably:
> > 
> > 1) Several architectures enable interrupts between guest_enter() and
> >     guest_exit(). As this period is an RCU extended quiescent state (EQS) this
> >     is unsound unless the irq entry code explicitly wakes RCU, which most
> >     architectures only do for entry from usersapce or idle.
> > 
> >     I believe this affects: arm64, riscv, s390
> > 
> >     I am not sure about powerpc.
> > 
> > 2) Several architectures permit instrumentation of code between
> >     guest_enter() and guest_exit(), e.g. KASAN, KCOV, KCSAN, etc. As
> >     instrumentation may directly o indirectly use RCU, this has the same
> >     problems as with interrupts.
> > 
> >     I believe this affects: arm64, mips, powerpc, riscv, s390
> > 
> > 3) Several architectures do not inform lockdep and tracing that
> >     interrupts are enabled during the execution of the guest, or do so in
> >     an incorrect order. Generally
> >     this means that logs will report IRQs being masked for much longer
> >     than is actually the case, which is not ideal for debugging. I don't
> >     know whether this affects the correctness of lockdep.
> > 
> >     I believe this affects: arm64, mips, powerpc, riscv, s390
> > 
> > This was previously fixed for x86 specifically in a series of commits:
> > 
> >    87fa7f3e98a1310e ("x86/kvm: Move context tracking where it belongs")
> >    0642391e2139a2c1 ("x86/kvm/vmx: Add hardirq tracing to guest enter/exit")
> >    9fc975e9efd03e57 ("x86/kvm/svm: Add hardirq tracing on guest enter/exit")
> >    3ebccdf373c21d86 ("x86/kvm/vmx: Move guest enter/exit into .noinstr.text")
> >    135961e0a7d555fc ("x86/kvm/svm: Move guest enter/exit into .noinstr.text")
> >    160457140187c5fb ("KVM: x86: Defer vtime accounting 'til after IRQ handling")
> >    bc908e091b326467 ("KVM: x86: Consolidate guest enter/exit logic to common helpers")
> > 
> > But other architectures were left broken, and the infrastructure for
> > handling this correctly is x86-specific.
> > 
> > This series introduces generic helper functions which can be used to
> > handle the problems above, and migrates architectures over to these,
> > fixing the latent issues.
> > 
> > I wasn't able to figure my way around powerpc and s390, so I have not
> 
> I think 2 later patches have moved the guest_enter/exit a bit out.
> Does this make the s390 code clearer?

Yes; that's much simpler to follow!

One major thing I wasn't sure about for s390 is the sequence:

	guest_enter_irqoff();	// Enters an RCU EQS
	...
	local_irq_enable();
	...
	sie64a(...);
	...
	local_irq_disable();
	...
	guest_exit_irqoff();	// Exits an RCU EQS

... since if an IRQ is taken between local_irq_{enable,disable}(), RCU won't be
watching, and I couldn't spot whether your regular IRQ entry logic would wake
RCU in this case, or whether there was something else I'm missing that saves
you here.

For other architectures, including x86 and arm64, we enter the guest with IRQs
masked and return from the guest with IRQs masked, and don't actually take IRQs
until we unmask them in the host, after the guest_exit_*() logic has woken RCU
and so on.

I wasn't able to find documentation on the semantics of SIE, so I couldn't spot
whether the local_irq_{enable,disable}() calls were necessary, or could be
removed.

Thanks,
Mark.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 577f1ead6a51..5859207c2cc0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4145,10 +4145,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>                  * As PF_VCPU will be used in fault handler, between
>                  * guest_enter and guest_exit should be no uaccess.
>                  */
> -               local_irq_disable();
> -               guest_enter_irqoff();
> -               __disable_cpu_timer_accounting(vcpu);
> -               local_irq_enable();
>                 if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>                         memcpy(sie_page->pv_grregs,
>                                vcpu->run->s.regs.gprs,
> @@ -4156,8 +4152,16 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>                 }
>                 if (test_cpu_flag(CIF_FPU))
>                         load_fpu_regs();
> +               local_irq_disable();
> +               __disable_cpu_timer_accounting(vcpu);
> +               guest_enter_irqoff();
> +               local_irq_enable();
>                 exit_reason = sie64a(vcpu->arch.sie_block,
>                                      vcpu->run->s.regs.gprs);
> +               local_irq_disable();
> +               guest_exit_irqoff();
> +               __enable_cpu_timer_accounting(vcpu);
> +               local_irq_enable();
>                 if (kvm_s390_pv_cpu_is_protected(vcpu)) {
>                         memcpy(vcpu->run->s.regs.gprs,
>                                sie_page->pv_grregs,
> @@ -4173,10 +4177,6 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>                                 vcpu->arch.sie_block->gpsw.mask &= ~PSW_INT_MASK;
>                         }
>                 }
> -               local_irq_disable();
> -               __enable_cpu_timer_accounting(vcpu);
> -               guest_exit_irqoff();
> -               local_irq_enable();
>                 vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>                 rc = vcpu_post_run(vcpu, exit_reason);

  reply	other threads:[~2022-01-14 12:19 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-11 15:35 [PATCH 0/5] kvm: fix latent guest entry/exit bugs Mark Rutland
2022-01-11 15:35 ` [PATCH 1/5] kvm: add exit_to_guest_mode() and enter_from_guest_mode() Mark Rutland
2022-01-11 17:54   ` Marc Zyngier
2022-01-13 11:01     ` Mark Rutland
2022-01-13 11:55       ` Marc Zyngier
2022-01-13 13:01         ` Mark Rutland
2022-01-13 20:32   ` Sean Christopherson
2022-01-14 11:48     ` Mark Rutland
2022-01-14 16:11       ` Sean Christopherson
2022-01-18 13:01         ` Mark Rutland
2022-01-11 15:35 ` [PATCH 2/5] kvm/arm64: rework guest entry logic Mark Rutland
2022-01-11 17:55   ` Marc Zyngier
2022-01-13 11:17     ` Mark Rutland
2022-01-13 11:43       ` Marc Zyngier
2022-01-13 12:58         ` Mark Rutland
2022-01-11 15:35 ` [PATCH 3/5] kvm/mips: " Mark Rutland
2022-01-11 15:35 ` [PATCH 4/5] kvm/riscv: " Mark Rutland
2022-01-11 15:35 ` [PATCH 5/5] kvm/x86: " Mark Rutland
2022-01-13 20:50   ` Sean Christopherson
2022-01-14 12:05     ` Mark Rutland
2022-01-14 16:49       ` Sean Christopherson
2022-01-11 18:47 ` [PATCH 0/5] kvm: fix latent guest entry/exit bugs Palmer Dabbelt
2022-01-13 15:20 ` Christian Borntraeger
2022-01-14 12:19   ` Mark Rutland [this message]
2022-01-14 12:29     ` Christian Borntraeger
2022-01-14 13:32       ` Mark Rutland
2022-01-14 13:51         ` Christian Borntraeger
2022-01-14 15:19           ` Mark Rutland
2022-01-17 17:45             ` Paolo Bonzini
2022-01-18 12:02               ` Mark Rutland
2022-01-18 12:08                 ` Christian Borntraeger
2022-01-18 12:42                 ` Christian Borntraeger
2022-01-18 13:12                   ` Mark Rutland
2022-01-18 14:15                     ` Christian Borntraeger
2022-01-18 15:43                       ` Mark Rutland
2022-01-18 16:09                     ` Sven Schnelle
2022-01-18 17:50                       ` Mark Rutland
2022-01-18 18:12                         ` Mark Rutland
2022-01-19  6:41                         ` Sven Schnelle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YeFqUlhqY+7uzUT1@FVFF77S0Q05N \
    --to=mark.rutland@arm.com \
    --cc=aleksandar.qemu.devel@gmail.com \
    --cc=alexandru.elisei@arm.com \
    --cc=anup.patel@wdc.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=atish.patra@wdc.com \
    --cc=benh@kernel.crashing.org \
    --cc=borntraeger@linux.ibm.com \
    --cc=bp@alien8.de \
    --cc=catalin.marinas@arm.com \
    --cc=chenhuacai@kernel.org \
    --cc=dave.hansen@linux.intel.com \
    --cc=david@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=frederic@kernel.org \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=imbrenda@linux.ibm.com \
    --cc=james.morse@arm.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mpe@ellerman.id.au \
    --cc=nsaenzju@redhat.com \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=paulmck@kernel.org \
    --cc=paulus@samba.org \
    --cc=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    --cc=suzuki.poulose@arm.com \
    --cc=tglx@linutronix.de \
    --cc=tsbogend@alpha.franken.de \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).