Re: [PATCH V6 2/2] libxl/arm: Add handling of extended regions for DomU

[Julien Grall <julien@xxxxxxx>]

Hi,

On 13/10/2021 17:44, Oleksandr wrote:
> On 13.10.21 19:24, Julien Grall wrote:
> > On 13/10/2021 16:49, Oleksandr wrote:
> > > On 13.10.21 18:15, Julien Grall wrote:
> > > > On 13/10/2021 14:46, Oleksandr wrote:
> > > > > Hi Julien
> > > > Hi Oleksandr,
> > > Hi Julien
> > >
> > >
> > > Thank you for the prompt response.
> > >
> > >
> > > > > On 13.10.21 00:43, Oleksandr wrote:
> > > > > diff --git a/tools/libs/light/libxl_arm.c 
> > > > > b/tools/libs/light/libxl_arm.c
> > > > > index e3140a6..53ae0f3 100644
> > > > > --- a/tools/libs/light/libxl_arm.c
> > > > > +++ b/tools/libs/light/libxl_arm.c
> > > > > @@ -615,9 +615,12 @@ static int make_hypervisor_node(libxl__gc *gc, 
> > > > > void *fdt,
> > > > >                                 "xen,xen");
> > > > >       if (res) return res;
> > > > >
> > > > > -    /* reg 0 is grant table space */
> > > > > +    /*
> > > > > +     * reg 0 is a placeholder for grant table space, reg 1...N are
> > > > > +     * the placeholders for extended regions.
> > > > > +     */
> > > > >       res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS, 
> > > > > GUEST_ROOT_SIZE_CELLS,
> > > > > -                            1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
> > > > > +                            GUEST_RAM_BANKS + 1, 0, 0, 0, 0, 0, 0);
> > > > Here you are relying on GUEST_RAM_BANKS == 2. I think this is pretty 
> > > > fragile as this may change in the future.
> > > > fdt_property_regs() is not really suitable for here. I would suggest 
> > > > to create a new helper fdt_property_placeholder() which takes the 
> > > > address cells, size cells and the number of ranges. The function 
> > > > will then create N range that are zeroed.
> > > You are right. Probably, we could add an assert here for now to be 
> > > triggered if "GUEST_RAM_BANKS != 2"?
> > > But, if you still think we need to make it with the helper right now, 
> > > I will. However, I don't know how long this will take.
> > I would prefer if we introduce the helper now. Below a potential 
> > version (not compiled):
>
> You wouldn't believe)))
> I decided to not wait for the answer and re-check. So, I ended up with 
> the similar to what you suggested below. Thank you.
> Yes, it will work if add missing locals. However, I initially named it 
> exactly as was suggested (fdt_property_placeholder) and got a 
> compilation error, since fdt_property_placeholder is already present.
> So, I was thinking to choose another name or to even open-code it, but I 
> see you already proposed new name fdt_property_reg_placeholder.
Ah I didn't realize that libfdt already implemented 
fdt_property_placeholder(). The function sounds perfect for us (and the 
code is nicer :)), but unfortunately this was introdcued only in 2017. 
So it is possible that some distros may not ship the last libfdt.
So for now, I think we need to implement our own. In a follow-up we 
could try to provide a compat layer as we did for other missing fdt_* 
helpers.
Cheers,

--
Julien Grall