[PATCH] x86/ioemul: Misc improvements to ioport_emulate.c

[PATCH] x86/ioemul: Misc improvements to ioport_emulate.c

[Andrew Cooper]

Put the opcode into an array and use memcpy.  This allows the compiled code to
be written with two movs, rather than 10 mov $imm8's.  Also, drop trailing
whitespace in the file.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/ioport_emulate.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/ioport_emulate.c b/xen/arch/x86/ioport_emulate.c
index e80993a..c2aded7 100644
--- a/xen/arch/x86/ioport_emulate.c
+++ b/xen/arch/x86/ioport_emulate.c
@@ -1,6 +1,6 @@
 /******************************************************************************
  * ioport_emulate.c
- * 
+ *
  * Handle I/O port access quirks of various platforms.
  */
 
@@ -11,32 +11,24 @@
 static bool ioemul_handle_proliant_quirk(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs)
 {
+    static const char stub[] = {
+        0x9c,       /*    pushf           */
+        0xfa,       /*    cli             */
+        0xee,       /*    out %al, %dx    */
+        0xec,       /* 1: in %dx, %al     */
+        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;
 
-    /*    pushf */
-    io_emul_stub[0] = 0x9c;
-    /*    cli */
-    io_emul_stub[1] = 0xfa;
-    /*    out %al,%dx */
-    io_emul_stub[2] = 0xee;
-    /* 1: in %dx,%al */
-    io_emul_stub[3] = 0xec;
-    /*    test $0x80,%al */
-    io_emul_stub[4] = 0xa8;
-    io_emul_stub[5] = 0x80;
-    /*    jnz 1b */
-    io_emul_stub[6] = 0x75;
-    io_emul_stub[7] = 0xfb;
-    /*    popf */
-    io_emul_stub[8] = 0x9d;
-    /*    ret */
-    io_emul_stub[9] = 0xc3;
-
-    BUILD_BUG_ON(IOEMUL_QUIRK_STUB_BYTES < 10);
+    memcpy(io_emul_stub, stub, sizeof(stub));
+    BUILD_BUG_ON(IOEMUL_QUIRK_STUB_BYTES < sizeof(stub));
 
     return true;
 }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/ioemul: Misc improvements to ioport_emulate.c

[Jan Beulich]

>>> On 26.01.18 at 15:48, <andrew.cooper3@citrix.com> wrote:
> Put the opcode into an array and use memcpy.  This allows the compiled code to
> be written with two movs, rather than 10 mov $imm8's.  Also, drop trailing
> whitespace in the file.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

It's a clear improvement, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but ...

> @@ -11,32 +11,24 @@
>  static bool ioemul_handle_proliant_quirk(
>      u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs)
>  {
> +    static const char stub[] = {
> +        0x9c,       /*    pushf           */
> +        0xfa,       /*    cli             */
> +        0xee,       /*    out %al, %dx    */
> +        0xec,       /* 1: in %dx, %al     */
> +        0xa8, 0x80, /*    test $0x80, %al */
> +        0x75, 0xfb, /*    jnz 1b          */
> +        0x9d,       /*    popf            */
> +        0xc3,       /*    ret             */
> +    };

... could you go even further and avoid this hex encoding of insns
altogether, but using a file scope asm(), or a function scope one
putting this into e.g. .fixup via .pushsection/.popsection?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/ioemul: Misc improvements to ioport_emulate.c

[Andrew Cooper]

On 29/01/18 11:23, Jan Beulich wrote:
>>>> On 26.01.18 at 15:48, <andrew.cooper3@citrix.com> wrote:
>> Put the opcode into an array and use memcpy.  This allows the compiled code to
>> be written with two movs, rather than 10 mov $imm8's.  Also, drop trailing
>> whitespace in the file.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> It's a clear improvement, so
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but ...
>
>> @@ -11,32 +11,24 @@
>>  static bool ioemul_handle_proliant_quirk(
>>      u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs)
>>  {
>> +    static const char stub[] = {
>> +        0x9c,       /*    pushf           */
>> +        0xfa,       /*    cli             */
>> +        0xee,       /*    out %al, %dx    */
>> +        0xec,       /* 1: in %dx, %al     */
>> +        0xa8, 0x80, /*    test $0x80, %al */
>> +        0x75, 0xfb, /*    jnz 1b          */
>> +        0x9d,       /*    popf            */
>> +        0xc3,       /*    ret             */
>> +    };
> ... could you go even further and avoid this hex encoding of insns
> altogether, but using a file scope asm(), or a function scope one
> putting this into e.g. .fixup via .pushsection/.popsection?

I tried that, but couldn't find a way of creating the blob as a static
in C's eyes, or the size out as a build-time constant.

~Andre

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel