|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 2/2] x86/monitor: Add new monitor event to catch all vmexits
On Tue, Apr 26, 2022 at 4:50 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Mon, Apr 25, 2022 at 11:40:11AM -0400, Tamas K Lengyel wrote:
> > On Mon, Apr 25, 2022 at 10:41 AM Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > wrote:
> > >
> > > On Wed, Apr 13, 2022 at 09:41:52AM -0400, Tamas K Lengyel wrote:
> > > > Add monitor event that hooks the vmexit handler allowing for both sync
> > > > and
> > > > async monitoring of events. With async monitoring an event is placed on
> > > > the
> > > > monitor ring for each exit and the rest of the vmexit handler resumes
> > > > normally.
> > > > If there are additional monitor events configured those will also place
> > > > their
> > > > respective events on the monitor ring.
> > > >
> > > > With the sync version an event is placed on the monitor ring but the
> > > > handler
> > > > does not get resumed, thus the sync version is only useful when the VM
> > > > is not
> > > > expected to resume normally after the vmexit. Our use-case is primarily
> > > > with
> > > > the sync version with VM forks where the fork gets reset after sync
> > > > vmexit
> > > > event, thus the rest of the vmexit handler can be safely skipped. This
> > > > is
> > > > very useful when we want to avoid Xen crashing the VM under any
> > > > circumstance,
> > > > for example during fuzzing. Collecting all vmexit information
> > > > regardless of
> > > > the root cause makes it easier to reason about the state of the VM on
> > > > the
> > > > monitor side, hence we opt to receive all events, even for external
> > > > interrupt
> > > > and NMI exits and let the monitor agent decide how to proceed.
> > > >
> > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > > > ---
> > > > v4: Minor tweaks and more verbose patch description.
> > > >
> > > > Note: making the sync version resume-friendly is specifically
> > > > out-of-scope as
> > > > it would require significant rearrangement of the vmexit handler. As
> > > > this
> > > > feature is not required for our use-case we opt for the version that
> > > > minimizes
> > > > impact on the existing code.
> > > > ---
> > > > tools/include/xenctrl.h | 2 ++
> > > > tools/libs/ctrl/xc_monitor.c | 15 +++++++++++++++
> > > > xen/arch/x86/hvm/monitor.c | 18 ++++++++++++++++++
> > > > xen/arch/x86/hvm/vmx/vmx.c | 12 ++++++++++++
> > > > xen/arch/x86/include/asm/domain.h | 2 ++
> > > > xen/arch/x86/include/asm/hvm/monitor.h | 2 ++
> > > > xen/arch/x86/include/asm/monitor.h | 3 ++-
> > > > xen/arch/x86/monitor.c | 14 ++++++++++++++
> > > > xen/include/public/domctl.h | 6 ++++++
> > > > xen/include/public/vm_event.h | 8 ++++++++
> > > > 10 files changed, 81 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > > > index 1b089a2c02..159eaac050 100644
> > > > --- a/tools/include/xenctrl.h
> > > > +++ b/tools/include/xenctrl.h
> > > > @@ -2096,6 +2096,8 @@ int xc_monitor_privileged_call(xc_interface *xch,
> > > > uint32_t domain_id,
> > > > bool enable);
> > > > int xc_monitor_emul_unimplemented(xc_interface *xch, uint32_t
> > > > domain_id,
> > > > bool enable);
> > > > +int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool
> > > > enable,
> > > > + bool sync);
> > > > /**
> > > > * This function enables / disables emulation for each REP for a
> > > > * REP-compatible instruction.
> > > > diff --git a/tools/libs/ctrl/xc_monitor.c b/tools/libs/ctrl/xc_monitor.c
> > > > index 4ac823e775..c5fa62ff30 100644
> > > > --- a/tools/libs/ctrl/xc_monitor.c
> > > > +++ b/tools/libs/ctrl/xc_monitor.c
> > > > @@ -246,6 +246,21 @@ int xc_monitor_emul_unimplemented(xc_interface
> > > > *xch, uint32_t domain_id,
> > > > return do_domctl(xch, &domctl);
> > > > }
> > > >
> > > > +int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool
> > > > enable,
> > > > + bool sync)
> > > > +{
> > > > + DECLARE_DOMCTL;
> > > > +
> > > > + domctl.cmd = XEN_DOMCTL_monitor_op;
> > > > + domctl.domain = domain_id;
> > > > + domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> > > > + : XEN_DOMCTL_MONITOR_OP_DISABLE;
> > > > + domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_VMEXIT;
> > > > + domctl.u.monitor_op.u.vmexit.sync = sync;
> > > > +
> > > > + return do_domctl(xch, &domctl);
> > > > +}
> > > > +
> > > > /*
> > > > * Local variables:
> > > > * mode: C
> > > > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> > > > index b44a1e1dfe..64a38e8fa7 100644
> > > > --- a/xen/arch/x86/hvm/monitor.c
> > > > +++ b/xen/arch/x86/hvm/monitor.c
> > > > @@ -328,6 +328,24 @@ bool hvm_monitor_check_p2m(unsigned long gla,
> > > > gfn_t gfn, uint32_t pfec,
> > > > return monitor_traps(curr, true, &req) >= 0;
> > > > }
> > > >
> > > > +int hvm_monitor_vmexit(unsigned long exit_reason,
> > > > + unsigned long exit_qualification)
> > >
> > > Should this maybe live in vmx code or have 'vmx' in the name
> > > somewhere, so that if an svm counterpart is added this doesn't need to
> > > be renamed?
> >
> > I don't follow. Why would this need to be renamed? I would presume the
> > same function would be used on both if it comes to that, perhaps with
> > a unified input structure if the two are not compatible as-is. In any
> > case, there is no vm_event/monitor support for AMD at all (not just
> > for this particular event type) and no plans on adding it any time
> > soon so IMHO we should cross that bridge when and if that becomes
> > necessary.
>
> SVM has at least 3 fields related to vmexit information AFAICT:
> exitcode, exitinfo1 and exitinfo2.
>
> Instead of having an union in hvm_monitor_vmexit to cover all possible
> vendor formats it might be easier to just have vmx_ and svm_ specific
> functions, so it's contained in vendor specific code.
>
> Or maybe that would be worse because you would have to expose a lot of
> vm_event logic to vendor specific code in order to put the request on
> the ring?
I would say it would be worse, I rather not have this code be littered
all over the place unless it must be.
>
> > >
> > > > +{
> > > > + struct vcpu *curr = current;
> > > > + struct arch_domain *ad = &curr->domain->arch;
> > > > + vm_event_request_t req = {};
> > > > +
> > > > + ASSERT(ad->monitor.vmexit_enabled);
> > > > +
> > > > + req.reason = VM_EVENT_REASON_VMEXIT;
> > > > + req.u.vmexit.reason = exit_reason;
> > > > + req.u.vmexit.qualification = exit_qualification;
> > >
> > > You could set those fields at definition.
> >
> > Sure, but this is the established style throughout the file.
> >
> > > > +
> > > > + set_npt_base(curr, &req);
> > > > +
> > > > + return monitor_traps(curr, ad->monitor.vmexit_sync, &req);
> > > > +}
> > > > +
> > > > /*
> > > > * Local variables:
> > > > * mode: C
> > > > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > > > index c075370f64..2794db46f9 100644
> > > > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > > > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > > > @@ -4008,6 +4008,18 @@ void vmx_vmexit_handler(struct cpu_user_regs
> > > > *regs)
> > > > }
> > > > }
> > > >
> > > > + if ( unlikely(currd->arch.monitor.vmexit_enabled) )
> > > > + {
> > > > + int rc;
> > > > +
> > > > + __vmread(EXIT_QUALIFICATION, &exit_qualification);
> > > > + rc = hvm_monitor_vmexit(exit_reason, exit_qualification);
> > > > + if ( rc < 0 )
> > > > + goto exit_and_crash;
> > > > + if ( rc )
> > > > + return;
> > > > + }
> > >
> > > Just for my understanding, is there any reason to not do this before
> > > updating the altp2m? AFAICT the update of the active EPTP won't
> > > affect the call to hvm_monitor_vmexit.
> >
> > The currently active altp2m information is included in the vm_event
> > that will be sent out, so it is good to have the correct info for it.
> > I don't currently plan on using altp2m with this particular even type
> > but we should make sure it doesn't send out stale info in case someone
> > wants to use it differently. Certainly no point in sending the event
> > before it as the exit condition in the altp2m update blob is really
> > just dead code and can't actually be reached.
>
> Ack, thanks for the explanation.
>
> > >
> > > > +
> > > > /* XXX: This looks ugly, but we need a mechanism to ensure
> > > > * any pending vmresume has really happened
> > > > */
> > > > diff --git a/xen/arch/x86/include/asm/domain.h
> > > > b/xen/arch/x86/include/asm/domain.h
> > > > index e62e109598..855db352c0 100644
> > > > --- a/xen/arch/x86/include/asm/domain.h
> > > > +++ b/xen/arch/x86/include/asm/domain.h
> > > > @@ -430,6 +430,8 @@ struct arch_domain
> > > > */
> > > > unsigned int inguest_pagefault_disabled
> > > > : 1;
> > > > unsigned int control_register_values
> > > > : 1;
> > > > + unsigned int vmexit_enabled
> > > > : 1;
> > > > + unsigned int vmexit_sync
> > > > : 1;
> > > > struct monitor_msr_bitmap *msr_bitmap;
> > > > uint64_t write_ctrlreg_mask[4];
> > > > } monitor;
> > > > diff --git a/xen/arch/x86/include/asm/hvm/monitor.h
> > > > b/xen/arch/x86/include/asm/hvm/monitor.h
> > > > index a75cd8545c..639f6dfa37 100644
> > > > --- a/xen/arch/x86/include/asm/hvm/monitor.h
> > > > +++ b/xen/arch/x86/include/asm/hvm/monitor.h
> > > > @@ -51,6 +51,8 @@ bool hvm_monitor_emul_unimplemented(void);
> > > >
> > > > bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t pfec,
> > > > uint16_t kind);
> > > > +int hvm_monitor_vmexit(unsigned long exit_reason,
> > > > + unsigned long exit_qualification);
> > > >
> > > > #endif /* __ASM_X86_HVM_MONITOR_H__ */
> > > >
> > > > diff --git a/xen/arch/x86/include/asm/monitor.h
> > > > b/xen/arch/x86/include/asm/monitor.h
> > > > index 01c6d63bb9..d8d54c5f23 100644
> > > > --- a/xen/arch/x86/include/asm/monitor.h
> > > > +++ b/xen/arch/x86/include/asm/monitor.h
> > > > @@ -89,7 +89,8 @@ static inline uint32_t
> > > > arch_monitor_get_capabilities(struct domain *d)
> > > > (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
> > > > (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
> > > > (1U <<
> > > > XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED) |
> > > > - (1U <<
> > > > XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT));
> > > > + (1U << XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT)
> > > > |
> > > > + (1U << XEN_DOMCTL_MONITOR_EVENT_VMEXIT));
> > > >
> > > > if ( hvm_is_singlestep_supported() )
> > > > capabilities |= (1U << XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP);
> > > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> > > > index 3079726a8b..30ca71432c 100644
> > > > --- a/xen/arch/x86/monitor.c
> > > > +++ b/xen/arch/x86/monitor.c
> > > > @@ -332,6 +332,20 @@ int arch_monitor_domctl_event(struct domain *d,
> > > > break;
> > > > }
> > > >
> > > > + case XEN_DOMCTL_MONITOR_EVENT_VMEXIT:
> > > > + {
> > > > + bool old_status = ad->monitor.vmexit_enabled;
> > > > +
> > > > + if ( unlikely(old_status == requested_status) )
> > > > + return -EEXIST;
> > >
> > > What about if the requested status is the same as the current one, but
> > > vmexit sync is not?
> >
> > You need to clear the currently registered event first, then register
> > the new one.
> >
> > > IOW, I'm not sure this check is helpful, and you could likely avoid
> > > the old_status local variable.
> >
> > It is helpful on the callee side. Usually the callee needs to keep
> > track of the state of what events are enabled so that it can clean up
> > after itself and if it ever runs into trying to set the event to
> > something it's already set to then that indicates its state being
> > out-of-sync.
>
> Hm, right. I wonder if you should also check that the ring is empty
> before changing the status? So that the callee doesn't change the
> status while requests are still pending on the ring from the previous
> type?
No, that becomes tricky because really the only way to ensure the ring
remains empty from the userspace is to pause the domain, which is very
heavy handed. There is nothing wrong with asking Xen not to produce
more of a certain type of request while still being able to handle the
ones that are already on the ring. For setups where the two should
happen at the same time is where the toolstack first pauses the
domain, clears the ring, then disables the event. Both are valid
approaches.
>
> > >
> > > > +
> > > > + domain_pause(d);
> > > > + ad->monitor.vmexit_enabled = requested_status;
> > > > + ad->monitor.vmexit_sync = mop->u.vmexit.sync;
> > > > + domain_unpause(d);
> > > > + break;
> > > > + }
> > > > +
> > > > default:
> > > > /*
> > > > * Should not be reached unless
> > > > arch_monitor_get_capabilities() is
> > > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > > > index b85e6170b0..4803ed7afc 100644
> > > > --- a/xen/include/public/domctl.h
> > > > +++ b/xen/include/public/domctl.h
> > > > @@ -1057,6 +1057,7 @@ struct xen_domctl_psr_cmt_op {
> > > > #define XEN_DOMCTL_MONITOR_EVENT_EMUL_UNIMPLEMENTED 10
> > > > /* Enabled by default */
> > > > #define XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT 11
> > > > +#define XEN_DOMCTL_MONITOR_EVENT_VMEXIT 12
> > > >
> > > > struct xen_domctl_monitor_op {
> > > > uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
> > > > @@ -1107,6 +1108,11 @@ struct xen_domctl_monitor_op {
> > > > /* Pause vCPU until response */
> > > > uint8_t sync;
> > > > } debug_exception;
> > > > +
> > > > + struct {
> > > > + /* Send event and don't process vmexit */
> > > > + uint8_t sync;
> > > > + } vmexit;
> > > > } u;
> > > > };
> > > >
> > > > diff --git a/xen/include/public/vm_event.h
> > > > b/xen/include/public/vm_event.h
> > > > index 81c2ee28cc..07f106f811 100644
> > > > --- a/xen/include/public/vm_event.h
> > > > +++ b/xen/include/public/vm_event.h
> > > > @@ -175,6 +175,8 @@
> > > > #define VM_EVENT_REASON_DESCRIPTOR_ACCESS 13
> > > > /* Current instruction is not implemented by the emulator */
> > > > #define VM_EVENT_REASON_EMUL_UNIMPLEMENTED 14
> > > > +/* VMEXIT */
> > > > +#define VM_EVENT_REASON_VMEXIT 15
> > > >
> > > > /* Supported values for the vm_event_write_ctrlreg index. */
> > > > #define VM_EVENT_X86_CR0 0
> > > > @@ -394,6 +396,11 @@ struct vm_event_emul_insn_data {
> > > > uint8_t data[16]; /* Has to be completely filled */
> > > > };
> > > >
> > > > +struct vm_event_vmexit {
> > > > + uint64_t reason;
> > > > + uint64_t qualification;
> > > > +};
> > >
> > > You are exposing an Intel specific interface publicly here. Might be
> > > worth adding a note, and/or adding 'intel' or 'vmx' in the structure
> > > name: vm_event_vmx_exit, so that a vm_event_svm_exit could also be
> > > added in the future.
> >
> > All vm_event monitor events are for vmx only right now. We can
> > certainly do that abstraction if and when someone decides to add svm
> > support, the ABI is versioned and no structure here is set in stone.
> > No guarantees are even implied for the structures to remain the same
> > in any way between one version of the ABI to the next. So with that I
> > don't see the need for complicating the structures at this time.
>
> Well, it's just altering the name slightly, but the structure layout
> would be the same. Just so that someone wanting to add SVM support
> doesn't have to go and rename the VMX specific structures. I think it
> also makes it easier to identify what's vendor specific and what
> should be shared between vendors.
>
> I don't think it adds any complications to the code you are adding,
> but would make it easier for someone wanting to add a new vendor
> support in the future.
I really don't think it's necessary at this point but oh well, I'm not
going to get hung up on that.
Tamas
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |