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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 13/07/2023 20:15, Julien Grall wrote:
> 
> 
> On 12/07/2023 08:01, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
>>
>> 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.
> 
> This is a bit of a mess because xmalloc_bytes() is expecting an unsigned
> long parameter. So we have some inconsistency here and we need to chose
> a side.
> 
> My preference would be to use the 'unsigned int/long' because the value
> is not meant to be negative.
> 
> 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).
> 
> By "use min() with cast", do you mean using min_t()? I would be OK with
> using MIN().
> 
>> Also, are you OK with the rest of the code?
> 
> The rest is fine to me. Anyway, I am OK with this patch as-is. So:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks. So, let's keep it as is and one day we may just choose a side
and do refactoring globally for consistency.

~Michal