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

[Mingwei Zhang <mizhang@google.com>]

On Wed, Aug 03, 2022, Mingwei Zhang wrote:
> 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'.
> > 

Ah, I think I understand what you are saying. hmm, so basically the
patch here is to continuously request vmcs12 pages if failed. But what
you are saying is that we just need to call 'get_nested_state_pages'
once. If it fails, then the VM fails to work. Let me double check and
get back.

> > Anything I missed? 
> > 
> > Best regards,
> > 	Maxim Levitsky
> >