Re: [PATCH] x86/ioemul: Rewrite stub generation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> --
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/ioport_emulate.c  | 11 ++---
>  xen/arch/x86/pv/emul-priv-op.c | 91 
> +++++++++++++++++++++++++++++++-----------
>  xen/arch/x86/pv/gpr_switch.S   | 37 +++++------------
>  xen/arch/x86/traps.c           |  3 --
>  xen/include/asm-x86/io.h       |  3 +-
>  5 files changed, 85 insertions(+), 60 deletions(-)
> 
> diff --git a/xen/arch/x86/ioport_emulate.c b/xen/arch/x86/ioport_emulate.c
> index 499c1f6056..f7511a9c49 100644
> --- 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);

uint8_t for opcode and also I think regs can be made const?

(at least looking at the only implementation from
ioemul_handle_proliant_quirk). I'm not familiar with this area
however.

> +
> +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?

Thanks, Roger.