diff mbox

[Xen-devel,v3,6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

Message ID 1466515243-27264-7-git-send-email-julien.grall@arm.com
State Superseded
Headers show

Commit Message

Julien Grall June 21, 2016, 1:20 p.m. UTC 
to avoid mixing machine frame with guest frame.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v3:
        - Use mfn_add when it is possible
        - Add Jan's acked-by
---
 xen/arch/arm/domain_build.c      |  4 ++--
 xen/arch/arm/gic-v2.c            |  4 ++--
 xen/arch/arm/p2m.c               | 22 +++++++++++-----------
 xen/arch/arm/platforms/exynos5.c |  8 ++++----
 xen/arch/arm/platforms/omap5.c   | 16 ++++++++--------
 xen/arch/arm/vgic-v2.c           |  4 ++--
 xen/arch/x86/mm/p2m.c            | 18 ++++++++++--------
 xen/common/domctl.c              |  4 ++--
 xen/include/xen/p2m-common.h     |  8 ++++----
 9 files changed, 45 insertions(+), 43 deletions(-)

Comments

Julien Grall June 23, 2016, 2:08 p.m. UTC | #1 
On 23/06/16 15:05, Stefano Stabellini wrote:
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index aa4e774..47cb383 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
>>   }
>>
>>   int map_mmio_regions(struct domain *d,
>> -                     unsigned long start_gfn,
>> +                     gfn_t start_gfn,
>>                        unsigned long nr,
>> -                     unsigned long mfn)
>> +                     mfn_t mfn)
>>   {
>>       return apply_p2m_changes(d, INSERT,
>> -                             pfn_to_paddr(start_gfn),
>> -                             pfn_to_paddr(start_gfn + nr),
>> -                             pfn_to_paddr(mfn),
>> +                             pfn_to_paddr(gfn_x(start_gfn)),
>> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
>> +                             pfn_to_paddr(mfn_x(mfn)),
>>                                MATTR_DEV, 0, p2m_mmio_direct,
>>                                d->arch.p2m.default_access);
>
> Any reason why you didn't push these changes down to apply_p2m_changes too?

To keep this series simple. I have another series coming up to push the 
change down to apply_p2m_changes and clean up the P2M code.

I can move the patch to push down the change in this series if you prefer.

Cheers,
Julien Grall June 23, 2016, 2:58 p.m. UTC | #2 
On 23/06/16 15:15, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Julien Grall wrote:
>> On 23/06/16 15:05, Stefano Stabellini wrote:
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index aa4e774..47cb383 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
>>>>    }
>>>>
>>>>    int map_mmio_regions(struct domain *d,
>>>> -                     unsigned long start_gfn,
>>>> +                     gfn_t start_gfn,
>>>>                         unsigned long nr,
>>>> -                     unsigned long mfn)
>>>> +                     mfn_t mfn)
>>>>    {
>>>>        return apply_p2m_changes(d, INSERT,
>>>> -                             pfn_to_paddr(start_gfn),
>>>> -                             pfn_to_paddr(start_gfn + nr),
>>>> -                             pfn_to_paddr(mfn),
>>>> +                             pfn_to_paddr(gfn_x(start_gfn)),
>>>> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
>>>> +                             pfn_to_paddr(mfn_x(mfn)),
>>>>                                 MATTR_DEV, 0, p2m_mmio_direct,
>>>>                                 d->arch.p2m.default_access);
>>>
>>> Any reason why you didn't push these changes down to apply_p2m_changes too?
>>
>> To keep this series simple. I have another series coming up to push the change
>> down to apply_p2m_changes and clean up the P2M code.
>>
>> I can move the patch to push down the change in this series if you prefer.
>
> Yeah, it makes sense to keep them together.

Well, I still plan to have a different patch to push down the change. 
Switching from unsigned long to gfn/mfn is a long work which need to be 
split to ease the review.

I will see what I can do.

Cheers,
diff mbox

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9035486..49185f0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1036,9 +1036,9 @@  static int map_range_to_domain(const struct dt_device_node *dev,
     if ( need_mapping )
     {
         res = map_mmio_regions(d,
-                               paddr_to_pfn(addr),
+                               _gfn(paddr_to_pfn(addr)),
                                DIV_ROUND_UP(len, PAGE_SIZE),
-                               paddr_to_pfn(addr));
+                               _mfn(paddr_to_pfn(addr)));
         if ( res < 0 )
         {
             printk(XENLOG_ERR "Unable to map 0x%"PRIx64
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 4e2f4c7..3893ece 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -601,9 +601,9 @@  static int gicv2_map_hwdown_extra_mappings(struct domain *d)
                d->domain_id, v2m_data->addr, v2m_data->size,
                v2m_data->spi_start, v2m_data->nr_spis);
 
-        ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr),
+        ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)),
                             DIV_ROUND_UP(v2m_data->size, PAGE_SIZE),
-                            paddr_to_pfn(v2m_data->addr));
+                            _mfn(paddr_to_pfn(v2m_data->addr)));
         if ( ret )
         {
             printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index aa4e774..47cb383 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1245,27 +1245,27 @@  int unmap_regions_rw_cache(struct domain *d,
 }
 
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn)
+                     mfn_t mfn)
 {
     return apply_p2m_changes(d, INSERT,
-                             pfn_to_paddr(start_gfn),
-                             pfn_to_paddr(start_gfn + nr),
-                             pfn_to_paddr(mfn),
+                             pfn_to_paddr(gfn_x(start_gfn)),
+                             pfn_to_paddr(gfn_x(start_gfn) + nr),
+                             pfn_to_paddr(mfn_x(mfn)),
                              MATTR_DEV, 0, p2m_mmio_direct,
                              d->arch.p2m.default_access);
 }
 
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn)
+                       mfn_t mfn)
 {
     return apply_p2m_changes(d, REMOVE,
-                             pfn_to_paddr(start_gfn),
-                             pfn_to_paddr(start_gfn + nr),
-                             pfn_to_paddr(mfn),
+                             pfn_to_paddr(gfn_x(start_gfn)),
+                             pfn_to_paddr(gfn_x(start_gfn) + nr),
+                             pfn_to_paddr(mfn_x(mfn)),
                              MATTR_DEV, 0, p2m_invalid,
                              d->arch.p2m.default_access);
 }
@@ -1280,7 +1280,7 @@  int map_dev_mmio_region(struct domain *d,
     if ( !(nr && iomem_access_permitted(d, start_gfn, start_gfn + nr - 1)) )
         return 0;
 
-    res = map_mmio_regions(d, start_gfn, nr, mfn);
+    res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn));
     if ( res < 0 )
     {
         printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n",
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index bf4964d..c43934f 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -83,12 +83,12 @@  static int exynos5_init_time(void)
 static int exynos5250_specific_mapping(struct domain *d)
 {
     /* Map the chip ID */
-    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1,
-                     paddr_to_pfn(EXYNOS5_PA_CHIPID));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)), 1,
+                     _mfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)));
 
     /* Map the PWM region */
-    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER), 2,
-                     paddr_to_pfn(EXYNOS5_PA_TIMER));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_TIMER)), 2,
+                     _mfn(paddr_to_pfn(EXYNOS5_PA_TIMER)));
 
     return 0;
 }
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index a49ba62..539588e 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,20 +102,20 @@  static int omap5_init_time(void)
 static int omap5_specific_mapping(struct domain *d)
 {
     /* Map the PRM module */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE), 2,
-                     paddr_to_pfn(OMAP5_PRM_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRM_BASE)), 2,
+                     _mfn(paddr_to_pfn(OMAP5_PRM_BASE)));
 
     /* Map the PRM_MPU */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE), 1,
-                     paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)), 1,
+                     _mfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)));
 
     /* Map the Wakeup Gen */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE), 1,
-                     paddr_to_pfn(OMAP5_WKUPGEN_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)), 1,
+                     _mfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)));
 
     /* Map the on-chip SRAM */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA), 32,
-                     paddr_to_pfn(OMAP5_SRAM_PA));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_SRAM_PA)), 32,
+                     _mfn(paddr_to_pfn(OMAP5_SRAM_PA)));
 
     return 0;
 }
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 9adb4a9..cbe61cf 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -688,8 +688,8 @@  static int vgic_v2_domain_init(struct domain *d)
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
      */
-    ret = map_mmio_regions(d, paddr_to_pfn(cbase), csize / PAGE_SIZE,
-                           paddr_to_pfn(vbase));
+    ret = map_mmio_regions(d, _gfn(paddr_to_pfn(cbase)), csize / PAGE_SIZE,
+                           _mfn(paddr_to_pfn(vbase)));
     if ( ret )
         return ret;
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16733a4..6258a5b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2214,9 +2214,9 @@  static unsigned int mmio_order(const struct domain *d,
 #define MAP_MMIO_MAX_ITER 64 /* pretty arbitrary */
 
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn)
+                     mfn_t mfn)
 {
     int ret = 0;
     unsigned long i;
@@ -2229,10 +2229,11 @@  int map_mmio_regions(struct domain *d,
           i += 1UL << order, ++iter )
     {
         /* OR'ing gfn and mfn values will return an order suitable to both. */
-        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
+        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
               order = ret - 1 )
         {
-            ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order,
+            ret = set_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
+                                     mfn_add(mfn, i), order,
                                      p2m_get_hostp2m(d)->default_access);
             if ( ret <= 0 )
                 break;
@@ -2246,9 +2247,9 @@  int map_mmio_regions(struct domain *d,
 }
 
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn)
+                       mfn_t mfn)
 {
     int ret = 0;
     unsigned long i;
@@ -2261,10 +2262,11 @@  int unmap_mmio_regions(struct domain *d,
           i += 1UL << order, ++iter )
     {
         /* OR'ing gfn and mfn values will return an order suitable to both. */
-        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
+        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
               order = ret - 1 )
         {
-            ret = clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order);
+            ret = clear_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
+                                       mfn_add(mfn, i), order);
             if ( ret <= 0 )
                 break;
             ASSERT(ret <= order);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..b784e6c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1074,7 +1074,7 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                    "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            ret = map_mmio_regions(d, gfn, nr_mfns, mfn);
+            ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
             if ( ret < 0 )
                 printk(XENLOG_G_WARNING
                        "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
@@ -1086,7 +1086,7 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                    "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            ret = unmap_mmio_regions(d, gfn, nr_mfns, mfn);
+            ret = unmap_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
             if ( ret < 0 && is_hardware_domain(current->domain) )
                 printk(XENLOG_ERR
                        "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 6374a5b..b4f9077 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -37,13 +37,13 @@  typedef enum {
  *  * the guest physical address space to map, starting from the machine
  *   * frame number mfn. */
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn);
+                     mfn_t mfn);
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn);
+                       mfn_t mfn);
 
 /*
  * Set access type for a region of gfns.