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.or...@amd.com>
>>>> ---
>>>> 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 <jgr...@amazon.com>
Thanks. So, let's keep it as is and one day we may just choose a side
and do refactoring globally for consistency.
~Michal
