Re: [PATCH 1/5] KVM: change API for requests to match bit operations

[Cornelia Huck]

On Thu, 16 Feb 2017 17:04:45 +0100
Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx> wrote:

> kvm_make_request was a wrapper that added barriers to bit_set and
> kvm_check_request did the same for bit_test and bit_check, but the name
> was not very obvious and we were also lacking operations that cover
> bit_test and bit_clear, which resulted in an inconsistent use.
> 
> The renaming:
>   kvm_request_set            <- kvm_make_request
>   kvm_request_test_and_clear <- kvm_check_request
> 
> Automated with coccinelle script:
>   @@
>   expression VCPU, REQ;
>   @@
>   -kvm_make_request(REQ, VCPU)
>   +kvm_request_set(REQ, VCPU)
> 
>   @@
>   expression VCPU, REQ;
>   @@
>   -kvm_check_request(REQ, VCPU)
>   +kvm_request_test_and_clear(REQ, VCPU)

Forgot your s-o-b?

> ---
>  arch/mips/kvm/emulate.c      |   2 +-
>  arch/mips/kvm/trap_emul.c    |   2 +-
>  arch/powerpc/kvm/book3s_pr.c |   2 +-
>  arch/powerpc/kvm/booke.c     |  16 +++---
>  arch/powerpc/kvm/powerpc.c   |   2 +-
>  arch/s390/kvm/kvm-s390.c     |  22 ++++----
>  arch/s390/kvm/kvm-s390.h     |   4 +-
>  arch/s390/kvm/priv.c         |   4 +-
>  arch/x86/kvm/hyperv.c        |  14 ++---
>  arch/x86/kvm/i8259.c         |   2 +-
>  arch/x86/kvm/lapic.c         |  22 ++++----
>  arch/x86/kvm/mmu.c           |  14 ++---
>  arch/x86/kvm/pmu.c           |   6 +-
>  arch/x86/kvm/svm.c           |  12 ++--
>  arch/x86/kvm/vmx.c           |  30 +++++-----
>  arch/x86/kvm/x86.c           | 128 +++++++++++++++++++++----------------------
>  include/linux/kvm_host.h     |  30 ++++++++--
>  virt/kvm/kvm_main.c          |   4 +-
>  18 files changed, 167 insertions(+), 149 deletions(-)

(...lots of coccinelle changes...)

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 8d69d5150748..21f91de3098b 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1084,24 +1084,42 @@ static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
>  
>  #endif /* CONFIG_HAVE_KVM_EVENTFD */
>  
> -static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> +/*
> + * An API for setting KVM requests.
> + * The general API design is inspired by bit_* API.
> + *
> + * A request can be set either to itself or to a remote VCPU.  If the request
> + * is set to a remote VCPU, then the VCPU needs to be notified, which is
> + * usually done with kvm_vcpu_kick().
> + * The request can also mean that some data is ready, so a remote requests
> + * needs a smp_wmb().  i.e. there are three types of requests:
> + *  1) local request
> + *  2) remote request with no data (= kick)
> + *  3) remote request with data (= kick + mb)
> + *
> + * TODO: the API is inconsistent -- a request doesn't call kvm_vcpu_kick(), but
> + * forces smp_wmb() for all requests.
> + */
> +static inline void kvm_request_set(unsigned req, struct kvm_vcpu *vcpu)

Should we make req unsigned long as well, so that it matches the bit
api even more?

>  {
>  	/*
> -	 * Ensure the rest of the request is published to kvm_check_request's
> -	 * caller.  Paired with the smp_mb__after_atomic in kvm_check_request.
> +	 * Ensure the rest of the request is published to
> +	 * kvm_request_test_and_clear's caller.
> +	 * Paired with the smp_mb__after_atomic in kvm_request_test_and_clear.
>  	 */
>  	smp_wmb();
>  	set_bit(req, &vcpu->requests);
>  }
>  
> -static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
> +static inline bool kvm_request_test_and_clear(unsigned req, struct kvm_vcpu *vcpu)
>  {
>  	if (test_bit(req, &vcpu->requests)) {
>  		clear_bit(req, &vcpu->requests);
>  
>  		/*
> -		 * Ensure the rest of the request is visible to kvm_check_request's
> -		 * caller.  Paired with the smp_wmb in kvm_make_request.
> +		 * Ensure the rest of the request is visible to
> +		 * kvm_request_test_and_clear's caller.
> +		 * Paired with the smp_wmb in kvm_request_set.
>  		 */
>  		smp_mb__after_atomic();
>  		return true;