[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
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |