[PATCH 0/8] xen/arm: Add xentrace support

[PATCH 0/8] xen/arm: Add xentrace support

[Julien Grall]

Hi all,

This patch series is a rework of the series sent by Benjamin Sanda in April
2016 [1]. It finally adds support for xentrace/xenanalyze on Arm.

Cheers,

[1] https://lists.xenproject.org/archives/html/xen-devel/2016-04/msg00464.html

Benjamin Sanda (3):
  xen/page_alloc: Move get_pg_owner()/put_pg_owner() from x86 to common
    code
  xen/arm: Initialize trace buffer
  xenalyze: Build for Both ARM and x86 Platforms

Julien Grall (5):
  xen/arm: p2m: Introduce p2m_get_page_from_gfn
  xen/arm: Rename p2m_map_foreign to p2m_map_foreign_rw
  xen/arm: Add support for read-only foreign mappings
  xen/arm: Allow a privileged domain to map foreign page from DOMID_XEN
  xen: Swich parameter in get_page_from_gfn to use typesafe gfn

 tools/xentrace/Makefile              |  3 +-
 xen/arch/arm/guestcopy.c             |  2 +-
 xen/arch/arm/mm.c                    | 16 ++++-----
 xen/arch/arm/p2m.c                   | 35 ++++++++++++++++++-
 xen/arch/arm/setup.c                 |  3 ++
 xen/arch/x86/cpu/vpmu.c              |  2 +-
 xen/arch/x86/domain.c                | 12 +++----
 xen/arch/x86/domctl.c                |  6 ++--
 xen/arch/x86/hvm/dm.c                |  2 +-
 xen/arch/x86/hvm/domain.c            |  2 +-
 xen/arch/x86/hvm/hvm.c               |  9 ++---
 xen/arch/x86/hvm/svm/svm.c           |  8 ++---
 xen/arch/x86/hvm/viridian/viridian.c | 24 ++++++-------
 xen/arch/x86/hvm/vmx/vmx.c           |  4 +--
 xen/arch/x86/hvm/vmx/vvmx.c          | 12 +++----
 xen/arch/x86/mm.c                    | 66 ++++++++----------------------------
 xen/arch/x86/mm/p2m.c                |  2 +-
 xen/arch/x86/mm/shadow/hvm.c         |  6 ++--
 xen/arch/x86/physdev.c               |  3 +-
 xen/arch/x86/pv/descriptor-tables.c  |  5 ++-
 xen/arch/x86/pv/emul-priv-op.c       |  6 ++--
 xen/arch/x86/pv/mm.c                 |  2 +-
 xen/arch/x86/traps.c                 | 11 +++---
 xen/common/domain.c                  |  2 +-
 xen/common/event_fifo.c              | 12 +++----
 xen/common/memory.c                  |  4 +--
 xen/common/page_alloc.c              | 38 +++++++++++++++++++++
 xen/common/tmem_xen.c                |  2 +-
 xen/include/asm-arm/p2m.h            | 57 ++++++++++++++++++-------------
 xen/include/asm-x86/p2m.h            | 11 +++---
 xen/include/xen/mm.h                 |  3 ++
 31 files changed, 212 insertions(+), 158 deletions(-)

-- 
2.11.0


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

[PATCH 1/8] xen/page_alloc: Move get_pg_owner()/put_pg_owner() from x86 to common code

[Julien Grall]

From: Benjamin Sanda <ben.sanda@dornerworks.com>

get_pg_owner() and put_pg_owner() will be necessary in a follow-up
commit to support xentrace on Arm. So move the helper to common code.

Note that put_pg_owner() has been turned to a macro rather than static
inline because of inter-dependency between includes.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
[julien: Rework commit title / turn put_pg_owner to a macro]
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/mm.c       | 42 ------------------------------------------
 xen/common/page_alloc.c | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/mm.h    |  3 +++
 3 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f043e43ac7..9363e9bd96 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3100,48 +3100,6 @@ static int vcpumask_to_pcpumask(
     }
 }
 
-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) )
-    {
-        gdprintk(XENLOG_WARNING, "Cannot specify itself as foreign domain\n");
-        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 )
-        {
-            gdprintk(XENLOG_WARNING, "Unknown domain d%d\n", domid);
-            break;
-        }
-        break;
-    }
-
- out:
-    return pg_owner;
-}
-
-static void put_pg_owner(struct domain *pg_owner)
-{
-    rcu_unlock_domain(pg_owner);
-}
-
 long do_mmuext_op(
     XEN_GUEST_HANDLE_PARAM(mmuext_op_t) uops,
     unsigned int count,
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 16e1b0c357..ef1b4f596a 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -146,6 +146,7 @@
 #include <asm/guest.h>
 #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)
@@ -2447,6 +2448,43 @@ 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\n");
+        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 )
+        {
+            gdprintk(XENLOG_WARNING, "Unknown domain d%d\n", domid);
+            break;
+        }
+        break;
+    }
+
+ out:
+    return pg_owner;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 054d02e6c0..dd4d990ae3 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -589,6 +589,9 @@ int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
 int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
                           unsigned int start);
 
+struct domain *get_pg_owner(domid_t domid);
+#define put_pg_owner(pg_owner) rcu_unlock_domain(pg_owner)
+
 /* Return 0 on success, or negative on error. */
 int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
 int __must_check steal_page(struct domain *d, struct page_info *page,
-- 
2.11.0


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

Re: [PATCH 1/8] xen/page_alloc: Move get_pg_owner()/put_pg_owner() from x86 to common code

[Jan Beulich]

>>> On 06.11.18 at 20:14, <julien.grall@arm.com> wrote:
> From: Benjamin Sanda <ben.sanda@dornerworks.com>
> 
> get_pg_owner() and put_pg_owner() will be necessary in a follow-up
> commit to support xentrace on Arm. So move the helper to common code.
> 
> Note that put_pg_owner() has been turned to a macro rather than static
> inline because of inter-dependency between includes.

Could this be avoided by placing both in a different header,
e.g. sched.h? Mid-term we should probably try to split out
type (structure) declarations from sched.h, to make it easier
to have full types available without having to respect other
include dependencies.

Jan



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

Re: [PATCH 1/8] xen/page_alloc: Move get_pg_owner()/put_pg_owner() from x86 to common code

[Julien Grall]

Hi Jan,

On 07/11/2018 09:28, Jan Beulich wrote:
>>>> On 06.11.18 at 20:14, <julien.grall@arm.com> wrote:
>> From: Benjamin Sanda <ben.sanda@dornerworks.com>
>>
>> get_pg_owner() and put_pg_owner() will be necessary in a follow-up
>> commit to support xentrace on Arm. So move the helper to common code.
>>
>> Note that put_pg_owner() has been turned to a macro rather than static
>> inline because of inter-dependency between includes.
> 
> Could this be avoided by placing both in a different header,
> e.g. sched.h?
This seems to work. I will use that in the next version.

> Mid-term we should probably try to split out
> type (structure) declarations from sched.h, to make it easier
> to have full types available without having to respect other
> include dependencies.

The include dependencies on Xen is a bit of a nightmare. I have started to clean 
it up on Arm and I will support any changes in the common headers.

Cheers,

-- 
Julien Grall

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

[PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

[Julien Grall]

In a follow-up patches, we will need to handle get_page_from_gfn
differently for DOMID_XEN. To keep the code simple move the current
content in a new separate helper p2m_get_page_from_gfn.

Note the new helper is a not anymore a static inline function as the helper
is quite complex.

Finally, take the opportunity to use typesafe gfn as the change is
minor.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c        | 32 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h | 33 ++++-----------------------------
 2 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 30cfb01498..04c8718e9f 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -379,6 +379,38 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
     return mfn;
 }
 
+struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
+                                        p2m_type_t *t)
+{
+    struct page_info *page;
+    p2m_type_t p2mt;
+    mfn_t mfn = p2m_lookup(d, gfn, &p2mt);
+
+    if (t)
+        *t = p2mt;
+
+    if ( !p2m_is_any_ram(p2mt) )
+        return NULL;
+
+    if ( !mfn_valid(mfn) )
+        return NULL;
+    page = mfn_to_page(mfn);
+
+    /*
+     * get_page won't work on foreign mapping because the page doesn't
+     * belong to the current domain.
+     */
+    if ( p2m_is_foreign(p2mt) )
+    {
+        struct domain *fdom = page_get_owner_and_reference(page);
+        ASSERT(fdom != NULL);
+        ASSERT(fdom != d);
+        return page;
+    }
+
+    return (get_page(page, d) ? page: NULL);
+}
+
 int guest_physmap_mark_populate_on_demand(struct domain *d,
                                           unsigned long gfn,
                                           unsigned int order)
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index c03557544a..a5914136e3 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -269,38 +269,13 @@ typedef unsigned int p2m_query_t;
 #define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
 #define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
 
+struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
+                                        p2m_type_t *t);
+
 static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
-    struct page_info *page;
-    p2m_type_t p2mt;
-    mfn_t mfn = p2m_lookup(d, _gfn(gfn), &p2mt);
-
-    if (t)
-        *t = p2mt;
-
-    if ( !p2m_is_any_ram(p2mt) )
-        return NULL;
-
-    if ( !mfn_valid(mfn) )
-        return NULL;
-    page = mfn_to_page(mfn);
-
-    /*
-     * get_page won't work on foreign mapping because the page doesn't
-     * belong to the current domain.
-     */
-    if ( p2m_is_foreign(p2mt) )
-    {
-        struct domain *fdom = page_get_owner_and_reference(page);
-        ASSERT(fdom != NULL);
-        ASSERT(fdom != d);
-        return page;
-    }
-
-    if ( !get_page(page, d) )
-        return NULL;
-    return page;
+    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
 }
 
 int get_page_type(struct page_info *page, unsigned long type);
-- 
2.11.0


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

Re: [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

[Andrii Anisov]

Hello Julien,

вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
>
> In a follow-up patches, we will need to handle get_page_from_gfn
> differently for DOMID_XEN. To keep the code simple move the current
> content in a new separate helper p2m_get_page_from_gfn.
>
> Note the new helper is a not anymore a static inline function as the helper
> is quite complex.

In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
mappings" you make p2m_get_page_from_gfn() comparingly complex, but
still keep it as static inline. Could you please be consistent (making
both of those functions inline or non-inline)

Sincerely,
Andrii Anisov.

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

Re: [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

[Andrii Anisov]

Sorry,

Not "comparingly complex", but "equally complex". ;)

Sincerely,
Andrii Anisov.
чт, 15 лист. 2018 о 15:31 Andrii Anisov <andrii.anisov@gmail.com> пише:
>
> Hello Julien,
>
> вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
> >
> > In a follow-up patches, we will need to handle get_page_from_gfn
> > differently for DOMID_XEN. To keep the code simple move the current
> > content in a new separate helper p2m_get_page_from_gfn.
> >
> > Note the new helper is a not anymore a static inline function as the helper
> > is quite complex.
>
> In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
> mappings" you make p2m_get_page_from_gfn() comparingly complex, but
> still keep it as static inline. Could you please be consistent (making
> both of those functions inline or non-inline)
>
> Sincerely,
> Andrii Anisov.

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

Re: [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

[Julien Grall]

On 11/15/18 1:31 PM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
>>
>> In a follow-up patches, we will need to handle get_page_from_gfn
>> differently for DOMID_XEN. To keep the code simple move the current
>> content in a new separate helper p2m_get_page_from_gfn.
>>
>> Note the new helper is a not anymore a static inline function as the helper
>> is quite complex.
> 
> In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign
> mappings" you make p2m_get_page_from_gfn() comparingly complex, but
> still keep it as static inline. Could you please be consistent (making
> both of those functions inline or non-inline)

The reason I didn't move the other one in p2m.c is because so far p2m.c 
is only dealing with auto-translated guest. I didn't want to add 
function related with non-auto translated guest in it.

I also don't think introduce a new file for one 10 line function is 
really useful.

So that was the best solution. I am open to other suggestion.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn

[Andrii Anisov]

On 15.11.18 17:22, Julien Grall wrote:
> The reason I didn't move the other one in p2m.c is because so far p2m.c is only dealing with auto-translated guest. I didn't want to add function related with non-auto translated guest in it.
> 
> I also don't think introduce a new file for one 10 line function is really useful.
> 
> So that was the best solution. I am open to other suggestion.
> 
> Cheers,

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

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

[PATCH 3/8] xen/arm: Rename p2m_map_foreign to p2m_map_foreign_rw

[Julien Grall]

A follow-up patch will introduce another type of foreign mapping. Rename
the type to make clear it is only used for read-write mapping.

No functional changes intended.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c         | 2 +-
 xen/arch/arm/p2m.c        | 2 +-
 xen/include/asm-arm/p2m.h | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7a06a33e21..cec821c3a3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1283,7 +1283,7 @@ int xenmem_add_to_physmap_one(
         }
 
         mfn = page_to_mfn(page);
-        t = p2m_map_foreign;
+        t = p2m_map_foreign_rw;
 
         rcu_unlock_domain(od);
         break;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 04c8718e9f..b32b16cd94 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -440,7 +440,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
         break;
 
     case p2m_iommu_map_rw:
-    case p2m_map_foreign:
+    case p2m_map_foreign_rw:
     case p2m_grant_map_rw:
     case p2m_mmio_direct_dev:
     case p2m_mmio_direct_nc:
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index a5914136e3..5f7f31186d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -117,7 +117,7 @@ enum p2m_type {
     p2m_mmio_direct_dev,/* Read/write mapping of genuine Device MMIO area */
     p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
     p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
-    p2m_map_foreign,    /* Ram pages from foreign domain */
+    p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
     p2m_grant_map_rw,   /* Read/write grant mapping */
     p2m_grant_map_ro,   /* Read-only grant mapping */
     /* The types below are only used to decide the page attribute in the P2M */
@@ -139,10 +139,10 @@ enum p2m_type {
 
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
-#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign))
+#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw))
 #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
                             (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
-                             p2m_to_mask(p2m_map_foreign)))
+                             p2m_to_mask(p2m_map_foreign_rw)))
 
 static inline
 void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
-- 
2.11.0


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

Re: [PATCH 3/8] xen/arm: Rename p2m_map_foreign to p2m_map_foreign_rw

[Andrii Anisov]

Hello Julien,

I'm not sure why do you need this patch to be separated from "[PATCH
4/8] xen/arm: Add support for read-only foreign mappings".
But I would not argue for that.
    Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Sincerely,
Andrii Anisov.

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

Re: [PATCH 3/8] xen/arm: Rename p2m_map_foreign to p2m_map_foreign_rw

[Julien Grall]

On 11/15/18 11:42 AM, Andrii Anisov wrote:
> Hello Julien,

Hi Andrii,

> 
> I'm not sure why do you need this patch to be separated from "[PATCH
> 4/8] xen/arm: Add support for read-only foreign mappings".
> But I would not argue for that.
>      Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

We tend to separate renaming/clean-up from new feature.

Thank you for the review.

Cheers,


-- 
Julien Grall

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

[PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

Current, foreign mappings can only be read-write. A follow-up patch will
extend foreign mapping for Xen backend memory (via XEN_DOMID), some of
that memory should only be read accessible for the mapping domain.

Introduce a new p2m_type to cater read-only foreign mappings. For now,
the decision between the two foreign mapping type is based on the type
of the guest page mapped.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c         |  2 +-
 xen/arch/arm/p2m.c        |  1 +
 xen/include/asm-arm/p2m.h | 42 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index cec821c3a3..e31b47a394 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1283,7 +1283,7 @@ int xenmem_add_to_physmap_one(
         }
 
         mfn = page_to_mfn(page);
-        t = p2m_map_foreign_rw;
+        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
 
         rcu_unlock_domain(od);
         break;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index b32b16cd94..0941be9e41 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -450,6 +450,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
         break;
 
     case p2m_iommu_map_ro:
+    case p2m_map_foreign_ro:
     case p2m_grant_map_ro:
     case p2m_invalid:
         e->p2m.xn = 1;
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 5f7f31186d..7c67806056 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -118,6 +118,7 @@ enum p2m_type {
     p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */
     p2m_mmio_direct_c,  /* Read/write mapping of genuine MMIO area cacheable */
     p2m_map_foreign_rw, /* Read/write RAM pages from foreign domain */
+    p2m_map_foreign_ro, /* Read-only RAM pages from foreign domain */
     p2m_grant_map_rw,   /* Read/write grant mapping */
     p2m_grant_map_ro,   /* Read-only grant mapping */
     /* The types below are only used to decide the page attribute in the P2M */
@@ -137,12 +138,16 @@ enum p2m_type {
 #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw) |  \
                          p2m_to_mask(p2m_grant_map_ro))
 
+/* Foreign mappings types */
+#define P2M_FOREIGN_TYPES (p2m_to_mask(p2m_map_foreign_rw) | \
+                           p2m_to_mask(p2m_map_foreign_ro))
+
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
-#define p2m_is_foreign(_t) (p2m_to_mask(_t) & p2m_to_mask(p2m_map_foreign_rw))
+#define p2m_is_foreign(_t) (p2m_to_mask(_t) & P2M_FOREIGN_TYPES)
 #define p2m_is_any_ram(_t) (p2m_to_mask(_t) &                   \
                             (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
-                             p2m_to_mask(p2m_map_foreign_rw)))
+                             P2M_FOREIGN_TYPES))
 
 static inline
 void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
@@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
 static inline struct page_info *get_page_from_gfn(
     struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
 {
-    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
+    mfn_t mfn;
+    p2m_type_t _t;
+    struct page_info *page;
+
+    /*
+     * Special case for DOMID_XEN as it is the only domain so far that is
+     * not auto-translated.
+     */
+    if ( unlikely(d != dom_xen) )
+        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
+
+    if ( !t )
+        t = &_t;
+
+    *t = p2m_invalid;
+
+    /*
+     * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
+     * page.
+     */
+    mfn = _mfn(gfn);
+    page = mfn_to_page(mfn);
+
+    if ( !mfn_valid(mfn) || !get_page(page, d) )
+        return NULL;
+
+    if ( page->u.inuse.type_info & PGT_writable_page )
+        *t = p2m_ram_rw;
+    else
+        *t = p2m_ram_ro;
+
+    return page;
 }
 
 int get_page_type(struct page_info *page, unsigned long type);
-- 
2.11.0


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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

Hello Julien,

вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
> @@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
>  static inline struct page_info *get_page_from_gfn(
>      struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>  {
> -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +    mfn_t mfn;
> +    p2m_type_t _t;

Me personally, do not like introducing intermediate `_t` variable.
IMO, constructs like following:

> +    if ( !t )
> +        t = &_t;
> +
> +    *t = p2m_invalid;

make the code harder to understand than simple checking `t` for nul
before assigning it a
value.

Sincerely,
Andrii Anisov.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

On 11/15/18 11:33 AM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:
>> @@ -275,7 +280,38 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
>>   static inline struct page_info *get_page_from_gfn(
>>       struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>   {
>> -    return p2m_get_page_from_gfn(d, _gfn(gfn), t);
>> +    mfn_t mfn;
>> +    p2m_type_t _t;
> 
> Me personally, do not like introducing intermediate `_t` variable.
> IMO, constructs like following:
> 
>> +    if ( !t )
>> +        t = &_t;
>> +
>> +    *t = p2m_invalid;
> 
> make the code harder to understand than simple checking `t` for nul
> before assigning it a
> value.

If I drop _t then I need to add if ( *t ) in 3 places in that code. So I 
don't think the approach is any better.

Feel free to suggest a solution that does not require to check 't' 
everywhere.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

чт, 15 лист. 2018 о 13:40 Julien Grall <julien.grall@arm.com> пише:
> If I drop _t then I need to add if ( *t ) in 3 places in that code. So I
> don't think the approach is any better.
Ouch, I kept in my mind two places.

Something like:

if ( t )
    *t = p2m_invalid;
....
if ( t )
    *t = ( page->u.inuse.type_info & PGT_writable_page ) ? p2m_ram_rw
: p2m_ram_ro;
.....

Sincerely,
Andrii Anisov.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

On 11/15/18 12:02 PM, Andrii Anisov wrote:
> чт, 15 лист. 2018 о 13:40 Julien Grall <julien.grall@arm.com> пише:
>> If I drop _t then I need to add if ( *t ) in 3 places in that code. So I
>> don't think the approach is any better.
> Ouch, I kept in my mind two places.
> 
> Something like:
> 
> if ( t )
>      *t = p2m_invalid;
> ....
> if ( t )
>      *t = ( page->u.inuse.type_info & PGT_writable_page ) ? p2m_ram_rw
> : p2m_ram_ro;

If it was fitting in a single line maybe. In that context, I find it 
more difficult to read.

So I would prefer to stick with _t which is quite common within the p2m 
code base so far.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

> So I would prefer to stick with _t which is quite common within the p2m
> code base so far.

I've found a similar code only in one place - p2m_get_entry()
function. And it is, at least, somehow commented there:
...
    /* Allow t to be NULL */
    t = t ?: &_t;

    *t = p2m_invalid;
...

But IMO, it is really confusing to write a code to calculate and store
a value which clearly is not needed by a caller.
From another hand, I'm not sure if a compiler would be intelligent
enough to factor out the odd code from execution pass on the incoming
null pointer.

I'm sorry, but I can't pass my RB for `_t`.

Sincerely,
Andrii Anisov.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

Hi,

On 11/15/18 1:19 PM, Andrii Anisov wrote:
>> So I would prefer to stick with _t which is quite common within the p2m
>> code base so far.
> 
> I've found a similar code only in one place - p2m_get_entry()
> function. And it is, at least, somehow commented there:
> ...
>      /* Allow t to be NULL */
>      t = t ?: &_t;
> 
>      *t = p2m_invalid;
> ...

And in x86 code...

> 
> But IMO, it is really confusing to write a code to calculate and store
> a value which clearly is not needed by a caller.

I disagree, you directly know that t can be NULL. Although, I agree that 
a comment would help to understand.

>  From another hand, I'm not sure if a compiler would be intelligent
> enough to factor out the odd code from execution pass on the incoming
> null pointer.

You can't assume the compiler will inline even with the inline keyword.

> 
> I'm sorry, but I can't pass my RB for `_t`.

I think this request is unreasonable because this is a matter of taste.

While this is a nice feature to have in Xen 4.12 and address a long 
standing open issue on Arm. I don't require it and I am not inclined to 
address bikeshedding.

So I will keep aside for now until someone cares about it.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Stefano Stabellini]

On Thu, 15 Nov 2018, Julien Grall wrote:
> Hi,
> 
> On 11/15/18 1:19 PM, Andrii Anisov wrote:
> > > So I would prefer to stick with _t which is quite common within the p2m
> > > code base so far.
> > 
> > I've found a similar code only in one place - p2m_get_entry()
> > function. And it is, at least, somehow commented there:
> > ...
> >      /* Allow t to be NULL */
> >      t = t ?: &_t;
> > 
> >      *t = p2m_invalid;
> > ...
> 
> And in x86 code...
> 
> > 
> > But IMO, it is really confusing to write a code to calculate and store
> > a value which clearly is not needed by a caller.
> 
> I disagree, you directly know that t can be NULL. Although, I agree that a
> comment would help to understand.
> 
> >  From another hand, I'm not sure if a compiler would be intelligent
> > enough to factor out the odd code from execution pass on the incoming
> > null pointer.
> 
> You can't assume the compiler will inline even with the inline keyword.
> 
> > 
> > I'm sorry, but I can't pass my RB for `_t`.
> 
> I think this request is unreasonable because this is a matter of taste.
> 
> While this is a nice feature to have in Xen 4.12 and address a long standing
> open issue on Arm. I don't require it and I am not inclined to address
> bikeshedding.
> 
> So I will keep aside for now until someone cares about it.

Let's try to be reasonable and have constructive conversations. If we do
have to disagree, let's disagree on meaningful design decisions, not
silly code style issues :-)

Andrii, when something can be done equally well in two different ways,
especially when it is a matter of style, I think it is best to express
our preference, but not to force a contributor in a particular
direction, unless specifically stated in CODING_STYLE or equivalent
docs. We don't have written rules about code reviews, but if we had,
I think this should be one of them. (I would love to have written rules
about code reviews.)

Julien, usually in situations like this, it is quicker to change the
code than to argue. I personally don't care and wouldn't ask you to
change it, but if a member of the community reads the code and has an
adverse reaction, it is always worth reconsidering the approach, because
others might find it antagonizing too. Given the choice, it is best to
go for something obvious to most, so if a new contributor sends a patch
to change, it is more likely to be correct from the start.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

Hi Stefano,

On 11/15/18 6:44 PM, Stefano Stabellini wrote:
> On Thu, 15 Nov 2018, Julien Grall wrote:
>> Hi,
>>
>> On 11/15/18 1:19 PM, Andrii Anisov wrote:
>>>> So I would prefer to stick with _t which is quite common within the p2m
>>>> code base so far.
>>>
>>> I've found a similar code only in one place - p2m_get_entry()
>>> function. And it is, at least, somehow commented there:
>>> ...
>>>       /* Allow t to be NULL */
>>>       t = t ?: &_t;
>>>
>>>       *t = p2m_invalid;
>>> ...
>>
>> And in x86 code...
>>
>>>
>>> But IMO, it is really confusing to write a code to calculate and store
>>> a value which clearly is not needed by a caller.
>>
>> I disagree, you directly know that t can be NULL. Although, I agree that a
>> comment would help to understand.
>>
>>>   From another hand, I'm not sure if a compiler would be intelligent
>>> enough to factor out the odd code from execution pass on the incoming
>>> null pointer.
>>
>> You can't assume the compiler will inline even with the inline keyword.
>>
>>>
>>> I'm sorry, but I can't pass my RB for `_t`.
>>
>> I think this request is unreasonable because this is a matter of taste.
>>
>> While this is a nice feature to have in Xen 4.12 and address a long standing
>> open issue on Arm. I don't require it and I am not inclined to address
>> bikeshedding.
>>
>> So I will keep aside for now until someone cares about it.
> 
> Let's try to be reasonable and have constructive conversations. If we do
> have to disagree, let's disagree on meaningful design decisions, not
> silly code style issues :-)
> 
> Andrii, when something can be done equally well in two different ways,
> especially when it is a matter of style, I think it is best to express
> our preference, but not to force a contributor in a particular
> direction, unless specifically stated in CODING_STYLE or equivalent
> docs. We don't have written rules about code reviews, but if we had,
> I think this should be one of them. (I would love to have written rules
> about code reviews.)
> 
> Julien, usually in situations like this, it is quicker to change the
> code than to argue. I personally don't care and wouldn't ask you to
> change it, but if a member of the community reads the code and has an
> adverse reaction, it is always worth reconsidering the approach, because
> others might find it antagonizing too. Given the choice, it is best to
> go for something obvious to most, so if a new contributor sends a patch
> to change, it is more likely to be correct from the start.

I agree with your point here but this is also valid in the other way. If 
the suggested alternative provokes an adverse reaction to the 
contributor, then why should he use it?

As I wrote earlier one trying to use ternary condition split over 2 
lines is not making the code more obvious. So I am not entirely sure why 
I should be forced to write such code?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Stefano Stabellini]

On Thu, 15 Nov 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/15/18 6:44 PM, Stefano Stabellini wrote:
> > On Thu, 15 Nov 2018, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 11/15/18 1:19 PM, Andrii Anisov wrote:
> > > > > So I would prefer to stick with _t which is quite common within the
> > > > > p2m
> > > > > code base so far.
> > > > 
> > > > I've found a similar code only in one place - p2m_get_entry()
> > > > function. And it is, at least, somehow commented there:
> > > > ...
> > > >       /* Allow t to be NULL */
> > > >       t = t ?: &_t;
> > > > 
> > > >       *t = p2m_invalid;
> > > > ...
> > > 
> > > And in x86 code...
> > > 
> > > > 
> > > > But IMO, it is really confusing to write a code to calculate and store
> > > > a value which clearly is not needed by a caller.
> > > 
> > > I disagree, you directly know that t can be NULL. Although, I agree that a
> > > comment would help to understand.
> > > 
> > > >   From another hand, I'm not sure if a compiler would be intelligent
> > > > enough to factor out the odd code from execution pass on the incoming
> > > > null pointer.
> > > 
> > > You can't assume the compiler will inline even with the inline keyword.
> > > 
> > > > 
> > > > I'm sorry, but I can't pass my RB for `_t`.
> > > 
> > > I think this request is unreasonable because this is a matter of taste.
> > > 
> > > While this is a nice feature to have in Xen 4.12 and address a long
> > > standing
> > > open issue on Arm. I don't require it and I am not inclined to address
> > > bikeshedding.
> > > 
> > > So I will keep aside for now until someone cares about it.
> > 
> > Let's try to be reasonable and have constructive conversations. If we do
> > have to disagree, let's disagree on meaningful design decisions, not
> > silly code style issues :-)
> > 
> > Andrii, when something can be done equally well in two different ways,
> > especially when it is a matter of style, I think it is best to express
> > our preference, but not to force a contributor in a particular
> > direction, unless specifically stated in CODING_STYLE or equivalent
> > docs. We don't have written rules about code reviews, but if we had,
> > I think this should be one of them. (I would love to have written rules
> > about code reviews.)
> > 
> > Julien, usually in situations like this, it is quicker to change the
> > code than to argue. I personally don't care and wouldn't ask you to
> > change it, but if a member of the community reads the code and has an
> > adverse reaction, it is always worth reconsidering the approach, because
> > others might find it antagonizing too. Given the choice, it is best to
> > go for something obvious to most, so if a new contributor sends a patch
> > to change, it is more likely to be correct from the start.
> 
> I agree with your point here but this is also valid in the other way. If the
> suggested alternative provokes an adverse reaction to the contributor, then
> why should he use it?
> 
> As I wrote earlier one trying to use ternary condition split over 2 lines is
> not making the code more obvious. So I am not entirely sure why I should be
> forced to write such code?

I don't think you should be. You can find another way. For instance, the
whole thing could be reduce to one more "if" condition:

  if ( t != NULL )
  {
      *t = p2m_invalid;
      if ( page->u.inuse.type_info & PGT_writable_page )
          *t = p2m_ram_rw;
      else
          *t = p2m_ram_ro;
  }

be creative :-)

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Julien Grall]

Hi,

On 11/15/18 7:48 PM, Stefano Stabellini wrote:
> On Thu, 15 Nov 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/15/18 6:44 PM, Stefano Stabellini wrote:
>>> On Thu, 15 Nov 2018, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 11/15/18 1:19 PM, Andrii Anisov wrote:
>>>>>> So I would prefer to stick with _t which is quite common within the
>>>>>> p2m
>>>>>> code base so far.
>>>>>
>>>>> I've found a similar code only in one place - p2m_get_entry()
>>>>> function. And it is, at least, somehow commented there:
>>>>> ...
>>>>>        /* Allow t to be NULL */
>>>>>        t = t ?: &_t;
>>>>>
>>>>>        *t = p2m_invalid;
>>>>> ...
>>>>
>>>> And in x86 code...
>>>>
>>>>>
>>>>> But IMO, it is really confusing to write a code to calculate and store
>>>>> a value which clearly is not needed by a caller.
>>>>
>>>> I disagree, you directly know that t can be NULL. Although, I agree that a
>>>> comment would help to understand.
>>>>
>>>>>    From another hand, I'm not sure if a compiler would be intelligent
>>>>> enough to factor out the odd code from execution pass on the incoming
>>>>> null pointer.
>>>>
>>>> You can't assume the compiler will inline even with the inline keyword.
>>>>
>>>>>
>>>>> I'm sorry, but I can't pass my RB for `_t`.
>>>>
>>>> I think this request is unreasonable because this is a matter of taste.
>>>>
>>>> While this is a nice feature to have in Xen 4.12 and address a long
>>>> standing
>>>> open issue on Arm. I don't require it and I am not inclined to address
>>>> bikeshedding.
>>>>
>>>> So I will keep aside for now until someone cares about it.
>>>
>>> Let's try to be reasonable and have constructive conversations. If we do
>>> have to disagree, let's disagree on meaningful design decisions, not
>>> silly code style issues :-)
>>>
>>> Andrii, when something can be done equally well in two different ways,
>>> especially when it is a matter of style, I think it is best to express
>>> our preference, but not to force a contributor in a particular
>>> direction, unless specifically stated in CODING_STYLE or equivalent
>>> docs. We don't have written rules about code reviews, but if we had,
>>> I think this should be one of them. (I would love to have written rulesAs
>>> about code reviews.)
>>>
>>> Julien, usually in situations like this, it is quicker to change the
>>> code than to argue. I personally don't care and wouldn't ask you to
>>> change it, but if a member of the community reads the code and has an
>>> adverse reaction, it is always worth reconsidering the approach, because
>>> others might find it antagonizing too. Given the choice, it is best to
>>> go for something obvious to most, so if a new contributor sends a patch
>>> to change, it is more likely to be correct from the start.
>>
>> I agree with your point here but this is also valid in the other way. If the
>> suggested alternative provokes an adverse reaction to the contributor, then
>> why should he use it?
>>
>> As I wrote earlier one trying to use ternary condition split over 2 lines is
>> not making the code more obvious. So I am not entirely sure why I should be
>> forced to write such code?
> 
> I don't think you should be. You can find another way. For instance, the
> whole thing could be reduce to one more "if" condition:
> 
>    if ( t != NULL )
>    {
>        *t = p2m_invalid;
>        if ( page->u.inuse.type_info & PGT_writable_page )
>            *t = p2m_ram_rw;
>        else
>            *t = p2m_ram_ro;
>    }
> 
> be creative :-)

Well the code suggested is different from what I intended :). t should 
be set to invalid before the check mfn_valid/get_page. So this needs at 
least 2 checks. 2 places were t != NULL needs to be explained.

As you said this is a matter of taste. If someone disagree on the coding 
style, then he should suggest an alternative way fitting the requirement.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

Hello Julien,

чт, 15 лист. 2018 о 22:20 Julien Grall <julien.grall@arm.com> пише:

> Well the code suggested is different from what I intended :). t should
> be set to invalid before the check mfn_valid/get_page. So this needs at
> least 2 checks. 2 places were t != NULL needs to be explained.

Well, I guess checking a pointer in order to avoid null pointer
dereference is pretty self-explaining. Even if it is done twice.
And the code itself would give a clear idea that we do calculate (and
assign) type value only in case a caller provided us a valid pointer.

For sure, I do not insist on a ternary operator usage.

Sincerely,
Andrii Anisov.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

чт, 15 лист. 2018 о 17:05 Julien Grall <julien.grall@arm.com> пише:
> And in x86 code...
Sorry, did not check.

> You can't assume the compiler will inline even with the inline keyword.
For sure!

> > I'm sorry, but I can't pass my RB for `_t`.
>
> I think this request is unreasonable because this is a matter of taste.
I would not say it is a request. I'm just saying that I personally do
not like that code, it confuses me, so I can't pass RB for it. Sorry
for that :(
I really hope someone else can come up with RB for it.

Sincerely,
Andrii Anisov.

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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

Hello Julien,


It is me again.

On 15.11.18 17:05, Julien Grall wrote:
> On 11/15/18 1:19 PM, Andrii Anisov wrote:
>>> So I would prefer to stick with _t which is quite common within the p2m
>>> code base so far.
>>
>> I've found a similar code only in one place - p2m_get_entry()
>> function. And it is, at least, somehow commented there:
>> ...
>>      /* Allow t to be NULL */
>>      t = t ?: &_t;
>>
>>      *t = p2m_invalid;
>> ...
>
> And in x86 code...
I've taken care to look into the x86 p2m code about that construction.

I've found it in x86's  `p2m_get_page_from_gfn` function. And it looks 
reasonable over there. Because that value is not only might be returned 
to a caller. But is needed by the function code itself to take decisions 
and passing it to called functions (which do not do a check for the null 
pointer).


Your code (as well as `p2m_get_entry`) does not actually use that value. 
So I see no need to calculate or keep it if it is not needed by a caller.

-- 
Sincerely,
Andrii Anisov.


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

Re: [PATCH 4/8] xen/arm: Add support for read-only foreign mappings

[Andrii Anisov]

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

-- 
Sincerely,
Andrii Anisov.

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

[PATCH 5/8] xen/arm: Allow a privileged domain to map foreign page from DOMID_XEN

[Julien Grall]

For auto-translated domain, the only way to map page to itself is the
using foreign map API. The current code does not allow mapping page from
special page (such as DOMID_XEN).

As xentrace buffer are shared using DOMID_XEN, it is not possible to use
tracing for Arm.

This could be solved by using the helper get_pg_owner(). This helper will
be able to get a reference on DOMID_XEN and therefore allow mapping for
privileged domain.

This patch replace the call to rcu_lock_domain_by_any_id() with
get_pg_owner(). For consistency, all the call to rcu_unlock_domain are
replaced by put_pg_owner().

Signed-off-by: Julien grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e31b47a394..72d0285768 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1249,20 +1249,20 @@ int xenmem_add_to_physmap_one(
         struct domain *od;
         p2m_type_t p2mt;
 
-        od = rcu_lock_domain_by_any_id(extra.foreign_domid);
+        od = get_pg_owner(extra.foreign_domid);
         if ( od == NULL )
             return -ESRCH;
 
         if ( od == d )
         {
-            rcu_unlock_domain(od);
+            put_pg_owner(od);
             return -EINVAL;
         }
 
         rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
         if ( rc )
         {
-            rcu_unlock_domain(od);
+            put_pg_owner(od);
             return rc;
         }
 
@@ -1271,21 +1271,21 @@ int xenmem_add_to_physmap_one(
         page = get_page_from_gfn(od, idx, &p2mt, P2M_ALLOC);
         if ( !page )
         {
-            rcu_unlock_domain(od);
+            put_pg_owner(od);
             return -EINVAL;
         }
 
         if ( !p2m_is_ram(p2mt) )
         {
             put_page(page);
-            rcu_unlock_domain(od);
+            put_pg_owner(od);
             return -EINVAL;
         }
 
         mfn = page_to_mfn(page);
         t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
 
-        rcu_unlock_domain(od);
+        put_pg_owner(od);
         break;
     }
     case XENMAPSPACE_dev_mmio:
-- 
2.11.0


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

Re: [PATCH 5/8] xen/arm: Allow a privileged domain to map foreign page from DOMID_XEN

[Andrii Anisov]

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Sincerely,
Andrii Anisov.

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

[PATCH 6/8] xen/arm: Initialize trace buffer

[Julien Grall]

From: Benjamin Sanda <ben.sanda@dornerworks.com>

Now that we allow a privileged domain to map tracing buffer, initialize
them so a user can effectively trace Xen.

Signed-off-by: Benjamin Sanda <ben.sanda@dornerworks.com>
[julien: rework commit message]
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 80f00286d3..7022904cc3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -36,6 +36,7 @@
 #include <xen/pfn.h>
 #include <xen/virtual_region.h>
 #include <xen/vmap.h>
+#include <xen/trace.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/acpi.h>
 #include <asm/alternative.h>
@@ -863,6 +864,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     heap_init_late();
 
+    init_trace_bufs();
+
     init_constructors();
 
     console_endboot();
-- 
2.11.0


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

Re: [PATCH 6/8] xen/arm: Initialize trace buffer

[Andrii Anisov]

Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

Sincerely,
Andrii Anisov.

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

[PATCH 7/8] xenalyze: Build for Both ARM and x86 Platforms

[Julien Grall]

From: Benjamin Sanda <ben.sanda@dornerworks.com>

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>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 tools/xentrace/Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile
index 0bad942bdf..9fb7fc96e7 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.11.0


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

Re: [PATCH 7/8] xenalyze: Build for Both ARM and x86 Platforms

[Wei Liu]

On Tue, Nov 06, 2018 at 07:14:53PM +0000, Julien Grall wrote:
> From: Benjamin Sanda <ben.sanda@dornerworks.com>
> 
> 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>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

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

[PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall]

No functional change intended.

Only reasonable clean-ups are done in this patch. The rest will use _gfn
for the time being.

Signed-off-by: Julien Grall <julie.grall@arm.com>
---
 xen/arch/arm/guestcopy.c             |  2 +-
 xen/arch/arm/mm.c                    |  2 +-
 xen/arch/x86/cpu/vpmu.c              |  2 +-
 xen/arch/x86/domain.c                | 12 ++++++------
 xen/arch/x86/domctl.c                |  6 +++---
 xen/arch/x86/hvm/dm.c                |  2 +-
 xen/arch/x86/hvm/domain.c            |  2 +-
 xen/arch/x86/hvm/hvm.c               |  9 +++++----
 xen/arch/x86/hvm/svm/svm.c           |  8 ++++----
 xen/arch/x86/hvm/viridian/viridian.c | 24 ++++++++++++------------
 xen/arch/x86/hvm/vmx/vmx.c           |  4 ++--
 xen/arch/x86/hvm/vmx/vvmx.c          | 12 ++++++------
 xen/arch/x86/mm.c                    | 24 ++++++++++++++----------
 xen/arch/x86/mm/p2m.c                |  2 +-
 xen/arch/x86/mm/shadow/hvm.c         |  6 +++---
 xen/arch/x86/physdev.c               |  3 ++-
 xen/arch/x86/pv/descriptor-tables.c  |  5 ++---
 xen/arch/x86/pv/emul-priv-op.c       |  6 +++---
 xen/arch/x86/pv/mm.c                 |  2 +-
 xen/arch/x86/traps.c                 | 11 ++++++-----
 xen/common/domain.c                  |  2 +-
 xen/common/event_fifo.c              | 12 ++++++------
 xen/common/memory.c                  |  4 ++--
 xen/common/tmem_xen.c                |  2 +-
 xen/include/asm-arm/p2m.h            |  6 +++---
 xen/include/asm-x86/p2m.h            | 11 +++++++----
 26 files changed, 95 insertions(+), 86 deletions(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 7a0f3e9d5f..55892062bb 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -37,7 +37,7 @@ static struct page_info *translate_get_page(copy_info_t info, uint64_t addr,
         return get_page_from_gva(info.gva.v, addr,
                                  write ? GV2M_WRITE : GV2M_READ);
 
-    page = get_page_from_gfn(info.gpa.d, paddr_to_pfn(addr), &p2mt, P2M_ALLOC);
+    page = get_page_from_gfn(info.gpa.d, gaddr_to_gfn(addr), &p2mt, P2M_ALLOC);
 
     if ( !page )
         return NULL;
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 72d0285768..88711096ef 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1268,7 +1268,7 @@ int xenmem_add_to_physmap_one(
 
         /* Take reference to the foreign domain page.
          * Reference will be released in XENMEM_remove_from_physmap */
-        page = get_page_from_gfn(od, idx, &p2mt, P2M_ALLOC);
+        page = get_page_from_gfn(od, _gfn(idx), &p2mt, P2M_ALLOC);
         if ( !page )
         {
             put_pg_owner(od);
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 8a4f753eae..4d8f153031 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -607,7 +607,7 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t *params)
     struct vcpu *v;
     struct vpmu_struct *vpmu;
     struct page_info *page;
-    uint64_t gfn = params->val;
+    gfn_t gfn = _gfn(params->val);
 
     if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] == NULL) )
         return -EINVAL;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f6fe954313..c5cce4b38d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -797,7 +797,7 @@ int arch_set_info_guest(
     unsigned long flags;
     bool compat;
 #ifdef CONFIG_PV
-    unsigned long cr3_gfn;
+    gfn_t cr3_gfn;
     struct page_info *cr3_page;
     unsigned long cr4;
     int rc = 0;
@@ -1061,9 +1061,9 @@ int arch_set_info_guest(
     set_bit(_VPF_in_reset, &v->pause_flags);
 
     if ( !compat )
-        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
+        cr3_gfn = _gfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
     else
-        cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);
+        cr3_gfn = _gfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
     cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
 
     if ( !cr3_page )
@@ -1092,7 +1092,7 @@ int arch_set_info_guest(
         case 0:
             if ( !compat && !VM_ASSIST(d, m2p_strict) &&
                  !paging_mode_refcounts(d) )
-                fill_ro_mpt(_mfn(cr3_gfn));
+                fill_ro_mpt(_mfn(gfn_x(cr3_gfn)));
             break;
         default:
             if ( cr3_page == current->arch.old_guest_table )
@@ -1107,7 +1107,7 @@ int arch_set_info_guest(
         v->arch.guest_table = pagetable_from_page(cr3_page);
         if ( c.nat->ctrlreg[1] )
         {
-            cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
+            cr3_gfn = _gfn(xen_cr3_to_pfn(c.nat->ctrlreg[1]));
             cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
 
             if ( !cr3_page )
@@ -1132,7 +1132,7 @@ int arch_set_info_guest(
                     break;
                 case 0:
                     if ( VM_ASSIST(d, m2p_strict) )
-                        zap_ro_mpt(_mfn(cr3_gfn));
+                        zap_ro_mpt(_mfn(gfn_x(cr3_gfn)));
                     break;
                 }
             }
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 33f9a869c0..6b0d8075cd 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -448,7 +448,7 @@ long arch_do_domctl(
                 break;
             }
 
-            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
+            page = get_page_from_gfn(d, _gfn(gfn), &t, P2M_ALLOC);
 
             if ( unlikely(!page) ||
                  unlikely(is_xen_heap_page(page)) )
@@ -498,11 +498,11 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_hypercall_init:
     {
-        unsigned long gmfn = domctl->u.hypercall_init.gmfn;
+        gfn_t gfn = _gfn(domctl->u.hypercall_init.gmfn);
         struct page_info *page;
         void *hypercall_page;
 
-        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d6d0e8be89..3b3ad27938 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -186,7 +186,7 @@ static int modified_memory(struct domain *d,
         {
             struct page_info *page;
 
-            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
+            page = get_page_from_gfn(d, _gfn(pfn), NULL, P2M_UNSHARE);
             if ( page )
             {
                 paging_mark_pfn_dirty(d, _pfn(pfn));
diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
index 5d5a746a25..73d2da8441 100644
--- a/xen/arch/x86/hvm/domain.c
+++ b/xen/arch/x86/hvm/domain.c
@@ -297,7 +297,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
     {
         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
         struct page_info *page = get_page_from_gfn(v->domain,
-                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
+                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),
                                  NULL, P2M_ALLOC);
         if ( !page )
         {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7be9cf4454..be262e5a1d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2146,7 +2146,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
 {
     struct vcpu *v = current;
     struct domain *d = v->domain;
-    unsigned long gfn, old_value = v->arch.hvm.guest_cr[0];
+    unsigned long old_value = v->arch.hvm.guest_cr[0];
     struct page_info *page;
 
     HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR0 value = %lx", value);
@@ -2201,7 +2201,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
         if ( !paging_mode_hap(d) )
         {
             /* The guest CR3 must be pointing to the guest physical. */
-            gfn = v->arch.hvm.guest_cr[3] >> PAGE_SHIFT;
+            gfn_t gfn = gaddr_to_gfn(v->arch.hvm.guest_cr[3]);
+
             page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
             if ( !page )
             {
@@ -2293,7 +2294,7 @@ int hvm_set_cr3(unsigned long value, bool may_defer)
     {
         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
-        page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
+        page = get_page_from_gfn(v->domain, gaddr_to_gfn(value),
                                  NULL, P2M_ALLOC);
         if ( !page )
             goto bad_cr3;
@@ -3120,7 +3121,7 @@ enum hvm_translation_result hvm_translate_get_page(
          && hvm_mmio_internal(gfn_to_gaddr(gfn)) )
         return HVMTRANS_bad_gfn_to_mfn;
 
-    page = get_page_from_gfn(v->domain, gfn_x(gfn), &p2mt, P2M_UNSHARE);
+    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_UNSHARE);
 
     if ( !page )
         return HVMTRANS_bad_gfn_to_mfn;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 5d00256aaa..a7419bd444 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
     {
         if ( c->cr0 & X86_CR0_PG )
         {
-            page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
+            page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
                                      NULL, P2M_ALLOC);
             if ( !page )
             {
@@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t vmcbaddr)
         return NULL;
 
     /* Need to translate L1-GPA to MPA */
-    page = get_page_from_gfn(v->domain, 
-                            nv->nv_vvmcxaddr >> PAGE_SHIFT, 
-                            &p2mt, P2M_ALLOC | P2M_UNSHARE);
+    page = get_page_from_gfn(v->domain,
+                             gaddr_to_gfn(nv->nv_vvmcxaddr >> PAGE_SHIFT),
+                             &p2mt, P2M_ALLOC | P2M_UNSHARE);
     if ( !page )
         return NULL;
 
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 2dc86dd0f3..1d3be156db 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -332,16 +332,16 @@ static void dump_reference_tsc(const struct domain *d)
 
 static void enable_hypercall_page(struct domain *d)
 {
-    unsigned long gmfn = d->arch.hvm.viridian.hypercall_gpa.fields.pfn;
-    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+    gfn_t gfn = _gfn(d->arch.hvm.viridian.hypercall_gpa.fields.pfn);
+    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
     uint8_t *p;
 
     if ( !page || !get_page_type(page, PGT_writable_page) )
     {
         if ( page )
             put_page(page);
-        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
-                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+                 gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
         return;
     }
 
@@ -367,8 +367,8 @@ static void enable_hypercall_page(struct domain *d)
 static void initialize_vp_assist(struct vcpu *v)
 {
     struct domain *d = v->domain;
-    unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
-    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+    gfn_t gfn = _gfn(v->arch.hvm.viridian.vp_assist.msr.fields.pfn);
+    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
     void *va;
 
     ASSERT(!v->arch.hvm.viridian.vp_assist.va);
@@ -395,8 +395,8 @@ static void initialize_vp_assist(struct vcpu *v)
     return;
 
  fail:
-    gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
-             gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+    gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+             gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
 }
 
 static void teardown_vp_assist(struct vcpu *v)
@@ -465,16 +465,16 @@ void viridian_apic_assist_clear(struct vcpu *v)
 
 static void update_reference_tsc(struct domain *d, bool_t initialize)
 {
-    unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
-    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
+    gfn_t gfn = _gfn(d->arch.hvm.viridian.reference_tsc.fields.pfn);
+    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
     HV_REFERENCE_TSC_PAGE *p;
 
     if ( !page || !get_page_type(page, PGT_writable_page) )
     {
         if ( page )
             put_page(page);
-        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
-                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
+        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
+                 gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
         return;
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e065f8bbdb..2070e78358 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -674,7 +674,7 @@ static int vmx_restore_cr0_cr3(
     {
         if ( cr0 & X86_CR0_PG )
         {
-            page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT,
+            page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3),
                                      NULL, P2M_ALLOC);
             if ( !page )
             {
@@ -1373,7 +1373,7 @@ static void vmx_load_pdptrs(struct vcpu *v)
     if ( (cr3 & 0x1fUL) && !hvm_pcid_enabled(v) )
         goto crash;
 
-    page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT, &p2mt, P2M_UNSHARE);
+    page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3), &p2mt, P2M_UNSHARE);
     if ( !page )
     {
         /* Ideally you don't want to crash but rather go into a wait 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index dfd08e2d0a..2953d05a17 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -649,11 +649,11 @@ static void nvmx_update_apic_access_address(struct vcpu *v)
     if ( ctrl & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES )
     {
         p2m_type_t p2mt;
-        unsigned long apic_gpfn;
+        gfn_t apic_gfn;
         struct page_info *apic_pg;
 
-        apic_gpfn = get_vvmcs(v, APIC_ACCESS_ADDR) >> PAGE_SHIFT;
-        apic_pg = get_page_from_gfn(v->domain, apic_gpfn, &p2mt, P2M_ALLOC);
+        apic_gfn = gaddr_to_gfn(get_vvmcs(v, APIC_ACCESS_ADDR));
+        apic_pg = get_page_from_gfn(v->domain, apic_gfn, &p2mt, P2M_ALLOC);
         ASSERT(apic_pg && !p2m_is_paging(p2mt));
         __vmwrite(APIC_ACCESS_ADDR, page_to_maddr(apic_pg));
         put_page(apic_pg);
@@ -670,11 +670,11 @@ static void nvmx_update_virtual_apic_address(struct vcpu *v)
     if ( ctrl & CPU_BASED_TPR_SHADOW )
     {
         p2m_type_t p2mt;
-        unsigned long vapic_gpfn;
+        gfn_t vapic_gfn;
         struct page_info *vapic_pg;
 
-        vapic_gpfn = get_vvmcs(v, VIRTUAL_APIC_PAGE_ADDR) >> PAGE_SHIFT;
-        vapic_pg = get_page_from_gfn(v->domain, vapic_gpfn, &p2mt, P2M_ALLOC);
+        vapic_gfn = gaddr_to_gfn(get_vvmcs(v, VIRTUAL_APIC_PAGE_ADDR));
+        vapic_pg = get_page_from_gfn(v->domain, vapic_gfn, &p2mt, P2M_ALLOC);
         ASSERT(vapic_pg && !p2m_is_paging(p2mt));
         __vmwrite(VIRTUAL_APIC_PAGE_ADDR, page_to_maddr(vapic_pg));
         put_page(vapic_pg);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 9363e9bd96..e3462f8a77 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2052,7 +2052,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e, l1_pgentry_t nl1e,
             p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
                             P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
 
-            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
+            page = get_page_from_gfn(pg_dom, _gfn(l1e_get_pfn(nl1e)), &p2mt, q);
 
             if ( p2m_is_paged(p2mt) )
             {
@@ -3223,7 +3223,8 @@ long do_mmuext_op(
             if ( paging_mode_refcounts(pg_owner) )
                 break;
 
-            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), NULL,
+                                     P2M_ALLOC);
             if ( unlikely(!page) )
             {
                 rc = -EINVAL;
@@ -3288,7 +3289,8 @@ long do_mmuext_op(
             if ( paging_mode_refcounts(pg_owner) )
                 break;
 
-            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), NULL,
+                                     P2M_ALLOC);
             if ( unlikely(!page) )
             {
                 gdprintk(XENLOG_WARNING,
@@ -3504,7 +3506,8 @@ long do_mmuext_op(
         }
 
         case MMUEXT_CLEAR_PAGE:
-            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt, P2M_ALLOC);
+            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), &p2mt,
+                                     P2M_ALLOC);
             if ( unlikely(p2mt != p2m_ram_rw) && page )
             {
                 put_page(page);
@@ -3532,7 +3535,7 @@ long do_mmuext_op(
         {
             struct page_info *src_page, *dst_page;
 
-            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, &p2mt,
+            src_page = get_page_from_gfn(pg_owner, _gfn(op.arg2.src_mfn), &p2mt,
                                          P2M_ALLOC);
             if ( unlikely(p2mt != p2m_ram_rw) && src_page )
             {
@@ -3548,7 +3551,7 @@ long do_mmuext_op(
                 break;
             }
 
-            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt,
+            dst_page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), &p2mt,
                                          P2M_ALLOC);
             if ( unlikely(p2mt != p2m_ram_rw) && dst_page )
             {
@@ -3636,7 +3639,8 @@ long do_mmu_update(
 {
     struct mmu_update req;
     void *va = NULL;
-    unsigned long gpfn, gmfn, mfn;
+    unsigned long gpfn, mfn;
+    gfn_t gfn;
     struct page_info *page;
     unsigned int cmd, i = 0, done = 0, pt_dom;
     struct vcpu *curr = current, *v = curr;
@@ -3749,8 +3753,8 @@ long do_mmu_update(
             rc = -EINVAL;
 
             req.ptr -= cmd;
-            gmfn = req.ptr >> PAGE_SHIFT;
-            page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
+            gfn = gaddr_to_gfn(req.ptr);
+            page = get_page_from_gfn(pt_owner, gfn, &p2mt, P2M_ALLOC);
 
             if ( unlikely(!page) || p2mt != p2m_ram_rw )
             {
@@ -3758,7 +3762,7 @@ long do_mmu_update(
                     put_page(page);
                 if ( p2m_is_paged(p2mt) )
                 {
-                    p2m_mem_paging_populate(pt_owner, gmfn);
+                    p2m_mem_paging_populate(pt_owner, gfn_x(gfn));
                     rc = -ENOENT;
                 }
                 else
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a00a3c1bff..3b2aac8804 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2718,7 +2718,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
      * Take a refcnt on the mfn. NB: following supported for foreign mapping:
      *     ram_rw | ram_logdirty | ram_ro | paging_out.
      */
-    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
+    page = get_page_from_gfn(fdom, _gfn(fgfn), &p2mt, P2M_ALLOC);
     if ( !page ||
          !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) )
     {
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
index 4cc75916b8..9275ba476c 100644
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -313,15 +313,15 @@ const struct x86_emulate_ops hvm_shadow_emulator_ops = {
 static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned long vaddr,
                                 struct sh_emulate_ctxt *sh_ctxt)
 {
-    unsigned long gfn;
+    gfn_t gfn;
     struct page_info *page;
     mfn_t mfn;
     p2m_type_t p2mt;
     uint32_t pfec = PFEC_page_present | PFEC_write_access;
 
     /* Translate the VA to a GFN. */
-    gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec);
-    if ( gfn == gfn_x(INVALID_GFN) )
+    gfn = _gfn(paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec));
+    if ( gfn_eq(gfn, INVALID_GFN) )
     {
         x86_emul_pagefault(pfec, vaddr, &sh_ctxt->ctxt);
 
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a3c15890b..4f3f438614 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -229,7 +229,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             break;
 
         ret = -EINVAL;
-        page = get_page_from_gfn(current->domain, info.gmfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(current->domain, _gfn(info.gmfn),
+                                 NULL, P2M_ALLOC);
         if ( !page )
             break;
         if ( !get_page_type(page, PGT_writable_page) )
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index 8b2d55fc2e..7e8f41d3fd 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -112,7 +112,7 @@ long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries)
     {
         struct page_info *page;
 
-        page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, _gfn(frames[i]), NULL, P2M_ALLOC);
         if ( !page )
             goto fail;
         if ( !get_page_type(page, PGT_seg_desc_page) )
@@ -209,7 +209,6 @@ int compat_set_gdt(XEN_GUEST_HANDLE_PARAM(uint) frame_list,
 long do_update_descriptor(uint64_t pa, uint64_t desc)
 {
     struct domain *currd = current->domain;
-    unsigned long gmfn = pa >> PAGE_SHIFT;
     unsigned long mfn;
     unsigned int  offset;
     struct desc_struct *gdt_pent, d;
@@ -220,7 +219,7 @@ long do_update_descriptor(uint64_t pa, uint64_t desc)
 
     *(uint64_t *)&d = desc;
 
-    page = get_page_from_gfn(currd, gmfn, NULL, P2M_ALLOC);
+    page = get_page_from_gfn(currd, gaddr_to_gfn(pa), NULL, P2M_ALLOC);
     if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
          !page ||
          !check_descriptor(currd, &d) )
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index f73ea4a163..a529ebcc3f 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned long val,
     case 3: /* Write CR3 */
     {
         struct domain *currd = curr->domain;
-        unsigned long gfn;
+        gfn_t gfn;
         struct page_info *page;
         int rc;
 
-        gfn = !is_pv_32bit_domain(currd)
-              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
+        gfn = _gfn(!is_pv_32bit_domain(currd)
+              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));
         page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
         if ( !page )
             break;
diff --git a/xen/arch/x86/pv/mm.c b/xen/arch/x86/pv/mm.c
index f5ea00ca4e..c9ad1152b4 100644
--- a/xen/arch/x86/pv/mm.c
+++ b/xen/arch/x86/pv/mm.c
@@ -106,7 +106,7 @@ bool pv_map_ldt_shadow_page(unsigned int offset)
     if ( unlikely(!(l1e_get_flags(gl1e) & _PAGE_PRESENT)) )
         return false;
 
-    page = get_page_from_gfn(currd, l1e_get_pfn(gl1e), NULL, P2M_ALLOC);
+    page = get_page_from_gfn(currd, _gfn(l1e_get_pfn(gl1e)), NULL, P2M_ALLOC);
     if ( unlikely(!page) )
         return false;
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9471d89022..d967e49432 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -795,7 +795,7 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
     case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> PAGE_SHIFT;
+        gfn_t gfn = gaddr_to_gfn(val);
         unsigned int page_index = val & (PAGE_SIZE - 1);
         struct page_info *page;
         p2m_type_t t;
@@ -808,7 +808,7 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
             return X86EMUL_EXCEPTION;
         }
 
-        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
+        page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
 
         if ( !page || !get_page_type(page, PGT_writable_page) )
         {
@@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
 
             if ( p2m_is_paging(t) )
             {
-                p2m_mem_paging_populate(d, gmfn);
+                p2m_mem_paging_populate(d, gfn_x(gfn));
                 return X86EMUL_RETRY;
             }
 
             gdprintk(XENLOG_WARNING,
-                     "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
-                     gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN), base);
+                     "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR %08x\n",
+                     gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN),
+                     base);
             return X86EMUL_EXCEPTION;
         }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index d6650f0656..5e3c05b96c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1250,7 +1250,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     if ( (v != current) && !(v->pause_flags & VPF_down) )
         return -EINVAL;
 
-    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
+    page = get_page_from_gfn(d, _gfn(gfn), NULL, P2M_ALLOC);
     if ( !page )
         return -EINVAL;
 
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index c49f446754..71a6f673b2 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -358,7 +358,7 @@ static const struct evtchn_port_ops evtchn_port_ops_fifo =
     .print_state   = evtchn_fifo_print_state,
 };
 
-static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)
+static int map_guest_page(struct domain *d, gfn_t gfn, void **virt)
 {
     struct page_info *p;
 
@@ -419,7 +419,7 @@ static int setup_control_block(struct vcpu *v)
     return 0;
 }
 
-static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t offset)
+static int map_control_block(struct vcpu *v, gfn_t gfn, uint32_t offset)
 {
     void *virt;
     unsigned int i;
@@ -505,7 +505,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
 {
     struct domain *d = current->domain;
     uint32_t vcpu_id;
-    uint64_t gfn;
+    gfn_t gfn;
     uint32_t offset;
     struct vcpu *v;
     int rc;
@@ -513,7 +513,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
     init_control->link_bits = EVTCHN_FIFO_LINK_BITS;
 
     vcpu_id = init_control->vcpu;
-    gfn     = init_control->control_gfn;
+    gfn     = _gfn(init_control->control_gfn);
     offset  = init_control->offset;
 
     if ( vcpu_id >= d->max_vcpus || !d->vcpu[vcpu_id] )
@@ -569,7 +569,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
     return rc;
 }
 
-static int add_page_to_event_array(struct domain *d, unsigned long gfn)
+static int add_page_to_event_array(struct domain *d, gfn_t gfn)
 {
     void *virt;
     unsigned int slot;
@@ -619,7 +619,7 @@ int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array)
         return -EOPNOTSUPP;
 
     spin_lock(&d->event_lock);
-    rc = add_page_to_event_array(d, expand_array->array_gfn);
+    rc = add_page_to_event_array(d, _gfn(expand_array->array_gfn));
     spin_unlock(&d->event_lock);
 
     return rc;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 987395fbb3..e02733dba0 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1365,7 +1365,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return rc;
         }
 
-        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
+        page = get_page_from_gfn(d, _gfn(xrfp.gpfn), NULL, P2M_ALLOC);
         if ( page )
         {
             rc = guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
@@ -1636,7 +1636,7 @@ int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
     p2m_type_t p2mt;
     struct page_info *page;
 
-    page = get_page_from_gfn(d, gfn_x(gfn), &p2mt, q);
+    page = get_page_from_gfn(d, gfn, &p2mt, q);
 
 #ifdef CONFIG_HAS_MEM_PAGING
     if ( p2m_is_paging(p2mt) )
diff --git a/xen/common/tmem_xen.c b/xen/common/tmem_xen.c
index bf7b14f79a..72cba7f10c 100644
--- a/xen/common/tmem_xen.c
+++ b/xen/common/tmem_xen.c
@@ -52,7 +52,7 @@ static inline void *cli_get_page(xen_pfn_t cmfn, mfn_t *pcli_mfn,
     p2m_type_t t;
     struct page_info *page;
 
-    page = get_page_from_gfn(current->domain, cmfn, &t, P2M_ALLOC);
+    page = get_page_from_gfn(current->domain, _gfn(cmfn), &t, P2M_ALLOC);
     if ( !page || t != p2m_ram_rw )
     {
         if ( page )
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 7c67806056..5e598a0b37 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -278,7 +278,7 @@ struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
                                         p2m_type_t *t);
 
 static inline struct page_info *get_page_from_gfn(
-    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
+    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
 {
     mfn_t mfn;
     p2m_type_t _t;
@@ -289,7 +289,7 @@ static inline struct page_info *get_page_from_gfn(
      * not auto-translated.
      */
     if ( unlikely(d != dom_xen) )
-        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
+        return p2m_get_page_from_gfn(d, gfn, t);
 
     if ( !t )
         t = &_t;
@@ -300,7 +300,7 @@ static inline struct page_info *get_page_from_gfn(
      * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
      * page.
      */
-    mfn = _mfn(gfn);
+    mfn = _mfn(gfn_x(gfn));
     page = mfn_to_page(mfn);
 
     if ( !mfn_valid(mfn) || !get_page(page, d) )
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d08c595887..db1ec37610 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
                                         p2m_query_t q);
 
 static inline struct page_info *get_page_from_gfn(
-    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
+    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
 {
     struct page_info *page;
+    mfn_t mfn;
 
     if ( paging_mode_translate(d) )
-        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t, NULL, q);
+        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL, q);
 
     /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
     if ( t )
         *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
-    page = mfn_to_page(_mfn(gfn));
-    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
+
+    mfn = _mfn(gfn_x(gfn));
+    page = mfn_to_page(mfn);
+    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
 }
 
 /* General conversion function from mfn to gfn */
-- 
2.11.0


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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Andrew Cooper]

Hi - just some cosmetic suggestions.

Subject s/Swich/Switch/

> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index f73ea4a163..a529ebcc3f 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned long val,
>      case 3: /* Write CR3 */
>      {
>          struct domain *currd = curr->domain;
> -        unsigned long gfn;
> +        gfn_t gfn;
>          struct page_info *page;
>          int rc;
>  
> -        gfn = !is_pv_32bit_domain(currd)
> -              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> +        gfn = _gfn(!is_pv_32bit_domain(currd)
> +              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));

Please re-indent.

>          page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
>          if ( !page )
>              break;
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9471d89022..d967e49432 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
>  
>              if ( p2m_is_paging(t) )
>              {
> -                p2m_mem_paging_populate(d, gmfn);
> +                p2m_mem_paging_populate(d, gfn_x(gfn));
>                  return X86EMUL_RETRY;
>              }
>  
>              gdprintk(XENLOG_WARNING,
> -                     "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> -                     gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN), base);
> +                     "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR %08x\n",

GMFN => GFN.

> +                     gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN),
> +                     base);
>              return X86EMUL_EXCEPTION;
>          }
>  
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index d08c595887..db1ec37610 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
>                                          p2m_query_t q);
>  
>  static inline struct page_info *get_page_from_gfn(
> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> +    mfn_t mfn;
>  
>      if ( paging_mode_translate(d) )
> -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t, NULL, q);
> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL, q);
>  
>      /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
>      if ( t )
>          *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> -    page = mfn_to_page(_mfn(gfn));
> -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> +
> +    mfn = _mfn(gfn_x(gfn));
> +    page = mfn_to_page(mfn);
> +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;

This looks like it would be cleaner by not splitting mfn out into a
separate variable.

page = mfn_to_page(_mfn(gfn_x(gfn)));

return mfn_valid(mfn) && get_page(page, d) ? page : NULL;

The only reason this looks odd is because of the mfn => gfn equality,
but we are just beside a comment explaining that we are non-translated.

~Andrew

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 November 2018 20:12
> To: Julien Grall <julien.grall@arm.com>; sstabellini@kernel.org; xen-
> devel@lists.xenproject.org
> Cc: George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei
> Liu <wei.liu2@citrix.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
> <brian.woods@amd.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Julien Grall <julie.grall@arm.com>
> Subject: Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
> typesafe gfn
> 
> Hi - just some cosmetic suggestions.
> 
> Subject s/Swich/Switch/
> 
> > diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
> op.c
> > index f73ea4a163..a529ebcc3f 100644
> > --- a/xen/arch/x86/pv/emul-priv-op.c
> > +++ b/xen/arch/x86/pv/emul-priv-op.c
> > @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned
> long val,
> >      case 3: /* Write CR3 */
> >      {
> >          struct domain *currd = curr->domain;
> > -        unsigned long gfn;
> > +        gfn_t gfn;
> >          struct page_info *page;
> >          int rc;
> >
> > -        gfn = !is_pv_32bit_domain(currd)
> > -              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> > +        gfn = _gfn(!is_pv_32bit_domain(currd)
> > +              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));
> 
> Please re-indent.
> 
> >          page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
> >          if ( !page )
> >              break;
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index 9471d89022..d967e49432 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
> >
> >              if ( p2m_is_paging(t) )
> >              {
> > -                p2m_mem_paging_populate(d, gmfn);
> > +                p2m_mem_paging_populate(d, gfn_x(gfn));
> >                  return X86EMUL_RETRY;
> >              }
> >
> >              gdprintk(XENLOG_WARNING,
> > -                     "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> > -                     gmfn, mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN), base);
> > +                     "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR
> %08x\n",
> 
> GMFN => GFN.
> 
> > +                     gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN),
> > +                     base);
> >              return X86EMUL_EXCEPTION;
> >          }
> >
> > diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> > index d08c595887..db1ec37610 100644
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct
> p2m_domain *p2m, gfn_t gfn,
> >                                          p2m_query_t q);
> >
> >  static inline struct page_info *get_page_from_gfn(
> > -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> > +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
> >  {
> >      struct page_info *page;
> > +    mfn_t mfn;
> >
> >      if ( paging_mode_translate(d) )
> > -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t,
> NULL, q);
> > +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL,
> q);
> >
> >      /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
> >      if ( t )
> >          *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> > -    page = mfn_to_page(_mfn(gfn));
> > -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> > +
> > +    mfn = _mfn(gfn_x(gfn));
> > +    page = mfn_to_page(mfn);
> > +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
> 
> This looks like it would be cleaner by not splitting mfn out into a
> separate variable.
> 
> page = mfn_to_page(_mfn(gfn_x(gfn)));
> 
> return mfn_valid(mfn) && get_page(page, d) ? page : NULL;

^^ er... how's that mfn_valid() going to work? You'd need mfn_valid(page_to_mfn(pahge)), or somesuch.

  Paul

> 
> The only reason this looks odd is because of the mfn => gfn equality,
> but we are just beside a comment explaining that we are non-translated.
> 
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Andrew Cooper]

On 07/11/18 08:57, Paul Durrant wrote:
>>
>>>  static inline struct page_info *get_page_from_gfn(
>>> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
>>> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>>>  {
>>>      struct page_info *page;
>>> +    mfn_t mfn;
>>>
>>>      if ( paging_mode_translate(d) )
>>> -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t,
>> NULL, q);
>>> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL,
>> q);
>>>      /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
>>>      if ( t )
>>>          *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
>>> -    page = mfn_to_page(_mfn(gfn));
>>> -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
>>> +
>>> +    mfn = _mfn(gfn_x(gfn));
>>> +    page = mfn_to_page(mfn);
>>> +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
>> This looks like it would be cleaner by not splitting mfn out into a
>> separate variable.
>>
>> page = mfn_to_page(_mfn(gfn_x(gfn)));
>>
>> return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
> ^^ er... how's that mfn_valid() going to work? You'd need mfn_valid(page_to_mfn(pahge)), or somesuch.

Oops - I'm blind.  Sorry for the noise.

~Andrew

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall]

Hi Andrew,

On 06/11/2018 20:11, Andrew Cooper wrote:
> This looks like it would be cleaner by not splitting mfn out into a
> separate variable.
> 
> page = mfn_to_page(_mfn(gfn_x(gfn)));
> 
> return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
> 
> The only reason this looks odd is because of the mfn => gfn equality,
> but we are just beside a comment explaining that we are non-translated.

I introduced the mfn variable to avoid duplicating _mfn(gfn_x(gfn)) in the two 
places where gfn was used.

I am happy to duplicated if you prefer.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 06 November 2018 19:15
> To: sstabellini@kernel.org; xen-devel@lists.xenproject.org
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei
> Liu <wei.liu2@citrix.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
> <brian.woods@amd.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
> Julien Grall <julie.grall@arm.com>
> Subject: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
> typesafe gfn
> 
> No functional change intended.
> 
> Only reasonable clean-ups are done in this patch. The rest will use _gfn
> for the time being.
> 
> Signed-off-by: Julien Grall <julie.grall@arm.com>
> ---
>  xen/arch/arm/guestcopy.c             |  2 +-
>  xen/arch/arm/mm.c                    |  2 +-
>  xen/arch/x86/cpu/vpmu.c              |  2 +-
>  xen/arch/x86/domain.c                | 12 ++++++------
>  xen/arch/x86/domctl.c                |  6 +++---
>  xen/arch/x86/hvm/dm.c                |  2 +-
>  xen/arch/x86/hvm/domain.c            |  2 +-
>  xen/arch/x86/hvm/hvm.c               |  9 +++++----
>  xen/arch/x86/hvm/svm/svm.c           |  8 ++++----
>  xen/arch/x86/hvm/viridian/viridian.c | 24 ++++++++++++------------
>  xen/arch/x86/hvm/vmx/vmx.c           |  4 ++--
>  xen/arch/x86/hvm/vmx/vvmx.c          | 12 ++++++------
>  xen/arch/x86/mm.c                    | 24 ++++++++++++++----------
>  xen/arch/x86/mm/p2m.c                |  2 +-
>  xen/arch/x86/mm/shadow/hvm.c         |  6 +++---
>  xen/arch/x86/physdev.c               |  3 ++-
>  xen/arch/x86/pv/descriptor-tables.c  |  5 ++---
>  xen/arch/x86/pv/emul-priv-op.c       |  6 +++---
>  xen/arch/x86/pv/mm.c                 |  2 +-
>  xen/arch/x86/traps.c                 | 11 ++++++-----
>  xen/common/domain.c                  |  2 +-
>  xen/common/event_fifo.c              | 12 ++++++------
>  xen/common/memory.c                  |  4 ++--
>  xen/common/tmem_xen.c                |  2 +-
>  xen/include/asm-arm/p2m.h            |  6 +++---
>  xen/include/asm-x86/p2m.h            | 11 +++++++----
>  26 files changed, 95 insertions(+), 86 deletions(-)
> 
[snip]
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5d00256aaa..a7419bd444 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct
> hvm_hw_cpu *c)
>      {
>          if ( c->cr0 & X86_CR0_PG )
>          {
> -            page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
> +            page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
>                                       NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t
> vmcbaddr)
>          return NULL;
> 
>      /* Need to translate L1-GPA to MPA */
> -    page = get_page_from_gfn(v->domain,
> -                            nv->nv_vvmcxaddr >> PAGE_SHIFT,
> -                            &p2mt, P2M_ALLOC | P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain,
> +                             gaddr_to_gfn(nv->nv_vvmcxaddr >>
> PAGE_SHIFT),

Don't you need to lose the '>> PAGE_SHIFT' now?

  Paul

> +                             &p2mt, P2M_ALLOC | P2M_UNSHARE);
>      if ( !page )
>          return NULL;
> 

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall]

Hi Paul,

On 07/11/2018 09:24, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall [mailto:julien.grall@arm.com]
>> Sent: 06 November 2018 19:15
>> To: sstabellini@kernel.org; xen-devel@lists.xenproject.org
>> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
>> <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
>> Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
>> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei
>> Liu <wei.liu2@citrix.com>; Boris Ostrovsky <boris.ostrovsky@oracle.com>;
>> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Brian Woods
>> <brian.woods@amd.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jun
>> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>;
>> Julien Grall <julie.grall@arm.com>
>> Subject: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use
>> typesafe gfn
>>
>> No functional change intended.
>>
>> Only reasonable clean-ups are done in this patch. The rest will use _gfn
>> for the time being.
>>
>> Signed-off-by: Julien Grall <julie.grall@arm.com>
>> ---
>>   xen/arch/arm/guestcopy.c             |  2 +-
>>   xen/arch/arm/mm.c                    |  2 +-
>>   xen/arch/x86/cpu/vpmu.c              |  2 +-
>>   xen/arch/x86/domain.c                | 12 ++++++------
>>   xen/arch/x86/domctl.c                |  6 +++---
>>   xen/arch/x86/hvm/dm.c                |  2 +-
>>   xen/arch/x86/hvm/domain.c            |  2 +-
>>   xen/arch/x86/hvm/hvm.c               |  9 +++++----
>>   xen/arch/x86/hvm/svm/svm.c           |  8 ++++----
>>   xen/arch/x86/hvm/viridian/viridian.c | 24 ++++++++++++------------
>>   xen/arch/x86/hvm/vmx/vmx.c           |  4 ++--
>>   xen/arch/x86/hvm/vmx/vvmx.c          | 12 ++++++------
>>   xen/arch/x86/mm.c                    | 24 ++++++++++++++----------
>>   xen/arch/x86/mm/p2m.c                |  2 +-
>>   xen/arch/x86/mm/shadow/hvm.c         |  6 +++---
>>   xen/arch/x86/physdev.c               |  3 ++-
>>   xen/arch/x86/pv/descriptor-tables.c  |  5 ++---
>>   xen/arch/x86/pv/emul-priv-op.c       |  6 +++---
>>   xen/arch/x86/pv/mm.c                 |  2 +-
>>   xen/arch/x86/traps.c                 | 11 ++++++-----
>>   xen/common/domain.c                  |  2 +-
>>   xen/common/event_fifo.c              | 12 ++++++------
>>   xen/common/memory.c                  |  4 ++--
>>   xen/common/tmem_xen.c                |  2 +-
>>   xen/include/asm-arm/p2m.h            |  6 +++---
>>   xen/include/asm-x86/p2m.h            | 11 +++++++----
>>   26 files changed, 95 insertions(+), 86 deletions(-)
>>
> [snip]
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 5d00256aaa..a7419bd444 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct
>> hvm_hw_cpu *c)
>>       {
>>           if ( c->cr0 & X86_CR0_PG )
>>           {
>> -            page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
>> +            page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
>>                                        NULL, P2M_ALLOC);
>>               if ( !page )
>>               {
>> @@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t
>> vmcbaddr)
>>           return NULL;
>>
>>       /* Need to translate L1-GPA to MPA */
>> -    page = get_page_from_gfn(v->domain,
>> -                            nv->nv_vvmcxaddr >> PAGE_SHIFT,
>> -                            &p2mt, P2M_ALLOC | P2M_UNSHARE);
>> +    page = get_page_from_gfn(v->domain,
>> +                             gaddr_to_gfn(nv->nv_vvmcxaddr >>
>> PAGE_SHIFT),
> 
> Don't you need to lose the '>> PAGE_SHIFT' now?

Yes. Brian reported on IRC and now it is fixed.

Thank you for the review.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Andrii Anisov]

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

Hello Julien,

I'm just wondering if this patch really belongs to xentrace series.
It rather looks like a separate cleanup patch.

Sincerely,
Andrii Anisov.


вт, 6 лист. 2018 о 21:16 Julien Grall <julien.grall@arm.com> пише:

> No functional change intended.
>
> Only reasonable clean-ups are done in this patch. The rest will use _gfn
> for the time being.
>
> Signed-off-by: Julien Grall <julie.grall@arm.com>
> ---
>  xen/arch/arm/guestcopy.c             |  2 +-
>  xen/arch/arm/mm.c                    |  2 +-
>  xen/arch/x86/cpu/vpmu.c              |  2 +-
>  xen/arch/x86/domain.c                | 12 ++++++------
>  xen/arch/x86/domctl.c                |  6 +++---
>  xen/arch/x86/hvm/dm.c                |  2 +-
>  xen/arch/x86/hvm/domain.c            |  2 +-
>  xen/arch/x86/hvm/hvm.c               |  9 +++++----
>  xen/arch/x86/hvm/svm/svm.c           |  8 ++++----
>  xen/arch/x86/hvm/viridian/viridian.c | 24 ++++++++++++------------
>  xen/arch/x86/hvm/vmx/vmx.c           |  4 ++--
>  xen/arch/x86/hvm/vmx/vvmx.c          | 12 ++++++------
>  xen/arch/x86/mm.c                    | 24 ++++++++++++++----------
>  xen/arch/x86/mm/p2m.c                |  2 +-
>  xen/arch/x86/mm/shadow/hvm.c         |  6 +++---
>  xen/arch/x86/physdev.c               |  3 ++-
>  xen/arch/x86/pv/descriptor-tables.c  |  5 ++---
>  xen/arch/x86/pv/emul-priv-op.c       |  6 +++---
>  xen/arch/x86/pv/mm.c                 |  2 +-
>  xen/arch/x86/traps.c                 | 11 ++++++-----
>  xen/common/domain.c                  |  2 +-
>  xen/common/event_fifo.c              | 12 ++++++------
>  xen/common/memory.c                  |  4 ++--
>  xen/common/tmem_xen.c                |  2 +-
>  xen/include/asm-arm/p2m.h            |  6 +++---
>  xen/include/asm-x86/p2m.h            | 11 +++++++----
>  26 files changed, 95 insertions(+), 86 deletions(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7a0f3e9d5f..55892062bb 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -37,7 +37,7 @@ static struct page_info *translate_get_page(copy_info_t
> info, uint64_t addr,
>          return get_page_from_gva(info.gva.v, addr,
>                                   write ? GV2M_WRITE : GV2M_READ);
>
> -    page = get_page_from_gfn(info.gpa.d, paddr_to_pfn(addr), &p2mt,
> P2M_ALLOC);
> +    page = get_page_from_gfn(info.gpa.d, gaddr_to_gfn(addr), &p2mt,
> P2M_ALLOC);
>
>      if ( !page )
>          return NULL;
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 72d0285768..88711096ef 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1268,7 +1268,7 @@ int xenmem_add_to_physmap_one(
>
>          /* Take reference to the foreign domain page.
>           * Reference will be released in XENMEM_remove_from_physmap */
> -        page = get_page_from_gfn(od, idx, &p2mt, P2M_ALLOC);
> +        page = get_page_from_gfn(od, _gfn(idx), &p2mt, P2M_ALLOC);
>          if ( !page )
>          {
>              put_pg_owner(od);
> diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
> index 8a4f753eae..4d8f153031 100644
> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -607,7 +607,7 @@ static int pvpmu_init(struct domain *d,
> xen_pmu_params_t *params)
>      struct vcpu *v;
>      struct vpmu_struct *vpmu;
>      struct page_info *page;
> -    uint64_t gfn = params->val;
> +    gfn_t gfn = _gfn(params->val);
>
>      if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] ==
> NULL) )
>          return -EINVAL;
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f6fe954313..c5cce4b38d 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -797,7 +797,7 @@ int arch_set_info_guest(
>      unsigned long flags;
>      bool compat;
>  #ifdef CONFIG_PV
> -    unsigned long cr3_gfn;
> +    gfn_t cr3_gfn;
>      struct page_info *cr3_page;
>      unsigned long cr4;
>      int rc = 0;
> @@ -1061,9 +1061,9 @@ int arch_set_info_guest(
>      set_bit(_VPF_in_reset, &v->pause_flags);
>
>      if ( !compat )
> -        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
> +        cr3_gfn = _gfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
>      else
> -        cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);
> +        cr3_gfn = _gfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
>      cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
>
>      if ( !cr3_page )
> @@ -1092,7 +1092,7 @@ int arch_set_info_guest(
>          case 0:
>              if ( !compat && !VM_ASSIST(d, m2p_strict) &&
>                   !paging_mode_refcounts(d) )
> -                fill_ro_mpt(_mfn(cr3_gfn));
> +                fill_ro_mpt(_mfn(gfn_x(cr3_gfn)));
>              break;
>          default:
>              if ( cr3_page == current->arch.old_guest_table )
> @@ -1107,7 +1107,7 @@ int arch_set_info_guest(
>          v->arch.guest_table = pagetable_from_page(cr3_page);
>          if ( c.nat->ctrlreg[1] )
>          {
> -            cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
> +            cr3_gfn = _gfn(xen_cr3_to_pfn(c.nat->ctrlreg[1]));
>              cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
>
>              if ( !cr3_page )
> @@ -1132,7 +1132,7 @@ int arch_set_info_guest(
>                      break;
>                  case 0:
>                      if ( VM_ASSIST(d, m2p_strict) )
> -                        zap_ro_mpt(_mfn(cr3_gfn));
> +                        zap_ro_mpt(_mfn(gfn_x(cr3_gfn)));
>                      break;
>                  }
>              }
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 33f9a869c0..6b0d8075cd 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -448,7 +448,7 @@ long arch_do_domctl(
>                  break;
>              }
>
> -            page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
> +            page = get_page_from_gfn(d, _gfn(gfn), &t, P2M_ALLOC);
>
>              if ( unlikely(!page) ||
>                   unlikely(is_xen_heap_page(page)) )
> @@ -498,11 +498,11 @@ long arch_do_domctl(
>
>      case XEN_DOMCTL_hypercall_init:
>      {
> -        unsigned long gmfn = domctl->u.hypercall_init.gmfn;
> +        gfn_t gfn = _gfn(domctl->u.hypercall_init.gmfn);
>          struct page_info *page;
>          void *hypercall_page;
>
> -        page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +        page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>
>          if ( !page || !get_page_type(page, PGT_writable_page) )
>          {
> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> index d6d0e8be89..3b3ad27938 100644
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -186,7 +186,7 @@ static int modified_memory(struct domain *d,
>          {
>              struct page_info *page;
>
> -            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> +            page = get_page_from_gfn(d, _gfn(pfn), NULL, P2M_UNSHARE);
>              if ( page )
>              {
>                  paging_mark_pfn_dirty(d, _pfn(pfn));
> diff --git a/xen/arch/x86/hvm/domain.c b/xen/arch/x86/hvm/domain.c
> index 5d5a746a25..73d2da8441 100644
> --- a/xen/arch/x86/hvm/domain.c
> +++ b/xen/arch/x86/hvm/domain.c
> @@ -297,7 +297,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const
> vcpu_hvm_context_t *ctx)
>      {
>          /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
>          struct page_info *page = get_page_from_gfn(v->domain,
> -                                 v->arch.hvm.guest_cr[3] >> PAGE_SHIFT,
> +                                 gaddr_to_gfn(v->arch.hvm.guest_cr[3]),
>                                   NULL, P2M_ALLOC);
>          if ( !page )
>          {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 7be9cf4454..be262e5a1d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2146,7 +2146,7 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
>  {
>      struct vcpu *v = current;
>      struct domain *d = v->domain;
> -    unsigned long gfn, old_value = v->arch.hvm.guest_cr[0];
> +    unsigned long old_value = v->arch.hvm.guest_cr[0];
>      struct page_info *page;
>
>      HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR0 value = %lx", value);
> @@ -2201,7 +2201,8 @@ int hvm_set_cr0(unsigned long value, bool may_defer)
>          if ( !paging_mode_hap(d) )
>          {
>              /* The guest CR3 must be pointing to the guest physical. */
> -            gfn = v->arch.hvm.guest_cr[3] >> PAGE_SHIFT;
> +            gfn_t gfn = gaddr_to_gfn(v->arch.hvm.guest_cr[3]);
> +
>              page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -2293,7 +2294,7 @@ int hvm_set_cr3(unsigned long value, bool may_defer)
>      {
>          /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
>          HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
> -        page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
> +        page = get_page_from_gfn(v->domain, gaddr_to_gfn(value),
>                                   NULL, P2M_ALLOC);
>          if ( !page )
>              goto bad_cr3;
> @@ -3120,7 +3121,7 @@ enum hvm_translation_result hvm_translate_get_page(
>           && hvm_mmio_internal(gfn_to_gaddr(gfn)) )
>          return HVMTRANS_bad_gfn_to_mfn;
>
> -    page = get_page_from_gfn(v->domain, gfn_x(gfn), &p2mt, P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain, gfn, &p2mt, P2M_UNSHARE);
>
>      if ( !page )
>          return HVMTRANS_bad_gfn_to_mfn;
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5d00256aaa..a7419bd444 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -317,7 +317,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct
> hvm_hw_cpu *c)
>      {
>          if ( c->cr0 & X86_CR0_PG )
>          {
> -            page = get_page_from_gfn(v->domain, c->cr3 >> PAGE_SHIFT,
> +            page = get_page_from_gfn(v->domain, gaddr_to_gfn(c->cr3),
>                                       NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -2412,9 +2412,9 @@ nsvm_get_nvmcb_page(struct vcpu *v, uint64_t
> vmcbaddr)
>          return NULL;
>
>      /* Need to translate L1-GPA to MPA */
> -    page = get_page_from_gfn(v->domain,
> -                            nv->nv_vvmcxaddr >> PAGE_SHIFT,
> -                            &p2mt, P2M_ALLOC | P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain,
> +                             gaddr_to_gfn(nv->nv_vvmcxaddr >> PAGE_SHIFT),
> +                             &p2mt, P2M_ALLOC | P2M_UNSHARE);
>      if ( !page )
>          return NULL;
>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 2dc86dd0f3..1d3be156db 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -332,16 +332,16 @@ static void dump_reference_tsc(const struct domain
> *d)
>
>  static void enable_hypercall_page(struct domain *d)
>  {
> -    unsigned long gmfn = d->arch.hvm.viridian.hypercall_gpa.fields.pfn;
> -    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    gfn_t gfn = _gfn(d->arch.hvm.viridian.hypercall_gpa.fields.pfn);
> +    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>      uint8_t *p;
>
>      if ( !page || !get_page_type(page, PGT_writable_page) )
>      {
>          if ( page )
>              put_page(page);
> -        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN
> %#"PRI_mfn")\n",
> -                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> +        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN
> %#"PRI_mfn")\n",
> +                 gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN));
>          return;
>      }
>
> @@ -367,8 +367,8 @@ static void enable_hypercall_page(struct domain *d)
>  static void initialize_vp_assist(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> -    unsigned long gmfn = v->arch.hvm.viridian.vp_assist.msr.fields.pfn;
> -    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    gfn_t gfn = _gfn(v->arch.hvm.viridian.vp_assist.msr.fields.pfn);
> +    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>      void *va;
>
>      ASSERT(!v->arch.hvm.viridian.vp_assist.va);
> @@ -395,8 +395,8 @@ static void initialize_vp_assist(struct vcpu *v)
>      return;
>
>   fail:
> -    gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> -             gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> +    gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN %#"PRI_mfn")\n",
> +             gfn_x(gfn), mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
>  }
>
>  static void teardown_vp_assist(struct vcpu *v)
> @@ -465,16 +465,16 @@ void viridian_apic_assist_clear(struct vcpu *v)
>
>  static void update_reference_tsc(struct domain *d, bool_t initialize)
>  {
> -    unsigned long gmfn = d->arch.hvm.viridian.reference_tsc.fields.pfn;
> -    struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
> +    gfn_t gfn = _gfn(d->arch.hvm.viridian.reference_tsc.fields.pfn);
> +    struct page_info *page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>      HV_REFERENCE_TSC_PAGE *p;
>
>      if ( !page || !get_page_type(page, PGT_writable_page) )
>      {
>          if ( page )
>              put_page(page);
> -        gdprintk(XENLOG_WARNING, "Bad GMFN %#"PRI_gfn" (MFN
> %#"PRI_mfn")\n",
> -                 gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN));
> +        gdprintk(XENLOG_WARNING, "Bad GFN %#"PRI_gfn" (MFN
> %#"PRI_mfn")\n",
> +                 gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN));
>          return;
>      }
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e065f8bbdb..2070e78358 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -674,7 +674,7 @@ static int vmx_restore_cr0_cr3(
>      {
>          if ( cr0 & X86_CR0_PG )
>          {
> -            page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT,
> +            page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3),
>                                       NULL, P2M_ALLOC);
>              if ( !page )
>              {
> @@ -1373,7 +1373,7 @@ static void vmx_load_pdptrs(struct vcpu *v)
>      if ( (cr3 & 0x1fUL) && !hvm_pcid_enabled(v) )
>          goto crash;
>
> -    page = get_page_from_gfn(v->domain, cr3 >> PAGE_SHIFT, &p2mt,
> P2M_UNSHARE);
> +    page = get_page_from_gfn(v->domain, gaddr_to_gfn(cr3), &p2mt,
> P2M_UNSHARE);
>      if ( !page )
>      {
>          /* Ideally you don't want to crash but rather go into a wait
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index dfd08e2d0a..2953d05a17 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -649,11 +649,11 @@ static void nvmx_update_apic_access_address(struct
> vcpu *v)
>      if ( ctrl & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES )
>      {
>          p2m_type_t p2mt;
> -        unsigned long apic_gpfn;
> +        gfn_t apic_gfn;
>          struct page_info *apic_pg;
>
> -        apic_gpfn = get_vvmcs(v, APIC_ACCESS_ADDR) >> PAGE_SHIFT;
> -        apic_pg = get_page_from_gfn(v->domain, apic_gpfn, &p2mt,
> P2M_ALLOC);
> +        apic_gfn = gaddr_to_gfn(get_vvmcs(v, APIC_ACCESS_ADDR));
> +        apic_pg = get_page_from_gfn(v->domain, apic_gfn, &p2mt,
> P2M_ALLOC);
>          ASSERT(apic_pg && !p2m_is_paging(p2mt));
>          __vmwrite(APIC_ACCESS_ADDR, page_to_maddr(apic_pg));
>          put_page(apic_pg);
> @@ -670,11 +670,11 @@ static void nvmx_update_virtual_apic_address(struct
> vcpu *v)
>      if ( ctrl & CPU_BASED_TPR_SHADOW )
>      {
>          p2m_type_t p2mt;
> -        unsigned long vapic_gpfn;
> +        gfn_t vapic_gfn;
>          struct page_info *vapic_pg;
>
> -        vapic_gpfn = get_vvmcs(v, VIRTUAL_APIC_PAGE_ADDR) >> PAGE_SHIFT;
> -        vapic_pg = get_page_from_gfn(v->domain, vapic_gpfn, &p2mt,
> P2M_ALLOC);
> +        vapic_gfn = gaddr_to_gfn(get_vvmcs(v, VIRTUAL_APIC_PAGE_ADDR));
> +        vapic_pg = get_page_from_gfn(v->domain, vapic_gfn, &p2mt,
> P2M_ALLOC);
>          ASSERT(vapic_pg && !p2m_is_paging(p2mt));
>          __vmwrite(VIRTUAL_APIC_PAGE_ADDR, page_to_maddr(vapic_pg));
>          put_page(vapic_pg);
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 9363e9bd96..e3462f8a77 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2052,7 +2052,7 @@ static int mod_l1_entry(l1_pgentry_t *pl1e,
> l1_pgentry_t nl1e,
>              p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
>                              P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
>
> -            page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
> +            page = get_page_from_gfn(pg_dom, _gfn(l1e_get_pfn(nl1e)),
> &p2mt, q);
>
>              if ( p2m_is_paged(p2mt) )
>              {
> @@ -3223,7 +3223,8 @@ long do_mmuext_op(
>              if ( paging_mode_refcounts(pg_owner) )
>                  break;
>
> -            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
> P2M_ALLOC);
> +            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), NULL,
> +                                     P2M_ALLOC);
>              if ( unlikely(!page) )
>              {
>                  rc = -EINVAL;
> @@ -3288,7 +3289,8 @@ long do_mmuext_op(
>              if ( paging_mode_refcounts(pg_owner) )
>                  break;
>
> -            page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL,
> P2M_ALLOC);
> +            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), NULL,
> +                                     P2M_ALLOC);
>              if ( unlikely(!page) )
>              {
>                  gdprintk(XENLOG_WARNING,
> @@ -3504,7 +3506,8 @@ long do_mmuext_op(
>          }
>
>          case MMUEXT_CLEAR_PAGE:
> -            page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt,
> P2M_ALLOC);
> +            page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn), &p2mt,
> +                                     P2M_ALLOC);
>              if ( unlikely(p2mt != p2m_ram_rw) && page )
>              {
>                  put_page(page);
> @@ -3532,7 +3535,7 @@ long do_mmuext_op(
>          {
>              struct page_info *src_page, *dst_page;
>
> -            src_page = get_page_from_gfn(pg_owner, op.arg2.src_mfn, &p2mt,
> +            src_page = get_page_from_gfn(pg_owner, _gfn(op.arg2.src_mfn),
> &p2mt,
>                                           P2M_ALLOC);
>              if ( unlikely(p2mt != p2m_ram_rw) && src_page )
>              {
> @@ -3548,7 +3551,7 @@ long do_mmuext_op(
>                  break;
>              }
>
> -            dst_page = get_page_from_gfn(pg_owner, op.arg1.mfn, &p2mt,
> +            dst_page = get_page_from_gfn(pg_owner, _gfn(op.arg1.mfn),
> &p2mt,
>                                           P2M_ALLOC);
>              if ( unlikely(p2mt != p2m_ram_rw) && dst_page )
>              {
> @@ -3636,7 +3639,8 @@ long do_mmu_update(
>  {
>      struct mmu_update req;
>      void *va = NULL;
> -    unsigned long gpfn, gmfn, mfn;
> +    unsigned long gpfn, mfn;
> +    gfn_t gfn;
>      struct page_info *page;
>      unsigned int cmd, i = 0, done = 0, pt_dom;
>      struct vcpu *curr = current, *v = curr;
> @@ -3749,8 +3753,8 @@ long do_mmu_update(
>              rc = -EINVAL;
>
>              req.ptr -= cmd;
> -            gmfn = req.ptr >> PAGE_SHIFT;
> -            page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
> +            gfn = gaddr_to_gfn(req.ptr);
> +            page = get_page_from_gfn(pt_owner, gfn, &p2mt, P2M_ALLOC);
>
>              if ( unlikely(!page) || p2mt != p2m_ram_rw )
>              {
> @@ -3758,7 +3762,7 @@ long do_mmu_update(
>                      put_page(page);
>                  if ( p2m_is_paged(p2mt) )
>                  {
> -                    p2m_mem_paging_populate(pt_owner, gmfn);
> +                    p2m_mem_paging_populate(pt_owner, gfn_x(gfn));
>                      rc = -ENOENT;
>                  }
>                  else
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index a00a3c1bff..3b2aac8804 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2718,7 +2718,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned
> long fgfn,
>       * Take a refcnt on the mfn. NB: following supported for foreign
> mapping:
>       *     ram_rw | ram_logdirty | ram_ro | paging_out.
>       */
> -    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
> +    page = get_page_from_gfn(fdom, _gfn(fgfn), &p2mt, P2M_ALLOC);
>      if ( !page ||
>           !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) )
>      {
> diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
> index 4cc75916b8..9275ba476c 100644
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -313,15 +313,15 @@ const struct x86_emulate_ops hvm_shadow_emulator_ops
> = {
>  static mfn_t emulate_gva_to_mfn(struct vcpu *v, unsigned long vaddr,
>                                  struct sh_emulate_ctxt *sh_ctxt)
>  {
> -    unsigned long gfn;
> +    gfn_t gfn;
>      struct page_info *page;
>      mfn_t mfn;
>      p2m_type_t p2mt;
>      uint32_t pfec = PFEC_page_present | PFEC_write_access;
>
>      /* Translate the VA to a GFN. */
> -    gfn = paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec);
> -    if ( gfn == gfn_x(INVALID_GFN) )
> +    gfn = _gfn(paging_get_hostmode(v)->gva_to_gfn(v, NULL, vaddr, &pfec));
> +    if ( gfn_eq(gfn, INVALID_GFN) )
>      {
>          x86_emul_pagefault(pfec, vaddr, &sh_ctxt->ctxt);
>
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 3a3c15890b..4f3f438614 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -229,7 +229,8 @@ ret_t do_physdev_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              break;
>
>          ret = -EINVAL;
> -        page = get_page_from_gfn(current->domain, info.gmfn, NULL,
> P2M_ALLOC);
> +        page = get_page_from_gfn(current->domain, _gfn(info.gmfn),
> +                                 NULL, P2M_ALLOC);
>          if ( !page )
>              break;
>          if ( !get_page_type(page, PGT_writable_page) )
> diff --git a/xen/arch/x86/pv/descriptor-tables.c
> b/xen/arch/x86/pv/descriptor-tables.c
> index 8b2d55fc2e..7e8f41d3fd 100644
> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -112,7 +112,7 @@ long pv_set_gdt(struct vcpu *v, unsigned long *frames,
> unsigned int entries)
>      {
>          struct page_info *page;
>
> -        page = get_page_from_gfn(d, frames[i], NULL, P2M_ALLOC);
> +        page = get_page_from_gfn(d, _gfn(frames[i]), NULL, P2M_ALLOC);
>          if ( !page )
>              goto fail;
>          if ( !get_page_type(page, PGT_seg_desc_page) )
> @@ -209,7 +209,6 @@ int compat_set_gdt(XEN_GUEST_HANDLE_PARAM(uint)
> frame_list,
>  long do_update_descriptor(uint64_t pa, uint64_t desc)
>  {
>      struct domain *currd = current->domain;
> -    unsigned long gmfn = pa >> PAGE_SHIFT;
>      unsigned long mfn;
>      unsigned int  offset;
>      struct desc_struct *gdt_pent, d;
> @@ -220,7 +219,7 @@ long do_update_descriptor(uint64_t pa, uint64_t desc)
>
>      *(uint64_t *)&d = desc;
>
> -    page = get_page_from_gfn(currd, gmfn, NULL, P2M_ALLOC);
> +    page = get_page_from_gfn(currd, gaddr_to_gfn(pa), NULL, P2M_ALLOC);
>      if ( (((unsigned int)pa % sizeof(struct desc_struct)) != 0) ||
>           !page ||
>           !check_descriptor(currd, &d) )
> diff --git a/xen/arch/x86/pv/emul-priv-op.c
> b/xen/arch/x86/pv/emul-priv-op.c
> index f73ea4a163..a529ebcc3f 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -760,12 +760,12 @@ static int write_cr(unsigned int reg, unsigned long
> val,
>      case 3: /* Write CR3 */
>      {
>          struct domain *currd = curr->domain;
> -        unsigned long gfn;
> +        gfn_t gfn;
>          struct page_info *page;
>          int rc;
>
> -        gfn = !is_pv_32bit_domain(currd)
> -              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val);
> +        gfn = _gfn(!is_pv_32bit_domain(currd)
> +              ? xen_cr3_to_pfn(val) : compat_cr3_to_pfn(val));
>          page = get_page_from_gfn(currd, gfn, NULL, P2M_ALLOC);
>          if ( !page )
>              break;
> diff --git a/xen/arch/x86/pv/mm.c b/xen/arch/x86/pv/mm.c
> index f5ea00ca4e..c9ad1152b4 100644
> --- a/xen/arch/x86/pv/mm.c
> +++ b/xen/arch/x86/pv/mm.c
> @@ -106,7 +106,7 @@ bool pv_map_ldt_shadow_page(unsigned int offset)
>      if ( unlikely(!(l1e_get_flags(gl1e) & _PAGE_PRESENT)) )
>          return false;
>
> -    page = get_page_from_gfn(currd, l1e_get_pfn(gl1e), NULL, P2M_ALLOC);
> +    page = get_page_from_gfn(currd, _gfn(l1e_get_pfn(gl1e)), NULL,
> P2M_ALLOC);
>      if ( unlikely(!page) )
>          return false;
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9471d89022..d967e49432 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -795,7 +795,7 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
>      case 0: /* Write hypercall page */
>      {
>          void *hypercall_page;
> -        unsigned long gmfn = val >> PAGE_SHIFT;
> +        gfn_t gfn = gaddr_to_gfn(val);
>          unsigned int page_index = val & (PAGE_SIZE - 1);
>          struct page_info *page;
>          p2m_type_t t;
> @@ -808,7 +808,7 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
>              return X86EMUL_EXCEPTION;
>          }
>
> -        page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> +        page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
>
>          if ( !page || !get_page_type(page, PGT_writable_page) )
>          {
> @@ -817,13 +817,14 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx,
> uint64_t val)
>
>              if ( p2m_is_paging(t) )
>              {
> -                p2m_mem_paging_populate(d, gmfn);
> +                p2m_mem_paging_populate(d, gfn_x(gfn));
>                  return X86EMUL_RETRY;
>              }
>
>              gdprintk(XENLOG_WARNING,
> -                     "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
> -                     gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN),
> base);
> +                     "Bad GMFN %#"PRI_gfn" (MFN %#"PRI_mfn") to MSR
> %08x\n",
> +                     gfn_x(gfn), mfn_x(page ? page_to_mfn(page) :
> INVALID_MFN),
> +                     base);
>              return X86EMUL_EXCEPTION;
>          }
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index d6650f0656..5e3c05b96c 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1250,7 +1250,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn,
> unsigned offset)
>      if ( (v != current) && !(v->pause_flags & VPF_down) )
>          return -EINVAL;
>
> -    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    page = get_page_from_gfn(d, _gfn(gfn), NULL, P2M_ALLOC);
>      if ( !page )
>          return -EINVAL;
>
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index c49f446754..71a6f673b2 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -358,7 +358,7 @@ static const struct evtchn_port_ops
> evtchn_port_ops_fifo =
>      .print_state   = evtchn_fifo_print_state,
>  };
>
> -static int map_guest_page(struct domain *d, uint64_t gfn, void **virt)
> +static int map_guest_page(struct domain *d, gfn_t gfn, void **virt)
>  {
>      struct page_info *p;
>
> @@ -419,7 +419,7 @@ static int setup_control_block(struct vcpu *v)
>      return 0;
>  }
>
> -static int map_control_block(struct vcpu *v, uint64_t gfn, uint32_t
> offset)
> +static int map_control_block(struct vcpu *v, gfn_t gfn, uint32_t offset)
>  {
>      void *virt;
>      unsigned int i;
> @@ -505,7 +505,7 @@ int evtchn_fifo_init_control(struct
> evtchn_init_control *init_control)
>  {
>      struct domain *d = current->domain;
>      uint32_t vcpu_id;
> -    uint64_t gfn;
> +    gfn_t gfn;
>      uint32_t offset;
>      struct vcpu *v;
>      int rc;
> @@ -513,7 +513,7 @@ int evtchn_fifo_init_control(struct
> evtchn_init_control *init_control)
>      init_control->link_bits = EVTCHN_FIFO_LINK_BITS;
>
>      vcpu_id = init_control->vcpu;
> -    gfn     = init_control->control_gfn;
> +    gfn     = _gfn(init_control->control_gfn);
>      offset  = init_control->offset;
>
>      if ( vcpu_id >= d->max_vcpus || !d->vcpu[vcpu_id] )
> @@ -569,7 +569,7 @@ int evtchn_fifo_init_control(struct
> evtchn_init_control *init_control)
>      return rc;
>  }
>
> -static int add_page_to_event_array(struct domain *d, unsigned long gfn)
> +static int add_page_to_event_array(struct domain *d, gfn_t gfn)
>  {
>      void *virt;
>      unsigned int slot;
> @@ -619,7 +619,7 @@ int evtchn_fifo_expand_array(const struct
> evtchn_expand_array *expand_array)
>          return -EOPNOTSUPP;
>
>      spin_lock(&d->event_lock);
> -    rc = add_page_to_event_array(d, expand_array->array_gfn);
> +    rc = add_page_to_event_array(d, _gfn(expand_array->array_gfn));
>      spin_unlock(&d->event_lock);
>
>      return rc;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 987395fbb3..e02733dba0 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1365,7 +1365,7 @@ long do_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              return rc;
>          }
>
> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> +        page = get_page_from_gfn(d, _gfn(xrfp.gpfn), NULL, P2M_ALLOC);
>          if ( page )
>          {
>              rc = guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
> @@ -1636,7 +1636,7 @@ int check_get_page_from_gfn(struct domain *d, gfn_t
> gfn, bool readonly,
>      p2m_type_t p2mt;
>      struct page_info *page;
>
> -    page = get_page_from_gfn(d, gfn_x(gfn), &p2mt, q);
> +    page = get_page_from_gfn(d, gfn, &p2mt, q);
>
>  #ifdef CONFIG_HAS_MEM_PAGING
>      if ( p2m_is_paging(p2mt) )
> diff --git a/xen/common/tmem_xen.c b/xen/common/tmem_xen.c
> index bf7b14f79a..72cba7f10c 100644
> --- a/xen/common/tmem_xen.c
> +++ b/xen/common/tmem_xen.c
> @@ -52,7 +52,7 @@ static inline void *cli_get_page(xen_pfn_t cmfn, mfn_t
> *pcli_mfn,
>      p2m_type_t t;
>      struct page_info *page;
>
> -    page = get_page_from_gfn(current->domain, cmfn, &t, P2M_ALLOC);
> +    page = get_page_from_gfn(current->domain, _gfn(cmfn), &t, P2M_ALLOC);
>      if ( !page || t != p2m_ram_rw )
>      {
>          if ( page )
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 7c67806056..5e598a0b37 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -278,7 +278,7 @@ struct page_info *p2m_get_page_from_gfn(struct domain
> *d, gfn_t gfn,
>                                          p2m_type_t *t);
>
>  static inline struct page_info *get_page_from_gfn(
> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      mfn_t mfn;
>      p2m_type_t _t;
> @@ -289,7 +289,7 @@ static inline struct page_info *get_page_from_gfn(
>       * not auto-translated.
>       */
>      if ( unlikely(d != dom_xen) )
> -        return p2m_get_page_from_gfn(d, _gfn(gfn), t);
> +        return p2m_get_page_from_gfn(d, gfn, t);
>
>      if ( !t )
>          t = &_t;
> @@ -300,7 +300,7 @@ static inline struct page_info *get_page_from_gfn(
>       * DOMID_XEN see 1-1 RAM. The p2m_type is based on the type of the
>       * page.
>       */
> -    mfn = _mfn(gfn);
> +    mfn = _mfn(gfn_x(gfn));
>      page = mfn_to_page(mfn);
>
>      if ( !mfn_valid(mfn) || !get_page(page, d) )
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index d08c595887..db1ec37610 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -489,18 +489,21 @@ struct page_info *p2m_get_page_from_gfn(struct
> p2m_domain *p2m, gfn_t gfn,
>                                          p2m_query_t q);
>
>  static inline struct page_info *get_page_from_gfn(
> -    struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q)
> +    struct domain *d, gfn_t gfn, p2m_type_t *t, p2m_query_t q)
>  {
>      struct page_info *page;
> +    mfn_t mfn;
>
>      if ( paging_mode_translate(d) )
> -        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), _gfn(gfn), t,
> NULL, q);
> +        return p2m_get_page_from_gfn(p2m_get_hostp2m(d), gfn, t, NULL, q);
>
>      /* Non-translated guests see 1-1 RAM / MMIO mappings everywhere */
>      if ( t )
>          *t = likely(d != dom_io) ? p2m_ram_rw : p2m_mmio_direct;
> -    page = mfn_to_page(_mfn(gfn));
> -    return mfn_valid(_mfn(gfn)) && get_page(page, d) ? page : NULL;
> +
> +    mfn = _mfn(gfn_x(gfn));
> +    page = mfn_to_page(mfn);
> +    return mfn_valid(mfn) && get_page(page, d) ? page : NULL;
>  }
>
>  /* General conversion function from mfn to gfn */
> --
> 2.11.0
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 39928 bytes --]

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

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall]

On 11/12/18 4:49 PM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> 
> I'm just wondering if this patch really belongs to xentrace series.
> It rather looks like a separate cleanup patch.

What's wrong with including clean-up patch in a series adding a feature?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Andrii Anisov]

> What's wrong with including clean-up patch in a series adding a feature?

I do not mean it is wrong.
Just assuming that introducing a new feature and cleaning up a code
might be different processes with a different review period.

Sincerely,
Andrii Anisov.

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

Re: [PATCH 8/8] xen: Swich parameter in get_page_from_gfn to use typesafe gfn

[Julien Grall]

Hi,

On 11/12/18 4:58 PM, Andrii Anisov wrote:
>> What's wrong with including clean-up patch in a series adding a feature?
> 
> I do not mean it is wrong.
> Just assuming that introducing a new feature and cleaning up a code
> might be different processes with a different review period.

We don't have different process nor different review period between 
clean-up and new feature.

I tend to do clean-up when writing new features... See my cacheflush 
series as well. If the clean-up is small then I will append/prepend to 
the feature series.

This patch modify a function that was called by this patch and therefore 
depends on the rest of the series. This was written with this series so 
it makes sense to me to do it together as it dictates the order I would 
like the patches applied and simplify tracking (I have many series in 
flight).

Cheers,

-- 
Julien Grall

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