[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.