Re: Question on dirty sync before kvm memslot removal

[Peter Xu]

On Thu, Apr 02, 2020 at 04:47:58PM -0400, Peter Xu wrote:
> 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);
> +       }
> +

Oops, this block should definitely be after the unlock as you
suggested...

>         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

-- 
Peter Xu