Re: [PATCH] KVM: nVMX: Do not recalc IOAPIC handled vectors while running L2

[Liran Alon <LIRAN.ALON@ORACLE.COM>]

On 10/11/17 10:37, Paolo Bonzini wrote:
> On 10/11/2017 02:24, Liran Alon wrote:
>>
>> @@ -6720,6 +6720,12 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>>   	if (!kvm_apic_hw_enabled(vcpu->arch.apic))
>>   		return;
>>
>> +	if (is_guest_mode(vcpu)) {
>> +		vcpu->arch.scan_ioapic_pending = true;
>> +		return;
>> +	}
>> +	vcpu->arch.scan_ioapic_pending = false;
>> +
>>   	bitmap_zero(vcpu->arch.ioapic_handled_vectors, 256);
>>
>>   	if (irqchip_split(vcpu->kvm))
>
> I am not sure it is correct to exit immediately.  The bug is that you're
> losing the EOI exit bitmap in the vmcs01; however, with your patch
> you're losing it in the vmcs02.  If the L1 hypervisor passes the local
> APIC registers to the L2 guest, you would have a similar bug.

I have indeed not thought about this case. However, I think it is also 
currently not supported.

Assume that at some point of time L1 IOAPIC redirection-table is already 
fully-configured and is not going to be changed by L1 anymore. Further 
assume that L1 wants to run L2 and give him full access to L1 LAPIC. In 
this case, vmcs12->eoi_exit_bitmap will be empty (all zeros) as L1 
assumes EOIs to L1 LAPIC are always broadcasted to L1 IOAPIC (as happens 
in real hardware). When L1 will exec VMRESUME, prepare_vmcs02() will 
just make vmcs02->eoi_exit_bitmap a copy of vmcs12->eoi_exit_bitmap and 
therefore empty as-well. Therefore, L2 EOIs will not cause EOI_INDUCED 
exits to L0 and therefore will not reach L1 IOAPIC emulation code at L0.

In order to fully fix the issue at hand here, you will basically need 
vmcs02->eoi_exit_bitmap to be a logical OR of vmcs12->eoi_exit_bitmap 
and vmcs01->eoi_exit_bitmap. Then on EOI_INDUCED exits to L0, 
nested_vmx_exit_reflected() will decide if it should be forwarded to L1 
or not. That will also fix the bug this patch aims to fix.
However, I am a bit worry about this approach as it can cause a 
significant performance overhead for the common case of not pass-through 
L1 LAPIC to L2 for supporting this esoteric case.
We will try to think if we can come up with a better approach to solving 
this without such a performance hit.

However, I do think that because this scenario is currently not 
supported by KVM, we should consider first apply the v2 of this patch 
(Because the other small issue you found) and only then try to find a 
good-enough fix for the case you just mentioned here.

Am I missing something here or does it make sense?

>
> However, the patch is generally in the right direction.  Another small
> issue:
>
>> @@ -6833,7 +6839,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   				goto out;
>>   			}
>>   		}
>> -		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
>> +		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu) ||
>> +		    (!is_guest_mode(vcpu) && vcpu->arch.scan_ioapic_pending))
>>   			vcpu_scan_ioapic(vcpu);
>
> I think this is not reliable because kvm_request_pending(vcpu) might be
> false here---and then you never reach the "if" in the first place.
>
> Maybe you can add a
>
> 	if (vcpu->arch.scan_ioapic_pending)
> 		kvm_make_request(KVM_REQ_SCAN_IOAPIC, vcpu);
>
> to leave_guest_mode in arch/x86/kvm/kvm_cache_regs.h?
>

Agree. Sorry I missed that. Our unit-test didn't caught this case :).
I will fix this in v2 of this patch and re-send.
Thanks for pointing this out.

-Liran

> Thanks,
>
> Paolo
>
>>   		if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu))
>>   			kvm_vcpu_reload_apic_access_page(vcpu);
>> @@ -7981,6 +7988,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>>   	kvm = vcpu->kvm;
>>
>>   	vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv(vcpu);
>> +	vcpu->arch.scan_ioapic_pending = false;
>>   	vcpu->arch.pv.pv_unhalted = false;
>>   	vcpu->arch.emulate_ctxt.ops = &emulate_ops;
>>   	if (!irqchip_in_kernel(kvm) || kvm_vcpu_is_reset_bsp(vcpu))
>>
>