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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH] x86/ioemul: Rewrite stub generation
Date: Mon, 27 Apr 2020 17:28:48 +0200	[thread overview]
Message-ID: <ca3374ed-6e00-7ab2-8255-f74c16b5ad3d@suse.com> (raw)
In-Reply-To: <20200427122041.7162-1-andrew.cooper3@citrix.com>

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