| Message ID | 20241203103735.2267589-17-qperret@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Non-protected guest stage-2 support for pKVM | expand |
Hi Quentin, On Tue, 3 Dec 2024 at 10:38, Quentin Perret <qperret@google.com> wrote: > > Introduce a new hypercall to flush the TLBs of non-protected guests. The > host kernel will be responsible for issuing this hypercall after changing > stage-2 permissions using the __pkvm_host_relax_guest_perms() or > __pkvm_host_wrprotect_guest() paths. This is left under the host's > responsibility for performance reasons. > > Note however that the TLB maintenance for all *unmap* operations still > remains entirely under the hypervisor's responsibility for security > reasons -- an unmapped page may be donated to another entity, so a stale > TLB entry could be used to leak private data. > > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/include/asm/kvm_asm.h | 1 + > arch/arm64/kvm/hyp/nvhe/hyp-main.c | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index 6178e12a0dbc..df6237d0459c 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -87,6 +87,7 @@ enum __kvm_host_smccc_func { > __KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm, > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_load, > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_put, > + __KVM_HOST_SMCCC_FUNC___pkvm_tlb_flush_vmid, > }; > > #define DECLARE_KVM_VHE_SYM(sym) extern char sym[] > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > index de0012a75827..219d7fb850ec 100644 > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > @@ -398,6 +398,22 @@ static void handle___kvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > __kvm_tlb_flush_vmid(kern_hyp_va(mmu)); > } > > +static void handle___pkvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > +{ > + DECLARE_REG(pkvm_handle_t, handle, host_ctxt, 1); > + struct pkvm_hyp_vm *hyp_vm; > + > + if (!is_protected_kvm_enabled()) > + return; > + > + hyp_vm = get_pkvm_hyp_vm(handle); > + if (!hyp_vm) > + return; > + > + __kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu); > + put_pkvm_hyp_vm(hyp_vm); > +} Since this is practically the same as kvm_tlb_flush_vmid(), does it make sense to modify that instead (handle___kvm_tlb_flush_vmid()) to do the right thing depending on whether pkvm is enabled? Thinking as well for the future in case we want to support the rest of the kvm_tlb_flush_vmid_*(). Cheers, /fuad > + > static void handle___kvm_flush_cpu_context(struct kvm_cpu_context *host_ctxt) > { > DECLARE_REG(struct kvm_s2_mmu *, mmu, host_ctxt, 1); > @@ -582,6 +598,7 @@ static const hcall_t host_hcall[] = { > HANDLE_FUNC(__pkvm_teardown_vm), > HANDLE_FUNC(__pkvm_vcpu_load), > HANDLE_FUNC(__pkvm_vcpu_put), > + HANDLE_FUNC(__pkvm_tlb_flush_vmid), > }; > > static void handle_host_hcall(struct kvm_cpu_context *host_ctxt) > -- > 2.47.0.338.g60cca15819-goog >
On Tuesday 10 Dec 2024 at 15:23:02 (+0000), Fuad Tabba wrote: > Hi Quentin, > > On Tue, 3 Dec 2024 at 10:38, Quentin Perret <qperret@google.com> wrote: > > > > Introduce a new hypercall to flush the TLBs of non-protected guests. The > > host kernel will be responsible for issuing this hypercall after changing > > stage-2 permissions using the __pkvm_host_relax_guest_perms() or > > __pkvm_host_wrprotect_guest() paths. This is left under the host's > > responsibility for performance reasons. > > > > Note however that the TLB maintenance for all *unmap* operations still > > remains entirely under the hypervisor's responsibility for security > > reasons -- an unmapped page may be donated to another entity, so a stale > > TLB entry could be used to leak private data. > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/include/asm/kvm_asm.h | 1 + > > arch/arm64/kvm/hyp/nvhe/hyp-main.c | 17 +++++++++++++++++ > > 2 files changed, 18 insertions(+) > > > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > > index 6178e12a0dbc..df6237d0459c 100644 > > --- a/arch/arm64/include/asm/kvm_asm.h > > +++ b/arch/arm64/include/asm/kvm_asm.h > > @@ -87,6 +87,7 @@ enum __kvm_host_smccc_func { > > __KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm, > > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_load, > > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_put, > > + __KVM_HOST_SMCCC_FUNC___pkvm_tlb_flush_vmid, > > }; > > > > #define DECLARE_KVM_VHE_SYM(sym) extern char sym[] > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > index de0012a75827..219d7fb850ec 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > @@ -398,6 +398,22 @@ static void handle___kvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > > __kvm_tlb_flush_vmid(kern_hyp_va(mmu)); > > } > > > > +static void handle___pkvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > > +{ > > + DECLARE_REG(pkvm_handle_t, handle, host_ctxt, 1); > > + struct pkvm_hyp_vm *hyp_vm; > > + > > + if (!is_protected_kvm_enabled()) > > + return; > > + > > + hyp_vm = get_pkvm_hyp_vm(handle); > > + if (!hyp_vm) > > + return; > > + > > + __kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu); > > + put_pkvm_hyp_vm(hyp_vm); > > +} > > Since this is practically the same as kvm_tlb_flush_vmid(), does it > make sense to modify that instead (handle___kvm_tlb_flush_vmid()) to > do the right thing depending on whether pkvm is enabled? Thinking as > well for the future in case we want to support the rest of the > kvm_tlb_flush_vmid_*(). I considered it, but the two implementations want different arguments -- pkvm wants the handle while standard KVM uses the kvm struct address directly. I had an implementation at some point that multiplexed the implementations on a single HVC (we'd interpret the arguments differently depending on pKVM being enabled or not) but that felt more error prone than simply having two HVCs. Happy to reconsider if we can find a good way to make it work though.
Hi Quentin, On Wed, 11 Dec 2024 at 10:03, Quentin Perret <qperret@google.com> wrote: > > On Tuesday 10 Dec 2024 at 15:23:02 (+0000), Fuad Tabba wrote: > > Hi Quentin, > > > > On Tue, 3 Dec 2024 at 10:38, Quentin Perret <qperret@google.com> wrote: > > > > > > Introduce a new hypercall to flush the TLBs of non-protected guests. The > > > host kernel will be responsible for issuing this hypercall after changing > > > stage-2 permissions using the __pkvm_host_relax_guest_perms() or > > > __pkvm_host_wrprotect_guest() paths. This is left under the host's > > > responsibility for performance reasons. > > > > > > Note however that the TLB maintenance for all *unmap* operations still > > > remains entirely under the hypervisor's responsibility for security > > > reasons -- an unmapped page may be donated to another entity, so a stale > > > TLB entry could be used to leak private data. > > > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > > --- > > > arch/arm64/include/asm/kvm_asm.h | 1 + > > > arch/arm64/kvm/hyp/nvhe/hyp-main.c | 17 +++++++++++++++++ > > > 2 files changed, 18 insertions(+) > > > > > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > > > index 6178e12a0dbc..df6237d0459c 100644 > > > --- a/arch/arm64/include/asm/kvm_asm.h > > > +++ b/arch/arm64/include/asm/kvm_asm.h > > > @@ -87,6 +87,7 @@ enum __kvm_host_smccc_func { > > > __KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm, > > > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_load, > > > __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_put, > > > + __KVM_HOST_SMCCC_FUNC___pkvm_tlb_flush_vmid, > > > }; > > > > > > #define DECLARE_KVM_VHE_SYM(sym) extern char sym[] > > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > > index de0012a75827..219d7fb850ec 100644 > > > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c > > > @@ -398,6 +398,22 @@ static void handle___kvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > > > __kvm_tlb_flush_vmid(kern_hyp_va(mmu)); > > > } > > > > > > +static void handle___pkvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) > > > +{ > > > + DECLARE_REG(pkvm_handle_t, handle, host_ctxt, 1); > > > + struct pkvm_hyp_vm *hyp_vm; > > > + > > > + if (!is_protected_kvm_enabled()) > > > + return; > > > + > > > + hyp_vm = get_pkvm_hyp_vm(handle); > > > + if (!hyp_vm) > > > + return; > > > + > > > + __kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu); > > > + put_pkvm_hyp_vm(hyp_vm); > > > +} > > > > Since this is practically the same as kvm_tlb_flush_vmid(), does it > > make sense to modify that instead (handle___kvm_tlb_flush_vmid()) to > > do the right thing depending on whether pkvm is enabled? Thinking as > > well for the future in case we want to support the rest of the > > kvm_tlb_flush_vmid_*(). > > I considered it, but the two implementations want different arguments -- > pkvm wants the handle while standard KVM uses the kvm struct address > directly. I had an implementation at some point that multiplexed the > implementations on a single HVC (we'd interpret the arguments > differently depending on pKVM being enabled or not) but that felt more > error prone than simply having two HVCs. > > Happy to reconsider if we can find a good way to make it work though. I don't have a strong opinion about this. I think that for now, since it's only this function, it's probably fine. That said, the multiplexing is (as of patch 18, which I haven't reviewed yet) is just lifted higher up to the host kernel, albeit with fewer parameters to wiggle around. To summarize, I think we can worry about it if/once we need the other tlb_flush_* variants. Cheers, /fuad
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index 6178e12a0dbc..df6237d0459c 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -87,6 +87,7 @@ enum __kvm_host_smccc_func { __KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm, __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_load, __KVM_HOST_SMCCC_FUNC___pkvm_vcpu_put, + __KVM_HOST_SMCCC_FUNC___pkvm_tlb_flush_vmid, }; #define DECLARE_KVM_VHE_SYM(sym) extern char sym[] diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c index de0012a75827..219d7fb850ec 100644 --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c @@ -398,6 +398,22 @@ static void handle___kvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) __kvm_tlb_flush_vmid(kern_hyp_va(mmu)); } +static void handle___pkvm_tlb_flush_vmid(struct kvm_cpu_context *host_ctxt) +{ + DECLARE_REG(pkvm_handle_t, handle, host_ctxt, 1); + struct pkvm_hyp_vm *hyp_vm; + + if (!is_protected_kvm_enabled()) + return; + + hyp_vm = get_pkvm_hyp_vm(handle); + if (!hyp_vm) + return; + + __kvm_tlb_flush_vmid(&hyp_vm->kvm.arch.mmu); + put_pkvm_hyp_vm(hyp_vm); +} + static void handle___kvm_flush_cpu_context(struct kvm_cpu_context *host_ctxt) { DECLARE_REG(struct kvm_s2_mmu *, mmu, host_ctxt, 1); @@ -582,6 +598,7 @@ static const hcall_t host_hcall[] = { HANDLE_FUNC(__pkvm_teardown_vm), HANDLE_FUNC(__pkvm_vcpu_load), HANDLE_FUNC(__pkvm_vcpu_put), + HANDLE_FUNC(__pkvm_tlb_flush_vmid), }; static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
Introduce a new hypercall to flush the TLBs of non-protected guests. The host kernel will be responsible for issuing this hypercall after changing stage-2 permissions using the __pkvm_host_relax_guest_perms() or __pkvm_host_wrprotect_guest() paths. This is left under the host's responsibility for performance reasons. Note however that the TLB maintenance for all *unmap* operations still remains entirely under the hypervisor's responsibility for security reasons -- an unmapped page may be donated to another entity, so a stale TLB entry could be used to leak private data. Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/include/asm/kvm_asm.h | 1 + arch/arm64/kvm/hyp/nvhe/hyp-main.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)