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

[Juergen Gross]

On 10/08/17 18:29, Peter Zijlstra wrote:
> On Thu, Aug 10, 2017 at 06:24:53PM +0200, Peter Zijlstra wrote:
>> -ENTRY(xen_irq_enable_direct)
>> -FRAME_BEGIN
>> -/* Unmask events */
>> -movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
>> -
>> -/*
>> - * Preempt here doesn't matter because that will deal with any
>> - * pending interrupts.  The pending check may end up being run
>> - * on the wrong CPU, but that doesn't hurt.
>> - */
>> -
>> -/* Test for pending */
>> -testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> -jz 1f
>> -
>> -2:  call check_events
>> -1:
>> -ENDPATCH(xen_irq_enable_direct)
>> -FRAME_END
>> -ret
>> -ENDPROC(xen_irq_enable_direct)
>> -RELOC(xen_irq_enable_direct, 2b+1)
> 
> Oh my bad, part of that is still used.
> 
> arch/x86/xen/enlighten_pv.c:pv_irq_ops.irq_enable = 
> __PV_IS_CALLEE_SAVE(xen_irq_enable_direct);
> 
> It just needs cleanups for the ENDPATCH and such.

Ah yes, of course.


Juergen

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

[Juergen Gross]

On 10/08/17 18:29, Peter Zijlstra wrote:
> On Thu, Aug 10, 2017 at 06:24:53PM +0200, Peter Zijlstra wrote:
>> -ENTRY(xen_irq_enable_direct)
>> -FRAME_BEGIN
>> -/* Unmask events */
>> -movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
>> -
>> -/*
>> - * Preempt here doesn't matter because that will deal with any
>> - * pending interrupts.  The pending check may end up being run
>> - * on the wrong CPU, but that doesn't hurt.
>> - */
>> -
>> -/* Test for pending */
>> -testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
>> -jz 1f
>> -
>> -2:  call check_events
>> -1:
>> -ENDPATCH(xen_irq_enable_direct)
>> -FRAME_END
>> -ret
>> -ENDPROC(xen_irq_enable_direct)
>> -RELOC(xen_irq_enable_direct, 2b+1)
> 
> Oh my bad, part of that is still used.
> 
> arch/x86/xen/enlighten_pv.c:pv_irq_ops.irq_enable = 
> __PV_IS_CALLEE_SAVE(xen_irq_enable_direct);
> 
> It just needs cleanups for the ENDPATCH and such.

Ah yes, of course.


Juergen

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

[Peter Zijlstra]

On Thu, Aug 10, 2017 at 06:24:53PM +0200, Peter Zijlstra wrote:
> -ENTRY(xen_irq_enable_direct)
> - FRAME_BEGIN
> - /* Unmask events */
> - movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
> -
> - /*
> -  * Preempt here doesn't matter because that will deal with any
> -  * pending interrupts.  The pending check may end up being run
> -  * on the wrong CPU, but that doesn't hurt.
> -  */
> -
> - /* Test for pending */
> - testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> - jz 1f
> -
> -2:   call check_events
> -1:
> -ENDPATCH(xen_irq_enable_direct)
> - FRAME_END
> - ret
> - ENDPROC(xen_irq_enable_direct)
> - RELOC(xen_irq_enable_direct, 2b+1)

Oh my bad, part of that is still used.

arch/x86/xen/enlighten_pv.c:pv_irq_ops.irq_enable = 
__PV_IS_CALLEE_SAVE(xen_irq_enable_direct);

It just needs cleanups for the ENDPATCH and such.

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

[Peter Zijlstra]

On Thu, Aug 10, 2017 at 06:24:53PM +0200, Peter Zijlstra wrote:
> -ENTRY(xen_irq_enable_direct)
> - FRAME_BEGIN
> - /* Unmask events */
> - movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
> -
> - /*
> -  * Preempt here doesn't matter because that will deal with any
> -  * pending interrupts.  The pending check may end up being run
> -  * on the wrong CPU, but that doesn't hurt.
> -  */
> -
> - /* Test for pending */
> - testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
> - jz 1f
> -
> -2:   call check_events
> -1:
> -ENDPATCH(xen_irq_enable_direct)
> - FRAME_END
> - ret
> - ENDPROC(xen_irq_enable_direct)
> - RELOC(xen_irq_enable_direct, 2b+1)

Oh my bad, part of that is still used.

arch/x86/xen/enlighten_pv.c:pv_irq_ops.irq_enable = 
__PV_IS_CALLEE_SAVE(xen_irq_enable_direct);

It just needs cleanups for the ENDPATCH and such.

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

[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

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

[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

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

[Peter Zijlstra]

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 
> ---
>  arch/x86/xen/enlighten_pv.c | 59 
> +
>  1 file changed, 1 insertion(+), 58 deletions(-)

> - SITE(pv_irq_ops, irq_enable);
> - SITE(pv_irq_ops, irq_disable);
> - SITE(pv_irq_ops, save_fl);
> - SITE(pv_irq_ops, restore_fl);

You forgot to remove the actual ASM that's then never used either.


 arch/x86/xen/Makefile  |   2 +-
 arch/x86/xen/xen-asm.S | 150 -
 arch/x86/xen/xen-ops.h |  12 
 3 files changed, 1 insertion(+), 163 deletions(-)

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index bced7a369a11..551e12ba2a7b 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -14,7 +14,7 @@ CFLAGS_enlighten_pv.o := $(nostackp)
 CFLAGS_mmu_pv.o:= $(nostackp)
 
 obj-y  := enlighten.o multicalls.o mmu.o irq.o \
-   time.o xen-asm.o xen-asm_$(BITS).o \
+   time.o xen-asm_$(BITS).o \
grant-table.o suspend.o platform-pci-unplug.o
 
 obj-$(CONFIG_XEN_PVHVM)+= enlighten_hvm.o mmu_hvm.o 
suspend_hvm.o
diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
deleted file mode 100644
index eff224df813f..
--- a/arch/x86/xen/xen-asm.S
+++ /dev/null
@@ -1,150 +0,0 @@
-/*
- * Asm versions of Xen pv-ops, suitable for either direct use or
- * inlining.  The inline versions are the same as the direct-use
- * versions, with the pre- and post-amble chopped off.
- *
- * This code is encoded for size rather than absolute efficiency, with
- * a view to being able to inline as much as possible.
- *
- * We only bother with direct forms (ie, vcpu in percpu data) of the
- * operations here; the indirect forms are better handled in C, since
- * they're generally too large to inline anyway.
- */
-
-#include 
-#include 
-#include 
-#include 
-
-#include "xen-asm.h"
-
-/*
- * Enable events.  This clears the event mask and tests the pending
- * event status with one and operation.  If there are pending events,
- * then enter the hypervisor to get them handled.
- */
-ENTRY(xen_irq_enable_direct)
-   FRAME_BEGIN
-   /* Unmask events */
-   movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-
-   /*
-* Preempt here doesn't matter because that will deal with any
-* pending interrupts.  The pending check may end up being run
-* on the wrong CPU, but that doesn't hurt.
-*/
-
-   /* Test for pending */
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
-   jz 1f
-
-2: call check_events
-1:
-ENDPATCH(xen_irq_enable_direct)
-   FRAME_END
-   ret
-   ENDPROC(xen_irq_enable_direct)
-   RELOC(xen_irq_enable_direct, 2b+1)
-
-
-/*
- * Disabling events is simply a matter of making the event mask
- * non-zero.
- */
-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)
-
-/*
- * (xen_)save_fl is used to get the current interrupt enable status.
- * Callers expect the status to be in X86_EFLAGS_IF, and other bits
- * may be set in the return value.  We take advantage of this by
- * making sure that X86_EFLAGS_IF has the right value (and other bits
- * in that byte are 0), but other bits in the return value are
- * undefined.  We need to toggle the state of the bit, because Xen and
- * x86 use opposite senses (mask vs enable).
- */
-ENTRY(xen_save_fl_direct)
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-   setz %ah
-   addb %ah, %ah
-ENDPATCH(xen_save_fl_direct)
-   ret
-   ENDPROC(xen_save_fl_direct)
-   RELOC(xen_save_fl_direct, 0)
-
-
-/*
- * In principle the caller should be passing us a value return from
- * xen_save_fl_direct, but for robustness sake we test only the
- * X86_EFLAGS_IF flag rather than the whole byte. After setting the
- * interrupt mask state, it checks for unmasked pending events and
- * enters the hypervisor to get them delivered if so.
- */
-ENTRY(xen_restore_fl_direct)
-   FRAME_BEGIN
-#ifdef CONFIG_X86_64
-   testw $X86_EFLAGS_IF, %di
-#else
-   testb $X86_EFLAGS_IF>>8, %ah
-#endif
-   setz PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-   /*
-* Preempt here doesn't matter because 

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

[Peter Zijlstra]

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 
> ---
>  arch/x86/xen/enlighten_pv.c | 59 
> +
>  1 file changed, 1 insertion(+), 58 deletions(-)

> - SITE(pv_irq_ops, irq_enable);
> - SITE(pv_irq_ops, irq_disable);
> - SITE(pv_irq_ops, save_fl);
> - SITE(pv_irq_ops, restore_fl);

You forgot to remove the actual ASM that's then never used either.


 arch/x86/xen/Makefile  |   2 +-
 arch/x86/xen/xen-asm.S | 150 -
 arch/x86/xen/xen-ops.h |  12 
 3 files changed, 1 insertion(+), 163 deletions(-)

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index bced7a369a11..551e12ba2a7b 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -14,7 +14,7 @@ CFLAGS_enlighten_pv.o := $(nostackp)
 CFLAGS_mmu_pv.o:= $(nostackp)
 
 obj-y  := enlighten.o multicalls.o mmu.o irq.o \
-   time.o xen-asm.o xen-asm_$(BITS).o \
+   time.o xen-asm_$(BITS).o \
grant-table.o suspend.o platform-pci-unplug.o
 
 obj-$(CONFIG_XEN_PVHVM)+= enlighten_hvm.o mmu_hvm.o 
suspend_hvm.o
diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
deleted file mode 100644
index eff224df813f..
--- a/arch/x86/xen/xen-asm.S
+++ /dev/null
@@ -1,150 +0,0 @@
-/*
- * Asm versions of Xen pv-ops, suitable for either direct use or
- * inlining.  The inline versions are the same as the direct-use
- * versions, with the pre- and post-amble chopped off.
- *
- * This code is encoded for size rather than absolute efficiency, with
- * a view to being able to inline as much as possible.
- *
- * We only bother with direct forms (ie, vcpu in percpu data) of the
- * operations here; the indirect forms are better handled in C, since
- * they're generally too large to inline anyway.
- */
-
-#include 
-#include 
-#include 
-#include 
-
-#include "xen-asm.h"
-
-/*
- * Enable events.  This clears the event mask and tests the pending
- * event status with one and operation.  If there are pending events,
- * then enter the hypervisor to get them handled.
- */
-ENTRY(xen_irq_enable_direct)
-   FRAME_BEGIN
-   /* Unmask events */
-   movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-
-   /*
-* Preempt here doesn't matter because that will deal with any
-* pending interrupts.  The pending check may end up being run
-* on the wrong CPU, but that doesn't hurt.
-*/
-
-   /* Test for pending */
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
-   jz 1f
-
-2: call check_events
-1:
-ENDPATCH(xen_irq_enable_direct)
-   FRAME_END
-   ret
-   ENDPROC(xen_irq_enable_direct)
-   RELOC(xen_irq_enable_direct, 2b+1)
-
-
-/*
- * Disabling events is simply a matter of making the event mask
- * non-zero.
- */
-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)
-
-/*
- * (xen_)save_fl is used to get the current interrupt enable status.
- * Callers expect the status to be in X86_EFLAGS_IF, and other bits
- * may be set in the return value.  We take advantage of this by
- * making sure that X86_EFLAGS_IF has the right value (and other bits
- * in that byte are 0), but other bits in the return value are
- * undefined.  We need to toggle the state of the bit, because Xen and
- * x86 use opposite senses (mask vs enable).
- */
-ENTRY(xen_save_fl_direct)
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-   setz %ah
-   addb %ah, %ah
-ENDPATCH(xen_save_fl_direct)
-   ret
-   ENDPROC(xen_save_fl_direct)
-   RELOC(xen_save_fl_direct, 0)
-
-
-/*
- * In principle the caller should be passing us a value return from
- * xen_save_fl_direct, but for robustness sake we test only the
- * X86_EFLAGS_IF flag rather than the whole byte. After setting the
- * interrupt mask state, it checks for unmasked pending events and
- * enters the hypervisor to get them delivered if so.
- */
-ENTRY(xen_restore_fl_direct)
-   FRAME_BEGIN
-#ifdef CONFIG_X86_64
-   testw $X86_EFLAGS_IF, %di
-#else
-   testb $X86_EFLAGS_IF>>8, %ah
-#endif
-   setz PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
-   /*
-* Preempt here doesn't matter because that will deal