[PATCH v2 0/6] Remove dependency on __LINE__

[PATCH v2 0/6] Remove dependency on __LINE__

[Ross Lagerwall]

Sorry for the long delay since the first version of this series
(previously called "Make building xSplice patches easier").  Here is a
set of changes that remove the use of __LINE__ when building with NDEBUG
and LivePatch enabled.  Tested to boot on x86.  Compile-tested on arm.

Changes in v2:
* Renamed xSplice to LivePatch.
* Dropped the patches for the page_alloc.c and the ACPI code as these
only changed __init functions which are not relevant for LivePatch.
* Dropped the patch to rename sections as it is not clear that is is
useful.
* Add a couple of new patches for uses of __LINE__ that have been
introduced since the last version of this series.

See the patches for further changes on each patch.

Ross Lagerwall (6):
  lib: Add a generic implementation of current_text_addr()
  sched: Remove dependency on __LINE__ for release builds
  mm: Use statically defined locking order
  iommu: Remove dependency on __LINE__ for release builds
  x86_emulate: Remove dependency on __LINE__ for release builds
  xen/arm: Remove dependency on __LINE__ for release builds

 xen/arch/arm/traps.c                   | 20 +++++++++++++++++---
 xen/arch/x86/mm/mm-locks.h             | 28 +++++++++++++++++++---------
 xen/arch/x86/x86_emulate/x86_emulate.c | 20 +++++++++++++++++---
 xen/common/lib.c                       | 12 ++++++++++++
 xen/drivers/passthrough/vtd/dmar.h     | 16 ++++++++++++++--
 xen/include/asm-x86/processor.h        | 10 ----------
 xen/include/xen/lib.h                  |  2 ++
 xen/include/xen/sched.h                | 22 +++++++++++++++-------
 8 files changed, 96 insertions(+), 34 deletions(-)

-- 
2.7.4


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

[PATCH v2 1/6] lib: Add a generic implementation of current_text_addr()

[Ross Lagerwall]

Remove the unused x86 implementation.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

Changes in v2:
* Include Clang in the comment.

 xen/common/lib.c                | 12 ++++++++++++
 xen/include/asm-x86/processor.h | 10 ----------
 xen/include/xen/lib.h           |  2 ++
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/common/lib.c b/xen/common/lib.c
index 6233020..7674d3a 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -501,6 +501,18 @@ void __init init_constructors(void)
 }
 
 /*
+ * The GCC and Clang docs state that the function must be marked noinline to
+ * have the expected result. From the GCC docs:
+ * "When inlining the expected behavior is that the function returns the
+ * address of the function that is returned to. To work around this behavior
+ * use the noinline function attribute."
+ */
+noinline void *current_text_addr(void)
+{
+    return __builtin_return_address(0);
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index dda8b83..f9de357 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -98,16 +98,6 @@
 struct domain;
 struct vcpu;
 
-/*
- * Default implementation of macro that returns current
- * instruction pointer ("program counter").
- */
-#define current_text_addr() ({                      \
-    void *pc;                                       \
-    asm ( "leaq 1f(%%rip),%0\n1:" : "=r" (pc) );    \
-    pc;                                             \
-})
-
 struct x86_cpu_id {
     uint16_t vendor;
     uint16_t family;
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 995a85a..6b4f1e4 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -162,6 +162,8 @@ extern void add_taint(unsigned int taint);
 struct cpu_user_regs;
 void dump_execstate(struct cpu_user_regs *);
 
+void *current_text_addr(void);
+
 void init_constructors(void);
 
 void *bsearch(const void *key, const void *base, size_t num, size_t size,
-- 
2.7.4


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

[PATCH v2 2/6] sched: Remove dependency on __LINE__ for release builds

[Ross Lagerwall]

When using LivePatch, use of __LINE__ can generate spurious changes in
functions due to embedded line numbers.  For release builds with
LivePatch enabled, remove the use of these line numbers in
domain_crash*() and print the current text address instead.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

Changes in v2:
* Simply macros.
* Use %pS.

 xen/include/xen/sched.h | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 0929c0b..f385de3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -625,20 +625,28 @@ void vcpu_end_shutdown_deferral(struct vcpu *v);
  * from any processor.
  */
 void __domain_crash(struct domain *d);
-#define domain_crash(d) do {                                              \
-    printk("domain_crash called from %s:%d\n", __FILE__, __LINE__);       \
-    __domain_crash(d);                                                    \
+
+#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
+#define _domain_crash(func, call) do {                                    \
+    printk(#func " called from %pS\n", current_text_addr());              \
+    call;                                                                 \
+} while (0)
+#else
+#define _domain_crash(func, call) do {                                    \
+    printk(#func " called from %s:%d\n", __FILE__, __LINE__);             \
+    call;                                                                 \
 } while (0)
+#endif
+
+#define domain_crash(d) _domain_crash(domain_crash, __domain_crash(d))
 
 /*
  * Mark current domain as crashed and synchronously deschedule from the local
  * processor. This function never returns.
  */
 void noreturn __domain_crash_synchronous(void);
-#define domain_crash_synchronous() do {                                   \
-    printk("domain_crash_sync called from %s:%d\n", __FILE__, __LINE__);  \
-    __domain_crash_synchronous();                                         \
-} while (0)
+#define domain_crash_synchronous() \
+    _domain_crash(domain_crash_sync, __domain_crash_synchronous())
 
 /*
  * Called from assembly code, with an optional address to help indicate why
-- 
2.7.4


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

[PATCH v2 3/6] mm: Use statically defined locking order

[Ross Lagerwall]

Instead of using a locking order based on line numbers which interacts
poorly with trying to create a live patch, statically define the locking
order.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---

Changes in v2:
* Rearranged defines.
* Make lock orders fit in a sign-extended 8-bit immediate.

 xen/arch/x86/mm/mm-locks.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 74fdfc1..e5fceb2 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -46,8 +46,10 @@ static inline int mm_locked_by_me(mm_lock_t *l)
     return (l->lock.recurse_cpu == current->processor);
 }
 
-/* If you see this crash, the numbers printed are lines in this file 
- * where the offending locks are declared. */
+/*
+ * If you see this crash, the numbers printed are order levels defined
+ * in this file.
+ */
 #define __check_lock_level(l)                           \
 do {                                                    \
     if ( unlikely(__get_lock_level() > (l)) )           \
@@ -152,12 +154,12 @@ static inline void mm_read_unlock(mm_rwlock_t *l)
 /* This wrapper uses the line number to express the locking order below */
 #define declare_mm_lock(name)                                                 \
     static inline void mm_lock_##name(mm_lock_t *l, const char *func, int rec)\
-    { _mm_lock(l, func, __LINE__, rec); }
+    { _mm_lock(l, func, MM_LOCK_ORDER_##name, rec); }
 #define declare_mm_rwlock(name)                                               \
     static inline void mm_write_lock_##name(mm_rwlock_t *l, const char *func) \
-    { _mm_write_lock(l, func, __LINE__); }                                    \
+    { _mm_write_lock(l, func, MM_LOCK_ORDER_##name); }                                    \
     static inline void mm_read_lock_##name(mm_rwlock_t *l)                    \
-    { _mm_read_lock(l, __LINE__); }
+    { _mm_read_lock(l, MM_LOCK_ORDER_##name); }
 /* These capture the name of the calling function */
 #define mm_lock(name, l) mm_lock_##name(l, __func__, 0)
 #define mm_lock_recursive(name, l) mm_lock_##name(l, __func__, 1)
@@ -169,10 +171,10 @@ static inline void mm_read_unlock(mm_rwlock_t *l)
  * to ordering constraints. */
 #define declare_mm_order_constraint(name)                                   \
     static inline void mm_enforce_order_lock_pre_##name(void)               \
-    { _mm_enforce_order_lock_pre(__LINE__); }                               \
+    { _mm_enforce_order_lock_pre(MM_LOCK_ORDER_##name); }                               \
     static inline void mm_enforce_order_lock_post_##name(                   \
                         int *unlock_level, unsigned short *recurse_count)   \
-    { _mm_enforce_order_lock_post(__LINE__, unlock_level, recurse_count); } \
+    { _mm_enforce_order_lock_post(MM_LOCK_ORDER_##name, unlock_level, recurse_count); } \
 
 static inline void mm_unlock(mm_lock_t *l)
 {
@@ -201,8 +203,8 @@ static inline void mm_enforce_order_unlock(int unlock_level,
 
 /************************************************************************
  *                                                                      *
- * To avoid deadlocks, these locks _MUST_ be taken in the order they're *
- * declared in this file.  The locking functions will enforce this.     *
+ * To avoid deadlocks, these locks _MUST_ be taken in the order listed  *
+ * below.  The locking functions will enforce this.                     *
  *                                                                      *
  ************************************************************************/
 
@@ -214,6 +216,7 @@ static inline void mm_enforce_order_unlock(int unlock_level,
  * - setting the "cr3" field of any p2m table to a non-P2M_BASE_EAADR value.
  *   (i.e. assigning a p2m table to be the shadow of that cr3 */
 
+#define MM_LOCK_ORDER_nestedp2m               8
 declare_mm_lock(nestedp2m)
 #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
 #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
@@ -240,6 +243,7 @@ declare_mm_lock(nestedp2m)
  * the altp2mlist lock in the middle.
  */
 
+#define MM_LOCK_ORDER_p2m                    16
 declare_mm_rwlock(p2m);
 
 /* Sharing per page lock
@@ -251,6 +255,7 @@ declare_mm_rwlock(p2m);
  *
  * The lock is recursive because during share we lock two pages. */
 
+#define MM_LOCK_ORDER_per_page_sharing       24
 declare_mm_order_constraint(per_page_sharing)
 #define page_sharing_mm_pre_lock()   mm_enforce_order_lock_pre_per_page_sharing()
 #define page_sharing_mm_post_lock(l, r) \
@@ -265,6 +270,7 @@ declare_mm_order_constraint(per_page_sharing)
  * in the target domain must be paused.
  */
 
+#define MM_LOCK_ORDER_altp2mlist             32
 declare_mm_lock(altp2mlist)
 #define altp2m_list_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_list_lock)
 #define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m_list_lock)
@@ -279,6 +285,7 @@ declare_mm_lock(altp2mlist)
  * up a gfn and later mutate it.
  */
 
+#define MM_LOCK_ORDER_altp2m                 40
 declare_mm_rwlock(altp2m);
 #define p2m_lock(p)                             \
     do {                                        \
@@ -307,6 +314,7 @@ declare_mm_rwlock(altp2m);
  * Protects private PoD data structs: entry and cache
  * counts, page lists, sweep parameters. */
 
+#define MM_LOCK_ORDER_pod                    48
 declare_mm_lock(pod)
 #define pod_lock(p)           mm_lock(pod, &(p)->pod.lock)
 #define pod_unlock(p)         mm_unlock(&(p)->pod.lock)
@@ -319,6 +327,7 @@ declare_mm_lock(pod)
  * the ordering which we enforce here.
  * The lock is not recursive. */
 
+#define MM_LOCK_ORDER_page_alloc             56
 declare_mm_order_constraint(page_alloc)
 #define page_alloc_mm_pre_lock()   mm_enforce_order_lock_pre_page_alloc()
 #define page_alloc_mm_post_lock(l) mm_enforce_order_lock_post_page_alloc(&(l), NULL)
@@ -339,6 +348,7 @@ declare_mm_order_constraint(page_alloc)
  * It also protects the log-dirty bitmap from concurrent accesses (and
  * teardowns, etc). */
 
+#define MM_LOCK_ORDER_paging                 64
 declare_mm_lock(paging)
 #define paging_lock(d)         mm_lock(paging, &(d)->arch.paging.lock)
 #define paging_lock_recursive(d) \
-- 
2.7.4


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

[PATCH v2 4/6] iommu: Remove dependency on __LINE__ for release builds

[Ross Lagerwall]

When using LivePatch, use of __LINE__ can generate spurious changes in
functions due to embedded line numbers.  For release builds with
LivePatch enabled, remove the use of these line numbers in
IOMMU_WAIT_OP() and print the current text address instead.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

Changes in v2:
* Simplified macros.
* Use %pS.

 xen/drivers/passthrough/vtd/dmar.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.h b/xen/drivers/passthrough/vtd/dmar.h
index 729b603..c3d4adc 100644
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -108,6 +108,19 @@ struct acpi_atsr_unit *acpi_find_matched_atsr_unit(const struct pci_dev *);
 
 #define DMAR_OPERATION_TIMEOUT MILLISECS(1000)
 
+#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
+#define iommu_wait_op_panic()                                              \
+    do {                                                                   \
+        panic("%pS: DMAR hardware is malfunctional", current_text_addr()); \
+    } while (0)
+#else
+#define iommu_wait_op_panic()                                              \
+    do {                                                                   \
+        panic("%s:%d:%s: DMAR hardware is malfunctional",                  \
+              __FILE__, __LINE__, __func__);                               \
+    } while (0)
+#endif
+
 #define IOMMU_WAIT_OP(iommu, offset, op, cond, sts) \
 do {                                                \
     s_time_t start_time = NOW();                    \
@@ -117,8 +130,7 @@ do {                                                \
             break;                                  \
         if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT ) {    \
             if ( !kexecing )                                    \
-                panic("%s:%d:%s: DMAR hardware is malfunctional",\
-                      __FILE__, __LINE__, __func__);            \
+                iommu_wait_op_panic();                          \
             else                                                \
                 break;                                          \
         }                                                       \
-- 
2.7.4


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

[PATCH v2 5/6] x86_emulate: Remove dependency on __LINE__ for release builds

[Ross Lagerwall]

When using LivePatch, use of __LINE__ can generate spurious changes in
functions due to embedded line numbers.  For release builds with
LivePatch enabled, remove the use of these line numbers and print the
current text address instead.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 613648e..cf05544 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -776,6 +776,22 @@ do{ asm volatile (                                                      \
 #define __emulate_1op_8byte(_op, _dst, _eflags)
 #endif /* __i386__ */
 
+#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
+#define invoke_stub_exception(trapnr, ec)                                   \
+    do {                                                                    \
+        gprintk(XENLOG_WARNING,                                             \
+                "exception %u (ec=%04x) in emulation stub (address %pS)\n", \
+                trapnr, ec, current_text_addr());                           \
+    } while (0)
+#else
+#define invoke_stub_exception(trapnr, ec)                                   \
+    do {                                                                    \
+        gprintk(XENLOG_WARNING,                                             \
+                "exception %u (ec=%04x) in emulation stub (line %u)\n",     \
+                trapnr, ec, __LINE__);                                      \
+    } while (0)
+#endif
+
 #ifdef __XEN__
 # define invoke_stub(pre, post, constraints...) do {                    \
     union stub_exception_token res_ = { .raw = ~0 };                    \
@@ -791,9 +807,7 @@ do{ asm volatile (                                                      \
                      [stub] "rm" (stub.func) );                         \
     if ( unlikely(~res_.raw) )                                          \
     {                                                                   \
-        gprintk(XENLOG_WARNING,                                         \
-                "exception %u (ec=%04x) in emulation stub (line %u)\n", \
-                res_.fields.trapnr, res_.fields.ec, __LINE__);          \
+        invoke_stub_exception(res_.fields.trapnr, res_.fields.ec);      \
         gprintk(XENLOG_INFO, "stub: %"__stringify(MAX_INST_LEN)"ph\n",  \
                 stub.func);                                             \
         generate_exception_if(res_.fields.trapnr == EXC_UD, EXC_UD);    \
-- 
2.7.4


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

[PATCH v2 6/6] xen/arm: Remove dependency on __LINE__ for release builds

[Ross Lagerwall]

When using LivePatch, use of __LINE__ can generate spurious changes in
functions due to embedded line numbers.  For release builds with
LivePatch enabled, remove the use of these line numbers and print the
current text address instead.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/arm/traps.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 614501f..059afe7 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -82,14 +82,28 @@ static inline void check_stack_alignment_constraints(void) {
  * Compared with regular BUG_ON it dumps the guest vcpu state instead
  * of Xen's state.
  */
+#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
+#define guest_bug_on_failed(p)                          \
+do {                                                    \
+    panic("Guest Bug: %pv: '%s', address %pS\n",        \
+          current, p, current_text_addr());             \
+} while (0)
+#else
 #define guest_bug_on_failed(p)                          \
 do {                                                    \
-    show_execution_state(guest_cpu_user_regs());        \
     panic("Guest Bug: %pv: '%s', line %d, file %s\n",   \
           current, p, __LINE__, __FILE__);              \
 } while (0)
-#define GUEST_BUG_ON(p) \
-    do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0)
+#endif
+
+#define GUEST_BUG_ON(p)                                 \
+do {                                                    \
+    if ( unlikely(p) )                                  \
+    {                                                   \
+        show_execution_state(guest_cpu_user_regs());    \
+        guest_bug_on_failed(#p);                        \
+    }                                                   \
+} while (0)
 
 #ifdef CONFIG_ARM_32
 static int debug_stack_lines = 20;
-- 
2.7.4


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

Re: [PATCH v2 1/6] lib: Add a generic implementation of current_text_addr()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 510 bytes --]

On Wed, 2017-03-08 at 17:46 +0000, Ross Lagerwall wrote:
> Remove the unused x86 implementation.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>
FWIW,

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 2/6] sched: Remove dependency on __LINE__ for release builds

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2499 bytes --]

On Wed, 2017-03-08 at 17:46 +0000, Ross Lagerwall wrote:
> When using LivePatch, use of __LINE__ can generate spurious changes
> in
> functions due to embedded line numbers.  For release builds with
> LivePatch enabled, remove the use of these line numbers in
> domain_crash*() and print the current text address instead.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>
This patch looks good to me.

I wonder, though...

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -625,20 +625,28 @@ void vcpu_end_shutdown_deferral(struct vcpu
> *v);
>   * from any processor.
>   */
>  void __domain_crash(struct domain *d);
> -#define domain_crash(d) do
> {                                              \
> -    printk("domain_crash called from %s:%d\n", __FILE__,
> __LINE__);       \
> -    __domain_crash(d);                                              
>       \
> +
> +#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
> +#define _domain_crash(func, call) do
> {                                    \
> +    printk(#func " called from %pS\n",
> current_text_addr());              \
> +    call;                                                           
>       \
> +} while (0)
> +#else
> +#define _domain_crash(func, call) do
> {                                    \
> +    printk(#func " called from %s:%d\n", __FILE__,
> __LINE__);             \
> +    call;                                                           
>       \
>  } while (0)
> +#endif
> +
...if it's really worth providing both the alternatives. It's more
code, it's --to some extent-- code duplication, for very little gain.
Actually, it even results in different output depending on configure
options, which is not the end of the world, but not super nice either,
IMO.

I'd say we would be better of with having just the former macro in all
cases.

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 1/6] lib: Add a generic implementation of current_text_addr()

[Jan Beulich]

>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
> Remove the unused x86 implementation.

Unused or not - what's wrong with it? (I can guess it from the
context of the series, but the justification should be put here.)
After all it's there for debugging purposes, so if it's unused it
also can't cause any issues for LivePatch, can it?

Jan


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

Re: [PATCH v2 0/6] Remove dependency on __LINE__

[Jan Beulich]

>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
> Sorry for the long delay since the first version of this series
> (previously called "Make building xSplice patches easier").  Here is a
> set of changes that remove the use of __LINE__ when building with NDEBUG
> and LivePatch enabled.  Tested to boot on x86.  Compile-tested on arm.

What I'm missing here is an evaluation of possible alternatives, as
converting from __LINE__ to code addresses implies an extra step
when wanting to look up the place where a problem occurred,
which I generally consider undesirable. For example, I had briefly
discussed with Andrew the addition of #line directives, but iirc he
had indicated some issue with that. Furthermore there is the
option (at least for some of the more simple patches) of squeezing
more than one statement on a single line, perhaps just for patch
creation. And I guess one can think of more.

Jan


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

Re: [PATCH v2 4/6] iommu: Remove dependency on __LINE__ for release builds

[Jan Beulich]

>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:

When seeing the title I wondered by I didn't get Cc-ed. Perhaps the
prefix would better have been VT-d: ?

> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -108,6 +108,19 @@ struct acpi_atsr_unit 
> *acpi_find_matched_atsr_unit(const struct pci_dev *);
>  
>  #define DMAR_OPERATION_TIMEOUT MILLISECS(1000)
>  
> +#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
> +#define iommu_wait_op_panic()                                              \
> +    do {                                                                   \
> +        panic("%pS: DMAR hardware is malfunctional", current_text_addr()); \
> +    } while (0)
> +#else
> +#define iommu_wait_op_panic()                                              \
> +    do {                                                                   \
> +        panic("%s:%d:%s: DMAR hardware is malfunctional",                  \
> +              __FILE__, __LINE__, __func__);                               \

If you touch this already, may I suggest eliminating the redundancy
here: Either file or function name should suffice to uniquely identify
the origin.

Jan


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

Re: [PATCH v2 5/6] x86_emulate: Remove dependency on __LINE__ for release builds

[Jan Beulich]

>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
> @@ -791,9 +807,7 @@ do{ asm volatile (                                        
>               \
>                       [stub] "rm" (stub.func) );                         \
>      if ( unlikely(~res_.raw) )                                          \
>      {                                                                   \
> -        gprintk(XENLOG_WARNING,                                         \
> -                "exception %u (ec=%04x) in emulation stub (line %u)\n", \
> -                res_.fields.trapnr, res_.fields.ec, __LINE__);          \
> +        invoke_stub_exception(res_.fields.trapnr, res_.fields.ec);      \

This presumably is the worst of the changes in this series, as
explained in reply to the overview mail: The text address is going
to point into a huge function, and reconstruction of the source
line is going to be rather cumbersome in case line2addr fails
(which afaik it is not unknown for).

Jan


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

Re: [PATCH v2 0/6] Remove dependency on __LINE__

[Ross Lagerwall]

On 03/09/2017 10:34 AM, Jan Beulich wrote:
>>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
>> Sorry for the long delay since the first version of this series
>> (previously called "Make building xSplice patches easier").  Here is a
>> set of changes that remove the use of __LINE__ when building with NDEBUG
>> and LivePatch enabled.  Tested to boot on x86.  Compile-tested on arm.
>
> What I'm missing here is an evaluation of possible alternatives, as
> converting from __LINE__ to code addresses implies an extra step
> when wanting to look up the place where a problem occurred,
> which I generally consider undesirable. For example, I had briefly
> discussed with Andrew the addition of #line directives, but iirc he
> had indicated some issue with that. Furthermore there is the
> option (at least for some of the more simple patches) of squeezing
> more than one statement on a single line, perhaps just for patch
> creation. And I guess one can think of more.
>

Both of the options you suggested require extra work when building a 
live patch. This series is intended to remove that extra work as the 
process is already more difficult than I'd like.

I don't think using addr2line is much harder than looking up the line 
number directly, especially as it is something that needs to be done for 
many other crashes. On your suggestion, use of __LINE__ is disabled only 
when CONFIG_LIVEPATCH=y and NDEBUG which means that in a typical 
development scenario, you'd still get line numbers. They would be 
removed only for "release" builds in which it is likely that the source 
code & debuginfo is archived somewhere such that looking up a line 
number requires several steps anyway. I could suggest making it a 
separate config option but IIRC you prefer to limit the number of config 
options.

-- 
Ross Lagerwall

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

Re: [PATCH v2 0/6] Remove dependency on __LINE__

[Jan Beulich]

>>> On 10.03.17 at 09:29, <ross.lagerwall@citrix.com> wrote:
> On 03/09/2017 10:34 AM, Jan Beulich wrote:
>>>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
>>> Sorry for the long delay since the first version of this series
>>> (previously called "Make building xSplice patches easier").  Here is a
>>> set of changes that remove the use of __LINE__ when building with NDEBUG
>>> and LivePatch enabled.  Tested to boot on x86.  Compile-tested on arm.
>>
>> What I'm missing here is an evaluation of possible alternatives, as
>> converting from __LINE__ to code addresses implies an extra step
>> when wanting to look up the place where a problem occurred,
>> which I generally consider undesirable. For example, I had briefly
>> discussed with Andrew the addition of #line directives, but iirc he
>> had indicated some issue with that. Furthermore there is the
>> option (at least for some of the more simple patches) of squeezing
>> more than one statement on a single line, perhaps just for patch
>> creation. And I guess one can think of more.
>>
> 
> Both of the options you suggested require extra work when building a 
> live patch. This series is intended to remove that extra work as the 
> process is already more difficult than I'd like.
> 
> I don't think using addr2line is much harder than looking up the line 
> number directly, especially as it is something that needs to be done for 
> many other crashes.

I agree with the second half, but I don't think I do for the first part:
I don't know how you would manage this, but for me this is several
steps:
- Locate and download the correct package (it's not reasonable to
  keep them all, there simply are too many)
- extract xen-syms or xen.efi (and of course I will need to know
  which one was actually used)
- run addr2line on it
Quite a bit more than just going to the source file and finding the
line right away - in most cases line numbers, as opposed to code
addresses, don't change much despite a delta of a few dozen
backports or other patches, so normally one doesn't even need
to look at the _precise_ tree.

> On your suggestion, use of __LINE__ is disabled only 
> when CONFIG_LIVEPATCH=y and NDEBUG which means that in a typical 
> development scenario, you'd still get line numbers.

That's understood, but I'm concerned about extra overhead when
looking at customer reports.

> They would be 
> removed only for "release" builds in which it is likely that the source 
> code & debuginfo is archived somewhere such that looking up a line 
> number requires several steps anyway. I could suggest making it a 
> separate config option but IIRC you prefer to limit the number of config 
> options.

I prefer to limit ones with non-EXPERT-visible prompts, yes. But
I also don't see how a separate config option would improve the
situation.

Jan


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

Re: [PATCH v2 4/6] iommu: Remove dependency on __LINE__ for release builds

[Tian, Kevin]

> From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com]
> Sent: Thursday, March 9, 2017 1:47 AM
> 
> When using LivePatch, use of __LINE__ can generate spurious changes in
> functions due to embedded line numbers.  For release builds with LivePatch
> enabled, remove the use of these line numbers in
> IOMMU_WAIT_OP() and print the current text address instead.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

with title fixed per Jan's suggestion:

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

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

Re: [PATCH v2 0/6] Remove dependency on __LINE__

[Ross Lagerwall]

On 03/10/2017 08:50 AM, Jan Beulich wrote:
>>>> On 10.03.17 at 09:29, <ross.lagerwall@citrix.com> wrote:
>> On 03/09/2017 10:34 AM, Jan Beulich wrote:
>>>>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
>>>> Sorry for the long delay since the first version of this series
>>>> (previously called "Make building xSplice patches easier").  Here is a
>>>> set of changes that remove the use of __LINE__ when building with NDEBUG
>>>> and LivePatch enabled.  Tested to boot on x86.  Compile-tested on arm.
>>>
>>> What I'm missing here is an evaluation of possible alternatives, as
>>> converting from __LINE__ to code addresses implies an extra step
>>> when wanting to look up the place where a problem occurred,
>>> which I generally consider undesirable. For example, I had briefly
>>> discussed with Andrew the addition of #line directives, but iirc he
>>> had indicated some issue with that. Furthermore there is the
>>> option (at least for some of the more simple patches) of squeezing
>>> more than one statement on a single line, perhaps just for patch
>>> creation. And I guess one can think of more.
>>>
>>
>> Both of the options you suggested require extra work when building a
>> live patch. This series is intended to remove that extra work as the
>> process is already more difficult than I'd like.
>>
>> I don't think using addr2line is much harder than looking up the line
>> number directly, especially as it is something that needs to be done for
>> many other crashes.
>
> I agree with the second half, but I don't think I do for the first part:
> I don't know how you would manage this, but for me this is several
> steps:
> - Locate and download the correct package (it's not reasonable to
>   keep them all, there simply are too many)
> - extract xen-syms or xen.efi (and of course I will need to know
>   which one was actually used)
> - run addr2line on it
> Quite a bit more than just going to the source file and finding the
> line right away - in most cases line numbers, as opposed to code
> addresses, don't change much despite a delta of a few dozen
> backports or other patches, so normally one doesn't even need
> to look at the _precise_ tree.
>
>> On your suggestion, use of __LINE__ is disabled only
>> when CONFIG_LIVEPATCH=y and NDEBUG which means that in a typical
>> development scenario, you'd still get line numbers.
>
> That's understood, but I'm concerned about extra overhead when
> looking at customer reports.
>
>> They would be
>> removed only for "release" builds in which it is likely that the source
>> code & debuginfo is archived somewhere such that looking up a line
>> number requires several steps anyway. I could suggest making it a
>> separate config option but IIRC you prefer to limit the number of config
>> options.
>
> I prefer to limit ones with non-EXPERT-visible prompts, yes. But
> I also don't see how a separate config option would improve the
> situation.
>

My suggestion was to have a separate config option so that the person 
configuring Xen could choose in the trade off between having line 
numbers in debug messages or easier live patching. Does that seem 
reasonable?

-- 
Ross Lagerwall

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

Re: [PATCH v2 0/6] Remove dependency on __LINE__

[Jan Beulich]

>>> On 17.03.17 at 09:57, <ross.lagerwall@citrix.com> wrote:
> On 03/10/2017 08:50 AM, Jan Beulich wrote:
>>>>> On 10.03.17 at 09:29, <ross.lagerwall@citrix.com> wrote:
>>> They would be
>>> removed only for "release" builds in which it is likely that the source
>>> code & debuginfo is archived somewhere such that looking up a line
>>> number requires several steps anyway. I could suggest making it a
>>> separate config option but IIRC you prefer to limit the number of config
>>> options.
>>
>> I prefer to limit ones with non-EXPERT-visible prompts, yes. But
>> I also don't see how a separate config option would improve the
>> situation.
>>
> 
> My suggestion was to have a separate config option so that the person 
> configuring Xen could choose in the trade off between having line 
> numbers in debug messages or easier live patching. Does that seem 
> reasonable?

Sure. I was merely trying to explain a little more the background of
my preference to limit config options, as implications result from that
for what the new option should look like afaic.

Jan


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

Re: [PATCH v2 3/6] mm: Use statically defined locking order

[George Dunlap]

On 08/03/17 17:46, Ross Lagerwall wrote:
> Instead of using a locking order based on line numbers which interacts
> poorly with trying to create a live patch, statically define the locking
> order.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

> ---
> 
> Changes in v2:
> * Rearranged defines.
> * Make lock orders fit in a sign-extended 8-bit immediate.
> 
>  xen/arch/x86/mm/mm-locks.h | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> index 74fdfc1..e5fceb2 100644
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -46,8 +46,10 @@ static inline int mm_locked_by_me(mm_lock_t *l)
>      return (l->lock.recurse_cpu == current->processor);
>  }
>  
> -/* If you see this crash, the numbers printed are lines in this file 
> - * where the offending locks are declared. */
> +/*
> + * If you see this crash, the numbers printed are order levels defined
> + * in this file.
> + */
>  #define __check_lock_level(l)                           \
>  do {                                                    \
>      if ( unlikely(__get_lock_level() > (l)) )           \
> @@ -152,12 +154,12 @@ static inline void mm_read_unlock(mm_rwlock_t *l)
>  /* This wrapper uses the line number to express the locking order below */
>  #define declare_mm_lock(name)                                                 \
>      static inline void mm_lock_##name(mm_lock_t *l, const char *func, int rec)\
> -    { _mm_lock(l, func, __LINE__, rec); }
> +    { _mm_lock(l, func, MM_LOCK_ORDER_##name, rec); }
>  #define declare_mm_rwlock(name)                                               \
>      static inline void mm_write_lock_##name(mm_rwlock_t *l, const char *func) \
> -    { _mm_write_lock(l, func, __LINE__); }                                    \
> +    { _mm_write_lock(l, func, MM_LOCK_ORDER_##name); }                                    \
>      static inline void mm_read_lock_##name(mm_rwlock_t *l)                    \
> -    { _mm_read_lock(l, __LINE__); }
> +    { _mm_read_lock(l, MM_LOCK_ORDER_##name); }
>  /* These capture the name of the calling function */
>  #define mm_lock(name, l) mm_lock_##name(l, __func__, 0)
>  #define mm_lock_recursive(name, l) mm_lock_##name(l, __func__, 1)
> @@ -169,10 +171,10 @@ static inline void mm_read_unlock(mm_rwlock_t *l)
>   * to ordering constraints. */
>  #define declare_mm_order_constraint(name)                                   \
>      static inline void mm_enforce_order_lock_pre_##name(void)               \
> -    { _mm_enforce_order_lock_pre(__LINE__); }                               \
> +    { _mm_enforce_order_lock_pre(MM_LOCK_ORDER_##name); }                               \
>      static inline void mm_enforce_order_lock_post_##name(                   \
>                          int *unlock_level, unsigned short *recurse_count)   \
> -    { _mm_enforce_order_lock_post(__LINE__, unlock_level, recurse_count); } \
> +    { _mm_enforce_order_lock_post(MM_LOCK_ORDER_##name, unlock_level, recurse_count); } \
>  
>  static inline void mm_unlock(mm_lock_t *l)
>  {
> @@ -201,8 +203,8 @@ static inline void mm_enforce_order_unlock(int unlock_level,
>  
>  /************************************************************************
>   *                                                                      *
> - * To avoid deadlocks, these locks _MUST_ be taken in the order they're *
> - * declared in this file.  The locking functions will enforce this.     *
> + * To avoid deadlocks, these locks _MUST_ be taken in the order listed  *
> + * below.  The locking functions will enforce this.                     *
>   *                                                                      *
>   ************************************************************************/
>  
> @@ -214,6 +216,7 @@ static inline void mm_enforce_order_unlock(int unlock_level,
>   * - setting the "cr3" field of any p2m table to a non-P2M_BASE_EAADR value.
>   *   (i.e. assigning a p2m table to be the shadow of that cr3 */
>  
> +#define MM_LOCK_ORDER_nestedp2m               8
>  declare_mm_lock(nestedp2m)
>  #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
>  #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
> @@ -240,6 +243,7 @@ declare_mm_lock(nestedp2m)
>   * the altp2mlist lock in the middle.
>   */
>  
> +#define MM_LOCK_ORDER_p2m                    16
>  declare_mm_rwlock(p2m);
>  
>  /* Sharing per page lock
> @@ -251,6 +255,7 @@ declare_mm_rwlock(p2m);
>   *
>   * The lock is recursive because during share we lock two pages. */
>  
> +#define MM_LOCK_ORDER_per_page_sharing       24
>  declare_mm_order_constraint(per_page_sharing)
>  #define page_sharing_mm_pre_lock()   mm_enforce_order_lock_pre_per_page_sharing()
>  #define page_sharing_mm_post_lock(l, r) \
> @@ -265,6 +270,7 @@ declare_mm_order_constraint(per_page_sharing)
>   * in the target domain must be paused.
>   */
>  
> +#define MM_LOCK_ORDER_altp2mlist             32
>  declare_mm_lock(altp2mlist)
>  #define altp2m_list_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_list_lock)
>  #define altp2m_list_unlock(d) mm_unlock(&(d)->arch.altp2m_list_lock)
> @@ -279,6 +285,7 @@ declare_mm_lock(altp2mlist)
>   * up a gfn and later mutate it.
>   */
>  
> +#define MM_LOCK_ORDER_altp2m                 40
>  declare_mm_rwlock(altp2m);
>  #define p2m_lock(p)                             \
>      do {                                        \
> @@ -307,6 +314,7 @@ declare_mm_rwlock(altp2m);
>   * Protects private PoD data structs: entry and cache
>   * counts, page lists, sweep parameters. */
>  
> +#define MM_LOCK_ORDER_pod                    48
>  declare_mm_lock(pod)
>  #define pod_lock(p)           mm_lock(pod, &(p)->pod.lock)
>  #define pod_unlock(p)         mm_unlock(&(p)->pod.lock)
> @@ -319,6 +327,7 @@ declare_mm_lock(pod)
>   * the ordering which we enforce here.
>   * The lock is not recursive. */
>  
> +#define MM_LOCK_ORDER_page_alloc             56
>  declare_mm_order_constraint(page_alloc)
>  #define page_alloc_mm_pre_lock()   mm_enforce_order_lock_pre_page_alloc()
>  #define page_alloc_mm_post_lock(l) mm_enforce_order_lock_post_page_alloc(&(l), NULL)
> @@ -339,6 +348,7 @@ declare_mm_order_constraint(page_alloc)
>   * It also protects the log-dirty bitmap from concurrent accesses (and
>   * teardowns, etc). */
>  
> +#define MM_LOCK_ORDER_paging                 64
>  declare_mm_lock(paging)
>  #define paging_lock(d)         mm_lock(paging, &(d)->arch.paging.lock)
>  #define paging_lock_recursive(d) \
> 


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

Re: [PATCH v2 4/6] iommu: Remove dependency on __LINE__ for release builds

[Jan Beulich]

>>> On 09.03.17 at 11:42,  wrote:
>>>> On 08.03.17 at 18:46, <ross.lagerwall@citrix.com> wrote:
> 
> When seeing the title I wondered by I didn't get Cc-ed. Perhaps the
> prefix would better have been VT-d: ?
> 
> > --- a/xen/drivers/passthrough/vtd/dmar.h
> > +++ b/xen/drivers/passthrough/vtd/dmar.h
> > @@ -108,6 +108,19 @@ struct acpi_atsr_unit 
> > *acpi_find_matched_atsr_unit(const struct pci_dev *);
> >  
> >  #define DMAR_OPERATION_TIMEOUT MILLISECS(1000)
> >  
> > +#if defined(NDEBUG) && defined(CONFIG_LIVEPATCH)
> > +#define iommu_wait_op_panic()                                              \
> > +    do {                                                                   \
> > +        panic("%pS: DMAR hardware is malfunctional", current_text_addr()); \
> > +    } while (0)
> > +#else
> > +#define iommu_wait_op_panic()                                              \
> > +    do {                                                                   \
> > +        panic("%s:%d:%s: DMAR hardware is malfunctional",                  \
> > +              __FILE__, __LINE__, __func__);                               \
> 
> If you touch this already, may I suggest eliminating the redundancy
> here: Either file or function name should suffice to uniquely identify
> the origin.

Actually, coming back to this old patch: Would there be anything
wrong with using the __LINE__-less variant in all cases?

Jan


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