Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 28 Jun 2023, at 21:39, Julien Grall <julien@xxxxxxx> 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@xxxxxxxxxx>
>>>>> 
>>>>> 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@xxxxxxxxxx>
>>>>> 
>>>>> ----
>>>>> 
>>>>> 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@xxxxxxx>
> 
> 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