Re: [PATCH v4 09/14] xen/arm32: head: Remove restriction where to load Xen

[Julien Grall <julien@xxxxxxx>]

On 16/01/2023 08:14, Michal Orzel wrote:
> Hi Julien,
Hi Luca,

> On 13/01/2023 11:11, Julien Grall wrote:
> > +/*
> > + * Remove the temporary mapping of Xen starting at TEMPORARY_XEN_VIRT_START.
> > + *
> > + * Clobbers r0 - r1
> > + */
> > +remove_temporary_mapping:
> > +        /* r2:r3 := invalid page-table entry */
> > +        mov   r2, #0
> > +        mov   r3, #0
> > +
> > +        adr_l r0, boot_pgtable
> > +        mov_w r1, TEMPORARY_XEN_VIRT_START
> > +        get_table_slot r1, r1, 1     /* r1 := first slot */
> Can't we just use TEMPORARY_AREA_FIRST_SLOT?
IMHO, it would make the code a bit more difficult to read because the 
connection between TEMPORARY_XEN_VIRT_START and 
TEMPORARY_AREA_FIRST_SLOT is not totally obvious.
So I would rather prefer if this stays like that.

> > +        lsl   r1, r1, #3             /* r1 := first slot offset */
> > +        strd  r2, r3, [r0, r1]
> > +
> > +        flush_xen_tlb_local r0
> > +
> > +        mov  pc, lr
> > +ENDPROC(remove_temporary_mapping)
> > +
> >   /*
> >    * Map the UART in the fixmap (when earlyprintk is used) and hook the
> >    * fixmap table in the page tables.
> > diff --git a/xen/arch/arm/include/asm/config.h 
> > b/xen/arch/arm/include/asm/config.h
> > index 87851e677701..6c1b762e976d 100644
> > --- a/xen/arch/arm/include/asm/config.h
> > +++ b/xen/arch/arm/include/asm/config.h
> > @@ -148,6 +148,20 @@
> >   /* Number of domheap pagetable pages required at the second level (2MB 
> > mappings) */
> >   #define DOMHEAP_SECOND_PAGES (DOMHEAP_VIRT_SIZE >> FIRST_SHIFT)
> >
> > +/*
> > + * The temporary area is overlapping with the domheap area. This may
> > + * be used to create an alias of the first slot containing Xen mappings
> > + * when turning on/off the MMU.
> > + */
> > +#define TEMPORARY_AREA_FIRST_SLOT    (first_table_offset(DOMHEAP_VIRT_START))
> > +
> > +/* Calculate the address in the temporary area */
> > +#define TEMPORARY_AREA_ADDR(addr)                           \
> > +     (((addr) & ~XEN_PT_LEVEL_MASK(1)) |                    \
> > +      (TEMPORARY_AREA_FIRST_SLOT << XEN_PT_LEVEL_SHIFT(1)))
> XEN_PT_LEVEL_{MASK/SHIFT} should be used when we do not know the level upfront.
> Otherwise, no need for opencoding and you should use FIRST_MASK and FIRST_SHIFT.
We discussed in the past to phase out the use of FIRST_MASK, FIRST_SHIFT 
because the name is too generic. So for new code, we should use 
XEN_PT_LEVEL_{MASK/SHIFT}.
Cheers,

--
Julien Grall