Re: [PART1 RFC v4 10/11] svm: Do not intercept CR8 when enable AVIC

[Paolo Bonzini <pbonzini@redhat.com>]

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