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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 15 Aug 2022, at 17:44, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.

I will add that in my todo list and try to suggest something in the future 
(this is really not critical).
I was just pointing out because doing a deep review for someone who did not 
write this is complex due to the macros of macros of macros of macros ...

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall