Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 16 Oct 2023, at 16:46, Julien Grall <julien@xxxxxxx> wrote:
> 
> CC Henry
> 
> Hi Alexey,
> 
> On 16/10/2023 15:24, Alexey Klimov wrote:
>> On Mon, 16 Oct 2023 at 15:13, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>>> 
>>> 
>>> 
>>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> 
>>>> wrote:
>>>> 
>>>> Hi
>>>> 
>>>> +Luca and Rahul
>>>> 
>>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 16/10/2023 09:44, Michal Orzel wrote:
>>>>>> Hi,
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> On 13/10/2023 14:26, Leo Yan wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse N1 cores),
>>>>>>> the physical memory regions are:
>>>>>>> 
>>>>>>>  DRAM memory regions:
>>>>>>>    Node[0] Region[0]: 0x000080000000 - 0x0000ffffffff
>>>>>>>    Node[0] Region[1]: 0x080000000000 - 0x08007fffffff
>>>>>>>    Node[0] Region[2]: 0x080100000000 - 0x0807ffffffff
>>>>>>> 
>>>>>>> The UEFI loads Xen hypervisor and DTB into the high memory, the kernel
>>>>>>> and ramdisk images are loaded into the low memory space:
>>>>>>> 
>>>>>>>  (XEN) MODULE[0]: 00000807f6df0000 - 00000807f6f3e000 Xen
>>>>>>>  (XEN) MODULE[1]: 00000807f8054000 - 00000807f8056000 Device Tree
>>>>>>>  (XEN) MODULE[2]: 00000000fa834000 - 00000000fc5de1d5 Ramdisk
>>>>>>>  (XEN) MODULE[3]: 00000000fc5df000 - 00000000ffb3f810 Kernel
>>>>>>> 
>>>>>>> In this case, the Xen binary is loaded above 8TB, which exceeds the
>>>>>>> maximum supported identity map space of 2TB in Xen. Consequently, the
>>>>>>> system fails to boot.
>>>>>>> 
>>>>>>> This patch enlarges identity map space to 10TB, allowing module loading
>>>>>>> within the range of [0x0 .. 0x000009ff_ffff_ffff].
>>>>>>> 
>>>>>>> Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to 
>>>>>>> prepare/enable/disable")
>>>>>> I don't think a fixes tag applies here given that 2TB was just a number 
>>>>>> we believed is enough
>>>>>> and all of this is platform dependent.
>>>>>> This can be dropped on commit if committer agrees
>>>>> Xen may have booted on that platform before hand. So this would be 
>>>>> considered a regression and therefore a tag would be warrant.
>>>>> 
>>>>> AFAICT, the commit is only present on the upcoming 4.18. So the question 
>>>>> is whether Xen 4.17 booted out-of-the-box on ADLink? If the answer is 
>>>>> yes, then we need to add a Fixes tag. But the correct one would be
>>>>> 
>>>> 
>>>> @Rahul or Luca: could you give an answer here ?
>>>> I know you used Xen on an AVA platform but was it booting out of the box ?
>>> 
>>> I can’t say for Xen 4.17, but our nightly job has run successfully on AVA 
>>> for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
>>> (docs/misra: add deviations.rst to document additional deviations.)
>>> 
>>> We are not applying any patch for it to run on AVA.
>> Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
>> 2.04.100.07.
>> This fix if for AVA machine with older UEFI firmware 1.07.300.03.
> 
> OOI, why not updating your firmware? I was expecting that it would also 
> contain other critical fixes.
> 
> With this in mind, I am now more in two mind to ask to merge this patch in 
> Xen 4.18. On one hand, I understand it will help to boot on AVA machine with 
> an older firmware. But on the other hand this is changing the memory layout 
> quite late in the release. The risk seems limited because Xen is not loaded 
> at the top of the virtual address space (there is the directmap afterwards).
> 
> Henry (as the release manager) and others, any opinions?

With the new information, I think it should be stated that it is fixing an 
issue on AVA platforms using an old firmware and platforms with and up-to-date 
firmware are not impacted.
It is an important information to keep in mind that this is not a fix to be 
backported for example (and for me the fixes tag should not be kept).

On whether or not it should go in the release, I am not quite sure that making 
a change in the layout at that stage is a good idea unless it is fixing a 
critical issue (which is not the case here).
So i would vote no but not go against if someone argue to have it in.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall