From: Marc Zyngier <maz@kernel.org>
To: Fuad Tabba <tabba@google.com>
Cc: kvmarm@lists.linux.dev, will@kernel.org, qperret@google.com,
seanjc@google.com, alexandru.elisei@arm.com,
catalin.marinas@arm.com, philmd@linaro.org, james.morse@arm.com,
suzuki.poulose@arm.com, oliver.upton@linux.dev,
mark.rutland@arm.com, broonie@kernel.org, joey.gouly@arm.com,
rananta@google.com
Subject: Re: [PATCH v1 07/44] KVM: arm64: Support TLB invalidation in guest context
Date: Mon, 15 Apr 2024 16:59:06 +0100 [thread overview]
Message-ID: <865xwisjlh.wl-maz@kernel.org> (raw)
In-Reply-To: <CA+EHjTwHdCuzsiE1xGVgY1ZrWzty9wVF7yr5wHx=9UCC4_bc6g@mail.gmail.com>
On Mon, 15 Apr 2024 16:02:02 +0100,
Fuad Tabba <tabba@google.com> wrote:
>
> Hi Marc,
>
> On Mon, Apr 15, 2024 at 12:36 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 17:34:54 +0000,
> > Fuad Tabba <tabba@google.com> wrote:
> > >
> > > From: Will Deacon <will@kernel.org>
> > >
> > > Typically, TLB invalidation of guest stage-2 mappings using nVHE is
> > > performed by a hypercall originating from the host. For the invalidation
> > > instruction to be effective, therefore, __tlb_switch_to_{guest,host}()
> > > swizzle the active stage-2 context around the TLBI instruction.
> > >
> > > With guest-to-host memory sharing and unsharing hypercalls
> > > originating from the guest under pKVM, there is need to support
> > > both guest and host VMID invalidations issued from guest context.
> > >
> > > Replace the __tlb_switch_to_{guest,host}() functions with a more general
> > > {enter,exit}_vmid_context() implementation which supports being invoked
> > > from guest context and acts as a no-op if the target context matches the
> > > running context.
> > >
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > Signed-off-by: Fuad Tabba <tabba@google.com>
> > > ---
> > > arch/arm64/kvm/hyp/nvhe/tlb.c | 114 +++++++++++++++++++++++++++-------
> > > 1 file changed, 90 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/tlb.c b/arch/arm64/kvm/hyp/nvhe/tlb.c
> > > index a60fb13e2192..05a66b2ed76d 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/tlb.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/tlb.c
> > > @@ -11,13 +11,23 @@
> > > #include <nvhe/mem_protect.h>
> > >
> > > struct tlb_inv_context {
> > > - u64 tcr;
> > > + struct kvm_s2_mmu *mmu;
> > > + u64 tcr;
> > > + u64 sctlr;
> > > };
> > >
> > > -static void __tlb_switch_to_guest(struct kvm_s2_mmu *mmu,
> > > - struct tlb_inv_context *cxt,
> > > - bool nsh)
> > > +static void enter_vmid_context(struct kvm_s2_mmu *mmu,
> > > + struct tlb_inv_context *cxt,
> > > + bool nsh)
> > > {
> > > + struct kvm_s2_mmu *host_s2_mmu = &host_mmu.arch.mmu;
> > > + struct kvm_cpu_context *host_ctxt;
> > > + struct kvm_vcpu *vcpu;
> > > +
> > > + host_ctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> > > + vcpu = host_ctxt->__hyp_running_vcpu;
> > > + cxt->mmu = NULL;
> > > +
> > > /*
> > > * We have two requirements:
> > > *
> > > @@ -40,20 +50,52 @@ static void __tlb_switch_to_guest(struct kvm_s2_mmu *mmu,
> > > else
> > > dsb(ish);
> > >
> > > + /*
> > > + * If we're already in the desired context, then there's nothing
> > > + * to do.
> > > + */
> > > + if (vcpu) {
> > > + /* We're in guest context */
> > > + if (mmu == vcpu->arch.hw_mmu || WARN_ON(mmu != host_s2_mmu))
> > > + return;
> >
> > I'm a bit concerned about this one, not so much for what it does, but
> > because it outlines an inconsistency we have.
> >
> > Under memory pressure, we can end-up unmapping a page via the MMU
> > notifiers, and will provide a s2_mmu context for the TLBI. This can
> > happen while *another* context is loaded (a vcpu from a different VM)
> > and that vcpu faults.
> >
> > You'd end up with a scenario very similar to the one I debugged here:
> >
> > https://lore.kernel.org/kvmarm/86y1gfn67v.wl-maz@kernel.org
> >
> > Now, this doesn't break here because __hyp_running_vcpu is set to NULL
> > on each exit from the HYP code. But that only happens on nVHE, and not
> > on VHE, which bizarrely only sets this on entry and leaves a dangling
> > pointer...
> >
> > I think we need to clarify how and when this pointer is considered
> > valid.
>
> Right. I'll add the patch to fix the dangling pointer in VHE. Should
> I add a comment about the validity of the pointer where it's defined
> as well?
Don't bother with the VHE bit, I'm already on it (moving it all the
way to load/put for consistency), and I'm currently testing the hack.
But adding a comment explaining that this plays the role of
kvm_get_current_vcpu() and that it is only valid from within
__kvm_vcpu_run() would be great.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
next prev parent reply other threads:[~2024-04-15 15:59 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-27 17:34 [PATCH v1 00/44] KVM: arm64: Preamble for pKVM Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 01/44] KVM: arm64: Change kvm_handle_mmio_return() return polarity Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 02/44] KVM: arm64: Use enum instead of helper for checking FP-state Fuad Tabba
2024-03-28 16:19 ` Mark Brown
2024-04-08 7:39 ` Marc Zyngier
2024-04-08 13:39 ` Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 03/44] KVM: arm64: Move setting the page as dirty out of the critical section Fuad Tabba
2024-04-08 7:41 ` Marc Zyngier
2024-04-08 15:41 ` Fuad Tabba
2024-04-08 15:53 ` Marc Zyngier
2024-04-08 15:57 ` Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 04/44] KVM: arm64: Avoid BUG-ing from the host abort path Fuad Tabba
2024-04-08 7:44 ` Marc Zyngier
2024-04-08 13:48 ` Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 05/44] KVM: arm64: Check for PTE validity when checking for executable/cacheable Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 06/44] KVM: arm64: Avoid BBM when changing only s/w bits in Stage-2 PTE Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 07/44] KVM: arm64: Support TLB invalidation in guest context Fuad Tabba
2024-04-15 11:36 ` Marc Zyngier
2024-04-15 15:02 ` Fuad Tabba
2024-04-15 15:59 ` Marc Zyngier [this message]
2024-03-27 17:34 ` [PATCH v1 08/44] KVM: arm64: Simplify vgic-v3 hypercalls Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 09/44] KVM: arm64: Add is_pkvm_initialized() helper Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 10/44] KVM: arm64: Introduce predicates to check for protected state Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 11/44] KVM: arm64: Split up nvhe/fixed_config.h Fuad Tabba
2024-03-27 17:34 ` [PATCH v1 12/44] KVM: arm64: Move pstate reset value definitions to kvm_arm.h Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 13/44] KVM: arm64: Clarify rationale for ZCR_EL1 value restored on guest exit Fuad Tabba
2024-03-28 18:53 ` Mark Brown
2024-04-08 13:34 ` Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 14/44] KVM: arm64: Refactor calculating SVE state size to use helpers Fuad Tabba
2024-03-28 18:57 ` Mark Brown
2024-04-08 13:35 ` Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 15/44] KVM: arm64: Use active guest SVE vector length on guest restore Fuad Tabba
2024-03-28 19:17 ` Mark Brown
2024-04-09 9:34 ` Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 16/44] KVM: arm64: Do not map the host fpsimd state to hyp in pKVM Fuad Tabba
2024-03-28 19:20 ` Mark Brown
2024-03-27 17:35 ` [PATCH v1 17/44] KVM: arm64: Move some kvm_psci functions to a shared header Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 18/44] KVM: arm64: Refactor reset_mpidr() to extract its computation Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 19/44] KVM: arm64: Refactor kvm_vcpu_enable_ptrauth() for hyp use Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 20/44] KVM: arm64: Refactor enter_exception64() Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 21/44] KVM: arm64: Add PC_UPDATE_REQ flags covering all PC updates Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 22/44] KVM: arm64: Add vcpu flag copy primitive Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 23/44] KVM: arm64: Introduce gfn_to_memslot_prot() Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 24/44] KVM: arm64: Do not use the hva in kvm_handle_guest_abort() Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 25/44] KVM: arm64: Introduce hyp_rwlock_t Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 26/44] KVM: arm64: Add atomics-based checking refcount implementation at EL2 Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 27/44] KVM: arm64: Use atomic refcount helpers for 'struct hyp_page::refcount' Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 28/44] KVM: arm64: Remove locking from EL2 allocation fast-paths Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 29/44] KVM: arm64: Reformat/beautify PTP hypercall documentation Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 30/44] KVM: arm64: Rename firmware pseudo-register documentation file Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 31/44] KVM: arm64: Document the KVM/arm64-specific calls in hypercalls.rst Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 32/44] KVM: arm64: Prevent kmemleak from accessing .hyp.data Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 33/44] KVM: arm64: Issue CMOs when tearing down guest s2 pages Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 34/44] KVM: arm64: Do not set the virtual timer offset for protected vCPUs Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 35/44] KVM: arm64: Fix comment for __pkvm_vcpu_init_traps() Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 36/44] KVM: arm64: Do not re-initialize the KVM lock Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 37/44] KVM: arm64: Check directly whether a vcpu is protected Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 38/44] KVM: arm64: Trap debug break and watch from guest Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 39/44] KVM: arm64: Restrict protected VM capabilities Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 40/44] KVM: arm64: Do not support MTE for protected VMs Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 41/44] KVM: arm64: Move pkvm_vcpu_init_traps() to hyp vcpu init Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 42/44] KVM: arm64: Fix initializing traps in protected mode Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 43/44] KVM: arm64: Advertise GICv3 sysreg interface to protected guests Fuad Tabba
2024-03-27 17:35 ` [PATCH v1 44/44] KVM: arm64: Force injection of a data abort on NISV MMIO exit Fuad Tabba
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=865xwisjlh.wl-maz@kernel.org \
--to=maz@kernel.org \
--cc=alexandru.elisei@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=james.morse@arm.com \
--cc=joey.gouly@arm.com \
--cc=kvmarm@lists.linux.dev \
--cc=mark.rutland@arm.com \
--cc=oliver.upton@linux.dev \
--cc=philmd@linaro.org \
--cc=qperret@google.com \
--cc=rananta@google.com \
--cc=seanjc@google.com \
--cc=suzuki.poulose@arm.com \
--cc=tabba@google.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 read-only IMAP folder(s) and NNTP newsgroup(s).