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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/03/2023 6:54 pm, Dmitry Isaykin wrote:
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index a43bcf2e92..49225f48a7 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2939,17 +2939,26 @@ void svm_vmexit_handler(void)
>          break;
>  
>      case VMEXIT_IOIO:
> -        if ( (vmcb->exitinfo1 & (1u<<2)) == 0 )
> +     {
> +        uint16_t port = (vmcb->exitinfo1 >> 16) & 0xFFFF;
> +        int bytes = ((vmcb->exitinfo1 >> 4) & 0x07);
> +        int dir = (vmcb->exitinfo1 & 1) ? IOREQ_READ : IOREQ_WRITE;
> +        bool string_ins = (vmcb->exitinfo1 & (1u<<2));
> +        int rc = hvm_monitor_io(port, bytes, dir, string_ins);
> +        if ( rc < 0 )
> +            goto unexpected_exit_type;
> +        if ( !rc )
>          {
> -            uint16_t port = (vmcb->exitinfo1 >> 16) & 0xFFFF;
> -            int bytes = ((vmcb->exitinfo1 >> 4) & 0x07);
> -            int dir = (vmcb->exitinfo1 & 1) ? IOREQ_READ : IOREQ_WRITE;
> -            if ( handle_pio(port, bytes, dir) )
> -                __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
> +            if ( !string_ins )
> +            {
> +                if ( handle_pio(port, bytes, dir) )
> +                    __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
> +            }
> +            else if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
>          }
> -        else if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
>          break;
> +    }

There are a few style issues, but it's also a mess because of the manual
exitinfo decoding, so I went ahead and did

https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=df9369154aa010b2322e3f3e0727a242784cfd4f

to clean it up.  The rebased version of this hunk is now:

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index bfe03316def6..17ac99f6cd56 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 ? IOREQ_READ : IOREQ_WRITE,
+                            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,

which I hope you'll agree is much more simple to follow.

I'm also trying to sort out a similar cleanup on the Intel side, but
haven't managed to post that yet.

~Andrew