[PATCH V2 0/9] xentrace/xenalyze support on ARM

[PATCH V2 0/9] xentrace/xenalyze support on ARM

[Benjamin Sanda]

This patch set adds support for xentrace/xenalyze to the ARM platform.

The Xen heap memory mapping, timestamping, and P2M translation needed
by xentrace is corrected for operation on the ARM platform using the
x86 platform as reference. Trace buffer initialization is added to
setup.c, XENMAPSPACE_gmfn_foreign page mapping and address translation
for DOMID_XEN is corrected in mm.c and p2m.c, and timestamping for the
trace buffers is corrected in time.c/.h.

Finally the xenaylze makefile is configured to build the tool for ARM.

---
Changed since v1:
  * Removed Flask changes as deemed uncessesary and unclear in 
    purpose
  * Corrected all commit messages to be line limited to 72 chars
  * Implimented v1 review comments as indicated in each file's
    commit log.

Benjamin Sanda (9):
  xenalyze: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform

 tools/xentrace/Makefile    |  3 +--
 xen/arch/arm/mm.c          |  3 ++-
 xen/arch/arm/p2m.c         | 35 +++++++++++++++++++++++++++----
 xen/arch/arm/setup.c       |  3 +++
 xen/arch/arm/time.c        |  9 +++++++-
 xen/arch/x86/mm.c          | 48 -------------------------------------------
 xen/common/page_alloc.c    | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/time.h | 11 +++++-----
 xen/include/xen/mm.h       |  2 ++
 9 files changed, 103 insertions(+), 62 deletions(-)

-- 
2.5.0


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

[PATCH V2 0/9] xentrace/xenalyze support on ARM

[Benjamin Sanda]

This patch set adds support for xentrace/xenalyze to the ARM platform.

The Xen heap memory mapping, timestamping, and P2M translation needed
by xentrace is corrected for operation on the ARM platform using the
x86 platform as reference. Trace buffer initialization is added to
setup.c, XENMAPSPACE_gmfn_foreign page mapping and address translation
for DOMID_XEN is corrected in mm.c and p2m.c, and timestamping for the
trace buffers is corrected in time.c/.h.

Finally the xenaylze makefile is configured to build the tool for ARM.

Benjamin Sanda (9):
  xenalyze: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform
  xentrace: Support for ARM platform

 tools/xentrace/Makefile    |  3 +--
 xen/arch/arm/mm.c          |  3 ++-
 xen/arch/arm/p2m.c         | 35 +++++++++++++++++++++++++++----
 xen/arch/arm/setup.c       |  3 +++
 xen/arch/arm/time.c        |  9 +++++++-
 xen/arch/x86/mm.c          | 48 -------------------------------------------
 xen/common/page_alloc.c    | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/time.h | 11 +++++-----
 xen/include/xen/mm.h       |  2 ++
 9 files changed, 103 insertions(+), 62 deletions(-)

-- 
2.5.0


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

[PATCH V2 1/9] xenalyze: Support for ARM platform

[Benjamin Sanda]

Modified to provide building of the xenalyze binary for ARM platforms.
This was done in conjunction with patches to xentrace allowing its use
on ARM. The xenalyze binary is now built as part of the BIN list.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

---
Changed since v1:
  * changed xenalyze target from SBIN to BIN
  * removed uneeded $(BIN-y) from BIN list
---
 tools/xentrace/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
index 0157be2..edcc5a7 100644
--- a/tools/xentrace/Makefile
+++ b/tools/xentrace/Makefile
@@ -9,8 +9,7 @@ LDLIBS += $(LDLIBS_libxenevtchn)
 LDLIBS += $(LDLIBS_libxenctrl)
 LDLIBS += $(ARGP_LDFLAGS)
 
-BIN-$(CONFIG_X86) = xenalyze
-BIN      = $(BIN-y)
+BIN      = xenalyze
 SBIN     = xentrace xentrace_setsize
 LIBBIN   = xenctx
 SCRIPTS  = xentrace_format
-- 
2.5.0


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

Re: [PATCH V2 1/9] xenalyze: Support for ARM platform

[Wei Liu]

I would retitle this patch:

  xentrace: build for both x86 and ARM platform

to be more specific.

On Fri, Apr 01, 2016 at 04:33:46PM -0400, Benjamin Sanda wrote:
> Modified to provide building of the xenalyze binary for ARM platforms.
> This was done in conjunction with patches to xentrace allowing its use
> on ARM. The xenalyze binary is now built as part of the BIN list.
> 
> Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
> 

Acked-by: Wei Liu <wei.liu2@citrix.com>

And I think this patch is better to be moved to the end of your series
because it depends on hypervisor support.

Wei.

> ---
> Changed since v1:
>   * changed xenalyze target from SBIN to BIN
>   * removed uneeded $(BIN-y) from BIN list
> ---
>  tools/xentrace/Makefile | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
> index 0157be2..edcc5a7 100644
> --- a/tools/xentrace/Makefile
> +++ b/tools/xentrace/Makefile
> @@ -9,8 +9,7 @@ LDLIBS += $(LDLIBS_libxenevtchn)
>  LDLIBS += $(LDLIBS_libxenctrl)
>  LDLIBS += $(ARGP_LDFLAGS)
>  
> -BIN-$(CONFIG_X86) = xenalyze
> -BIN      = $(BIN-y)
> +BIN      = xenalyze
>  SBIN     = xentrace xentrace_setsize
>  LIBBIN   = xenctx
>  SCRIPTS  = xentrace_format
> -- 
> 2.5.0
> 

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

Re: [PATCH V2 1/9] xenalyze: Support for ARM platform

[Wei Liu]

On Mon, Apr 04, 2016 at 03:10:46PM +0100, Wei Liu wrote:
> I would retitle this patch:
> 
>   xentrace: build for both x86 and ARM platform
    ^
    xenalyze

Sorry for the typo.

Wei.

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

[PATCH V2 2/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Modified xenmem_add_to_physmap_one() to provide support for xentrace
on the ARM platform. Checks for DOMID_XEN added via new function,
get_pg_owner, ported and commonized from x86 code base. This provides
correct calls to rcu_lock_domain() when DOMID_XEN is requested.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

---
Changed since v1:

  * Moved get_pg_owner() from the arch specific mm.c source files into
    the common page_alloc.c source. This was done as get_pg_owner() is
    now needed by both architectures and is essentially identical in
    both.
  * Moved put_pg_owner from the arch specific mm.c source files into
    the common page_alloc.c source to make available to all platforms.
  * Removed DOMID_XEN check from page_to_mfn(page) call in
    xenmem_add_to_physmap_one() per review comment (check considered
    to be in excess of need).
  * Removed forward declare of get_pg_owner from mm.c (arm) as it is
    now in the mm.h common header.
---
 xen/arch/arm/mm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 81f9e2e..19d6c2c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1099,7 +1099,8 @@ int xenmem_add_to_physmap_one(
     {
         struct domain *od;
         p2m_type_t p2mt;
-        od = rcu_lock_domain_by_any_id(foreign_domid);
+        od = get_pg_owner(foreign_domid);
+
         if ( od == NULL )
             return -ESRCH;
 
-- 
2.5.0


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

[PATCH V2 3/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Moved get_pg_owner() and put_pg_owner() from the arch specific mm.c
source files into the common page_alloc.c source. This was done as
theses functions are now needed by both architectures to support
xentrace on the ARM platform. Forward declarations were added to mm.h.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
---
 xen/arch/x86/mm.c | 48 ------------------------------------------------
 1 file changed, 48 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c997b53..0d695dd 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2998,54 +2998,6 @@ int new_guest_cr3(unsigned long mfn)
     return rc;
 }
 
-static struct domain *get_pg_owner(domid_t domid)
-{
-    struct domain *pg_owner = NULL, *curr = current->domain;
-
-    if ( likely(domid == DOMID_SELF) )
-    {
-        pg_owner = rcu_lock_current_domain();
-        goto out;
-    }
-
-    if ( unlikely(domid == curr->domain_id) )
-    {
-        MEM_LOG("Cannot specify itself as foreign domain");
-        goto out;
-    }
-
-    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
-    {
-        MEM_LOG("Cannot mix foreign mappings with translated domains");
-        goto out;
-    }
-
-    switch ( domid )
-    {
-    case DOMID_IO:
-        pg_owner = rcu_lock_domain(dom_io);
-        break;
-    case DOMID_XEN:
-        pg_owner = rcu_lock_domain(dom_xen);
-        break;
-    default:
-        if ( (pg_owner = rcu_lock_domain_by_id(domid)) == NULL )
-        {
-            MEM_LOG("Unknown domain '%u'", domid);
-            break;
-        }
-        break;
-    }
-
- out:
-    return pg_owner;
-}
-
-static void put_pg_owner(struct domain *pg_owner)
-{
-    rcu_unlock_domain(pg_owner);
-}
-
 static inline int vcpumask_to_pcpumask(
     struct domain *d, XEN_GUEST_HANDLE_PARAM(const_void) bmap, cpumask_t *pmask)
 {
-- 
2.5.0


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

[PATCH V2 4/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Moved get_pg_owner() and put_pg_owner() from the arch specific mm.c
source files into the common page_alloc.c source. This was done as
theses functions are now needed by both architectures to support
xentrace on the ARM platform. Forward declarations were added to mm.h.

One conditional compilation check was added in get_pg_owner() for the
is_pvh_domain(curr) check which is only valid to perform on X86
platforms.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
---
 xen/common/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 98e30e5..8fe9c03 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -145,6 +145,7 @@
 #ifdef CONFIG_X86
 #include <asm/p2m.h>
 #include <asm/setup.h> /* for highmem_start only */
+#include <asm/paging.h>
 #else
 #define p2m_pod_offline_or_broken_hit(pg) 0
 #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
@@ -1996,6 +1997,56 @@ static __init int register_heap_trigger(void)
 }
 __initcall(register_heap_trigger);
 
+struct domain *get_pg_owner(domid_t domid)
+{
+    struct domain *pg_owner = NULL, *curr = current->domain;
+
+    if ( likely(domid == DOMID_SELF) )
+    {
+        pg_owner = rcu_lock_current_domain();
+        goto out;
+    }
+
+    if ( unlikely(domid == curr->domain_id) )
+    {
+        gdprintk(XENLOG_WARNING,"Cannot specify itself as foreign domain");
+        goto out;
+    }
+
+#ifdef CONFIG_X86
+    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
+    {
+        gdprintk(XENLOG_WARNING,"Cannot mix foreign mappings with translated domains");
+        goto out;
+    }
+#endif
+
+    switch ( domid )
+    {
+    case DOMID_IO:
+        pg_owner = rcu_lock_domain(dom_io);
+        break;
+    case DOMID_XEN:
+        pg_owner = rcu_lock_domain(dom_xen);
+        break;
+    default:
+        if ( (pg_owner = rcu_lock_domain_by_id(domid)) == NULL )
+        {
+            gdprintk(XENLOG_WARNING,"Unknown domain '%u'", domid);
+            break;
+        }
+        break;
+    }
+
+ out:
+    return pg_owner;
+}
+
+void put_pg_owner(struct domain *pg_owner)
+{
+    rcu_unlock_domain(pg_owner);
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.5.0


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

[PATCH V2 5/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Moved get_pg_owner() and put_pg_owner() from the arch specific mm.c
source files into the common page_alloc.c source. This was done as
theses functions are now needed by both architectures to support
xentrace on the ARM platform. Forward declarations were added to mm.h.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
---
 xen/include/xen/mm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d62394f..7b4dd87 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -505,6 +505,8 @@ void scrub_one_page(struct page_info *);
 int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
                               domid_t foreign_domid,
                               unsigned long idx, xen_pfn_t gpfn);
+struct domain *get_pg_owner(domid_t domid);
+void put_pg_owner(struct domain *pg_owner);
 
 /* Returns 1 on success, 0 on error, negative if the ring
  * for event propagation is full in the presence of paging */
-- 
2.5.0


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

[PATCH V2 6/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Modified to provide support for xentrace on the ARM platform. Moved
get_cycles() to time.c and modified to return the core timestamp tick
count for use by the trace buffer timestamping routines in xentrace.
get_cycles() was moved to the C file to avoid including the register
specific header file in time.h and to commonize it with the
get_s_time() function.

get_s_time() was also modified to now use the updated get_cycles() to
retrieve the tick count instead of directly reading it.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
---
 xen/arch/arm/time.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 7dae28b..9aface3 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -192,10 +192,17 @@ int __init init_xen_time(void)
 /* Return number of nanoseconds since boot */
 s_time_t get_s_time(void)
 {
-    uint64_t ticks = READ_SYSREG64(CNTPCT_EL0) - boot_count;
+    cycles_t ticks = get_cycles();
     return ticks_to_ns(ticks);
 }
 
+/* Return the number of ticks since boot */
+cycles_t get_cycles(void)
+{
+        /* return raw tick count of main timer */
+        return READ_SYSREG64(CNTPCT_EL0) - boot_count;
+}
+
 /* Set the timer to wake us up at a particular time.
  * Timeout is a Xen system time (nanoseconds since boot); 0 disables the timer.
  * Returns 1 on success; 0 if the timeout is too soon or is in the past. */
-- 
2.5.0


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

[PATCH V2 7/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Modified to provide support for xentrace on the ARM platform. Moved
get_cycles() to time.c and added function prototype for inclusion.
get_cycles() was moved to the C file to avoid including the register
specific header file in the time.h file and to commonize it with the
get_s_time() function. Also defined cycles_t as uint64_t to simplify
casting.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

---
Changed since v1:
  * Moved get_cycles() to time.c
  * Added function prototype for get_cycles()
---
 xen/include/asm-arm/time.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 5b9a31d..b57f4c1 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -5,12 +5,8 @@
     DT_MATCH_COMPATIBLE("arm,armv7-timer"), \
     DT_MATCH_COMPATIBLE("arm,armv8-timer")
 
-typedef unsigned long cycles_t;
-
-static inline cycles_t get_cycles (void)
-{
-        return 0;
-}
+/* Tick count type */
+typedef uint64_t cycles_t;
 
 /* List of timer's IRQ */
 enum timer_ppi
@@ -37,6 +33,9 @@ extern void init_timer_interrupt(void);
 /* Counter value at boot time */
 extern uint64_t boot_count;
 
+/* Get raw system tick count */
+cycles_t get_cycles(void);
+
 extern s_time_t ticks_to_ns(uint64_t ticks);
 extern uint64_t ns_to_ticks(s_time_t ns);
 
-- 
2.5.0


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

[PATCH V2 8/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Modified p2m_lookup() to provide support for xentrace on the ARM
platform. Added check for DOMID_XEN which skips PFN to MFN
translation. DOMID_XEN is considered a PV guest on x86 (i.e MFN ==
GFN), but on ARM there is no such concept. Thus requests to DOMID_XEN
on ARM use a MFN address directly and do not need translation from
PFN.

Check added to determine read/write or read only page type when
requesting DOMID_XEN. This is  done by checking the u.inuse.type_info
parameter of the requested page. The page rw/ro  paddr_t type is then
set accordingly.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

---
Changed since v1:
  * Added check to determine page rw/ro type to correctly set the
    paddr_t to p2m_ram_rw or p2m_ram_ro instead of assuming p2m_ram_rw
  * Corrected block comment format to conform to Xen coding standard
---
 xen/arch/arm/p2m.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a2a9c4b..a99b670 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -227,11 +227,38 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
 {
     paddr_t ret;
     struct p2m_domain *p2m = &d->arch.p2m;
+    struct page_info *page;
+    unsigned long mfn;
 
-    spin_lock(&p2m->lock);
-    ret = __p2m_lookup(d, paddr, t);
-    spin_unlock(&p2m->lock);
-
+    /*
+    * DOMID_XEN is considered a PV guest on x86 (i.e MFN == GFN), but
+    * on ARM there is no such concept. Thus requests to DOMID_XEN
+    * on ARM use a MFN address directly and do not need translation 
+    * from PFN.
+    */
+    if(DOMID_XEN != d->domain_id)
+    {
+        spin_lock(&p2m->lock);
+        ret = __p2m_lookup(d, paddr, t);
+        spin_unlock(&p2m->lock);
+    }
+    else
+    {
+        /* retrieve the page to determine read/write or read only mapping */
+        mfn = paddr >> PAGE_SHIFT;
+        if (mfn_valid(mfn))
+        {
+            page = mfn_to_page(mfn);
+            *t = (page->u.inuse.type_info == PGT_writable_page ? 
+                                p2m_ram_rw : p2m_ram_ro);
+        }
+        else
+        {
+            *t = p2m_invalid;
+        }
+        ret = paddr;
+    }
+    
     return ret;
 }
 
-- 
2.5.0


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

[PATCH V2 9/9] xentrace: Support for ARM platform

[Benjamin Sanda]

Modified to provide support for xentrace on the ARM platform. Added
call to init_trace_bufs() to initialize the trace buffers for use by
xentrace.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

---
Changed since v1:
  * No changes
---
 xen/arch/arm/setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6d205a9..87e70c9 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -35,6 +35,7 @@
 #include <xen/cpu.h>
 #include <xen/pfn.h>
 #include <xen/vmap.h>
+#include <xen/trace.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/acpi.h>
 #include <asm/page.h>
@@ -851,6 +852,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* Scrub RAM that is still free and so may go to an unprivileged domain. */
     scrub_heap_pages();
 
+    init_trace_bufs();
+
     init_constructors();
 
     console_endboot();
-- 
2.5.0


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

Re: [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Wei Liu]

On Fri, Apr 01, 2016 at 04:33:44PM -0400, Benjamin Sanda wrote:
> This patch set adds support for xentrace/xenalyze to the ARM platform.
> 
> The Xen heap memory mapping, timestamping, and P2M translation needed
> by xentrace is corrected for operation on the ARM platform using the
> x86 platform as reference. Trace buffer initialization is added to
> setup.c, XENMAPSPACE_gmfn_foreign page mapping and address translation
> for DOMID_XEN is corrected in mm.c and p2m.c, and timestamping for the
> trace buffers is corrected in time.c/.h.
> 
> Finally the xenaylze makefile is configured to build the tool for ARM.
> 
> ---
> Changed since v1:
>   * Removed Flask changes as deemed uncessesary and unclear in 
>     purpose
>   * Corrected all commit messages to be line limited to 72 chars
>   * Implimented v1 review comments as indicated in each file's
>     commit log.
> 
> Benjamin Sanda (9):
>   xenalyze: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform
>   xentrace: Support for ARM platform

You patches all have the same subject line.  Please make them more
specific. See my reply to #1 for example.

Wei.

> 
>  tools/xentrace/Makefile    |  3 +--
>  xen/arch/arm/mm.c          |  3 ++-
>  xen/arch/arm/p2m.c         | 35 +++++++++++++++++++++++++++----
>  xen/arch/arm/setup.c       |  3 +++
>  xen/arch/arm/time.c        |  9 +++++++-
>  xen/arch/x86/mm.c          | 48 -------------------------------------------
>  xen/common/page_alloc.c    | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/time.h | 11 +++++-----
>  xen/include/xen/mm.h       |  2 ++
>  9 files changed, 103 insertions(+), 62 deletions(-)
> 
> -- 
> 2.5.0
> 

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

Re: [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Julien Grall]

Hello,

On 04/04/2016 15:11, Wei Liu wrote:
> On Fri, Apr 01, 2016 at 04:33:44PM -0400, Benjamin Sanda wrote:
>> ---
>> Changed since v1:
>>    * Removed Flask changes as deemed uncessesary and unclear in
>>      purpose
>>    * Corrected all commit messages to be line limited to 72 chars
>>    * Implimented v1 review comments as indicated in each file's
>>      commit log.
>>
>> Benjamin Sanda (9):
>>    xenalyze: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>>    xentrace: Support for ARM platform
>
> You patches all have the same subject line.  Please make them more
> specific. See my reply to #1 for example.

+1

Also, you should at least check that Xen still builds after applying 
each patch. Ideally, you also need to be careful to not break any 
feature currently supported. It's useful when someone needs to bisect 
the tree.

For instance, you use the function get_pg_owner for ARM in patch #2 but 
introduce the function in patch #4. This will break ARM build. So the 
patch #2 should be moved after #4.

Furthermore, you remove the functions get_pg_owner and put_pg_owner for 
x86 in patch #3 and then re-introduced them in patch #4. Therefore, the 
x86 will be broken after #3. In this case, it's better to have a patch 
that move the 2 functions from x86 to common.

Finally, please CC all the x86 maintainers (Jan and Andrew) on x86 
changes. You need to manually check the CCs as under certain conditions 
the script get_maintainers.pl may not return all the relevant maintainers.

I will wait the next iteration of this series to review the code.

Regards,

-- 
Julien Grall

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

Re: [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Ben Sanda]

Julien and Wei,

>> You patches all have the same subject line.  Please make them more 
>> specific. See my reply to #1 for example.
>
> +1
> 
> Also, you should at least check that Xen still builds after applying
> each patch. Ideally, you also need to be careful to not break any
> feature currently supported. It's useful when someone needs to 
> bisect the tree.
>
> For instance, you use the function get_pg_owner for ARM in patch #2 
> but introduce the function in patch #4. This will break ARM build. 
> So the patch #2 should be moved after #4.
>
> Furthermore, you remove the functions get_pg_owner and put_pg_owner 
> for x86 in patch #3 and then re-introduced them in patch #4. 
> Therefore, the x86 will be broken after #3. In this case, it's better
> to have a patch that move the 2 functions from x86 to common.

Thank you for the comments. I apologize for the errors in the patch
format. This is my first time submitting a patch to Xen and I was
unaware that the patch set order mattered or that I had to account for
a piecewise application of the patch set. I will attempt to resubmit
with this corrected and the patch names updated. 

So then it is permissible to have multiple file changes in one
patch/commit? E.g. a patch that removes from one file and  adds to
another in the same commit. I initially thought each patch/ commit was
only supposed to modify one file and that's why I did it that way

Thank you,
Ben Sanda

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

Re: [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Wei Liu]

On Mon, Apr 04, 2016 at 03:50:35PM +0000, Ben Sanda wrote:
> Julien and Wei,
> 
> >> You patches all have the same subject line.  Please make them more 
> >> specific. See my reply to #1 for example.
> >
> > +1
> > 
> > Also, you should at least check that Xen still builds after applying
> > each patch. Ideally, you also need to be careful to not break any
> > feature currently supported. It's useful when someone needs to 
> > bisect the tree.
> >
> > For instance, you use the function get_pg_owner for ARM in patch #2 
> > but introduce the function in patch #4. This will break ARM build. 
> > So the patch #2 should be moved after #4.
> >
> > Furthermore, you remove the functions get_pg_owner and put_pg_owner 
> > for x86 in patch #3 and then re-introduced them in patch #4. 
> > Therefore, the x86 will be broken after #3. In this case, it's better
> > to have a patch that move the 2 functions from x86 to common.
> 
> Thank you for the comments. I apologize for the errors in the patch
> format. This is my first time submitting a patch to Xen and I was
> unaware that the patch set order mattered or that I had to account for
> a piecewise application of the patch set. I will attempt to resubmit
> with this corrected and the patch names updated. 
> 
> So then it is permissible to have multiple file changes in one
> patch/commit? E.g. a patch that removes from one file and  adds to

That's definitely allowed. Just think of each commit as a logically
complete unit. It should compile. It should not break existing
functionality.

Wei.

> another in the same commit. I initially thought each patch/ commit was
> only supposed to modify one file and that's why I did it that way
> 
> Thank you,
> Ben Sanda

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

Re: [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Andrew Cooper]

On 04/04/16 16:50, Ben Sanda wrote:
> Julien and Wei,
>
>>> You patches all have the same subject line.  Please make them more 
>>> specific. See my reply to #1 for example.
>> +1
>>
>> Also, you should at least check that Xen still builds after applying
>> each patch. Ideally, you also need to be careful to not break any
>> feature currently supported. It's useful when someone needs to 
>> bisect the tree.
>>
>> For instance, you use the function get_pg_owner for ARM in patch #2 
>> but introduce the function in patch #4. This will break ARM build. 
>> So the patch #2 should be moved after #4.
>>
>> Furthermore, you remove the functions get_pg_owner and put_pg_owner 
>> for x86 in patch #3 and then re-introduced them in patch #4. 
>> Therefore, the x86 will be broken after #3. In this case, it's better
>> to have a patch that move the 2 functions from x86 to common.
> Thank you for the comments. I apologize for the errors in the patch
> format. This is my first time submitting a patch to Xen and I was
> unaware that the patch set order mattered or that I had to account for
> a piecewise application of the patch set. I will attempt to resubmit
> with this corrected and the patch names updated. 
>
> So then it is permissible to have multiple file changes in one
> patch/commit? E.g. a patch that removes from one file and  adds to
> another in the same commit. I initially thought each patch/ commit was
> only supposed to modify one file and that's why I did it that way

One logical change per patch (wherever possible).  It is fine to change
more than one file in a patch.

In particular, your patches 3 though 5 are one logical change, "move
get_pg_owner() from being x86-specific to being common".

It is an absolute must that each invidiual patch compiles and doesn't
regress functionality, so the resulting patches can be bisected in the
case of an error being introduced.

~Andrew

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