Re: Question on dirty sync before kvm memslot removal

[Peter Xu]

On Wed, Apr 01, 2020 at 07:09:28PM -0400, Peter Xu wrote:
> On Wed, Apr 01, 2020 at 01:12:04AM +0200, Paolo Bonzini wrote:
> > On 31/03/20 18:51, Peter Xu wrote:
> > > On Tue, Mar 31, 2020 at 05:34:43PM +0200, Paolo Bonzini wrote:
> > >> On 31/03/20 17:23, Peter Xu wrote:
> > >>>> Or KVM_MEM_READONLY.
> > >>> Yeah, I used a new flag because I thought READONLY was a bit tricky to
> > >>> be used directly here.  The thing is IIUC if guest writes to a
> > >>> READONLY slot then KVM should either ignore the write or trigger an
> > >>> error which I didn't check, however here what we want to do is to let
> > >>> the write to fallback to the userspace so it's neither dropped (we
> > >>> still want the written data to land gracefully on RAM), nor triggering
> > >>> an error (because the slot is actually writable).
> > >>
> > >> No, writes fall back to userspace with KVM_MEM_READONLY.
> > > 
> > > I read that __kvm_write_guest_page() will return -EFAULT when writting
> > > to the read-only memslot, and e.g. kvm_write_guest_virt_helper() will
> > > return with X86EMUL_IO_NEEDED, which will be translated into a
> > > EMULATION_OK in x86_emulate_insn().  Then in x86_emulate_instruction()
> > > it seems to get a "1" returned (note that I think it does not set
> > > either vcpu->arch.pio.count or vcpu->mmio_needed).  Does that mean
> > > it'll retry the write forever instead of quit into the userspace?  I
> > > may possibly have misread somewhere, though..
> > 
> > We are definitely relying on KVM_MEM_READONLY to exit to userspace, in
> > order to emulate flash memory.
> > 
> > > However... I think I might find another race with this:
> > > 
> > >           main thread                       vcpu thread
> > >           -----------                       -----------
> > >                                             dirty GFN1, cached in PML
> > >                                             ...
> > >           remove memslot1 of GFN1
> > >             set slot READONLY (whatever, or INVALID)
> > >             sync log (NOTE: no GFN1 yet)
> > >                                             vmexit, flush PML with RCU
> > >                                             (will flush to old bitmap) 
> > > <------- [1]
> > >             delete memslot1 (old bitmap freed)                         
> > > <------- [2]
> > >           add memslot2 of GFN1 (memslot2 could be smaller)
> > >             add memslot2
> > > 
> > > I'm not 100% sure, but I think GFN1's dirty bit will be lost though
> > > it's correctly applied at [1] but quickly freed at [2].
> > 
> > Yes, we probably need to do a mass vCPU kick when a slot is made
> > READONLY, before KVM_SET_USER_MEMORY_REGION returns (and after releasing
> > slots_lock).  It makes sense to guarantee that you can't get any more
> > dirtying after KVM_SET_USER_MEMORY_REGION returns.
> 
> Sounds doable.  Though we still need a synchronous way to kick vcpus
> in KVM to make sure the PML is flushed before KVM_SET_MEMORY_REGION
> returns, am I right?  Is there an existing good way to do this?

Paolo,

I'm not sure whether it's anything close to acceptable, but this is
something I was thinking about below (pesudo code).  Do you think it
makes any sense?  Thanks,

8<-------------------------------------------
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1b6d9ac9533c..437d669dde42 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8161,6 +8161,22 @@ void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(__kvm_request_immediate_exit);

+void kvm_vcpu_sync(struct kvm_vcpu *vcpu)
+{
+       DECLARE_WAITQUEUE(wait, current);
+
+       add_wait_queue(&vcpu->sync_wq, &wait);
+       set_current_state(TASK_UNINTERRUPTIBLE);
+       kvm_make_request(KVM_REQ_SYNC_VCPU, vcpu);
+       schedule();
+       remove_wait_queue(&vcpu->sync_wq, &wait);
+}
+
+void kvm_vcpu_sync_ack(struct kvm_vcpu *vcpu)
+{
+       wake_up(&vcpu->sync_wq);
+}
+
 /*
  * Returns 1 to let vcpu_run() continue the guest execution loop without
  * exiting to the userspace.  Otherwise, the value will be returned to the
@@ -8274,6 +8290,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
                        kvm_hv_process_stimers(vcpu);
                if (kvm_check_request(KVM_REQ_APICV_UPDATE, vcpu))
                        kvm_vcpu_update_apicv(vcpu);
+               if (kvm_check_request(KVM_REQ_SYNC_VCPU, vcpu))
+                       kvm_vcpu_sync_ack(vcpu);
        }

        if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f6a1905da9bf..e825d2e0a221 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -146,6 +146,7 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MMU_RELOAD        (1 | KVM_REQUEST_WAIT | 
KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_PENDING_TIMER     2
 #define KVM_REQ_UNHALT            3
+#define KVM_REQ_SYNC_VCPU         4
 #define KVM_REQUEST_ARCH_BASE     8

 #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \
@@ -278,6 +279,7 @@ struct kvm_vcpu {
        struct kvm_run *run;

        struct swait_queue_head wq;
+       struct wait_queue_head sync_wq;
        struct pid __rcu *pid;
        int sigset_active;
        sigset_t sigset;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f744bc603c53..35216aeb0365 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -342,6 +342,7 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm 
*kvm, unsigned id)
        vcpu->vcpu_id = id;
        vcpu->pid = NULL;
        init_swait_queue_head(&vcpu->wq);
+       init_wait_queue_head(&vcpu->sync_wq);
        kvm_async_pf_vcpu_init(vcpu);

        vcpu->pre_pcpu = -1;
@@ -1316,9 +1317,20 @@ int kvm_set_memory_region(struct kvm *kvm,
                          const struct kvm_userspace_memory_region *mem)
 {
        int r;
+       unsigned int i;
+       struct kvm_vcpu *vcpu;

        mutex_lock(&kvm->slots_lock);
+
        r = __kvm_set_memory_region(kvm, mem);
+
+       /* TBD: use arch-specific hooks; this won't compile on non-x86 */
+       if ((mem->flags & KVM_MEM_LOG_DIRTY_PAGES) &&
+           (mem->flags & KVM_MEM_READONLY)) {
+               kvm_for_each_vcpu(i, vcpu, kvm)
+                   kvm_vcpu_sync(vcpu);
+       }
+
        mutex_unlock(&kvm->slots_lock);
        return r;
 }
@@ -2658,6 +2670,8 @@ static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
                goto out;
        if (signal_pending(current))
                goto out;
+       if (kvm_check_request(KVM_REQ_SYNC_VCPU, vcpu))
+               goto out;

        ret = 0;
 out:
8<-------------------------------------------

-- 
Peter Xu