| Message ID | 20230625204907.57291-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Enable UBSAN support | expand |
Hi Julien, > -----Original Message----- > Subject: [PATCH 1/9] xen/arm: Check Xen size when linking > > The linker will happily link Xen if it is bigger than what we can handle > (e.g 2MB). This will result to unexpected failure after boot. I found this a little bit misleading because when I looked for XEN_VIRT_SIZE, I am having: #define XEN_VIRT_SIZE _AT(vaddr_t, MB(8)) May I please suggest it is better to keep the example in commit message consistent with the actual value used in code? > > This unexpected failure can be prevented by forbidding linking if Xen is > bigger than the area we reversed. I am not sure this is a typo: s/reversed/reserved/, but I find the current sentence a little bit hard to understand. > > Signed-off-by: Julien Grall <julien@xen.org> Both comment above can be fixed on commit, so: Reviewed-by: Henry Wang <Henry.Wang@arm.com> I've also tested this patch on top of today's staging by our internal CI, and I don't see any build error there, hence also: Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On 26/06/2023 07:15, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> Subject: [PATCH 1/9] xen/arm: Check Xen size when linking >> >> The linker will happily link Xen if it is bigger than what we can handle >> (e.g 2MB). This will result to unexpected failure after boot. > > I found this a little bit misleading because when I looked for > XEN_VIRT_SIZE, I am having: > > #define XEN_VIRT_SIZE _AT(vaddr_t, MB(8)) I think you looked at the tree with my series applied because patch #9 will bump the reserved size. The value in the commit message match the current value in staging. [...] >> This unexpected failure can be prevented by forbidding linking if Xen is >> bigger than the area we reversed. > > I am not sure this is a typo: s/reversed/reserved/, but I find the current > sentence a little bit hard to understand. It is a typo. I will fix it. > >> >> Signed-off-by: Julien Grall <julien@xen.org> > > Both comment above can be fixed on commit, so: > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > > I've also tested this patch on top of today's staging by our internal CI, > and I don't see any build error there, hence also: > > Tested-by: Henry Wang <Henry.Wang@arm.com> Thanks! Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH 1/9] xen/arm: Check Xen size when linking > > On 26/06/2023 07:15, Henry Wang wrote: > > Hi Julien, > > Hi Henry, > > > >> -----Original Message----- > >> Subject: [PATCH 1/9] xen/arm: Check Xen size when linking > >> > >> The linker will happily link Xen if it is bigger than what we can handle > >> (e.g 2MB). This will result to unexpected failure after boot. > > > > I found this a little bit misleading because when I looked for > > XEN_VIRT_SIZE, I am having: > > > > #define XEN_VIRT_SIZE _AT(vaddr_t, MB(8)) > > I think you looked at the tree with my series applied because patch #9 > will bump the reserved size. The value in the commit message match the > current value in staging. Yeah exactly, I started my review from patch #1 and when I saw patch #9 I realized the size is bumped there, but the email was already sent then.... Sorry for the confusion. Kind regards, Henry
On 25/06/2023 22:48, Julien Grall wrote: > > > The linker will happily link Xen if it is bigger than what we can handle > (e.g 2MB). This will result to unexpected failure after boot. > > This unexpected failure can be prevented by forbidding linking if Xen is > bigger than the area we reversed. s/reversed/reserved > > Signed-off-by: Julien Grall <julien@xen.org> > --- > xen/arch/arm/xen.lds.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S > index be58c2c39514..c5d8c6201423 100644 > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -241,3 +241,4 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") > ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") > ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") > ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") > +ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big") Would it be possible to use _start so that we can have a consolidated way of calculating xen size across arch linker scripts and C code? It makes it easier for grepping. All in all, Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi Michal, On 26/06/2023 12:24, Michal Orzel wrote: > > > On 25/06/2023 22:48, Julien Grall wrote: >> >> >> The linker will happily link Xen if it is bigger than what we can handle >> (e.g 2MB). This will result to unexpected failure after boot. >> >> This unexpected failure can be prevented by forbidding linking if Xen is >> bigger than the area we reversed. > s/reversed/reserved > >> >> Signed-off-by: Julien Grall <julien@xen.org> >> --- >> xen/arch/arm/xen.lds.S | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S >> index be58c2c39514..c5d8c6201423 100644 >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -241,3 +241,4 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") >> ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") >> ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") >> ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") >> +ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big") > Would it be possible to use _start so that we can have a consolidated way of calculating xen size > across arch linker scripts and C code? It makes it easier for grepping. Fair point. I have renamed to _start. > > All in all, > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks! Cheers,
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index be58c2c39514..c5d8c6201423 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -241,3 +241,4 @@ ASSERT(IS_ALIGNED(__init_begin, 4), "__init_begin is misaligned") ASSERT(IS_ALIGNED(__init_end, 4), "__init_end is misaligned") ASSERT(IS_ALIGNED(__bss_start, POINTER_ALIGN), "__bss_start is misaligned") ASSERT(IS_ALIGNED(__bss_end, POINTER_ALIGN), "__bss_end is misaligned") +ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big")
The linker will happily link Xen if it is bigger than what we can handle (e.g 2MB). This will result to unexpected failure after boot. This unexpected failure can be prevented by forbidding linking if Xen is bigger than the area we reversed. Signed-off-by: Julien Grall <julien@xen.org> --- xen/arch/arm/xen.lds.S | 1 + 1 file changed, 1 insertion(+)