| Message ID | 20230625204907.57291-9-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Enable UBSAN support | expand |
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN Typo in title: s/USBAN/UBSAN/ and... > > From: Julien Grall <jgrall@amazon.com> > > UBSAN has been enabled a few years ago on x86 but was never > enabled on Arm because the final binary is bigger than 2MB ( > the maximum we can currently handled). > > With the recent rework, it is now possible to grow Xen over 2MB. > So there is no more roadblock to enable Xen other than increasing > the reserved area. > > On my setup, for arm32, the final binaray was very close to 4MB. > Furthermore, one may want to enable USBAN and GCOV which would put ...here also. > the binary well-over 4MB (both features require for some space). > Therefore, increase the size to 8MB which should us some margin. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Both typos can be fixed on commit and I think I am good with the current approach, so: Reviewed-by: Henry Wang <Henry.Wang@arm.com> I did a play with UBSAN enabled on FVP (arm64), in my setup, the result Xen file is 3.7MB: -rwxrwxr-x 1 xinwan02 xinwan02 3.7M Jun 26 14:47 xen lrwxrwxrwx 1 xinwan02 xinwan02 3 Jun 26 14:47 xen.efi -> xen -rwxrwxr-x 1 xinwan02 xinwan02 14M Jun 26 14:47 xen-syms and the Xen and Dom0 booted fine and I can login Dom0. So technically: Tested-by: Henry Wang <Henry.Wang@arm.com> However, I noticed Xen will print below during the Dom0 boot: (XEN) ================================================================================ (XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15 (XEN) left shift of 1 by 31 places cannot be represented in type 'int' (XEN) Xen WARN at common/ubsan/ubsan.c:172 Just want to make sure you also noticed this, otherwise maybe you can include another patch in the series to fix this? Or I can do that in case you don't have enough bandwidth. Kind regards, Henry
On 25/06/2023 22:49, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > UBSAN has been enabled a few years ago on x86 but was never > enabled on Arm because the final binary is bigger than 2MB ( > the maximum we can currently handled). > > With the recent rework, it is now possible to grow Xen over 2MB. > So there is no more roadblock to enable Xen other than increasing > the reserved area. > > On my setup, for arm32, the final binaray was very close to 4MB. > Furthermore, one may want to enable USBAN and GCOV which would put > the binary well-over 4MB (both features require for some space). > Therefore, increase the size to 8MB which should us some margin. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > ---- > > The drawback with this approach is that we are adding 6 new > page-table (3 for boot and 3 for runtime) that are statically > allocated. So the final Xen binary will be 24KB bigger when > neither UBSAN nor GCOV. > > If this is not considered acceptable, then we could make the > size of configurable in the Kconfig and decide it based on the > features enabled. Both of these features are enabled only in a debug build of Xen, so another option would be to increase Xen size only for a debug build. ~Michal
On 26/06/2023 8:29 am, Henry Wang wrote: > However, I noticed Xen will print below during the Dom0 boot: > (XEN) ================================================================================ > (XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15 > (XEN) left shift of 1 by 31 places cannot be represented in type 'int' > (XEN) Xen WARN at common/ubsan/ubsan.c:172 > > Just want to make sure you also noticed this, otherwise maybe you > can include another patch in the series to fix this? Or I can do that > in case you don't have enough bandwidth. Just as a general note. What UBSAN does and doesn't notice depends on your compiler, optimisation level, etc, and whether you encounter a problem case depends on your hardware. Finding differing reports from different people is entirely normal. ~Andrew
Hi, On 26/06/2023 12:56, Michal Orzel wrote: > > > On 25/06/2023 22:49, Julien Grall wrote: >> >> >> From: Julien Grall <jgrall@amazon.com> >> >> UBSAN has been enabled a few years ago on x86 but was never >> enabled on Arm because the final binary is bigger than 2MB ( >> the maximum we can currently handled). >> >> With the recent rework, it is now possible to grow Xen over 2MB. >> So there is no more roadblock to enable Xen other than increasing >> the reserved area. >> >> On my setup, for arm32, the final binaray was very close to 4MB. >> Furthermore, one may want to enable USBAN and GCOV which would put >> the binary well-over 4MB (both features require for some space). >> Therefore, increase the size to 8MB which should us some margin. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> ---- >> >> The drawback with this approach is that we are adding 6 new >> page-table (3 for boot and 3 for runtime) that are statically >> allocated. So the final Xen binary will be 24KB bigger when >> neither UBSAN nor GCOV. >> >> If this is not considered acceptable, then we could make the >> size of configurable in the Kconfig and decide it based on the >> features enabled. > Both of these features are enabled only in a debug build of Xen, so > another option would be to increase Xen size only for a debug build. At least UBSAN can be selected without DEBUG. For that you need to add EXPERT. Anyway, from your comment, it is not clear to me whether you dislike the existing approach (and why) or you are OK with the increase. Cheers,
Hi Julien, On 26/06/2023 14:56, Julien Grall wrote: > > > Hi, > > On 26/06/2023 12:56, Michal Orzel wrote: >> >> >> On 25/06/2023 22:49, Julien Grall wrote: >>> >>> >>> From: Julien Grall <jgrall@amazon.com> >>> >>> UBSAN has been enabled a few years ago on x86 but was never >>> enabled on Arm because the final binary is bigger than 2MB ( >>> the maximum we can currently handled). >>> >>> With the recent rework, it is now possible to grow Xen over 2MB. >>> So there is no more roadblock to enable Xen other than increasing >>> the reserved area. >>> >>> On my setup, for arm32, the final binaray was very close to 4MB. >>> Furthermore, one may want to enable USBAN and GCOV which would put >>> the binary well-over 4MB (both features require for some space). >>> Therefore, increase the size to 8MB which should us some margin. >>> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> >>> ---- >>> >>> The drawback with this approach is that we are adding 6 new >>> page-table (3 for boot and 3 for runtime) that are statically >>> allocated. So the final Xen binary will be 24KB bigger when >>> neither UBSAN nor GCOV. >>> >>> If this is not considered acceptable, then we could make the >>> size of configurable in the Kconfig and decide it based on the >>> features enabled. >> Both of these features are enabled only in a debug build of Xen, so >> another option would be to increase Xen size only for a debug build. > > At least UBSAN can be selected without DEBUG. For that you need to add > EXPERT. > > Anyway, from your comment, it is not clear to me whether you dislike the > existing approach (and why) or you are OK with the increase. Sorry, I was traveling and did not have time to complete review. I cannot see why increasing the size would be problematic so it is ok. That said, it could be a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated, so that we are on the safe side at the time of activating features like UBSAN/GCOV. Also, it would be nice to update comments in head.S of both arm32 and arm64 above GLOBAL(start) that were left stating: "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment." Other than that: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
On 26/06/2023 08:29, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN > > Typo in title: s/USBAN/UBSAN/ and... > >> >> From: Julien Grall <jgrall@amazon.com> >> >> UBSAN has been enabled a few years ago on x86 but was never >> enabled on Arm because the final binary is bigger than 2MB ( >> the maximum we can currently handled). >> >> With the recent rework, it is now possible to grow Xen over 2MB. >> So there is no more roadblock to enable Xen other than increasing >> the reserved area. >> >> On my setup, for arm32, the final binaray was very close to 4MB. >> Furthermore, one may want to enable USBAN and GCOV which would put > > ...here also. > >> the binary well-over 4MB (both features require for some space). >> Therefore, increase the size to 8MB which should us some margin. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Both typos can be fixed on commit and I think I am good with the > current approach, so: > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> Thanks, I will fix both typos. > > I did a play with UBSAN enabled on FVP (arm64), in my setup, > the result Xen file is 3.7MB: > > -rwxrwxr-x 1 xinwan02 xinwan02 3.7M Jun 26 14:47 xen > lrwxrwxrwx 1 xinwan02 xinwan02 3 Jun 26 14:47 xen.efi -> xen > -rwxrwxr-x 1 xinwan02 xinwan02 14M Jun 26 14:47 xen-syms > > and the Xen and Dom0 booted fine and I can login Dom0. So > technically: > > Tested-by: Henry Wang <Henry.Wang@arm.com> > > However, I noticed Xen will print below during the Dom0 boot: > (XEN) ================================================================================ > (XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15 > (XEN) left shift of 1 by 31 places cannot be represented in type 'int' > (XEN) Xen WARN at common/ubsan/ubsan.c:172 > > Just want to make sure you also noticed this, otherwise maybe you > can include another patch in the series to fix this? I haven't seen this one. Probably because you would need a setup where interrupts are around ID 1022. Or I can do that > in case you don't have enough bandwidth. You have the setup to exercise the problem. So it would be best if you do it. Cheers,
On 27/06/2023 09:09, Michal Orzel wrote: > Hi Julien, > > On 26/06/2023 14:56, Julien Grall wrote: >> >> >> Hi, >> >> On 26/06/2023 12:56, Michal Orzel wrote: >>> >>> >>> On 25/06/2023 22:49, Julien Grall wrote: >>>> >>>> >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> UBSAN has been enabled a few years ago on x86 but was never >>>> enabled on Arm because the final binary is bigger than 2MB ( >>>> the maximum we can currently handled). >>>> >>>> With the recent rework, it is now possible to grow Xen over 2MB. >>>> So there is no more roadblock to enable Xen other than increasing >>>> the reserved area. >>>> >>>> On my setup, for arm32, the final binaray was very close to 4MB. >>>> Furthermore, one may want to enable USBAN and GCOV which would put >>>> the binary well-over 4MB (both features require for some space). >>>> Therefore, increase the size to 8MB which should us some margin. >>>> >>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>> >>>> ---- >>>> >>>> The drawback with this approach is that we are adding 6 new >>>> page-table (3 for boot and 3 for runtime) that are statically >>>> allocated. So the final Xen binary will be 24KB bigger when >>>> neither UBSAN nor GCOV. >>>> >>>> If this is not considered acceptable, then we could make the >>>> size of configurable in the Kconfig and decide it based on the >>>> features enabled. >>> Both of these features are enabled only in a debug build of Xen, so >>> another option would be to increase Xen size only for a debug build. >> >> At least UBSAN can be selected without DEBUG. For that you need to add >> EXPERT. >> >> Anyway, from your comment, it is not clear to me whether you dislike the >> existing approach (and why) or you are OK with the increase. > Sorry, I was traveling and did not have time to complete review. > I cannot see why increasing the size would be problematic so it is ok. That said, it could be > a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated, > so that we are on the safe side at the time of activating features like UBSAN/GCOV. Sure. I will add a comment in this patch. For ... > > Also, it would be nice to update comments in head.S of both arm32 and arm64 above > GLOBAL(start) that were left stating: > "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment." ... this one, I am thinking to drop the comments in patch #5. They don't make sense anymore as we can now in theory deal with Xen up to the size of a L2 mapping (1GB for 4KB). > > Other than that: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> Thanks. Cheers,
> On 28 Jun 2023, at 21:39, Julien Grall <julien@xen.org> wrote: > > > > On 27/06/2023 09:09, Michal Orzel wrote: >> Hi Julien, >> On 26/06/2023 14:56, Julien Grall wrote: >>> >>> >>> Hi, >>> >>> On 26/06/2023 12:56, Michal Orzel wrote: >>>> >>>> >>>> On 25/06/2023 22:49, Julien Grall wrote: >>>>> >>>>> >>>>> From: Julien Grall <jgrall@amazon.com> >>>>> >>>>> UBSAN has been enabled a few years ago on x86 but was never >>>>> enabled on Arm because the final binary is bigger than 2MB ( >>>>> the maximum we can currently handled). >>>>> >>>>> With the recent rework, it is now possible to grow Xen over 2MB. >>>>> So there is no more roadblock to enable Xen other than increasing >>>>> the reserved area. >>>>> >>>>> On my setup, for arm32, the final binaray was very close to 4MB. >>>>> Furthermore, one may want to enable USBAN and GCOV which would put >>>>> the binary well-over 4MB (both features require for some space). >>>>> Therefore, increase the size to 8MB which should us some margin. >>>>> >>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>>> >>>>> ---- >>>>> >>>>> The drawback with this approach is that we are adding 6 new >>>>> page-table (3 for boot and 3 for runtime) that are statically >>>>> allocated. So the final Xen binary will be 24KB bigger when >>>>> neither UBSAN nor GCOV. >>>>> >>>>> If this is not considered acceptable, then we could make the >>>>> size of configurable in the Kconfig and decide it based on the >>>>> features enabled. >>>> Both of these features are enabled only in a debug build of Xen, so >>>> another option would be to increase Xen size only for a debug build. >>> >>> At least UBSAN can be selected without DEBUG. For that you need to add >>> EXPERT. >>> >>> Anyway, from your comment, it is not clear to me whether you dislike the >>> existing approach (and why) or you are OK with the increase. >> Sorry, I was traveling and did not have time to complete review. >> I cannot see why increasing the size would be problematic so it is ok. That said, it could be >> a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated, >> so that we are on the safe side at the time of activating features like UBSAN/GCOV. > > Sure. I will add a comment in this patch. For ... > >> Also, it would be nice to update comments in head.S of both arm32 and arm64 above >> GLOBAL(start) that were left stating: >> "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment." > > ... this one, I am thinking to drop the comments in patch #5. They don't make sense anymore as we can now in theory deal with Xen up to the size of a L2 mapping (1GB for 4KB). > >> Other than that: >> Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > Thanks. > > Cheers, > > -- > Julien Grall Hi Julien, Don’t know if it was pointed out before, but the commit message has a typo s/USBAN/UBSAN/ Cheers, Luca
Hi Julien, > -----Original Message----- > Subject: Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN > > On 26/06/2023 08:29, Henry Wang wrote: > > Hi Julien, > Hi Henry, > > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > > Thanks, I will fix both typos. Great, thanks! > > > > > Just want to make sure you also noticed this, otherwise maybe you > > can include another patch in the series to fix this? > > I haven't seen this one. Probably because you would need a setup where > interrupts are around ID 1022. I took a closer look today, I think the reason is from this device tree node for the FVP: ethernet@202000000 { compatible = "smsc,lan91c111"; reg = <0x2 0x2000000 0x10000>; interrupts = <0xf>; }; The value 0xf is passed to vgic_get_virq_type() as "index" then "intr" in VGIC_ICFG_MASK. Hence the 31 in "(XEN) left shift of 1 by 31 places cannot be represented in type 'int'". > > Or I can do that > > in case you don't have enough bandwidth. > > You have the setup to exercise the problem. So it would be best if you > do it. I've prepared below on top of your series: -#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1)) +#define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1)) If you think it is ok, I will send it out to the list after I double check our CI will not complain about it. Thanks! Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 28/06/2023 22:39, Julien Grall wrote: > > > On 27/06/2023 09:09, Michal Orzel wrote: >> Hi Julien, >> >> On 26/06/2023 14:56, Julien Grall wrote: >>> >>> >>> Hi, >>> >>> On 26/06/2023 12:56, Michal Orzel wrote: >>>> >>>> >>>> On 25/06/2023 22:49, Julien Grall wrote: >>>>> >>>>> >>>>> From: Julien Grall <jgrall@amazon.com> >>>>> >>>>> UBSAN has been enabled a few years ago on x86 but was never >>>>> enabled on Arm because the final binary is bigger than 2MB ( >>>>> the maximum we can currently handled). >>>>> >>>>> With the recent rework, it is now possible to grow Xen over 2MB. >>>>> So there is no more roadblock to enable Xen other than increasing >>>>> the reserved area. >>>>> >>>>> On my setup, for arm32, the final binaray was very close to 4MB. >>>>> Furthermore, one may want to enable USBAN and GCOV which would put >>>>> the binary well-over 4MB (both features require for some space). >>>>> Therefore, increase the size to 8MB which should us some margin. >>>>> >>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>>> >>>>> ---- >>>>> >>>>> The drawback with this approach is that we are adding 6 new >>>>> page-table (3 for boot and 3 for runtime) that are statically >>>>> allocated. So the final Xen binary will be 24KB bigger when >>>>> neither UBSAN nor GCOV. >>>>> >>>>> If this is not considered acceptable, then we could make the >>>>> size of configurable in the Kconfig and decide it based on the >>>>> features enabled. >>>> Both of these features are enabled only in a debug build of Xen, so >>>> another option would be to increase Xen size only for a debug build. >>> >>> At least UBSAN can be selected without DEBUG. For that you need to add >>> EXPERT. >>> >>> Anyway, from your comment, it is not clear to me whether you dislike the >>> existing approach (and why) or you are OK with the increase. >> Sorry, I was traveling and did not have time to complete review. >> I cannot see why increasing the size would be problematic so it is ok. That said, it could be >> a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated, >> so that we are on the safe side at the time of activating features like UBSAN/GCOV. > > Sure. I will add a comment in this patch. For ... > >> >> Also, it would be nice to update comments in head.S of both arm32 and arm64 above >> GLOBAL(start) that were left stating: >> "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment." > > ... this one, I am thinking to drop the comments in patch #5. They don't > make sense anymore as we can now in theory deal with Xen up to the size > of a L2 mapping (1GB for 4KB). Seems right. ~Michal
On 29/06/2023 02:36, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> Subject: Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN >> >> On 26/06/2023 08:29, Henry Wang wrote: >>> Hi Julien, >> Hi Henry, >> >>> Reviewed-by: Henry Wang <Henry.Wang@arm.com> >> >> Thanks, I will fix both typos. > > Great, thanks! > >> >>> >>> Just want to make sure you also noticed this, otherwise maybe you >>> can include another patch in the series to fix this? >> >> I haven't seen this one. Probably because you would need a setup where >> interrupts are around ID 1022. > > I took a closer look today, I think the reason is from this device tree node > for the FVP: > > ethernet@202000000 { > compatible = "smsc,lan91c111"; > reg = <0x2 0x2000000 0x10000>; > interrupts = <0xf>; > }; > > The value 0xf is passed to vgic_get_virq_type() as "index" then "intr" in > VGIC_ICFG_MASK. Hence the 31 in > "(XEN) left shift of 1 by 31 places cannot be represented in type 'int'". > >> >> Or I can do that >>> in case you don't have enough bandwidth. >> >> You have the setup to exercise the problem. So it would be best if you >> do it. > > I've prepared below on top of your series: > -#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1)) > +#define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1)) This LGTM. Feel free to send a patch. Cheers,
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 61e581b8c2b0..06b5ff755c95 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -17,6 +17,7 @@ config ARM select HAS_PASSTHROUGH select HAS_PDX select HAS_PMAP + select HAS_UBSAN select IOMMU_FORCE_PT_SHARE config ARCH_DEFCONFIG diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index 6d246ab23c48..1bbf4cc9958d 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -74,10 +74,10 @@ /* * ARM32 layout: * 0 - 2M Unmapped - * 2M - 4M Xen text, data, bss - * 4M - 6M Fixmap: special-purpose 4K mapping slots - * 6M - 10M Early boot mapping of FDT - * 10M - 12M Livepatch vmap (if compiled in) + * 2M - 10M Xen text, data, bss + * 10M - 12M Fixmap: special-purpose 4K mapping slots + * 12M - 16M Early boot mapping of FDT + * 16M - 18M Livepatch vmap (if compiled in) * * 32M - 128M Frametable: 32 bytes per page for 12GB of RAM * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address @@ -94,10 +94,10 @@ * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4]) * (Relative offsets) * 0 - 2M Unmapped - * 2M - 4M Xen text, data, bss - * 4M - 6M Fixmap: special-purpose 4K mapping slots - * 6M - 10M Early boot mapping of FDT - * 10M - 12M Livepatch vmap (if compiled in) + * 2M - 10M Xen text, data, bss + * 10M - 12M Fixmap: special-purpose 4K mapping slots + * 12M - 16M Early boot mapping of FDT + * 16M - 18M Livepatch vmap (if compiled in) * * 1G - 2G VMAP: ioremap and early_ioremap * @@ -124,7 +124,7 @@ #define XEN_VIRT_START (SLOT0(4) + _AT(vaddr_t, MB(2))) #endif -#define XEN_VIRT_SIZE _AT(vaddr_t, MB(2)) +#define XEN_VIRT_SIZE _AT(vaddr_t, MB(8)) #define XEN_NR_ENTRIES(lvl) (XEN_VIRT_SIZE / XEN_PT_LEVEL_SIZE(lvl)) #define FIXMAP_VIRT_START (XEN_VIRT_START + XEN_VIRT_SIZE)