On 16/01/2023 09:55, Julien Grall wrote:
>
>
> 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}.
In that case:
Reviewed-by: Michal Orzel <michal.or...@amd.com>
~Michal
