| 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: <87sg3u5l8z.fsf@vitty.brq.redhat.com>
Date: Tue, 13 Apr 2021 16:09:48 +0200
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Siddharth Chandrasekaran <sidcha@...zon.de>
Cc: Alexander Graf <graf@...zon.com>,
Evgeny Iakovlev <eyakovl@...zon.de>,
Liran Alon <liran@...zon.com>,
Ioannis Aslanidis <iaslan@...zon.de>,
linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org, "K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Stephen Hemminger <sthemmin@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Sean Christopherson <seanjc@...gle.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH v2 3/4] KVM: x86: kvm_hv_flush_tlb use inputs from XMM
registers
Siddharth Chandrasekaran <sidcha@...zon.de> writes:
> Hyper-V supports the use of XMM registers to perform fast hypercalls.
> This allows guests to take advantage of the improved performance of the
> fast hypercall interface even though a hypercall may require more than
> (the current maximum of) two input registers.
>
> The XMM fast hypercall interface uses six additional XMM registers (XMM0
> to XMM5) to allow the guest to pass an input parameter block of up to
> 112 bytes. Hyper-V can also return data back to the guest in the
> remaining XMM registers that are not used by the current hypercall.
>
> Add framework to read/write to XMM registers in kvm_hv_hypercall() and
> use the additional hypercall inputs from XMM registers in
> kvm_hv_flush_tlb() when possible.
>
> Cc: Alexander Graf <graf@...zon.com>
> Co-developed-by: Evgeny Iakovlev <eyakovl@...zon.de>
> Signed-off-by: Evgeny Iakovlev <eyakovl@...zon.de>
> Signed-off-by: Siddharth Chandrasekaran <sidcha@...zon.de>
> ---
> arch/x86/kvm/hyperv.c | 109 ++++++++++++++++++++++++++++++++++--------
> 1 file changed, 90 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 8f6babd1ea0d..1f9959aba70d 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -36,6 +36,7 @@
>
> #include "trace.h"
> #include "irq.h"
> +#include "fpu.h"
>
> /* "Hv#1" signature */
> #define HYPERV_CPUID_SIGNATURE_EAX 0x31237648
> @@ -1623,6 +1624,8 @@ static __always_inline unsigned long *sparse_set_to_vcpu_mask(
> return vcpu_bitmap;
> }
>
> +#define KVM_HV_HYPERCALL_MAX_XMM_REGISTERS 6
Nitpick: this is not KVM-specific so could probably go to arch/x86/include/asm/hyperv-tlfs.h
> +
> struct kvm_hv_hcall {
> u64 param;
> u64 ingpa;
> @@ -1632,10 +1635,14 @@ struct kvm_hv_hcall {
> u16 rep_idx;
> bool fast;
> bool rep;
> + sse128_t xmm[KVM_HV_HYPERCALL_MAX_XMM_REGISTERS];
> + bool xmm_dirty;
> };
>
> static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool ex)
> {
> + int i, j;
> + gpa_t gpa;
> struct kvm *kvm = vcpu->kvm;
> struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
> struct hv_tlb_flush_ex flush_ex;
> @@ -1649,8 +1656,15 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool
> bool all_cpus;
>
> if (!ex) {
> - if (unlikely(kvm_read_guest(kvm, hc->ingpa, &flush, sizeof(flush))))
> - return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + if (hc->fast) {
> + flush.address_space = hc->ingpa;
> + flush.flags = hc->outgpa;
> + flush.processor_mask = sse128_lo(hc->xmm[0]);
> + } else {
> + if (unlikely(kvm_read_guest(kvm, hc->ingpa,
> + &flush, sizeof(flush))))
> + return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + }
>
> trace_kvm_hv_flush_tlb(flush.processor_mask,
> flush.address_space, flush.flags);
> @@ -1668,9 +1682,16 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool
> all_cpus = (flush.flags & HV_FLUSH_ALL_PROCESSORS) ||
> flush.processor_mask == 0;
> } else {
> - if (unlikely(kvm_read_guest(kvm, hc->ingpa, &flush_ex,
> - sizeof(flush_ex))))
> - return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + if (hc->fast) {
> + flush_ex.address_space = hc->ingpa;
> + flush_ex.flags = hc->outgpa;
> + memcpy(&flush_ex.hv_vp_set,
> + &hc->xmm[0], sizeof(hc->xmm[0]));
> + } else {
> + if (unlikely(kvm_read_guest(kvm, hc->ingpa, &flush_ex,
> + sizeof(flush_ex))))
> + return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + }
>
> trace_kvm_hv_flush_tlb_ex(flush_ex.hv_vp_set.valid_bank_mask,
> flush_ex.hv_vp_set.format,
> @@ -1681,20 +1702,29 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool
> all_cpus = flush_ex.hv_vp_set.format !=
> HV_GENERIC_SET_SPARSE_4K;
>
> - sparse_banks_len =
> - bitmap_weight((unsigned long *)&valid_bank_mask, 64) *
> - sizeof(sparse_banks[0]);
> + sparse_banks_len = bitmap_weight((unsigned long *)&valid_bank_mask, 64);
>
> if (!sparse_banks_len && !all_cpus)
> goto ret_success;
>
> - if (!all_cpus &&
> - kvm_read_guest(kvm,
> - hc->ingpa + offsetof(struct hv_tlb_flush_ex,
> - hv_vp_set.bank_contents),
> - sparse_banks,
> - sparse_banks_len))
> - return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + if (!all_cpus) {
> + if (hc->fast) {
> + if (sparse_banks_len > KVM_HV_HYPERCALL_MAX_XMM_REGISTERS - 1)
> + return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + for (i = 0, j = 1; i < sparse_banks_len; i += 2, j++) {
Nitpick: you don't really need 'j' here as 'j == i/2 + 1', right?
> + sparse_banks[i + 0] = sse128_lo(hc->xmm[j]);
Using ' + 0' for identation is ... unusual :-) I'm not opposed to it
here though.
> + sparse_banks[i + 1] = sse128_hi(hc->xmm[j]);
> + }
> + } else {
> + gpa = hc->ingpa;
> + gpa += offsetof(struct hv_tlb_flush_ex,
> + hv_vp_set.bank_contents);
Nitpick: if splitting these into two lines is only done to fit into 80
chars then I'd the requirement is no more so we can be a bit wider.
gpa = hc->ingpa + offsetof(...)
> + if (unlikely(kvm_read_guest(kvm, gpa, sparse_banks,
> + sparse_banks_len *
> + sizeof(sparse_banks[0]))))
> + return HV_STATUS_INVALID_HYPERCALL_INPUT;
> + }
> + }
> }
>
> cpumask_clear(&hv_vcpu->tlb_flush);
> @@ -1890,6 +1920,41 @@ static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *h
> return HV_STATUS_SUCCESS;
> }
>
> +static bool is_xmm_fast_hypercall(struct kvm_hv_hcall *hc)
> +{
> + switch (hc->code) {
> + case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST:
> + case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE:
> + case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX:
> + case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX:
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static inline void kvm_hv_hypercall_read_xmm(struct kvm_hv_hcall *hc)
> +{
> + int reg;
> +
> + kvm_fpu_get();
> + for (reg = 0; reg < KVM_HV_HYPERCALL_MAX_XMM_REGISTERS; reg++)
> + _kvm_read_sse_reg(reg, &hc->xmm[reg]);
> + kvm_fpu_put();
> + hc->xmm_dirty = false;
> +}
> +
> +static inline void kvm_hv_hypercall_write_xmm(struct kvm_hv_hcall *hc)
> +{
> + int reg;
> +
> + kvm_fpu_get();
> + for (reg = 0; reg < KVM_HV_HYPERCALL_MAX_XMM_REGISTERS; reg++)
> + _kvm_write_sse_reg(reg, &hc->xmm[reg]);
> + kvm_fpu_put();
> + hc->xmm_dirty = false;
> +}
> +
> int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> {
> struct kvm_hv_hcall hc;
> @@ -1926,6 +1991,9 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> hc.rep_idx = (hc.param >> HV_HYPERCALL_REP_START_OFFSET) & 0xfff;
> hc.rep = !!(hc.rep_cnt || hc.rep_idx);
>
> + if (hc.fast && is_xmm_fast_hypercall(&hc))
> + kvm_hv_hypercall_read_xmm(&hc);
> +
> trace_kvm_hv_hypercall(hc.code, hc.fast, hc.rep_cnt, hc.rep_idx,
> hc.ingpa, hc.outgpa);
>
> @@ -1961,28 +2029,28 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> kvm_hv_hypercall_complete_userspace;
> return 0;
> case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST:
> - if (unlikely(hc.fast || !hc.rep_cnt || hc.rep_idx)) {
> + if (unlikely(!hc.rep_cnt || hc.rep_idx)) {
> ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
> break;
> }
> ret = kvm_hv_flush_tlb(vcpu, &hc, false);
> break;
> case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE:
> - if (unlikely(hc.fast || hc.rep)) {
> + if (unlikely(hc.rep)) {
> ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
> break;
> }
> ret = kvm_hv_flush_tlb(vcpu, &hc, false);
> break;
> case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX:
> - if (unlikely(hc.fast || !hc.rep_cnt || hc.rep_idx)) {
> + if (unlikely(!hc.rep_cnt || hc.rep_idx)) {
> ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
> break;
> }
> ret = kvm_hv_flush_tlb(vcpu, &hc, true);
> break;
> case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX:
> - if (unlikely(hc.fast || hc.rep)) {
> + if (unlikely(hc.rep)) {
> ret = HV_STATUS_INVALID_HYPERCALL_INPUT;
> break;
> }
> @@ -2035,6 +2103,9 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu)
> break;
> }
>
> + if (hc.xmm_dirty)
> + kvm_hv_hypercall_write_xmm(&hc);
> +
Wei already mention that but as 'xmm_dirty' is not being used in this
patch I'd suggest we move it out too.
> return kvm_hv_hypercall_complete(vcpu, ret);
> }
--
Vitaly
Powered by blists - more mailing lists