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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/03/2023 3:57 am, Dmitry Isaykin wrote:
> Adds monitor support for I/O instructions.
>
> Signed-off-by: Dmitry Isaykin <isaikin-dmitry@xxxxxxxxx>
> Signed-off-by: Anton Belousov <abelousov@xxxxxxxxxxxxxx>

Thankyou for the patch.  A couple of questions.

Right now, you monitor all IO ports that end up in a vmexit.  But this
is already subject to hvm_io_bitmap[] which means by default that port
0x80 and 0xed are not intercepted.  Therefore, you'll not pick them up
with this monitor either.  Is this intentional?

MSRs (which are the other monitored resource with a fine grain bitmap)
take a different approach, requiring each monitored MSR to be explicitly
opted in to.  Are you able to discuss your intended usecase here at all?

> ---
>  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 ++

You've wired up all the control infrastructure as common, but only
plugged hvm_io_instruction_intercept() in to the Intel side.

All you need to make the AMD side work equivalently is to hook
VMEXIT_IOIO in svm.c in a similar manner to how you've already modified
EXIT_REASON_IO_INSTRUCTION in vmx.c

Please do this.  I know the monitor subsystem doesn't get much love on
AMD, but we do want to do a best-effort attempt to maintain parity.

> 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;
> +            hvm_io_instruction_intercept(port, dir, bytes, str_ins);

I'm afraid this can't be correct.  Separate to Tamas' observation, in
the case that we do monitor the IO port, we must not continue with the
rest of the logic.

Otherwise what we'll end up doing is putting a monitor event on the
monitor ring, *and* sending the same event to qemu (or terminating it
with internal emulation).  This is fine if all you're trying to do is
log the access, but doesn't work if the monitor framework wants first
refusal of the access.

Monitor functions typically return int, identifying whether the vCPU has
been paused, which then controls whether the subsequent intercept logic
is performed.  See hvm_monitor_{vmexit,debug}() as examples.

> diff --git a/xen/arch/x86/include/asm/hvm/monitor.h 
> b/xen/arch/x86/include/asm/hvm/monitor.h
> index 639f6dfa37..22d2b366a6 100644
> --- a/xen/arch/x86/include/asm/hvm/monitor.h
> +++ b/xen/arch/x86/include/asm/hvm/monitor.h
> @@ -54,6 +54,9 @@ bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, 
> uint32_t pfec,
>  int hvm_monitor_vmexit(unsigned long exit_reason,
>                         unsigned long exit_qualification);
>  
> +void hvm_monitor_io_instruction(uint16_t port, int dir,
> +                                unsigned int bytes, unsigned int string_ins);

Please could this be named hvm_monitor_io() for consistency with the
rest of the monitor subsystem?

> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
> index 0035c26e12..86e4cdba7c 100644
> --- a/xen/include/public/vm_event.h
> +++ b/xen/include/public/vm_event.h
> @@ -388,6 +390,13 @@ struct vm_event_vmexit {
>      } arch;
>  };
>  
> +struct vm_event_io_instruction {
> +    uint32_t data_size;
> +    uint32_t port;
> +    uint32_t dir;
> +    uint32_t string_ins;
> +};

For a string instruction, don't you need %rsi/%rdi too?

~Andrew