[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/ioemul: Rewrite stub generation
On 27.04.2020 14:20, 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. There is no reason for it to be in neither of the two > translation units which use it. Alter the behaviour to return the number of > bytes written into the stub. > > Access the addresses of the host/guest helpers with extern const char arrays. > Nothing good will come of C thinking they are regular functions. I agree with the C aspect, but imo the assembly routines should, with the changes you make, be marked as being ordinary functions. A reasonable linker would then warn about the C file wanting an STT_OBJECT while the assembly file provides an STT_FUNC. I'd therefore prefer, along with marking the functions as such, to have them also declared as functions in C. > --- a/xen/arch/x86/ioport_emulate.c > +++ b/xen/arch/x86/ioport_emulate.c > @@ -8,7 +8,10 @@ > #include <xen/sched.h> > #include <xen/dmi.h> > > -static bool ioemul_handle_proliant_quirk( > +unsigned int (*ioemul_handle_quirk)( > + u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs); Would you mind adding __read_mostly on this occasion? > @@ -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)); So you treat a build failure for a runtime crash. I can see the advantages of the new approach, but the original got away with less stub space. If our L1_CACHE_SHIFT wasn't hard coded to 7 just to cover some unusual CPUs, your new approach would, if I got the counting and calculations right, not work (with a value resulting in a 64-byte cache line size). Introducing a Kconfig option for this should imo not come with a need to re-work the logic here again. Therefore I'd like us to think about a way to make the space needed not exceed 32 bytes. One option might be to arrange for some or all of R12-R15 to not need spilling. And of course there then still ought to be a BUILD_BUG_ON() identifying ahead of any testing that yet smaller cache line sizes would indeed require re-work here. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |