[PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments

[PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments

[Jan Beulich]

1: introduce read_sregs() to allow storing to memory directly
2: ELF: don't open-code read_sreg()
3: ELF: don't store function pointer in elf_core_save_regs()
4: ELF: also record FS/GS bases in elf_core_save_regs()
5: ELF: eliminate pointless local variable from elf_core_save_regs()

Jan

[PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly

[Jan Beulich]

When storing all (data) segment registers in one go, prefer writing the
selector values directly to memory (as opposed to read_sreg()).

Also move the single register variant into the regs.h.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1703,10 +1703,7 @@ static void save_segments(struct vcpu *v
 {
     struct cpu_user_regs *regs = &v->arch.user_regs;
 
-    regs->ds = read_sreg(ds);
-    regs->es = read_sreg(es);
-    regs->fs = read_sreg(fs);
-    regs->gs = read_sreg(gs);
+    read_sregs(regs);
 
     if ( !is_pv_32bit_vcpu(v) )
     {
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -43,10 +43,7 @@ static void read_registers(struct cpu_us
     crs[2] = read_cr2();
     crs[3] = read_cr3();
     crs[4] = read_cr4();
-    regs->ds = read_sreg(ds);
-    regs->es = read_sreg(es);
-    regs->fs = read_sreg(fs);
-    regs->gs = read_sreg(gs);
+    read_sregs(regs);
     crs[5] = rdfsbase();
     crs[6] = rdgsbase();
     crs[7] = rdgsshadow();
--- a/xen/include/asm-x86/regs.h
+++ b/xen/include/asm-x86/regs.h
@@ -15,4 +15,18 @@
     (diff == 0);                                                              \
 })
 
+#define read_sreg(name) ({                                    \
+    unsigned int __sel;                                       \
+    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) ); \
+    __sel;                                                    \
+})
+
+static inline void read_sregs(struct cpu_user_regs *regs)
+{
+    asm volatile ( "mov %%ds, %0" : "=m" (regs->ds) );
+    asm volatile ( "mov %%es, %0" : "=m" (regs->es) );
+    asm volatile ( "mov %%fs, %0" : "=m" (regs->fs) );
+    asm volatile ( "mov %%gs, %0" : "=m" (regs->gs) );
+}
+
 #endif /* __X86_REGS_H__ */
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -5,12 +5,6 @@
 #include <xen/bitops.h>
 #include <asm/processor.h>
 
-#define read_sreg(name)                                         \
-({  unsigned int __sel;                                         \
-    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) );   \
-    __sel;                                                      \
-})
-
 static inline void wbinvd(void)
 {
     asm volatile ( "wbinvd" ::: "memory" );

Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly

[Andrew Cooper]

On 28/09/2020 13:05, Jan Beulich wrote:
> --- a/xen/include/asm-x86/regs.h
> +++ b/xen/include/asm-x86/regs.h
> @@ -15,4 +15,18 @@
>      (diff == 0);                                                              \
>  })
>  
> +#define read_sreg(name) ({                                    \
> +    unsigned int __sel;                                       \
> +    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) ); \
> +    __sel;                                                    \
> +})
> +
> +static inline void read_sregs(struct cpu_user_regs *regs)
> +{
> +    asm volatile ( "mov %%ds, %0" : "=m" (regs->ds) );
> +    asm volatile ( "mov %%es, %0" : "=m" (regs->es) );
> +    asm volatile ( "mov %%fs, %0" : "=m" (regs->fs) );
> +    asm volatile ( "mov %%gs, %0" : "=m" (regs->gs) );

It occurs to me that reads don't need to be volatile.  There are no side
effects.

With that fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly

[Jan Beulich]

On 28.09.2020 14:47, Andrew Cooper wrote:
> On 28/09/2020 13:05, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/regs.h
>> +++ b/xen/include/asm-x86/regs.h
>> @@ -15,4 +15,18 @@
>>      (diff == 0);                                                              \
>>  })
>>  
>> +#define read_sreg(name) ({                                    \
>> +    unsigned int __sel;                                       \
>> +    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) ); \
>> +    __sel;                                                    \
>> +})
>> +
>> +static inline void read_sregs(struct cpu_user_regs *regs)
>> +{
>> +    asm volatile ( "mov %%ds, %0" : "=m" (regs->ds) );
>> +    asm volatile ( "mov %%es, %0" : "=m" (regs->es) );
>> +    asm volatile ( "mov %%fs, %0" : "=m" (regs->fs) );
>> +    asm volatile ( "mov %%gs, %0" : "=m" (regs->gs) );
> 
> It occurs to me that reads don't need to be volatile.  There are no side
> effects.

Oh yes, of course. Too mechanical moving / copying ...

> With that fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, Jan

Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly

[Jan Beulich]

On 28.09.2020 14:47, Andrew Cooper wrote:
> On 28/09/2020 13:05, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/regs.h
>> +++ b/xen/include/asm-x86/regs.h
>> @@ -15,4 +15,18 @@
>>      (diff == 0);                                                              \
>>  })
>>  
>> +#define read_sreg(name) ({                                    \
>> +    unsigned int __sel;                                       \
>> +    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) ); \
>> +    __sel;                                                    \
>> +})
>> +
>> +static inline void read_sregs(struct cpu_user_regs *regs)
>> +{
>> +    asm volatile ( "mov %%ds, %0" : "=m" (regs->ds) );
>> +    asm volatile ( "mov %%es, %0" : "=m" (regs->es) );
>> +    asm volatile ( "mov %%fs, %0" : "=m" (regs->fs) );
>> +    asm volatile ( "mov %%gs, %0" : "=m" (regs->gs) );
> 
> It occurs to me that reads don't need to be volatile.  There are no side
> effects.

I'll do the same for what patches 3 and 5 alter anyway, assuming
this won't invalidate your R-b there.

Jan

Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly

[Andrew Cooper]

On 28/09/2020 15:49, Jan Beulich wrote:
> On 28.09.2020 14:47, Andrew Cooper wrote:
>> On 28/09/2020 13:05, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/regs.h
>>> +++ b/xen/include/asm-x86/regs.h
>>> @@ -15,4 +15,18 @@
>>>      (diff == 0);                                                              \
>>>  })
>>>  
>>> +#define read_sreg(name) ({                                    \
>>> +    unsigned int __sel;                                       \
>>> +    asm volatile ( "mov %%" STR(name) ",%0" : "=r" (__sel) ); \
>>> +    __sel;                                                    \
>>> +})
>>> +
>>> +static inline void read_sregs(struct cpu_user_regs *regs)
>>> +{
>>> +    asm volatile ( "mov %%ds, %0" : "=m" (regs->ds) );
>>> +    asm volatile ( "mov %%es, %0" : "=m" (regs->es) );
>>> +    asm volatile ( "mov %%fs, %0" : "=m" (regs->fs) );
>>> +    asm volatile ( "mov %%gs, %0" : "=m" (regs->gs) );
>> It occurs to me that reads don't need to be volatile.  There are no side
>> effects.
> I'll do the same for what patches 3 and 5 alter anyway, assuming
> this won't invalidate your R-b there.

3 is fine.  5 is a little more problematic, because there are
serialising side effects, but I suppose we really don't care here.

~Andrew

[PATCH 2/5] x86/ELF: don't open-code read_sreg()

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/x86_64/elf.h
+++ b/xen/include/asm-x86/x86_64/elf.h
@@ -1,6 +1,8 @@
 #ifndef __X86_64_ELF_H__
 #define __X86_64_ELF_H__
 
+#include <asm/regs.h>
+
 typedef struct {
     unsigned long r15;
     unsigned long r14;
@@ -53,16 +55,16 @@ static inline void elf_core_save_regs(EL
     asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
     /* orig_rax not filled in for now */
     core_regs->rip = (unsigned long)elf_core_save_regs;
-    asm volatile("movl %%cs, %%eax;" :"=a"(core_regs->cs));
+    core_regs->cs = read_sreg(cs);
     asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
     asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
     asm volatile("movl %%ss, %%eax;" :"=a"(core_regs->ss));
     /* thread_fs not filled in for now */
     /* thread_gs not filled in for now */
-    asm volatile("movl %%ds, %%eax;" :"=a"(core_regs->ds));
-    asm volatile("movl %%es, %%eax;" :"=a"(core_regs->es));
-    asm volatile("movl %%fs, %%eax;" :"=a"(core_regs->fs));
-    asm volatile("movl %%gs, %%eax;" :"=a"(core_regs->gs));
+    core_regs->ds = read_sreg(ds);
+    core_regs->es = read_sreg(es);
+    core_regs->fs = read_sreg(fs);
+    core_regs->gs = read_sreg(gs);
 
     asm volatile("mov %%cr0, %0" : "=r" (tmp) : );
     xen_core_regs->cr0 = tmp;

Re: [PATCH 2/5] x86/ELF: don't open-code read_sreg()

[Andrew Cooper]

On 28/09/2020 13:05, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/include/asm-x86/x86_64/elf.h
> +++ b/xen/include/asm-x86/x86_64/elf.h
> @@ -1,6 +1,8 @@
>  #ifndef __X86_64_ELF_H__
>  #define __X86_64_ELF_H__
>  
> +#include <asm/regs.h>
> +
>  typedef struct {
>      unsigned long r15;
>      unsigned long r14;
> @@ -53,16 +55,16 @@ static inline void elf_core_save_regs(EL
>      asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
>      /* orig_rax not filled in for now */
>      core_regs->rip = (unsigned long)elf_core_save_regs;
> -    asm volatile("movl %%cs, %%eax;" :"=a"(core_regs->cs));
> +    core_regs->cs = read_sreg(cs);
>      asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
>      asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
>      asm volatile("movl %%ss, %%eax;" :"=a"(core_regs->ss));

Another one here.

With that fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>      /* thread_fs not filled in for now */
>      /* thread_gs not filled in for now */
> -    asm volatile("movl %%ds, %%eax;" :"=a"(core_regs->ds));
> -    asm volatile("movl %%es, %%eax;" :"=a"(core_regs->es));
> -    asm volatile("movl %%fs, %%eax;" :"=a"(core_regs->fs));
> -    asm volatile("movl %%gs, %%eax;" :"=a"(core_regs->gs));
> +    core_regs->ds = read_sreg(ds);
> +    core_regs->es = read_sreg(es);
> +    core_regs->fs = read_sreg(fs);
> +    core_regs->gs = read_sreg(gs);
>  
>      asm volatile("mov %%cr0, %0" : "=r" (tmp) : );
>      xen_core_regs->cr0 = tmp;

[PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs()

[Jan Beulich]

This keeps at least gcc 10 from generating a separate function instance
in common/kexec.o alongside the inlining of the function in its sole
caller. I also think putting the address of the actual code storing the
registers is a better indication to consumers than that of an otherwise
unreferenced function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/x86_64/elf.h
+++ b/xen/include/asm-x86/x86_64/elf.h
@@ -54,7 +54,7 @@ static inline void elf_core_save_regs(EL
     asm volatile("movq %%rsi,%0" : "=m"(core_regs->rsi));
     asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
     /* orig_rax not filled in for now */
-    core_regs->rip = (unsigned long)elf_core_save_regs;
+    asm volatile("call 0f; 0: popq %0" : "=m" (core_regs->rip));
     core_regs->cs = read_sreg(cs);
     asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
     asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));

Re: [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs()

[Andrew Cooper]

On 28/09/2020 13:06, Jan Beulich wrote:
> This keeps at least gcc 10 from generating a separate function instance
> in common/kexec.o alongside the inlining of the function in its sole
> caller. I also think putting the address of the actual code storing the
> registers is a better indication to consumers than that of an otherwise
> unreferenced function.

Hmm - that's unfortunate.

elf_core_save_regs is certainly a useful name to spot in a backtrace.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/include/asm-x86/x86_64/elf.h
> +++ b/xen/include/asm-x86/x86_64/elf.h
> @@ -54,7 +54,7 @@ static inline void elf_core_save_regs(EL
>      asm volatile("movq %%rsi,%0" : "=m"(core_regs->rsi));
>      asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
>      /* orig_rax not filled in for now */
> -    core_regs->rip = (unsigned long)elf_core_save_regs;
> +    asm volatile("call 0f; 0: popq %0" : "=m" (core_regs->rip));

lea 0(%rip) will be faster to execute, and this is 64bit code specifically.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>      core_regs->cs = read_sreg(cs);
>      asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
>      asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
>

[PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()

[Jan Beulich]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/x86_64/elf.h
+++ b/xen/include/asm-x86/x86_64/elf.h
@@ -1,6 +1,7 @@
 #ifndef __X86_64_ELF_H__
 #define __X86_64_ELF_H__
 
+#include <asm/msr.h>
 #include <asm/regs.h>
 
 typedef struct {
@@ -59,8 +60,8 @@ static inline void elf_core_save_regs(EL
     asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
     asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
     asm volatile("movl %%ss, %%eax;" :"=a"(core_regs->ss));
-    /* thread_fs not filled in for now */
-    /* thread_gs not filled in for now */
+    rdmsrl(MSR_FS_BASE, core_regs->thread_fs);
+    rdmsrl(MSR_GS_BASE, core_regs->thread_gs);
     core_regs->ds = read_sreg(ds);
     core_regs->es = read_sreg(es);
     core_regs->fs = read_sreg(fs);

Re: [PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()

[Andrew Cooper]

On 28/09/2020 13:06, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Any idea why this wasn't done before?  At a minimum, I'd be tempted to
put a sentence in the commit message saying "no idea why this wasn't
done before".

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/include/asm-x86/x86_64/elf.h
> +++ b/xen/include/asm-x86/x86_64/elf.h
> @@ -1,6 +1,7 @@
>  #ifndef __X86_64_ELF_H__
>  #define __X86_64_ELF_H__
>  
> +#include <asm/msr.h>
>  #include <asm/regs.h>
>  
>  typedef struct {
> @@ -59,8 +60,8 @@ static inline void elf_core_save_regs(EL
>      asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
>      asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
>      asm volatile("movl %%ss, %%eax;" :"=a"(core_regs->ss));
> -    /* thread_fs not filled in for now */
> -    /* thread_gs not filled in for now */
> +    rdmsrl(MSR_FS_BASE, core_regs->thread_fs);
> +    rdmsrl(MSR_GS_BASE, core_regs->thread_gs);
>      core_regs->ds = read_sreg(ds);
>      core_regs->es = read_sreg(es);
>      core_regs->fs = read_sreg(fs);
>

Re: [PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()

[Jan Beulich]

On 28.09.2020 15:04, Andrew Cooper wrote:
> On 28/09/2020 13:06, Jan Beulich wrote:
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Any idea why this wasn't done before?

No. My only guess is laziness, in the sense of "I'll do it later" and
then forgetting.

>  At a minimum, I'd be tempted to
> put a sentence in the commit message saying "no idea why this wasn't
> done before".

Sure, done.

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

[PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs()

[Jan Beulich]

We can just as well specify the CRn structure fields directly in the
asm()s, just like done for all other ones.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/x86_64/elf.h
+++ b/xen/include/asm-x86/x86_64/elf.h
@@ -37,8 +37,6 @@ typedef struct {
 static inline void elf_core_save_regs(ELF_Gregset *core_regs, 
                                       crash_xen_core_t *xen_core_regs)
 {
-    unsigned long tmp;
-
     asm volatile("movq %%r15,%0" : "=m"(core_regs->r15));
     asm volatile("movq %%r14,%0" : "=m"(core_regs->r14));
     asm volatile("movq %%r13,%0" : "=m"(core_regs->r13));
@@ -67,17 +65,10 @@ static inline void elf_core_save_regs(EL
     core_regs->fs = read_sreg(fs);
     core_regs->gs = read_sreg(gs);
 
-    asm volatile("mov %%cr0, %0" : "=r" (tmp) : );
-    xen_core_regs->cr0 = tmp;
-
-    asm volatile("mov %%cr2, %0" : "=r" (tmp) : );
-    xen_core_regs->cr2 = tmp;
-
-    asm volatile("mov %%cr3, %0" : "=r" (tmp) : );
-    xen_core_regs->cr3 = tmp;
-
-    asm volatile("mov %%cr4, %0" : "=r" (tmp) : );
-    xen_core_regs->cr4 = tmp;
+    asm volatile("mov %%cr0, %0" : "=r" (xen_core_regs->cr0));
+    asm volatile("mov %%cr2, %0" : "=r" (xen_core_regs->cr2));
+    asm volatile("mov %%cr3, %0" : "=r" (xen_core_regs->cr3));
+    asm volatile("mov %%cr4, %0" : "=r" (xen_core_regs->cr4));
 }
 
 #endif /* __X86_64_ELF_H__ */

Re: [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs()

[Andrew Cooper]

On 28/09/2020 13:07, Jan Beulich wrote:
> We can just as well specify the CRn structure fields directly in the
> asm()s, just like done for all other ones.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()

[Jan Beulich]

There are no hidden side effects here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/include/asm-x86/x86_64/elf.h
+++ b/xen/include/asm-x86/x86_64/elf.h
@@ -37,26 +37,26 @@ typedef struct {
 static inline void elf_core_save_regs(ELF_Gregset *core_regs, 
                                       crash_xen_core_t *xen_core_regs)
 {
-    asm volatile("movq %%r15,%0" : "=m"(core_regs->r15));
-    asm volatile("movq %%r14,%0" : "=m"(core_regs->r14));
-    asm volatile("movq %%r13,%0" : "=m"(core_regs->r13));
-    asm volatile("movq %%r12,%0" : "=m"(core_regs->r12));
-    asm volatile("movq %%rbp,%0" : "=m"(core_regs->rbp));
-    asm volatile("movq %%rbx,%0" : "=m"(core_regs->rbx));
-    asm volatile("movq %%r11,%0" : "=m"(core_regs->r11));
-    asm volatile("movq %%r10,%0" : "=m"(core_regs->r10));
-    asm volatile("movq %%r9,%0" : "=m"(core_regs->r9));
-    asm volatile("movq %%r8,%0" : "=m"(core_regs->r8));
-    asm volatile("movq %%rax,%0" : "=m"(core_regs->rax));
-    asm volatile("movq %%rcx,%0" : "=m"(core_regs->rcx));
-    asm volatile("movq %%rdx,%0" : "=m"(core_regs->rdx));
-    asm volatile("movq %%rsi,%0" : "=m"(core_regs->rsi));
-    asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
+    asm ( "movq %%r15,%0" : "=m" (core_regs->r15) );
+    asm ( "movq %%r14,%0" : "=m" (core_regs->r14) );
+    asm ( "movq %%r13,%0" : "=m" (core_regs->r13) );
+    asm ( "movq %%r12,%0" : "=m" (core_regs->r12) );
+    asm ( "movq %%rbp,%0" : "=m" (core_regs->rbp) );
+    asm ( "movq %%rbx,%0" : "=m" (core_regs->rbx) );
+    asm ( "movq %%r11,%0" : "=m" (core_regs->r11) );
+    asm ( "movq %%r10,%0" : "=m" (core_regs->r10) );
+    asm ( "movq %%r9,%0" : "=m" (core_regs->r9) );
+    asm ( "movq %%r8,%0" : "=m" (core_regs->r8) );
+    asm ( "movq %%rax,%0" : "=m" (core_regs->rax) );
+    asm ( "movq %%rcx,%0" : "=m" (core_regs->rcx) );
+    asm ( "movq %%rdx,%0" : "=m" (core_regs->rdx) );
+    asm ( "movq %%rsi,%0" : "=m" (core_regs->rsi) );
+    asm ( "movq %%rdi,%0" : "=m" (core_regs->rdi) );
     /* orig_rax not filled in for now */
     asm ( "call 0f; 0: popq %0" : "=m" (core_regs->rip) );
     core_regs->cs = read_sreg(cs);
-    asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
-    asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
+    asm ( "pushfq; popq %0" : "=m" (core_regs->rflags) );
+    asm ( "movq %%rsp,%0" : "=m" (core_regs->rsp) );
     core_regs->ss = read_sreg(ss);
     rdmsrl(MSR_FS_BASE, core_regs->thread_fs);
     rdmsrl(MSR_GS_BASE, core_regs->thread_gs);

Re: [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()

[Andrew Cooper]

On 28/09/2020 16:04, Jan Beulich wrote:
> There are no hidden side effects here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/xen/include/asm-x86/x86_64/elf.h
> +++ b/xen/include/asm-x86/x86_64/elf.h
> @@ -37,26 +37,26 @@ typedef struct {
>  static inline void elf_core_save_regs(ELF_Gregset *core_regs, 
>                                        crash_xen_core_t *xen_core_regs)
>  {
> -    asm volatile("movq %%r15,%0" : "=m"(core_regs->r15));
> -    asm volatile("movq %%r14,%0" : "=m"(core_regs->r14));
> -    asm volatile("movq %%r13,%0" : "=m"(core_regs->r13));
> -    asm volatile("movq %%r12,%0" : "=m"(core_regs->r12));
> -    asm volatile("movq %%rbp,%0" : "=m"(core_regs->rbp));
> -    asm volatile("movq %%rbx,%0" : "=m"(core_regs->rbx));
> -    asm volatile("movq %%r11,%0" : "=m"(core_regs->r11));
> -    asm volatile("movq %%r10,%0" : "=m"(core_regs->r10));
> -    asm volatile("movq %%r9,%0" : "=m"(core_regs->r9));
> -    asm volatile("movq %%r8,%0" : "=m"(core_regs->r8));
> -    asm volatile("movq %%rax,%0" : "=m"(core_regs->rax));
> -    asm volatile("movq %%rcx,%0" : "=m"(core_regs->rcx));
> -    asm volatile("movq %%rdx,%0" : "=m"(core_regs->rdx));
> -    asm volatile("movq %%rsi,%0" : "=m"(core_regs->rsi));
> -    asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
> +    asm ( "movq %%r15,%0" : "=m" (core_regs->r15) );
> +    asm ( "movq %%r14,%0" : "=m" (core_regs->r14) );
> +    asm ( "movq %%r13,%0" : "=m" (core_regs->r13) );
> +    asm ( "movq %%r12,%0" : "=m" (core_regs->r12) );
> +    asm ( "movq %%rbp,%0" : "=m" (core_regs->rbp) );
> +    asm ( "movq %%rbx,%0" : "=m" (core_regs->rbx) );
> +    asm ( "movq %%r11,%0" : "=m" (core_regs->r11) );
> +    asm ( "movq %%r10,%0" : "=m" (core_regs->r10) );
> +    asm ( "movq %%r9,%0" : "=m" (core_regs->r9) );
> +    asm ( "movq %%r8,%0" : "=m" (core_regs->r8) );

Any chance we can align these seeing as they're changing?

What about spaces before %0 ?

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +    asm ( "movq %%rax,%0" : "=m" (core_regs->rax) );
> +    asm ( "movq %%rcx,%0" : "=m" (core_regs->rcx) );
> +    asm ( "movq %%rdx,%0" : "=m" (core_regs->rdx) );
> +    asm ( "movq %%rsi,%0" : "=m" (core_regs->rsi) );
> +    asm ( "movq %%rdi,%0" : "=m" (core_regs->rdi) );
>      /* orig_rax not filled in for now */
>      asm ( "call 0f; 0: popq %0" : "=m" (core_regs->rip) );
>      core_regs->cs = read_sreg(cs);
> -    asm volatile("pushfq; popq %0" :"=m"(core_regs->rflags));
> -    asm volatile("movq %%rsp,%0" : "=m"(core_regs->rsp));
> +    asm ( "pushfq; popq %0" : "=m" (core_regs->rflags) );
> +    asm ( "movq %%rsp,%0" : "=m" (core_regs->rsp) );
>      core_regs->ss = read_sreg(ss);
>      rdmsrl(MSR_FS_BASE, core_regs->thread_fs);
>      rdmsrl(MSR_GS_BASE, core_regs->thread_gs);
>

Re: [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()

[Jan Beulich]

On 28.09.2020 17:15, Andrew Cooper wrote:
> On 28/09/2020 16:04, Jan Beulich wrote:
>> There are no hidden side effects here.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: New.
>>
>> --- a/xen/include/asm-x86/x86_64/elf.h
>> +++ b/xen/include/asm-x86/x86_64/elf.h
>> @@ -37,26 +37,26 @@ typedef struct {
>>  static inline void elf_core_save_regs(ELF_Gregset *core_regs, 
>>                                        crash_xen_core_t *xen_core_regs)
>>  {
>> -    asm volatile("movq %%r15,%0" : "=m"(core_regs->r15));
>> -    asm volatile("movq %%r14,%0" : "=m"(core_regs->r14));
>> -    asm volatile("movq %%r13,%0" : "=m"(core_regs->r13));
>> -    asm volatile("movq %%r12,%0" : "=m"(core_regs->r12));
>> -    asm volatile("movq %%rbp,%0" : "=m"(core_regs->rbp));
>> -    asm volatile("movq %%rbx,%0" : "=m"(core_regs->rbx));
>> -    asm volatile("movq %%r11,%0" : "=m"(core_regs->r11));
>> -    asm volatile("movq %%r10,%0" : "=m"(core_regs->r10));
>> -    asm volatile("movq %%r9,%0" : "=m"(core_regs->r9));
>> -    asm volatile("movq %%r8,%0" : "=m"(core_regs->r8));
>> -    asm volatile("movq %%rax,%0" : "=m"(core_regs->rax));
>> -    asm volatile("movq %%rcx,%0" : "=m"(core_regs->rcx));
>> -    asm volatile("movq %%rdx,%0" : "=m"(core_regs->rdx));
>> -    asm volatile("movq %%rsi,%0" : "=m"(core_regs->rsi));
>> -    asm volatile("movq %%rdi,%0" : "=m"(core_regs->rdi));
>> +    asm ( "movq %%r15,%0" : "=m" (core_regs->r15) );
>> +    asm ( "movq %%r14,%0" : "=m" (core_regs->r14) );
>> +    asm ( "movq %%r13,%0" : "=m" (core_regs->r13) );
>> +    asm ( "movq %%r12,%0" : "=m" (core_regs->r12) );
>> +    asm ( "movq %%rbp,%0" : "=m" (core_regs->rbp) );
>> +    asm ( "movq %%rbx,%0" : "=m" (core_regs->rbx) );
>> +    asm ( "movq %%r11,%0" : "=m" (core_regs->r11) );
>> +    asm ( "movq %%r10,%0" : "=m" (core_regs->r10) );
>> +    asm ( "movq %%r9,%0" : "=m" (core_regs->r9) );
>> +    asm ( "movq %%r8,%0" : "=m" (core_regs->r8) );
> 
> Any chance we can align these seeing as they're changing?

I wasn't really sure about this - alignment to cover for the
difference between r8 and r9 vs r10-r15 never comes out nicely,
as the padding should really be in the number part of the
names. I'd prefer to leave it as is, while ...

> What about spaces before %0 ?

... I certainly will add these (as I should have noticed their
lack myself).

> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan