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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Oct 2023, Julien Grall wrote:
> On 16/10/2023 16:07, Bertrand Marquis wrote:
> > > 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).
> 
> IMHO, the fixes tag should not be based solely on the AVA platform. It should
> be based on whether this could sensibly affect others. Right now, there is
> nothing that would indicate either way.
> 
> And therefore a Fixes tag is sensible. This doesn't mean I would want to
> backport it right now (note that only 4.18 is affected). But this could change
> in the future if we get another report (post-4.18) on a platform where there
> are no other workaround.
> 
> Stefano any opinions?

The Fixes tag carries useful information but the problem is that it is
typically used for identifying backports and this is not a backport (at
least today we would not consider it a backport).

So I would provide the same information but without using the Fixes tag.
For instance: "This commit fixes an issue that was introduced by XXX
because of YYY and only affects the AVA platform with not up-to-date
firmware".

That way, we avoid the risk of someone taking all the applicable commits
with a Fixes tag and backporting them without thinking twice about it.
But we still have the information in the git log.