[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/ioemul: Rewrite stub generation
On 27/04/2020 16:18, Roger Pau Monné wrote: > On Mon, Apr 27, 2020 at 01:20:41PM +0100, Andrew Cooper wrote: >> The logic is completely undocumented and almost impossible to follow. It >> actually uses return oriented programming. Rewrite it to conform to more >> normal call mechanics, and leave a big comment explaining thing. As well as >> the code being easier to follow, it will execute faster as it isn't fighting >> the branch predictor. >> >> Move the ioemul_handle_quirk() function pointer from traps.c to >> ioport_emulate.c. > Seeing as the newest quirk was added in 2008, I wonder if such quirks > are still relevant? > > Maybe they are also used by newer boxes, I really have no idea. Its not something which I'd consider altering in this patch anyway. > >> + >> +static unsigned int ioemul_handle_proliant_quirk( >> u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs) >> { >> static const char stub[] = { >> @@ -19,18 +22,16 @@ static bool ioemul_handle_proliant_quirk( >> 0xa8, 0x80, /* test $0x80, %al */ >> 0x75, 0xfb, /* jnz 1b */ >> 0x9d, /* popf */ >> - 0xc3, /* ret */ >> }; >> uint16_t port = regs->dx; >> uint8_t value = regs->al; >> >> if ( (opcode != 0xee) || (port != 0xcd4) || !(value & 0x80) ) >> - return false; >> + return 0; >> >> memcpy(io_emul_stub, stub, sizeof(stub)); >> - BUILD_BUG_ON(IOEMUL_QUIRK_STUB_BYTES < sizeof(stub)); >> >> - return true; >> + return sizeof(stub); >> } >> >> /* This table is the set of system-specific I/O emulation hooks. */ >> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c >> index e24b84f46a..f150886711 100644 >> --- a/xen/arch/x86/pv/emul-priv-op.c >> +++ b/xen/arch/x86/pv/emul-priv-op.c >> @@ -54,51 +54,96 @@ struct priv_op_ctxt { >> unsigned int bpmatch; >> }; >> >> -/* I/O emulation support. Helper routines for, and type of, the stack stub. >> */ >> -void host_to_guest_gpr_switch(struct cpu_user_regs *); >> -unsigned long guest_to_host_gpr_switch(unsigned long); >> +/* I/O emulation helpers. Use non-standard calling conventions. */ >> +extern const char load_guest_gprs[], save_guest_gprs[]; >> >> typedef void io_emul_stub_t(struct cpu_user_regs *); >> >> static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 >> opcode, >> unsigned int port, unsigned int >> bytes) >> { >> + /* >> + * Construct a stub for IN/OUT emulation. >> + * >> + * Some platform drivers communicate with the SMM handler using GPRs as >> a >> + * mailbox. Therefore, we must perform the emulation with the hardware >> + * domain's registers in view. >> + * >> + * We write a stub of the following form, using the guest load/save >> + * helpers (abnormal calling conventions), and one of several possible >> + * stubs performing the real I/O. >> + */ >> + static const char prologue[] = { >> + 0x53, /* push %rbx */ >> + 0x55, /* push %rbp */ >> + 0x41, 0x54, /* push %r12 */ >> + 0x41, 0x55, /* push %r13 */ >> + 0x41, 0x56, /* push %r14 */ >> + 0x41, 0x57, /* push %r15 */ >> + 0x57, /* push %rdi (param for save_guest_gprs) */ >> + }; /* call load_guest_gprs */ >> + /* <I/O stub> */ >> + /* call save_guest_gprs */ >> + static const char epilogue[] = { >> + 0x5f, /* pop %rdi */ >> + 0x41, 0x5f, /* pop %r15 */ >> + 0x41, 0x5e, /* pop %r14 */ >> + 0x41, 0x5d, /* pop %r13 */ >> + 0x41, 0x5c, /* pop %r12 */ >> + 0x5d, /* pop %rbp */ >> + 0x5b, /* pop %rbx */ >> + 0xc3, /* ret */ >> + }; >> + >> struct stubs *this_stubs = &this_cpu(stubs); >> unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2; >> - long disp; >> - bool use_quirk_stub = false; >> + unsigned int quirk_bytes = 0; >> + char *p; >> + >> + /* Helpers - Read outer scope but only modify p. */ >> +#define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p += sizeof(b); }) >> +#define APPEND_CALL(f) \ >> + ({ \ >> + long disp = (long)(f) - (stub_va + p - ctxt->io_emul_stub + 5); \ >> + BUG_ON((int32_t)disp != disp); \ > I'm not sure I get the point of using signed integers instead of > unsigned ones, AFAICT you just want to check that the displacement is > < 4GB so that a relative call can be used? Displacements are +/- 2G, not <4G. Using unsigned here would be buggy. ~Andrew
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |