Re: [Xen-devel] [PATCH v2 03/16] xen/arm: mm: Use typesafe mfn for xenheap_mfn_*

[Stefano Stabellini]

On Wed, 21 Jun 2017, Stefano Stabellini wrote:
> On Mon, 19 Jun 2017, Julien Grall wrote:
> > Add more safety when using xenheap_mfn_*.
> > 
> > Signed-off-by: Julien Grall 
> 
> Reviewed-by: Stefano Stabellini 
> 
> 
> > ---
> > 
> > I haven't introduced mfn_less_than() and mfn_greather_than() as
> > there would be only a couple of usage. We would be able to introduce
> > them and replace the open-coding easily in the future grepping
> > mfn_x().
> > ---
> >  xen/arch/arm/mm.c| 16 
> >  xen/arch/arm/setup.c | 18 +-
> >  xen/include/asm-arm/mm.h | 11 ++-
> >  3 files changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 7b313ca123..452c1e26c3 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -138,8 +138,8 @@ uint64_t init_ttbr;
> >  static paddr_t phys_offset;
> >  
> >  /* Limits of the Xen heap */
> > -unsigned long xenheap_mfn_start __read_mostly = ~0UL;
> > -unsigned long xenheap_mfn_end __read_mostly;
> > +mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN;
> > +mfn_t xenheap_mfn_end __read_mostly;

Actually I get the following build error with
gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu:

mm.c:141:1: error: initializer element is not constant
 mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN;
 ^
make[4]: *** [mm.o] Error 1

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 03/16] xen/arm: mm: Use typesafe mfn for xenheap_mfn_*

[Stefano Stabellini]

On Mon, 19 Jun 2017, Julien Grall wrote:
> Add more safety when using xenheap_mfn_*.
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Stefano Stabellini 


> ---
> 
> I haven't introduced mfn_less_than() and mfn_greather_than() as
> there would be only a couple of usage. We would be able to introduce
> them and replace the open-coding easily in the future grepping
> mfn_x().
> ---
>  xen/arch/arm/mm.c| 16 
>  xen/arch/arm/setup.c | 18 +-
>  xen/include/asm-arm/mm.h | 11 ++-
>  3 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7b313ca123..452c1e26c3 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -138,8 +138,8 @@ uint64_t init_ttbr;
>  static paddr_t phys_offset;
>  
>  /* Limits of the Xen heap */
> -unsigned long xenheap_mfn_start __read_mostly = ~0UL;
> -unsigned long xenheap_mfn_end __read_mostly;
> +mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN;
> +mfn_t xenheap_mfn_end __read_mostly;
>  vaddr_t xenheap_virt_end __read_mostly;
>  #ifdef CONFIG_ARM_64
>  vaddr_t xenheap_virt_start __read_mostly;
> @@ -801,8 +801,8 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>  
>  /* Record where the xenheap is, for translation routines. */
>  xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> -xenheap_mfn_start = base_mfn;
> -xenheap_mfn_end = base_mfn + nr_mfns;
> +xenheap_mfn_start = _mfn(base_mfn);
> +xenheap_mfn_end = _mfn(base_mfn + nr_mfns);
>  }
>  #else /* CONFIG_ARM_64 */
>  void __init setup_xenheap_mappings(unsigned long base_mfn,
> @@ -816,16 +816,16 @@ void __init setup_xenheap_mappings(unsigned long 
> base_mfn,
>  mfn = base_mfn & ~((FIRST_SIZE>>PAGE_SHIFT)-1);
>  
>  /* First call sets the xenheap physical and virtual offset. */
> -if ( xenheap_mfn_start == ~0UL )
> +if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) )
>  {
> -xenheap_mfn_start = base_mfn;
> +xenheap_mfn_start = _mfn(base_mfn);
>  xenheap_virt_start = DIRECTMAP_VIRT_START +
>  (base_mfn - mfn) * PAGE_SIZE;
>  }
>  
> -if ( base_mfn < xenheap_mfn_start )
> +if ( base_mfn < mfn_x(xenheap_mfn_start) )
>  panic("cannot add xenheap mapping at %lx below heap start %lx",
> -  base_mfn, xenheap_mfn_start);
> +  base_mfn, mfn_x(xenheap_mfn_start));
>  
>  end_mfn = base_mfn + nr_mfns;
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ab4d8e4218..3b34855668 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -555,8 +555,8 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>   * and enough mapped pages for copying the DTB.
>   */
>  dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT;
> -boot_mfn_start = xenheap_mfn_end - dtb_pages - 1;
> -boot_mfn_end = xenheap_mfn_end;
> +boot_mfn_start = mfn_x(xenheap_mfn_end) - dtb_pages - 1;
> +boot_mfn_end = mfn_x(xenheap_mfn_end);
>  
>  init_boot_pages(pfn_to_paddr(boot_mfn_start), 
> pfn_to_paddr(boot_mfn_end));
>  
> @@ -591,11 +591,11 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>  e = bank_end;
>  
>  /* Avoid the xenheap */
> -if ( s < pfn_to_paddr(xenheap_mfn_start+xenheap_pages)
> - && pfn_to_paddr(xenheap_mfn_start) < e )
> +if ( s < mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages))
> + && mfn_to_maddr(xenheap_mfn_start) < e )
>  {
> -e = pfn_to_paddr(xenheap_mfn_start);
> -n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
> +e = mfn_to_maddr(xenheap_mfn_start);
> +n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
>  }
>  
>  dt_unreserved_regions(s, e, init_boot_pages, 0);
> @@ -610,7 +610,7 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>  
>  /* Add xenheap memory that was not already added to the boot
> allocator. */
> -init_xenheap_pages(pfn_to_paddr(xenheap_mfn_start),
> +init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
> pfn_to_paddr(boot_mfn_start));
>  }
>  #else /* CONFIG_ARM_64 */
> @@ -662,8 +662,8 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>  total_pages += ram_size >> PAGE_SHIFT;
>  
>  xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
> -xenheap_mfn_start = ram_start >> PAGE_SHIFT;
> -xenheap_mfn_end = ram_end >> PAGE_SHIFT;
> +xenheap_mfn_start = maddr_to_mfn(ram_start);
> +xenheap_mfn_end = maddr_to_mfn(ram_end);
>  
>  /*
>   * Need enough mapped pages for copying the DTB.
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 

[Xen-devel] [PATCH v2 03/16] xen/arm: mm: Use typesafe mfn for xenheap_mfn_*

[Julien Grall]

Add more safety when using xenheap_mfn_*.

Signed-off-by: Julien Grall 
---

I haven't introduced mfn_less_than() and mfn_greather_than() as
there would be only a couple of usage. We would be able to introduce
them and replace the open-coding easily in the future grepping
mfn_x().
---
 xen/arch/arm/mm.c| 16 
 xen/arch/arm/setup.c | 18 +-
 xen/include/asm-arm/mm.h | 11 ++-
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7b313ca123..452c1e26c3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -138,8 +138,8 @@ uint64_t init_ttbr;
 static paddr_t phys_offset;
 
 /* Limits of the Xen heap */
-unsigned long xenheap_mfn_start __read_mostly = ~0UL;
-unsigned long xenheap_mfn_end __read_mostly;
+mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN;
+mfn_t xenheap_mfn_end __read_mostly;
 vaddr_t xenheap_virt_end __read_mostly;
 #ifdef CONFIG_ARM_64
 vaddr_t xenheap_virt_start __read_mostly;
@@ -801,8 +801,8 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 
 /* Record where the xenheap is, for translation routines. */
 xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
-xenheap_mfn_start = base_mfn;
-xenheap_mfn_end = base_mfn + nr_mfns;
+xenheap_mfn_start = _mfn(base_mfn);
+xenheap_mfn_end = _mfn(base_mfn + nr_mfns);
 }
 #else /* CONFIG_ARM_64 */
 void __init setup_xenheap_mappings(unsigned long base_mfn,
@@ -816,16 +816,16 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 mfn = base_mfn & ~((FIRST_SIZE>>PAGE_SHIFT)-1);
 
 /* First call sets the xenheap physical and virtual offset. */
-if ( xenheap_mfn_start == ~0UL )
+if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) )
 {
-xenheap_mfn_start = base_mfn;
+xenheap_mfn_start = _mfn(base_mfn);
 xenheap_virt_start = DIRECTMAP_VIRT_START +
 (base_mfn - mfn) * PAGE_SIZE;
 }
 
-if ( base_mfn < xenheap_mfn_start )
+if ( base_mfn < mfn_x(xenheap_mfn_start) )
 panic("cannot add xenheap mapping at %lx below heap start %lx",
-  base_mfn, xenheap_mfn_start);
+  base_mfn, mfn_x(xenheap_mfn_start));
 
 end_mfn = base_mfn + nr_mfns;
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ab4d8e4218..3b34855668 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -555,8 +555,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t 
dtb_size)
  * and enough mapped pages for copying the DTB.
  */
 dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT;
-boot_mfn_start = xenheap_mfn_end - dtb_pages - 1;
-boot_mfn_end = xenheap_mfn_end;
+boot_mfn_start = mfn_x(xenheap_mfn_end) - dtb_pages - 1;
+boot_mfn_end = mfn_x(xenheap_mfn_end);
 
 init_boot_pages(pfn_to_paddr(boot_mfn_start), pfn_to_paddr(boot_mfn_end));
 
@@ -591,11 +591,11 @@ static void __init setup_mm(unsigned long dtb_paddr, 
size_t dtb_size)
 e = bank_end;
 
 /* Avoid the xenheap */
-if ( s < pfn_to_paddr(xenheap_mfn_start+xenheap_pages)
- && pfn_to_paddr(xenheap_mfn_start) < e )
+if ( s < mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages))
+ && mfn_to_maddr(xenheap_mfn_start) < e )
 {
-e = pfn_to_paddr(xenheap_mfn_start);
-n = pfn_to_paddr(xenheap_mfn_start+xenheap_pages);
+e = mfn_to_maddr(xenheap_mfn_start);
+n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages));
 }
 
 dt_unreserved_regions(s, e, init_boot_pages, 0);
@@ -610,7 +610,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t 
dtb_size)
 
 /* Add xenheap memory that was not already added to the boot
allocator. */
-init_xenheap_pages(pfn_to_paddr(xenheap_mfn_start),
+init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
pfn_to_paddr(boot_mfn_start));
 }
 #else /* CONFIG_ARM_64 */
@@ -662,8 +662,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t 
dtb_size)
 total_pages += ram_size >> PAGE_SHIFT;
 
 xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
-xenheap_mfn_start = ram_start >> PAGE_SHIFT;
-xenheap_mfn_end = ram_end >> PAGE_SHIFT;
+xenheap_mfn_start = maddr_to_mfn(ram_start);
+xenheap_mfn_end = maddr_to_mfn(ram_end);
 
 /*
  * Need enough mapped pages for copying the DTB.
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 274b1752b3..b2e7ea7761 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -115,7 +115,7 @@ struct page_info
 #define PGC_count_width   PG_shift(9)
 #define PGC_count_mask((1UL<