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

From: Julien Grall <julien@xen.org>
To: Bertrand Marquis <Bertrand.Marquis@arm.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Julien Grall <jgrall@amazon.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
Date: Mon, 15 Aug 2022 17:44:21 +0100	[thread overview]
Message-ID: <3136c797-652b-97c9-b356-b49fe53a40b5@xen.org> (raw)
In-Reply-To: <8FA138D1-CBF2-4BAB-9283-2573A6A09C79@arm.com>

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

Hi Bertrand,

>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@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 <jgrall@amazon.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@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.

Cheers,

-- 
Julien Grall