| 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
| ||
|
Message-ID: <87tuz9hpnu.fsf@vitty.brq.redhat.com>
Date: Wed, 17 Jun 2020 13:56:05 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Babu Moger <babu.moger@....com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
wanpengli@...cent.com, joro@...tes.org, x86@...nel.org,
sean.j.christopherson@...el.com, mingo@...hat.com, bp@...en8.de,
hpa@...or.com, pbonzini@...hat.com, tglx@...utronix.de,
jmattson@...gle.com
Subject: Re: [PATCH v2 1/3] KVM: X86: Move handling of INVPCID types to x86
Babu Moger <babu.moger@....com> writes:
> INVPCID instruction handling is mostly same across both VMX and
> SVM. So, move the code to common x86.c.
>
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---
> arch/x86/kvm/vmx/vmx.c | 68 +----------------------------------------
> arch/x86/kvm/x86.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/x86.h | 3 +-
> 3 files changed, 82 insertions(+), 68 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 170cc76a581f..b4140cfd15fd 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5477,11 +5477,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
> {
> u32 vmx_instruction_info;
> unsigned long type;
> - bool pcid_enabled;
> gva_t gva;
> - struct x86_exception e;
> - unsigned i;
> - unsigned long roots_to_free = 0;
> struct {
> u64 pcid;
> u64 gla;
> @@ -5508,69 +5504,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
> sizeof(operand), &gva))
> return 1;
>
> - if (kvm_read_guest_virt(vcpu, gva, &operand, sizeof(operand), &e)) {
> - kvm_inject_emulated_page_fault(vcpu, &e);
> - return 1;
> - }
> -
> - if (operand.pcid >> 12 != 0) {
> - kvm_inject_gp(vcpu, 0);
> - return 1;
> - }
> -
> - pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> -
> - switch (type) {
> - case INVPCID_TYPE_INDIV_ADDR:
> - if ((!pcid_enabled && (operand.pcid != 0)) ||
> - is_noncanonical_address(operand.gla, vcpu)) {
> - kvm_inject_gp(vcpu, 0);
> - return 1;
> - }
> - kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
> - return kvm_skip_emulated_instruction(vcpu);
> -
> - case INVPCID_TYPE_SINGLE_CTXT:
> - if (!pcid_enabled && (operand.pcid != 0)) {
> - kvm_inject_gp(vcpu, 0);
> - return 1;
> - }
> -
> - if (kvm_get_active_pcid(vcpu) == operand.pcid) {
> - kvm_mmu_sync_roots(vcpu);
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> - }
> -
> - for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++)
> - if (kvm_get_pcid(vcpu, vcpu->arch.mmu->prev_roots[i].pgd)
> - == operand.pcid)
> - roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
> -
> - kvm_mmu_free_roots(vcpu, vcpu->arch.mmu, roots_to_free);
> - /*
> - * If neither the current cr3 nor any of the prev_roots use the
> - * given PCID, then nothing needs to be done here because a
> - * resync will happen anyway before switching to any other CR3.
> - */
> -
> - return kvm_skip_emulated_instruction(vcpu);
> -
> - case INVPCID_TYPE_ALL_NON_GLOBAL:
> - /*
> - * Currently, KVM doesn't mark global entries in the shadow
> - * page tables, so a non-global flush just degenerates to a
> - * global flush. If needed, we could optimize this later by
> - * keeping track of global entries in shadow page tables.
> - */
> -
> - /* fall-through */
> - case INVPCID_TYPE_ALL_INCL_GLOBAL:
> - kvm_mmu_unload(vcpu);
> - return kvm_skip_emulated_instruction(vcpu);
> -
> - default:
> - BUG(); /* We have already checked above that type <= 3 */
> - }
> + return kvm_handle_invpcid_types(vcpu, gva, type);
Nit: redundant space.
> }
>
> static int handle_pml_full(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9e41b5135340..9c858ca0e592 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -70,6 +70,7 @@
> #include <asm/irq_remapping.h>
> #include <asm/mshyperv.h>
> #include <asm/hypervisor.h>
> +#include <asm/tlbflush.h>
> #include <asm/intel_pt.h>
> #include <asm/emulate_prefix.h>
> #include <clocksource/hyperv_timer.h>
> @@ -10714,6 +10715,84 @@ u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvm_spec_ctrl_valid_bits);
>
> +int kvm_handle_invpcid_types(struct kvm_vcpu *vcpu, gva_t gva,
> + unsigned long type)
(sorry if this was discussed before) do we really need '_types' suffix?
> +{
> + unsigned long roots_to_free = 0;
> + struct x86_exception e;
> + bool pcid_enabled;
> + unsigned int i;
> + struct {
> + u64 pcid;
> + u64 gla;
> + } operand;
> +
> + if (kvm_read_guest_virt(vcpu, gva, &operand, sizeof(operand), &e)) {
> + kvm_inject_emulated_page_fault(vcpu, &e);
> + return 1;
> + }
> +
> + if (operand.pcid >> 12 != 0) {
> + kvm_inject_gp(vcpu, 0);
> + return 1;
> + }
> +
> + pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
> +
> + switch (type) {
> + case INVPCID_TYPE_INDIV_ADDR:
> + if ((!pcid_enabled && (operand.pcid != 0)) ||
> + is_noncanonical_address(operand.gla, vcpu)) {
> + kvm_inject_gp(vcpu, 0);
> + return 1;
> + }
> + kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
> + return kvm_skip_emulated_instruction(vcpu);
> +
> + case INVPCID_TYPE_SINGLE_CTXT:
> + if (!pcid_enabled && (operand.pcid != 0)) {
> + kvm_inject_gp(vcpu, 0);
> + return 1;
> + }
> +
> + if (kvm_get_active_pcid(vcpu) == operand.pcid) {
> + kvm_mmu_sync_roots(vcpu);
> + kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + }
> +
> + for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++)
> + if (kvm_get_pcid(vcpu, vcpu->arch.mmu->prev_roots[i].pgd)
> + == operand.pcid)
> + roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
> +
> + kvm_mmu_free_roots(vcpu, vcpu->arch.mmu, roots_to_free);
> + /*
> + * If neither the current cr3 nor any of the prev_roots use the
> + * given PCID, then nothing needs to be done here because a
> + * resync will happen anyway before switching to any other CR3.
> + */
> +
> + return kvm_skip_emulated_instruction(vcpu);
> +
> + case INVPCID_TYPE_ALL_NON_GLOBAL:
> + /*
> + * Currently, KVM doesn't mark global entries in the shadow
> + * page tables, so a non-global flush just degenerates to a
> + * global flush. If needed, we could optimize this later by
> + * keeping track of global entries in shadow page tables.
> + */
> +
> + /* fall-through */
> + case INVPCID_TYPE_ALL_INCL_GLOBAL:
> + kvm_mmu_unload(vcpu);
> + return kvm_skip_emulated_instruction(vcpu);
> +
> + default:
> + BUG(); /* We have already checked above that type <= 3 */
The check was left in VMX' handle_invpcid() so we either need to update
the comment to something like "the caller was supposed to check that
type <= 3" or move the check to kvm_handle_invpcid_types().
> + }
> +}
> +EXPORT_SYMBOL_GPL(kvm_handle_invpcid_types);
> +
> EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
> EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio);
> EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 6eb62e97e59f..f706f6f7196d 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -365,5 +365,6 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
> void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
> u64 kvm_spec_ctrl_valid_bits(struct kvm_vcpu *vcpu);
> bool kvm_vcpu_exit_request(struct kvm_vcpu *vcpu);
> -
> +int kvm_handle_invpcid_types(struct kvm_vcpu *vcpu, gva_t gva,
> + unsigned long type);
> #endif
>
--
Vitaly
Powered by blists - more mailing lists