[PATCH v3 0/5] xentrace/xenalyze Support on ARM

[PATCH v3 0/5] 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 v2:
  * Merged previous single file patches into atomic patches which can
    be applied and compiled independently. 
  * Updated individual patch names to be more descriptive.
  * Correct order of patches in patch set to provide correct
    application/build order.

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

Benjamin Sanda (5):
  xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86
  xentrace: Memory/Page Mapping support for DOMID_XEN on ARM
  xentrace: Timestamp support for ARM platform
  xentrace: Trace Buffer Initialization on ARM
  xenalyze: Build for Both ARM and x86 Platforms

 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 v3 1/5] xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86

[Benjamin Sanda]

Moved get_pg_owner() and put_pg_owner() from the arch specific x86
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>

---
Changed since v2:
  * Combined patches 3-5 from v2 into one patch for v3. No code change.
---
 xen/arch/x86/mm.c       | 48 ----------------------------------------------
 xen/common/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/mm.h    |  2 ++
 3 files changed, 53 insertions(+), 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)
 {
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
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 v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Benjamin Sanda]

Modified ARM arch specific p2m.c and mm.c files to provide support for
xentrace. Case for DOMID_XEN added xenmem_add_to_physmap_one() when
XENMAPSPACE_gmfn_foreign domain requested via get_pg_owner. This
provides correct calls to rcu_lock_domain() when DOMID_XEN is
requested.

Check for DOMID_XEN added to p2m_lookup() 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 p2m_lookup() 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.

---
Changed since v2:

  * Combined v2 patches 2 and 8 into one patch for v3. No code changes.

---
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.
  * 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

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
---
 xen/arch/arm/mm.c  |  3 ++-
 xen/arch/arm/p2m.c | 35 +++++++++++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 5 deletions(-)

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;
 
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 v3 3/5] xentrace: Timestamp support for ARM platform

[Benjamin Sanda]

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. Also defined cycles_t as uint64_t to simplify
casting.

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>

---
Changed since v2:
  * Combined v2 patches 7 and 6 into one patch in v3. No code change.

---
Changed since v1:
  * Moved get_cycles() to time.c
  * Added function prototype for get_cycles()
---
 xen/arch/arm/time.c        |  9 ++++++++-
 xen/include/asm-arm/time.h | 11 +++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

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. */
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 v3 4/5] xentrace: Trace Buffer Initialization on ARM

[Benjamin Sanda]

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 v2:
  * No changes

---
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

[PATCH v3 5/5] xenalyze: Build for Both ARM and x86 Platforms

[Benjamin Sanda]

Modified to provide building of the xenalyze binary for both ARM and
x86 platforms. The xenalyze binary is now built as part of the BIN
list for both platforms.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>

---
Changed since v2:
  * No changes.

---
Changed since v1:
  * Changed xenalyze target from SBIN to BIN
  * Removed unneeded $(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 v3 1/5] xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86

[Andrew Cooper]

On 04/04/2016 19:48, Benjamin Sanda wrote:
> Moved get_pg_owner() and put_pg_owner() from the arch specific x86
> 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>
>
> ---
> Changed since v2:
>   * Combined patches 3-5 from v2 into one patch for v3. No code change.
> ---
>  xen/arch/x86/mm.c       | 48 ----------------------------------------------
>  xen/common/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/mm.h    |  2 ++
>  3 files changed, 53 insertions(+), 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)
>  {
> 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");

Space after comma.

> +        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);

While changing this code, please use d%d for the domain id, to be
consistent with the newer style.

With these two minor issues fixed, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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

Re: [PATCH v3 0/5] xentrace/xenalyze Support on ARM

[Jan Beulich]

>>> On 04.04.16 at 20:48, <ben.sanda@dornerworks.com> 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 v2:
>   * Merged previous single file patches into atomic patches which can
>     be applied and compiled independently. 
>   * Updated individual patch names to be more descriptive.
>   * Correct order of patches in patch set to provide correct
>     application/build order.
> 
> ---
> Changed since v1:
>   * Removed Flask changes as deemed unnecessary and unclear in 
>     purpose
>   * Corrected all commit messages to be line limited to 72 chars
>   * Implemented v1 review comments as indicated in each file's
>     commit log.
> 
> Benjamin Sanda (5):
>   xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86
>   xentrace: Memory/Page Mapping support for DOMID_XEN on ARM
>   xentrace: Timestamp support for ARM platform
>   xentrace: Trace Buffer Initialization on ARM
>   xenalyze: Build for Both ARM and x86 Platforms
> 
>  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(-)

A couple of formal things: This is v3, and I only now notice indeed
I should have looked at some of the patches. Yet which of them
isn't clear - I'm being Cc-ed on all of them, instead of just the ones
that submission guidelines say I should be. And then all patch
subjects start with xenalyze: or xentrace:, suggesting this series
isn't touching code other than those two. Generalization of
{get,put}_pg_owner(), otoh, while apparently a prereq for the
generalization of one or both of the two, should use a more
indicative component prefix (or maybe even none at all).

Jan


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

Re: [PATCH v3 1/5] xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86

[Jan Beulich]

>>> On 04.04.16 at 20:48, <ben.sanda@dornerworks.com> wrote:
> +void put_pg_owner(struct domain *pg_owner)
> +{
> +    rcu_unlock_domain(pg_owner);
> +}

I cannot see why this then can't just become an inline function.

Jan


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

Re: [PATCH v3 0/5] xentrace/xenalyze Support on ARM

[Ben Sanda]

Jan, 

> A couple of formal things: This is v3, and I only now notice
> indeed I should have looked at some of the patches. Yet which of them
> isn't clear - I'm being Cc-ed on all of them, instead of just the ones
> that submission guidelines say I should be. And then all patch
> subjects start with xenalyze: or xentrace:, suggesting this series
> isn't touching code other than those two. Generalization of
> {get,put}_pg_owner(), otoh, while apparently a prereq for the
> generalization of one or both of the two, should use a more indicative
> component prefix (or maybe even none at all).

Thank you for the comments, I will update the names of the patches so
they are more obvious as to what they touch. As far as the CC'ing
though, I'm not sure how to do this. When I create the patch and send
it out  with git send-email I send one email for the entire patch set
with one CC list. I don't know of a method to send a patch set but
have each individual patch go to a different CC list.

Thanks,
Ben


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

Re: [PATCH v3 0/5] xentrace/xenalyze Support on ARM

[Andrew Cooper]

On 06/04/16 17:51, Ben Sanda wrote:
> Jan, 
>
>> A couple of formal things: This is v3, and I only now notice
>> indeed I should have looked at some of the patches. Yet which of them
>> isn't clear - I'm being Cc-ed on all of them, instead of just the ones
>> that submission guidelines say I should be. And then all patch
>> subjects start with xenalyze: or xentrace:, suggesting this series
>> isn't touching code other than those two. Generalization of
>> {get,put}_pg_owner(), otoh, while apparently a prereq for the
>> generalization of one or both of the two, should use a more indicative
>> component prefix (or maybe even none at all).
> Thank you for the comments, I will update the names of the patches so
> they are more obvious as to what they touch. As far as the CC'ing
> though, I'm not sure how to do this. When I create the patch and send
> it out  with git send-email I send one email for the entire patch set
> with one CC list. I don't know of a method to send a patch set but
> have each individual patch go to a different CC list.

Word your commit message like so:

Signed-off-by: $ME
---
CC: $SOMEONE
CC: $SOMEONE_ELSE

and `git send-email` will DTRT.

~Andrew

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

Re: [PATCH v3 0/5] xentrace/xenalyze Support on ARM

[Ben Sanda]

Andrew,

> Word your commit message like so:
> 
> Signed-off-by: $ME
> ---
> CC: $SOMEONE
> CC: $SOMEONE_ELSE
>
> and `git send-email` will DTRT.

Ah, excellent. Then git send-email will auto add. Thanks,
I'll make the change for v4.

Ben

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Julien Grall]

(CC Stefano's new e-mail address)

Hello Benjamin,

On 04/04/16 19:48, Benjamin Sanda wrote:
>   xen/arch/arm/mm.c  |  3 ++-
>   xen/arch/arm/p2m.c | 35 +++++++++++++++++++++++++++++++----
>   2 files changed, 33 insertions(+), 5 deletions(-)
>
> 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);
> +

Please also replace the call to rcu_unlock_domain by put_pg_owner to 
stay consistent.

>           if ( od == NULL )
>               return -ESRCH;
>
> 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.
> +    */

The coding style for the comment should be:

/*
  * FOo
  * Bar
  */

> +    if(DOMID_XEN != d->domain_id)

if ( ... )

> +    {
> +        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);

Unfortunately, xenmem_add_to_physmap_one will ignore the return type and 
will always map using the type p2m_map_foreign. I would introduce
a new type p2m_map_foreign_ro to allow read-only foreign mapping.

I've looked at the x86 code (p2m_add_foreign) and I haven't been able to 
find how the page will be mapped read-only in the guest P2M. 
get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a 
non translated domain.

Andrew and Jan, do you know how this is supposed to work when xentrace 
is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?

> +        }
> +        else
> +        {
> +            *t = p2m_invalid;
> +        }

The brackets are not necessary for a single statement.

> +        ret = paddr;
> +    }
> +
>       return ret;
>   }
>
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 3/5] xentrace: Timestamp support for ARM platform

[Julien Grall]

Hello Benjamin,

On 04/04/16 19:48, Benjamin Sanda wrote:
> 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. Also defined cycles_t as uint64_t to simplify
> casting.

I'm not sure what you mean by "simplify casting".

The type cycles_t is not correctly defined for ARM32 because "unsigned 
long" is always 32-bits. However, the physical count register (CNTPCT) 
is always 64-bits. So the number of cycles would have been truncated.

The rest of the patch looks good to me.

> 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>
>
> ---
> Changed since v2:
>    * Combined v2 patches 7 and 6 into one patch in v3. No code change.
>
> ---
> Changed since v1:
>    * Moved get_cycles() to time.c
>    * Added function prototype for get_cycles()
> ---
>   xen/arch/arm/time.c        |  9 ++++++++-
>   xen/include/asm-arm/time.h | 11 +++++------
>   2 files changed, 13 insertions(+), 7 deletions(-)
>
> 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. */
> 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);
>
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 4/5] xentrace: Trace Buffer Initialization on ARM

[Julien Grall]

Hello Benjamin,

On 04/04/16 19:48, Benjamin Sanda wrote:
> Added call to init_trace_bufs() to initialize the trace buffers for
> use by xentrace.
>
> Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 0/5] xentrace/xenalyze Support on ARM

[George Dunlap]

On 05/04/16 09:09, Jan Beulich wrote:
>>>> On 04.04.16 at 20:48, <ben.sanda@dornerworks.com> 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 v2:
>>   * Merged previous single file patches into atomic patches which can
>>     be applied and compiled independently. 
>>   * Updated individual patch names to be more descriptive.
>>   * Correct order of patches in patch set to provide correct
>>     application/build order.
>>
>> ---
>> Changed since v1:
>>   * Removed Flask changes as deemed unnecessary and unclear in 
>>     purpose
>>   * Corrected all commit messages to be line limited to 72 chars
>>   * Implemented v1 review comments as indicated in each file's
>>     commit log.
>>
>> Benjamin Sanda (5):
>>   xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86
>>   xentrace: Memory/Page Mapping support for DOMID_XEN on ARM
>>   xentrace: Timestamp support for ARM platform
>>   xentrace: Trace Buffer Initialization on ARM
>>   xenalyze: Build for Both ARM and x86 Platforms
>>
>>  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(-)
> 
> A couple of formal things: This is v3, and I only now notice indeed
> I should have looked at some of the patches. Yet which of them
> isn't clear - I'm being Cc-ed on all of them, instead of just the ones
> that submission guidelines say I should be. And then all patch
> subjects start with xenalyze: or xentrace:, suggesting this series
> isn't touching code other than those two. Generalization of
> {get,put}_pg_owner(), otoh, while apparently a prereq for the
> generalization of one or both of the two, should use a more
> indicative component prefix (or maybe even none at all).

To make things a bit more concrete, here might be a better set of
one-line descriptions that help people skimming figure out what pertains
to them and what's going on:

[1/5] xen: Move {get,put}_pg_owner() into common code
[2/5] xen/arm: Support mapping pages owned by DOMID_XEN
[3/5] xen/arm: Have get_cycles() use the hardware timestamp counter
[4/5] xen/arm: Enable tracing
[5/5] tools/xenalyze: Build on ARM

 -George

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Jan Beulich]

>>> On 08.04.16 at 12:42, <julien.grall@arm.com> wrote:
> On 04/04/16 19:48, Benjamin Sanda wrote:
>> +    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);
> 
> Unfortunately, xenmem_add_to_physmap_one will ignore the return type and 
> will always map using the type p2m_map_foreign. I would introduce
> a new type p2m_map_foreign_ro to allow read-only foreign mapping.
> 
> I've looked at the x86 code (p2m_add_foreign) and I haven't been able to 
> find how the page will be mapped read-only in the guest P2M. 
> get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a 
> non translated domain.
> 
> Andrew and Jan, do you know how this is supposed to work when xentrace 
> is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?

I don't think that case is being taken care of right now: xentrace
is to be used by privileged guests only anyway, and the only
HVM-like privileged guest would be a PVHv1 Dom0 (which likely
no-one cared about to make work with xentrace so far).

Jan


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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Andrew Cooper]

On 08/04/16 16:49, Jan Beulich wrote:
>>>> On 08.04.16 at 12:42, <julien.grall@arm.com> wrote:
>> On 04/04/16 19:48, Benjamin Sanda wrote:
>>> +    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);
>> Unfortunately, xenmem_add_to_physmap_one will ignore the return type and 
>> will always map using the type p2m_map_foreign. I would introduce
>> a new type p2m_map_foreign_ro to allow read-only foreign mapping.
>>
>> I've looked at the x86 code (p2m_add_foreign) and I haven't been able to 
>> find how the page will be mapped read-only in the guest P2M. 
>> get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a 
>> non translated domain.
>>
>> Andrew and Jan, do you know how this is supposed to work when xentrace 
>> is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?
> I don't think that case is being taken care of right now: xentrace
> is to be used by privileged guests only anyway, and the only
> HVM-like privileged guest would be a PVHv1 Dom0 (which likely
> no-one cared about to make work with xentrace so far).

Answer to questions of the form "Has anyone considered $X for a
privileged HVM domain on x86" are almost always "No".

The real question is whether the domain making the mapping needs to
write into the pages or not.  If xentrace has to update shared pointers,
then it needs to be rw.  If it simply consumes the data without any
backwards notification, then it should be ro.

~Andrew

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[George Dunlap]

On Fri, Apr 8, 2016 at 6:58 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 08/04/16 16:49, Jan Beulich wrote:
>>>>> On 08.04.16 at 12:42, <julien.grall@arm.com> wrote:
>>> On 04/04/16 19:48, Benjamin Sanda wrote:
>>>> +    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);
>>> Unfortunately, xenmem_add_to_physmap_one will ignore the return type and
>>> will always map using the type p2m_map_foreign. I would introduce
>>> a new type p2m_map_foreign_ro to allow read-only foreign mapping.
>>>
>>> I've looked at the x86 code (p2m_add_foreign) and I haven't been able to
>>> find how the page will be mapped read-only in the guest P2M.
>>> get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a
>>> non translated domain.
>>>
>>> Andrew and Jan, do you know how this is supposed to work when xentrace
>>> is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?
>> I don't think that case is being taken care of right now: xentrace
>> is to be used by privileged guests only anyway, and the only
>> HVM-like privileged guest would be a PVHv1 Dom0 (which likely
>> no-one cared about to make work with xentrace so far).
>
> Answer to questions of the form "Has anyone considered $X for a
> privileged HVM domain on x86" are almost always "No".
>
> The real question is whether the domain making the mapping needs to
> write into the pages or not.  If xentrace has to update shared pointers,
> then it needs to be rw.  If it simply consumes the data without any
> backwards notification, then it should be ro.

It does access shared pointers, and so needs at lest one page to be
rw.  At the moment there's sort of two levels: the "trace info"
page(s), mapped RO, which has the list of all the MFNs used for the
actual trace data, and the trace data MFNs themselves, which are
mapped RW.

Re Julien's question about how DOMID_XEN pages are marked RO on x86
when get_page_from_gfn() always returns p2m_ram_rw: The answer is that
get_page_from_gfn() is only really used by the p2m code.  For PV
guests, it's the page type that restricts a page's type to RO or RW.
trace.c calls share_xen_page_with_privileged_guests(), which on x86
calls xen/arch/x86/mm.c:share_xen_page_with_guest(), which sets the
type to PGT_writable_page.

 -George

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

Re: [PATCH v3 3/5] xentrace: Timestamp support for ARM platform

[Konrad Rzeszutek Wilk]

On Mon, Apr 04, 2016 at 02:48:45PM -0400, Benjamin Sanda wrote:
> 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. Also defined cycles_t as uint64_t to simplify
> casting.
> 
> get_s_time() was also modified to now use the updated get_cycles() to
> retrieve the tick count instead of directly reading it.
> 

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Julien Grall]

Hi George,

On 11/04/2016 10:52, George Dunlap wrote:
> On Fri, Apr 8, 2016 at 6:58 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 08/04/16 16:49, Jan Beulich wrote:
>>>>>> On 08.04.16 at 12:42, <julien.grall@arm.com> wrote:
>>>> On 04/04/16 19:48, Benjamin Sanda wrote:
>>>>> +    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);
>>>> Unfortunately, xenmem_add_to_physmap_one will ignore the return type and
>>>> will always map using the type p2m_map_foreign. I would introduce
>>>> a new type p2m_map_foreign_ro to allow read-only foreign mapping.
>>>>
>>>> I've looked at the x86 code (p2m_add_foreign) and I haven't been able to
>>>> find how the page will be mapped read-only in the guest P2M.
>>>> get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's a
>>>> non translated domain.
>>>>
>>>> Andrew and Jan, do you know how this is supposed to work when xentrace
>>>> is used in a HVM domain? Does x86 Xen always mapped Read-Write the page?
>>> I don't think that case is being taken care of right now: xentrace
>>> is to be used by privileged guests only anyway, and the only
>>> HVM-like privileged guest would be a PVHv1 Dom0 (which likely
>>> no-one cared about to make work with xentrace so far).
>>
>> Answer to questions of the form "Has anyone considered $X for a
>> privileged HVM domain on x86" are almost always "No".
>>
>> The real question is whether the domain making the mapping needs to
>> write into the pages or not.  If xentrace has to update shared pointers,
>> then it needs to be rw.  If it simply consumes the data without any
>> backwards notification, then it should be ro.
>
> It does access shared pointers, and so needs at lest one page to be
> rw.  At the moment there's sort of two levels: the "trace info"
> page(s), mapped RO, which has the list of all the MFNs used for the
> actual trace data, and the trace data MFNs themselves, which are
> mapped RW.
>
> Re Julien's question about how DOMID_XEN pages are marked RO on x86
> when get_page_from_gfn() always returns p2m_ram_rw: The answer is that
> get_page_from_gfn() is only really used by the p2m code.  For PV
> guests, it's the page type that restricts a page's type to RO or RW.
> trace.c calls share_xen_page_with_privileged_guests(), which on x86
> calls xen/arch/x86/mm.c:share_xen_page_with_guest(), which sets the
> type to PGT_writable_page.

Thank you for the explanation.

The ARM implementation of share_xen_page_with_guest is nearly the same 
as the x86 one. However, the type is never used so far for the P2M code.

So far, all ARM domains have been auto-translated. DOMID_XEN is the 
first non auto-translated domain.

We could make DOMID_XEN an auto-translated domain by introducing page 
table for dummy domain. This would make the code cleaner but use more 
memory (allocation of 3 level of page tables).

Stefano, do you have any opinions on this?

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Ben Sanda]

Julien, George, Andrew, and Stefano,

> Thank you for the explanation.
> 
> The ARM implementation of share_xen_page_with_guest is nearly the same
> as the x86 one. However, the type is never used so far for the P2M
> code.
> 
> So far, all ARM domains have been auto-translated. DOMID_XEN is the
> first non auto-translated domain.
> 
> We could make DOMID_XEN an auto-translated domain by introducing page
> table for dummy domain. This would make the code cleaner but use more
> memory (allocation of 3 level of page tables).
> 
> Stefano, do you have any opinions on this?

How would you like to me proceed with this issue with regard to the
patch set? It sounds like this is a more far reaching architecture
design question for ARM. I have made the other changes to this version
of the code requested, should I submit v4? Or wait for a further
resolution to this discussion?

Thanks,
Ben Sanda

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

Re: [PATCH v3 1/5] xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86

[Ben Sanda]

Jan,

>> +void put_pg_owner(struct domain *pg_owner) {
>> +    rcu_unlock_domain(pg_owner);
>> +}

> I cannot see why this then can't just become an inline function.

I investigated this but making put_pg_owner() static inline creates a
circular dependency on rcu_unlock_domain(), which is also a static
inline function. The two functions are in different header files and
this creates a dependency on the one header being included by the
other, which, depending on how C files include them, creates an
implicit definition error by the compiler. For now I will leave the
function as is.

Thanks,
Ben Sanda

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

Re: [PATCH v3 1/5] xentrace: Common Support for get_pg_owner/put_pg_owner on ARM and x86

[Jan Beulich]

>>> Ben Sanda <Ben.Sanda@dornerworks.com> 04/14/16 10:03 PM >>>
>>> +void put_pg_owner(struct domain *pg_owner) {
>>> +    rcu_unlock_domain(pg_owner);
>>> +}
> I cannot see why this then can't just become an inline function.
>
>I investigated this but making put_pg_owner() static inline creates a
>circular dependency on rcu_unlock_domain(), which is also a static
>inline function. The two functions are in different header files and
>this creates a dependency on the one header being included by the
>other, which, depending on how C files include them, creates an
>implicit definition error by the compiler. For now I will leave the
>function as is.

While generally macros are undesirable in place of inline functions,
with the type checking aspect not being relevant here (both functions
have identically typed parameters), using a #define here instead of an
out of line call to a function which does nothing other than adjust the
preempt count seems warranted if the include dependency can't be
addressed in a satisfactory manner.

Jan


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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Julien Grall]

On 14/04/16 20:52, Ben Sanda wrote:
> Julien, George, Andrew, and Stefano,

Hello Ben,

Sorry for the late answer, we were at the Xen hackathon.

>
>> Thank you for the explanation.
>>
>> The ARM implementation of share_xen_page_with_guest is nearly the same
>> as the x86 one. However, the type is never used so far for the P2M
>> code.
>>
>> So far, all ARM domains have been auto-translated. DOMID_XEN is the
>> first non auto-translated domain.
>>
>> We could make DOMID_XEN an auto-translated domain by introducing page
>> table for dummy domain. This would make the code cleaner but use more
>> memory (allocation of 3 level of page tables).
>>
>> Stefano, do you have any opinions on this?
>
> How would you like to me proceed with this issue with regard to the
> patch set? It sounds like this is a more far reaching architecture
> design question for ARM. I have made the other changes to this version
> of the code requested, should I submit v4? Or wait for a further
> resolution to this discussion?

This patch is the meat of this series. The result of the discussion may 
change drastically the way this series add support to map xenhap page in 
the guest.

So I would wait until we agree on the solution.

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Stefano Stabellini]

On Tue, 12 Apr 2016, Julien Grall wrote:
> On 11/04/2016 10:52, George Dunlap wrote:
> > On Fri, Apr 8, 2016 at 6:58 PM, Andrew Cooper <andrew.cooper3@citrix.com>
> > wrote:
> > > On 08/04/16 16:49, Jan Beulich wrote:
> > > > > > > On 08.04.16 at 12:42, <julien.grall@arm.com> wrote:
> > > > > On 04/04/16 19:48, Benjamin Sanda wrote:
> > > > > > +    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);
> > > > > Unfortunately, xenmem_add_to_physmap_one will ignore the return type
> > > > > and
> > > > > will always map using the type p2m_map_foreign. I would introduce
> > > > > a new type p2m_map_foreign_ro to allow read-only foreign mapping.
> > > > > 
> > > > > I've looked at the x86 code (p2m_add_foreign) and I haven't been able
> > > > > to
> > > > > find how the page will be mapped read-only in the guest P2M.
> > > > > get_page_from_gfn will always return p2m_raw_rw for DOMID_XEN as it's
> > > > > a
> > > > > non translated domain.
> > > > > 
> > > > > Andrew and Jan, do you know how this is supposed to work when xentrace
> > > > > is used in a HVM domain? Does x86 Xen always mapped Read-Write the
> > > > > page?
> > > > I don't think that case is being taken care of right now: xentrace
> > > > is to be used by privileged guests only anyway, and the only
> > > > HVM-like privileged guest would be a PVHv1 Dom0 (which likely
> > > > no-one cared about to make work with xentrace so far).
> > > 
> > > Answer to questions of the form "Has anyone considered $X for a
> > > privileged HVM domain on x86" are almost always "No".
> > > 
> > > The real question is whether the domain making the mapping needs to
> > > write into the pages or not.  If xentrace has to update shared pointers,
> > > then it needs to be rw.  If it simply consumes the data without any
> > > backwards notification, then it should be ro.
> > 
> > It does access shared pointers, and so needs at lest one page to be
> > rw.  At the moment there's sort of two levels: the "trace info"
> > page(s), mapped RO, which has the list of all the MFNs used for the
> > actual trace data, and the trace data MFNs themselves, which are
> > mapped RW.
> > 
> > Re Julien's question about how DOMID_XEN pages are marked RO on x86
> > when get_page_from_gfn() always returns p2m_ram_rw: The answer is that
> > get_page_from_gfn() is only really used by the p2m code.  For PV
> > guests, it's the page type that restricts a page's type to RO or RW.
> > trace.c calls share_xen_page_with_privileged_guests(), which on x86
> > calls xen/arch/x86/mm.c:share_xen_page_with_guest(), which sets the
> > type to PGT_writable_page.
> 
> Thank you for the explanation.
> 
> The ARM implementation of share_xen_page_with_guest is nearly the same as the
> x86 one. However, the type is never used so far for the P2M code.
> 
> So far, all ARM domains have been auto-translated. DOMID_XEN is the first non
> auto-translated domain.
> 
> We could make DOMID_XEN an auto-translated domain by introducing page table
> for dummy domain. This would make the code cleaner but use more memory
> (allocation of 3 level of page tables).
> 
> Stefano, do you have any opinions on this?

If it is just one "if" or two the issue, I would keep DOMID_XEN as it is
now and deal with it as a special case.  If we expect that we are going
to grow more "if domain_id == DOMID_XEN" all around the code base, then
I would follow your suggestion and make DOMID_XEN auto-translated. But
we don't need to decide now, we could go with the special if for the
moment and keep this in mind.

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

Re: [PATCH v3 2/5] xentrace: Memory/Page Mapping support for DOMID_XEN on ARM

[Julien Grall]

Hi Stefano,

On 22/04/16 10:42, Stefano Stabellini wrote:
> On Tue, 12 Apr 2016, Julien Grall wrote:
>> The ARM implementation of share_xen_page_with_guest is nearly the same as the
>> x86 one. However, the type is never used so far for the P2M code.
>>
>> So far, all ARM domains have been auto-translated. DOMID_XEN is the first non
>> auto-translated domain.
>>
>> We could make DOMID_XEN an auto-translated domain by introducing page table
>> for dummy domain. This would make the code cleaner but use more memory
>> (allocation of 3 level of page tables).
>>
>> Stefano, do you have any opinions on this?
>
> If it is just one "if" or two the issue, I would keep DOMID_XEN as it is
> now and deal with it as a special case.  If we expect that we are going
> to grow more "if domain_id == DOMID_XEN" all around the code base, then
> I would follow your suggestion and make DOMID_XEN auto-translated. But
> we don't need to decide now, we could go with the special if for the
> moment and keep this in mind.

The plan sounds good to me. I cannot think of other place at the moment.

Regards,

-- 
Julien Grall

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