[PATCH 0/2] xen/arm: Support for bigger domU passthrough dtbs

[PATCH 0/2] xen/arm: Support for bigger domU passthrough dtbs

[Michal Orzel]

First patch with a fix (more for a logic than a bug) added to series for
ease of merging.

Michal Orzel (2):
  xen/arm: Fix domain_handle_dtb_bootmodule() error path
  xen/arm: Account for domU dtb bootmodule size separately

 xen/arch/arm/domain_build.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)


base-commit: b831326ee2f9ed94523b3d8b0fb2da2a82113e9e
-- 
2.25.1

[PATCH 1/2] xen/arm: Fix domain_handle_dtb_bootmodule() error path

[Michal Orzel]

Fix the error path in domain_handle_dtb_bootmodule(), so that the memory
previously mapped is unmapped before returning the error code. This is
because the function shall not make assumptions on the way of handling
its error code in the callers. Today we call panic in case of domU
creation failure, so having memory not unmapped is not a bug, but it can
change.

Similarly, fix prepare_dtb_domU() so that the memory allocated is freed
before returning the error code from domain_handle_dtb_bootmodule().

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/domain_build.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d0d6be922db1..f2134f24b971 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3204,7 +3204,7 @@ static int __init domain_handle_dtb_bootmodule(struct domain *d,
 
     res = check_partial_fdt(pfdt, kinfo->dtb_bootmodule->size);
     if ( res < 0 )
-        return res;
+        goto out;
 
     for ( node_next = fdt_first_subnode(pfdt, 0); 
           node_next > 0;
@@ -3235,7 +3235,7 @@ static int __init domain_handle_dtb_bootmodule(struct domain *d,
                                  DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
                                  false);
             if ( res )
-                return res;
+                goto out;
             continue;
         }
         if ( dt_node_cmp(name, "passthrough") == 0 )
@@ -3245,11 +3245,12 @@ static int __init domain_handle_dtb_bootmodule(struct domain *d,
                                  DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
                                  true);
             if ( res )
-                return res;
+                goto out;
             continue;
         }
     }
 
+ out:
     iounmap(pfdt);
 
     return res;
@@ -3326,7 +3327,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     {
         ret = domain_handle_dtb_bootmodule(d, kinfo);
         if ( ret )
-            return ret;
+            goto err;
     }
 
     ret = make_gic_domU_node(kinfo);
-- 
2.25.1

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

[Michal Orzel]

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@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.
---
 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;
 
     kinfo->phandle_gic = GUEST_PHANDLE_GIC;
     kinfo->gnttab_start = GUEST_GNTTAB_BASE;
@@ -3273,11 +3274,18 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     addrcells = GUEST_ROOT_ADDRESS_CELLS;
     sizecells = GUEST_ROOT_SIZE_CELLS;
 
-    kinfo->fdt = xmalloc_bytes(DOMU_DTB_SIZE);
+    /* Account for domU passthrough DT size */
+    if ( kinfo->dtb_bootmodule )
+        fdt_size += kinfo->dtb_bootmodule->size;
+
+    /* Cap to max DT size if needed */
+    fdt_size = min(fdt_size, SZ_2M);
+
+    kinfo->fdt = xmalloc_bytes(fdt_size);
     if ( kinfo->fdt == NULL )
         return -ENOMEM;
 
-    ret = fdt_create(kinfo->fdt, DOMU_DTB_SIZE);
+    ret = fdt_create(kinfo->fdt, fdt_size);
     if ( ret < 0 )
         goto err;
 
-- 
2.25.1

Re: [PATCH 1/2] xen/arm: Fix domain_handle_dtb_bootmodule() error path

[Luca Fancellu]

> On 11 Jul 2023, at 09:29, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Fix the error path in domain_handle_dtb_bootmodule(), so that the memory
> previously mapped is unmapped before returning the error code. This is
> because the function shall not make assumptions on the way of handling
> its error code in the callers. Today we call panic in case of domU
> creation failure, so having memory not unmapped is not a bug, but it can
> change.
> 
> Similarly, fix prepare_dtb_domU() so that the memory allocated is freed
> before returning the error code from domain_handle_dtb_bootmodule().
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

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

[Luca Fancellu]

> On 11 Jul 2023, at 09:29, Michal Orzel <michal.orzel@amd.com> 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@amd.com>

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH 1/2] xen/arm: Fix domain_handle_dtb_bootmodule() error path

[Julien Grall]

Hi,

On 11/07/2023 10:15, Luca Fancellu wrote:
> 
> 
>> On 11 Jul 2023, at 09:29, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Fix the error path in domain_handle_dtb_bootmodule(), so that the memory
>> previously mapped is unmapped before returning the error code. This is
>> because the function shall not make assumptions on the way of handling
>> its error code in the callers. Today we call panic in case of domU
>> creation failure, so having memory not unmapped is not a bug, but it can
>> change.
>>
>> Similarly, fix prepare_dtb_domU() so that the memory allocated is freed
>> before returning the error code from domain_handle_dtb_bootmodule().
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Julien Grall]

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@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?

>   
>       kinfo->phandle_gic = GUEST_PHANDLE_GIC;
>       kinfo->gnttab_start = GUEST_GNTTAB_BASE;
> @@ -3273,11 +3274,18 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>       addrcells = GUEST_ROOT_ADDRESS_CELLS;
>       sizecells = GUEST_ROOT_SIZE_CELLS;
>   
> -    kinfo->fdt = xmalloc_bytes(DOMU_DTB_SIZE);
> +    /* Account for domU passthrough DT size */
> +    if ( kinfo->dtb_bootmodule )
> +        fdt_size += kinfo->dtb_bootmodule->size;
> +
> +    /* Cap to max DT size if needed */
> +    fdt_size = min(fdt_size, SZ_2M);
> +
> +    kinfo->fdt = xmalloc_bytes(fdt_size);
>       if ( kinfo->fdt == NULL )
>           return -ENOMEM;
>   
> -    ret = fdt_create(kinfo->fdt, DOMU_DTB_SIZE);
> +    ret = fdt_create(kinfo->fdt, fdt_size);
>       if ( ret < 0 )
>           goto err;
>   

Cheers,

-- 
Julien Grall

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

[Michal Orzel]

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@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. 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

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

[Julien Grall]

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@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 <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

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

[Michal Orzel]

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@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 <jgrall@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

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

[Julien Grall]

Hi,

On 14/07/2023 08:04, Michal Orzel wrote:
> Thanks. So, let's keep it as is and one day we may just choose a side
> and do refactoring globally for consistency.

The series is now committed.

Cheers,

-- 
Julien Grall