Re: [PATCH 1/5] KVM: x86: Get vmcs12 pages before checking pending interrupts

[Mingwei Zhang <mizhang@google.com>]

On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> On Tue, 2022-08-02 at 23:07 +0000, Mingwei Zhang wrote:
> > From: Oliver Upton <oupton@google.com>
> > 
> > vmx_guest_apic_has_interrupts implicitly depends on the virtual APIC
> > page being present + mapped into the kernel address space. However, with
> > demand paging we break this dependency, as the KVM_REQ_GET_VMCS12_PAGES
> > event isn't assessed before entering vcpu_block.
> > 
> > Fix this by getting vmcs12 pages before inspecting the guest's APIC
> > page. Note that upstream does not have this issue, as they will directly
> > get the vmcs12 pages on vmlaunch/vmresume instead of relying on the
> > event request mechanism. However, the upstream approach is problematic,
> > as the vmcs12 pages will not be present if a live migration occurred
> > before checking the virtual APIC page.
> 
> Since this patch is intended for upstream, I don't fully understand
> the meaning of the above paragraph.

My apology. Some of the statement needs to be updated, which I should do
before sending. But I think the point here is that there is a missing
get_nested_state_pages() call here within vcpu_block() when there is the
request of KVM_REQ_GET_NESTED_STATE_PAGES.

> 
> 
> > 
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > Signed-off-by: Mingwei Zhang <mizhang@google.com>
> > ---
> >  arch/x86/kvm/x86.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 5366f884e9a7..1d3d8127aaea 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10599,6 +10599,23 @@ static inline int vcpu_block(struct kvm_vcpu *vcpu)
> >  {
> >  	bool hv_timer;
> >  
> > +	/*
> > +	 * We must first get the vmcs12 pages before checking for interrupts
> > +	 * that might unblock the guest if L1 is using virtual-interrupt
> > +	 * delivery.
> > +	 */
> > +	if (kvm_check_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu)) {
> > +		/*
> > +		 * If we have to ask user-space to post-copy a page,
> > +		 * then we have to keep trying to get all of the
> > +		 * VMCS12 pages until we succeed.
> > +		 */
> > +		if (unlikely(!kvm_x86_ops.nested_ops->get_nested_state_pages(vcpu))) {
> > +			kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
> > +			return 0;
> > +		}
> > +	}
> > +
> >  	if (!kvm_arch_vcpu_runnable(vcpu)) {
> >  		/*
> >  		 * Switch to the software timer before halt-polling/blocking as
> 
> 
> If I understand correctly, you are saying that if apic backing page is migrated in post copy
> then 'get_nested_state_pages' will return false and thus fail?

What I mean is that when the vCPU was halted and then migrated in this
case, KVM did not call get_nested_state_pages() before getting into
kvm_arch_vcpu_runnable(). This function checks the apic backing page and
fails on that check and triggered the warning.
> 
> AFAIK both SVM and VMX versions of 'get_nested_state_pages' assume that this is not the case
> for many things like MSR bitmaps and such - they always uses non atomic versions
> of guest memory access like 'kvm_vcpu_read_guest' and 'kvm_vcpu_map' which
> supposed to block if they attempt to access HVA which is not present, and then
> userfaultd should take over and wake them up.

You are right here.
> 
> If that still fails, nested VM entry is usually failed, and/or the whole VM
> is crashed with 'KVM_EXIT_INTERNAL_ERROR'.
> 
> Anything I missed? 
> 
> Best regards,
> 	Maxim Levitsky
>