RE: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to directmap_*

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v3 3/4] xen/arm: mm: Rename xenheap_* variable to
> directmap_*
> 
> Hi Henry,
> 
> On 07/09/2022 09:36, Henry Wang wrote:
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 7f5b317d3e..4a70ed2986 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -132,12 +132,12 @@ uint64_t init_ttbr;
> >   static paddr_t phys_offset;
> >
> >   /* Limits of the Xen heap */
> > -mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> > -mfn_t xenheap_mfn_end __read_mostly;
> > -vaddr_t xenheap_virt_end __read_mostly;
> > +mfn_t directmap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;
> > +mfn_t directmap_mfn_end __read_mostly;
> > +vaddr_t directmap_virt_end __read_mostly;
> >   #ifdef CONFIG_ARM_64
> > -vaddr_t xenheap_virt_start __read_mostly;
> > -unsigned long xenheap_base_pdx __read_mostly;
> > +vaddr_t directmap_virt_start __read_mostly;
> > +unsigned long directmap_base_pdx __read_mostly;
> >   #endif
> >
> >   unsigned long frametable_base_pdx __read_mostly;
> > @@ -609,7 +609,7 @@ void __init setup_xenheap_mappings(unsigned
> long base_mfn,
> 
> I think the function also want to be renamed to match the code below.

Hmmm, renaming the name to "setup_directmap_mappings" would
somehow lead me to think of we are getting rid of the name "xenheap"
completely in the code, which seems a little bit scary to me...

But I just checked there is a comment
"/* Set up the xenheap: up to 1GB of contiguous, always-mapped memory."
above the function and the declaration so I guess we are fine?

> 
> >           panic("Unable to setup the xenheap mappings.\n");
> 
> Likely, I think this wants to be s/xenheap/directmap/

Ok.

> 
> >
> >       /* Record where the xenheap is, for translation routines. */
> 
> Same here because you set directmap_virt_end.

Ok.

> 
> > -    xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> > +    directmap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
> 
> I would be OK to keep "XENHEAP_*" for now.

Thanks for your confirmation.

> 
> >   }
> >   #else /* CONFIG_ARM_64 */
> >   void __init setup_xenheap_mappings(unsigned long base_mfn,
> > @@ -618,12 +618,12 @@ void __init setup_xenheap_mappings(unsigned
> long base_mfn,
> >       int rc;
> >
> >       /* First call sets the xenheap physical and virtual offset. */
> 
> s/xenheap/directmap/ I haven't looked if there are other instances in
> the function.

Don't bother, I will take care of the rest.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall