Re: [PATCH v1 repost 1/4] arm/mmu: Move init_ttbr to a new section .data.idmap

From: Michal Orzel <michal.orzel@amd.com>
To: Julien Grall <julien@xen.org>, <xen-devel@lists.xenproject.org>
Cc: <carlo.nonato@minervasys.tech>, Julien Grall <jgrall@amazon.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v1 repost 1/4] arm/mmu: Move init_ttbr to a new section .data.idmap
Date: Wed, 17 Jan 2024 14:03:46 +0100	[thread overview]
Message-ID: <f91e3222-c72a-453e-9160-fdcc94211d82@amd.com> (raw)
In-Reply-To: <ea7b5499-5b15-4c03-8187-39a9456e6ea4@xen.org>


On 17/01/2024 13:10, Julien Grall wrote:
> 
> 
> On 17/01/2024 08:30, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 16/01/2024 15:37, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> With the upcoming work to color Xen, the binary will not be anymore
>>> physically contiguous. This will be a problem during boot as the
>>> assembly code will need to work out where each piece of Xen reside.
>>>
>>> An easy way to solve the issue is to have all code/data accessed
>>> by the secondary CPUs while the MMU is off within a single page.
>>>
>>> Right now, init_ttbr is used by secondary CPUs to find there page-tables
>>> before the MMU is on. Yet it is currently in .data which is unlikely
>>> to be within the same page as the rest of the idmap.
>>>
>>> Create a new section .data.idmap that will be used for variables
>>> accessed by the early boot code. The first one is init_ttbr.
>>>
>>> The idmap is currently part of the text section and therefore will
>>> be mapped read-only executable. This means that we need to temporarily
>>> remap init_ttbr in order to update it.
>>>
>>> Introduce a new function set_init_ttbr() for this purpose so the code
>>> is not duplicated between arm64 and arm32.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> with ...
>>
>>> ---
>>>   xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
>>>   xen/arch/arm/xen.lds.S     |  1 +
>>>   2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
>>> index b6fc0aae07f1..f1cf9252710c 100644
>>> --- a/xen/arch/arm/mmu/smpboot.c
>>> +++ b/xen/arch/arm/mmu/smpboot.c
>>> @@ -9,6 +9,10 @@
>>>
>>>   #include <asm/setup.h>
>>>
>>> +/* Override macros from asm/page.h to make them work with mfn_t */
>>> +#undef virt_to_mfn
>>> +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>>> +
>>>   /*
>>>    * Static start-of-day pagetables that we use before the allocators
>>>    * are up. These are used by all CPUs during bringup before switching
>>> @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
>>>   DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
>>>
>>>   /* Non-boot CPUs use this to find the correct pagetables. */
>>> -uint64_t init_ttbr;
>>> +uint64_t __section(".data.idmap") init_ttbr;
>> Do we need to keep the declaration in mmu/mm.h? This variable is only used in this file
>> and in assembly, so maybe better to drop declaration and use asmlinkage instead?
> 
> I don't see the problem of keeping the declaration in mmu/mm.h. In any
> case, this seems to be unrelated to this patch.
This was just a question about the sense of a declaration that is not used/needed at all.
If you also thought so, it could be done in this patch given that it touches definition.
Since you don't, no problem whatsoever.

~Michal