Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S964985AbcDLW0k (ORCPT <rfc822;w@1wt.eu>);
	Tue, 12 Apr 2016 18:26:40 -0400
Received: from mx1.redhat.com ([209.132.183.28]:43523 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1756565AbcDLW0j (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 12 Apr 2016 18:26:39 -0400
Subject: Re: [PART1 RFC v4 10/11] svm: Do not intercept CR8 when enable AVIC
To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
References: <1460017232-17429-1-git-send-email-Suravee.Suthikulpanit@amd.com>
 <1460017232-17429-11-git-send-email-Suravee.Suthikulpanit@amd.com>
 <20160412141820.GB6762@potion.brq.redhat.com>
Cc: joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com,
        sherry.hurwitz@amd.com
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <570D7614.6080203@redhat.com>
Date: Wed, 13 Apr 2016 00:26:28 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.1
MIME-Version: 1.0
In-Reply-To: <20160412141820.GB6762@potion.brq.redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1409
Lines: 42



On 12/04/2016 16:18, Radim Krčmář wrote:
>> > @@ -4069,7 +4070,8 @@ static void update_cr8_intercept(struct kvm_vcpu *vcpu, int tpr, int irr)
>> > -	if (is_guest_mode(vcpu) && (vcpu->arch.hflags & HF_VINTR_MASK))
>> > +	if ((is_guest_mode(vcpu) && (vcpu->arch.hflags & HF_VINTR_MASK)) ||
>> > +	     svm_vcpu_avic_enabled(svm))
>> > @@ -4255,14 +4257,15 @@ static inline void sync_cr8_to_lapic(struct kvm_vcpu *vcpu)
>> >  static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
>> >  {
>> >  	struct vcpu_svm *svm = to_svm(vcpu);
>> > -	u64 cr8;
>> > +	struct kvm_lapic *apic = vcpu->arch.apic;
>> >  
>> > -	if (is_guest_mode(vcpu) && (vcpu->arch.hflags & HF_VINTR_MASK))
>> > +	if (is_guest_mode(vcpu) && (vcpu->arch.hflags & HF_VINTR_MASK) &&
> Should be "||" at the end of line, like above.
> 
> (Naming this condition would reduce the chance of errors.)
> 

I think it's just "is_guest_mode(vcpu) && (vcpu->arch.hflags &
HF_VINTR_MASK)" that should become a static inline.  It is used also in
update_cr8_intercept.  Then something like

	if (svm_in_nested_interrupt_shadow(vcpu) &&
	    svm_vcpu_avic_enabled(svm))
		return;

makes little sense and stands out much better.

In fact, because nested SVM and AVIC have nothing to do with each other,
it's even better to write it like

	if (svm_in_nested_interrupt_shadow(vcpu))
		return;
	if (svm_vcpu_avic_enabled(svm))
		return;

Thanks,

Paolo