Re: [XEN PATCH] x86/monitor: Add new monitor event to catch I/O instructions

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Mar 10, 2023 at 10:57 PM Dmitry Isaykin <isaikin-dmitry@xxxxxxxxx> wrote:
>
> Adds monitor support for I/O instructions.
>
> Signed-off-by: Dmitry Isaykin <isaikin-dmitry@xxxxxxxxx>
> Signed-off-by: Anton Belousov <abelousov@xxxxxxxxxxxxxx>
> ---
>  tools/include/xenctrl.h                |  1 +
>  tools/libs/ctrl/xc_monitor.c           | 13 +++++++++++++
>  xen/arch/x86/hvm/hvm.c                 |  5 +++++
>  xen/arch/x86/hvm/monitor.c             | 21 +++++++++++++++++++++
>  xen/arch/x86/hvm/vmx/vmx.c             |  2 ++
>  xen/arch/x86/include/asm/domain.h      |  1 +
>  xen/arch/x86/include/asm/hvm/monitor.h |  3 +++
>  xen/arch/x86/include/asm/hvm/support.h |  3 +++
>  xen/arch/x86/include/asm/monitor.h     |  3 ++-
>  xen/arch/x86/monitor.c                 | 13 +++++++++++++
>  xen/include/public/domctl.h            |  1 +
>  xen/include/public/vm_event.h          | 10 ++++++++++
>  12 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 23037874d3..05967ecc92 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2102,6 +2102,7 @@ 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);
> +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable);
>  /**
>   * 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 c5fa62ff30..3cb96f444f 100644
> --- a/tools/libs/ctrl/xc_monitor.c
> +++ b/tools/libs/ctrl/xc_monitor.c
> @@ -261,6 +261,19 @@ int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable,
>      return do_domctl(xch, &domctl);
>  }
>
> +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable)
> +{
> +    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_IO;
> +
> +    return do_domctl(xch, &domctl);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0c81e2afc7..72c9f65626 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3484,6 +3484,11 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs, unsigned int inst_len)
>      return hvm_monitor_cpuid(inst_len, leaf, subleaf);
>  }
>
> +void hvm_io_instruction_intercept(uint16_t port, int dir, unsigned int bytes, unsigned int string_ins)
> +{
> +    hvm_monitor_io_instruction(port, dir, bytes, string_ins);
> +}
> +
>  void hvm_rdtsc_intercept(struct cpu_user_regs *regs)
>  {
>      msr_split(regs, hvm_get_guest_tsc(current));
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index a11cd76f4d..f8b11d1de9 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -233,6 +233,27 @@ int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf,
>      return monitor_traps(curr, 1, &req);
>  }
>
> +void hvm_monitor_io_instruction(uint16_t port, int dir,
> +                                unsigned int bytes, unsigned int string_ins)
> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *ad = &curr->domain->arch;
> +    vm_event_request_t req = {};
> +
> +    if ( !ad->monitor.io_enabled )
> +        return;
> +
> +    req.reason = VM_EVENT_REASON_IO_INSTRUCTION;
> +    req.u.io_instruction.data_size = bytes;
> +    req.u.io_instruction.port = port;
> +    req.u.io_instruction.dir = dir;
> +    req.u.io_instruction.string_ins = string_ins;
> +
> +    set_npt_base(curr, &req);
> +
> +    monitor_traps(curr, true, &req);
> +}
> +
>  void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
>                             unsigned int err, uint64_t cr2)
>  {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 278b829f73..a64c5078c5 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>              int bytes = (exit_qualification & 0x07) + 1;
>              int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;

> +            int str_ins = (exit_qualification & 0x10) ? 1 : 0;

You are already in a branch here where str_ins is checked and known to be 1.

> +            hvm_io_instruction_intercept(port, dir, bytes, str_ins);

IMHO you should have this intercept be called outside the if-else. The function already kind-of indicates str_ins is an input yet right now only called when it's 1.

The rest of the plumbing in the patch LGTM.

Thanks,

Tamas