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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Oct 16, 2023, at 22:46, Julien Grall <julien@xxxxxxx> wrote:
> 
> CC Henry

Thanks.

> 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/10/2023 09:44, Michal Orzel wrote:
>>>>>> 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.

Not sure what is the reason from Alexey’s side, but from my experience of
updating AVA firmware, it is sometimes tricky as I remember I did 5 boards
using the same set of upgrade instructions but one board was bricked
magically during this progress (Funny that the reason why we upgrade
the firmware was to fix this issue…). Once this happens, the only method to
recover is to have a dediprog or something similar to program the flash.
If there is no handy tools, then there would be a problem…So I understand
people may don’t really want to do the upgrading, but indeed I also recommend
to upgrade the firmware.

> 
> 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?

I tested this series on top of today’s staging in our CI and it seems that this
series is not breaking any boards/emulators we used for testing, so:

Tested-by: Henry Wang <Henry.Wang@xxxxxxx>

I saw you and Bertrand had a slightly different opinion on this and probably
we need Stefano’s input too, Any ideas Stefano?

But I personally won’t block this patch, so if the discussion is settled to
merge this to 4.18, feel free:

Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall