All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xen/x86: Improve hypercall page writing
@ 2016-12-19 16:55 Andrew Cooper
  2016-12-19 19:14 ` Boris Ostrovsky
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Andrew Cooper @ 2016-12-19 16:55 UTC (permalink / raw)
  To: Xen-devel
  Cc: Kevin Tian, Jan Beulich, Andrew Cooper, Jun Nakajima,
	Boris Ostrovsky, Suravee Suthikulpanit

Use memcpy() rather than individual writes with explicit casts.  The
__builtin_memcpy() wrapper does a better job at combining adjacent writes into
a larger word size.

This results in better generated assembly.  No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c         | 10 +++++-----
 xen/arch/x86/hvm/vmx/vmx.c         | 10 +++++-----
 xen/arch/x86/x86_64/compat/traps.c | 15 +++++++++------
 xen/arch/x86/x86_64/traps.c        | 31 ++++++++++++++++++-------------
 4 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 811ea4e..962667a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -888,12 +888,12 @@ static void svm_init_hypercall_page(struct domain *d, void *hypercall_page)
             continue;
 
         p = (char *)(hypercall_page + (i * 32));
-        *(u8  *)(p + 0) = 0xb8; /* mov imm32, %eax */
+        *(u8  *)(p + 0) = 0xb8; /* mov $<i>, %eax */
         *(u32 *)(p + 1) = i;
-        *(u8  *)(p + 5) = 0x0f; /* vmmcall */
-        *(u8  *)(p + 6) = 0x01;
-        *(u8  *)(p + 7) = 0xd9;
-        *(u8  *)(p + 8) = 0xc3; /* ret */
+        memcpy(p + 5,
+               "\x0f\x01\xd9"   /* vmmcall */
+               "\xc3",          /* ret */
+               4);
     }
 
     /* Don't support HYPERVISOR_iret at the moment */
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index d50d49e..c0e62a2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1295,12 +1295,12 @@ static void vmx_init_hypercall_page(struct domain *d, void *hypercall_page)
             continue;
 
         p = (char *)(hypercall_page + (i * 32));
-        *(u8  *)(p + 0) = 0xb8; /* mov imm32, %eax */
+        *(u8  *)(p + 0) = 0xb8; /* mov $<i>, %eax */
         *(u32 *)(p + 1) = i;
-        *(u8  *)(p + 5) = 0x0f; /* vmcall */
-        *(u8  *)(p + 6) = 0x01;
-        *(u8  *)(p + 7) = 0xc1;
-        *(u8  *)(p + 8) = 0xc3; /* ret */
+        memcpy(p + 5,
+               "\x0f\x01\xc1"   /* vmmcall */
+               "\xc3",          /* ret */
+               4);
     }
 
     /* Don't support HYPERVISOR_iret at the moment */
diff --git a/xen/arch/x86/x86_64/compat/traps.c b/xen/arch/x86/x86_64/compat/traps.c
index 95c5cb3..8b03291 100644
--- a/xen/arch/x86/x86_64/compat/traps.c
+++ b/xen/arch/x86/x86_64/compat/traps.c
@@ -388,8 +388,10 @@ static void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
         p = (char *)(hypercall_page + (i * 32));
         *(u8  *)(p+ 0) = 0xb8;    /* mov  $<i>,%eax */
         *(u32 *)(p+ 1) = i;
-        *(u16 *)(p+ 5) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
-        *(u8  *)(p+ 7) = 0xc3;    /* ret */
+        memcpy(p + 5,
+               "\xcd\x82" /* int $HYPERCALL_VECTOR */
+               "\xc3",    /* ret */
+               3);
     }
 
     /*
@@ -398,10 +400,11 @@ static void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
      * calling it.
      */
     p = (char *)(hypercall_page + (__HYPERVISOR_iret * 32));
-    *(u8  *)(p+ 0) = 0x50;    /* push %eax */
-    *(u8  *)(p+ 1) = 0xb8;    /* mov  $__HYPERVISOR_iret,%eax */
-    *(u32 *)(p+ 2) = __HYPERVISOR_iret;
-    *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
+    memcpy(p,
+           "\x50"                 /* push %eax */
+           "\xb8\x17\x00\x00\x00" /* mov  $__HYPERVISOR_iret, %eax */
+           "\xcd\x82",            /* int  $HYPERCALL_VECTOR */
+           8);
 }
 
 /*
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index fc8cde6..7a586cd 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -591,14 +591,18 @@ static void hypercall_page_initialise_ring3_kernel(void *hypercall_page)
             continue;
 
         p = (char *)(hypercall_page + (i * 32));
-        *(u8  *)(p+ 0) = 0x51;    /* push %rcx */
-        *(u16 *)(p+ 1) = 0x5341;  /* push %r11 */
-        *(u8  *)(p+ 3) = 0xb8;    /* mov  $<i>,%eax */
+        memcpy(p,
+               "\x51"      /* push %rcx */
+               "\x41\x53"  /* push %r11 */
+               "\xb8",     /* mov  $<i>,%eax */
+               4);
         *(u32 *)(p+ 4) = i;
-        *(u16 *)(p+ 8) = 0x050f;  /* syscall */
-        *(u16 *)(p+10) = 0x5b41;  /* pop  %r11 */
-        *(u8  *)(p+12) = 0x59;    /* pop  %rcx */
-        *(u8  *)(p+13) = 0xc3;    /* ret */
+        memcpy(p + 8,
+               "\x0f\x05"  /* syscall */
+               "\x41\x5b"  /* pop  %r11 */
+               "\x59"      /* pop  %rcx */
+               "\xc3",     /* ret */
+               6);
     }
 
     /*
@@ -607,12 +611,13 @@ static void hypercall_page_initialise_ring3_kernel(void *hypercall_page)
      * calling it.
      */
     p = (char *)(hypercall_page + (__HYPERVISOR_iret * 32));
-    *(u8  *)(p+ 0) = 0x51;    /* push %rcx */
-    *(u16 *)(p+ 1) = 0x5341;  /* push %r11 */
-    *(u8  *)(p+ 3) = 0x50;    /* push %rax */
-    *(u8  *)(p+ 4) = 0xb8;    /* mov  $__HYPERVISOR_iret,%eax */
-    *(u32 *)(p+ 5) = __HYPERVISOR_iret;
-    *(u16 *)(p+ 9) = 0x050f;  /* syscall */
+    memcpy(p,
+           "\x51"                 /* push %rcx */
+           "\x41\x53"             /* push %r11 */
+           "\x50"                 /* push %rax */
+           "\xb8\x17\x00\x00\x00" /* mov  $__HYPERVISOR_iret,%eax */
+           "\x0f\x05",            /* syscall */
+           11);
 }
 
 #include "compat/traps.c"
-- 
2.1.4


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: Improve hypercall page writing
  2016-12-19 16:55 [PATCH] xen/x86: Improve hypercall page writing Andrew Cooper
@ 2016-12-19 19:14 ` Boris Ostrovsky
  2016-12-20  5:11 ` Tian, Kevin
  2016-12-20  7:48 ` Jan Beulich
  2 siblings, 0 replies; 6+ messages in thread
From: Boris Ostrovsky @ 2016-12-19 19:14 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Suravee Suthikulpanit, Kevin Tian, Jun Nakajima, Jan Beulich

On 12/19/2016 11:55 AM, Andrew Cooper wrote:
> Use memcpy() rather than individual writes with explicit casts.  The
> __builtin_memcpy() wrapper does a better job at combining adjacent writes into
> a larger word size.
>
> This results in better generated assembly.  No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: Improve hypercall page writing
  2016-12-19 16:55 [PATCH] xen/x86: Improve hypercall page writing Andrew Cooper
  2016-12-19 19:14 ` Boris Ostrovsky
@ 2016-12-20  5:11 ` Tian, Kevin
  2016-12-20  7:48 ` Jan Beulich
  2 siblings, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2016-12-20  5:11 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel
  Cc: Suravee Suthikulpanit, Boris Ostrovsky, Nakajima, Jun, Jan Beulich

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, December 20, 2016 12:55 AM
> 
> Use memcpy() rather than individual writes with explicit casts.  The
> __builtin_memcpy() wrapper does a better job at combining adjacent writes into
> a larger word size.
> 
> This results in better generated assembly.  No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: Improve hypercall page writing
  2016-12-19 16:55 [PATCH] xen/x86: Improve hypercall page writing Andrew Cooper
  2016-12-19 19:14 ` Boris Ostrovsky
  2016-12-20  5:11 ` Tian, Kevin
@ 2016-12-20  7:48 ` Jan Beulich
  2016-12-20 12:21   ` Andrew Cooper
  2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-12-20  7:48 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Boris Ostrovsky, Kevin Tian, Jun Nakajima, SuraveeSuthikulpanit,
	Xen-devel

>>> On 19.12.16 at 17:55, <andrew.cooper3@citrix.com> wrote:
> Use memcpy() rather than individual writes with explicit casts.  The
> __builtin_memcpy() wrapper does a better job at combining adjacent writes into
> a larger word size.
> 
> This results in better generated assembly.  No functional change.

I don't think generated code matters much for these functions, so
I'd don't view open coding constants like ...

> --- a/xen/arch/x86/x86_64/compat/traps.c
> +++ b/xen/arch/x86/x86_64/compat/traps.c
> @@ -388,8 +388,10 @@ static void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
>          p = (char *)(hypercall_page + (i * 32));
>          *(u8  *)(p+ 0) = 0xb8;    /* mov  $<i>,%eax */
>          *(u32 *)(p+ 1) = i;
> -        *(u16 *)(p+ 5) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
> -        *(u8  *)(p+ 7) = 0xc3;    /* ret */
> +        memcpy(p + 5,
> +               "\xcd\x82" /* int $HYPERCALL_VECTOR */

... here or ...

> @@ -398,10 +400,11 @@ static void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
>       * calling it.
>       */
>      p = (char *)(hypercall_page + (__HYPERVISOR_iret * 32));
> -    *(u8  *)(p+ 0) = 0x50;    /* push %eax */
> -    *(u8  *)(p+ 1) = 0xb8;    /* mov  $__HYPERVISOR_iret,%eax */
> -    *(u32 *)(p+ 2) = __HYPERVISOR_iret;
> -    *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
> +    memcpy(p,
> +           "\x50"                 /* push %eax */
> +           "\xb8\x17\x00\x00\x00" /* mov  $__HYPERVISOR_iret, %eax */
> +           "\xcd\x82",            /* int  $HYPERCALL_VECTOR */

here as a good idea. If you used a static const uint8_t[] instead of
a string literal (which even includes a pointless nul terminator), all of
this could be avoided afaict.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: Improve hypercall page writing
  2016-12-20  7:48 ` Jan Beulich
@ 2016-12-20 12:21   ` Andrew Cooper
  2016-12-20 12:57     ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2016-12-20 12:21 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Kevin Tian, Jun Nakajima, SuraveeSuthikulpanit,
	Xen-devel

On 20/12/2016 07:48, Jan Beulich wrote:
>
>> @@ -398,10 +400,11 @@ static void hypercall_page_initialise_ring1_kernel(void *hypercall_page)
>>       * calling it.
>>       */
>>      p = (char *)(hypercall_page + (__HYPERVISOR_iret * 32));
>> -    *(u8  *)(p+ 0) = 0x50;    /* push %eax */
>> -    *(u8  *)(p+ 1) = 0xb8;    /* mov  $__HYPERVISOR_iret,%eax */
>> -    *(u32 *)(p+ 2) = __HYPERVISOR_iret;
>> -    *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
>> +    memcpy(p,
>> +           "\x50"                 /* push %eax */
>> +           "\xb8\x17\x00\x00\x00" /* mov  $__HYPERVISOR_iret, %eax */
>> +           "\xcd\x82",            /* int  $HYPERCALL_VECTOR */
> here as a good idea.

Well, they are fixed in the ABI.  It is not as if we could ever change them.

>  If you used a static const uint8_t[] instead of
> a string literal (which even includes a pointless nul terminator), all of
> this could be avoided afaict.

I can see how that would work for the `int $0x82` case, but how do you
propose fitting 4 bytes of __HYPERVISOR_iret into an initialiser for a
uint8_t array?

~Andrew

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xen/x86: Improve hypercall page writing
  2016-12-20 12:21   ` Andrew Cooper
@ 2016-12-20 12:57     ` Jan Beulich
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2016-12-20 12:57 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Boris Ostrovsky, Kevin Tian, Jun Nakajima, SuraveeSuthikulpanit,
	Xen-devel

>>> On 20.12.16 at 13:21, <andrew.cooper3@citrix.com> wrote:
> On 20/12/2016 07:48, Jan Beulich wrote:
>>
>>> @@ -398,10 +400,11 @@ static void hypercall_page_initialise_ring1_kernel(void 
> *hypercall_page)
>>>       * calling it.
>>>       */
>>>      p = (char *)(hypercall_page + (__HYPERVISOR_iret * 32));
>>> -    *(u8  *)(p+ 0) = 0x50;    /* push %eax */
>>> -    *(u8  *)(p+ 1) = 0xb8;    /* mov  $__HYPERVISOR_iret,%eax */
>>> -    *(u32 *)(p+ 2) = __HYPERVISOR_iret;
>>> -    *(u16 *)(p+ 6) = (HYPERCALL_VECTOR << 8) | 0xcd; /* int  $xx */
>>> +    memcpy(p,
>>> +           "\x50"                 /* push %eax */
>>> +           "\xb8\x17\x00\x00\x00" /* mov  $__HYPERVISOR_iret, %eax */
>>> +           "\xcd\x82",            /* int  $HYPERCALL_VECTOR */
>> here as a good idea.
> 
> Well, they are fixed in the ABI.  It is not as if we could ever change them.
> 
>>  If you used a static const uint8_t[] instead of
>> a string literal (which even includes a pointless nul terminator), all of
>> this could be avoided afaict.
> 
> I can see how that would work for the `int $0x82` case, but how do you
> propose fitting 4 bytes of __HYPERVISOR_iret into an initialiser for a
> uint8_t array?

One byte __HYPERVISOR_iret, the other three zeros. perhaps
accompanied by a BUILD_BUG_ON(). Another alternative would
be a static const struct.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-20 12:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-19 16:55 [PATCH] xen/x86: Improve hypercall page writing Andrew Cooper
2016-12-19 19:14 ` Boris Ostrovsky
2016-12-20  5:11 ` Tian, Kevin
2016-12-20  7:48 ` Jan Beulich
2016-12-20 12:21   ` Andrew Cooper
2016-12-20 12:57     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.