Hi,
> On 15 Aug 2022, at 17:44, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 15/08/2022 15:45, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi Bertrand,
>
>>> On 12 Aug 2022, at 20:24, Julien Grall <jul...@xen.org> wrote:
>>>
>>> From: Julien Grall <jgr...@amazon.com>
>>>
>>> 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 <jgr...@amazon.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marq...@arm.com>
>
> 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
