| 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: <20200429212708.GA40678@xz-x1>
Date: Wed, 29 Apr 2020 17:27:08 -0400
From: Peter Xu <peterx@...hat.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Andy Lutomirski <luto@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"H. Peter Anvin" <hpa@...or.com>,
Sean Christopherson <sean.j.christopherson@...el.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH RFC 3/6] KVM: x86: interrupt based APF page-ready event
delivery
Hi, Vitaly,
On Wed, Apr 29, 2020 at 11:36:31AM +0200, Vitaly Kuznetsov wrote:
> + Type 1 page (page missing) events are currently always delivered as
> + synthetic #PF exception. Type 2 (page ready) are either delivered
> + by #PF exception (when bit 3 of MSR_KVM_ASYNC_PF_EN is clear) or
> + via an APIC interrupt (when bit 3 set). APIC interrupt delivery is
> + controlled by MSR_KVM_ASYNC_PF2.
How about MSR_KVM_ASYNC_PF_INT instead of MSR_KVM_ASYNC_PF2 (to match
MSR_KVM_ASYNC_EN and MSR_KVM_ASYNC_ACK where they're all MSR_KVM_ASYNC_* with a
meaningful ending word)?
> +
> + For #PF delivery, disabling interrupt inhibits APFs. Guest must
> + not enable interrupt before the reason is read, or it may be
> + overwritten by another APF. Since APF uses the same exception
> + vector as regular page fault guest must reset the reason to 0
> + before it does something that can generate normal page fault.
> + If during pagefault APF reason is 0 it means that this is regular
> + page fault.
>
> During delivery of type 1 APF cr2 contains a token that will
> be used to notify a guest when missing page becomes
> @@ -319,3 +326,18 @@ data:
>
> KVM guests can request the host not to poll on HLT, for example if
> they are performing polling themselves.
> +
> +MSR_KVM_ASYNC_PF2:
> + 0x4b564d06
> +
> +data:
> + Second asynchronous page fault control MSR.
> +
> + Bits 0-7: APIC vector for interrupt based delivery of type 2 APF
> + events (page ready notification).
> + Bit 8: Interrupt based delivery of type 2 APF events is enabled
> + Bits 9-63: Reserved
(may need to fix up the indents)
> +
> + To switch to interrupt based delivery of type 2 APF events guests
> + are supposed to enable asynchronous page faults and set bit 3 in
> + MSR_KVM_ASYNC_PF_EN first.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 42a2d0d3984a..6215f61450cb 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -763,12 +763,15 @@ struct kvm_vcpu_arch {
> bool halted;
> gfn_t gfns[roundup_pow_of_two(ASYNC_PF_PER_VCPU)];
> struct gfn_to_hva_cache data;
> - u64 msr_val;
> + u64 msr_val; /* MSR_KVM_ASYNC_PF_EN */
> + u64 msr2_val; /* MSR_KVM_ASYNC_PF2 */
> + u16 vec;
> u32 id;
> bool send_user_only;
> u32 host_apf_reason;
> unsigned long nested_apf_token;
> bool delivery_as_pf_vmexit;
> + bool delivery_as_int;
> } apf;
>
> /* OSVW MSRs (AMD only) */
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
> index df2ba34037a2..1bbb0b7e062f 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -50,6 +50,7 @@
> #define MSR_KVM_STEAL_TIME 0x4b564d03
> #define MSR_KVM_PV_EOI_EN 0x4b564d04
> #define MSR_KVM_POLL_CONTROL 0x4b564d05
> +#define MSR_KVM_ASYNC_PF2 0x4b564d06
>
> struct kvm_steal_time {
> __u64 steal;
> @@ -81,6 +82,11 @@ struct kvm_clock_pairing {
> #define KVM_ASYNC_PF_ENABLED (1 << 0)
> #define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1)
> #define KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT (1 << 2)
> +#define KVM_ASYNC_PF_DELIVERY_AS_INT (1 << 3)
> +
> +#define KVM_ASYNC_PF2_VEC_MASK GENMASK(7, 0)
> +#define KVM_ASYNC_PF2_ENABLED BIT(8)
There are two enable bits, this one in ASYNC_PF_EN and the other old one in
ASYNC_PF2. Could it work with only one knob (e.g., set bit 0 of ASYNC_PF_EN
always to enable apf)? After all we have had bit 3 of ASYNC_PF_EN to show
whether interrupt is enabled, which seems to be the real switch for whether to
enable interrupt for apf.
If we can keep the only knob in ASYNC_PF_EN (bit 0), iiuc we can also keep the
below kvm_async_pf_wakeup_all() untouched (so we only set bit 0 of ASYNC_PF_EN
after configure everything).
Thanks,
> +
>
> /* Operations for KVM_HC_MMU_OP */
> #define KVM_MMU_OP_WRITE_PTE 1
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7c21c0cf0a33..861dce1e7cf5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1243,7 +1243,7 @@ static const u32 emulated_msrs_all[] = {
> HV_X64_MSR_TSC_EMULATION_STATUS,
>
> MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME,
> - MSR_KVM_PV_EOI_EN,
> + MSR_KVM_PV_EOI_EN, MSR_KVM_ASYNC_PF2,
>
> MSR_IA32_TSC_ADJUST,
> MSR_IA32_TSCDEADLINE,
> @@ -2649,8 +2649,8 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
> {
> gpa_t gpa = data & ~0x3f;
>
> - /* Bits 3:5 are reserved, Should be zero */
> - if (data & 0x38)
> + /* Bits 4:5 are reserved, Should be zero */
> + if (data & 0x30)
> return 1;
>
> vcpu->arch.apf.msr_val = data;
> @@ -2667,7 +2667,35 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>
> vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS);
> vcpu->arch.apf.delivery_as_pf_vmexit = data & KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT;
> - kvm_async_pf_wakeup_all(vcpu);
> + vcpu->arch.apf.delivery_as_int = data & KVM_ASYNC_PF_DELIVERY_AS_INT;
> +
> + /*
> + * If delivery via interrupt is configured make sure MSR_KVM_ASYNC_PF2
> + * was written to before sending 'wakeup all'.
> + */
> + if (!vcpu->arch.apf.delivery_as_int ||
> + vcpu->arch.apf.msr2_val & KVM_ASYNC_PF2_ENABLED)
> + kvm_async_pf_wakeup_all(vcpu);
> +
> + return 0;
> +}
> +
> +static int kvm_pv_enable_async_pf2(struct kvm_vcpu *vcpu, u64 data)
> +{
> + /* Bits 9-63 are reserved */
> + if (data & ~0x1ff)
> + return 1;
> +
> + if (!lapic_in_kernel(vcpu))
> + return 1;
> +
> + vcpu->arch.apf.msr2_val = data;
> +
> + vcpu->arch.apf.vec = data & KVM_ASYNC_PF2_VEC_MASK;
> +
> + if (data & KVM_ASYNC_PF2_ENABLED)
> + kvm_async_pf_wakeup_all(vcpu);
> +
> return 0;
> }
>
> @@ -2883,6 +2911,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> if (kvm_pv_enable_async_pf(vcpu, data))
> return 1;
> break;
> + case MSR_KVM_ASYNC_PF2:
> + if (kvm_pv_enable_async_pf2(vcpu, data))
> + return 1;
> + break;
> case MSR_KVM_STEAL_TIME:
>
> if (unlikely(!sched_info_on()))
> @@ -3159,6 +3191,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> case MSR_KVM_ASYNC_PF_EN:
> msr_info->data = vcpu->arch.apf.msr_val;
> break;
> + case MSR_KVM_ASYNC_PF2:
> + msr_info->data = vcpu->arch.apf.msr2_val;
> + break;
> case MSR_KVM_STEAL_TIME:
> msr_info->data = vcpu->arch.st.msr_val;
> break;
> @@ -10367,6 +10402,16 @@ static int apf_get_user(struct kvm_vcpu *vcpu, u32 *val)
> sizeof(u32));
> }
>
> +static bool apf_slot_free(struct kvm_vcpu *vcpu)
> +{
> + u32 val;
> +
> + if (apf_get_user(vcpu, &val))
> + return false;
> +
> + return !val;
> +}
> +
> static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
> {
> if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu))
> @@ -10382,11 +10427,23 @@ static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu)
>
> bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu)
> {
> + /*
> + * TODO: when we are injecting a 'page present' event with an interrupt
> + * we may ignore pending exceptions.
> + */
> if (unlikely(!lapic_in_kernel(vcpu) ||
> kvm_event_needs_reinjection(vcpu) ||
> vcpu->arch.exception.pending))
> return false;
>
> + /*'
> + * Regardless of the type of event we're trying to deliver, we need to
> + * check that the previous even was already consumed, this may not be
> + * the case with interrupt based delivery.
> + */
> + if (vcpu->arch.apf.delivery_as_int && !apf_slot_free(vcpu))
> + return false;
> +
> if (kvm_hlt_in_guest(vcpu->kvm) && !kvm_can_deliver_async_pf(vcpu))
> return false;
--
Peter Xu
Powered by blists - more mailing lists