Re: [PATCH 2/2] xen/arm: Account for domU dtb bootmodule size separately

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 11/07/2023 18:07, Julien Grall wrote:
> 
> 
> Hi Michal,
> 
> On 11/07/2023 09:29, Michal Orzel wrote:
>> At the moment, we limit the allocation size when creating a domU dtb to
>> 4KB, which is not enough when using a passthrough dtb with several nodes.
>> Improve the handling by accounting for a dtb bootmodule (if present)
>> size separately, while keeping 4KB for the Xen generated nodes (still
>> plenty of space for new nodes). Also, cap the allocation size to 2MB,
>> which is the max dtb size allowed.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> Note for the future:
>> As discussed with Julien, really the best way would be to generate dtb 
>> directly
>> in the guest memory, where no allocation would be necessary. This of course
>> requires some rework. The solution in this patch is good enough for now and
>> can be treated as an intermediated step to support dtb creation of various
>> sizes.
> 
> Thanks for summarizing our discussion :).
> 
>> ---
>>   xen/arch/arm/domain_build.c | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f2134f24b971..1dc0eca37bd6 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3257,14 +3257,15 @@ static int __init 
>> domain_handle_dtb_bootmodule(struct domain *d,
>>   }
>>
>>   /*
>> - * The max size for DT is 2MB. However, the generated DT is small, 4KB
>> - * are enough for now, but we might have to increase it in the future.
>> + * The max size for DT is 2MB. However, the generated DT is small (not 
>> including
>> + * domU passthrough DT nodes whose size we account separately), 4KB are 
>> enough
>> + * for now, but we might have to increase it in the future.
>>    */
>>   #define DOMU_DTB_SIZE 4096
>>   static int __init prepare_dtb_domU(struct domain *d, struct kernel_info 
>> *kinfo)
>>   {
>>       int addrcells, sizecells;
>> -    int ret;
>> +    int ret, fdt_size = DOMU_DTB_SIZE;
> 
> Can fdt_size be unsigned?
I used int because by looking at all the fdt_create() calls in our codebase
we seem to use int and not unsigned. Also, I used min() that does strict type 
checking
and SZ_2M is int. So if you want, I can use unsigned int but will also have to 
use
MIN() macro instead not to do type checking (I cannot use MB(2) as it has ULL 
type
and do not want to use min() with cast).

Also, are you OK with the rest of the code?

~Michal