Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> 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
