[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Dmitry Isaykin <isaikin-dmitry@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 13 Mar 2023 12:32:53 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=o9dFtfzQkyiRQz3eN4tzt3B8OXqbHpwxydE9VnCLBj4=; b=CVNiCy/EIWxqYww531hKdWKV4KiiNOKUVKVkdxV/GXYz6e9M/yCQ63R/OML+t7hGLSc35n2rGy19oihV5KicbwlWnMSw8yjGCrnkzNIf9P0lWMR6q6LN67sHcCdu9gCmpEnW2DSZy6Vd2k0J48YHCWeon45QKOuvY/CJ3DCU3gT3isKJpJ3/n+G5e3O4eOv4HQ8AvQ+iiPyVx62H8uJ0w/vtuflPzpKlx+7n9AoGFyqRvD8pI+YOfL6pM9s0u1z4yjLqNtB0sjiLofIAm+k6VAGpfAT951AeJ5F5y/hpcRi4ZJJ+q23S5EJkVDgCcerOS82ekcJDV9dhggfA/Q+I6A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Rb3QHEqlBbGm52GBDCTjqkMdMYmB/RlZnoDMbzLVMgxUSlxLKnhUYeCHsOkOtDdcJrv8cWRTaMn+LzbF4e7LoZV/H+XvaB9kFLOQcwsD7IqNkOWmb6ILqUYvNwP3wcUv2x32C9Jfh8cflWZEk/y05f0KXyjgnm/B6+VP6kyC4nZuosMoJWvrPKIJSbcLkHKqV3KRJ1qrbOsOOts9l3pyVqKETtG5nhtIEajryIYQbqhQJvnaNLruwZ/IKCdnUWethoNLogJu4ap8w+W8UtWCZNz4iBY7aoagHXD3tuwf4C32yIhEdyVOKzSIE6M38LSbcS1dhVJeWXtOjyOjOeee1g==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Wei Liu <wl@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Anton Belousov <abelousov@xxxxxxxxxxxxxx>
  • Delivery-date: Mon, 13 Mar 2023 13:39:13 +0000
  • Ironport-data: A9a23:VWCS8K/4YsTGHJ5aGcJ/DrUDiH6TJUtcMsCJ2f8bNWPcYEJGY0x3m 2IfCDrSb/uPZGajeYsib4vkpE4FvsXRydc1HlM9pHo8E34SpcT7XtnIdU2Y0wF+jCHgZBk+s 5hBMImowOQcFCK0SsKFa+C5xZVE/fjUAOG6UKicYXoZqTZMEE8JkQhkl/MynrlmiN24BxLlk d7pqojUNUTNNwRcawr40Ire7kI/1BjOkGlA5AdmPqkQ5AW2e0Q9V/rzG4ngdxMUfaEMdgKKb 76r5K20+Grf4yAsBruN+losWhRXKlJ6FVHmZkt+A8BOsDAbzsAB+v9T2M4nQVVWk120c+VZk 72hg3ASpTABZcUgkMxFO/VR/roX0aduoNcrKlDn2SCfItGvn9IBDJyCAWlvVbD09NqbDklR/ M04BBM2MSyhnua46rGpS7ZDlMcaeZyD0IM34hmMzBn/JNN/GNXpZfWP4tVVmjAtmspJAPDSI dIDbiZiZwjBZBsJPUoLDJU5n6GjgXyXnz9w8QrJ4/ZopTWDilUvgNABM/KMEjCObexTklyVu STt+GPhDwtBHNee1SCE4jSngeqncSbTAdpLTOXopqcz6LGV7i9CBzgoekuwmsWeoEy3aeIFI GoV9AN7+MDe82TuFLERRSaQoWWNvx0dXZ9cFuwm8hCl26PS7wuJQHIZJhZRZdpjuMIoSDgC0 l6Sg8ivFTFpqKeSS3+W6vGTtzzaETcRBX8PY2kDVwRty9vsuoYolTrUU81uVqWyi7XdGzv93 jSLpygWnKgIgIgA0KDT1VLahzOhoLDZQwhz4R/YNkq+9R9wboOhY42u6HDY4OxGIYLfSUOO1 FAYks2X98gSDpWAkiOcTeFLF7asj96MMSbVgFpmN5Mg6zig9XOlcY1Kpjp5IS9BKt4FYzTgZ EbZpCtb5YNfMXWna6N6ecS6DMFC5ar9E5LjX/PdbNtLa7BwchOK+GdlYkv492Pgjkkq170+M JGzcMCwAHJcAqNipBK/TeoZ1qIwwT4W3X/ISJvm1RW7wPyVY3v9dFseGF6Hb+R85qXUpgzQq o9bL5HTlUUZV/DiaC7K94JVNUoNMXUwGZHxrYpQa/KHJQ1lXmomDpc93I8cRmCspIwN/s+gw 513chYwJIbX7ZEfFTi3Vw==
  • Ironport-hdrordr: A9a23:Vqg+aqgiOLFl5ka6AU3db/PmLXBQXusji2hC6mlwRA09TyXPrb HKoB17726OtN91YhtMpTnuAtjmfZqxz+8N3WBzB9aftWvdyQ+VxehZhOOI/9SHIVycygdz79 YDT0EUMqyXMbEVt7eD3OB6KbkdKRu8nJxASd2x856ld2FXV50=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.