Re: [PATCH RFC 3/6] KVM: x86: interrupt based APF page-ready event delivery

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> 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)?
>

Sure.

>> +
>> +	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).

Yes,

as we need to write to two MSRs to configure the new mechanism ordering
becomes important. If the guest writes to ASYNC_PF_EN first to establish
the shared memory stucture the interrupt in ASYNC_PF2 is not yet set
(and AFAIR '0' is a valid interrupt!) so if an async pf happens
immediately after that we'll be forced to inject INT0 in the guest and
it'll get confused and linkely miss the event.

We can probably mandate the reverse sequence: guest has to set up
interrupt in ASYNC_PF2 first and then write to ASYNC_PF_EN (with both
bit 0 and bit 3). In that case the additional 'enable' bit in ASYNC_PF2
seems redundant. This protocol doesn't look too complex for guests to
follow.

>
> 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;

-- 
Vitaly