diff mbox series

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

Message ID 20231013122658.1270506-3-leo.yan@linaro.org (mailing list archive)
State New, archived
Headers show
Series xen/arm: Enlarge identity map space | expand

Commit Message

Leo Yan Oct. 13, 2023, 12:26 p.m. UTC
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")
Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 xen/arch/arm/arm64/mm.c               | 6 ++++--
 xen/arch/arm/include/asm/mmu/layout.h | 8 ++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

Comments

Michal Orzel Oct. 16, 2023, 8:44 a.m. UTC | #1
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.

> Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Bertrand Marquis Oct. 16, 2023, 1:40 p.m. UTC | #2
Hi,

> On 13 Oct 2023, at 14:26, Leo Yan <leo.yan@linaro.org> 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 agree with Michal here, this is not a fix so this should be removed (can be done
on commit).

> Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/arm64/mm.c               | 6 ++++--
> xen/arch/arm/include/asm/mmu/layout.h | 8 ++++----
> 2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
> index 78b7c7eb00..cb69df0661 100644
> --- a/xen/arch/arm/arm64/mm.c
> +++ b/xen/arch/arm/arm64/mm.c
> @@ -41,7 +41,8 @@ static void __init prepare_boot_identity_mapping(void)
>     clear_page(boot_third_id);
> 
>     if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
> -        panic("Cannot handle ID mapping above 2TB\n");
> +        panic("Cannot handle ID mapping above %uTB\n",
> +              IDENTITY_MAPPING_AREA_NR_L0 >> 1);
> 
>     /* Link first ID table */
>     pte = mfn_to_xen_entry(virt_to_mfn(boot_first_id), MT_NORMAL);
> @@ -74,7 +75,8 @@ static void __init prepare_runtime_identity_mapping(void)
>     DECLARE_OFFSETS(id_offsets, id_addr);
> 
>     if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
> -        panic("Cannot handle ID mapping above 2TB\n");
> +        panic("Cannot handle ID mapping above %uTB\n",
> +              IDENTITY_MAPPING_AREA_NR_L0 >> 1);
> 
>     /* Link first ID table */
>     pte = pte_of_xenaddr((vaddr_t)xen_first_id);
> diff --git a/xen/arch/arm/include/asm/mmu/layout.h b/xen/arch/arm/include/asm/mmu/layout.h
> index 2cb2382fbf..eac7eef885 100644
> --- a/xen/arch/arm/include/asm/mmu/layout.h
> +++ b/xen/arch/arm/include/asm/mmu/layout.h
> @@ -19,11 +19,11 @@
>  *   2G -   4G   Domheap: on-demand-mapped
>  *
>  * ARM64 layout:
> - * 0x0000000000000000 - 0x000001ffffffffff (2TB, L0 slots [0..3])
> + * 0x0000000000000000 - 0x000009ffffffffff (10TB, L0 slots [0..19])
>  *
>  *  Reserved to identity map Xen
>  *
> - * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4])
> + * 0x00000a0000000000 - 0x00000a7fffffffff (512GB, L0 slot [20])
>  *  (Relative offsets)
>  *   0  -   2M   Unmapped
>  *   2M -  10M   Xen text, data, bss
> @@ -35,7 +35,7 @@
>  *
>  *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>  *
> - * 0x0000028000000000 - 0x00007fffffffffff (125TB, L0 slots [5..255])
> + * 0x00000a8000000000 - 0x00007fffffffffff (512GB+117TB, L0 slots [21..255])
>  *  Unused
>  *
>  * 0x0000800000000000 - 0x000084ffffffffff (5TB, L0 slots [256..265])
> @@ -49,7 +49,7 @@
> #define XEN_VIRT_START          _AT(vaddr_t, MB(2))
> #else
> 
> -#define IDENTITY_MAPPING_AREA_NR_L0     4
> +#define IDENTITY_MAPPING_AREA_NR_L0     20
> #define XEN_VM_MAPPING                  SLOT0(IDENTITY_MAPPING_AREA_NR_L0)
> 
> #define SLOT0_ENTRY_BITS  39
> -- 
> 2.39.2
>
Leo Yan Oct. 16, 2023, 1:54 p.m. UTC | #3
On Mon, Oct 16, 2023 at 01:40:26PM +0000, Bertrand Marquis wrote:

[...]

> > 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 agree with Michal here, this is not a fix so this should be removed (can be done
> on commit).

This is fine for me.

I'd like to confirm with maintainers that should I spin a new patch
set to remove the fix tag?  Or maintainers could help to remove it
when pick up this patch set.

And thanks for review, Michal and Bertrand.

Leo

> > Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Cheers
> Bertrand
Julien Grall Oct. 16, 2023, 1:54 p.m. UTC | #4
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

1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to 
prepare/enable/disable the identity mapping").

We would also need to consider it as a candidate for Xen 4.18 because we 
would regress boot on ADLink.

Cheers,
Bertrand Marquis Oct. 16, 2023, 2 p.m. UTC | #5
Hi
 
+Luca and Rahul

> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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 ?

> 1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable the identity mapping").
> 
> We would also need to consider it as a candidate for Xen 4.18 because we would regress boot on ADLink.

Ack

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall
Alexey Klimov Oct. 16, 2023, 2:06 p.m. UTC | #6
On Mon, 16 Oct 2023 at 14:54, Julien Grall <julien@xen.org> 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

Yes. The upstream xen 4.17 booted fine and the mentioned commit was
found during bisect, so it is introduced regression.
I'd personally say that "Fixes" tag is needed here.

Thanks,
Alexey
Luca Fancellu Oct. 16, 2023, 2:13 p.m. UTC | #7
> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> Hi
> 
> +Luca and Rahul
> 
>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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.

> 
>> 1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable the identity mapping").
>> 
>> We would also need to consider it as a candidate for Xen 4.18 because we would regress boot on ADLink.
> 
> Ack
> 
> Cheers
> Bertrand
> 
>> 
>> Cheers,
>> 
>> --
>> Julien Grall
> 
>
Alexey Klimov Oct. 16, 2023, 2:24 p.m. UTC | #8
On Mon, 16 Oct 2023 at 15:13, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>
>
>
> > On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> >
> > Hi
> >
> > +Luca and Rahul
> >
> >> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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.

Best regards,
Alexey
Luca Fancellu Oct. 16, 2023, 2:28 p.m. UTC | #9
>>> 
>>> @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.

Yes indeed, all our AVA have a newer firmware, so I’m afraid we can’t check

Cheers,
Luca
Julien Grall Oct. 16, 2023, 2:46 p.m. UTC | #10
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@arm.com> wrote:
>>
>>
>>
>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>
>>> Hi
>>>
>>> +Luca and Rahul
>>>
>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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?

Cheers,
Julien Grall Oct. 16, 2023, 2:52 p.m. UTC | #11
Hi Leo,

On 16/10/2023 14:54, Leo Yan wrote:
> On Mon, Oct 16, 2023 at 01:40:26PM +0000, Bertrand Marquis wrote:
> 
> [...]
> 
>>> 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 agree with Michal here, this is not a fix so this should be removed (can be done
>> on commit).
> 
> This is fine for me.
> 
> I'd like to confirm with maintainers that should I spin a new patch
> set to remove the fix tag?  Or maintainers could help to remove it
> when pick up this patch set.

I can do small changes while committing (updating the fixes tag is one).

That said, we are in the middle of the code freeze for Xen 4.18. Any 
patch requires a release-ack from the release manager (Henry). I am a 
bit split at the moment whether we want this patch for Xen 4.18. So I 
have asked opinions from Henry (others can also provide some).

If it doesn't go for Xen 4.18, then Stefano might be able to queue it in 
his for-4.19. Otherwise, it will be picked up when the tree re-open 
hopefully in a couple of weeks time.

Cheers,

> 
> And thanks for review, Michal and Bertrand.
> 
> Leo
> 
>>> Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Cheers
>> Bertrand
Bertrand Marquis Oct. 16, 2023, 3:07 p.m. UTC | #12
Hi,

> On 16 Oct 2023, at 16:46, Julien Grall <julien@xen.org> 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@arm.com> wrote:
>>> 
>>> 
>>> 
>>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>> 
>>>> Hi
>>>> 
>>>> +Luca and Rahul
>>>> 
>>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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
Julien Grall Oct. 16, 2023, 3:31 p.m. UTC | #13
On 16/10/2023 16:07, Bertrand Marquis wrote:
> Hi,
> 
>> On 16 Oct 2023, at 16:46, Julien Grall <julien@xen.org> 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@arm.com> wrote:
>>>>
>>>>
>>>>
>>>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>>>
>>>>> Hi
>>>>>
>>>>> +Luca and Rahul
>>>>>
>>>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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?

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

I agree with your statement with the information we have today. But it 
could become a critical issue if another platform is hit by the same 
issue and have no other workaround.

Cheers,
Bertrand Marquis Oct. 16, 2023, 3:39 p.m. UTC | #14
> On 16 Oct 2023, at 17:31, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/10/2023 16:07, Bertrand Marquis wrote:
>> Hi,
>>> On 16 Oct 2023, at 16:46, Julien Grall <julien@xen.org> 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@arm.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>>>> 
>>>>>> Hi
>>>>>> 
>>>>>> +Luca and Rahul
>>>>>> 
>>>>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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?
> 
>> 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.
> 
> I agree with your statement with the information we have today. But it could become a critical issue if another platform is hit by the same issue and have no other workaround.

I am more seeing this as adding support for platforms with a topology that was not supported until now (because it was not encountered) rather than fixing a bug which is why i find it a bit odd to say that it is fixing some issue. But definitely this is an opinion and nothing that i could argue on :-)

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall
Julien Grall Oct. 16, 2023, 4:23 p.m. UTC | #15
On 16/10/2023 16:39, Bertrand Marquis wrote:
> 
> 
>> On 16 Oct 2023, at 17:31, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 16/10/2023 16:07, Bertrand Marquis wrote:
>>> Hi,
>>>> On 16 Oct 2023, at 16:46, Julien Grall <julien@xen.org> 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@arm.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On 16 Oct 2023, at 15:00, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> +Luca and Rahul
>>>>>>>
>>>>>>>> On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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?
>>
>>> 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.
>>
>> I agree with your statement with the information we have today. But it could become a critical issue if another platform is hit by the same issue and have no other workaround.
> 
> I am more seeing this as adding support for platforms with a topology that was not supported until now (because it was not encountered) rather than fixing a bug which is why i find it a bit odd to say that it is fixing some issue. But definitely this is an opinion and nothing that i could argue on :-)

This is a valid point. The problem is SUPPORT.md doesn't say anything 
about new HW. So in theory, if it works out-of-the-box on Xen 4.17, then 
one could expect that it should work on 4.18.

I can see why the owner of the platform may consider it critical. But I 
can see why the community may say it is not important.

This is not an easy one to solve because we don't exactly know all the 
use-cases where Xen is used. We only know the public ones.

Cheers,
Henry Wang Oct. 17, 2023, 4:51 a.m. UTC | #16
Hi Julien,

> On Oct 16, 2023, at 22:46, Julien Grall <julien@xen.org> 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@arm.com> 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@arm.com>

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@arm.com>

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall
Stefano Stabellini Oct. 17, 2023, 7:58 p.m. UTC | #17
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@xen.org> 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@arm.com>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > > On 16 Oct 2023, at 15:00, Bertrand Marquis
> > > > > > <Bertrand.Marquis@arm.com> wrote:
> > > > > > 
> > > > > > Hi
> > > > > > 
> > > > > > +Luca and Rahul
> > > > > > 
> > > > > > > On 16 Oct 2023, at 15:54, Julien Grall <julien@xen.org> 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.
Julien Grall Oct. 18, 2023, 10:59 a.m. UTC | #18
Hi Stefano,

On 17/10/2023 20:58, Stefano Stabellini wrote:
>> 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).

Below the definition of Fixes tag (from  process/sending-patches.pandoc):

If your patch fixes a bug in a specific commit, e.g. you found an issue 
using ``git bisect``, please use the `Fixes:` tag with the first 12 
characters of the commit id, and the one line summary.

This doesn't say anything about backport. In fact, we introduced a 
separate tag for that (see 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".

The part after 'and' is misleading. So far we had a report only on AVA 
platform but this doesn't mean it couldn't happen on other HW. The same 
is true with the new limit.

So this wants to be:

"and so far we only had a report for the AVA platform when using UEFI 
older than vX."

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

I don't really see the problem for someone to mistakenly backport this 
patch. In fact, this could potentially save them a lot of debugging if 
it happens that Xen is loaded above 2TB.

Anyway, both Bertrand and you seems to be against the Fixes tag here. So 
I can compromise with the "This commit fixes...". However, can Bertrand 
or you update process/send-patches.pandoc so it is clear for a 
contributor when they should add Fixes tag (which BTW I still disagree 
with but if the majority agrees, then I will not nack)?

Regarding this patch itself, Stefano can you queue to your branch for-4.19?

Cheers,
Julien Grall Oct. 18, 2023, 6:11 p.m. UTC | #19
Hi,

On 18/10/2023 11:59, Julien Grall wrote:
> On 17/10/2023 20:58, Stefano Stabellini wrote:
>>> 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).
> 
> Below the definition of Fixes tag (from  process/sending-patches.pandoc):
> 
> If your patch fixes a bug in a specific commit, e.g. you found an issue 
> using ``git bisect``, please use the `Fixes:` tag with the first 12 
> characters of the commit id, and the one line summary.
> 
> This doesn't say anything about backport. In fact, we introduced a 
> separate tag for that (see 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".
> 
> The part after 'and' is misleading. So far we had a report only on AVA 
> platform but this doesn't mean it couldn't happen on other HW. The same 
> is true with the new limit.
> 
> So this wants to be:
> 
> "and so far we only had a report for the AVA platform when using UEFI 
> older than vX."
> 
>>
>> 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.
> 
> I don't really see the problem for someone to mistakenly backport this 
> patch. In fact, this could potentially save them a lot of debugging if 
> it happens that Xen is loaded above 2TB.
> 
> Anyway, both Bertrand and you seems to be against the Fixes tag here. So 
> I can compromise with the "This commit fixes...". However, can Bertrand 
> or you update process/send-patches.pandoc so it is clear for a 
> contributor when they should add Fixes tag (which BTW I still disagree 
> with but if the majority agrees, then I will not nack)?

We had a chat about this during the Arm maintainer calls. The 
disagreement boiled down to the fact that SUPPORT.md (or the 
documentation) doesn't say anything about whether loading Xen above 2TB 
was supported or not. Depending on the view, one could consider a bug or 
not.

Looking through the documentation, the best place to document might 
actually be misc/arm/booting.txt where we already have some requirements 
to boot Xen (such the binary must be entered in NS EL2 mode).

I will prepare a patch and send one.

Cheers,
Leo Yan Oct. 19, 2023, 9:35 a.m. UTC | #20
Hi Julien,

On Wed, Oct 18, 2023 at 07:11:11PM +0100, Julien Grall wrote:
> On 18/10/2023 11:59, Julien Grall wrote:
> > On 17/10/2023 20:58, Stefano Stabellini wrote:

[...]

> > I don't really see the problem for someone to mistakenly backport this
> > patch. In fact, this could potentially save them a lot of debugging if
> > it happens that Xen is loaded above 2TB.
> > 
> > Anyway, both Bertrand and you seems to be against the Fixes tag here. So
> > I can compromise with the "This commit fixes...". However, can Bertrand
> > or you update process/send-patches.pandoc so it is clear for a
> > contributor when they should add Fixes tag (which BTW I still disagree
> > with but if the majority agrees, then I will not nack)?
> 
> We had a chat about this during the Arm maintainer calls. The disagreement
> boiled down to the fact that SUPPORT.md (or the documentation) doesn't say
> anything about whether loading Xen above 2TB was supported or not. Depending
> on the view, one could consider a bug or not.
> 
> Looking through the documentation, the best place to document might actually
> be misc/arm/booting.txt where we already have some requirements to boot Xen
> (such the binary must be entered in NS EL2 mode).
> 
> I will prepare a patch and send one.


I would like to check if here is anything specific I should follow up
on. Based on the discussion in this thread, I've come to the following
conclusions:

- Remove the fixes tags;
- Add a description in commit log, something like:
  "Since commit 1c78d76b67e1 ('xen/arm64: mm: Introduce helpers to
   prepare/enable/disable the identity mapping'), Xen will fail to boot
   up if it's loaded in memory above 2TB. This commit fixes the
   regression introduced by that commit."
- Add tages:
  A review tag from Michal Orzel
  A review tag from Bertrand Marquis
  A test tag from Henry Wang

Should I repin a new patch set to address the items mentioned above?

Another question is for the 'Release-acked-by' tag.  Henry gave this
tag, but I don't know how to handle it if I need to respin this patch.
Seems to me this is a special tag only for release process, so I don't
need to include it in the new patch, right?

Thanks all of you for the efforts on this patch set!

Leo
Leo Yan Nov. 6, 2023, 2:24 a.m. UTC | #21
Hi all,

On Wed, Oct 18, 2023 at 07:11:11PM +0100, Julien Grall wrote:

[...]

> > Anyway, both Bertrand and you seems to be against the Fixes tag here. So
> > I can compromise with the "This commit fixes...". However, can Bertrand
> > or you update process/send-patches.pandoc so it is clear for a
> > contributor when they should add Fixes tag (which BTW I still disagree
> > with but if the majority agrees, then I will not nack)?
> 
> We had a chat about this during the Arm maintainer calls. The disagreement
> boiled down to the fact that SUPPORT.md (or the documentation) doesn't say
> anything about whether loading Xen above 2TB was supported or not. Depending
> on the view, one could consider a bug or not.
> 
> Looking through the documentation, the best place to document might actually
> be misc/arm/booting.txt where we already have some requirements to boot Xen
> (such the binary must be entered in NS EL2 mode).
> 
> I will prepare a patch and send one.

I saw Julien's patch "docs/arm: Document where Xen should be loaded in
memory" has landed on the staging branch [1].

Should I respin this patch set and update the document to 10TB
limitation?  I need maintainer's sugguestion, otherwise I don't know
how to proceed for this patch set.

Thanks,
Leo

[1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=5415b2b2118bd78d8a04f276a8312f7f0cb1a466
Julien Grall Nov. 6, 2023, 9:39 a.m. UTC | #22
On 19/10/2023 10:35, Leo Yan wrote:
> Hi Julien,

Hi Leo,


> On Wed, Oct 18, 2023 at 07:11:11PM +0100, Julien Grall wrote:
>> On 18/10/2023 11:59, Julien Grall wrote:
>>> On 17/10/2023 20:58, Stefano Stabellini wrote:
> 
> [...]
> 
>>> I don't really see the problem for someone to mistakenly backport this
>>> patch. In fact, this could potentially save them a lot of debugging if
>>> it happens that Xen is loaded above 2TB.
>>>
>>> Anyway, both Bertrand and you seems to be against the Fixes tag here. So
>>> I can compromise with the "This commit fixes...". However, can Bertrand
>>> or you update process/send-patches.pandoc so it is clear for a
>>> contributor when they should add Fixes tag (which BTW I still disagree
>>> with but if the majority agrees, then I will not nack)?
>>
>> We had a chat about this during the Arm maintainer calls. The disagreement
>> boiled down to the fact that SUPPORT.md (or the documentation) doesn't say
>> anything about whether loading Xen above 2TB was supported or not. Depending
>> on the view, one could consider a bug or not.
>>
>> Looking through the documentation, the best place to document might actually
>> be misc/arm/booting.txt where we already have some requirements to boot Xen
>> (such the binary must be entered in NS EL2 mode).
>>
>> I will prepare a patch and send one.
> 
> 
> I would like to check if here is anything specific I should follow up
> on. Based on the discussion in this thread, I've come to the following
> conclusions:
> 
> - Remove the fixes tags;
> - Add a description in commit log, something like:
>    "Since commit 1c78d76b67e1 ('xen/arm64: mm: Introduce helpers to
>     prepare/enable/disable the identity mapping'), Xen will fail to boot
>     up if it's loaded in memory above 2TB. This commit fixes the
>     regression introduced by that commit."
> - Add tages:
>    A review tag from Michal Orzel
>    A review tag from Bertrand Marquis
>    A test tag from Henry Wang
> 
> Should I repin a new patch set to address the items mentioned above?

You will also want to update the documentation after
"docs/arm: Document where Xen should be loaded in
memory"

> 
> Another question is for the 'Release-acked-by' tag.  Henry gave this
> tag, but I don't know how to handle it if I need to respin this patch.
> Seems to me this is a special tag only for release process, so I don't
> need to include it in the new patch, right?

The release-acked-by tag is only necessary during freeze period if the 
patch will land in the next release (i.e. 4.18). In this case, your 
patch will be part of the 4.19, so you can remove the release-acked-by.


If your patch was targeting 4.19, then it is usually fine to keep the 
release-acked-by even for the respin. It means that the release manager 
is happy for the patch to go assuming the patch has all the necessary 
reviews.

Cheers,
Leo Yan Nov. 6, 2023, 9:45 a.m. UTC | #23
Hi Julien,

On Mon, Nov 06, 2023 at 09:39:24AM +0000, Julien Grall wrote:

[...]

> > I would like to check if here is anything specific I should follow up
> > on. Based on the discussion in this thread, I've come to the following
> > conclusions:
> > 
> > - Remove the fixes tags;
> > - Add a description in commit log, something like:
> >    "Since commit 1c78d76b67e1 ('xen/arm64: mm: Introduce helpers to
> >     prepare/enable/disable the identity mapping'), Xen will fail to boot
> >     up if it's loaded in memory above 2TB. This commit fixes the
> >     regression introduced by that commit."
> > - Add tages:
> >    A review tag from Michal Orzel
> >    A review tag from Bertrand Marquis
> >    A test tag from Henry Wang
> > 
> > Should I repin a new patch set to address the items mentioned above?
> 
> You will also want to update the documentation after
> "docs/arm: Document where Xen should be loaded in
> memory"

Will do.

> > Another question is for the 'Release-acked-by' tag.  Henry gave this
> > tag, but I don't know how to handle it if I need to respin this patch.
> > Seems to me this is a special tag only for release process, so I don't
> > need to include it in the new patch, right?
> 
> The release-acked-by tag is only necessary during freeze period if the patch
> will land in the next release (i.e. 4.18). In this case, your patch will be
> part of the 4.19, so you can remove the release-acked-by.

Okay, I will _not_ include release-acked-by tag in the respin.

> If your patch was targeting 4.19, then it is usually fine to keep the
> release-acked-by even for the respin. It means that the release manager is
> happy for the patch to go assuming the patch has all the necessary reviews.

Thanks for explaination.

Leo
Julien Grall Nov. 16, 2023, 2:21 p.m. UTC | #24
Hi Leo,

On 06/11/2023 09:45, Leo Yan wrote:
> Hi Julien,
> 
> On Mon, Nov 06, 2023 at 09:39:24AM +0000, Julien Grall wrote:
> 
> [...]
> 
>>> I would like to check if here is anything specific I should follow up
>>> on. Based on the discussion in this thread, I've come to the following
>>> conclusions:
>>>
>>> - Remove the fixes tags;
>>> - Add a description in commit log, something like:
>>>     "Since commit 1c78d76b67e1 ('xen/arm64: mm: Introduce helpers to
>>>      prepare/enable/disable the identity mapping'), Xen will fail to boot
>>>      up if it's loaded in memory above 2TB. This commit fixes the
>>>      regression introduced by that commit."
>>> - Add tages:
>>>     A review tag from Michal Orzel
>>>     A review tag from Bertrand Marquis
>>>     A test tag from Henry Wang
>>>
>>> Should I repin a new patch set to address the items mentioned above?
>>
>> You will also want to update the documentation after
>> "docs/arm: Document where Xen should be loaded in
>> memory"
> 
> Will do.
> 
>>> Another question is for the 'Release-acked-by' tag.  Henry gave this
>>> tag, but I don't know how to handle it if I need to respin this patch.
>>> Seems to me this is a special tag only for release process, so I don't
>>> need to include it in the new patch, right?
>>
>> The release-acked-by tag is only necessary during freeze period if the patch
>> will land in the next release (i.e. 4.18). In this case, your patch will be
>> part of the 4.19, so you can remove the release-acked-by.
> 
> Okay, I will _not_ include release-acked-by tag in the respin.

I have done the changes and directly committed the series. So no need to 
respin.

Cheers,
Leo Yan Nov. 16, 2023, 3:36 p.m. UTC | #25
On Thu, Nov 16, 2023 at 02:21:18PM +0000, Julien Grall wrote:

[...]

> I have done the changes and directly committed the series. So no need to
> respin.

Thanks a lot, Julien!

Leo
diff mbox series

Patch

diff --git a/xen/arch/arm/arm64/mm.c b/xen/arch/arm/arm64/mm.c
index 78b7c7eb00..cb69df0661 100644
--- a/xen/arch/arm/arm64/mm.c
+++ b/xen/arch/arm/arm64/mm.c
@@ -41,7 +41,8 @@  static void __init prepare_boot_identity_mapping(void)
     clear_page(boot_third_id);
 
     if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
-        panic("Cannot handle ID mapping above 2TB\n");
+        panic("Cannot handle ID mapping above %uTB\n",
+              IDENTITY_MAPPING_AREA_NR_L0 >> 1);
 
     /* Link first ID table */
     pte = mfn_to_xen_entry(virt_to_mfn(boot_first_id), MT_NORMAL);
@@ -74,7 +75,8 @@  static void __init prepare_runtime_identity_mapping(void)
     DECLARE_OFFSETS(id_offsets, id_addr);
 
     if ( id_offsets[0] >= IDENTITY_MAPPING_AREA_NR_L0 )
-        panic("Cannot handle ID mapping above 2TB\n");
+        panic("Cannot handle ID mapping above %uTB\n",
+              IDENTITY_MAPPING_AREA_NR_L0 >> 1);
 
     /* Link first ID table */
     pte = pte_of_xenaddr((vaddr_t)xen_first_id);
diff --git a/xen/arch/arm/include/asm/mmu/layout.h b/xen/arch/arm/include/asm/mmu/layout.h
index 2cb2382fbf..eac7eef885 100644
--- a/xen/arch/arm/include/asm/mmu/layout.h
+++ b/xen/arch/arm/include/asm/mmu/layout.h
@@ -19,11 +19,11 @@ 
  *   2G -   4G   Domheap: on-demand-mapped
  *
  * ARM64 layout:
- * 0x0000000000000000 - 0x000001ffffffffff (2TB, L0 slots [0..3])
+ * 0x0000000000000000 - 0x000009ffffffffff (10TB, L0 slots [0..19])
  *
  *  Reserved to identity map Xen
  *
- * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4])
+ * 0x00000a0000000000 - 0x00000a7fffffffff (512GB, L0 slot [20])
  *  (Relative offsets)
  *   0  -   2M   Unmapped
  *   2M -  10M   Xen text, data, bss
@@ -35,7 +35,7 @@ 
  *
  *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
  *
- * 0x0000028000000000 - 0x00007fffffffffff (125TB, L0 slots [5..255])
+ * 0x00000a8000000000 - 0x00007fffffffffff (512GB+117TB, L0 slots [21..255])
  *  Unused
  *
  * 0x0000800000000000 - 0x000084ffffffffff (5TB, L0 slots [256..265])
@@ -49,7 +49,7 @@ 
 #define XEN_VIRT_START          _AT(vaddr_t, MB(2))
 #else
 
-#define IDENTITY_MAPPING_AREA_NR_L0     4
+#define IDENTITY_MAPPING_AREA_NR_L0     20
 #define XEN_VM_MAPPING                  SLOT0(IDENTITY_MAPPING_AREA_NR_L0)
 
 #define SLOT0_ENTRY_BITS  39