[PATCH for-next 0/9] xen/arm: Properly disable M2P on Arm.

[PATCH for-next 0/9] xen/arm: Properly disable M2P on Arm.

[Julien Grall]

Hi all,

Arm never supported an M2P yet there are some helpers implemented to deal with
the common code. However, the implementation of mfn_to_gmfn is completely
bogus.

This series aims to properly disable the M2P on Arm. See patch #8 for the
rationale regarding the lack of M2P on Arm.

While looking at the code, I also continue to convert some code to use
typesafe MFN/GFN.

Cheers,

Julien Grall (9):
  xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible
  xen/x86: Constify the parameter "d" in mfn_to_mfn
  xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn
  xen/grant-table: Make arch specific macros typesafe
  xen: Convert hotplug page function to use typesafe MFN
  xen: Convert is_xen_fixed_mfn to use typesafe MFN
  xen: Convert is_xen_heap_mfn to use typesafe MFN
  xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call
  xen: Remove mfn_to_gmfn macro

 xen/arch/arm/mm.c                   |  2 +-
 xen/arch/x86/Kconfig                |  1 +
 xen/arch/x86/cpu/mcheck/mcaction.c  | 18 ++++++++++--------
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/mm.c                   | 11 ++++++-----
 xen/arch/x86/mm/p2m.c               |  2 +-
 xen/arch/x86/mm/shadow/multi.c      |  2 +-
 xen/arch/x86/pv/emul-priv-op.c      |  4 ++--
 xen/arch/x86/tboot.c                |  2 +-
 xen/common/Kconfig                  |  3 +++
 xen/common/domctl.c                 |  2 +-
 xen/common/grant_table.c            |  4 ++--
 xen/common/memory.c                 |  4 ++++
 xen/common/page_alloc.c             | 26 +++++++++++++-------------
 xen/common/sysctl.c                 | 14 +++++++-------
 xen/drivers/passthrough/iommu.c     | 12 ++++++++----
 xen/drivers/passthrough/x86/iommu.c |  6 +++---
 xen/include/asm-arm/domain.h        |  5 +++++
 xen/include/asm-arm/grant_table.h   | 12 ++++++------
 xen/include/asm-arm/mm.h            | 22 ++++++++++------------
 xen/include/asm-x86/grant_table.h   | 20 ++++++++------------
 xen/include/asm-x86/mm.h            | 11 +++--------
 xen/include/asm-x86/p2m.h           |  2 +-
 xen/include/xen/domain.h            |  9 +++++++++
 xen/include/xen/mm.h                |  6 +++---
 25 files changed, 110 insertions(+), 92 deletions(-)

-- 
2.11.0


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

[PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
the Arm code to use the former.

No functional changes.

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

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cccc0..be5338bb4c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -886,7 +886,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     int i;
 #endif
 
-    frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
+    frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
     /* Round up to 2M or 32M boundary, as appropriate. */
     frametable_size = ROUNDUP(frametable_size, mapping_size);
     base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index eafa26f56e..7b6aaf5e3f 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -225,7 +225,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 /* Convert between frame number and address formats.  */
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
-#define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
+#define paddr_to_pdx(pa)    mfn_to_pdx(maddr_to_mfn(pa))
 #define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
 #define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
 #define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
@@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
 #else
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
+    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
                     mfn_to_maddr(xenheap_mfn_start) +
                     ((ma & ma_va_bottom_mask) |
@@ -301,7 +301,7 @@ static inline struct page_info *virt_to_page(const void *v)
     ASSERT(va < xenheap_virt_end);
 
     pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
-    pdx += pfn_to_pdx(mfn_x(xenheap_mfn_start));
+    pdx += mfn_to_pdx(xenheap_mfn_start);
     return frame_table + pdx - frametable_base_pdx;
 }
 
-- 
2.11.0


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

[PATCH for-next 2/9] xen/x86: Constify the parameter "d" in mfn_to_mfn

[Julien Grall]

The parameter "d" holds the domain and is not modified by the function.
So constify it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-x86/p2m.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2095076556..b6852dc133 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -506,7 +506,7 @@ static inline struct page_info *get_page_from_gfn(
 }
 
 /* General conversion function from mfn to gfn */
-static inline unsigned long mfn_to_gfn(struct domain *d, mfn_t mfn)
+static inline unsigned long mfn_to_gfn(const struct domain *d, mfn_t mfn)
 {
     if ( paging_mode_translate(d) )
         return get_gpfn_from_mfn(mfn_x(mfn));
-- 
2.11.0


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

[PATCH for-next 3/9] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

[Julien Grall]

mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
is using mfn_t.

Furthermore, the naming of the former is more consistent with the
current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
mfn_to_gfn in x86 code.

No functional changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/domain.c               | 2 +-
 xen/arch/x86/mm.c                   | 2 +-
 xen/arch/x86/pv/emul-priv-op.c      | 4 ++--
 xen/drivers/passthrough/x86/iommu.c | 6 +++---
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 32dc4253ff..ab1f25f49d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -714,7 +714,7 @@ int arch_domain_soft_reset(struct domain *d)
     ASSERT( owner == d );
 
     mfn = page_to_mfn(page);
-    gfn = mfn_to_gmfn(d, mfn_x(mfn));
+    gfn = mfn_to_gfn(d, mfn);
 
     /*
      * gfn == INVALID_GFN indicates that the shared_info page was never mapped
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7ec5954b03..df6e5bdd31 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2518,7 +2518,7 @@ int free_page_type(struct page_info *page, unsigned long type,
 
         ASSERT(!shadow_mode_refcounts(owner));
 
-        gmfn = mfn_to_gmfn(owner, mfn_x(page_to_mfn(page)));
+        gmfn = mfn_to_gfn(owner, page_to_mfn(page));
         if ( VALID_M2P(gmfn) )
             shadow_remove_all_shadows(owner, _mfn(gmfn));
     }
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 942ece2ca0..0da1e29782 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -711,7 +711,7 @@ static int read_cr(unsigned int reg, unsigned long *val,
         if ( !is_pv_32bit_domain(currd) )
         {
             mfn = pagetable_get_mfn(curr->arch.guest_table);
-            *val = xen_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn)));
+            *val = xen_pfn_to_cr3(mfn_to_gfn(currd, mfn));
         }
         else
         {
@@ -720,7 +720,7 @@ static int read_cr(unsigned int reg, unsigned long *val,
 
             mfn = l4e_get_mfn(*pl4e);
             unmap_domain_page(pl4e);
-            *val = compat_pfn_to_cr3(mfn_to_gmfn(currd, mfn_x(mfn)));
+            *val = compat_pfn_to_cr3(mfn_to_gfn(currd, mfn));
         }
         /* PTs should not be shared */
         BUG_ON(page_get_owner(mfn_to_page(mfn)) == dom_cow);
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index a88ef9b189..2b1915548a 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -59,15 +59,15 @@ int arch_iommu_populate_page_table(struct domain *d)
         if ( is_hvm_domain(d) ||
             (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
         {
-            unsigned long mfn = mfn_x(page_to_mfn(page));
-            unsigned long gfn = mfn_to_gmfn(d, mfn);
+            mfn_t mfn = page_to_mfn(page);
+            unsigned long gfn = mfn_to_gfn(d, mfn);
             unsigned int flush_flags = 0;
 
             if ( gfn != gfn_x(INVALID_GFN) )
             {
                 ASSERT(!(gfn >> DEFAULT_DOMAIN_ADDRESS_WIDTH));
                 BUG_ON(SHARED_M2P(gfn));
-                rc = iommu_map(d, _dfn(gfn), _mfn(mfn), PAGE_ORDER_4K,
+                rc = iommu_map(d, _dfn(gfn), mfn, PAGE_ORDER_4K,
                                IOMMUF_readable | IOMMUF_writable,
                                &flush_flags);
             }
-- 
2.11.0


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

[PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Julien Grall]

No functional changes intended.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/common/grant_table.c          |  4 ++--
 xen/include/asm-arm/grant_table.h | 12 ++++++------
 xen/include/asm-x86/grant_table.h | 20 ++++++++------------
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fd099a8f25..e7a65b38e0 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1935,7 +1935,7 @@ gnttab_setup_table(
     op.status = GNTST_okay;
     for ( i = 0; i < op.nr_frames; i++ )
     {
-        xen_pfn_t gmfn = gnttab_shared_gmfn(d, gt, i);
+        xen_pfn_t gmfn = gfn_x(gnttab_shared_gfn(d, gt, i));
 
         /* Grant tables cannot be shared */
         BUG_ON(SHARED_M2P(gmfn));
@@ -3147,7 +3147,7 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
 
     for ( i = 0; i < op.nr_frames; i++ )
     {
-        gmfn = gnttab_status_gmfn(d, gt, i);
+        gmfn = gfn_x(gnttab_status_gfn(d, gt, i));
         if ( copy_to_guest_offset(op.frame_list, i, &gmfn, 1) )
             op.status = GNTST_bad_virt_addr;
     }
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 816e3c6d68..4c44b720f2 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -67,15 +67,15 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
     } while ( 0 )
 
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
-   _gfn((st) ? gnttab_status_gmfn(NULL, gt, idx)                         \
-             : gnttab_shared_gmfn(NULL, gt, idx));                       \
+   (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
+             : gnttab_shared_gfn(NULL, gt, idx);                         \
 })
 
-#define gnttab_shared_gmfn(d, t, i)                                      \
-    gfn_x(((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
+#define gnttab_shared_gfn(d, t, i)                                       \
+    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
 
-#define gnttab_status_gmfn(d, t, i)                                      \
-    gfn_x(((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
+#define gnttab_status_gfn(d, t, i)                                       \
+    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
 
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index 4b8c4f9160..8736d7286d 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -43,24 +43,20 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
 #define gnttab_destroy_arch(gt) do {} while ( 0 )
 #define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
-    unsigned long mfn_ = (st) ? gnttab_status_mfn(gt, idx)               \
-                              : gnttab_shared_mfn(gt, idx);              \
-    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_);                       \
+    mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
+                      : gnttab_shared_mfn(gt, idx);                      \
+    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_));                \
     VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
 })
 
-#define gnttab_shared_mfn(t, i)                         \
-    ((virt_to_maddr((t)->shared_raw[i]) >> PAGE_SHIFT))
+#define gnttab_shared_mfn(t, i) _mfn(__virt_to_mfn((t)->shared_raw[i]))
 
-#define gnttab_shared_gmfn(d, t, i)                     \
-    (mfn_to_gmfn(d, gnttab_shared_mfn(t, i)))
+#define gnttab_shared_gfn(d, t, i) _gfn(mfn_to_gfn(d, gnttab_shared_mfn(t, i)))
 
+#define gnttab_status_mfn(t, i) _mfn(__virt_to_mfn((t)->status[i]))
 
-#define gnttab_status_mfn(t, i)                         \
-    ((virt_to_maddr((t)->status[i]) >> PAGE_SHIFT))
-
-#define gnttab_status_gmfn(d, t, i)                     \
-    (mfn_to_gmfn(d, gnttab_status_mfn(t, i)))
+#define gnttab_status_gfn(d, t, i) \
+    _gfn(mfn_to_gfn(d, gnttab_status_mfn(t, i)))
 
 #define gnttab_mark_dirty(d, f) paging_mark_dirty((d), f)
 
-- 
2.11.0


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

[PATCH for-next 5/9] xen: Convert hotplug page function to use typesafe MFN

[Julien Grall]

Convert online_page, offline_page and query_page_offline to use
typesafe MFN.

No functional changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/cpu/mcheck/mcaction.c | 18 ++++++++++--------
 xen/common/page_alloc.c            | 24 ++++++++++++------------
 xen/common/sysctl.c                | 14 +++++++-------
 xen/include/xen/mm.h               |  6 +++---
 4 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mcaction.c b/xen/arch/x86/cpu/mcheck/mcaction.c
index e42267414e..69332fb84d 100644
--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -6,7 +6,7 @@
 
 static struct mcinfo_recovery *
 mci_action_add_pageoffline(int bank, struct mc_info *mi,
-                           uint64_t mfn, uint32_t status)
+                           mfn_t mfn, uint32_t status)
 {
     struct mcinfo_recovery *rec;
 
@@ -22,7 +22,7 @@ mci_action_add_pageoffline(int bank, struct mc_info *mi,
 
     rec->mc_bank = bank;
     rec->action_types = MC_ACTION_PAGE_OFFLINE;
-    rec->action_info.page_retire.mfn = mfn;
+    rec->action_info.page_retire.mfn = mfn_x(mfn);
     rec->action_info.page_retire.status = status;
     return rec;
 }
@@ -42,7 +42,8 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
     struct mcinfo_bank *bank = binfo->mib;
     struct mcinfo_global *global = binfo->mig;
     struct domain *d;
-    unsigned long mfn, gfn;
+    mfn_t mfn;
+    unsigned long gfn;
     uint32_t status;
     int vmce_vcpuid;
     unsigned int mc_vcpuid;
@@ -54,11 +55,12 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
         return;
     }
 
-    mfn = bank->mc_addr >> PAGE_SHIFT;
+    mfn = maddr_to_mfn(bank->mc_addr);
     if ( offline_page(mfn, 1, &status) )
     {
         dprintk(XENLOG_WARNING,
-                "Failed to offline page %lx for MCE error\n", mfn);
+                "Failed to offline page %"PRI_mfn" for MCE error\n",
+                mfn_x(mfn));
         return;
     }
 
@@ -89,10 +91,10 @@ mc_memerr_dhandler(struct mca_binfo *binfo,
                 ASSERT(d);
                 gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
 
-                if ( unmmap_broken_page(d, _mfn(mfn), gfn) )
+                if ( unmmap_broken_page(d, mfn, gfn) )
                 {
-                    printk("Unmap broken memory %lx for DOM%d failed\n",
-                           mfn, d->domain_id);
+                    printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
+                           mfn_x(mfn), d->domain_id);
                     goto vmce_failed;
                 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index f71d3bb7a1..5684a13557 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg)
 
 }
 
-int offline_page(unsigned long mfn, int broken, uint32_t *status)
+int offline_page(mfn_t mfn, int broken, uint32_t *status)
 {
     unsigned long old_info = 0;
     struct domain *owner;
     struct page_info *pg;
 
-    if ( !mfn_valid(_mfn(mfn)) )
+    if ( !mfn_valid(mfn) )
     {
         dprintk(XENLOG_WARNING,
-                "try to offline page out of range %lx\n", mfn);
+                "try to offline page out of range %"PRI_mfn"\n", mfn_x(mfn));
         return -EINVAL;
     }
 
     *status = 0;
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
-    if ( is_xen_fixed_mfn(mfn) )
+    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
     {
         *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
           (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
@@ -1595,7 +1595,7 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status)
      * N.B. xen's txt in x86_64 is marked reserved and handled already.
      * Also kexec range is reserved.
      */
-    if ( !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
+    if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
     {
         *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM;
         return -EINVAL;
@@ -1677,19 +1677,19 @@ int offline_page(unsigned long mfn, int broken, uint32_t *status)
  *   The caller should make sure end_pfn <= max_page,
  *   if not, expand_pages() should be called prior to online_page().
  */
-unsigned int online_page(unsigned long mfn, uint32_t *status)
+unsigned int online_page(mfn_t mfn, uint32_t *status)
 {
     unsigned long x, nx, y;
     struct page_info *pg;
     int ret;
 
-    if ( !mfn_valid(_mfn(mfn)) )
+    if ( !mfn_valid(mfn) )
     {
         dprintk(XENLOG_WARNING, "call expand_pages() first\n");
         return -EINVAL;
     }
 
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
     spin_lock(&heap_lock);
 
@@ -1730,11 +1730,11 @@ unsigned int online_page(unsigned long mfn, uint32_t *status)
     return ret;
 }
 
-int query_page_offline(unsigned long mfn, uint32_t *status)
+int query_page_offline(mfn_t mfn, uint32_t *status)
 {
     struct page_info *pg;
 
-    if ( !mfn_valid(_mfn(mfn)) || !page_is_ram_type(mfn, RAM_TYPE_CONVENTIONAL) )
+    if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
     {
         dprintk(XENLOG_WARNING, "call expand_pages() first\n");
         return -EINVAL;
@@ -1743,7 +1743,7 @@ int query_page_offline(unsigned long mfn, uint32_t *status)
     *status = 0;
     spin_lock(&heap_lock);
 
-    pg = mfn_to_page(_mfn(mfn));
+    pg = mfn_to_page(mfn);
 
     if ( page_state_is(pg, offlining) )
         *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING;
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c0aa6bde4e..ab161793e5 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -186,7 +186,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
     case XEN_SYSCTL_page_offline_op:
     {
         uint32_t *status, *ptr;
-        unsigned long pfn;
+        mfn_t mfn;
 
         ret = xsm_page_offline(XSM_HOOK, op->u.page_offline.cmd);
         if ( ret )
@@ -205,21 +205,21 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         memset(status, PG_OFFLINE_INVALID, sizeof(uint32_t) *
                       (op->u.page_offline.end - op->u.page_offline.start + 1));
 
-        for ( pfn = op->u.page_offline.start;
-              pfn <= op->u.page_offline.end;
-              pfn ++ )
+        for ( mfn = _mfn(op->u.page_offline.start);
+              mfn_x(mfn) <= op->u.page_offline.end;
+              mfn = mfn_add(mfn, 1) )
         {
             switch ( op->u.page_offline.cmd )
             {
                 /* Shall revert her if failed, or leave caller do it? */
                 case sysctl_page_offline:
-                    ret = offline_page(pfn, 0, ptr++);
+                    ret = offline_page(mfn, 0, ptr++);
                     break;
                 case sysctl_page_online:
-                    ret = online_page(pfn, ptr++);
+                    ret = online_page(mfn, ptr++);
                     break;
                 case sysctl_query_page_offline:
-                    ret = query_page_offline(pfn, ptr++);
+                    ret = query_page_offline(mfn, ptr++);
                     break;
                 default:
                     ret = -EINVAL;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index e971147234..3ba7168cc9 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -206,9 +206,9 @@ unsigned long avail_domheap_pages(void);
 unsigned long avail_node_heap_pages(unsigned int);
 #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
 #define free_domheap_page(p)  (free_domheap_pages(p,0))
-unsigned int online_page(unsigned long mfn, uint32_t *status);
-int offline_page(unsigned long mfn, int broken, uint32_t *status);
-int query_page_offline(unsigned long mfn, uint32_t *status);
+unsigned int online_page(mfn_t mfn, uint32_t *status);
+int offline_page(mfn_t mfn, int broken, uint32_t *status);
+int query_page_offline(mfn_t mfn, uint32_t *status);
 unsigned long total_free_pages(void);
 
 void heap_init_late(void);
-- 
2.11.0


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

[PATCH for-next 6/9] xen: Convert is_xen_fixed_mfn to use typesafe MFN

[Julien Grall]

No functional changes.

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

diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index f3fdee4d39..30d159cc62 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -280,7 +280,7 @@ static void tboot_gen_xenheap_integrity(const uint8_t key[TB_KEY_SIZE],
 
         if ( !mfn_valid(_mfn(mfn)) )
             continue;
-        if ( is_xen_fixed_mfn(mfn) )
+        if ( is_xen_fixed_mfn(_mfn(mfn)) )
             continue; /* skip Xen */
         if ( (mfn >= PFN_DOWN(g_tboot_shared->tboot_base - 3 * PAGE_SIZE))
              && (mfn < PFN_UP(g_tboot_shared->tboot_base
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5684a13557..5de3686d85 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1584,7 +1584,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
     *status = 0;
     pg = mfn_to_page(mfn);
 
-    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
+    if ( is_xen_fixed_mfn(mfn) )
     {
         *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
           (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 7b6aaf5e3f..b56018aace 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
 #endif
 
 #define is_xen_fixed_mfn(mfn)                                   \
-    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
-     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
+    ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
+     (mfn_to_maddr(mfn) <= virt_to_maddr(&_end)))
 
 #define page_get_owner(_p)    (_p)->v.inuse.domain
 #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6faa563167..f124f57964 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -280,8 +280,8 @@ struct page_info
 #define is_xen_heap_mfn(mfn) \
     (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
 #define is_xen_fixed_mfn(mfn)                     \
-    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
-     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
+    (((mfn_to_maddr(mfn)) >= __pa(&_stext)) &&    \
+     ((mfn_to_maddr(mfn)) <= __pa(&__2M_rwdata_end)))
 
 #define PRtype_info "016lx"/* should only be used for printk's */
 
-- 
2.11.0


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

[PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Julien Grall]

No functional changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/mm.c              | 9 +++++----
 xen/arch/x86/mm/p2m.c          | 2 +-
 xen/arch/x86/mm/shadow/multi.c | 2 +-
 xen/common/page_alloc.c        | 4 ++--
 xen/include/asm-arm/mm.h       | 8 ++++----
 xen/include/asm-x86/mm.h       | 2 +-
 6 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index df6e5bdd31..a1cd2fb421 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4300,7 +4300,8 @@ int xenmem_add_to_physmap_one(
 {
     struct page_info *page = NULL;
     unsigned long gfn = 0; /* gcc ... */
-    unsigned long prev_mfn, old_gpfn;
+    mfn_t prev_mfn;
+    unsigned long old_gpfn;
     int rc = 0;
     mfn_t mfn = INVALID_MFN;
     p2m_type_t p2mt;
@@ -4349,12 +4350,12 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Remove previously mapped page if it was present. */
-    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
-    if ( mfn_valid(_mfn(prev_mfn)) )
+    prev_mfn = get_gfn(d, gfn_x(gpfn), &p2mt);
+    if ( mfn_valid(prev_mfn) )
     {
         if ( is_xen_heap_mfn(prev_mfn) )
             /* Xen heap frames are simply unhooked from this phys slot. */
-            rc = guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
+            rc = guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
             rc = guest_remove_page(d, gfn_x(gpfn));
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d14ce57dd5..40fed111b9 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2910,7 +2910,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     prev_mfn = get_gfn(tdom, gpfn, &p2mt_prev);
     if ( mfn_valid(prev_mfn) )
     {
-        if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )
+        if ( is_xen_heap_mfn(prev_mfn) )
             /* Xen heap frames are simply unhooked from this phys slot */
             rc = guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
         else
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 7e9cbc69be..2d25e356d3 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -602,7 +602,7 @@ _sh_propagate(struct vcpu *v,
      * caching attributes in the shadows to match what was asked for.
      */
     if ( (level == 1) && is_hvm_domain(d) &&
-         !is_xen_heap_mfn(mfn_x(target_mfn)) )
+         !is_xen_heap_mfn(target_mfn) )
     {
         int type;
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5de3686d85..e43c98d5cb 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
      * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
      * prevent merging of power-of-two blocks across the zone boundary.
      */
-    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
+    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
         ps += PAGE_SIZE;
-    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
+    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
         pe -= PAGE_SIZE;
 
     memguard_guard_range(maddr_to_virt(ps), pe - ps);
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index b56018aace..a9c8352b94 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -138,16 +138,16 @@ extern vaddr_t xenheap_virt_start;
 #endif
 
 #ifdef CONFIG_ARM_32
-#define is_xen_heap_page(page) is_xen_heap_mfn(mfn_x(page_to_mfn(page)))
+#define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
 #define is_xen_heap_mfn(mfn) ({                                 \
-    unsigned long mfn_ = (mfn);                                 \
+    unsigned long mfn_ = mfn_x(mfn);                            \
     (mfn_ >= mfn_x(xenheap_mfn_start) &&                        \
      mfn_ < mfn_x(xenheap_mfn_end));                            \
 })
 #else
 #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
 #define is_xen_heap_mfn(mfn) \
-    (mfn_valid(_mfn(mfn)) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
+    (mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(mfn)))
 #endif
 
 #define is_xen_fixed_mfn(mfn)                                   \
@@ -246,7 +246,7 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
 #ifdef CONFIG_ARM_32
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT(is_xen_heap_mfn(ma >> PAGE_SHIFT));
+    ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
     ma -= mfn_to_maddr(xenheap_mfn_start);
     return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
 }
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index f124f57964..1ca4154382 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -278,7 +278,7 @@ struct page_info
 
 #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
 #define is_xen_heap_mfn(mfn) \
-    (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
+    (__mfn_valid(mfn_x(mfn)) && is_xen_heap_page(mfn_to_page(mfn)))
 #define is_xen_fixed_mfn(mfn)                     \
     (((mfn_to_maddr(mfn)) >= __pa(&_stext)) &&    \
      ((mfn_to_maddr(mfn)) <= __pa(&__2M_rwdata_end)))
-- 
2.11.0


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

[PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty
bogus as we directly return the MFN passed in parameter.

Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There
are only 3 callers:
    - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU
    page-tables when the P2M is not shared with the IOMMU. No issues so
    far as Arm does not yet support non-shared P2M case.
    - memory_exchange: Arm cannot not use it because steal_page is not
    implemented.
    - getdomaininfo: Toolstack may map the shared page. It looks like
    this is mostly used for mapping the P2M of PV guest. Therefore the
    issue might be minor.

Implementing the M2P on Arm is not planned. The M2P would require significant
amount of VA address (very tough on 32-bit) that can hardly be justified with
the current use of mfn_to_gmfn.
    - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the
    IOMMU page-tables is delayed until the first device is assigned.
    In the embedded case, we will known in most of the times what
    devices are assigned during the domain creation. So it is possible
    to take to enable the IOMMU from start. See [1] for the patch.
    - memory_exchange: This does not work and I haven't seen any
    request for it so far.
    - getdomaininfo: The structure on Arm does not seem to contain a lot
    of useful information on Arm. It is unclear whether we want to
    allow the toolstack mapping it on Arm.

This patch introduces a config option HAS_M2P to tell whether an
architecture implements the M2P.
    - iommu_hwdom_init: For now, we require the M2P support when the IOMMU
    is not sharing the P2M.
    - memory_exchange: The hypercall is marked as not supported when the
    M2P does not exist.
    - getdomaininfo: A new helper is introduced to wrap the call to
    mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping
    will fail.

[1] https://patchwork.kernel.org/patch/9719913/

Signed-off-by Julien Grall <julien.grall@arm.com>

---

Cc: oleksandr_tyshchenko@epam.com
Cc: andrii_anisov@epam.com
---
 xen/arch/x86/Kconfig            | 1 +
 xen/common/Kconfig              | 3 +++
 xen/common/domctl.c             | 2 +-
 xen/common/memory.c             | 4 ++++
 xen/drivers/passthrough/iommu.c | 6 +++++-
 xen/include/asm-arm/domain.h    | 5 +++++
 xen/include/xen/domain.h        | 9 +++++++++
 7 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 5c2d1070b6..1892bc3895 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -16,6 +16,7 @@ config X86
 	select HAS_IOPORTS
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
+	select HAS_M2P
 	select HAS_MEM_PAGING
 	select HAS_MEM_SHARING
 	select HAS_NS16550
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 04384628bb..65c0282e90 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -52,6 +52,9 @@ config HAS_GDBSX
 config HAS_IOPORTS
 	bool
 
+config HAS_M2P
+	bool
+
 config NEEDS_LIBELF
 	bool
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index d08b6274e2..ce157e11fe 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
-    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
+    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
     BUG_ON(SHARED_M2P(info->shared_info_frame));
 
     info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index b6cf09585c..6cbbe4c3c8 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -510,6 +510,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
     return true;
 }
 
+#ifdef CONFIG_M2P
 static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 {
     struct xen_memory_exchange exch;
@@ -802,6 +803,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
         rc = -EFAULT;
     return rc;
 }
+#endif
 
 int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
                           unsigned int start)
@@ -1233,12 +1235,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         break;
 
+#ifdef CONFIG_M2P
     case XENMEM_exchange:
         if ( unlikely(start_extent) )
             return -EINVAL;
 
         rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
         break;
+#endif
 
     case XENMEM_maximum_ram_page:
         if ( unlikely(start_extent) )
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5ecaa10bb4..5742cd05b8 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -186,9 +186,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
     hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
     if ( need_iommu_pt_sync(d) )
     {
+        int rc = 0;
+#ifdef CONFIG_HAS_M2P
         struct page_info *page;
         unsigned int i = 0, flush_flags = 0;
-        int rc = 0;
 
         page_list_for_each ( page, &d->page_list )
         {
@@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
         /* Use while-break to avoid compiler warning */
         while ( iommu_iotlb_flush_all(d, flush_flags) )
             break;
+#else
+        rc = -ENOSYS;
+#endif
 
         if ( rc )
             printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 312fec8932..d61b0188da 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
 
 static inline void arch_vcpu_block(struct vcpu *v) {}
 
+static inline gfn_t domain_shared_info_gfn(struct domain *d)
+{
+    return INVALID_GFN;
+}
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index d1bfc82f57..00d8b09794 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -118,4 +118,13 @@ struct vnuma_info {
 
 void vnuma_destroy(struct vnuma_info *vnuma);
 
+#ifdef CONFIG_HAS_M2P
+#define domain_shared_info_gfn(d_) ({                           \
+    struct domain *d = (d_);                                    \
+                                                                \
+    _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))));   \
+})
+
+#endif
+
 #endif /* __XEN_DOMAIN_H__ */
-- 
2.11.0


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

[PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
no more call to mfn_to_gmfn, so the helper can be dropped.

At the same time rework a comment in Arm code that does not make sense.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/drivers/passthrough/iommu.c | 6 +++---
 xen/include/asm-arm/mm.h        | 4 +---
 xen/include/asm-x86/mm.h        | 5 -----
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5742cd05b8..04ac46239e 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -193,8 +193,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
 
         page_list_for_each ( page, &d->page_list )
         {
-            unsigned long mfn = mfn_x(page_to_mfn(page));
-            unsigned long dfn = mfn_to_gmfn(d, mfn);
+            mfn_t mfn = page_to_mfn(page);
+            unsigned long dfn = mfn_to_gfn(d, mfn);
             unsigned int mapping = IOMMUF_readable;
             int ret;
 
@@ -203,7 +203,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
                   == PGT_writable_page) )
                 mapping |= IOMMUF_writable;
 
-            ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
+            ret = iommu_map(d, _dfn(dfn), mfn, 0, mapping,
                             &flush_flags);
 
             if ( !rc )
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index a9c8352b94..a9cb98a6c7 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
 #define SHARED_M2P_ENTRY         (~0UL - 1UL)
 #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
 
-/* Xen always owns P2M on ARM */
+/* We don't have a M2P on Arm */
 #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-#define mfn_to_gmfn(_d, mfn)  (mfn)
-
 
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 1ca4154382..3324dd7abf 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -506,11 +506,6 @@ extern struct rangeset *mmio_ro_ranges;
 
 #define get_gpfn_from_mfn(mfn)      (machine_to_phys_mapping[(mfn)])
 
-#define mfn_to_gmfn(_d, mfn)                            \
-    ( (paging_mode_translate(_d))                       \
-      ? get_gpfn_from_mfn(mfn)                          \
-      : (mfn) )
-
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20))
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
 
-- 
2.11.0


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

Re: [PATCH for-next 2/9] xen/x86: Constify the parameter "d" in mfn_to_mfn

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> The parameter "d" holds the domain and is not modified by the function.
> So constify it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

With the subject suitably corrected to name the actual function
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH for-next 3/9] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
> is using mfn_t.
> 
> Furthermore, the naming of the former is more consistent with the
> current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
> mfn_to_gfn in x86 code.
> 
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Fundamentally I'm fine with this change, but before making its use
more wide-spread, wouldn't it be better to make mfn_to_gfn()
fully type-safe, i.e. have it also return gfn_t? There aren't that
many uses of the function just yet, and doing the conversion now
would save us from having to touch all places you now change
yet another time.

Jan



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

Re: [PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -67,15 +67,15 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>      } while ( 0 )
>  
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -   _gfn((st) ? gnttab_status_gmfn(NULL, gt, idx)                         \
> -             : gnttab_shared_gmfn(NULL, gt, idx));                       \
> +   (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> +             : gnttab_shared_gfn(NULL, gt, idx);                         \

Please also adjust indentation of the latter line.

> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -43,24 +43,20 @@ static inline int replace_grant_host_mapping(uint64_t 
> addr, mfn_t frame,
>  #define gnttab_destroy_arch(gt) do {} while ( 0 )
>  #define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -    unsigned long mfn_ = (st) ? gnttab_status_mfn(gt, idx)               \
> -                              : gnttab_shared_mfn(gt, idx);              \
> -    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_);                       \
> +    mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
> +                      : gnttab_shared_mfn(gt, idx);                      \
> +    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_));                \
>      VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
>  })
>  
> -#define gnttab_shared_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->shared_raw[i]) >> PAGE_SHIFT))
> +#define gnttab_shared_mfn(t, i) _mfn(__virt_to_mfn((t)->shared_raw[i]))
>  
> -#define gnttab_shared_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_shared_mfn(t, i)))
> +#define gnttab_shared_gfn(d, t, i) _gfn(mfn_to_gfn(d, gnttab_shared_mfn(t, i)))

This and ...

> +#define gnttab_status_mfn(t, i) _mfn(__virt_to_mfn((t)->status[i]))
>  
> -#define gnttab_status_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->status[i]) >> PAGE_SHIFT))
> -
> -#define gnttab_status_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_status_mfn(t, i)))
> +#define gnttab_status_gfn(d, t, i) \
> +    _gfn(mfn_to_gfn(d, gnttab_status_mfn(t, i)))

... this would already benefit from mfn_to_gfn() returning gfn_t.
Either way non-Arm parts
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH for-next 5/9] xen: Convert hotplug page function to use typesafe MFN

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> Convert online_page, offline_page and query_page_offline to use
> typesafe MFN.
> 
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
But ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg)
>  
>  }
>  
> -int offline_page(unsigned long mfn, int broken, uint32_t *status)
> +int offline_page(mfn_t mfn, int broken, uint32_t *status)
>  {
>      unsigned long old_info = 0;
>      struct domain *owner;
>      struct page_info *pg;
>  
> -    if ( !mfn_valid(_mfn(mfn)) )
> +    if ( !mfn_valid(mfn) )
>      {
>          dprintk(XENLOG_WARNING,
> -                "try to offline page out of range %lx\n", mfn);
> +                "try to offline page out of range %"PRI_mfn"\n", mfn_x(mfn));

... would you mind adjusting the wording here as well:
"attempt to offline out of range page %"PRI_mfn"\n" or
some such?

Of course the usefulness of the message as a whole is
questionable, so I'd also be happy to see it deleted altogether.

Jan



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

Re: [PATCH for-next 6/9] xen: Convert is_xen_fixed_mfn to use typesafe MFN

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4300,7 +4300,8 @@ int xenmem_add_to_physmap_one(
>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> -    unsigned long prev_mfn, old_gpfn;
> +    mfn_t prev_mfn;
> +    unsigned long old_gpfn;

Please can you put this together with "gfn"?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>       * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
>       * prevent merging of power-of-two blocks across the zone boundary.
>       */
> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )

Please add the missing blanks around - at the same time.

>          ps += PAGE_SIZE;
> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )

Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
we don't have any mfn_sub(), I see.

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -278,7 +278,7 @@ struct page_info
>  
>  #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
>  #define is_xen_heap_mfn(mfn) \
> -    (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> +    (__mfn_valid(mfn_x(mfn)) && is_xen_heap_page(mfn_to_page(mfn)))

Please don't open code mfn_valid().

With these minor issues taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH for-next 3/9] xen/x86: Use mfn_to_gfn rather than mfn_to_gmfn

[Julien Grall]

Hi Jan,

On 13/03/2019 14:45, Jan Beulich wrote:
>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>> mfn_to_gfn and mfn_to_gmfn are doing exactly the same except the former
>> is using mfn_t.
>>
>> Furthermore, the naming of the former is more consistent with the
>> current naming scheme (GFN/MFN). So use replace mfn_to_gmfn with
>> mfn_to_gfn in x86 code.
>>
>> No functional changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Fundamentally I'm fine with this change, but before making its use
> more wide-spread, wouldn't it be better to make mfn_to_gfn()
> fully type-safe, i.e. have it also return gfn_t? There aren't that
> many uses of the function just yet, and doing the conversion now
> would save us from having to touch all places you now change
> yet another time.

I vaguely recall some problems when trying to use typesafe GFN. Maybe it is 
because I was trying to cleanup the code at the same time.

Let me have another try.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Jan Beulich]

>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>      info->outstanding_pages = d->outstanding_pages;
>      info->shr_pages         = atomic_read(&d->shr_pages);
>      info->paged_pages       = atomic_read(&d->paged_pages);
> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));

I think this change wants to be accompanied by a warning attached
to the field declaration in the public header.

But I'd also like to have the tool stack maintainers' view on making
this field effectively unusable for Arm.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -510,6 +510,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>      return true;
>  }
>  
> +#ifdef CONFIG_M2P
>  static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  {
>      struct xen_memory_exchange exch;
> @@ -802,6 +803,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>          rc = -EFAULT;
>      return rc;
>  }
> +#endif

Please can you move the #ifdef inside the function body, add #else
to return -EOPNOTSUPP, and ...

> @@ -1233,12 +1235,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +#ifdef CONFIG_M2P
>      case XENMEM_exchange:
>          if ( unlikely(start_extent) )
>              return -EINVAL;
>  
>          rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
>          break;
> +#endif

... avoid the extra #ifdef-ary here altogether?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -186,9 +186,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>      if ( need_iommu_pt_sync(d) )
>      {
> +        int rc = 0;
> +#ifdef CONFIG_HAS_M2P
>          struct page_info *page;
>          unsigned int i = 0, flush_flags = 0;
> -        int rc = 0;
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          /* Use while-break to avoid compiler warning */
>          while ( iommu_iotlb_flush_all(d, flush_flags) )
>              break;
> +#else
> +        rc = -ENOSYS;

-EOPNOTSUPP please.

> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct 
> vcpu_guest_context *vgc)
>  
>  static inline void arch_vcpu_block(struct vcpu *v) {}
>  
> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
> +{
> +    return INVALID_GFN;
> +}
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index d1bfc82f57..00d8b09794 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -118,4 +118,13 @@ struct vnuma_info {
>  
>  void vnuma_destroy(struct vnuma_info *vnuma);
>  
> +#ifdef CONFIG_HAS_M2P
> +#define domain_shared_info_gfn(d_) ({                           \
> +    struct domain *d = (d_);                                    \

const

Also the naming needs to be the other way around: The macro
parameter should be named d (all instances will get substituted
anyway, and hence name collisions are impossible) while the
local variable should be named d_, to avoid collisions with names
in the outer scopes.

> +                                                                \

I won't insist, but especially small macros are often an exception
to the blank-line-between-declarations-and-statements rule. IOW
I'd prefer if you dropped this line, unless others correct my view
here.

Jan



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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Jan Beulich]

>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>  #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>  #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>  
> -/* Xen always owns P2M on ARM */
> +/* We don't have a M2P on Arm */
>  #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
> -#define mfn_to_gmfn(_d, mfn)  (mfn)

So is the plan to remove the other macro from Arm then as well?
In any event
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

Hi Jan,

On 13/03/2019 15:22, Jan Beulich wrote:
>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>   #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>   
>> -/* Xen always owns P2M on ARM */
>> +/* We don't have a M2P on Arm */
>>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> 
> So is the plan to remove the other macro from Arm then as well?

Do you mean mfn_to_gfn? If so it does not exist on Arm.

> In any event
> Acked-by: Jan Beulich <jbeulich@suse.com>

Cheers,

> 
> Jan
> 
> 

-- 
Julien Grall

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Jan Beulich]

>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>>   #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>   
>>> -/* Xen always owns P2M on ARM */
>>> +/* We don't have a M2P on Arm */
>>>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>> 
>> So is the plan to remove the other macro from Arm then as well?
> 
> Do you mean mfn_to_gfn? If so it does not exist on Arm.

No, I mean the one in context above - set_gpfn_from_mfn().

Jan



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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

Hi,

On 13/03/2019 15:40, Jan Beulich wrote:
>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>>>    #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>    #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>    
>>>> -/* Xen always owns P2M on ARM */
>>>> +/* We don't have a M2P on Arm */
>>>>    #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>
>>> So is the plan to remove the other macro from Arm then as well?
>>
>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
> 
> No, I mean the one in context above - set_gpfn_from_mfn().

It is used in common code, so we would need to #idef the caller.

I think it is better to provide a NOP implementation. Could be moved somewhere 
in the common header though. Any opinions?

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Jan Beulich]

>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 13/03/2019 15:40, Jan Beulich wrote:
>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>>> --- a/xen/include/asm-arm/mm.h
>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
> vaddr_t va,
>>>>>    #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>>    #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>>    
>>>>> -/* Xen always owns P2M on ARM */
>>>>> +/* We don't have a M2P on Arm */
>>>>>    #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } 
> while (0)
>>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>>
>>>> So is the plan to remove the other macro from Arm then as well?
>>>
>>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
>> 
>> No, I mean the one in context above - set_gpfn_from_mfn().
> 
> It is used in common code, so we would need to #idef the caller.

Hmm, right, such #ifdef-ary would be undesirable (and two out of
the three common code callers would need it.

> I think it is better to provide a NOP implementation. Could be moved somewhere 
> in the common header though. Any opinions?

This would perhaps be better, now that you have HAVE_M2P.

Jan



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

Re: [PATCH for-next 5/9] xen: Convert hotplug page function to use typesafe MFN

[Julien Grall]

Hi,

On 13/03/2019 14:57, Jan Beulich wrote:
>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>> Convert online_page, offline_page and query_page_offline to use
>> typesafe MFN.
>>
>> No functional changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> But ...
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1568,23 +1568,23 @@ static int reserve_heap_page(struct page_info *pg)
>>   
>>   }
>>   
>> -int offline_page(unsigned long mfn, int broken, uint32_t *status)
>> +int offline_page(mfn_t mfn, int broken, uint32_t *status)
>>   {
>>       unsigned long old_info = 0;
>>       struct domain *owner;
>>       struct page_info *pg;
>>   
>> -    if ( !mfn_valid(_mfn(mfn)) )
>> +    if ( !mfn_valid(mfn) )
>>       {
>>           dprintk(XENLOG_WARNING,
>> -                "try to offline page out of range %lx\n", mfn);
>> +                "try to offline page out of range %"PRI_mfn"\n", mfn_x(mfn));
> 
> ... would you mind adjusting the wording here as well:
> "attempt to offline out of range page %"PRI_mfn"\n" or > some such?

Sure.

> 
> Of course the usefulness of the message as a whole is
> questionable, so I'd also be happy to see it deleted altogether.

While I am happy to do formatting, I would rather leave code removal in separate 
patch.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Julien Grall]

Hi,

On 13/03/2019 15:04, Jan Beulich wrote:
>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4300,7 +4300,8 @@ int xenmem_add_to_physmap_one(
>>   {
>>       struct page_info *page = NULL;
>>       unsigned long gfn = 0; /* gcc ... */
>> -    unsigned long prev_mfn, old_gpfn;
>> +    mfn_t prev_mfn;
>> +    unsigned long old_gpfn;
> 
> Please can you put this together with "gfn"?

Sure.

> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>        * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
>>        * prevent merging of power-of-two blocks across the zone boundary.
>>        */
>> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
>> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
> 
> Please add the missing blanks around - at the same time.

Ok.

> 
>>           ps += PAGE_SIZE;
>> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
>> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
> 
> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
> we don't have any mfn_sub(), I see.

Yes we don't have mfn_sub() (or even gfn_sub()). I only found a couple of places 
where such helpers might be useful. I can introduce the 2 helpers if you think 
it is worth it.

> 
>> --- a/xen/include/asm-x86/mm.h
>> +++ b/xen/include/asm-x86/mm.h
>> @@ -278,7 +278,7 @@ struct page_info
>>   
>>   #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
>>   #define is_xen_heap_mfn(mfn) \
>> -    (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
>> +    (__mfn_valid(mfn_x(mfn)) && is_xen_heap_page(mfn_to_page(mfn)))
> 
> Please don't open code mfn_valid().

That's a mistake. Someone I thought there were place in the code that was 
overriding mfn_valid().

I will update it.

> 
> With these minor issues taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you!

> 
> Jan
> 
> 

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi Jan,

On 13/03/2019 15:20, Jan Beulich wrote:
>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>>       info->outstanding_pages = d->outstanding_pages;
>>       info->shr_pages         = atomic_read(&d->shr_pages);
>>       info->paged_pages       = atomic_read(&d->paged_pages);
>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
> 
> I think this change wants to be accompanied by a warning attached
> to the field declaration in the public header.

Make sense.

> 
> But I'd also like to have the tool stack maintainers' view on making
> this field effectively unusable for Arm.

The value in shared_info_frame was plain wrong since the creation of Xen Arm. So 
this is just making the error more obvious. I don't expect any user of it on Arm.

> 
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -510,6 +510,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags)
>>       return true;
>>   }
>>   
>> +#ifdef CONFIG_M2P
>>   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>   {
>>       struct xen_memory_exchange exch;
>> @@ -802,6 +803,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>           rc = -EFAULT;
>>       return rc;
>>   }
>> +#endif
> 
> Please can you move the #ifdef inside the function body, add #else
> to return -EOPNOTSUPP, and ...

Sure.

> 
>> @@ -1233,12 +1235,14 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>   
>>           break;
>>   
>> +#ifdef CONFIG_M2P
>>       case XENMEM_exchange:
>>           if ( unlikely(start_extent) )
>>               return -EINVAL;
>>   
>>           rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
>>           break;
>> +#endif
> 
> ... avoid the extra #ifdef-ary here altogether?
> 
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -186,9 +186,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>       hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>>       if ( need_iommu_pt_sync(d) )
>>       {
>> +        int rc = 0;
>> +#ifdef CONFIG_HAS_M2P
>>           struct page_info *page;
>>           unsigned int i = 0, flush_flags = 0;
>> -        int rc = 0;
>>   
>>           page_list_for_each ( page, &d->page_list )
>>           {
>> @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>>           /* Use while-break to avoid compiler warning */
>>           while ( iommu_iotlb_flush_all(d, flush_flags) )
>>               break;
>> +#else
>> +        rc = -ENOSYS;
> 
> -EOPNOTSUPP please.

Sure.

> 
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct
>> vcpu_guest_context *vgc)
>>   
>>   static inline void arch_vcpu_block(struct vcpu *v) {}
>>   
>> +static inline gfn_t domain_shared_info_gfn(struct domain *d)
>> +{
>> +    return INVALID_GFN;
>> +}
>> +
>>   #endif /* __ASM_DOMAIN_H__ */
>>   
>>   /*
>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
>> index d1bfc82f57..00d8b09794 100644
>> --- a/xen/include/xen/domain.h
>> +++ b/xen/include/xen/domain.h
>> @@ -118,4 +118,13 @@ struct vnuma_info {
>>   
>>   void vnuma_destroy(struct vnuma_info *vnuma);
>>   
>> +#ifdef CONFIG_HAS_M2P
>> +#define domain_shared_info_gfn(d_) ({                           \
>> +    struct domain *d = (d_);                                    \
> 
> const
> 
> Also the naming needs to be the other way around: The macro
> parameter should be named d (all instances will get substituted
> anyway, and hence name collisions are impossible) while the
> local variable should be named d_, to avoid collisions with names
> in the outer scopes.

I will do.

> 
>> +                                                                \
> 
> I won't insist, but especially small macros are often an exception
> to the blank-line-between-declarations-and-statements rule. IOW
> I'd prefer if you dropped this line, unless others correct my view
> here.
The newline is here for clarity. I am not keen to drop it unless someone else 
agrees with the removal.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Andrew Cooper]

On 13/03/2019 15:59, Jan Beulich wrote:
>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 13/03/2019 15:40, Jan Beulich wrote:
>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>>>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>>>> --- a/xen/include/asm-arm/mm.h
>>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
>> vaddr_t va,
>>>>>>    #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>>>    #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>>>    
>>>>>> -/* Xen always owns P2M on ARM */
>>>>>> +/* We don't have a M2P on Arm */
>>>>>>    #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } 
>> while (0)
>>>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>>> So is the plan to remove the other macro from Arm then as well?
>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
>>> No, I mean the one in context above - set_gpfn_from_mfn().
>> It is used in common code, so we would need to #idef the caller.
> Hmm, right, such #ifdef-ary would be undesirable (and two out of
> the three common code callers would need it.
>
>> I think it is better to provide a NOP implementation. Could be moved somewhere 
>> in the common header though. Any opinions?
> This would perhaps be better, now that you have HAVE_M2P.

Given that "having an M2P" is now an x86-specific concept, I think
phasing set_gpfn_from_mfn()'s use out of common code is the way to go.

~Andrew

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

Hi Andrew,

On 13/03/2019 17:34, Andrew Cooper wrote:
> On 13/03/2019 15:59, Jan Beulich wrote:
>>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote:
>>> Hi,
>>>
>>> On 13/03/2019 15:40, Jan Beulich wrote:
>>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>>>>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>>>>> --- a/xen/include/asm-arm/mm.h
>>>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v,
>>> vaddr_t va,
>>>>>>>     #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>>>>     #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>>>>     
>>>>>>> -/* Xen always owns P2M on ARM */
>>>>>>> +/* We don't have a M2P on Arm */
>>>>>>>     #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); }
>>> while (0)
>>>>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>>>> So is the plan to remove the other macro from Arm then as well?
>>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
>>>> No, I mean the one in context above - set_gpfn_from_mfn().
>>> It is used in common code, so we would need to #idef the caller.
>> Hmm, right, such #ifdef-ary would be undesirable (and two out of
>> the three common code callers would need it.
>>
>>> I think it is better to provide a NOP implementation. Could be moved somewhere
>>> in the common header though. Any opinions?
>> This would perhaps be better, now that you have HAVE_M2P.
> 
> Given that "having an M2P" is now an x86-specific concept, I think
> phasing set_gpfn_from_mfn()'s use out of common code is the way to go.

So you never expect other architecture to use the M2P?

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Andrew Cooper]

On 13/03/2019 17:42, Julien Grall wrote:
> Hi Andrew,
>
> On 13/03/2019 17:34, Andrew Cooper wrote:
>> On 13/03/2019 15:59, Jan Beulich wrote:
>>>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote:
>>>> Hi,
>>>>
>>>> On 13/03/2019 15:40, Jan Beulich wrote:
>>>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>>>>>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>>>>>> --- a/xen/include/asm-arm/mm.h
>>>>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct
>>>>>>>> vcpu *v,
>>>> vaddr_t va,
>>>>>>>>     #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>>>>>     #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>>>>>     -/* Xen always owns P2M on ARM */
>>>>>>>> +/* We don't have a M2P on Arm */
>>>>>>>>     #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn),
>>>>>>>> (void)(pfn); }
>>>> while (0)
>>>>>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>>>>> So is the plan to remove the other macro from Arm then as well?
>>>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
>>>>> No, I mean the one in context above - set_gpfn_from_mfn().
>>>> It is used in common code, so we would need to #idef the caller.
>>> Hmm, right, such #ifdef-ary would be undesirable (and two out of
>>> the three common code callers would need it.
>>>
>>>> I think it is better to provide a NOP implementation. Could be
>>>> moved somewhere
>>>> in the common header though. Any opinions?
>>> This would perhaps be better, now that you have HAVE_M2P.
>>
>> Given that "having an M2P" is now an x86-specific concept, I think
>> phasing set_gpfn_from_mfn()'s use out of common code is the way to go.
>
> So you never expect other architecture to use the M2P?

I guess that depends on how likely it is going to be that Xen gains a
new non-HVM-like virtualisation mode on a new architecture.

~Andrew

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Jan Beulich]

>>> On 13.03.19 at 18:24, <julien.grall@arm.com> wrote:
> On 13/03/2019 15:04, Jan Beulich wrote:
>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>        * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
>>>        * prevent merging of power-of-two blocks across the zone boundary.
>>>        */
>>> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
>>> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
>>>           ps += PAGE_SIZE;
>>> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
>>> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
>> 
>> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
>> we don't have any mfn_sub(), I see.
> 
> Yes we don't have mfn_sub() (or even gfn_sub()). I only found a couple of places 
> where such helpers might be useful. I can introduce the 2 helpers if you think 
> it is worth it.

Well, I guess in the end I'm fine either way. It simply struck me
as odd at the first glance that you use maddr_to_mfn() in one
case but not the other.

Jan



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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Jan Beulich]

>>> On 13.03.19 at 18:30, <julien.grall@arm.com> wrote:
> On 13/03/2019 15:20, Jan Beulich wrote:
>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct 
> xen_domctl_getdomaininfo *info)
>>>       info->outstanding_pages = d->outstanding_pages;
>>>       info->shr_pages         = atomic_read(&d->shr_pages);
>>>       info->paged_pages       = atomic_read(&d->paged_pages);
>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>> 
>> I think this change wants to be accompanied by a warning attached
>> to the field declaration in the public header.
> 
> Make sense.
> 
>> 
>> But I'd also like to have the tool stack maintainers' view on making
>> this field effectively unusable for Arm.
> 
> The value in shared_info_frame was plain wrong since the creation of Xen Arm. So 
> this is just making the error more obvious. I don't expect any user of it on Arm.

Well, my request for tool stack maintainer input wasn't to put under
question that the field can't currently be used sensibly on Arm.
Instead I'm meaning to know whether it can be sensibly expected
for the tool stack to want to use the field uniformly, in which case
rather than making it more obviously not work it should be fixed
instead.

Jan



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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Jan Beulich]

>>> On 13.03.19 at 18:34, <andrew.cooper3@citrix.com> wrote:
> On 13/03/2019 15:59, Jan Beulich wrote:
>>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote:
>>> On 13/03/2019 15:40, Jan Beulich wrote:
>>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote:
>>>>> On 13/03/2019 15:22, Jan Beulich wrote:
>>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>>>>>>> --- a/xen/include/asm-arm/mm.h
>>>>>>> +++ b/xen/include/asm-arm/mm.h
>>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, 
>>> vaddr_t va,
>>>>>>>    #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>>>>>>    #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>>>>>>    
>>>>>>> -/* Xen always owns P2M on ARM */
>>>>>>> +/* We don't have a M2P on Arm */
>>>>>>>    #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } 
>>> while (0)
>>>>>>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
>>>>>> So is the plan to remove the other macro from Arm then as well?
>>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm.
>>>> No, I mean the one in context above - set_gpfn_from_mfn().
>>> It is used in common code, so we would need to #idef the caller.
>> Hmm, right, such #ifdef-ary would be undesirable (and two out of
>> the three common code callers would need it.
>>
>>> I think it is better to provide a NOP implementation. Could be moved somewhere 
>>> in the common header though. Any opinions?
>> This would perhaps be better, now that you have HAVE_M2P.
> 
> Given that "having an M2P" is now an x86-specific concept, I think
> phasing set_gpfn_from_mfn()'s use out of common code is the way to go.

But what's the implication of this? There would need to be some
arch_*() hook used in the place that set_gpfn_from_mfn() is
invoked currently. But then it can as well remain
set_gpfn_from_mfn() (with the !HAVE_M2P stubbed out properly
in a common header), can't it?

Jan



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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Jan Beulich]

>>> On 13.03.19 at 19:41, <andrew.cooper3@citrix.com> wrote:
> On 13/03/2019 17:42, Julien Grall wrote:
>> On 13/03/2019 17:34, Andrew Cooper wrote:
>>> Given that "having an M2P" is now an x86-specific concept, I think
>>> phasing set_gpfn_from_mfn()'s use out of common code is the way to go.
>>
>> So you never expect other architecture to use the M2P?
> 
> I guess that depends on how likely it is going to be that Xen gains a
> new non-HVM-like virtualisation mode on a new architecture.

Well, not quite. I don't think it would be straightforward to make
x86 select HAVE_M2P only when PV is also enabled. Hence a HVM-
like implementation may still want to maintain M2P. In fact it is
my understanding that 64-bit Arm could easily do (leaving aside
the question of whether the memory needed to build the tables
would be well spent this), but it's prohibitive on 32-bit, and hence
it's easier for the Arm code overall to uniformly resort to
alternative means where such a translation is indeed needed.

Jan



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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Julien Grall]

Hi Jan,

On 3/14/19 7:52 AM, Jan Beulich wrote:
>>>> On 13.03.19 at 18:24, <julien.grall@arm.com> wrote:
>> On 13/03/2019 15:04, Jan Beulich wrote:
>>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>>         * Yuk! Ensure there is a one-page buffer between Xen and Dom zones, to
>>>>         * prevent merging of power-of-two blocks across the zone boundary.
>>>>         */
>>>> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
>>>> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
>>>>            ps += PAGE_SIZE;
>>>> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
>>>> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
>>>
>>> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
>>> we don't have any mfn_sub(), I see.
>>
>> Yes we don't have mfn_sub() (or even gfn_sub()). I only found a couple of places
>> where such helpers might be useful. I can introduce the 2 helpers if you think
>> it is worth it.
> 
> Well, I guess in the end I'm fine either way. It simply struck me
> as odd at the first glance that you use maddr_to_mfn() in one
> case but not the other.

I wanted to avoid adding mfn_x(...) on the line:

_mfn(mfn_x(maddr_to_mfn(ps)) - 1)

I will look to introduce mfn_sub()/gfn_sub().

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Andrew Cooper]

On 14/03/2019 10:12, Julien Grall wrote:
> Hi Jan,
>
> On 3/14/19 7:52 AM, Jan Beulich wrote:
>>>>> On 13.03.19 at 18:24, <julien.grall@arm.com> wrote:
>>> On 13/03/2019 15:04, Jan Beulich wrote:
>>>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>>>> --- a/xen/common/page_alloc.c
>>>>> +++ b/xen/common/page_alloc.c
>>>>> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>>>         * Yuk! Ensure there is a one-page buffer between Xen and
>>>>> Dom zones, to
>>>>>         * prevent merging of power-of-two blocks across the zone
>>>>> boundary.
>>>>>         */
>>>>> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
>>>>> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
>>>>>            ps += PAGE_SIZE;
>>>>> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
>>>>> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
>>>>
>>>> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
>>>> we don't have any mfn_sub(), I see.
>>>
>>> Yes we don't have mfn_sub() (or even gfn_sub()). I only found a
>>> couple of places
>>> where such helpers might be useful. I can introduce the 2 helpers if
>>> you think
>>> it is worth it.
>>
>> Well, I guess in the end I'm fine either way. It simply struck me
>> as odd at the first glance that you use maddr_to_mfn() in one
>> case but not the other.
>
> I wanted to avoid adding mfn_x(...) on the line:
>
> _mfn(mfn_x(maddr_to_mfn(ps)) - 1)
>
> I will look to introduce mfn_sub()/gfn_sub().

You do know that mfn_add(mfn, -1) will DTRT?

~Andrew

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Julien Grall]

Hi Andrew,

On 3/14/19 10:14 AM, Andrew Cooper wrote:
> On 14/03/2019 10:12, Julien Grall wrote:
>> Hi Jan,
>>
>> On 3/14/19 7:52 AM, Jan Beulich wrote:
>>>>>> On 13.03.19 at 18:24, <julien.grall@arm.com> wrote:
>>>> On 13/03/2019 15:04, Jan Beulich wrote:
>>>>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>>>>> --- a/xen/common/page_alloc.c
>>>>>> +++ b/xen/common/page_alloc.c
>>>>>> @@ -2121,9 +2121,9 @@ void init_xenheap_pages(paddr_t ps, paddr_t pe)
>>>>>>          * Yuk! Ensure there is a one-page buffer between Xen and
>>>>>> Dom zones, to
>>>>>>          * prevent merging of power-of-two blocks across the zone
>>>>>> boundary.
>>>>>>          */
>>>>>> -    if ( ps && !is_xen_heap_mfn(paddr_to_pfn(ps)-1) )
>>>>>> +    if ( ps && !is_xen_heap_mfn(_mfn(paddr_to_pfn(ps)-1)) )
>>>>>>             ps += PAGE_SIZE;
>>>>>> -    if ( !is_xen_heap_mfn(paddr_to_pfn(pe)) )
>>>>>> +    if ( !is_xen_heap_mfn(maddr_to_mfn(pe)) )
>>>>>
>>>>> Why maddr_to_mfn() here but still paddr_to_pfn() above? Oh,
>>>>> we don't have any mfn_sub(), I see.
>>>>
>>>> Yes we don't have mfn_sub() (or even gfn_sub()). I only found a
>>>> couple of places
>>>> where such helpers might be useful. I can introduce the 2 helpers if
>>>> you think
>>>> it is worth it.
>>>
>>> Well, I guess in the end I'm fine either way. It simply struck me
>>> as odd at the first glance that you use maddr_to_mfn() in one
>>> case but not the other.
>>
>> I wanted to avoid adding mfn_x(...) on the line:
>>
>> _mfn(mfn_x(maddr_to_mfn(ps)) - 1)
>>
>> I will look to introduce mfn_sub()/gfn_sub().
> 
> You do know that mfn_add(mfn, -1) will DTRT?

It didn't occur to me until you said it. I will use that then.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Jan Beulich]

>>> On 14.03.19 at 11:14, <andrew.cooper3@citrix.com> wrote:
> You do know that mfn_add(mfn, -1) will DTRT?

It will, but imo it looks slightly odd in particular in e.g.
for ( ...; ...; mfn_add(mfn, -1) ), hence I didn't suggest it. But
I don't object either.

Jan



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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Andrew Cooper]

On 14/03/2019 11:47, Jan Beulich wrote:
>>>> On 14.03.19 at 11:14, <andrew.cooper3@citrix.com> wrote:
>> You do know that mfn_add(mfn, -1) will DTRT?
> It will, but imo it looks slightly odd in particular in e.g.
> for ( ...; ...; mfn_add(mfn, -1) ), hence I didn't suggest it. But
> I don't object either.

Well - in mathematics, x - 1 is identical to x + (-1), so the construct
doesn't look overly odd to me.

Alternatively, we could rename mfn_add to something else, but nothing
comes to mind which would be as clear.

~Andrew

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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
> the Arm code to use the former.

This is good but maybe we can go even further.

You should also be able to replace one call site of pfn_to_pdx in
mfn_valid and the one in maddr_to_virt. Something like this:


diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index eafa26f..b3455ea 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 /* XXX -- account for base */
 #define mfn_valid(mfn)        ({                                              \
     unsigned long __m_f_n = mfn_x(mfn);                                       \
-    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
+    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
 })
 
 /* Convert between machine frame numbers and page-info structures. */
@@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
 #else
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
+    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
                     mfn_to_maddr(xenheap_mfn_start) +
                     ((ma & ma_va_bottom_mask) |



> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c        | 2 +-
>  xen/include/asm-arm/mm.h | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 01ae2cccc0..be5338bb4c 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -886,7 +886,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      int i;
>  #endif
>  
> -    frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
> +    frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>      /* Round up to 2M or 32M boundary, as appropriate. */
>      frametable_size = ROUNDUP(frametable_size, mapping_size);
>      base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index eafa26f56e..7b6aaf5e3f 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -225,7 +225,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>  /* Convert between frame number and address formats.  */
>  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
>  #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
> -#define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> +#define paddr_to_pdx(pa)    mfn_to_pdx(maddr_to_mfn(pa))
>  #define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
>  #define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
>  #define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>  #else
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>      return (void *)(XENHEAP_VIRT_START -
>                      mfn_to_maddr(xenheap_mfn_start) +
>                      ((ma & ma_va_bottom_mask) |
> @@ -301,7 +301,7 @@ static inline struct page_info *virt_to_page(const void *v)
>      ASSERT(va < xenheap_virt_end);
>  
>      pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
> -    pdx += pfn_to_pdx(mfn_x(xenheap_mfn_start));
> +    pdx += mfn_to_pdx(xenheap_mfn_start);
>      return frame_table + pdx - frametable_base_pdx;
>  }
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
> the Arm code to use the former.

This is good but maybe we can go even further.

You should also be able to replace one call site of pfn_to_pdx in
mfn_valid and the one in maddr_to_virt. Something like this:


diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index eafa26f..b3455ea 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 /* XXX -- account for base */
 #define mfn_valid(mfn)        ({                                              \
     unsigned long __m_f_n = mfn_x(mfn);                                       \
-    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
+    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
 })
 
 /* Convert between machine frame numbers and page-info structures. */
@@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
 #else
 static inline void *maddr_to_virt(paddr_t ma)
 {
-    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
+    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
     return (void *)(XENHEAP_VIRT_START -
                     mfn_to_maddr(xenheap_mfn_start) +
                     ((ma & ma_va_bottom_mask) |



> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c        | 2 +-
>  xen/include/asm-arm/mm.h | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 01ae2cccc0..be5338bb4c 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -886,7 +886,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      int i;
>  #endif
>  
> -    frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
> +    frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
>      /* Round up to 2M or 32M boundary, as appropriate. */
>      frametable_size = ROUNDUP(frametable_size, mapping_size);
>      base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index eafa26f56e..7b6aaf5e3f 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -225,7 +225,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>  /* Convert between frame number and address formats.  */
>  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
>  #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
> -#define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
> +#define paddr_to_pdx(pa)    mfn_to_pdx(maddr_to_mfn(pa))
>  #define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
>  #define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
>  #define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>  #else
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>      return (void *)(XENHEAP_VIRT_START -
>                      mfn_to_maddr(xenheap_mfn_start) +
>                      ((ma & ma_va_bottom_mask) |
> @@ -301,7 +301,7 @@ static inline struct page_info *virt_to_page(const void *v)
>      ASSERT(va < xenheap_virt_end);
>  
>      pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT;
> -    pdx += pfn_to_pdx(mfn_x(xenheap_mfn_start));
> +    pdx += mfn_to_pdx(xenheap_mfn_start);
>      return frame_table + pdx - frametable_base_pdx;
>  }
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> No functional changes intended.

Please list here the changes this patch is making:

- renaming gnttab_shared_gmfn to gnttab_shared_gfn to follow the naming
  convention (and others.)
- making gnttab_shared_gmfn returning gfn_t

With the better commit message, add my acked-by.


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/common/grant_table.c          |  4 ++--
>  xen/include/asm-arm/grant_table.h | 12 ++++++------
>  xen/include/asm-x86/grant_table.h | 20 ++++++++------------
>  3 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fd099a8f25..e7a65b38e0 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1935,7 +1935,7 @@ gnttab_setup_table(
>      op.status = GNTST_okay;
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
> -        xen_pfn_t gmfn = gnttab_shared_gmfn(d, gt, i);
> +        xen_pfn_t gmfn = gfn_x(gnttab_shared_gfn(d, gt, i));
>  
>          /* Grant tables cannot be shared */
>          BUG_ON(SHARED_M2P(gmfn));
> @@ -3147,7 +3147,7 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
>  
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
> -        gmfn = gnttab_status_gmfn(d, gt, i);
> +        gmfn = gfn_x(gnttab_status_gfn(d, gt, i));
>          if ( copy_to_guest_offset(op.frame_list, i, &gmfn, 1) )
>              op.status = GNTST_bad_virt_addr;
>      }
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 816e3c6d68..4c44b720f2 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -67,15 +67,15 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>      } while ( 0 )
>  
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -   _gfn((st) ? gnttab_status_gmfn(NULL, gt, idx)                         \
> -             : gnttab_shared_gmfn(NULL, gt, idx));                       \
> +   (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> +             : gnttab_shared_gfn(NULL, gt, idx);                         \
>  })
>  
> -#define gnttab_shared_gmfn(d, t, i)                                      \
> -    gfn_x(((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
> +#define gnttab_shared_gfn(d, t, i)                                       \
> +    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
>  
> -#define gnttab_status_gmfn(d, t, i)                                      \
> -    gfn_x(((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
> +#define gnttab_status_gfn(d, t, i)                                       \
> +    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
>  
>  #define gnttab_need_iommu_mapping(d)                    \
>      (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
> index 4b8c4f9160..8736d7286d 100644
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -43,24 +43,20 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
>  #define gnttab_destroy_arch(gt) do {} while ( 0 )
>  #define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -    unsigned long mfn_ = (st) ? gnttab_status_mfn(gt, idx)               \
> -                              : gnttab_shared_mfn(gt, idx);              \
> -    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_);                       \
> +    mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
> +                      : gnttab_shared_mfn(gt, idx);                      \
> +    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_));                \
>      VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
>  })
>  
> -#define gnttab_shared_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->shared_raw[i]) >> PAGE_SHIFT))
> +#define gnttab_shared_mfn(t, i) _mfn(__virt_to_mfn((t)->shared_raw[i]))
>  
> -#define gnttab_shared_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_shared_mfn(t, i)))
> +#define gnttab_shared_gfn(d, t, i) _gfn(mfn_to_gfn(d, gnttab_shared_mfn(t, i)))
>  
> +#define gnttab_status_mfn(t, i) _mfn(__virt_to_mfn((t)->status[i]))
>  
> -#define gnttab_status_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->status[i]) >> PAGE_SHIFT))
> -
> -#define gnttab_status_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_status_mfn(t, i)))
> +#define gnttab_status_gfn(d, t, i) \
> +    _gfn(mfn_to_gfn(d, gnttab_status_mfn(t, i)))
>  
>  #define gnttab_mark_dirty(d, f) paging_mark_dirty((d), f)

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

Re: [Xen-devel] [PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> No functional changes intended.

Please list here the changes this patch is making:

- renaming gnttab_shared_gmfn to gnttab_shared_gfn to follow the naming
  convention (and others.)
- making gnttab_shared_gmfn returning gfn_t

With the better commit message, add my acked-by.


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/common/grant_table.c          |  4 ++--
>  xen/include/asm-arm/grant_table.h | 12 ++++++------
>  xen/include/asm-x86/grant_table.h | 20 ++++++++------------
>  3 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index fd099a8f25..e7a65b38e0 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1935,7 +1935,7 @@ gnttab_setup_table(
>      op.status = GNTST_okay;
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
> -        xen_pfn_t gmfn = gnttab_shared_gmfn(d, gt, i);
> +        xen_pfn_t gmfn = gfn_x(gnttab_shared_gfn(d, gt, i));
>  
>          /* Grant tables cannot be shared */
>          BUG_ON(SHARED_M2P(gmfn));
> @@ -3147,7 +3147,7 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
>  
>      for ( i = 0; i < op.nr_frames; i++ )
>      {
> -        gmfn = gnttab_status_gmfn(d, gt, i);
> +        gmfn = gfn_x(gnttab_status_gfn(d, gt, i));
>          if ( copy_to_guest_offset(op.frame_list, i, &gmfn, 1) )
>              op.status = GNTST_bad_virt_addr;
>      }
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 816e3c6d68..4c44b720f2 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -67,15 +67,15 @@ void gnttab_mark_dirty(struct domain *d, mfn_t mfn);
>      } while ( 0 )
>  
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -   _gfn((st) ? gnttab_status_gmfn(NULL, gt, idx)                         \
> -             : gnttab_shared_gmfn(NULL, gt, idx));                       \
> +   (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
> +             : gnttab_shared_gfn(NULL, gt, idx);                         \
>  })
>  
> -#define gnttab_shared_gmfn(d, t, i)                                      \
> -    gfn_x(((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
> +#define gnttab_shared_gfn(d, t, i)                                       \
> +    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
>  
> -#define gnttab_status_gmfn(d, t, i)                                      \
> -    gfn_x(((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
> +#define gnttab_status_gfn(d, t, i)                                       \
> +    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
>  
>  #define gnttab_need_iommu_mapping(d)                    \
>      (is_domain_direct_mapped(d) && need_iommu_pt_sync(d))
> diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
> index 4b8c4f9160..8736d7286d 100644
> --- a/xen/include/asm-x86/grant_table.h
> +++ b/xen/include/asm-x86/grant_table.h
> @@ -43,24 +43,20 @@ static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
>  #define gnttab_destroy_arch(gt) do {} while ( 0 )
>  #define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 )
>  #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
> -    unsigned long mfn_ = (st) ? gnttab_status_mfn(gt, idx)               \
> -                              : gnttab_shared_mfn(gt, idx);              \
> -    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_);                       \
> +    mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx)                       \
> +                      : gnttab_shared_mfn(gt, idx);                      \
> +    unsigned long gpfn_ = get_gpfn_from_mfn(mfn_x(mfn_));                \
>      VALID_M2P(gpfn_) ? _gfn(gpfn_) : INVALID_GFN;                        \
>  })
>  
> -#define gnttab_shared_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->shared_raw[i]) >> PAGE_SHIFT))
> +#define gnttab_shared_mfn(t, i) _mfn(__virt_to_mfn((t)->shared_raw[i]))
>  
> -#define gnttab_shared_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_shared_mfn(t, i)))
> +#define gnttab_shared_gfn(d, t, i) _gfn(mfn_to_gfn(d, gnttab_shared_mfn(t, i)))
>  
> +#define gnttab_status_mfn(t, i) _mfn(__virt_to_mfn((t)->status[i]))
>  
> -#define gnttab_status_mfn(t, i)                         \
> -    ((virt_to_maddr((t)->status[i]) >> PAGE_SHIFT))
> -
> -#define gnttab_status_gmfn(d, t, i)                     \
> -    (mfn_to_gmfn(d, gnttab_status_mfn(t, i)))
> +#define gnttab_status_gfn(d, t, i) \
> +    _gfn(mfn_to_gfn(d, gnttab_status_mfn(t, i)))
>  
>  #define gnttab_mark_dirty(d, f) paging_mark_dirty((d), f)

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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi,

On 4/15/19 10:55 PM, Stefano Stabellini wrote:
> On Mon, 18 Feb 2019, Julien Grall wrote:
>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>> the Arm code to use the former.
> 
> This is good but maybe we can go even further.
> 
> You should also be able to replace one call site of pfn_to_pdx in
> mfn_valid and the one in maddr_to_virt. Something like this:
> 
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index eafa26f..b3455ea 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>   /* XXX -- account for base */
>   #define mfn_valid(mfn)        ({                                              \
>       unsigned long __m_f_n = mfn_x(mfn);                                       \
> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \

This is quite undesirable, you will end up to evaluate mfn twice here.

The other solution is to turn _m_f_n to an mfn_t but then it does make 
much difference as you would need to use a mfn_x(mfn) in the code.

>   })
>   
>   /* Convert between machine frame numbers and page-info structures. */
> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>   #else
>   static inline void *maddr_to_virt(paddr_t ma)
>   {
> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>       return (void *)(XENHEAP_VIRT_START -
>                       mfn_to_maddr(xenheap_mfn_start) +
>                       ((ma & ma_va_bottom_mask) |
> 

I fail to see what this chunk adds compare to the existing one...

>> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>>   #else
>>   static inline void *maddr_to_virt(paddr_t ma)
>>   {
>> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>>       return (void *)(XENHEAP_VIRT_START -
>>                       mfn_to_maddr(xenheap_mfn_start) +
>>                       ((ma & ma_va_bottom_mask) |
... here.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi,

On 4/15/19 10:55 PM, Stefano Stabellini wrote:
> On Mon, 18 Feb 2019, Julien Grall wrote:
>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>> the Arm code to use the former.
> 
> This is good but maybe we can go even further.
> 
> You should also be able to replace one call site of pfn_to_pdx in
> mfn_valid and the one in maddr_to_virt. Something like this:
> 
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index eafa26f..b3455ea 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>   /* XXX -- account for base */
>   #define mfn_valid(mfn)        ({                                              \
>       unsigned long __m_f_n = mfn_x(mfn);                                       \
> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \
> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \

This is quite undesirable, you will end up to evaluate mfn twice here.

The other solution is to turn _m_f_n to an mfn_t but then it does make 
much difference as you would need to use a mfn_x(mfn) in the code.

>   })
>   
>   /* Convert between machine frame numbers and page-info structures. */
> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>   #else
>   static inline void *maddr_to_virt(paddr_t ma)
>   {
> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>       return (void *)(XENHEAP_VIRT_START -
>                       mfn_to_maddr(xenheap_mfn_start) +
>                       ((ma & ma_va_bottom_mask) |
> 

I fail to see what this chunk adds compare to the existing one...

>> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>>   #else
>>   static inline void *maddr_to_virt(paddr_t ma)
>>   {
>> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>> +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
>>       return (void *)(XENHEAP_VIRT_START -
>>                       mfn_to_maddr(xenheap_mfn_start) +
>>                       ((ma & ma_va_bottom_mask) |
... here.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 6/9] xen: Convert is_xen_fixed_mfn to use typesafe MFN

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/x86/tboot.c     | 2 +-
>  xen/common/page_alloc.c  | 2 +-
>  xen/include/asm-arm/mm.h | 4 ++--
>  xen/include/asm-x86/mm.h | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> index f3fdee4d39..30d159cc62 100644
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -280,7 +280,7 @@ static void tboot_gen_xenheap_integrity(const uint8_t key[TB_KEY_SIZE],
>  
>          if ( !mfn_valid(_mfn(mfn)) )
>              continue;
> -        if ( is_xen_fixed_mfn(mfn) )
> +        if ( is_xen_fixed_mfn(_mfn(mfn)) )
>              continue; /* skip Xen */
>          if ( (mfn >= PFN_DOWN(g_tboot_shared->tboot_base - 3 * PAGE_SIZE))
>               && (mfn < PFN_UP(g_tboot_shared->tboot_base
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 5684a13557..5de3686d85 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1584,7 +1584,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>      *status = 0;
>      pg = mfn_to_page(mfn);
>  
> -    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
> +    if ( is_xen_fixed_mfn(mfn) )
>      {
>          *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
>            (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 7b6aaf5e3f..b56018aace 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
>  #endif
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
> -    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
> -     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
> +    ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
> +     (mfn_to_maddr(mfn) <= virt_to_maddr(&_end)))
>  
>  #define page_get_owner(_p)    (_p)->v.inuse.domain
>  #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 6faa563167..f124f57964 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -280,8 +280,8 @@ struct page_info
>  #define is_xen_heap_mfn(mfn) \
>      (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
>  #define is_xen_fixed_mfn(mfn)                     \
> -    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
> -     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
> +    (((mfn_to_maddr(mfn)) >= __pa(&_stext)) &&    \
> +     ((mfn_to_maddr(mfn)) <= __pa(&__2M_rwdata_end)))
>  
>  #define PRtype_info "016lx"/* should only be used for printk's */
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH for-next 6/9] xen: Convert is_xen_fixed_mfn to use typesafe MFN

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> No functional changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/x86/tboot.c     | 2 +-
>  xen/common/page_alloc.c  | 2 +-
>  xen/include/asm-arm/mm.h | 4 ++--
>  xen/include/asm-x86/mm.h | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> index f3fdee4d39..30d159cc62 100644
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -280,7 +280,7 @@ static void tboot_gen_xenheap_integrity(const uint8_t key[TB_KEY_SIZE],
>  
>          if ( !mfn_valid(_mfn(mfn)) )
>              continue;
> -        if ( is_xen_fixed_mfn(mfn) )
> +        if ( is_xen_fixed_mfn(_mfn(mfn)) )
>              continue; /* skip Xen */
>          if ( (mfn >= PFN_DOWN(g_tboot_shared->tboot_base - 3 * PAGE_SIZE))
>               && (mfn < PFN_UP(g_tboot_shared->tboot_base
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 5684a13557..5de3686d85 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1584,7 +1584,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>      *status = 0;
>      pg = mfn_to_page(mfn);
>  
> -    if ( is_xen_fixed_mfn(mfn_x(mfn)) )
> +    if ( is_xen_fixed_mfn(mfn) )
>      {
>          *status = PG_OFFLINE_XENPAGE | PG_OFFLINE_FAILED |
>            (DOMID_XEN << PG_OFFLINE_OWNER_SHIFT);
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 7b6aaf5e3f..b56018aace 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -151,8 +151,8 @@ extern vaddr_t xenheap_virt_start;
>  #endif
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
> -    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
> -     (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))
> +    ((mfn_to_maddr(mfn) >= virt_to_maddr(&_start)) &&           \
> +     (mfn_to_maddr(mfn) <= virt_to_maddr(&_end)))
>  
>  #define page_get_owner(_p)    (_p)->v.inuse.domain
>  #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 6faa563167..f124f57964 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -280,8 +280,8 @@ struct page_info
>  #define is_xen_heap_mfn(mfn) \
>      (__mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
>  #define is_xen_fixed_mfn(mfn)                     \
> -    ((((mfn) << PAGE_SHIFT) >= __pa(&_stext)) &&  \
> -     (((mfn) << PAGE_SHIFT) <= __pa(&__2M_rwdata_end)))
> +    (((mfn_to_maddr(mfn)) >= __pa(&_stext)) &&    \
> +     ((mfn_to_maddr(mfn)) <= __pa(&__2M_rwdata_end)))
>  
>  #define PRtype_info "016lx"/* should only be used for printk's */
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Julien Grall]

Hi,

On 4/15/19 11:03 PM, Stefano Stabellini wrote:
> On Mon, 18 Feb 2019, Julien Grall wrote:
>> No functional changes intended.
> 
> Please list here the changes this patch is making:
> 
> - renaming gnttab_shared_gmfn to gnttab_shared_gfn to follow the naming
>    convention (and others.)

Ok for this but...

> - making gnttab_shared_gmfn returning gfn_t

... it does not make sense to mention one macro and not all the other 
that was modified because of the change. This is also already pretty 
clear from the commit title that all macros will be typesafe.

So I would rather not mention this.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 4/9] xen/grant-table: Make arch specific macros typesafe

[Julien Grall]

Hi,

On 4/15/19 11:03 PM, Stefano Stabellini wrote:
> On Mon, 18 Feb 2019, Julien Grall wrote:
>> No functional changes intended.
> 
> Please list here the changes this patch is making:
> 
> - renaming gnttab_shared_gmfn to gnttab_shared_gfn to follow the naming
>    convention (and others.)

Ok for this but...

> - making gnttab_shared_gmfn returning gfn_t

... it does not make sense to mention one macro and not all the other 
that was modified because of the change. This is also already pretty 
clear from the commit title that all macros will be typesafe.

So I would rather not mention this.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index b56018aace..a9c8352b94 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -138,16 +138,16 @@ extern vaddr_t xenheap_virt_start;
>  #endif
>  
>  #ifdef CONFIG_ARM_32
> -#define is_xen_heap_page(page) is_xen_heap_mfn(mfn_x(page_to_mfn(page)))
> +#define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
>  #define is_xen_heap_mfn(mfn) ({                                 \
> -    unsigned long mfn_ = (mfn);                                 \
> +    unsigned long mfn_ = mfn_x(mfn);                            \
>      (mfn_ >= mfn_x(xenheap_mfn_start) &&                        \
>       mfn_ < mfn_x(xenheap_mfn_end));                            \
>  })
>  #else
>  #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
>  #define is_xen_heap_mfn(mfn) \
> -    (mfn_valid(_mfn(mfn)) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> +    (mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(mfn)))
>  #endif
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
> @@ -246,7 +246,7 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
>  #ifdef CONFIG_ARM_32
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    ASSERT(is_xen_heap_mfn(ma >> PAGE_SHIFT));
> +    ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
>      ma -= mfn_to_maddr(xenheap_mfn_start);
>      return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
>  }

For the ARM parts:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [Xen-devel] [PATCH for-next 7/9] xen: Convert is_xen_heap_mfn to use typesafe MFN

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index b56018aace..a9c8352b94 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -138,16 +138,16 @@ extern vaddr_t xenheap_virt_start;
>  #endif
>  
>  #ifdef CONFIG_ARM_32
> -#define is_xen_heap_page(page) is_xen_heap_mfn(mfn_x(page_to_mfn(page)))
> +#define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
>  #define is_xen_heap_mfn(mfn) ({                                 \
> -    unsigned long mfn_ = (mfn);                                 \
> +    unsigned long mfn_ = mfn_x(mfn);                            \
>      (mfn_ >= mfn_x(xenheap_mfn_start) &&                        \
>       mfn_ < mfn_x(xenheap_mfn_end));                            \
>  })
>  #else
>  #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
>  #define is_xen_heap_mfn(mfn) \
> -    (mfn_valid(_mfn(mfn)) && is_xen_heap_page(mfn_to_page(_mfn(mfn))))
> +    (mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(mfn)))
>  #endif
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
> @@ -246,7 +246,7 @@ static inline paddr_t __virt_to_maddr(vaddr_t va)
>  #ifdef CONFIG_ARM_32
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    ASSERT(is_xen_heap_mfn(ma >> PAGE_SHIFT));
> +    ASSERT(is_xen_heap_mfn(maddr_to_mfn(ma)));
>      ma -= mfn_to_maddr(xenheap_mfn_start);
>      return (void *)(unsigned long) ma + XENHEAP_VIRT_START;
>  }

For the ARM parts:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Stefano Stabellini]

On Wed, 13 Mar 2019, Jan Beulich wrote:
> > @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >          /* Use while-break to avoid compiler warning */
> >          while ( iommu_iotlb_flush_all(d, flush_flags) )
> >              break;
> > +#else
> > +        rc = -ENOSYS;
> 
> -EOPNOTSUPP please.

The patch is fine by me and I am also fine with Jan's comments.

I only want to point out that we haven't been entirely consistent with
-ENOSYS and/or -EOPNOTSUPP. We have lots of places that return -ENOSYS.
Should we change them to -EOPNOTSUPP going forward?

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Stefano Stabellini]

On Wed, 13 Mar 2019, Jan Beulich wrote:
> > @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >          /* Use while-break to avoid compiler warning */
> >          while ( iommu_iotlb_flush_all(d, flush_flags) )
> >              break;
> > +#else
> > +        rc = -ENOSYS;
> 
> -EOPNOTSUPP please.

The patch is fine by me and I am also fine with Jan's comments.

I only want to point out that we haven't been entirely consistent with
-ENOSYS and/or -EOPNOTSUPP. We have lots of places that return -ENOSYS.
Should we change them to -EOPNOTSUPP going forward?

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
> no more call to mfn_to_gmfn, so the helper can be dropped.
> 
> At the same time rework a comment in Arm code that does not make sense.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Lovely.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/drivers/passthrough/iommu.c | 6 +++---
>  xen/include/asm-arm/mm.h        | 4 +---
>  xen/include/asm-x86/mm.h        | 5 -----
>  3 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 5742cd05b8..04ac46239e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -193,8 +193,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> -            unsigned long mfn = mfn_x(page_to_mfn(page));
> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
> +            mfn_t mfn = page_to_mfn(page);
> +            unsigned long dfn = mfn_to_gfn(d, mfn);
>              unsigned int mapping = IOMMUF_readable;
>              int ret;
>  
> @@ -203,7 +203,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>                    == PGT_writable_page) )
>                  mapping |= IOMMUF_writable;
>  
> -            ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
> +            ret = iommu_map(d, _dfn(dfn), mfn, 0, mapping,
>                              &flush_flags);
>  
>              if ( !rc )
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index a9c8352b94..a9cb98a6c7 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>  #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>  #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>  
> -/* Xen always owns P2M on ARM */
> +/* We don't have a M2P on Arm */
>  #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> -
>  
>  /* Arch-specific portion of memory_op hypercall. */
>  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);

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

Re: [Xen-devel] [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Stefano Stabellini]

On Mon, 18 Feb 2019, Julien Grall wrote:
> On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are
> no more call to mfn_to_gmfn, so the helper can be dropped.
> 
> At the same time rework a comment in Arm code that does not make sense.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Lovely.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  xen/drivers/passthrough/iommu.c | 6 +++---
>  xen/include/asm-arm/mm.h        | 4 +---
>  xen/include/asm-x86/mm.h        | 5 -----
>  3 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 5742cd05b8..04ac46239e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -193,8 +193,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>  
>          page_list_for_each ( page, &d->page_list )
>          {
> -            unsigned long mfn = mfn_x(page_to_mfn(page));
> -            unsigned long dfn = mfn_to_gmfn(d, mfn);
> +            mfn_t mfn = page_to_mfn(page);
> +            unsigned long dfn = mfn_to_gfn(d, mfn);
>              unsigned int mapping = IOMMUF_readable;
>              int ret;
>  
> @@ -203,7 +203,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>                    == PGT_writable_page) )
>                  mapping |= IOMMUF_writable;
>  
> -            ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping,
> +            ret = iommu_map(d, _dfn(dfn), mfn, 0, mapping,
>                              &flush_flags);
>  
>              if ( !rc )
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index a9c8352b94..a9cb98a6c7 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>  #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>  #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>  
> -/* Xen always owns P2M on ARM */
> +/* We don't have a M2P on Arm */
>  #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> -
>  
>  /* Arch-specific portion of memory_op hypercall. */
>  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);

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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Stefano Stabellini]

On Mon, 15 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
> > On Mon, 18 Feb 2019, Julien Grall wrote:
> > > mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
> > > the Arm code to use the former.
> > 
> > This is good but maybe we can go even further.
> > 
> > You should also be able to replace one call site of pfn_to_pdx in
> > mfn_valid and the one in maddr_to_virt. Something like this:
> > 
> > 
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index eafa26f..b3455ea 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
> > size_t len)
> >   /* XXX -- account for base */
> >   #define mfn_valid(mfn)        ({
> > \
> >       unsigned long __m_f_n = mfn_x(mfn);
> > \
> > -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
> > __mfn_valid(__m_f_n)); \
> > +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
> > \
> 
> This is quite undesirable, you will end up to evaluate mfn twice here.

Yes you are right


> The other solution is to turn _m_f_n to an mfn_t but then it does make much
> difference as you would need to use a mfn_x(mfn) in the code.

I think that would be better


> >   })
> >     /* Convert between machine frame numbers and page-info structures. */
> > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> >   #else
> >   static inline void *maddr_to_virt(paddr_t ma)
> >   {
> > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> >       return (void *)(XENHEAP_VIRT_START -
> >                       mfn_to_maddr(xenheap_mfn_start) +
> >                       ((ma & ma_va_bottom_mask) |
> > 
> 
> I fail to see what this chunk adds compare to the existing one...
> 
> > > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> > >   #else
> > >   static inline void *maddr_to_virt(paddr_t ma)
> > >   {
> > > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > >       return (void *)(XENHEAP_VIRT_START -
> > >                       mfn_to_maddr(xenheap_mfn_start) +
> > >                       ((ma & ma_va_bottom_mask) |
> ... here.

That's weird. I think `patch' didn't apply completely the patch to my
tree. Great you already have it.

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Stefano Stabellini]

On Mon, 15 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
> > On Mon, 18 Feb 2019, Julien Grall wrote:
> > > mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
> > > the Arm code to use the former.
> > 
> > This is good but maybe we can go even further.
> > 
> > You should also be able to replace one call site of pfn_to_pdx in
> > mfn_valid and the one in maddr_to_virt. Something like this:
> > 
> > 
> > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > index eafa26f..b3455ea 100644
> > --- a/xen/include/asm-arm/mm.h
> > +++ b/xen/include/asm-arm/mm.h
> > @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
> > size_t len)
> >   /* XXX -- account for base */
> >   #define mfn_valid(mfn)        ({
> > \
> >       unsigned long __m_f_n = mfn_x(mfn);
> > \
> > -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
> > __mfn_valid(__m_f_n)); \
> > +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
> > \
> 
> This is quite undesirable, you will end up to evaluate mfn twice here.

Yes you are right


> The other solution is to turn _m_f_n to an mfn_t but then it does make much
> difference as you would need to use a mfn_x(mfn) in the code.

I think that would be better


> >   })
> >     /* Convert between machine frame numbers and page-info structures. */
> > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> >   #else
> >   static inline void *maddr_to_virt(paddr_t ma)
> >   {
> > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> >       return (void *)(XENHEAP_VIRT_START -
> >                       mfn_to_maddr(xenheap_mfn_start) +
> >                       ((ma & ma_va_bottom_mask) |
> > 
> 
> I fail to see what this chunk adds compare to the existing one...
> 
> > > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma)
> > >   #else
> > >   static inline void *maddr_to_virt(paddr_t ma)
> > >   {
> > > -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > > +    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >>
> > > PAGE_SHIFT));
> > >       return (void *)(XENHEAP_VIRT_START -
> > >                       mfn_to_maddr(xenheap_mfn_start) +
> > >                       ((ma & ma_va_bottom_mask) |
> ... here.

That's weird. I think `patch' didn't apply completely the patch to my
tree. Great you already have it.

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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi,

On 4/15/19 11:25 PM, Stefano Stabellini wrote:
> On Mon, 15 Apr 2019, Julien Grall wrote:
>> Hi,
>>
>> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
>>> On Mon, 18 Feb 2019, Julien Grall wrote:
>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>>>> the Arm code to use the former.
>>>
>>> This is good but maybe we can go even further.
>>>
>>> You should also be able to replace one call site of pfn_to_pdx in
>>> mfn_valid and the one in maddr_to_virt. Something like this:
>>>
>>>
>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>> index eafa26f..b3455ea 100644
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
>>> size_t len)
>>>    /* XXX -- account for base */
>>>    #define mfn_valid(mfn)        ({
>>> \
>>>        unsigned long __m_f_n = mfn_x(mfn);
>>> \
>>> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
>>> __mfn_valid(__m_f_n)); \
>>> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
>>> \
>>
>> This is quite undesirable, you will end up to evaluate mfn twice here.
> 
> Yes you are right
> 
> 
>> The other solution is to turn _m_f_n to an mfn_t but then it does make much
>> difference as you would need to use a mfn_x(mfn) in the code.
> 
> I think that would be better

I should have probably said that I haven't done that in the patch 
because adding the mfn_x(...) is breached the 80-characters limit. So we 
would need to split the line.

I don't really think this will make easier to read the code in this context.

I am curious to know how this would make it better.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi,

On 4/15/19 11:25 PM, Stefano Stabellini wrote:
> On Mon, 15 Apr 2019, Julien Grall wrote:
>> Hi,
>>
>> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
>>> On Mon, 18 Feb 2019, Julien Grall wrote:
>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>>>> the Arm code to use the former.
>>>
>>> This is good but maybe we can go even further.
>>>
>>> You should also be able to replace one call site of pfn_to_pdx in
>>> mfn_valid and the one in maddr_to_virt. Something like this:
>>>
>>>
>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>> index eafa26f..b3455ea 100644
>>> --- a/xen/include/asm-arm/mm.h
>>> +++ b/xen/include/asm-arm/mm.h
>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
>>> size_t len)
>>>    /* XXX -- account for base */
>>>    #define mfn_valid(mfn)        ({
>>> \
>>>        unsigned long __m_f_n = mfn_x(mfn);
>>> \
>>> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
>>> __mfn_valid(__m_f_n)); \
>>> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
>>> \
>>
>> This is quite undesirable, you will end up to evaluate mfn twice here.
> 
> Yes you are right
> 
> 
>> The other solution is to turn _m_f_n to an mfn_t but then it does make much
>> difference as you would need to use a mfn_x(mfn) in the code.
> 
> I think that would be better

I should have probably said that I haven't done that in the patch 
because adding the mfn_x(...) is breached the 80-characters limit. So we 
would need to split the line.

I don't really think this will make easier to read the code in this context.

I am curious to know how this would make it better.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

(+ Andrew and Jan)

On 15/04/2019 23:42, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 11:25 PM, Stefano Stabellini wrote:
>> On Mon, 15 Apr 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
>>>> On Mon, 18 Feb 2019, Julien Grall wrote:
>>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>>>>> the Arm code to use the former.
>>>>
>>>> This is good but maybe we can go even further.
>>>>
>>>> You should also be able to replace one call site of pfn_to_pdx in
>>>> mfn_valid and the one in maddr_to_virt. Something like this:
>>>>
>>>>
>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>> index eafa26f..b3455ea 100644
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
>>>> size_t len)
>>>>    /* XXX -- account for base */
>>>>    #define mfn_valid(mfn)        ({
>>>> \
>>>>        unsigned long __m_f_n = mfn_x(mfn);
>>>> \
>>>> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
>>>> __mfn_valid(__m_f_n)); \
>>>> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
>>>> \
>>>
>>> This is quite undesirable, you will end up to evaluate mfn twice here.

I have been looking at making __mfn_valid(...) typesafe. While it compiles
on Arm, I have a build error on x86:

In file included from /home/julieng/works/xen/xen/include/asm/x86_64/page.h:47:0,
                 from /home/julieng/works/xen/xen/include/asm/page.h:23,
                 from /home/julieng/works/xen/xen/include/asm/current.h:12,
                 from /home/julieng/works/xen/xen/include/asm/smp.h:10,
                 from /home/julieng/works/xen/xen/include/xen/smp.h:4,
                 from /home/julieng/works/xen/xen/include/xen/perfc.h:7,
                 from x86_64/asm-offsets.c:8:
/home/julieng/works/xen/xen/include/xen/pdx.h:24:18: error: unknown type name ‘mfn_t’
 bool __mfn_valid(mfn_t mfn);
                  ^~~~~
In file included from /home/julieng/works/xen/xen/include/xen/config.h:13:0,
                 from <command-line>:0:
/home/julieng/works/xen/xen/include/asm/mm.h: In function ‘get_page_from_mfn’:
/home/julieng/works/xen/xen/include/asm/page.h:262:29: error: implicit declaration of function ‘__mfn_valid’ [-Werror=implicit-function-declaration]
 #define mfn_valid(mfn)      __mfn_valid(mfn)
                             ^
/home/julieng/works/xen/xen/include/xen/compiler.h:11:43: note: in definition of macro ‘unlikely’
 #define unlikely(x)   __builtin_expect(!!(x),0)

We get away on Arm because mfn_valid is implemented in mm.h. On x86 it is implemented
in page.h. I am quite impressed we never had build failure in common code until now...

Anyway, it is not the first time I see build error when trying to make the code using
typesafe gfn/mfn. The headers dependency are quite messy in general.

Andrew, Jan do you have a suggestion how to process on the x86 side?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

(+ Andrew and Jan)

On 15/04/2019 23:42, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 11:25 PM, Stefano Stabellini wrote:
>> On Mon, 15 Apr 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
>>>> On Mon, 18 Feb 2019, Julien Grall wrote:
>>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>>>>> the Arm code to use the former.
>>>>
>>>> This is good but maybe we can go even further.
>>>>
>>>> You should also be able to replace one call site of pfn_to_pdx in
>>>> mfn_valid and the one in maddr_to_virt. Something like this:
>>>>
>>>>
>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>> index eafa26f..b3455ea 100644
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
>>>> size_t len)
>>>>    /* XXX -- account for base */
>>>>    #define mfn_valid(mfn)        ({
>>>> \
>>>>        unsigned long __m_f_n = mfn_x(mfn);
>>>> \
>>>> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
>>>> __mfn_valid(__m_f_n)); \
>>>> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
>>>> \
>>>
>>> This is quite undesirable, you will end up to evaluate mfn twice here.

I have been looking at making __mfn_valid(...) typesafe. While it compiles
on Arm, I have a build error on x86:

In file included from /home/julieng/works/xen/xen/include/asm/x86_64/page.h:47:0,
                 from /home/julieng/works/xen/xen/include/asm/page.h:23,
                 from /home/julieng/works/xen/xen/include/asm/current.h:12,
                 from /home/julieng/works/xen/xen/include/asm/smp.h:10,
                 from /home/julieng/works/xen/xen/include/xen/smp.h:4,
                 from /home/julieng/works/xen/xen/include/xen/perfc.h:7,
                 from x86_64/asm-offsets.c:8:
/home/julieng/works/xen/xen/include/xen/pdx.h:24:18: error: unknown type name ‘mfn_t’
 bool __mfn_valid(mfn_t mfn);
                  ^~~~~
In file included from /home/julieng/works/xen/xen/include/xen/config.h:13:0,
                 from <command-line>:0:
/home/julieng/works/xen/xen/include/asm/mm.h: In function ‘get_page_from_mfn’:
/home/julieng/works/xen/xen/include/asm/page.h:262:29: error: implicit declaration of function ‘__mfn_valid’ [-Werror=implicit-function-declaration]
 #define mfn_valid(mfn)      __mfn_valid(mfn)
                             ^
/home/julieng/works/xen/xen/include/xen/compiler.h:11:43: note: in definition of macro ‘unlikely’
 #define unlikely(x)   __builtin_expect(!!(x),0)

We get away on Arm because mfn_valid is implemented in mm.h. On x86 it is implemented
in page.h. I am quite impressed we never had build failure in common code until now...

Anyway, it is not the first time I see build error when trying to make the code using
typesafe gfn/mfn. The headers dependency are quite messy in general.

Andrew, Jan do you have a suggestion how to process on the x86 side?

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

@Wei, @Ian: Do you have any input?

On 14/03/2019 07:55, Jan Beulich wrote:
>>>> On 13.03.19 at 18:30, <julien.grall@arm.com> wrote:
>> On 13/03/2019 15:20, Jan Beulich wrote:
>>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>> xen_domctl_getdomaininfo *info)
>>>>        info->outstanding_pages = d->outstanding_pages;
>>>>        info->shr_pages         = atomic_read(&d->shr_pages);
>>>>        info->paged_pages       = atomic_read(&d->paged_pages);
>>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>>>
>>> I think this change wants to be accompanied by a warning attached
>>> to the field declaration in the public header.
>>
>> Make sense.
>>
>>>
>>> But I'd also like to have the tool stack maintainers' view on making
>>> this field effectively unusable for Arm.
>>
>> The value in shared_info_frame was plain wrong since the creation of Xen Arm. So
>> this is just making the error more obvious. I don't expect any user of it on Arm.
> 
> Well, my request for tool stack maintainer input wasn't to put under
> question that the field can't currently be used sensibly on Arm.
> Instead I'm meaning to know whether it can be sensibly expected
> for the tool stack to want to use the field uniformly, in which case
> rather than making it more obviously not work it should be fixed
> instead.
> 
> Jan
> 
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

@Wei, @Ian: Do you have any input?

On 14/03/2019 07:55, Jan Beulich wrote:
>>>> On 13.03.19 at 18:30, <julien.grall@arm.com> wrote:
>> On 13/03/2019 15:20, Jan Beulich wrote:
>>>>>> On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
>>>> --- a/xen/common/domctl.c
>>>> +++ b/xen/common/domctl.c
>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
>> xen_domctl_getdomaininfo *info)
>>>>        info->outstanding_pages = d->outstanding_pages;
>>>>        info->shr_pages         = atomic_read(&d->shr_pages);
>>>>        info->paged_pages       = atomic_read(&d->paged_pages);
>>>> -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
>>>> +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
>>>
>>> I think this change wants to be accompanied by a warning attached
>>> to the field declaration in the public header.
>>
>> Make sense.
>>
>>>
>>> But I'd also like to have the tool stack maintainers' view on making
>>> this field effectively unusable for Arm.
>>
>> The value in shared_info_frame was plain wrong since the creation of Xen Arm. So
>> this is just making the error more obvious. I don't expect any user of it on Arm.
> 
> Well, my request for tool stack maintainer input wasn't to put under
> question that the field can't currently be used sensibly on Arm.
> Instead I'm meaning to know whether it can be sensibly expected
> for the tool stack to want to use the field uniformly, in which case
> rather than making it more obviously not work it should be fixed
> instead.
> 
> Jan
> 
> 

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Wei Liu]

On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
> Hi,
> 
> @Wei, @Ian: Do you have any input?

Sorry I haven't been following this closely, but shouldn't we fix the
interface to return gfn instead? And then the mapping of it should
interpret the value properly per guest type.

I don't see immediately how user space program will need to access this
page for autotranslated guests though.

Wei.

> 
> On 14/03/2019 07:55, Jan Beulich wrote:
> > > > > On 13.03.19 at 18:30, <julien.grall@arm.com> wrote:
> > > On 13/03/2019 15:20, Jan Beulich wrote:
> > > > > > > On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> > > > > --- a/xen/common/domctl.c
> > > > > +++ b/xen/common/domctl.c
> > > > > @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
> > > xen_domctl_getdomaininfo *info)
> > > > >        info->outstanding_pages = d->outstanding_pages;
> > > > >        info->shr_pages         = atomic_read(&d->shr_pages);
> > > > >        info->paged_pages       = atomic_read(&d->paged_pages);
> > > > > -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> > > > > +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
> > > > 
> > > > I think this change wants to be accompanied by a warning attached
> > > > to the field declaration in the public header.
> > > 
> > > Make sense.
> > > 
> > > > 
> > > > But I'd also like to have the tool stack maintainers' view on making
> > > > this field effectively unusable for Arm.
> > > 
> > > The value in shared_info_frame was plain wrong since the creation of Xen Arm. So
> > > this is just making the error more obvious. I don't expect any user of it on Arm.
> > 
> > Well, my request for tool stack maintainer input wasn't to put under
> > question that the field can't currently be used sensibly on Arm.
> > Instead I'm meaning to know whether it can be sensibly expected
> > for the tool stack to want to use the field uniformly, in which case
> > rather than making it more obviously not work it should be fixed
> > instead.
> > 
> > Jan
> > 
> > 
> 
> -- 
> Julien Grall

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Wei Liu]

On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
> Hi,
> 
> @Wei, @Ian: Do you have any input?

Sorry I haven't been following this closely, but shouldn't we fix the
interface to return gfn instead? And then the mapping of it should
interpret the value properly per guest type.

I don't see immediately how user space program will need to access this
page for autotranslated guests though.

Wei.

> 
> On 14/03/2019 07:55, Jan Beulich wrote:
> > > > > On 13.03.19 at 18:30, <julien.grall@arm.com> wrote:
> > > On 13/03/2019 15:20, Jan Beulich wrote:
> > > > > > > On 18.02.19 at 12:35, <julien.grall@arm.com> wrote:
> > > > > --- a/xen/common/domctl.c
> > > > > +++ b/xen/common/domctl.c
> > > > > @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct
> > > xen_domctl_getdomaininfo *info)
> > > > >        info->outstanding_pages = d->outstanding_pages;
> > > > >        info->shr_pages         = atomic_read(&d->shr_pages);
> > > > >        info->paged_pages       = atomic_read(&d->paged_pages);
> > > > > -    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
> > > > > +    info->shared_info_frame = gfn_x(domain_shared_info_gfn(d));
> > > > 
> > > > I think this change wants to be accompanied by a warning attached
> > > > to the field declaration in the public header.
> > > 
> > > Make sense.
> > > 
> > > > 
> > > > But I'd also like to have the tool stack maintainers' view on making
> > > > this field effectively unusable for Arm.
> > > 
> > > The value in shared_info_frame was plain wrong since the creation of Xen Arm. So
> > > this is just making the error more obvious. I don't expect any user of it on Arm.
> > 
> > Well, my request for tool stack maintainer input wasn't to put under
> > question that the field can't currently be used sensibly on Arm.
> > Instead I'm meaning to know whether it can be sensibly expected
> > for the tool stack to want to use the field uniformly, in which case
> > rather than making it more obviously not work it should be fixed
> > instead.
> > 
> > Jan
> > 
> > 
> 
> -- 
> Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

On 18/04/2019 12:46, Wei Liu wrote:
> On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
>> Hi,
>>
>> @Wei, @Ian: Do you have any input?
> 
> Sorry I haven't been following this closely, but shouldn't we fix the
> interface to return gfn instead? And then the mapping of it should
> interpret the value properly per guest type.

We already return a GFN today. The problem is we only hold an MFN at that time.

At the moment, we rely on the M2P to find the corresponding GFN. As we don't 
have an M2P on Arm, we would have to store the associated GFN.

But all this logic is a bit broken. It relies on the toolstack (or the guest) to 
have mapped the shared info frame in the P2M using the physmap hypervisor with 
XENMAPSPACE_shared_info beforehand. This is also racy as you may return a GFN 
that was unmapped/remapped to something else before the toolstack has a chance 
to map in its address space the page.

The correct solution would be to phase out the field shared_info_frame and use 
XENMEM_acquire_resource instead. However, this requires the associated ioctl to 
be implemented in Linux.

> 
> I don't see immediately how user space program will need to access this
> page for autotranslated guests though.

If there are no use, then I am not convinced it is worth trying to make 
shared_info_frame working on Arm.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

On 18/04/2019 12:46, Wei Liu wrote:
> On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
>> Hi,
>>
>> @Wei, @Ian: Do you have any input?
> 
> Sorry I haven't been following this closely, but shouldn't we fix the
> interface to return gfn instead? And then the mapping of it should
> interpret the value properly per guest type.

We already return a GFN today. The problem is we only hold an MFN at that time.

At the moment, we rely on the M2P to find the corresponding GFN. As we don't 
have an M2P on Arm, we would have to store the associated GFN.

But all this logic is a bit broken. It relies on the toolstack (or the guest) to 
have mapped the shared info frame in the P2M using the physmap hypervisor with 
XENMAPSPACE_shared_info beforehand. This is also racy as you may return a GFN 
that was unmapped/remapped to something else before the toolstack has a chance 
to map in its address space the page.

The correct solution would be to phase out the field shared_info_frame and use 
XENMEM_acquire_resource instead. However, this requires the associated ioctl to 
be implemented in Linux.

> 
> I don't see immediately how user space program will need to access this
> page for autotranslated guests though.

If there are no use, then I am not convinced it is worth trying to make 
shared_info_frame working on Arm.

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

@Ian, @Wei: Gentle ping.

On 18/04/2019 16:09, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 12:46, Wei Liu wrote:
>> On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
>>> Hi,
>>>
>>> @Wei, @Ian: Do you have any input?
>>
>> Sorry I haven't been following this closely, but shouldn't we fix the
>> interface to return gfn instead? And then the mapping of it should
>> interpret the value properly per guest type.
> 
> We already return a GFN today. The problem is we only hold an MFN at that time.
> 
> At the moment, we rely on the M2P to find the corresponding GFN. As we don't 
> have an M2P on Arm, we would have to store the associated GFN.
> 
> But all this logic is a bit broken. It relies on the toolstack (or the guest) to 
> have mapped the shared info frame in the P2M using the physmap hypervisor with 
> XENMAPSPACE_shared_info beforehand. This is also racy as you may return a GFN 
> that was unmapped/remapped to something else before the toolstack has a chance 
> to map in its address space the page.
> 
> The correct solution would be to phase out the field shared_info_frame and use 
> XENMEM_acquire_resource instead. However, this requires the associated ioctl to 
> be implemented in Linux.
> 
>>
>> I don't see immediately how user space program will need to access this
>> page for autotranslated guests though.
> 
> If there are no use, then I am not convinced it is worth trying to make 
> shared_info_frame working on Arm.
> 
> Cheers,
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Julien Grall]

Hi,

@Ian, @Wei: Gentle ping.

On 18/04/2019 16:09, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 12:46, Wei Liu wrote:
>> On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
>>> Hi,
>>>
>>> @Wei, @Ian: Do you have any input?
>>
>> Sorry I haven't been following this closely, but shouldn't we fix the
>> interface to return gfn instead? And then the mapping of it should
>> interpret the value properly per guest type.
> 
> We already return a GFN today. The problem is we only hold an MFN at that time.
> 
> At the moment, we rely on the M2P to find the corresponding GFN. As we don't 
> have an M2P on Arm, we would have to store the associated GFN.
> 
> But all this logic is a bit broken. It relies on the toolstack (or the guest) to 
> have mapped the shared info frame in the P2M using the physmap hypervisor with 
> XENMAPSPACE_shared_info beforehand. This is also racy as you may return a GFN 
> that was unmapped/remapped to something else before the toolstack has a chance 
> to map in its address space the page.
> 
> The correct solution would be to phase out the field shared_info_frame and use 
> XENMEM_acquire_resource instead. However, this requires the associated ioctl to 
> be implemented in Linux.
> 
>>
>> I don't see immediately how user space program will need to access this
>> page for autotranslated guests though.
> 
> If there are no use, then I am not convinced it is worth trying to make 
> shared_info_frame working on Arm.
> 
> Cheers,
> 

-- 
Julien Grall

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Wei Liu]

On Thu, Apr 18, 2019 at 04:09:21PM +0100, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 12:46, Wei Liu wrote:
> > On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
> > > Hi,
> > > 
> > > @Wei, @Ian: Do you have any input?
> > 
> > Sorry I haven't been following this closely, but shouldn't we fix the
> > interface to return gfn instead? And then the mapping of it should
> > interpret the value properly per guest type.
> 
> We already return a GFN today. The problem is we only hold an MFN at that time.
> 
> At the moment, we rely on the M2P to find the corresponding GFN. As we don't
> have an M2P on Arm, we would have to store the associated GFN.
> 
> But all this logic is a bit broken. It relies on the toolstack (or the
> guest) to have mapped the shared info frame in the P2M using the physmap
> hypervisor with XENMAPSPACE_shared_info beforehand. This is also racy as you
> may return a GFN that was unmapped/remapped to something else before the
> toolstack has a chance to map in its address space the page.
> 
> The correct solution would be to phase out the field shared_info_frame and
> use XENMEM_acquire_resource instead. However, this requires the associated
> ioctl to be implemented in Linux.

OK. That seems to be a sensible way forward.

Wei.

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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Wei Liu]

On Thu, Apr 18, 2019 at 04:09:21PM +0100, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 12:46, Wei Liu wrote:
> > On Wed, Apr 17, 2019 at 06:42:47PM +0100, Julien Grall wrote:
> > > Hi,
> > > 
> > > @Wei, @Ian: Do you have any input?
> > 
> > Sorry I haven't been following this closely, but shouldn't we fix the
> > interface to return gfn instead? And then the mapping of it should
> > interpret the value properly per guest type.
> 
> We already return a GFN today. The problem is we only hold an MFN at that time.
> 
> At the moment, we rely on the M2P to find the corresponding GFN. As we don't
> have an M2P on Arm, we would have to store the associated GFN.
> 
> But all this logic is a bit broken. It relies on the toolstack (or the
> guest) to have mapped the shared info frame in the P2M using the physmap
> hypervisor with XENMAPSPACE_shared_info beforehand. This is also racy as you
> may return a GFN that was unmapped/remapped to something else before the
> toolstack has a chance to map in its address space the page.
> 
> The correct solution would be to phase out the field shared_info_frame and
> use XENMEM_acquire_resource instead. However, this requires the associated
> ioctl to be implemented in Linux.

OK. That seems to be a sensible way forward.

Wei.

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

Re: [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Jan Beulich]

>>> On 16.04.19 at 00:17, <sstabellini@kernel.org> wrote:
> On Wed, 13 Mar 2019, Jan Beulich wrote:
>> > @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>> >          /* Use while-break to avoid compiler warning */
>> >          while ( iommu_iotlb_flush_all(d, flush_flags) )
>> >              break;
>> > +#else
>> > +        rc = -ENOSYS;
>> 
>> -EOPNOTSUPP please.
> 
> The patch is fine by me and I am also fine with Jan's comments.
> 
> I only want to point out that we haven't been entirely consistent with
> -ENOSYS and/or -EOPNOTSUPP. We have lots of places that return -ENOSYS.
> Should we change them to -EOPNOTSUPP going forward?

Ideally we would, but iirc a small step I had once made in that
direction did face resistance. Hence I've changed to the mode
of simply trying to keep new instances of ENOSYS abuse from
going in.

Jan



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

Re: [Xen-devel] [PATCH for-next 8/9] xen: Introduce HAS_M2P config and use to protect mfn_to_gmfn call

[Jan Beulich]

>>> On 16.04.19 at 00:17, <sstabellini@kernel.org> wrote:
> On Wed, 13 Mar 2019, Jan Beulich wrote:
>> > @@ -215,6 +216,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>> >          /* Use while-break to avoid compiler warning */
>> >          while ( iommu_iotlb_flush_all(d, flush_flags) )
>> >              break;
>> > +#else
>> > +        rc = -ENOSYS;
>> 
>> -EOPNOTSUPP please.
> 
> The patch is fine by me and I am also fine with Jan's comments.
> 
> I only want to point out that we haven't been entirely consistent with
> -ENOSYS and/or -EOPNOTSUPP. We have lots of places that return -ENOSYS.
> Should we change them to -EOPNOTSUPP going forward?

Ideally we would, but iirc a small step I had once made in that
direction did face resistance. Hence I've changed to the mode
of simply trying to keep new instances of ENOSYS abuse from
going in.

Jan



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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Jan Beulich]

>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote:
> Anyway, it is not the first time I see build error when trying to make the code using
> typesafe gfn/mfn. The headers dependency are quite messy in general.

Because of this, ...

> Andrew, Jan do you have a suggestion how to process on the x86 side?

... I don't, I'm sorry, without being able to invest some time to
play with this myself.

Jan



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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Jan Beulich]

>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote:
> Anyway, it is not the first time I see build error when trying to make the code using
> typesafe gfn/mfn. The headers dependency are quite messy in general.

Because of this, ...

> Andrew, Jan do you have a suggestion how to process on the x86 side?

... I don't, I'm sorry, without being able to invest some time to
play with this myself.

Jan



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

Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi Jan,

On 25/04/2019 12:20, Jan Beulich wrote:
>>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote:
>> Anyway, it is not the first time I see build error when trying to make the code using
>> typesafe gfn/mfn. The headers dependency are quite messy in general.
> 
> Because of this, ...
> 
>> Andrew, Jan do you have a suggestion how to process on the x86 side?
> 
> ... I don't, I'm sorry, without being able to invest some time to
> play with this myself.

I thought I would CCed you just in case you have an idea to quickly fix it. 
Anyway, this is more a cleanup and not a priority for me.

Cheers,


-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall]

Hi Jan,

On 25/04/2019 12:20, Jan Beulich wrote:
>>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote:
>> Anyway, it is not the first time I see build error when trying to make the code using
>> typesafe gfn/mfn. The headers dependency are quite messy in general.
> 
> Because of this, ...
> 
>> Andrew, Jan do you have a suggestion how to process on the x86 side?
> 
> ... I don't, I'm sorry, without being able to invest some time to
> play with this myself.

I thought I would CCed you just in case you have an idea to quickly fix it. 
Anyway, this is more a cleanup and not a priority for me.

Cheers,


-- 
Julien Grall

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

Re: [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

Hi,

On 3/13/19 3:22 PM, Jan Beulich wrote:
>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>   #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>   
>> -/* Xen always owns P2M on ARM */
>> +/* We don't have a M2P on Arm */
>>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> 
> So is the plan to remove the other macro from Arm then as well?
> In any event
> Acked-by: Jan Beulich <jbeulich@suse.com>

Just to keep everyone aware, I kept this patch as is with the 2 acked-by 
and provided a separate patch to move the helpers in common code under 
!CONFIG_HAVE_M2P.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH for-next 9/9] xen: Remove mfn_to_gmfn macro

[Julien Grall]

Hi,

On 3/13/19 3:22 PM, Jan Beulich wrote:
>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote:
>> --- a/xen/include/asm-arm/mm.h
>> +++ b/xen/include/asm-arm/mm.h
>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
>>   #define SHARED_M2P_ENTRY         (~0UL - 1UL)
>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>   
>> -/* Xen always owns P2M on ARM */
>> +/* We don't have a M2P on Arm */
>>   #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
>> -#define mfn_to_gmfn(_d, mfn)  (mfn)
> 
> So is the plan to remove the other macro from Arm then as well?
> In any event
> Acked-by: Jan Beulich <jbeulich@suse.com>

Just to keep everyone aware, I kept this patch as is with the 2 acked-by 
and provided a separate patch to move the helpers in common code under 
!CONFIG_HAVE_M2P.

Cheers,

-- 
Julien Grall

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