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

[Julien Grall <julien@xxxxxxx>]

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>

Cheers,

--
Julien Grall