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

Dmitry Isaykin posted 1 patch 1 year, 1 month ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/1d3e2081fba9a1ce07c3e2a28eddcd6f51d52854.1679348946.git.isaikin-dmitry@yandex.ru
There is a newer version of this series
tools/include/xenctrl.h                |  1 +
tools/libs/ctrl/xc_monitor.c           | 13 +++++++++++++
xen/arch/x86/hvm/monitor.c             | 21 +++++++++++++++++++++
xen/arch/x86/hvm/svm/svm.c             |  9 +++++++++
xen/arch/x86/hvm/vmx/vmx.c             | 24 +++++++++++++++++++-----
xen/arch/x86/include/asm/domain.h      |  1 +
xen/arch/x86/include/asm/hvm/monitor.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 ++++++++++
11 files changed, 93 insertions(+), 6 deletions(-)
[XEN PATCH v4] x86/monitor: Add new monitor event to catch I/O instructions
Posted by Dmitry Isaykin 1 year, 1 month ago
Adds monitor support for I/O instructions.

Signed-off-by: Dmitry Isaykin <isaikin-dmitry@yandex.ru>
Signed-off-by: Anton Belousov <abelousov@ptsecurity.com>
---
Changes in v4:
 * Avoid the use of fixed-width types
 * Document vm_event_io structure fields
 * Untie vm-event interface from ioreq one

Changes in v3:
 * Rebase on staging
 * Refactor branch logic on monitor_traps response

Changes in v2:
 * Handled INS and OUTS instructions too
 * Added I/O monitoring support for AMD
 * Rename functions and structures (remove "_instruction" part)
 * Reorder parameters of hvm_monitor_io to match handle_pio's order
 * Change type of string_ins parameter to bool
 * Change vm_event_io structure
 * Handle monitor_traps's return status
---
 tools/include/xenctrl.h                |  1 +
 tools/libs/ctrl/xc_monitor.c           | 13 +++++++++++++
 xen/arch/x86/hvm/monitor.c             | 21 +++++++++++++++++++++
 xen/arch/x86/hvm/svm/svm.c             |  9 +++++++++
 xen/arch/x86/hvm/vmx/vmx.c             | 24 +++++++++++++++++++-----
 xen/arch/x86/include/asm/domain.h      |  1 +
 xen/arch/x86/include/asm/hvm/monitor.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 ++++++++++
 11 files changed, 93 insertions(+), 6 deletions(-)

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/monitor.c b/xen/arch/x86/hvm/monitor.c
index a11cd76f4d..4f500beaf5 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -346,6 +346,27 @@ int hvm_monitor_vmexit(unsigned long exit_reason,
     return monitor_traps(curr, ad->monitor.vmexit_sync, &req);
 }
 
+int hvm_monitor_io(unsigned int port, unsigned int bytes,
+                   bool in, bool str)
+{
+    struct vcpu *curr = current;
+    struct arch_domain *ad = &curr->domain->arch;
+    vm_event_request_t req = {
+        .reason = VM_EVENT_REASON_IO_INSTRUCTION,
+        .u.io.bytes = bytes,
+        .u.io.port = port,
+        .u.io.in = in,
+        .u.io.str = str,
+    };
+
+    if ( !ad->monitor.io_enabled )
+        return 0;
+
+    set_npt_base(curr, &req);
+
+    return monitor_traps(curr, true, &req);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bfe03316de..02563e4b70 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2939,6 +2939,15 @@ void svm_vmexit_handler(void)
         break;
 
     case VMEXIT_IOIO:
+        rc = hvm_monitor_io(vmcb->ei.io.port,
+                            vmcb->ei.io.bytes,
+                            vmcb->ei.io.in,
+                            vmcb->ei.io.str);
+        if ( rc < 0 )
+            goto unexpected_exit_type;
+        if ( rc )
+            break;
+
         if ( !vmcb->ei.io.str )
         {
             if ( handle_pio(vmcb->ei.io.port,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 00b531f76c..0b7a302928 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4560,8 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case EXIT_REASON_IO_INSTRUCTION:
+    {
+        unsigned int port, bytes;
+        bool in, str;
+        int rc;
+
         __vmread(EXIT_QUALIFICATION, &exit_qualification);
-        if ( exit_qualification & 0x10 )
+
+        port = (exit_qualification >> 16) & 0xFFFF;
+        bytes = (exit_qualification & 0x07) + 1;
+        in = (exit_qualification & 0x08);
+        str = (exit_qualification & 0x10);
+        rc = hvm_monitor_io(port, bytes, in, str);
+        if ( rc < 0 )
+            goto exit_and_crash;
+        if ( rc )
+            break;
+
+        if ( str )
         {
             /* INS, OUTS */
             if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
@@ -4570,13 +4586,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         else
         {
             /* IN, OUT */
-            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
-            int bytes = (exit_qualification & 0x07) + 1;
-            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
-            if ( handle_pio(port, bytes, dir) )
+            if ( handle_pio(port, bytes, in ? IOREQ_READ : IOREQ_WRITE) )
                 update_guest_eip(); /* Safe: IN, OUT */
         }
         break;
+    }
 
     case EXIT_REASON_INVD:
     case EXIT_REASON_WBINVD:
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 7bc126587d..29027ffd29 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -435,6 +435,7 @@ struct arch_domain
         unsigned int descriptor_access_enabled                             : 1;
         unsigned int guest_request_userspace_enabled                       : 1;
         unsigned int emul_unimplemented_enabled                            : 1;
+        unsigned int io_enabled                                            : 1;
         /*
          * By default all events are sent.
          * This is used to filter out pagefaults.
diff --git a/xen/arch/x86/include/asm/hvm/monitor.h b/xen/arch/x86/include/asm/hvm/monitor.h
index 639f6dfa37..6884f38d73 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);
 
+int hvm_monitor_io(unsigned int port, unsigned int bytes,
+                   bool in, bool str);
+
 #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 d8d54c5f23..96e6a9d0d8 100644
--- a/xen/arch/x86/include/asm/monitor.h
+++ b/xen/arch/x86/include/asm/monitor.h
@@ -90,7 +90,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
                     (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_VMEXIT));
+                    (1U << XEN_DOMCTL_MONITOR_EVENT_VMEXIT) |
+                    (1U << XEN_DOMCTL_MONITOR_EVENT_IO));
 
     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 30ca71432c..d4857faf8a 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -346,6 +346,19 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_IO:
+    {
+        bool old_status = ad->monitor.io_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.io_enabled = requested_status;
+        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 51be28c3de..7280e9f968 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1063,6 +1063,7 @@ struct xen_domctl_psr_cmt_op {
 /* Enabled by default */
 #define XEN_DOMCTL_MONITOR_EVENT_INGUEST_PAGEFAULT     11
 #define XEN_DOMCTL_MONITOR_EVENT_VMEXIT                12
+#define XEN_DOMCTL_MONITOR_EVENT_IO                    13
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 0035c26e12..3a86f0e208 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -160,6 +160,8 @@
 #define VM_EVENT_REASON_EMUL_UNIMPLEMENTED      14
 /* VMEXIT */
 #define VM_EVENT_REASON_VMEXIT                  15
+/* IN/OUT Instruction executed */
+#define VM_EVENT_REASON_IO_INSTRUCTION          16
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
@@ -388,6 +390,13 @@ struct vm_event_vmexit {
     } arch;
 };
 
+struct vm_event_io {
+    uint32_t bytes; /* size of access */
+    uint16_t port;  /* port number */
+    uint8_t  in;    /* direction (0 = OUT, 1 = IN) */
+    uint8_t  str;   /* string instruction (0 = not string, 1 = string) */
+};
+
 typedef struct vm_event_st {
     uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
     uint32_t flags;     /* VM_EVENT_FLAG_* */
@@ -409,6 +418,7 @@ typedef struct vm_event_st {
         struct vm_event_debug                 debug_exception;
         struct vm_event_cpuid                 cpuid;
         struct vm_event_vmexit                vmexit;
+        struct vm_event_io                    io;
         union {
             struct vm_event_interrupt_x86     x86;
         } interrupt;
-- 
2.39.2
Re: [XEN PATCH v4] x86/monitor: Add new monitor event to catch I/O instructions
Posted by Andrew Cooper 1 year, 1 month ago
On 20/03/2023 9:56 pm, Dmitry Isaykin wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 00b531f76c..0b7a302928 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4560,8 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case EXIT_REASON_IO_INSTRUCTION:
> +    {
> +        unsigned int port, bytes;
> +        bool in, str;
> +        int rc;
> +
>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
> -        if ( exit_qualification & 0x10 )
> +
> +        port = (exit_qualification >> 16) & 0xFFFF;
> +        bytes = (exit_qualification & 0x07) + 1;
> +        in = (exit_qualification & 0x08);
> +        str = (exit_qualification & 0x10);
> +        rc = hvm_monitor_io(port, bytes, in, str);
> +        if ( rc < 0 )
> +            goto exit_and_crash;
> +        if ( rc )
> +            break;
> +
> +        if ( str )
>          {
>              /* INS, OUTS */
>              if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
> @@ -4570,13 +4586,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          else
>          {
>              /* IN, OUT */
> -            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
> -            int bytes = (exit_qualification & 0x07) + 1;
> -            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> -            if ( handle_pio(port, bytes, dir) )
> +            if ( handle_pio(port, bytes, in ? IOREQ_READ : IOREQ_WRITE) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }
>          break;
> +    }

Sorry for the delay.  I've got the Intel side sorted now too with
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f71f8e95c34fedb0d9ae21a100bfa9f012543abf

The rebase is:

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 78ac9ece6ff2..7233e805a905 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4578,6 +4578,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         __vmread(EXIT_QUALIFICATION, &io_qual.raw);
         bytes = io_qual.size + 1;
 
+        rc = hvm_monitor_io(io_qual.port, bytes,
+                            io_qual.in ? IOREQ_READ : IOREQ_WRITE,
+                            io_qual.str);
+        if ( rc < 0 )
+            goto exit_and_crash;
+        if ( rc )
+            break;
+
         if ( io_qual.str )
         {
             if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )


~Andrew

Re: [XEN PATCH v4] x86/monitor: Add new monitor event to catch I/O instructions
Posted by Jan Beulich 1 year, 1 month ago
On 21.03.2023 11:51, Andrew Cooper wrote:
> On 20/03/2023 9:56 pm, Dmitry Isaykin wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 00b531f76c..0b7a302928 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -4560,8 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>          break;
>>  
>>      case EXIT_REASON_IO_INSTRUCTION:
>> +    {
>> +        unsigned int port, bytes;
>> +        bool in, str;
>> +        int rc;
>> +
>>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
>> -        if ( exit_qualification & 0x10 )
>> +
>> +        port = (exit_qualification >> 16) & 0xFFFF;
>> +        bytes = (exit_qualification & 0x07) + 1;
>> +        in = (exit_qualification & 0x08);
>> +        str = (exit_qualification & 0x10);
>> +        rc = hvm_monitor_io(port, bytes, in, str);
>> +        if ( rc < 0 )
>> +            goto exit_and_crash;
>> +        if ( rc )
>> +            break;
>> +
>> +        if ( str )
>>          {
>>              /* INS, OUTS */
>>              if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
>> @@ -4570,13 +4586,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>          else
>>          {
>>              /* IN, OUT */
>> -            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>> -            int bytes = (exit_qualification & 0x07) + 1;
>> -            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
>> -            if ( handle_pio(port, bytes, dir) )
>> +            if ( handle_pio(port, bytes, in ? IOREQ_READ : IOREQ_WRITE) )
>>                  update_guest_eip(); /* Safe: IN, OUT */
>>          }
>>          break;
>> +    }
> 
> Sorry for the delay.  I've got the Intel side sorted now too with
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f71f8e95c34fedb0d9ae21a100bfa9f012543abf
> 
> The rebase is:
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 78ac9ece6ff2..7233e805a905 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4578,6 +4578,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          __vmread(EXIT_QUALIFICATION, &io_qual.raw);
>          bytes = io_qual.size + 1;
>  
> +        rc = hvm_monitor_io(io_qual.port, bytes,
> +                            io_qual.in ? IOREQ_READ : IOREQ_WRITE,

Here the conditional operator needs dropping; it just "io_qual.in" which
wants passing.

Jan

> +                            io_qual.str);
> +        if ( rc < 0 )
> +            goto exit_and_crash;
> +        if ( rc )
> +            break;
> +
>          if ( io_qual.str )
>          {
>              if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
> 
> 
> ~Andrew


Re: [XEN PATCH v4] x86/monitor: Add new monitor event to catch I/O instructions
Posted by Andrew Cooper 1 year, 1 month ago
On 21/03/2023 11:06 am, Jan Beulich wrote:
> On 21.03.2023 11:51, Andrew Cooper wrote:
>> On 20/03/2023 9:56 pm, Dmitry Isaykin wrote:
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 00b531f76c..0b7a302928 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -4560,8 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>          break;
>>>  
>>>      case EXIT_REASON_IO_INSTRUCTION:
>>> +    {
>>> +        unsigned int port, bytes;
>>> +        bool in, str;
>>> +        int rc;
>>> +
>>>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
>>> -        if ( exit_qualification & 0x10 )
>>> +
>>> +        port = (exit_qualification >> 16) & 0xFFFF;
>>> +        bytes = (exit_qualification & 0x07) + 1;
>>> +        in = (exit_qualification & 0x08);
>>> +        str = (exit_qualification & 0x10);
>>> +        rc = hvm_monitor_io(port, bytes, in, str);
>>> +        if ( rc < 0 )
>>> +            goto exit_and_crash;
>>> +        if ( rc )
>>> +            break;
>>> +
>>> +        if ( str )
>>>          {
>>>              /* INS, OUTS */
>>>              if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
>>> @@ -4570,13 +4586,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>          else
>>>          {
>>>              /* IN, OUT */
>>> -            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
>>> -            int bytes = (exit_qualification & 0x07) + 1;
>>> -            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
>>> -            if ( handle_pio(port, bytes, dir) )
>>> +            if ( handle_pio(port, bytes, in ? IOREQ_READ : IOREQ_WRITE) )
>>>                  update_guest_eip(); /* Safe: IN, OUT */
>>>          }
>>>          break;
>>> +    }
>> Sorry for the delay.  I've got the Intel side sorted now too with
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f71f8e95c34fedb0d9ae21a100bfa9f012543abf
>>
>> The rebase is:
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 78ac9ece6ff2..7233e805a905 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -4578,6 +4578,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>          __vmread(EXIT_QUALIFICATION, &io_qual.raw);
>>          bytes = io_qual.size + 1;
>>  
>> +        rc = hvm_monitor_io(io_qual.port, bytes,
>> +                            io_qual.in ? IOREQ_READ : IOREQ_WRITE,
> Here the conditional operator needs dropping; it just "io_qual.in" which
> wants passing.

Oh, of course.  In which case the delta is even smaller:

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 78ac9ece6ff2..076752d9e84b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4578,6 +4578,12 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         __vmread(EXIT_QUALIFICATION, &io_qual.raw);
         bytes = io_qual.size + 1;
 
+        rc = hvm_monitor_io(io_qual.port, bytes, io_qual.in, io_qual.str);
+        if ( rc < 0 )
+            goto exit_and_crash;
+        if ( rc )
+            break;
+
         if ( io_qual.str )
         {
             if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )


~Andrew

Re: [XEN PATCH v4] x86/monitor: Add new monitor event to catch I/O instructions
Posted by Jan Beulich 1 year, 1 month ago
On 20.03.2023 22:56, Dmitry Isaykin wrote:
> Adds monitor support for I/O instructions.
> 
> Signed-off-by: Dmitry Isaykin <isaikin-dmitry@yandex.ru>
> Signed-off-by: Anton Belousov <abelousov@ptsecurity.com>

LGTM now FWIW, but this part ...

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4560,8 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case EXIT_REASON_IO_INSTRUCTION:
> +    {
> +        unsigned int port, bytes;
> +        bool in, str;
> +        int rc;
> +
>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
> -        if ( exit_qualification & 0x10 )
> +
> +        port = (exit_qualification >> 16) & 0xFFFF;
> +        bytes = (exit_qualification & 0x07) + 1;
> +        in = (exit_qualification & 0x08);
> +        str = (exit_qualification & 0x10);
> +        rc = hvm_monitor_io(port, bytes, in, str);
> +        if ( rc < 0 )
> +            goto exit_and_crash;
> +        if ( rc )
> +            break;
> +
> +        if ( str )
>          {
>              /* INS, OUTS */
>              if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
> @@ -4570,13 +4586,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          else
>          {
>              /* IN, OUT */
> -            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
> -            int bytes = (exit_qualification & 0x07) + 1;
> -            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> -            if ( handle_pio(port, bytes, dir) )
> +            if ( handle_pio(port, bytes, in ? IOREQ_READ : IOREQ_WRITE) )
>                  update_guest_eip(); /* Safe: IN, OUT */
>          }
>          break;
> +    }

... would preferably be re-based over Andrew's change to use bitfields here
as well (just like was already done by him for SVM).

Jan