Re: [RFC PATCH 2/3] KVM: Avoid re-reading kvm->max_halt_poll_ns during halt-polling

[Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>]

Am 17.11.22 um 01:16 schrieb David Matlack:
> Avoid re-reading kvm->max_halt_poll_ns multiple times during
> halt-polling except when it is explicitly useful, e.g. to check if the
> max time changed across a halt. kvm->max_halt_poll_ns can be changed at
> any time by userspace via KVM_CAP_HALT_POLL.
>
> This bug is unlikely to cause any serious side-effects. In the worst
> case one halt polls for shorter or longer than it should, and then is
> fixed up on the next halt. Furthmore, this is still possible since
> kvm->max_halt_poll_ns are not synchronized with halts.
>
> Fixes: acd05785e48c ("kvm: add capability for halt polling")
> Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>

Acked-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
> ---
>   virt/kvm/kvm_main.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4b868f33c45d..78caf19608eb 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3488,6 +3488,11 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start,
>   	}
>   }
>   
> +static unsigned int kvm_vcpu_max_halt_poll_ns(struct kvm_vcpu *vcpu)
> +{
> +	return READ_ONCE(vcpu->kvm->max_halt_poll_ns);
> +}
> +
>   /*
>    * Emulate a vCPU halt condition, e.g. HLT on x86, WFI on arm, etc...  If halt
>    * polling is enabled, busy wait for a short time before blocking to avoid the
> @@ -3496,14 +3501,15 @@ static inline void update_halt_poll_stats(struct kvm_vcpu *vcpu, ktime_t start,
>    */
>   void kvm_vcpu_halt(struct kvm_vcpu *vcpu)
>   {
> +	unsigned int max_halt_poll_ns = kvm_vcpu_max_halt_poll_ns(vcpu);
>   	bool halt_poll_allowed = !kvm_arch_no_poll(vcpu);
>   	ktime_t start, cur, poll_end;
>   	bool waited = false;
>   	bool do_halt_poll;
>   	u64 halt_ns;
>   
> -	if (vcpu->halt_poll_ns > vcpu->kvm->max_halt_poll_ns)
> -		vcpu->halt_poll_ns = vcpu->kvm->max_halt_poll_ns;
> +	if (vcpu->halt_poll_ns > max_halt_poll_ns)
> +		vcpu->halt_poll_ns = max_halt_poll_ns;
>   
>   	do_halt_poll = halt_poll_allowed && vcpu->halt_poll_ns;
>   
> @@ -3545,18 +3551,21 @@ void kvm_vcpu_halt(struct kvm_vcpu *vcpu)
>   		update_halt_poll_stats(vcpu, start, poll_end, !waited);
>   
>   	if (halt_poll_allowed) {
> +		/* Recompute the max halt poll time in case it changed. */
> +		max_halt_poll_ns = kvm_vcpu_max_halt_poll_ns(vcpu);
> +
>   		if (!vcpu_valid_wakeup(vcpu)) {
>   			shrink_halt_poll_ns(vcpu);
> -		} else if (vcpu->kvm->max_halt_poll_ns) {
> +		} else if (max_halt_poll_ns) {
>   			if (halt_ns <= vcpu->halt_poll_ns)
>   				;
>   			/* we had a long block, shrink polling */
>   			else if (vcpu->halt_poll_ns &&
> -				 halt_ns > vcpu->kvm->max_halt_poll_ns)
> +				 halt_ns > max_halt_poll_ns)
>   				shrink_halt_poll_ns(vcpu);
>   			/* we had a short halt and our poll time is too small */
> -			else if (vcpu->halt_poll_ns < vcpu->kvm->max_halt_poll_ns &&
> -				 halt_ns < vcpu->kvm->max_halt_poll_ns)
> +			else if (vcpu->halt_poll_ns < max_halt_poll_ns &&
> +				 halt_ns < max_halt_poll_ns)
>   				grow_halt_poll_ns(vcpu);
>   		} else {
>   			vcpu->halt_poll_ns = 0;