Re: [Xen-devel] [PATCH 03/13] x86/paravirt: Convert native patch assembly code strings to macros

2017-11-18 Thread Josh Poimboeuf 
On Sat, Nov 18, 2017 at 11:20:06AM +0100, Juergen Gross wrote:
>  +#define NATIVE_ZERO "xor " _REG_ARG1 ", " _REG_ARG1
> >>>
> >>> NATIVE_ZERO_OUT
> >>>
> >>> I guess. NATIVE_ZERO reads like the native representation of 0 :-)
> >>
> >> NATIVE_ZERO_ARG1?
> > 
> > On a slight tangent, does anybody know why it zeros the arg?
> 
> Why are _you_ asking? You've introduced it.

So I did.  Touché!

> > The only place it's used is here:
> > 
> > #if defined(CONFIG_PARAVIRT_SPINLOCKS)
> > DEF_NATIVE(pv_lock_ops, queued_spin_unlock, 
> > NATIVE_QUEUED_SPIN_UNLOCK);
> > DEF_NATIVE(pv_lock_ops, vcpu_is_preempted,  NATIVE_ZERO);
> > #endif
> > 
> > Isn't that a bug?  Seems like it should _return_ zero.  Zeroing the arg
> > shouldn't have any effect.
> 
> Right. Before that patch it _did_ return zero instead of zeroing arg1.

Oops!

> > If I'm right, we could call it NATIVE_FALSE.
> 
> I'd prefer NATIVE_ZERO, as it will be usable for non-boolean cases, too.

NATIVE_ZERO works for me.

-- 
Josh

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

Re: [Xen-devel] [PATCH 03/13] x86/paravirt: Convert native patch assembly code strings to macros

2017-11-17 Thread Josh Poimboeuf 
On Fri, Nov 17, 2017 at 08:10:13PM +0100, Juergen Gross wrote:
> On 17/11/17 19:07, Borislav Petkov wrote:
> > On Wed, Oct 04, 2017 at 10:58:24AM -0500, Josh Poimboeuf wrote:
> >> Convert the hard-coded native patch assembly code strings to macros to
> >> facilitate sharing common code between 32-bit and 64-bit.
> >>
> >> These macros will also be used by a future patch which requires the GCC
> >> extended asm syntax of two '%' characters instead of one when specifying
> >> a register name.
> >>
> >> Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
> >> ---
> >>  arch/x86/include/asm/special_insns.h | 24 
> >>  arch/x86/kernel/paravirt_patch_32.c  | 21 +++--
> >>  arch/x86/kernel/paravirt_patch_64.c  | 29 +++--
> >>  3 files changed, 50 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/special_insns.h 
> >> b/arch/x86/include/asm/special_insns.h
> >> index ac402c6fc24b..0549c5f2c1b3 100644
> >> --- a/arch/x86/include/asm/special_insns.h
> >> +++ b/arch/x86/include/asm/special_insns.h
> >> @@ -6,6 +6,30 @@
> >>  
> >>  #include 
> >>  
> >> +#ifdef CONFIG_X86_64
> >> +# define _REG_ARG1"%rdi"
> >> +# define NATIVE_IDENTITY_32   "mov %edi, %eax"
> > 
> > Yeah, that "identity" looks strange. How about NATIVE_NOOP and
> > NATIVE_NOOP_32 ?
> 
> Those are not NOPs. They return the identical value which was passed to
> them. So identity isn't a bad name after all.

Right, like the math identity function:

  https://en.wikipedia.org/wiki/Identity_function

> >> +# define NATIVE_USERGS_SYSRET64   "swapgs; sysretq"
> >> +#else
> >> +# define _REG_ARG1"%eax"
> >> +#endif
> >> +
> >> +#define _REG_RET  "%" _ASM_AX
> >> +
> >> +#define NATIVE_ZERO   "xor " _REG_ARG1 ", " _REG_ARG1
> > 
> > NATIVE_ZERO_OUT
> > 
> > I guess. NATIVE_ZERO reads like the native representation of 0 :-)
> 
> NATIVE_ZERO_ARG1?

On a slight tangent, does anybody know why it zeros the arg?

The only place it's used is here:

#if defined(CONFIG_PARAVIRT_SPINLOCKS)
DEF_NATIVE(pv_lock_ops, queued_spin_unlock, NATIVE_QUEUED_SPIN_UNLOCK);
DEF_NATIVE(pv_lock_ops, vcpu_is_preempted,  NATIVE_ZERO);
#endif

Isn't that a bug?  Seems like it should _return_ zero.  Zeroing the arg
shouldn't have any effect.

If I'm right, we could call it NATIVE_FALSE.

-- 
Josh

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

Re: [Xen-devel] [PATCH 10/13] x86/alternative: Support indirect call replacement

2017-11-16 Thread Josh Poimboeuf 
On Wed, Oct 25, 2017 at 01:25:02PM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Add alternative patching support for replacing an instruction with an
> > indirect call.  This will be needed for the paravirt alternatives.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
> > ---
> >  arch/x86/kernel/alternative.c | 22 +++---
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 3344d3382e91..81c577c7deba 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -410,20 +410,28 @@ void __init_or_module noinline 
> > apply_alternatives(struct alt_instr *start,
> > insnbuf_sz = a->replacementlen;
> >  
> > /*
> > -* 0xe8 is a relative jump; fix the offset.
> > -*
> > -* Instruction length is checked before the opcode to avoid
> > -* accessing uninitialized bytes for zero-length replacements.
> > +* Fix the address offsets for call and jump instructions which
> > +* use PC-relative addressing.
> >  */
> > if (a->replacementlen == 5 && *insnbuf == 0xe8) {
> > +   /* direct call */
> > *(s32 *)(insnbuf + 1) += replacement - instr;
> > -   DPRINTK("Fix CALL offset: 0x%x, CALL 0x%lx",
> > +   DPRINTK("Fix direct CALL offset: 0x%x, CALL 0x%lx",
> > *(s32 *)(insnbuf + 1),
> > (unsigned long)instr + *(s32 *)(insnbuf + 1) + 
> > 5);
> > -   }
> >  
> > -   if (a->replacementlen && is_jmp(replacement[0]))
> > +   } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
> > +  *(insnbuf+1) == 0x15) {
> > +   /* indirect call */
> > +   *(s32 *)(insnbuf + 2) += replacement - instr;
> > +   DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
> > +   *(s32 *)(insnbuf + 2),
> > +   (unsigned long)instr + *(s32 *)(insnbuf + 2) + 
> > 6);
> > +
> > +   } else if (a->replacementlen && is_jmp(replacement[0])) {
> 
> Is this correct? Without your patch this was:
> 
> if (*insnbuf == 0xe8) ...
> if (is_jmp(replacement[0])) ...
> 
> Now you have:
> 
> if (*insnbuf == 0xe8) ...
> else if (*insnbuf == 0xff15) ...
> else if (is_jmp(replacement[0])) ...
> 
> So only one or none of the three variants will be executed. In the past
> it could be none, one or both.

It can't be a call *and* a jump.  It's either one or the other.

Maybe it's a little confusing that the jump check uses replacement[0]
while the other checks use *insnbuf?  They have the same value, so the
same variable should probably be used everywhere for consistency.  I can
make them more consistent.

-- 
Josh

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

Re: [Xen-devel] [PATCH 03/13] x86/paravirt: Convert native patch assembly code strings to macros

2017-11-16 Thread Josh Poimboeuf 
On Wed, Oct 25, 2017 at 11:46:18AM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Convert the hard-coded native patch assembly code strings to macros to
> > facilitate sharing common code between 32-bit and 64-bit.
> > 
> > These macros will also be used by a future patch which requires the GCC
> > extended asm syntax of two '%' characters instead of one when specifying
> > a register name.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
> 
> Reviewed-by: Juergen Gross <jgr...@suse.com>
> 
> Mind adding another patch to merge the now nearly identical
> paravirt_patch_32.c and paravirt_patch_64.c either into paravirt.c
> or paravirt_patch.c? This would require only very few #ifdef.

Good idea, will do.

-- 
Josh

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

Re: [Xen-devel] [PATCH 02/13] x86/paravirt: Fix output constraint macro names

2017-11-16 Thread Josh Poimboeuf 
On Wed, Oct 25, 2017 at 11:33:43AM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Some of the paravirt '*_CLOBBERS' macros refer to output constraints
> > instead of clobbers, which makes the code extra confusing.  Rename the
> > output constraint related macros to '*_OUTPUTS'.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
> 
> I'm fine with the changes, but you might want to rename the "call_clbr"
> parameter of PVOP_[V]CALL, too, e.g. to "outputs".

Yeah, good catch.

> You could then drop the "CALL_" from the macros, too.

Hm, which macros are you referring to, and why?

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-17 Thread Josh Poimboeuf 
On Tue, Oct 17, 2017 at 04:59:41PM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 04:50 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> >>> On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> >>>> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>>>> Maybe we can add a new field to the alternatives entry struct which
> >>>>> specifies the offset to the CALL instruction, so apply_alternatives()
> >>>>> can find it.
> >>>> We'd also have to assume that the restore part of an alternative entry
> >>>> is the same size as the save part. Which is true now.
> >>> Why?
> >>>
> >> Don't you need to know the size of the instruction without save and
> >> restore part?
> >>
> >> + if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
> >>
> >> Otherwise you'd need another field for the actual instruction length.
> > If we know where the CALL instruction starts, and can verify that it
> > starts with "ff 15", then we know the instruction length: 6 bytes.
> > Right?
> >
> 
> Oh, OK. Then you shouldn't need a->replacementlen test(s?) in
> apply_alternatives()?

Right.  Though in the above code it was needed for a different reason,
to make sure it wasn't reading past the end of the buffer.

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-17 Thread Josh Poimboeuf 
On Tue, Oct 17, 2017 at 04:36:00PM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 04:17 PM, Josh Poimboeuf wrote:
> > On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> >> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >>> Maybe we can add a new field to the alternatives entry struct which
> >>> specifies the offset to the CALL instruction, so apply_alternatives()
> >>> can find it.
> >> We'd also have to assume that the restore part of an alternative entry
> >> is the same size as the save part. Which is true now.
> > Why?
> >
> 
> Don't you need to know the size of the instruction without save and
> restore part?
> 
> + if (a->replacementlen == 6 && *insnbuf == 0xff && *(insnbuf+1) == 0x15)
> 
> Otherwise you'd need another field for the actual instruction length.

If we know where the CALL instruction starts, and can verify that it
starts with "ff 15", then we know the instruction length: 6 bytes.
Right?

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-17 Thread Josh Poimboeuf 
On Tue, Oct 17, 2017 at 11:36:57AM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 10:36 AM, Josh Poimboeuf wrote:
> >
> > Maybe we can add a new field to the alternatives entry struct which
> > specifies the offset to the CALL instruction, so apply_alternatives()
> > can find it.
> 
> We'd also have to assume that the restore part of an alternative entry
> is the same size as the save part. Which is true now.

Why?

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-17 Thread Josh Poimboeuf 
On Tue, Oct 17, 2017 at 09:58:59AM -0400, Boris Ostrovsky wrote:
> On 10/17/2017 01:24 AM, Josh Poimboeuf wrote:
> > On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
> >> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
> >>> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
> >>>> On 12/10/17 20:11, Boris Ostrovsky wrote:
> >>>>> There is also another problem:
> >>>>>
> >>>>> [1.312425] general protection fault:  [#1] SMP
> >>>>> [1.312901] Modules linked in:
> >>>>> [1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
> >>>>> [1.313878] task: 88003e2c task.stack: c938c000
> >>>>> [1.314360] RIP: 1e030:entry_SYSCALL_64_fastpath+0x1/0xa5
> >>>>> [1.314854] RSP: e02b:c938ff50 EFLAGS: 00010046
> >>>>> [1.315336] RAX: 000c RBX: 55f550168040 RCX:
> >>>>> 7fcfc959f59a
> >>>>> [1.315827] RDX:  RSI:  RDI:
> >>>>> 
> >>>>> [1.316315] RBP: 000a R08: 037f R09:
> >>>>> 0064
> >>>>> [1.316805] R10: 1f89cbf5 R11: 88003e2c R12:
> >>>>> 7fcfc958ad60
> >>>>> [1.317300] R13:  R14: 55f550185954 R15:
> >>>>> 1000
> >>>>> [1.317801] FS:  () GS:88003f80()
> >>>>> knlGS:
> >>>>> [1.318267] CS:  e033 DS:  ES:  CR0: 80050033
> >>>>> [1.318750] CR2: 7fcfc97ab218 CR3: 3c88e000 CR4:
> >>>>> 00042660
> >>>>> [1.319235] Call Trace:
> >>>>> [1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
> >>>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
> >>>>> 00 50  15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
> >>>>> [1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: 
> >>>>> c938ff50
> >>>>> [1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
> >>>>> [1.345009] Kernel panic - not syncing: Attempted to kill init!
> >>>>> exitcode=0x000b
> >>>>>
> >>>>>
> >>>>> All code
> >>>>> 
> >>>>>0:51   push   %rcx
> >>>>>1:50   push   %rax
> >>>>>2:57   push   %rdi
> >>>>>3:56   push   %rsi
> >>>>>4:52   push   %rdx
> >>>>>5:51   push   %rcx
> >>>>>6:6a dapushq  $0xffda
> >>>>>8:41 50push   %r8
> >>>>>a:41 51push   %r9
> >>>>>c:41 52push   %r10
> >>>>>e:41 53push   %r11
> >>>>>   10:48 83 ec 30  sub$0x30,%rsp
> >>>>>   14:65 4c 8b 1c 25 c0 d2 mov%gs:0xd2c0,%r11
> >>>>>   1b:00 00
> >>>>>   1d:41 f7 03 df 39 08 90 testl  $0x900839df,(%r11)
> >>>>>   24:0f 85 a5 00 00 00jne0xcf
> >>>>>   2a:50   push   %rax
> >>>>>   2b:*ff 15 9c 95 d0 ffcallq  *-0x2f6a64(%rip)#
> >>>>> 0xffd095cd<-- trapping instruction
> >>>>>   31:58   pop%rax
> >>>>>   32:48 3d 4c 01 00 00cmp$0x14c,%rax
> >>>>>   38:77 0fja 0x49
> >>>>>   3a:4c 89 d1 mov%r10,%rcx
> >>>>>   3d:ff   .byte 0xff
> >>>>>   3e:14 c5adc$0xc5,%al
> >>>>>
> >>>>>
> >>>>> so the original 'cli' was replaced with the pv call but to me the offset
> >>>>> looks a bit off, no? Shouldn't it always be positive?
> >>>> callq takes a 32bit signed displacement, so jumping back by

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-16 Thread Josh Poimboeuf 
On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
> > On 10/12/2017 03:27 PM, Andrew Cooper wrote:
> >> On 12/10/17 20:11, Boris Ostrovsky wrote:
> >>> There is also another problem:
> >>>
> >>> [1.312425] general protection fault:  [#1] SMP
> >>> [1.312901] Modules linked in:
> >>> [1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
> >>> [1.313878] task: 88003e2c task.stack: c938c000
> >>> [1.314360] RIP: 1e030:entry_SYSCALL_64_fastpath+0x1/0xa5
> >>> [1.314854] RSP: e02b:c938ff50 EFLAGS: 00010046
> >>> [1.315336] RAX: 000c RBX: 55f550168040 RCX:
> >>> 7fcfc959f59a
> >>> [1.315827] RDX:  RSI:  RDI:
> >>> 
> >>> [1.316315] RBP: 000a R08: 037f R09:
> >>> 0064
> >>> [1.316805] R10: 1f89cbf5 R11: 88003e2c R12:
> >>> 7fcfc958ad60
> >>> [1.317300] R13:  R14: 55f550185954 R15:
> >>> 1000
> >>> [1.317801] FS:  () GS:88003f80()
> >>> knlGS:
> >>> [1.318267] CS:  e033 DS:  ES:  CR0: 80050033
> >>> [1.318750] CR2: 7fcfc97ab218 CR3: 3c88e000 CR4:
> >>> 00042660
> >>> [1.319235] Call Trace:
> >>> [1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
> >>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
> >>> 00 50  15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
> >>> [1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: 
> >>> c938ff50
> >>> [1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
> >>> [1.345009] Kernel panic - not syncing: Attempted to kill init!
> >>> exitcode=0x000b
> >>>
> >>>
> >>> All code
> >>> 
> >>>0:51   push   %rcx
> >>>1:50   push   %rax
> >>>2:57   push   %rdi
> >>>3:56   push   %rsi
> >>>4:52   push   %rdx
> >>>5:51   push   %rcx
> >>>6:6a dapushq  $0xffda
> >>>8:41 50push   %r8
> >>>a:41 51push   %r9
> >>>c:41 52push   %r10
> >>>e:41 53push   %r11
> >>>   10:48 83 ec 30  sub$0x30,%rsp
> >>>   14:65 4c 8b 1c 25 c0 d2 mov%gs:0xd2c0,%r11
> >>>   1b:00 00
> >>>   1d:41 f7 03 df 39 08 90 testl  $0x900839df,(%r11)
> >>>   24:0f 85 a5 00 00 00jne0xcf
> >>>   2a:50   push   %rax
> >>>   2b:*ff 15 9c 95 d0 ffcallq  *-0x2f6a64(%rip)#
> >>> 0xffd095cd<-- trapping instruction
> >>>   31:58   pop%rax
> >>>   32:48 3d 4c 01 00 00cmp$0x14c,%rax
> >>>   38:77 0fja 0x49
> >>>   3a:4c 89 d1 mov%r10,%rcx
> >>>   3d:ff   .byte 0xff
> >>>   3e:14 c5adc$0xc5,%al
> >>>
> >>>
> >>> so the original 'cli' was replaced with the pv call but to me the offset
> >>> looks a bit off, no? Shouldn't it always be positive?
> >> callq takes a 32bit signed displacement, so jumping back by up to 2G is
> >> perfectly legitimate.
> > Yes, but
> >
> > ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
> > 817365dd t entry_SYSCALL_64_fastpath
> > ostr@workbase> nm vmlinux | grep " pv_irq_ops"
> > 81c2dbc0 D pv_irq_ops
> > ostr@workbase>
> >
> > so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
> > didn't mean that x86 instruction set doesn't allow negative
> > displacement, I was trying to say that pv_irq_ops always live further down)
> 
> I believe the problem is this:
> 
> #define PV_INDIRECT(addr)   *addr(%rip)
> 
> The displacement that the linker computes will be relative to the where
> this instruction is placed at the time of linking, which is in
> .pv_altinstructions (and not .text). So when we copy it into .text the
> displacement becomes bogus.

apply_alternatives() is supposed to adjust that displacement based on
the new IP, though it could be messing that up somehow.  (See patch
10/13.)

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-06 Thread Josh Poimboeuf 
On Fri, Oct 06, 2017 at 11:29:52AM -0400, Boris Ostrovsky wrote:
> >>> +
> >>>  void __init_or_module apply_paravirt(struct paravirt_patch_site *start,
> >>>struct paravirt_patch_site *end)
> >>>  {
> >>> diff --git a/arch/x86/kernel/cpu/hypervisor.c 
> >>> b/arch/x86/kernel/cpu/hypervisor.c
> >>> index 4fa90006ac68..17243fe0f5ce 100644
> >>> --- a/arch/x86/kernel/cpu/hypervisor.c
> >>> +++ b/arch/x86/kernel/cpu/hypervisor.c
> >>> @@ -71,6 +71,8 @@ void __init init_hypervisor_platform(void)
> >>>   if (!x86_hyper)
> >>>   return;
> >>>  
> >>> + apply_pv_alternatives();
> >> Not for Xen PV guests who have already done this.
> > I think it would be harmless, but yeah, it's probably best to only write
> > it once.
> 
> I also wonder whether calling apply_pv_alternatives() here before
> x86_hyper->init_platform() will work since the latter may be setting
> those op. In fact, that's what Xen HVM does for pv_mmu_ops.exit_mmap.

apply_pv_alternatives() changes:

  (native code)

to

  call *pv_whatever_ops.whatever

So apply_pv_alternatives() should be called *before* any of the ops are
set up.

-- 
Josh

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

Re: [Xen-devel] [PATCH v4 01/27] linkage: new macros for assembler symbols

2017-10-06 Thread Josh Poimboeuf 
On Mon, Oct 02, 2017 at 11:12:20AM +0200, Jiri Slaby wrote:
>SYM_CODE_INNER_LABEL -- only for labels in the middle of code
>SYM_CODE_INNER_LABEL_NOALIGN -- only for labels in the middle of code

Why are the inner labels aligned by default?  Seems like unaligned would
be the most common case.

> 
> d) For data
>SYM_DATA_START -- global data symbol
>SYM_DATA_END -- the end of the SYM_DATA_START symbol
>SYM_DATA_END_LABEL -- the labeled end of SYM_DATA_START symbol
>SYM_DATA_SIMPLE -- start+end wrapper around simple global data
>SYM_DATA_SIMPLE_LOCAL -- start+end wrapper around simple local data

"SIMPLE" seems superfluous, how about s/SYM_DATA_SIMPLE/SYM_DATA/ ?

-- 
Josh

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

Re: [Xen-devel] [PATCH 00/13] x86/paravirt: Make pv ops code generation more closely match reality

2017-10-06 Thread Josh Poimboeuf 
On Fri, Oct 06, 2017 at 09:35:16AM +0200, Vitaly Kuznetsov wrote:
> Josh Poimboeuf <jpoim...@redhat.com> writes:
> 
> > - For the most common runtime cases (everything except Xen and vSMP),
> >   vmlinux disassembly now matches what the actual runtime code looks
> >   like.  This improves debuggability and kernel developer sanity (a
> >   precious resource).
> >
> > ...
> >
> > - It's hopefully a first step in simplifying paravirt patching by
> >   getting rid of .parainstructions, pv ops, and apply_paravirt()
> >   completely.  (I think Xen can be changed to set CPU feature bits to
> >   specify which ops it needs during early boot, then those ops can be
> >   patched in using early alternatives.)
> 
> JFYI starting 4.14 Xen PV is not the only user of pv_mmu_ops, Hyper-V
> uses it for TLB shootdown now.

Yeah, I saw that.  It should be fine because the pv_alternatives get
patched before the Hyper-V code sets up pv_mmu_ops.

-- 
Josh

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

Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-06 Thread Josh Poimboeuf 
On Thu, Oct 05, 2017 at 04:35:03PM -0400, Boris Ostrovsky wrote:
> 
> >  #ifdef CONFIG_PARAVIRT
> > +/*
> > + * Paravirt alternatives are applied much earlier than normal alternatives.
> > + * They are only applied when running on a hypervisor.  They replace some
> > + * native instructions with calls to pv ops.
> > + */
> > +void __init apply_pv_alternatives(void)
> > +{
> > +   setup_force_cpu_cap(X86_FEATURE_PV_OPS);
> 
> Not for Xen HVM guests.

From what I can tell, HVM guests still use pv_time_ops and
pv_mmu_ops.exit_mmap, right?

> > +   apply_alternatives(__pv_alt_instructions, __pv_alt_instructions_end);
> > +}
> 
> 
> This is a problem (at least for Xen PV guests):
> apply_alternatives()->text_poke_early()->local_irq_save()->...'cli'->death.

Ah, right.

> It might be possible not to turn off/on the interrupts in this
> particular case since the guest probably won't be able to handle an
> interrupt at this point anyway.

Yeah, that should work.  For Xen and for the other hypervisors, this is
called well before irq init, so interrupts can't be handled yet anyway.

> > +
> >  void __init_or_module apply_paravirt(struct paravirt_patch_site *start,
> >  struct paravirt_patch_site *end)
> >  {
> > diff --git a/arch/x86/kernel/cpu/hypervisor.c 
> > b/arch/x86/kernel/cpu/hypervisor.c
> > index 4fa90006ac68..17243fe0f5ce 100644
> > --- a/arch/x86/kernel/cpu/hypervisor.c
> > +++ b/arch/x86/kernel/cpu/hypervisor.c
> > @@ -71,6 +71,8 @@ void __init init_hypervisor_platform(void)
> > if (!x86_hyper)
> > return;
> >  
> > +   apply_pv_alternatives();
> 
> Not for Xen PV guests who have already done this.

I think it would be harmless, but yeah, it's probably best to only write
it once.

Thanks for the review!

-- 
Josh

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

[Xen-devel] [PATCH 13/13] x86/paravirt: Convert natively patched pv ops to use paravirt alternatives

2017-10-04 Thread Josh Poimboeuf 
Now that the paravirt alternatives infrastructure is in place, use it
for all natively patched pv ops.

This fixes KASAN warnings in the ORC unwinder like the following:

  BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x123/0x140

This also improves debuggability by making vmlinux more likely to match
reality.

Reported-by: Sasha Levin <alexander.le...@verizon.com>
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt-asm.h | 23 +--
 arch/x86/include/asm/paravirt.h | 37 +
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/paravirt-asm.h 
b/arch/x86/include/asm/paravirt-asm.h
index a8139ea27cc1..b051f9254ace 100644
--- a/arch/x86/include/asm/paravirt-asm.h
+++ b/arch/x86/include/asm/paravirt-asm.h
@@ -86,16 +86,18 @@
pv_cpu_ops, PV_CPU_iret, CLBR_NONE)
 
 #define DISABLE_INTERRUPTS(clobbers)   \
-   PV_SITE(PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);  \
-   call PV_INDIRECT(pv_irq_ops+PV_IRQ_irq_disable);\
-   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE),   \
-   pv_irq_ops, PV_IRQ_irq_disable, clobbers)
+   PV_ALT_SITE(cli,\
+   PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);  \
+   call PV_INDIRECT(pv_irq_ops+PV_IRQ_irq_disable);\
+   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE),   \
+   pv_irq_ops, PV_IRQ_irq_disable, clobbers)
 
 #define ENABLE_INTERRUPTS(clobbers)\
-   PV_SITE(PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);  \
-   call PV_INDIRECT(pv_irq_ops+PV_IRQ_irq_enable); \
-   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE),   \
-   pv_irq_ops, PV_IRQ_irq_enable, clobbers)
+   PV_ALT_SITE(sti,\
+   PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);  \
+   call PV_INDIRECT(pv_irq_ops+PV_IRQ_irq_enable); \
+   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE),   \
+   pv_irq_ops, PV_IRQ_irq_enable, clobbers)
 
 #ifdef CONFIG_X86_32
 
@@ -128,8 +130,9 @@
call PV_INDIRECT(pv_mmu_ops+PV_MMU_read_cr2)
 
 #define USERGS_SYSRET64
\
-   PV_SITE(jmp PV_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64), \
-   pv_cpu_ops, PV_CPU_usergs_sysret64, CLBR_NONE)
+   PV_ALT_SITE(swapgs; sysret, \
+   jmp PV_INDIRECT(pv_cpu_ops+PV_CPU_usergs_sysret64), \
+   pv_cpu_ops, PV_CPU_usergs_sysret64, CLBR_NONE)
 
 #endif /* !CONFIG_X86_32 */
 
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index bfd02c3335cb..4216a3b02832 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static inline void load_sp0(struct tss_struct *tss,
 struct thread_struct *thread)
@@ -50,9 +51,10 @@ static inline void write_cr0(unsigned long x)
PVOP_VCALL1(pv_cpu_ops.write_cr0, x);
 }
 
-static inline unsigned long read_cr2(void)
+static __always_inline unsigned long read_cr2(void)
 {
-   return PVOP_CALL0(unsigned long, pv_mmu_ops.read_cr2);
+   return PVOP_ALT_CALL0(unsigned long, NATIVE_READ_CR2,
+ pv_mmu_ops.read_cr2);
 }
 
 static inline void write_cr2(unsigned long x)
@@ -60,14 +62,15 @@ static inline void write_cr2(unsigned long x)
PVOP_VCALL1(pv_mmu_ops.write_cr2, x);
 }
 
-static inline unsigned long __read_cr3(void)
+static __always_inline unsigned long __read_cr3(void)
 {
-   return PVOP_CALL0(unsigned long, pv_mmu_ops.read_cr3);
+   return PVOP_ALT_CALL0(unsigned long, NATIVE_READ_CR3,
+ pv_mmu_ops.read_cr3);
 }
 
-static inline void write_cr3(unsigned long x)
+static __always_inline void write_cr3(unsigned long x)
 {
-   PVOP_VCALL1(pv_mmu_ops.write_cr3, x);
+   PVOP_ALT_VCALL1(NATIVE_WRITE_CR3, pv_mmu_ops.write_cr3, x);
 }
 
 static inline void __write_cr4(unsigned long x)
@@ -291,9 +294,10 @@ static inline void __flush_tlb_global(void)
 {
PVOP_VCALL0(pv_mmu_ops.flush_tlb_kernel);
 }
-static inline void __flush_tlb_single(unsigned long addr)
+static __always_inline void __flush_tlb_single(unsigned long addr)
 {
-   PVOP_VCALL1(pv_mmu_ops.flush_tlb_single, addr);
+   PVOP_ALT_VCALL1(NATIVE_FLUSH_TLB_SINGLE, pv_mmu_ops.flush_tlb_single,
+   addr);
 }
 
 static inline void flush_tlb_others(const struct cpumask *cpumask,
@@ -761,24 +765,25 @@ static __always_inline bool pv_vcpu_is_preempted(long c

[Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure

2017-10-04 Thread Josh Poimboeuf 
With CONFIG_PARAVIRT, the kernel .text is littered with a bunch of calls
to pv_irq_ops function pointers, like:

  callq  *0x81e3a400 (pv_irq_ops.save_fl)

In non-Xen paravirt environments -- including native, KVM, Hyper-V, and
VMware -- the above code gets patched by native_patch() to look like
this instead:

   pushfq
   pop%rax
   nopl   0x0(%rax,%rax,1)

So in most scenarios, there's a mismatch between what vmlinux shows and
the actual runtime code.  This mismatch hurts debuggability and makes
the assembly code harder to understand.

It also causes the ORC unwinder to produce KASAN warnings like:

  BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x123/0x140

This warning is due to the fact that objtool doesn't know about
parainstructions, so it doesn't know about the "pushfq; pop %rax"
sequence above.

Prepare to fix both of these issues (debuggability and ORC KASAN
warnings) by adding a paravirt alternatives infrastructure to put the
native instructions in .text by default.  Then, when booting on a
hypervisor, replace the native instructions with pv ops calls.

The pv ops calls need to be available much earlier than when
alternatives are normally applied.  So put these alternatives in a
dedicated ".pv_alternatives" section.

So now these instructions may be patched twice:

- in apply_pv_alternatives(), to allow the kernel to boot in the
  virtualized environment;

- and again in apply_paravirt(), to enable performance improvements
  (e.g., replacing an indirect call with a direct call).

That's a bit more complex, but overall this approach should cause less
confusion than before because the vmlinux code is now much more likely
to represent the actual runtime state of the code in the most common
paravirt cases (everything except Xen and vSMP).

It could be simplified by redesigning the paravirt patching code such
that it uses alternatives for all of its patching.  Instead of using pv
ops to specify which functions they need, they would instead set CPU
feature bits, which would then be used by the alternatives to decide
what to replace the native code with.  Then each site would only be
patched once.

But that's going to be a bit more work.  At least this patch creates a
good foundation for eventually getting rid of .parainstructions and pv
ops completely.

Suggested-by: Andy Lutomirski <l...@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/alternative-asm.h |  9 +++-
 arch/x86/include/asm/alternative.h | 12 +++--
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/include/asm/paravirt-asm.h| 10 
 arch/x86/include/asm/paravirt_types.h  | 84 ++
 arch/x86/kernel/alternative.c  | 13 ++
 arch/x86/kernel/cpu/hypervisor.c   |  2 +
 arch/x86/kernel/module.c   | 11 -
 arch/x86/kernel/vmlinux.lds.S  |  6 +++
 arch/x86/xen/enlighten_pv.c|  1 +
 10 files changed, 141 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/alternative-asm.h 
b/arch/x86/include/asm/alternative-asm.h
index 60073947350d..0ced2e3d0a30 100644
--- a/arch/x86/include/asm/alternative-asm.h
+++ b/arch/x86/include/asm/alternative-asm.h
@@ -39,14 +39,14 @@
  * @newinstr. ".skip" directive takes care of proper instruction padding
  * in case @newinstr is longer than @oldinstr.
  */
-#define ALTERNATIVE(oldinstr, newinstr, feature)   \
+#define __ALTERNATIVE(section, oldinstr, newinstr, feature)\
 140:;  \
oldinstr;   \
 141:;  \
.skip -(((144f-143f)-(141b-140b)) > 0) *\
((144f-143f)-(141b-140b)),0x90; \
 142:;  \
-   .pushsection .altinstructions, "a"; \
+   .pushsection section, "a";  \
altinstruction_entry 140b,143f,feature,142b-140b,144f-143f,142b-141b;\
.popsection;\
.pushsection .altinstr_replacement, "ax";   \
@@ -55,6 +55,11 @@
 144:;  \
.popsection
 
+#define ARGS(args...) args
+
+#define ALTERNATIVE(oldinstr, newinstr, feature)   \
+   __ALTERNATIVE(.altinstructions, ARGS(oldinstr), ARGS(newinstr), feature)
+
 #define old_len141b-140b
 #define new_len1   144f-143f
 #define new_len2   145f-144f
diff --git a/arch/x86/include/asm/alternative.h 
b/arch/x86/include/asm/alternative.h
index c096624137ae..8482f90d5078 100644
--- a/arch/x86/includ

[Xen-devel] [PATCH 08/13] x86/paravirt: Clean up paravirt_types.h

2017-10-04 Thread Josh Poimboeuf 
Make paravirt_types.h more understandable:

- Use more consistent and logical naming
- Simplify interfaces
- Put related macros together
- Improve whitespace

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt_types.h | 104 ++
 1 file changed, 54 insertions(+), 50 deletions(-)

diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index 01f9e10983c1..5656aea79412 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -331,33 +331,6 @@ extern struct pv_irq_ops pv_irq_ops;
 extern struct pv_mmu_ops pv_mmu_ops;
 extern struct pv_lock_ops pv_lock_ops;
 
-#define PARAVIRT_PATCH(x)  \
-   (offsetof(struct paravirt_patch_template, x) / sizeof(void *))
-
-#define paravirt_type(op)  \
-   [paravirt_typenum] "i" (PARAVIRT_PATCH(op)),\
-   [paravirt_opptr] "i" (&(op))
-#define paravirt_clobber(clobber)  \
-   [paravirt_clobber] "i" (clobber)
-
-/*
- * Generate some code, and mark it as patchable by the
- * apply_paravirt() alternate instruction patcher.
- */
-#define _paravirt_alt(insn_string, type, clobber)  \
-   "771:\n\t" insn_string "\n" "772:\n"\
-   ".pushsection .parainstructions,\"a\"\n"\
-   _ASM_ALIGN "\n" \
-   _ASM_PTR " 771b\n"  \
-   "  .byte " type "\n"\
-   "  .byte 772b-771b\n"   \
-   "  .short " clobber "\n"\
-   ".popsection\n"
-
-/* Generate patchable code, with the default asm parameters. */
-#define paravirt_alt(insn_string)  \
-   _paravirt_alt(insn_string, "%c[paravirt_typenum]", 
"%c[paravirt_clobber]")
-
 /* Simple instruction patching code. */
 #define NATIVE_LABEL(a,x,b) "\n" a #x "_" #b ":\n\t"
 
@@ -388,13 +361,46 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
 
 int paravirt_disable_iospace(void);
 
+
 /*
- * This generates an indirect call based on the operation type number.
- * The type number, computed in PARAVIRT_PATCH, is derived from the
- * offset into the paravirt_patch_template structure, and can therefore be
- * freely converted back into a structure offset.
+ * Generate some code, and mark it as patchable by apply_paravirt().
  */
-#define PARAVIRT_CALL  "call *%c[paravirt_opptr];"
+#define _PV_SITE(insn_string, type, clobber)   \
+   "771:\n\t" insn_string "\n" "772:\n"\
+   ".pushsection .parainstructions,\"a\"\n"\
+   _ASM_ALIGN "\n" \
+   _ASM_PTR " 771b\n"  \
+   "  .byte " type "\n"\
+   "  .byte 772b-771b\n"   \
+   "  .short " clobber "\n"\
+   ".popsection\n"
+
+#define PARAVIRT_PATCH(x)  \
+   (offsetof(struct paravirt_patch_template, x) / sizeof(void *))
+
+#define PV_STRINGIFY(constraint)   "%c[" __stringify(constraint) "]"
+
+#define PV_CALL_CONSTRAINT pv_op_ptr
+#define PV_TYPE_CONSTRAINT pv_typenum
+#define PV_CLBR_CONSTRAINT pv_clobber
+
+#define PV_CALL_CONSTRAINT_STR PV_STRINGIFY(PV_CALL_CONSTRAINT)
+#define PV_TYPE_CONSTRAINT_STR PV_STRINGIFY(PV_TYPE_CONSTRAINT)
+#define PV_CLBR_CONSTRAINT_STR PV_STRINGIFY(PV_CLBR_CONSTRAINT)
+
+#define PV_CALL_STR"call *" PV_CALL_CONSTRAINT_STR ";"
+
+#define PV_INPUT_CONSTRAINTS(op, clobber)  \
+   [PV_TYPE_CONSTRAINT] "i" (PARAVIRT_PATCH(op)),  \
+   [PV_CALL_CONSTRAINT] "i" (&(op)),   \
+   [PV_CLBR_CONSTRAINT] "i" (clobber)
+
+#define PV_SITE(insn_string)   \
+   _PV_SITE(insn_string, PV_TYPE_CONSTRAINT_STR, PV_CLBR_CONSTRAINT_STR)
+
+#define PV_ALT_SITE(oldinstr, newinstr)
\
+   _PV_ALT_SITE(oldinstr, newinstr, PV_TYPE_CONSTRAINT_STR,\
+PV_CLBR_CONSTRAINT_STR)
 
 /*
  * These macros are intended to

[Xen-devel] [PATCH 09/13] x86/asm: Convert ALTERNATIVE*() assembler macros to preprocessor macros

2017-10-04 Thread Josh Poimboeuf 
The ALTERNATIVE() and ALTERNATIVE_2() macros are GNU assembler macros,
which makes them quite inflexible for future changes.  Convert them to
preprocessor macros.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/entry/entry_32.S| 12 +++---
 arch/x86/entry/entry_64.S| 10 ++---
 arch/x86/entry/entry_64_compat.S |  8 ++--
 arch/x86/entry/vdso/vdso32/system_call.S | 10 ++---
 arch/x86/include/asm/alternative-asm.h   | 68 +++-
 arch/x86/include/asm/smap.h  |  4 +-
 arch/x86/lib/copy_page_64.S  |  2 +-
 arch/x86/lib/memcpy_64.S |  4 +-
 arch/x86/lib/memmove_64.S|  3 +-
 arch/x86/lib/memset_64.S |  4 +-
 10 files changed, 59 insertions(+), 66 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 21d1197779a4..338dc838a9a8 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -443,8 +443,8 @@ ENTRY(entry_SYSENTER_32)
movl%esp, %eax
calldo_fast_syscall_32
/* XEN PV guests always use IRET path */
-   ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
-   "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
+   #define JMP_IF_IRET testl %eax, %eax; jz .Lsyscall_32_done
+   ALTERNATIVE(JMP_IF_IRET, jmp .Lsyscall_32_done, X86_FEATURE_XENPV)
 
 /* Opportunistic SYSEXIT */
TRACE_IRQS_ON   /* User mode traces as IRQs on. */
@@ -536,7 +536,7 @@ restore_all:
TRACE_IRQS_IRET
 .Lrestore_all_notrace:
 #ifdef CONFIG_X86_ESPFIX32
-   ALTERNATIVE "jmp .Lrestore_nocheck", "", X86_BUG_ESPFIX
+   ALTERNATIVE(jmp .Lrestore_nocheck, , X86_BUG_ESPFIX)
 
movlPT_EFLAGS(%esp), %eax   # mix EFLAGS, SS and CS
/*
@@ -692,9 +692,9 @@ ENTRY(simd_coprocessor_error)
pushl   $0
 #ifdef CONFIG_X86_INVD_BUG
/* AMD 486 bug: invd from userspace calls exception 19 instead of #GP */
-   ALTERNATIVE "pushl  $do_general_protection",\
-   "pushl  $do_simd_coprocessor_error",\
-   X86_FEATURE_XMM
+   ALTERNATIVE(pushl   $do_general_protection,
+   pushl   $do_simd_coprocessor_error,
+   X86_FEATURE_XMM)
 #else
pushl   $do_simd_coprocessor_error
 #endif
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index c7c85724d7e0..49733c72619a 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -925,7 +925,7 @@ ENTRY(native_load_gs_index)
SWAPGS
 .Lgs_change:
movl%edi, %gs
-2: ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
+2: ALTERNATIVE(, mfence, X86_BUG_SWAPGS_FENCE)
SWAPGS
popfq
FRAME_END
@@ -938,12 +938,8 @@ EXPORT_SYMBOL(native_load_gs_index)
/* running with kernelgs */
 bad_gs:
SWAPGS  /* switch back to user gs */
-.macro ZAP_GS
-   /* This can't be a string because the preprocessor needs to see it. */
-   movl $__USER_DS, %eax
-   movl %eax, %gs
-.endm
-   ALTERNATIVE "", "ZAP_GS", X86_BUG_NULL_SEG
+   #define ZAP_GS movl $__USER_DS, %eax; movl %eax, %gs
+   ALTERNATIVE(, ZAP_GS, X86_BUG_NULL_SEG)
xorl%eax, %eax
movl%eax, %gs
jmp 2b
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 4d9385529c39..16e82b5103b5 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -124,8 +124,8 @@ ENTRY(entry_SYSENTER_compat)
movq%rsp, %rdi
calldo_fast_syscall_32
/* XEN PV guests always use IRET path */
-   ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
-   "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
+   #define JMP_IF_IRET testl %eax, %eax; jz .Lsyscall_32_done
+   ALTERNATIVE(JMP_IF_IRET, jmp .Lsyscall_32_done, X86_FEATURE_XENPV)
jmp sysret32_from_system_call
 
 .Lsysenter_fix_flags:
@@ -224,8 +224,8 @@ GLOBAL(entry_SYSCALL_compat_after_hwframe)
movq%rsp, %rdi
calldo_fast_syscall_32
/* XEN PV guests always use IRET path */
-   ALTERNATIVE "testl %eax, %eax; jz .Lsyscall_32_done", \
-   "jmp .Lsyscall_32_done", X86_FEATURE_XENPV
+   ALTERNATIVE(JMP_IF_IRET,
+   jmp .Lsyscall_32_done, X86_FEATURE_XENPV)
 
/* Opportunistic SYSRET */
 sysret32_from_system_call:
diff --git a/arch/x86/entry/vdso/vdso32/system_call.S 
b/arch/x86/entry/vdso/vdso32/system_call.S
index ed4bc9731cbb..a0c5f9e8226c 100644
--- a/arch/x86/entry/vdso/vdso32/system_call.S
+++ b/arch/x86/entry/vdso/vdso32/system_call.S
@@ -48,15 +48,15 @@ __kernel_vsyscall:
CFI_ADJUST_CFA_OFFSET

[Xen-devel] [PATCH 05/13] x86/paravirt: Move paravirt asm macros to paravirt-asm.h

2017-10-04 Thread Josh Poimboeuf 
The paravirt.h file is quite big and the asm interfaces for paravirt
don't need to be in the same file as the C interfaces.  Move the asm
interfaces to a dedicated header file.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/entry/entry_32.S   |   1 +
 arch/x86/entry/entry_64.S   |   2 +-
 arch/x86/entry/entry_64_compat.S|   1 +
 arch/x86/include/asm/paravirt-asm.h | 126 ++
 arch/x86/include/asm/paravirt.h | 132 +++-
 arch/x86/kernel/head_64.S   |   2 +-
 6 files changed, 138 insertions(+), 126 deletions(-)
 create mode 100644 arch/x86/include/asm/paravirt-asm.h

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 8a13d468635a..21d1197779a4 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -40,6 +40,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 49167258d587..c7c85724d7e0 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -30,7 +30,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index e26c25ca7756..4d9385529c39 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
diff --git a/arch/x86/include/asm/paravirt-asm.h 
b/arch/x86/include/asm/paravirt-asm.h
new file mode 100644
index ..add8a190fdac
--- /dev/null
+++ b/arch/x86/include/asm/paravirt-asm.h
@@ -0,0 +1,126 @@
+#ifndef _ASM_X86_PARAVIRT_ASM_H
+#define _ASM_X86_PARAVIRT_ASM_H
+
+#ifdef CONFIG_PARAVIRT
+#ifdef __ASSEMBLY__
+
+#include 
+#include 
+
+#define _PVSITE(ptype, clobbers, ops, word, algn)  \
+771:;  \
+   ops;\
+772:;  \
+   .pushsection .parainstructions,"a"; \
+.align algn;   \
+word 771b; \
+.byte ptype;   \
+.byte 772b-771b;   \
+.short clobbers;   \
+   .popsection
+
+
+#define COND_PUSH(set, mask, reg)  \
+   .if ((~(set)) & mask); push %reg; .endif
+#define COND_POP(set, mask, reg)   \
+   .if ((~(set)) & mask); pop %reg; .endif
+
+#ifdef CONFIG_X86_64
+
+#define PV_SAVE_REGS(set)  \
+   COND_PUSH(set, CLBR_RAX, rax);  \
+   COND_PUSH(set, CLBR_RCX, rcx);  \
+   COND_PUSH(set, CLBR_RDX, rdx);  \
+   COND_PUSH(set, CLBR_RSI, rsi);  \
+   COND_PUSH(set, CLBR_RDI, rdi);  \
+   COND_PUSH(set, CLBR_R8, r8);\
+   COND_PUSH(set, CLBR_R9, r9);\
+   COND_PUSH(set, CLBR_R10, r10);  \
+   COND_PUSH(set, CLBR_R11, r11)
+#define PV_RESTORE_REGS(set)   \
+   COND_POP(set, CLBR_R11, r11);   \
+   COND_POP(set, CLBR_R10, r10);   \
+   COND_POP(set, CLBR_R9, r9); \
+   COND_POP(set, CLBR_R8, r8); \
+   COND_POP(set, CLBR_RDI, rdi);   \
+   COND_POP(set, CLBR_RSI, rsi);   \
+   COND_POP(set, CLBR_RDX, rdx);   \
+   COND_POP(set, CLBR_RCX, rcx);   \
+   COND_POP(set, CLBR_RAX, rax)
+
+#define PARA_PATCH(struct, off)((PARAVIRT_PATCH_##struct + (off)) / 8)
+#define PARA_SITE(ptype, clobbers, ops) _PVSITE(ptype, clobbers, ops, .quad, 8)
+#define PARA_INDIRECT(addr)*addr(%rip)
+#else
+#define PV_SAVE_REGS(set)  \
+   COND_PUSH(set, CLBR_EAX, eax);  \
+   COND_PUSH(set, CLBR_EDI, edi);  \
+   COND_PUSH(set, CLBR_ECX, ecx);  \
+   COND_PUSH(set, CLBR_EDX, edx)
+#define PV_RESTORE_REGS(set)   \
+   COND_POP(set, CLBR_EDX, edx);   \
+   COND_POP(set, CLBR_ECX, ecx);   \
+   COND_POP(set, CLBR_EDI, edi);   \
+   COND_POP(set, CLBR_EAX, eax)
+
+#define PARA_PATCH(struct, off)((PARAVIRT_PATCH_##struct + (off)) / 4)
+#define PARA_SITE(ptype, clobbers, ops) _PVSITE(ptype, clobbers, ops, .long, 4)
+#define PARA_INDIRECT(addr)*%cs:addr
+#endif
+
+#define INTERRUPT_RETURN   \
+   PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_iret), CLBR_NONE,   \
+ jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_iret))
+
+#define DISABLE_INTERRUPTS(clobbers)   \
+   PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_irq_disable), clobbers, \
+ PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);\
+ call

[Xen-devel] [PATCH 06/13] x86/paravirt: Clean up paravirt-asm.h

2017-10-04 Thread Josh Poimboeuf 
Some cleanup to make the code easier to read and understand:

- Use the common "PV_" prefix
- Simplify the PV_SITE macro interface
- Improve whitespace

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt-asm.h | 95 +++--
 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/arch/x86/include/asm/paravirt-asm.h 
b/arch/x86/include/asm/paravirt-asm.h
index add8a190fdac..8bdd50ee4bf3 100644
--- a/arch/x86/include/asm/paravirt-asm.h
+++ b/arch/x86/include/asm/paravirt-asm.h
@@ -7,16 +7,18 @@
 #include 
 #include 
 
-#define _PVSITE(ptype, clobbers, ops, word, algn)  \
-771:;  \
-   ops;\
-772:;  \
-   .pushsection .parainstructions,"a"; \
-.align algn;   \
-word 771b; \
-.byte ptype;   \
-.byte 772b-771b;   \
-.short clobbers;   \
+#define PV_TYPE(ops, off) ((PARAVIRT_PATCH_##ops + (off)) / __ASM_SEL(4, 8))
+
+#define PV_SITE(insns, ops, off, clobbers) \
+771:;  \
+   insns;  \
+772:;  \
+   .pushsection .parainstructions, "a";\
+_ASM_ALIGN;\
+_ASM_PTR 771b; \
+.byte PV_TYPE(ops, off);   \
+.byte 772b-771b;   \
+.short clobbers;   \
.popsection
 
 
@@ -33,62 +35,65 @@
COND_PUSH(set, CLBR_RDX, rdx);  \
COND_PUSH(set, CLBR_RSI, rsi);  \
COND_PUSH(set, CLBR_RDI, rdi);  \
-   COND_PUSH(set, CLBR_R8, r8);\
-   COND_PUSH(set, CLBR_R9, r9);\
+   COND_PUSH(set, CLBR_R8,  r8);   \
+   COND_PUSH(set, CLBR_R9,  r9);   \
COND_PUSH(set, CLBR_R10, r10);  \
COND_PUSH(set, CLBR_R11, r11)
+
 #define PV_RESTORE_REGS(set)   \
COND_POP(set, CLBR_R11, r11);   \
COND_POP(set, CLBR_R10, r10);   \
-   COND_POP(set, CLBR_R9, r9); \
-   COND_POP(set, CLBR_R8, r8); \
+   COND_POP(set, CLBR_R9,  r9);\
+   COND_POP(set, CLBR_R8,  r8);\
COND_POP(set, CLBR_RDI, rdi);   \
COND_POP(set, CLBR_RSI, rsi);   \
COND_POP(set, CLBR_RDX, rdx);   \
COND_POP(set, CLBR_RCX, rcx);   \
COND_POP(set, CLBR_RAX, rax)
 
-#define PARA_PATCH(struct, off)((PARAVIRT_PATCH_##struct + (off)) / 8)
-#define PARA_SITE(ptype, clobbers, ops) _PVSITE(ptype, clobbers, ops, .quad, 8)
-#define PARA_INDIRECT(addr)*addr(%rip)
-#else
+#define PV_INDIRECT(addr)  *addr(%rip)
+
+#else /* !CONFIG_X86_64 */
+
 #define PV_SAVE_REGS(set)  \
COND_PUSH(set, CLBR_EAX, eax);  \
COND_PUSH(set, CLBR_EDI, edi);  \
COND_PUSH(set, CLBR_ECX, ecx);  \
COND_PUSH(set, CLBR_EDX, edx)
+
 #define PV_RESTORE_REGS(set)   \
COND_POP(set, CLBR_EDX, edx);   \
COND_POP(set, CLBR_ECX, ecx);   \
COND_POP(set, CLBR_EDI, edi);   \
COND_POP(set, CLBR_EAX, eax)
 
-#define PARA_PATCH(struct, off)((PARAVIRT_PATCH_##struct + (off)) / 4)
-#define PARA_SITE(ptype, clobbers, ops) _PVSITE(ptype, clobbers, ops, .long, 4)
-#define PARA_INDIRECT(addr)*%cs:addr
-#endif
+#define PV_INDIRECT(addr)  *%cs:addr
+
+#endif /* !CONFIG_X86_64 */
 
 #define INTERRUPT_RETURN   \
-   PARA_SITE(PARA_PATCH(pv_cpu_ops, PV_CPU_iret), CLBR_NONE,   \
- jmp PARA_INDIRECT(pv_cpu_ops+PV_CPU_iret))
+   PV_SITE(jmp PV_INDIRECT(pv_cpu_ops+PV_CPU_iret),\
+   pv_cpu_ops, PV_CPU_iret, CLBR_NONE)
 
 #define DISABLE_INTERRUPTS(clobbers)   \
-   PARA_SITE(PARA_PATCH(pv_irq_ops, PV_IRQ_irq_disable), clobbers, \
- PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);\
- call PARA_INDIRECT(pv_irq_ops+PV_IRQ_irq_disable);\
- PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE);)
+   PV_SITE(PV_SAVE_REGS(clobbers | CLBR_CALLEE_SAVE);  \
+   call PV_INDIRECT(pv_irq_ops+PV_IRQ_irq_disable);\
+   PV_RESTORE_REGS(clobbers | CLBR_CALLEE_SAVE),

[Xen-devel] [PATCH 07/13] x86/paravirt: Simplify ____PVOP_CALL()

2017-10-04 Thread Josh Poimboeuf 
Remove the inline asm duplication in PVOP_CALL().

Also add 'IS_ENABLED(CONFIG_X86_32)' to the return variable logic,
making the code clearer and rendering the comment unnecessary.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt_types.h | 36 +--
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index ab7aabe6b668..01f9e10983c1 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -529,29 +529,19 @@ int paravirt_disable_iospace(void);
rettype __ret;  \
PVOP_CALL_ARGS; \
PVOP_TEST_NULL(op); \
-   /* This is 32-bit specific, but is okay in 64-bit */\
-   /* since this condition will never hold */  \
-   if (sizeof(rettype) > sizeof(unsigned long)) {  \
-   asm volatile(pre\
-paravirt_alt(PARAVIRT_CALL)\
-post   \
-: call_clbr, ASM_CALL_CONSTRAINT   \
-: paravirt_type(op),   \
-  paravirt_clobber(clbr),  \
-  ##__VA_ARGS__\
-: "memory", "cc" extra_clbr);  \
-   __ret = (rettype)u64)__edx) << 32) | __eax); \
-   } else {\
-   asm volatile(pre\
-paravirt_alt(PARAVIRT_CALL)\
-post   \
-: call_clbr, ASM_CALL_CONSTRAINT   \
-: paravirt_type(op),   \
-  paravirt_clobber(clbr),  \
-  ##__VA_ARGS__\
-: "memory", "cc" extra_clbr);  \
-   __ret = (rettype)(__eax & PVOP_RETMASK(rettype));   
\
-   }   \
+   asm volatile(pre\
+paravirt_alt(PARAVIRT_CALL)\
+post   \
+: call_clbr, ASM_CALL_CONSTRAINT   \
+: paravirt_type(op),   \
+  paravirt_clobber(clbr),  \
+  ##__VA_ARGS__\
+: "memory", "cc" extra_clbr);  \
+   if (IS_ENABLED(CONFIG_X86_32) &&\
+   sizeof(rettype) > sizeof(unsigned long))\
+   __ret = (rettype)u64)__edx) << 32) | __eax);\
+   else\
+   __ret = (rettype)(__eax & PVOP_RETMASK(rettype));\
__ret;  \
})
 
-- 
2.13.6


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

[Xen-devel] [PATCH 12/13] objtool: Add support for new .pv_altinstructions section

2017-10-04 Thread Josh Poimboeuf 
Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 tools/objtool/special.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index 84f001d52322..dc15a3564fc9 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -63,6 +63,16 @@ struct special_entry entries[] = {
.feature = ALT_FEATURE_OFFSET,
},
{
+   .sec = ".pv_altinstructions",
+   .group = true,
+   .size = ALT_ENTRY_SIZE,
+   .orig = ALT_ORIG_OFFSET,
+   .orig_len = ALT_ORIG_LEN_OFFSET,
+   .new = ALT_NEW_OFFSET,
+   .new_len = ALT_NEW_LEN_OFFSET,
+   .feature = ALT_FEATURE_OFFSET,
+   },
+   {
.sec = "__jump_table",
.jump_or_nop = true,
.size = JUMP_ENTRY_SIZE,
-- 
2.13.6


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

[Xen-devel] [PATCH 10/13] x86/alternative: Support indirect call replacement

2017-10-04 Thread Josh Poimboeuf 
Add alternative patching support for replacing an instruction with an
indirect call.  This will be needed for the paravirt alternatives.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/kernel/alternative.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 3344d3382e91..81c577c7deba 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -410,20 +410,28 @@ void __init_or_module noinline apply_alternatives(struct 
alt_instr *start,
insnbuf_sz = a->replacementlen;
 
/*
-* 0xe8 is a relative jump; fix the offset.
-*
-* Instruction length is checked before the opcode to avoid
-* accessing uninitialized bytes for zero-length replacements.
+* Fix the address offsets for call and jump instructions which
+* use PC-relative addressing.
 */
if (a->replacementlen == 5 && *insnbuf == 0xe8) {
+   /* direct call */
*(s32 *)(insnbuf + 1) += replacement - instr;
-   DPRINTK("Fix CALL offset: 0x%x, CALL 0x%lx",
+   DPRINTK("Fix direct CALL offset: 0x%x, CALL 0x%lx",
*(s32 *)(insnbuf + 1),
(unsigned long)instr + *(s32 *)(insnbuf + 1) + 
5);
-   }
 
-   if (a->replacementlen && is_jmp(replacement[0]))
+   } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
+  *(insnbuf+1) == 0x15) {
+   /* indirect call */
+   *(s32 *)(insnbuf + 2) += replacement - instr;
+   DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
+   *(s32 *)(insnbuf + 2),
+   (unsigned long)instr + *(s32 *)(insnbuf + 2) + 
6);
+
+   } else if (a->replacementlen && is_jmp(replacement[0])) {
+   /* direct jump */
recompute_jump(a, instr, replacement, insnbuf);
+   }
 
if (a->instrlen > a->replacementlen) {
add_nops(insnbuf + a->replacementlen,
-- 
2.13.6


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

[Xen-devel] [PATCH 01/13] x86/paravirt: remove wbinvd() paravirt interface

2017-10-04 Thread Josh Poimboeuf 
Since lguest was removed, only the native version of wbinvd() is used.
The paravirt interface is no longer needed.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt.h   | 5 -
 arch/x86/include/asm/paravirt_types.h | 1 -
 arch/x86/include/asm/special_insns.h  | 7 +--
 arch/x86/kernel/paravirt.c| 1 -
 arch/x86/kernel/paravirt_patch_64.c   | 2 --
 arch/x86/xen/enlighten_pv.c   | 2 --
 6 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 12deec722cf0..2f51fbf175da 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -98,11 +98,6 @@ static inline void halt(void)
PVOP_VCALL0(pv_irq_ops.halt);
 }
 
-static inline void wbinvd(void)
-{
-   PVOP_VCALL0(pv_cpu_ops.wbinvd);
-}
-
 #define get_kernel_rpl()  (pv_info.kernel_rpl)
 
 static inline u64 paravirt_read_msr(unsigned msr)
diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index 280d94c36dad..0e112f279514 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -137,7 +137,6 @@ struct pv_cpu_ops {
 
void (*set_iopl_mask)(unsigned mask);
 
-   void (*wbinvd)(void);
void (*io_delay)(void);
 
/* cpuid emulation, mostly so that caps bits can be disabled */
diff --git a/arch/x86/include/asm/special_insns.h 
b/arch/x86/include/asm/special_insns.h
index a24dfcf79f4a..ac402c6fc24b 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -128,7 +128,7 @@ static inline void __write_pkru(u32 pkru)
 }
 #endif
 
-static inline void native_wbinvd(void)
+static inline void wbinvd(void)
 {
asm volatile("wbinvd": : :"memory");
 }
@@ -183,11 +183,6 @@ static inline void __write_cr4(unsigned long x)
native_write_cr4(x);
 }
 
-static inline void wbinvd(void)
-{
-   native_wbinvd();
-}
-
 #ifdef CONFIG_X86_64
 
 static inline unsigned long read_cr8(void)
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 19a3e8f961c7..3fead3a50723 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -332,7 +332,6 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
.read_cr8 = native_read_cr8,
.write_cr8 = native_write_cr8,
 #endif
-   .wbinvd = native_wbinvd,
.read_msr = native_read_msr,
.write_msr = native_write_msr,
.read_msr_safe = native_read_msr_safe,
diff --git a/arch/x86/kernel/paravirt_patch_64.c 
b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..0a1ba3f80cbf 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -10,7 +10,6 @@ DEF_NATIVE(pv_mmu_ops, read_cr2, "movq %cr2, %rax");
 DEF_NATIVE(pv_mmu_ops, read_cr3, "movq %cr3, %rax");
 DEF_NATIVE(pv_mmu_ops, write_cr3, "movq %rdi, %cr3");
 DEF_NATIVE(pv_mmu_ops, flush_tlb_single, "invlpg (%rdi)");
-DEF_NATIVE(pv_cpu_ops, wbinvd, "wbinvd");
 
 DEF_NATIVE(pv_cpu_ops, usergs_sysret64, "swapgs; sysretq");
 DEF_NATIVE(pv_cpu_ops, swapgs, "swapgs");
@@ -60,7 +59,6 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
PATCH_SITE(pv_mmu_ops, read_cr3);
PATCH_SITE(pv_mmu_ops, write_cr3);
PATCH_SITE(pv_mmu_ops, flush_tlb_single);
-   PATCH_SITE(pv_cpu_ops, wbinvd);
 #if defined(CONFIG_PARAVIRT_SPINLOCKS)
case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
if (pv_is_native_spin_unlock()) {
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 73f809a6ca87..c0cb5c2bfd92 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1045,8 +1045,6 @@ static const struct pv_cpu_ops xen_cpu_ops __initconst = {
.write_cr8 = xen_write_cr8,
 #endif
 
-   .wbinvd = native_wbinvd,
-
.read_msr = xen_read_msr,
.write_msr = xen_write_msr,
 
-- 
2.13.6


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

[Xen-devel] [PATCH 04/13] x86/paravirt: Convert DEF_NATIVE macro to GCC extended asm syntax

2017-10-04 Thread Josh Poimboeuf 
In a future patch, the NATIVE_* instruction string macros will be used
in GCC extended inline asm, which requires registers to have two '%'
instead of one in the asm template string.  Convert the DEF_NATIVE macro
to the GCC extended asm syntax so the NATIVE_* macros can be shared more
broadly.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt_types.h | 10 +++---
 arch/x86/include/asm/special_insns.h  | 14 +++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index e99e5ac3e036..ab7aabe6b668 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -359,11 +359,15 @@ extern struct pv_lock_ops pv_lock_ops;
_paravirt_alt(insn_string, "%c[paravirt_typenum]", 
"%c[paravirt_clobber]")
 
 /* Simple instruction patching code. */
-#define NATIVE_LABEL(a,x,b) "\n\t.globl " a #x "_" #b "\n" a #x "_" #b ":\n\t"
+#define NATIVE_LABEL(a,x,b) "\n" a #x "_" #b ":\n\t"
 
 #define DEF_NATIVE(ops, name, code)\
-   __visible extern const char start_##ops##_##name[], 
end_##ops##_##name[];   \
-   asm(NATIVE_LABEL("start_", ops, name) code NATIVE_LABEL("end_", ops, 
name))
+static inline void __used __native_ ## name ## _insns(void) {  \
+   asm volatile(NATIVE_LABEL("start_", ops, name)  \
+code   \
+NATIVE_LABEL("end_", ops, name) : );   \
+} \
+__visible extern const char start_##ops##_##name[], end_##ops##_##name[];
 
 unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len);
 unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
diff --git a/arch/x86/include/asm/special_insns.h 
b/arch/x86/include/asm/special_insns.h
index 0549c5f2c1b3..4b89668f2862 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -7,14 +7,14 @@
 #include 
 
 #ifdef CONFIG_X86_64
-# define _REG_ARG1 "%rdi"
-# define NATIVE_IDENTITY_32"mov %edi, %eax"
+# define _REG_ARG1 "%%rdi"
+# define NATIVE_IDENTITY_32"mov %%edi, %%eax"
 # define NATIVE_USERGS_SYSRET64"swapgs; sysretq"
 #else
-# define _REG_ARG1 "%eax"
+# define _REG_ARG1 "%%eax"
 #endif
 
-#define _REG_RET   "%" _ASM_AX
+#define _REG_RET   "%%" _ASM_AX
 
 #define NATIVE_ZERO"xor " _REG_ARG1 ", " _REG_ARG1
 #define NATIVE_IDENTITY"mov " _REG_ARG1 ", " _REG_RET
@@ -22,9 +22,9 @@
 #define NATIVE_RESTORE_FL  "push " _REG_ARG1 "; popf"
 #define NATIVE_IRQ_DISABLE "cli"
 #define NATIVE_IRQ_ENABLE  "sti"
-#define NATIVE_READ_CR2"mov %cr2, " _REG_RET
-#define NATIVE_READ_CR3"mov %cr3, " _REG_RET
-#define NATIVE_WRITE_CR3   "mov " _REG_ARG1 ", %cr3"
+#define NATIVE_READ_CR2"mov %%cr2, " _REG_RET
+#define NATIVE_READ_CR3"mov %%cr3, " _REG_RET
+#define NATIVE_WRITE_CR3   "mov " _REG_ARG1 ", %%cr3"
 #define NATIVE_FLUSH_TLB_SINGLE"invlpg (" _REG_ARG1 ")"
 #define NATIVE_SWAPGS  "swapgs"
 #define NATIVE_IRET"iret"
-- 
2.13.6


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

[Xen-devel] [PATCH 02/13] x86/paravirt: Fix output constraint macro names

2017-10-04 Thread Josh Poimboeuf 
Some of the paravirt '*_CLOBBERS' macros refer to output constraints
instead of clobbers, which makes the code extra confusing.  Rename the
output constraint related macros to '*_OUTPUTS'.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/paravirt_types.h | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/paravirt_types.h 
b/arch/x86/include/asm/paravirt_types.h
index 0e112f279514..e99e5ac3e036 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -466,12 +466,12 @@ int paravirt_disable_iospace(void);
 #define PVOP_CALL_ARG2(x)  "d" ((unsigned long)(x))
 #define PVOP_CALL_ARG3(x)  "c" ((unsigned long)(x))
 
-#define PVOP_VCALL_CLOBBERS"=a" (__eax), "=d" (__edx), \
+#define PVOP_VCALL_OUTPUTS "=a" (__eax), "=d" (__edx), \
"=c" (__ecx)
-#define PVOP_CALL_CLOBBERS PVOP_VCALL_CLOBBERS
+#define PVOP_CALL_OUTPUTS  PVOP_VCALL_OUTPUTS
 
-#define PVOP_VCALLEE_CLOBBERS  "=a" (__eax), "=d" (__edx)
-#define PVOP_CALLEE_CLOBBERS   PVOP_VCALLEE_CLOBBERS
+#define PVOP_VCALLEE_OUTPUTS   "=a" (__eax), "=d" (__edx)
+#define PVOP_CALLEE_OUTPUTSPVOP_VCALLEE_OUTPUTS
 
 #define EXTRA_CLOBBERS
 #define VEXTRA_CLOBBERS
@@ -488,14 +488,14 @@ int paravirt_disable_iospace(void);
 #define PVOP_CALL_ARG3(x)  "d" ((unsigned long)(x))
 #define PVOP_CALL_ARG4(x)  "c" ((unsigned long)(x))
 
-#define PVOP_VCALL_CLOBBERS"=D" (__edi),   \
+#define PVOP_VCALL_OUTPUTS "=D" (__edi),   \
"=S" (__esi), "=d" (__edx), \
"=c" (__ecx)
-#define PVOP_CALL_CLOBBERS PVOP_VCALL_CLOBBERS, "=a" (__eax)
+#define PVOP_CALL_OUTPUTS  PVOP_VCALL_OUTPUTS, "=a" (__eax)
 
 /* void functions are still allowed [re]ax for scratch */
-#define PVOP_VCALLEE_CLOBBERS  "=a" (__eax)
-#define PVOP_CALLEE_CLOBBERS   PVOP_VCALLEE_CLOBBERS
+#define PVOP_VCALLEE_OUTPUTS   "=a" (__eax)
+#define PVOP_CALLEE_OUTPUTSPVOP_VCALLEE_OUTPUTS
 
 #define EXTRA_CLOBBERS  , "r8", "r9", "r10", "r11"
 #define VEXTRA_CLOBBERS , "rax", "r8", "r9", "r10", "r11"
@@ -552,12 +552,12 @@ int paravirt_disable_iospace(void);
})
 
 #define __PVOP_CALL(rettype, op, pre, post, ...)   \
-   PVOP_CALL(rettype, op, CLBR_ANY, PVOP_CALL_CLOBBERS,\
+   PVOP_CALL(rettype, op, CLBR_ANY, PVOP_CALL_OUTPUTS, \
  EXTRA_CLOBBERS, pre, post, ##__VA_ARGS__)
 
 #define __PVOP_CALLEESAVE(rettype, op, pre, post, ...) \
PVOP_CALL(rettype, op.func, CLBR_RET_REG,   \
- PVOP_CALLEE_CLOBBERS, ,   \
+ PVOP_CALLEE_OUTPUTS, ,\
  pre, post, ##__VA_ARGS__)
 
 
@@ -576,13 +576,13 @@ int paravirt_disable_iospace(void);
})
 
 #define __PVOP_VCALL(op, pre, post, ...)   \
-   PVOP_VCALL(op, CLBR_ANY, PVOP_VCALL_CLOBBERS,   \
+   PVOP_VCALL(op, CLBR_ANY, PVOP_VCALL_OUTPUTS,\
   VEXTRA_CLOBBERS, \
   pre, post, ##__VA_ARGS__)
 
 #define __PVOP_VCALLEESAVE(op, pre, post, ...) \
PVOP_VCALL(op.func, CLBR_RET_REG,   \
- PVOP_VCALLEE_CLOBBERS, ,  \
+ PVOP_VCALLEE_OUTPUTS, ,   \
  pre, post, ##__VA_ARGS__)
 
 
-- 
2.13.6


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

[Xen-devel] [PATCH 00/13] x86/paravirt: Make pv ops code generation more closely match reality

2017-10-04 Thread Josh Poimboeuf 
This changes the pv ops code generation to more closely match reality.

For example, instead of:

  callq  *0x81e3a400 (pv_irq_ops.save_fl)

vmlinux will now show:

  pushfq
  pop%rax
  nop
  nop
  nop
  nop
  nop

which is what the runtime version of the code will show in most cases.

This idea was suggested by Andy Lutomirski.

The benefits are:

- For the most common runtime cases (everything except Xen and vSMP),
  vmlinux disassembly now matches what the actual runtime code looks
  like.  This improves debuggability and kernel developer sanity (a
  precious resource).

- It fixes a KASAN warning in the ORC unwinder due to objtool not
  understanding the .parainstructions stuff.

- It's hopefully a first step in simplifying paravirt patching by
  getting rid of .parainstructions, pv ops, and apply_paravirt()
  completely.  (I think Xen can be changed to set CPU feature bits to
  specify which ops it needs during early boot, then those ops can be
  patched in using early alternatives.)

For more details, see the commit log in patch 11/13.

Josh Poimboeuf (13):
  x86/paravirt: remove wbinvd() paravirt interface
  x86/paravirt: Fix output constraint macro names
  x86/paravirt: Convert native patch assembly code strings to macros
  x86/paravirt: Convert DEF_NATIVE macro to GCC extended asm syntax
  x86/paravirt: Move paravirt asm macros to paravirt-asm.h
  x86/paravirt: Clean up paravirt-asm.h
  x86/paravirt: Simplify PVOP_CALL()
  x86/paravirt: Clean up paravirt_types.h
  x86/asm: Convert ALTERNATIVE*() assembler macros to preprocessor
macros
  x86/alternative: Support indirect call replacement
  x86/paravirt: Add paravirt alternatives infrastructure
  objtool: Add support for new .pv_altinstructions section
  x86/paravirt: Convert natively patched pv ops to use paravirt
alternatives

 arch/x86/entry/entry_32.S|  13 +-
 arch/x86/entry/entry_64.S|  12 +-
 arch/x86/entry/entry_64_compat.S |   9 +-
 arch/x86/entry/vdso/vdso32/system_call.S |  10 +-
 arch/x86/include/asm/alternative-asm.h   |  71 -
 arch/x86/include/asm/alternative.h   |  12 +-
 arch/x86/include/asm/cpufeatures.h   |   1 +
 arch/x86/include/asm/paravirt-asm.h  | 142 ++
 arch/x86/include/asm/paravirt.h  | 174 --
 arch/x86/include/asm/paravirt_types.h| 243 ---
 arch/x86/include/asm/smap.h  |   4 +-
 arch/x86/include/asm/special_insns.h |  31 +++-
 arch/x86/kernel/alternative.c|  35 -
 arch/x86/kernel/cpu/hypervisor.c |   2 +
 arch/x86/kernel/head_64.S|   2 +-
 arch/x86/kernel/module.c |  11 +-
 arch/x86/kernel/paravirt.c   |   1 -
 arch/x86/kernel/paravirt_patch_32.c  |  21 +--
 arch/x86/kernel/paravirt_patch_64.c  |  31 ++--
 arch/x86/kernel/vmlinux.lds.S|   6 +
 arch/x86/lib/copy_page_64.S  |   2 +-
 arch/x86/lib/memcpy_64.S |   4 +-
 arch/x86/lib/memmove_64.S|   3 +-
 arch/x86/lib/memset_64.S |   4 +-
 arch/x86/xen/enlighten_pv.c  |   3 +-
 tools/objtool/special.c  |  10 ++
 26 files changed, 516 insertions(+), 341 deletions(-)
 create mode 100644 arch/x86/include/asm/paravirt-asm.h

-- 
2.13.6


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

[Xen-devel] [PATCH 03/13] x86/paravirt: Convert native patch assembly code strings to macros

2017-10-04 Thread Josh Poimboeuf 
Convert the hard-coded native patch assembly code strings to macros to
facilitate sharing common code between 32-bit and 64-bit.

These macros will also be used by a future patch which requires the GCC
extended asm syntax of two '%' characters instead of one when specifying
a register name.

Signed-off-by: Josh Poimboeuf <jpoim...@redhat.com>
---
 arch/x86/include/asm/special_insns.h | 24 
 arch/x86/kernel/paravirt_patch_32.c  | 21 +++--
 arch/x86/kernel/paravirt_patch_64.c  | 29 +++--
 3 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h 
b/arch/x86/include/asm/special_insns.h
index ac402c6fc24b..0549c5f2c1b3 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -6,6 +6,30 @@
 
 #include 
 
+#ifdef CONFIG_X86_64
+# define _REG_ARG1 "%rdi"
+# define NATIVE_IDENTITY_32"mov %edi, %eax"
+# define NATIVE_USERGS_SYSRET64"swapgs; sysretq"
+#else
+# define _REG_ARG1 "%eax"
+#endif
+
+#define _REG_RET   "%" _ASM_AX
+
+#define NATIVE_ZERO"xor " _REG_ARG1 ", " _REG_ARG1
+#define NATIVE_IDENTITY"mov " _REG_ARG1 ", " _REG_RET
+#define NATIVE_SAVE_FL "pushf; pop " _REG_RET
+#define NATIVE_RESTORE_FL  "push " _REG_ARG1 "; popf"
+#define NATIVE_IRQ_DISABLE "cli"
+#define NATIVE_IRQ_ENABLE  "sti"
+#define NATIVE_READ_CR2"mov %cr2, " _REG_RET
+#define NATIVE_READ_CR3"mov %cr3, " _REG_RET
+#define NATIVE_WRITE_CR3   "mov " _REG_ARG1 ", %cr3"
+#define NATIVE_FLUSH_TLB_SINGLE"invlpg (" _REG_ARG1 ")"
+#define NATIVE_SWAPGS  "swapgs"
+#define NATIVE_IRET"iret"
+#define NATIVE_QUEUED_SPIN_UNLOCK  "movb $0, (" _REG_ARG1 ")"
+
 /*
  * Volatile isn't enough to prevent the compiler from reordering the
  * read/write functions for the control registers and messing everything up.
diff --git a/arch/x86/kernel/paravirt_patch_32.c 
b/arch/x86/kernel/paravirt_patch_32.c
index 553acbbb4d32..c9c6106ae714 100644
--- a/arch/x86/kernel/paravirt_patch_32.c
+++ b/arch/x86/kernel/paravirt_patch_32.c
@@ -1,17 +1,18 @@
 #include 
+#include 
 
-DEF_NATIVE(pv_irq_ops, irq_disable, "cli");
-DEF_NATIVE(pv_irq_ops, irq_enable, "sti");
-DEF_NATIVE(pv_irq_ops, restore_fl, "push %eax; popf");
-DEF_NATIVE(pv_irq_ops, save_fl, "pushf; pop %eax");
-DEF_NATIVE(pv_cpu_ops, iret, "iret");
-DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax");
-DEF_NATIVE(pv_mmu_ops, write_cr3, "mov %eax, %cr3");
-DEF_NATIVE(pv_mmu_ops, read_cr3, "mov %cr3, %eax");
+DEF_NATIVE(pv_irq_ops, save_fl,NATIVE_SAVE_FL);
+DEF_NATIVE(pv_irq_ops, restore_fl, NATIVE_RESTORE_FL);
+DEF_NATIVE(pv_irq_ops, irq_disable,NATIVE_IRQ_DISABLE);
+DEF_NATIVE(pv_irq_ops, irq_enable, NATIVE_IRQ_ENABLE);
+DEF_NATIVE(pv_cpu_ops, iret,   NATIVE_IRET);
+DEF_NATIVE(pv_mmu_ops, read_cr2,   NATIVE_READ_CR2);
+DEF_NATIVE(pv_mmu_ops, read_cr3,   NATIVE_READ_CR3);
+DEF_NATIVE(pv_mmu_ops, write_cr3,  NATIVE_WRITE_CR3);
 
 #if defined(CONFIG_PARAVIRT_SPINLOCKS)
-DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%eax)");
-DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %eax, %eax");
+DEF_NATIVE(pv_lock_ops,queued_spin_unlock, 
NATIVE_QUEUED_SPIN_UNLOCK);
+DEF_NATIVE(pv_lock_ops,vcpu_is_preempted,  NATIVE_ZERO);
 #endif
 
 unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
diff --git a/arch/x86/kernel/paravirt_patch_64.c 
b/arch/x86/kernel/paravirt_patch_64.c
index 0a1ba3f80cbf..0aa232edd670 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -1,25 +1,26 @@
 #include 
 #include 
+#include 
 #include 
 
-DEF_NATIVE(pv_irq_ops, irq_disable, "cli");
-DEF_NATIVE(pv_irq_ops, irq_enable, "sti");
-DEF_NATIVE(pv_irq_ops, restore_fl, "pushq %rdi; popfq");
-DEF_NATIVE(pv_irq_ops, save_fl, "pushfq; popq %rax");
-DEF_NATIVE(pv_mmu_ops, read_cr2, "movq %cr2, %rax");
-DEF_NATIVE(pv_mmu_ops, read_cr3, "movq %cr3, %rax");
-DEF_NATIVE(pv_mmu_ops, write_cr3, "movq %rdi, %cr3");
-DEF_NATIVE(pv_mmu_ops, flush_tlb_single, "invlpg (%rdi)");
+DEF_NATIVE(pv_irq_ops, save_fl,NATIVE_SAVE_FL);
+DEF_NATIVE(pv_irq_ops, restore_fl, NATIVE_RESTORE_FL);
+DEF_NAT

Re: [Xen-devel] [PATCH v2 1/2] paravirt,xen: remove xen_patch()

2017-08-16 Thread Josh Poimboeuf 
On Wed, Aug 16, 2017 at 07:31:56PM +0200, Juergen Gross wrote:
>  ENTRY(xen_irq_disable_direct)
>   movb $1, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
> -ENDPATCH(xen_irq_disable_direct)
>   ret
>   ENDPROC(xen_irq_disable_direct)
> - RELOC(xen_irq_disable_direct, 0)

Might as well remove the ENDPROC indentations while you're at it, for
readability and consistency with other asm code.

Otherwise,

Reviewed-by: Josh Poimboeuf <jpoim...@redhat.com>

-- 
Josh

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

Re: [Xen-devel] [PATCH 1/3] paravirt,xen: remove xen_patch()

2017-08-10 Thread Josh Poimboeuf 
On Thu, Aug 10, 2017 at 02:52:52PM +0200, Juergen Gross wrote:
> Xen's paravirt patch function xen_patch() does some special casing for
> irq_ops functions to apply relocations when those functions can be
> patched inline instead of calls.
> 
> Unfortunately none of the special case function replacements is small
> enough to be patches inline, so the special case never applies.
> 
> As xen_patch() will call paravirt_patch_default() in all cases it can
> be just dropped.
> 
> Signed-off-by: Juergen Gross 

Can the ENDPATCH and RELOC macros can also be removed, along with their
usages in xen-asm.S and xen-asm_64.S?

-- 
Josh

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

Re: [Xen-devel] [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

2017-05-19 Thread Josh Poimboeuf 
On Fri, May 19, 2017 at 11:17:24AM +0200, Jiri Slaby wrote:
> On 05/17/2017, 03:23 PM, Jiri Slaby wrote:
> >> So the initial CFI state is different between the two types of
> >> "functions".  And there are a lot of other differences.  C-type
> >> functions have to follow frame pointer conventions, for example.  So
> >> your FUNC_START macro (and objtool) would have to somehow figure out a
> >> way to make a distinction between the two.  So it would probably work
> >> out better if we kept the distinction between C-type functions and other
> >> code.
> > 
> > Ok, that makes a lot of sense.
> 
> A quick question:
> Do you consider these to be C-type functions?
> 
>   ENTRY(function_hook)
> ret
>   END(function_hook)
> 
> or this?
> 
>   ENTRY(native_load_gs_index)
> pushfq
> DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
> SWAPGS
> movl%edi, %gs
> SWAPGS
> popfq
> ret
>   END(native_load_gs_index)
> 
> Both are called from C, but they do not setup frame pointer etc.

Yeah, those are valid C-type functions.  Setting up the frame pointer is
optional for leaf functions (i.e.  functions which don't call other
functions).

-- 
Josh

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

Re: [Xen-devel] [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

2017-05-12 Thread Josh Poimboeuf 
On Fri, May 12, 2017 at 09:53:48AM +0200, Jiri Slaby wrote:
> On 04/26/2017, 03:42 AM, Josh Poimboeuf wrote:
> >> @@ -323,7 +323,7 @@ ENTRY(resume_userspace)
> >>movl%esp, %eax
> >>callprepare_exit_to_usermode
> >>jmp restore_all
> >> -END(ret_from_exception)
> >> +ENDPROC(ret_from_exception)
> > 
> > What exactly is the motivation of this patch?  It would be good to
> > describe that in the commit message.
> > 
> > Is the point to allow objtool to generate CFI for it?  If so, I don't
> > really see how that would work.  Today, objtool considers ENDPROC to
> > annotate a *callable* function which conforms to the C calling ABI and
> > can be called by another function.  The stack is in a known state at
> > function entry, and so the CFI (or frame pointer info) can be reliably
> > determined.
> 
> Ugh, I haven't checked this in 100 % of cases, but this looks pretty
> fragile to me. From reading the code, the use of END or ENDPROC is
> rather random -- depending on mood and who wrote the code.

Yes, it would be fragile, but objtool has a fix for that.  It looks at
every instruction in the object file and warns if it finds a return
instruction outside of an ENDPROC function.  That works because all
callable instructions have return instructions (except when they have
sibling calls, but objtool detects those too).  So objtool will flag any
C-type functions that forgot to use ENDPROC.

> > But entry code is different.  In most cases, the global symbols aren't
> > actually called, and they don't follow any conventions.  The code is
> > spaghetti-esque, with HW handlers and jumps everywhere.  The state of
> > the stack at symbol entry varies per "function".  That's why objtool
> > ignores these files.
> 
> Unfortunately, this is true.
> 
> > For special cases (like entry code), I was thinking we'd need manual CFI
> > annotations, like we had before.  Or maybe there's another way, like
> > some new macros which tell objtool about the HW entry points and the
> > state of the registers there.
> > 
> > But I'm having trouble seeing how marking these code snippets with
> > ENTRY/ENDPROC would help objtool make any sense of the code and where
> > things are on the stack.
> 
> Ok, my intention was to have every line of assembly code in between of
> FUNC_START/FUNC_END. That way, every rsp related push/pop/sub/add can be
> annotated very easily. For the C-like functions this is all what needs
> to be done.
> 
> Then there is the spaghetti code. And I was thinking about manual
> annotations like:
> 
>   # skip the frame pointer checking between START+END here
>   OBJTOOL(SKIP_CHECKING)
> 
>   # this fn has unusual frame (like interrupts have),
> and you can find return RIP stored at fp + 0x20
>   OBJTOOL(RIP_IS_AT, 0x20)
> 
>   # put this raw CFI for this location into eh_frame
>   OBJTOOL(RAW_CFI, 0x00, 0x00, 0x00)
> 
> 
> Similarly, I have OBJTOOL(START_FUNC) and OBJTOOL(END_FUNC) emitted with
> each FUNC_START/FUNC_END. So far, when manually expanded for simplicity,
> it looks like this:

I like the idea of making objtool smart enough to read the entry code,
and of combining automated annotations (where possible) with manual
annotations (where necessary).  And it does make sense for objtool to
automate every rsp-related push/pop/sub/add annotation.  That will make
the entry code quite a bit cleaner since we don't need 'push_cfi' and
friends anymore.

However, I think trying to force the entry code snippets into being
normal functions would be awkward.  For example, C-type functions all
start off with the following initial CFI state:

 LOC   CFA  ra
   rsp+8c-8

That means the previous frame's stack pointer was at rsp+8 and the
return instruction pointer is (rsp).  But those assumptions don't hold
for non-C-type functions, which usually start with pt_regs or iret regs
on the stack, or a blank slate.

So the initial CFI state is different between the two types of
"functions".  And there are a lot of other differences.  C-type
functions have to follow frame pointer conventions, for example.  So
your FUNC_START macro (and objtool) would have to somehow figure out a
way to make a distinction between the two.  So it would probably work
out better if we kept the distinction between C-type functions and other
code.

I think ENDPROC (or FUNC_START/FUNC_END) should mean "this function is
100% standardized to the C ABI and its debuginfo can be completely
automated".  And any code outside of that would be "this code is special
and needs a mix of automated and manual debuginfo annotations."

I'm also not sure we need the objtool-specific macros.

Re: [Xen-devel] [PATCH v3 04/29] x86: assembly, use ENDPROC for functions

2017-04-25 Thread Josh Poimboeuf 
On Fri, Apr 21, 2017 at 04:12:40PM +0200, Jiri Slaby wrote:
> Somewhere END was used to end a function. It is not intended to be used
> for functions, because it does not mark the actual symbols as functions.
> Use ENDPROC in such cases which does the right job.
> 
> Signed-off-by: Jiri Slaby 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: 
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Reviewed-by: Juergen Gross  [xen parts]
> Cc: 
> ---
>  arch/x86/entry/entry_32.S| 58 
> 
>  arch/x86/entry/entry_64.S| 40 +--
>  arch/x86/entry/entry_64_compat.S |  4 +--
>  arch/x86/kernel/ftrace_32.S  |  8 +++---
>  arch/x86/kernel/ftrace_64.S  | 10 +++
>  arch/x86/xen/xen-pvh.S   |  2 +-
>  6 files changed, 61 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 50bc26949e9e..a546b84aec01 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -249,7 +249,7 @@ ENTRY(__switch_to_asm)
>   popl%ebp
>  
>   jmp __switch_to
> -END(__switch_to_asm)
> +ENDPROC(__switch_to_asm)
>  
>  /*
>   * A newly forked process directly context switches into this address.
> @@ -289,7 +289,7 @@ ENTRY(ret_from_fork)
>*/
>   movl$0, PT_EAX(%esp)
>   jmp 2b
> -END(ret_from_fork)
> +ENDPROC(ret_from_fork)
>  
>  /*
>   * Return to user mode is not as complex as all this looks,
> @@ -323,7 +323,7 @@ ENTRY(resume_userspace)
>   movl%esp, %eax
>   callprepare_exit_to_usermode
>   jmp restore_all
> -END(ret_from_exception)
> +ENDPROC(ret_from_exception)

What exactly is the motivation of this patch?  It would be good to
describe that in the commit message.

Is the point to allow objtool to generate CFI for it?  If so, I don't
really see how that would work.  Today, objtool considers ENDPROC to
annotate a *callable* function which conforms to the C calling ABI and
can be called by another function.  The stack is in a known state at
function entry, and so the CFI (or frame pointer info) can be reliably
determined.

But entry code is different.  In most cases, the global symbols aren't
actually called, and they don't follow any conventions.  The code is
spaghetti-esque, with HW handlers and jumps everywhere.  The state of
the stack at symbol entry varies per "function".  That's why objtool
ignores these files.

For special cases (like entry code), I was thinking we'd need manual CFI
annotations, like we had before.  Or maybe there's another way, like
some new macros which tell objtool about the HW entry points and the
state of the registers there.

But I'm having trouble seeing how marking these code snippets with
ENTRY/ENDPROC would help objtool make any sense of the code and where
things are on the stack.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

2017-04-10 Thread Josh Poimboeuf 
On Mon, Apr 10, 2017 at 01:23:46PM +0200, Jiri Slaby wrote:
> On 03/22/2017, 04:44 PM, Jiri Slaby wrote:
> > On 03/22/2017, 03:26 PM, Josh Poimboeuf wrote:
> >> On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
> >>> Somewhere END was used to end a function, elsewhere, nothing was used.
> >>> So unify it and mark them all by SYM_FUNC_END.
> >>>
> >>> Signed-off-by: Jiri Slaby <jsl...@suse.cz>
> >>
> >> For me these patches would be easier to review if the SYM_FUNC_START and
> >> SYM_FUNC_END pairs for a given function are done in the same patch.
> > 
> > This patchset was intended to make everything paired with minimum
> > changes. I certainly can change also counter-elements of each
> > added/changed one if you prefer.
> 
> So do really you want me to use the new macros while I am
> adding/changing the counter-macro? Is there anything else blocking the
> merge of the patches?

The code should be in a mergeable state after each patch.  If only
patches 1-3 were merged, the code would be in an inconsistent state,
with some functions having confusing ENTRY/SYM_FUNC_END pairs.  That
complicates git history and also makes it harder to review each patch.

It would be cleaner to separate things out.  First, convert ENTRY/END
functions to use ENDPROC, which is a minor bug fix.  Then they can be
converted to the new SYM_FUNC_START/END macros in a separate patch.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

2017-03-23 Thread Josh Poimboeuf 
On Thu, Mar 23, 2017 at 08:38:20AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoim...@redhat.com> wrote:
> 
> > On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> > > 
> > > * Jiri Slaby <jsl...@suse.cz> wrote:
> > > 
> > > > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > > > 
> > > > > * Pavel Machek <pa...@ucw.cz> wrote:
> > > > > 
> > > > >> Hi!
> > > > >>
> > > > >>> -ENTRY(saved_rbp)   .quad   0
> > > > >>> -ENTRY(saved_rsi)   .quad   0
> > > > >>> -ENTRY(saved_rdi)   .quad   0
> > > > >>> -ENTRY(saved_rbx)   .quad   0
> > > > >>> +SYM_DATA_START(saved_rbp)  .quad   0
> > > > >>> +SYM_DATA_START(saved_rsi)  .quad   0
> > > > >>> +SYM_DATA_START(saved_rdi)  .quad   0
> > > > >>> +SYM_DATA_START(saved_rbx)  .quad   0
> > > > >>
> > > > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > > > >> corresponding end?
> > > > > 
> > > > > That looks like a bug - I think we should strive for them to always 
> > > > > be in pairs.
> > > > > 
> > > > > Jiri, Josh, could objtool help here perhaps, to detect 
> > > > > 'non-terminated' 
> > > > > SYM_*_START() uses? This could be done by emitting debug data into a 
> > > > > special 
> > > > > section and then analyzing that section for unpaired entries. The 
> > > > > section can be 
> > > > > discarded in the final link, it won't show up in the kernel image.
> > > > 
> > > > It should be easier than that. No introduction of other info needed --
> > > > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > > > be a bug now.
> > > 
> > > I'm all for that!
> > 
> > It would be easy to add this checking to objtool since it already reads
> > the symbol table.  The hard part is figuring out the logistics. :-)
> > 
> > - Should the warnings be on by default?
> 
> Yes, if objtool is running. Keep it simple.
> 
> > - Part of the "objtool check" command or something else?
> 
> Yes - I think it's still within the 'object file check' functionality.
> 
> > - Separate config option or just include it with
> >   CONFIG_STACK_VALIDATION?
> 
> Yeah, but I'd rename CONFIG_STACK_VALIDATION to CONFIG_OBJ_VALIDATION or 
> such. As 
> I predicted early on, objtool will go beyond stack checking! ;-)
> 
> > - Should all asm files be checked, including those currently skipped by
> >   objtool with OBJECT_FILES_NON_STANDARD?
> 
> The symbol syntax check should definitely be for all files, yes.

That all sounds reasonable.  I'll work something up.

> Could we perhaps emit 'non-standard stack frames' information into the .o 
> itself 
> (via a flag or a special section?), so that objtool can decide on its own 
> whether 
> to complain about any weirdnesses there?

For the OBJECT_FILES_NON_STANDARD case, where the whole file is
"special", we can just provide a flag to "objtool check" to tell it to
skip stack checking for that file, but still do the symbol checks.

> > > Can we detect double ends as well - i.e. do a build check of the full 
> > > syntax of 
> > > these symbol definition primitives?
> > 
> > Detecting double ends would be a little trickier.  The second SYM_*_END
> > supersedes the first, so that information isn't in the ELF symbol table.
> 
> Indeed.
> 
> > We could use a special section to annotate all the macro uses and have
> > objtool do the checking, similar to what you suggested earlier.
> 
> That might be useful for other purposes as well - such as the non-standard 
> stack 
> frame annotations?

To start with we can try going without all the special sections (other
than the SYM_END double end check).  If we end up finding another case
which isn't covered then we can always add the special sections later.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

2017-03-22 Thread Josh Poimboeuf 
On Wed, Mar 22, 2017 at 04:01:08PM +0100, Jiri Slaby wrote:
> On 03/22/2017, 03:11 PM, Josh Poimboeuf wrote:
> > Or, here's a much easier way to do it, without involving objtool:
> > 
> > --- a/include/linux/linkage.h
> > +++ b/include/linux/linkage.h
> > @@ -138,9 +138,17 @@
> > name:
> >  #endif
> >  
> > +#ifndef CHECK_DUP_SYM_END
> > +#define CHECK_DUP_SYM_END(name)\
> > +   .pushsection .discard.sym_func_end ASM_NL   \
> > +   SYM_END_##name: .byte 0 ASM_NL  \
> > +   .popsection
> > +#endif
> > +
> >  /* SYM_END -- use only if you have to */
> >  #ifndef SYM_END
> >  #define SYM_END(name, sym_type)\
> > +   CHECK_DUP_SYM_END(name) ASM_NL  \
> > .type name sym_type ASM_NL  \
> > .size name, .-name
> >  #endif
> 
> I tried this approach and it didn't work for me inside .macros. Oh,
> well, the name cannot be first, so now, we can have a check for both
> correct pairing _and_ duplicate ends in one:
> 
> #define SYM_CHECK_START(name)   \
> .pushsection .rodata.bubak ASM_NL   \
> .long has_no_SYM_END_##name - . ASM_NL  \
> .popsection
> 
> #define SYM_CHECK_END(name) \
> has_no_SYM_END_##name:
> 
> /* SYM_START -- use only if you have to */
> #ifndef SYM_START
> #define SYM_START(name, align, visibility, entry)   \
> SYM_CHECK_START(name) ASM_NL\
> visibility(name) ASM_NL \
> align ASM_NL\
> name: ASM_NL\
> entry
> #endif
> 
> /* SYM_END -- use only if you have to */
> #ifndef SYM_END
> #define SYM_END(name, sym_type, exit)   \
> exit ASM_NL \
> SYM_CHECK_END(name) ASM_NL  \
> .type name sym_type ASM_NL  \
> .size name, .-name
> #endif
> 
> 
> So for the ftrace mistake I did:
> 
>   AS  arch/x86/kernel/mcount_64.o
> /home/latest/linux/arch/x86/kernel/mcount_64.S: Assembler messages:
> /home/latest/linux/arch/x86/kernel/mcount_64.S:192: Error: symbol
> `has_no_SYM_END_ftrace_caller' is already defined
> 
> 
> or if I remove SYM_END_FUNC completely:
>   LD  vmlinux.o
>   MODPOST vmlinux.o
> arch/x86/built-in.o:(.rodata.bubak+0x130): undefined reference to
> `has_no_SYM_END_ftrace_stub'
> 
> 
> Sad is that this occurs only during linking, so I cannot put it in the
> .discard section -- ideas?

Ah, interesting idea but I can't think of a way to do the missing end
check before link time.

But it would be easy for objtool to check for a missing end because the
symbol would have a zero size.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

2017-03-22 Thread Josh Poimboeuf 
On Wed, Mar 22, 2017 at 08:46:16AM +0100, Ingo Molnar wrote:
> 
> * Jiri Slaby  wrote:
> 
> > On 03/22/2017, 08:25 AM, Ingo Molnar wrote:
> > > 
> > > * Pavel Machek  wrote:
> > > 
> > >> Hi!
> > >>
> > >>> -ENTRY(saved_rbp)   .quad   0
> > >>> -ENTRY(saved_rsi)   .quad   0
> > >>> -ENTRY(saved_rdi)   .quad   0
> > >>> -ENTRY(saved_rbx)   .quad   0
> > >>> +SYM_DATA_START(saved_rbp)  .quad   0
> > >>> +SYM_DATA_START(saved_rsi)  .quad   0
> > >>> +SYM_DATA_START(saved_rdi)  .quad   0
> > >>> +SYM_DATA_START(saved_rbx)  .quad   0
> > >>
> > >> Does it make sense to call it SYM_DATA_*START* when there's no
> > >> corresponding end?
> > > 
> > > That looks like a bug - I think we should strive for them to always be in 
> > > pairs.
> > > 
> > > Jiri, Josh, could objtool help here perhaps, to detect 'non-terminated' 
> > > SYM_*_START() uses? This could be done by emitting debug data into a 
> > > special 
> > > section and then analyzing that section for unpaired entries. The section 
> > > can be 
> > > discarded in the final link, it won't show up in the kernel image.
> > 
> > It should be easier than that. No introduction of other info needed --
> > every global symbol without a ".type" or ".size" (i.e. SYM_*_END) should
> > be a bug now.
> 
> I'm all for that!

It would be easy to add this checking to objtool since it already reads
the symbol table.  The hard part is figuring out the logistics. :-)

- Should the warnings be on by default?

- Part of the "objtool check" command or something else?

- Separate config option or just include it with
  CONFIG_STACK_VALIDATION?

- Should all asm files be checked, including those currently skipped by
  objtool with OBJECT_FILES_NON_STANDARD?

> Can we detect double ends as well - i.e. do a build check of the full syntax 
> of 
> these symbol definition primitives?

Detecting double ends would be a little trickier.  The second SYM_*_END
supersedes the first, so that information isn't in the ELF symbol table.

We could use a special section to annotate all the macro uses and have
objtool do the checking, similar to what you suggested earlier.

Or, here's a much easier way to do it, without involving objtool:

--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -138,9 +138,17 @@
name:
 #endif
 
+#ifndef CHECK_DUP_SYM_END
+#define CHECK_DUP_SYM_END(name)\
+   .pushsection .discard.sym_func_end ASM_NL   \
+   SYM_END_##name: .byte 0 ASM_NL  \
+   .popsection
+#endif
+
 /* SYM_END -- use only if you have to */
 #ifndef SYM_END
 #define SYM_END(name, sym_type)\
+   CHECK_DUP_SYM_END(name) ASM_NL  \
.type name sym_type ASM_NL  \
.size name, .-name
 #endif


If there's an extra SYM_*_END, the build fails.  For example, if I add
an extra SYM_FUNC_END(\name) to the THUNK macro:

AS  arch/x86/entry/thunk_64.o
  arch/x86/entry/thunk_64.S: Assembler messages:
  arch/x86/entry/thunk_64.S:42: Error: symbol `SYM_END_trace_hardirqs_on_thunk' 
is already defined
  arch/x86/entry/thunk_64.S:43: Error: symbol 
`SYM_END_trace_hardirqs_off_thunk' is already defined
  arch/x86/entry/thunk_64.S:47: Error: symbol `SYM_END_lockdep_sys_exit_thunk' 
is already defined
  arch/x86/entry/thunk_64.S:51: Error: symbol `SYM_ENDpreempt_schedule' is 
already defined
  arch/x86/entry/thunk_64.S:52: Error: symbol 
`SYM_ENDpreempt_schedule_notrace' is already defined
  scripts/Makefile.build:395: recipe for target 'arch/x86/entry/thunk_64.o' 
failed

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

2017-03-22 Thread Josh Poimboeuf 
On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
> Somewhere END was used to end a function, elsewhere, nothing was used.
> So unify it and mark them all by SYM_FUNC_END.
> 
> Signed-off-by: Jiri Slaby 

For me these patches would be easier to review if the SYM_FUNC_START and
SYM_FUNC_END pairs for a given function are done in the same patch.

Also I noticed several cases in entry_64.S where the old ENTRY macro is
still used, and paired with SYM_FUNC_END.

Maybe there should be an x86 version of the deprecated ENTRY/ENDPROC/etc
macros which throw a warning or an error?

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 03/10] x86: assembly, use SYM_FUNC_END for functions

2017-03-21 Thread Josh Poimboeuf 
On Mon, Mar 20, 2017 at 01:32:15PM +0100, Jiri Slaby wrote:
>  ENTRY(ftrace_caller)
>   /* save_mcount_regs fills in first two parameters */
> @@ -184,11 +184,12 @@ GLOBAL(ftrace_epilogue)
>  GLOBAL(ftrace_graph_call)
>   jmp ftrace_stub
>  #endif
> +SYM_FUNC_END(ftrace_caller)
>  
>  /* This is weak to keep gas from relaxing the jumps */
>  WEAK(ftrace_stub)
>   retq
> -END(ftrace_caller)
> +SYM_FUNC_END(ftrace_caller)

This gives ftrace_caller() two ends.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

2017-03-20 Thread Josh Poimboeuf 
On Mon, Mar 20, 2017 at 04:32:09PM +0100, Jiri Slaby wrote:
> On 03/20/2017, 02:32 PM, Josh Poimboeuf wrote:
> > On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> >> This is a start of series to cleanup macros used for starting functions,
> >> data, globals etc. across x86. When we have all this sorted out, this
> >> will help to inject DWARF unwinding info by objtool later.
> >>
> >> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> >> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.
> > 
> > Do we still want to emit .cfi_startproc/endproc from the macro?  From
> > our last discussion, that seemed to be up in the air.
> > 
> >   https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble
> 
> "Automatically at best" above means "completely from objtool". I am
> still uncertain whether it will work 100% or we would have to help by
> generating some pieces from the added macros. In particular, the ALIASes
> are evil which cause harm here:
> 
> fun_alias:
> fun:
> 
> .size fun, .-fun
> .type fun STT_FUNC
> .size fun_alias, .-fun_alias
> .type fun_alias STT_FUNC
> 
> Both cannot create (overlapping) .cfi_startproc/endproc, only the inner
> shall.
> 
> But it seems so far, that we might be able to deal with all of that from
> objtool... (I have not been thinking about this particular thing deep
> enough yet.) Some sort of "from the last label that is marked as
> STT_FUNC till its .size" might work.

Ok.

> > What did you think about making CFI read-only for .c object files and
> > write-only for .S object files?
> 
> There are those functions like sync_core() or native_save_fl() with
> inline asm. And they seem to need a) read-write support, or b) manual
> annotation. I would like to avoid b) for sure.

Ah, so I guess those inline asm functions cause problems because they
muck with the stack pointer with pushes and pops?

I don't think manual annotation of inline asm would be so bad.  IIUC, it
would only mean replacing the pushes and pops with a macro which does
the CFI-annotated version, like PUSH_CFI and POP_CFI.  And the benefit
would be that objtool doesn't have to try to rewrite a bunch of .c
object files.

Objtool read-write worries me because it gives more responsibility to
objtool.  It could be tricky to insert CFI instructions within the ones
already created by gcc.  Also, while unlikely, a bug in objtool could
theoretically corrupt an object file and brick the kernel.  Also I
wonder how all those extra file writes would affect build performance.

If at all possible, I would rather objtool stay out of the way of the
compiler and let gcc do its job of generating CFI.

-- 
Josh

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

Re: [Xen-devel] [PATCH v2 02/10] x86: assembly, FUNC_START for fn, DATA_START for data

2017-03-20 Thread Josh Poimboeuf 
On Mon, Mar 20, 2017 at 01:32:14PM +0100, Jiri Slaby wrote:
> This is a start of series to cleanup macros used for starting functions,
> data, globals etc. across x86. When we have all this sorted out, this
> will help to inject DWARF unwinding info by objtool later.
> 
> The goal is forcing SYM_FUNC_START to emit .cfi_startproc and
> SYM_FUNC_END to emit .cfi_endproc. Automatically at best.

Do we still want to emit .cfi_startproc/endproc from the macro?  From
our last discussion, that seemed to be up in the air.

  https://lkml.kernel.org/r/20170217211804.j6l2d7t5mfzqzmbt@treble

What did you think about making CFI read-only for .c object files and
write-only for .S object files?

-- 
Josh

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

Re: [Xen-devel] [RFC v3 10/13] jump_label: port __jump_table to linker tables

2016-07-22 Thread Josh Poimboeuf 
On Fri, Jul 22, 2016 at 02:24:44PM -0700, Luis R. Rodriguez wrote:
> diff --git a/tools/objtool/special.c b/tools/objtool/special.c
> index bff8abb3a4aa..f0ad369f994b 100644
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -26,6 +26,10 @@
>  #include "special.h"
>  #include "warn.h"
>  
> +#include "../../include/asm-generic/sections.h"
> +#include "../../include/asm-generic/tables.h"
> +#include "../../include/linux/stringify.h"
> +
>  #define EX_ENTRY_SIZE12
>  #define EX_ORIG_OFFSET   0
>  #define EX_NEW_OFFSET4
> @@ -63,7 +67,9 @@ struct special_entry entries[] = {
>   .feature = ALT_FEATURE_OFFSET,
>   },
>   {
> - .sec = "__jump_table",
> + .sec = __stringify(SECTION_TBL(SECTION_DATA,
> +__jump_table,
> +SECTION_ORDER_ANY)),
>   .jump_or_nop = true,
>   .size = JUMP_ENTRY_SIZE,
>   .orig = JUMP_ORIG_OFFSET,

(continuing our discussion from another thread...)

I think tools code isn't allowed to include kernel files because the
tools subdirectory is supposed to be completely independent.
 
As far as I can tell, the section name will always be
".data.tbl.__jump_table.any".  Is that true?  If so, any reason why we
can't just hard-code the string here?  As you saw, if the string
changes, objtool will complain and 0-day will report it.

-- 
Josh

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

Re: [Xen-devel] [RFC v3 10/13] jump_label: port __jump_table to linker tables

2016-07-22 Thread Josh Poimboeuf 
On Sat, Jul 23, 2016 at 12:26:54AM +0200, Luis R. Rodriguez wrote:
> On Fri, Jul 22, 2016 at 04:49:45PM -0500, Josh Poimboeuf wrote:
> > On Fri, Jul 22, 2016 at 02:24:44PM -0700, Luis R. Rodriguez wrote:
> > > diff --git a/tools/objtool/special.c b/tools/objtool/special.c
> > > index bff8abb3a4aa..f0ad369f994b 100644
> > > --- a/tools/objtool/special.c
> > > +++ b/tools/objtool/special.c
> > > @@ -26,6 +26,10 @@
> > >  #include "special.h"
> > >  #include "warn.h"
> > >  
> > > +#include "../../include/asm-generic/sections.h"
> > > +#include "../../include/asm-generic/tables.h"
> > > +#include "../../include/linux/stringify.h"
> > > +
> > >  #define EX_ENTRY_SIZE12
> > >  #define EX_ORIG_OFFSET   0
> > >  #define EX_NEW_OFFSET4
> > > @@ -63,7 +67,9 @@ struct special_entry entries[] = {
> > >   .feature = ALT_FEATURE_OFFSET,
> > >   },
> > >   {
> > > - .sec = "__jump_table",
> > > + .sec = __stringify(SECTION_TBL(SECTION_DATA,
> > > +__jump_table,
> > > +SECTION_ORDER_ANY)),
> > >   .jump_or_nop = true,
> > >   .size = JUMP_ENTRY_SIZE,
> > >   .orig = JUMP_ORIG_OFFSET,
> > 
> > (continuing our discussion from another thread...)
> > 
> > I think tools code isn't allowed to include kernel files because the
> > tools subdirectory is supposed to be completely independent.
> 
> That was also the case for userpsace tools in scripts/ -- for instance
> scripts/mod/modpost.c made an exception. What I've proposed here to
> keep things simple was to ensure -UKERNEL is passed and then only
> include files we know will work.
> 
> Refer to the patch "kprobes: port .kprobes.text to section range"
> in this series for an extension of the scripts/mod/modpost.c kernel
> headers use.

I think the rule of separating code is stricter for tools/ than it is
for scripts/.  The scripts are generally kernel-specific whereas the
tools are independent.  I believe the goal is to be able to copy them
out of the kernel tree and still be able to compile them.

> > As far as I can tell, the section name will always be
> > ".data.tbl.__jump_table.any".  Is that true?  If so, any reason why we
> > can't just hard-code the string here?
> 
> Its a fair strategy, however if upstream changes the order name we'd
> have to hand code this as well, by using a macro we keep it all in one
> place.

Hm, do you expect the section name to change often?

> > As you saw, if the string
> > changes, objtool will complain and 0-day will report it.
> 
> Indeed, which is why I was hoping to instead stick to a standard
> sections set of header files that lets us get these in on place.

Actually, I meant that obtool reporting the change would be a good
thing, in favor of just hard-coding the string.  It lets objtool do its
job of letting us know when something changes, like it did today.

> The only place I hand coded in this series was in the perl
> file scripts/recordmcount.pl but I suppose if we wanted to get
> creative we could even generate a header for it too.
> 
> If we want to avoid all this hacker include stuff another option
> is to *generate* each respective sections.h for the kernel, and
> also, one for tools, and one for perl. What do you think?

If the generated files aren't checked into git, tools/ will rely on
kernel files and will no longer be independent.  If they *are* checked
in, then we have to keep the files in sync.  Either way it sounds like
overkill, just to avoid hard-coding a string for which objtool will
already warn if it changes.

-- 
Josh

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