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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.03.2023 13:01, Dmitry Isaykin wrote:
> --- 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(uint16_t port, unsigned int bytes,

Please avoid the use of fixed-width types where not really necessary. See
./CODING_STYLE.

> +                   int dir, bool string_ins)

"ins" is an ambiguous abbreviation, even more so when anyway talking about
x86'es I/O instructions. You might consider simply omitting the suffix, or
alternatively I'd like to ask for "insn" (my preference) or "instr".

> +{
> +    struct vcpu *curr = current;
> +    struct arch_domain *ad = &curr->domain->arch;
> +    vm_event_request_t req = {};
> +
> +    if ( !ad->monitor.io_enabled )
> +        return 0;
> +
> +    req.reason = VM_EVENT_REASON_IO_INSTRUCTION;
> +    req.u.io.data_size = bytes;
> +    req.u.io.port = port;
> +    req.u.io.dir = dir;
> +    req.u.io.string_ins = string_ins;

Having these be the variable's initializer would probably be more in line
with what we do elsewhere, including in many cases right in this same
source file (yet sadly it's not really consistent).

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4560,7 +4560,24 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case EXIT_REASON_IO_INSTRUCTION:
> +    {
> +        uint16_t port;
> +        int bytes, dir;

Since you move it, "bytes" wants to be "unsigned int" (together with "port").
At which point "dir" ...

> +        bool string_ins;
> +        int rc;

... can share a declaration with "rc".

>          __vmread(EXIT_QUALIFICATION, &exit_qualification);
> +
> +        port = (exit_qualification >> 16) & 0xFFFF;
> +        bytes = (exit_qualification & 0x07) + 1;
> +        dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> +        string_ins = (exit_qualification & 0x10);
> +        rc = hvm_monitor_io(port, bytes, dir, string_ins);
> +        if ( rc < 0 )
> +            goto exit_and_crash;
> +        if ( rc )
> +            break;
> +
>          if ( exit_qualification & 0x10 )

Please either use the new local variable here then as well, or omit it
in favor of using the same expression in the other function call.

> --- 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 data_size;
> +    uint16_t port;
> +    uint8_t  dir; /* IOREQ_READ or IOREQ_WRITE */

Are you actually sure you want to tie the vm-event interface to the ioreq
one (this is also a question to you, Tamas)? It would look slightly better
to me if this was a simple boolean named after its purpose (e.g. "write"
or "out" when it's meant to be set for OUT / OUTS and clear for IN / INS).

Jan