RE: [PATCH v19 for-4.14 01/13] x86/mem_sharing: block interrupt injection for forks

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Tian, Kevin
> Sent: Wednesday, June 10, 2020 7:44 AM
> 
> > From: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>
> > Sent: Monday, June 1, 2020 9:22 PM
> >
> > When running VM forks without device models (QEMU), it may
> > be undesirable for Xen to inject interrupts. When creating such forks from
> > Windows VMs we have observed the kernel trying to process interrupts
> > immediately after the fork is executed. However without QEMU running
> such
> > interrupt handling may not be possible because it may attempt to interact
> > with
> > devices that are not emulated by a backend. In the best case scenario such
> 
> I asked this question before. the interrupts could come from Xen itself, e.g.
> due to timer virtualization. So I didn't get why it's desired to block all
> interrupts
> just because no QEMU is running. Also it's weird why Windows VMs are
> observed to process interrupts that are generated by QEMU when no such
> backend emulation exists at all. It sounds like a workaround instead of a real
> fix...

ok, I rechecked your reply. Looks it's about the case that parent VM has QEMU
and pending interrupts while you fork it into child VMs without QEMU so those
pending interrupts become problematic.

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> 
> > interrupt handling would only present a detour in the VM forks' execution
> > flow, but in the worst case as we actually observed can completely stall it.
> > By disabling interrupt injection a fuzzer can exercise the target code
> without
> > interference. For other use-cases this option probably doesn't make sense,
> > that's why this is not enabled by default.
> >
> > Forks & memory sharing are only available on Intel CPUs so this only
> applies
> > to vmx. Note that this is part of the experimental VM forking feature that's
> > completely disabled by default and can only be enabled by using
> > XEN_CONFIG_EXPERT during compile time.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/vmx/intr.c      | 6 ++++++
> >  xen/arch/x86/mm/mem_sharing.c    | 6 +++++-
> >  xen/include/asm-x86/hvm/domain.h | 2 +-
> >  xen/include/public/memory.h      | 3 +++
> >  4 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> > index 000e14af49..80bfbb4787 100644
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -256,6 +256,12 @@ void vmx_intr_assist(void)
> >      if ( unlikely(v->arch.vm_event) && v->arch.vm_event->sync_event )
> >          return;
> >
> > +#ifdef CONFIG_MEM_SHARING
> > +    /* Block event injection for VM fork if requested */
> > +    if ( unlikely(v->domain->arch.hvm.mem_sharing.block_interrupts) )
> > +        return;
> > +#endif
> > +
> >      /* Crank the handle on interrupt state. */
> >      pt_vector = pt_update_irq(v);
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c
> > b/xen/arch/x86/mm/mem_sharing.c
> > index 19922ab5d1..c428fd16ce 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -2106,7 +2106,8 @@ int
> >
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op
> > _t) arg)
> >          rc = -EINVAL;
> >          if ( mso.u.fork.pad )
> >              goto out;
> > -        if ( mso.u.fork.flags & ~XENMEM_FORK_WITH_IOMMU_ALLOWED )
> > +        if ( mso.u.fork.flags &
> > +             ~(XENMEM_FORK_WITH_IOMMU_ALLOWED |
> > XENMEM_FORK_BLOCK_INTERRUPTS) )
> >              goto out;
> >
> >          rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
> > @@ -2134,6 +2135,9 @@ int
> >
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op
> > _t) arg)
> >              rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
> >                                                 "lh", XENMEM_sharing_op,
> >                                                 arg);
> > +        else if ( !rc && (mso.u.fork.flags &
> > XENMEM_FORK_BLOCK_INTERRUPTS) )
> > +            d->arch.hvm.mem_sharing.block_interrupts = true;
> > +
> >          rcu_unlock_domain(pd);
> >          break;
> >      }
> > diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> > x86/hvm/domain.h
> > index 95fe18cddc..9d247baf4d 100644
> > --- a/xen/include/asm-x86/hvm/domain.h
> > +++ b/xen/include/asm-x86/hvm/domain.h
> > @@ -67,7 +67,7 @@ struct hvm_ioreq_server {
> >  #ifdef CONFIG_MEM_SHARING
> >  struct mem_sharing_domain
> >  {
> > -    bool enabled;
> > +    bool enabled, block_interrupts;
> >
> >      /*
> >       * When releasing shared gfn's in a preemptible manner, recall where
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index dbd35305df..850bd72c52 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -536,7 +536,10 @@ struct xen_mem_sharing_op {
> >          } debug;
> >          struct mem_sharing_op_fork {      /* OP_FORK */
> >              domid_t parent_domain;        /* IN: parent's domain id */
> > +/* Only makes sense for short-lived forks */
> >  #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> > +/* Only makes sense for short-lived forks */
> > +#define XENMEM_FORK_BLOCK_INTERRUPTS   (1u << 1)
> >              uint16_t flags;               /* IN: optional settings */
> >              uint32_t pad;                 /* Must be set to 0 */
> >          } fork;
> > --
> > 2.25.1