Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it

[Julien Grall <julien@xxxxxxx>]

On 15/08/2022 15:45, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,

> > On 12 Aug 2022, at 20:24, Julien Grall <julien@xxxxxxx> wrote:
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > There are a few places in the code that need to find the slot
> > at a given page-table level.
> >
> > So create a new macro get_table_slot() for that. This will reduce
> > the effort to figure out whether the code is doing the right thing.
> >
> > Take the opportunity to use 'ubfx'. The only benefits is reducing
> > the number of instructions from 2 to 1.
> >
> > The new macro is used everywhere we need to compute the slot. This
> > requires to tweak the parameter of create_table_entry() to pass
> > a level rather than shift.
> >
> > Note, for slot 0 the code is currently skipping the masking part. While
> > this is fine, it is safer to mask it as technically slot 0 only covers
> > bit 48 - 39 bit (assuming 4KB page granularity).
> >
> > Take the opportunity to correct the comment when finding the second
> > slot for the identity mapping (we are computing the second slot
> > rather than first).
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
Thanks!

> > ----
> >
> >     This patch also has the benefits to reduce the number
> >     of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
> >     patch for arm32 will reduce further.
> > ---
> > xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
> > 1 file changed, 30 insertions(+), 25 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index 26cc7705f556..ad014716db6f 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -493,13 +493,24 @@ cpu_init:
> >          ret
> > ENDPROC(cpu_init)
> >
> > +/*
> > + * Macro to find the slot number at a given page-table level
> > + *
> > + * slot:     slot computed
> > + * virt:     virtual address
> > + * lvl:      page-table level
> > + */
> > +.macro get_table_slot, slot, virt, lvl
> > +        ubfx  \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> > +.endm
> > +
> Crawling through the macros to verify the code was not that easy.
> This is not related to this patch but XEN_PT_* macros could really do with more 
> comments.
To me, the name of the macros are self-explaining. So I am not entirely 
what to write in the comments. Please suggest.
Cheers,

--
Julien Grall