From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PART1 RFC v4 10/11] svm: Do not intercept CR8 when enable AVIC
Date: Wed, 13 Apr 2016 00:26:28 +0200
Message-ID: <570D7614.6080203@redhat.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>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
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
To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
	Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20160412141820.GB6762@potion.brq.redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org



On 12/04/2016 16:18, Radim Kr=C4=8Dm=C3=A1=C5=99 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(struc=
t kvm_vcpu *vcpu)
>> >  static inline void sync_lapic_to_cr8(struct kvm_vcpu *vcpu)
>> >  {
>> >  	struct vcpu_svm *svm =3D to_svm(vcpu);
>> > -	u64 cr8;
>> > +	struct kvm_lapic *apic =3D vcpu->arch.apic;
>> > =20
>> > -	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.
>=20
> (Naming this condition would reduce the chance of errors.)
>=20

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