* [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments
@ 2020-09-28 12:04 Jan Beulich
2020-09-28 12:05 ` [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly Jan Beulich
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:04 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
@ 2020-09-28 12:05 ` Jan Beulich
2020-09-28 12:47 ` Andrew Cooper
2020-09-28 12:05 ` [PATCH 2/5] x86/ELF: don't open-code read_sreg() Jan Beulich
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:05 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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" );
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/5] x86/ELF: don't open-code read_sreg()
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
2020-09-28 12:05 ` [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly Jan Beulich
@ 2020-09-28 12:05 ` Jan Beulich
2020-09-28 12:57 ` Andrew Cooper
2020-09-28 12:06 ` [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs() Jan Beulich
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:05 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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;
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs()
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
2020-09-28 12:05 ` [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly Jan Beulich
2020-09-28 12:05 ` [PATCH 2/5] x86/ELF: don't open-code read_sreg() Jan Beulich
@ 2020-09-28 12:06 ` Jan Beulich
2020-09-28 12:58 ` Andrew Cooper
2020-09-28 12:06 ` [PATCH 4/5] x86/ELF: also record FS/GS bases " Jan Beulich
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:06 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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));
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
` (2 preceding siblings ...)
2020-09-28 12:06 ` [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs() Jan Beulich
@ 2020-09-28 12:06 ` Jan Beulich
2020-09-28 13:04 ` Andrew Cooper
2020-09-28 12:07 ` [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs() Jan Beulich
2020-09-28 15:04 ` [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs() Jan Beulich
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:06 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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);
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs()
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
` (3 preceding siblings ...)
2020-09-28 12:06 ` [PATCH 4/5] x86/ELF: also record FS/GS bases " Jan Beulich
@ 2020-09-28 12:07 ` Jan Beulich
2020-09-28 13:06 ` Andrew Cooper
2020-09-28 15:04 ` [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs() Jan Beulich
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 12:07 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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__ */
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly
2020-09-28 12:05 ` [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly Jan Beulich
@ 2020-09-28 12:47 ` Andrew Cooper
2020-09-28 13:29 ` Jan Beulich
2020-09-28 14:49 ` Jan Beulich
0 siblings, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 12:47 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] x86/ELF: don't open-code read_sreg()
2020-09-28 12:05 ` [PATCH 2/5] x86/ELF: don't open-code read_sreg() Jan Beulich
@ 2020-09-28 12:57 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 12:57 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs()
2020-09-28 12:06 ` [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs() Jan Beulich
@ 2020-09-28 12:58 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 12:58 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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));
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()
2020-09-28 12:06 ` [PATCH 4/5] x86/ELF: also record FS/GS bases " Jan Beulich
@ 2020-09-28 13:04 ` Andrew Cooper
2020-09-28 13:38 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 13:04 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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);
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs()
2020-09-28 12:07 ` [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs() Jan Beulich
@ 2020-09-28 13:06 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 13:06 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly
2020-09-28 12:47 ` Andrew Cooper
@ 2020-09-28 13:29 ` Jan Beulich
2020-09-28 14:49 ` Jan Beulich
1 sibling, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 13:29 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] x86/ELF: also record FS/GS bases in elf_core_save_regs()
2020-09-28 13:04 ` Andrew Cooper
@ 2020-09-28 13:38 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 13:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly
2020-09-28 12:47 ` Andrew Cooper
2020-09-28 13:29 ` Jan Beulich
@ 2020-09-28 14:49 ` Jan Beulich
2020-09-28 15:11 ` Andrew Cooper
1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 14:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
` (4 preceding siblings ...)
2020-09-28 12:07 ` [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs() Jan Beulich
@ 2020-09-28 15:04 ` Jan Beulich
2020-09-28 15:15 ` Andrew Cooper
5 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 15:04 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly
2020-09-28 14:49 ` Jan Beulich
@ 2020-09-28 15:11 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 15:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()
2020-09-28 15:04 ` [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs() Jan Beulich
@ 2020-09-28 15:15 ` Andrew Cooper
2020-09-28 15:40 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2020-09-28 15:15 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné
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);
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs()
2020-09-28 15:15 ` Andrew Cooper
@ 2020-09-28 15:40 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2020-09-28 15:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Roger Pau Monné
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-09-28 15:41 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 12:04 [PATCH 0/5] x86: introduce read_sregs() and elf_core_save_regs() adjustments Jan Beulich
2020-09-28 12:05 ` [PATCH 1/5] x86: introduce read_sregs() to allow storing to memory directly Jan Beulich
2020-09-28 12:47 ` Andrew Cooper
2020-09-28 13:29 ` Jan Beulich
2020-09-28 14:49 ` Jan Beulich
2020-09-28 15:11 ` Andrew Cooper
2020-09-28 12:05 ` [PATCH 2/5] x86/ELF: don't open-code read_sreg() Jan Beulich
2020-09-28 12:57 ` Andrew Cooper
2020-09-28 12:06 ` [PATCH 3/5] x86/ELF: don't store function pointer in elf_core_save_regs() Jan Beulich
2020-09-28 12:58 ` Andrew Cooper
2020-09-28 12:06 ` [PATCH 4/5] x86/ELF: also record FS/GS bases " Jan Beulich
2020-09-28 13:04 ` Andrew Cooper
2020-09-28 13:38 ` Jan Beulich
2020-09-28 12:07 ` [PATCH 5/5] x86/ELF: eliminate pointless local variable from elf_core_save_regs() Jan Beulich
2020-09-28 13:06 ` Andrew Cooper
2020-09-28 15:04 ` [PATCH 6/5] x86/ELF: drop unnecessary volatile from asm()-s in elf_core_save_regs() Jan Beulich
2020-09-28 15:15 ` Andrew Cooper
2020-09-28 15:40 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).