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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>, Wei Liu <wl@xen.org>,
	Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH] x86/ioemul: Rewrite stub generation
Date: Mon, 27 Apr 2020 17:18:52 +0100	[thread overview]
Message-ID: <cd4ae124-ce0b-85e6-bae7-94d32e1dba73@citrix.com> (raw)
In-Reply-To: <20200427151829.GP28601@Air-de-Roger>

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