linux-kernel - Re: [PATCH 1/3] KVM: SVM: extract avic_ring

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YgaScaJD6fpk2xgJ@google.com>
Date:   Fri, 11 Feb 2022 16:44:33 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        mlevitsk@...hat.com
Subject: Re: [PATCH 1/3] KVM: SVM: extract avic_ring_doorbell

On Fri, Feb 11, 2022, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@...hat.com>
> 
> The check on the current CPU adds an extra level of indentation to
> svm_deliver_avic_intr and conflates documentation on what happens
> if the vCPU exits (of interest to svm_deliver_avic_intr) and migrates
> (only of interest to avic_ring_doorbell, which calls get/put_cpu()).
> Extract the wrmsr to a separate function and rewrite the
> comment in svm_deliver_avic_intr().
> 
> Co-developed-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>

Bad SoB chain, should be:

  Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
  Co-developed-by: Paolo Bonzini <pbonzini@...hat.com>
  Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>

Interestingly, git-apply drops the second, redundant SoB and yields

  Co-developed-by: Paolo Bonzini <pbonzini@...hat.com>
  Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
  Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>

Which will probably get you yelled at by Stephen's scripts :-)

A few nits below...

Reviewed-by: Sean Christopherson <seanjc@...gle.com>

> ---
>  arch/x86/kvm/svm/avic.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3f9b48732aea..4d1baf5c8f6a 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -269,6 +269,24 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +

Spurious newline.

> +static void avic_ring_doorbell(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * Note, the vCPU could get migrated to a different pCPU at any
> +	 * point, which could result in signalling the wrong/previous
> +	 * pCPU.  But if that happens the vCPU is guaranteed to do a
> +	 * VMRUN (after being migrated) and thus will process pending
> +	 * interrupts, i.e. a doorbell is not needed (and the spurious
> +	 * one is harmless).

Please run these out to 80 chars, it saves a whole line!

	/*
	 * Note, the vCPU could get migrated to a different pCPU at any point,
	 * which could result in signalling the wrong/previous pCPU.  But if
	 * that happens the vCPU is guaranteed to do a VMRUN (after being
	 * migrated) and thus will process pending interrupts, i.e. a doorbell
	 * is not needed (and the spurious one is harmless).
	 */

> +	 */
> +	int cpu = READ_ONCE(vcpu->cpu);
> +
> +	if (cpu != get_cpu())
> +		wrmsrl(MSR_AMD64_SVM_AVIC_DOORBELL, kvm_cpu_get_apicid(cpu));
> +	put_cpu();
> +}
> +
>  static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
>  				   u32 icrl, u32 icrh)
>  {