[Xen-devel,03/24] xen/arm: setup: Remove bogus xenheap_mfn_end in setup_mm for arm64

Message ID 20170613161323.25196-4-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Extend the usage of typesafe MFN
Related show

Commit Message

Julien Grall June 13, 2017, 4:13 p.m.
xenheap_mfn_end is storing an MFN and not a physical address. Thankfully
xenheap_mfn_end is not used in the arm64 code. So drop it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/setup.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Stefano Stabellini June 15, 2017, 10:28 p.m. | #1
On Tue, 13 Jun 2017, Julien Grall wrote:
> xenheap_mfn_end is storing an MFN and not a physical address. Thankfully
> xenheap_mfn_end is not used in the arm64 code. So drop it.

That's fine, but in that case I would prefer to move the definition of
xenheap_mfn_end under #ifdef CONFIG_ARM_32. In fact, there is another
assignment of xenheap_mfn_end few lines below in the arm64 version of
setup_mm: don't we need to remove that too?


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/setup.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index f00f29a45b..ab4d8e4218 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -654,8 +654,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>              if ( e > bank_end )
>                  e = bank_end;
>  
> -            xenheap_mfn_end = e;
> -
>              dt_unreserved_regions(s, e, init_boot_pages, 0);
>              s = n;
>          }
> -- 
> 2.11.0
>
Julien Grall June 16, 2017, 6:52 a.m. | #2
Hi Stefano,

On 15/06/2017 23:28, Stefano Stabellini wrote:
> On Tue, 13 Jun 2017, Julien Grall wrote:
>> xenheap_mfn_end is storing an MFN and not a physical address. Thankfully
>> xenheap_mfn_end is not used in the arm64 code. So drop it.
>
> That's fine, but in that case I would prefer to move the definition of
> xenheap_mfn_end under #ifdef CONFIG_ARM_32. In fact, there is another
> assignment of xenheap_mfn_end few lines below in the arm64 version of
> setup_mm: don't we need to remove that too?

The other xenheap_mfn_end contains valid mfn that point to the end and I 
didn't want to #ifdef it because:
	1) It complexify the code
	2) All regions should be bound with start/end to simplify potential use.

Cheers,
Stefano Stabellini June 16, 2017, 5:33 p.m. | #3
On Fri, 16 Jun 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 15/06/2017 23:28, Stefano Stabellini wrote:
> > On Tue, 13 Jun 2017, Julien Grall wrote:
> > > xenheap_mfn_end is storing an MFN and not a physical address. Thankfully
> > > xenheap_mfn_end is not used in the arm64 code. So drop it.
> > 
> > That's fine, but in that case I would prefer to move the definition of
> > xenheap_mfn_end under #ifdef CONFIG_ARM_32. In fact, there is another
> > assignment of xenheap_mfn_end few lines below in the arm64 version of
> > setup_mm: don't we need to remove that too?
> 
> The other xenheap_mfn_end contains valid mfn that point to the end and I
> didn't want to #ifdef it because:
> 	1) It complexify the code
> 	2) All regions should be bound with start/end to simplify potential
> use.

I am only suggesting to move its definition and declaration under #ifdef
CONFIG_ARM_32 in xen/include/asm-arm/mm.h and xen/arch/arm/mm.c.

After that, all users of xenheap_mfn_end are already #ifdef
CONFIG_ARM_32, except for xen/arch/arm/setup.c:setup_mm. The setup_mm
under #ifdef CONFIG_ARM_32 will be fine. The setup_mm under
#ifdef CONFIG_ARM_64, doesn't need xenheap_mfn_end and we could just
remove it from there.

Does it make sense? Am I missing something?
Julien Grall June 16, 2017, 6:18 p.m. | #4
Hi Stefano,

On 06/16/2017 06:33 PM, Stefano Stabellini wrote:
> On Fri, 16 Jun 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 15/06/2017 23:28, Stefano Stabellini wrote:
>>> On Tue, 13 Jun 2017, Julien Grall wrote:
>>>> xenheap_mfn_end is storing an MFN and not a physical address. Thankfully
>>>> xenheap_mfn_end is not used in the arm64 code. So drop it.
>>>
>>> That's fine, but in that case I would prefer to move the definition of
>>> xenheap_mfn_end under #ifdef CONFIG_ARM_32. In fact, there is another
>>> assignment of xenheap_mfn_end few lines below in the arm64 version of
>>> setup_mm: don't we need to remove that too?
>>
>> The other xenheap_mfn_end contains valid mfn that point to the end and I
>> didn't want to #ifdef it because:
>> 	1) It complexify the code
>> 	2) All regions should be bound with start/end to simplify potential
>> use.
> 
> I am only suggesting to move its definition and declaration under #ifdef
> CONFIG_ARM_32 in xen/include/asm-arm/mm.h and xen/arch/arm/mm.c.
> 
> After that, all users of xenheap_mfn_end are already #ifdef
> CONFIG_ARM_32, except for xen/arch/arm/setup.c:setup_mm. The setup_mm
> under #ifdef CONFIG_ARM_32 will be fine. The setup_mm under
> #ifdef CONFIG_ARM_64, doesn't need xenheap_mfn_end and we could just
> remove it from there.
> 
> Does it make sense? Am I missing something?

To be honest, I really want to limit the ifdefery in the mm code. This 
is a bit complex to follow. One of my side project is to look at that.

Also, even if xenheap_mfn_end today is not used, I think the current 
value is valid and could be helpful to have in hand. For instance, it 
does not seem justify to have different implementation of at least 
is_xen_heap_page for arm32 and arm64.

So I am not in favor of dropping xenheap_mfn_end at the moment.

Cheers,
Stefano Stabellini June 16, 2017, 8:55 p.m. | #5
On Fri, 16 Jun 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/16/2017 06:33 PM, Stefano Stabellini wrote:
> > On Fri, 16 Jun 2017, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 15/06/2017 23:28, Stefano Stabellini wrote:
> > > > On Tue, 13 Jun 2017, Julien Grall wrote:
> > > > > xenheap_mfn_end is storing an MFN and not a physical address.
> > > > > Thankfully
> > > > > xenheap_mfn_end is not used in the arm64 code. So drop it.
> > > > 
> > > > That's fine, but in that case I would prefer to move the definition of
> > > > xenheap_mfn_end under #ifdef CONFIG_ARM_32. In fact, there is another
> > > > assignment of xenheap_mfn_end few lines below in the arm64 version of
> > > > setup_mm: don't we need to remove that too?
> > > 
> > > The other xenheap_mfn_end contains valid mfn that point to the end and I
> > > didn't want to #ifdef it because:
> > > 	1) It complexify the code
> > > 	2) All regions should be bound with start/end to simplify potential
> > > use.
> > 
> > I am only suggesting to move its definition and declaration under #ifdef
> > CONFIG_ARM_32 in xen/include/asm-arm/mm.h and xen/arch/arm/mm.c.
> > 
> > After that, all users of xenheap_mfn_end are already #ifdef
> > CONFIG_ARM_32, except for xen/arch/arm/setup.c:setup_mm. The setup_mm
> > under #ifdef CONFIG_ARM_32 will be fine. The setup_mm under
> > #ifdef CONFIG_ARM_64, doesn't need xenheap_mfn_end and we could just
> > remove it from there.
> > 
> > Does it make sense? Am I missing something?
> 
> To be honest, I really want to limit the ifdefery in the mm code. This is a
> bit complex to follow. One of my side project is to look at that.
> 
> Also, even if xenheap_mfn_end today is not used, I think the current value is
> valid and could be helpful to have in hand. For instance, it does not seem
> justify to have different implementation of at least is_xen_heap_page for
> arm32 and arm64.
> 
> So I am not in favor of dropping xenheap_mfn_end at the moment.

All right, then if we are going to keep xenheap_mfn_end around on arm64,
please update the commit message of this patch because it is confusing. 
It is just this one instance of xenheap_mfn_end in setup_mm which is
superfluous on arm64 because we are setting it again later.

Patch hide | download patch | download mbox

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f00f29a45b..ab4d8e4218 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -654,8 +654,6 @@  static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
             if ( e > bank_end )
                 e = bank_end;
 
-            xenheap_mfn_end = e;
-
             dt_unreserved_regions(s, e, init_boot_pages, 0);
             s = n;
         }