| Message ID | 20170613161323.25196-4-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Extend the usage of typesafe MFN | expand |
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 >
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,
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?
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,
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.
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; }
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(-)