Hi Oleksandr,
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):
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e0039..df59a0521412 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -269,6 +269,20 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
return fdt_property(fdt, "reg", regs, sizeof(regs));
}
+static int fdt_property_reg_placeholder(libxl__gc *gc, void *fdt,
+ unsigned int addr_cells,
+ unsigned int size_cells,
+ unsigned int num_regs)
+{
+ uint32_t regs[num_regs * (addr_cells + size_cells)];
+
+ for (i = 0 ; i < num_regs; i++) {
+ set_range(&cells, addr_cells, size_cells, 0, 0);
+ }
+
+ return fdt_property(fdt, "reg", regs, sizeof(regs));
+}
+
static int make_root_properties(libxl__gc *gc,
const libxl_version_info *vers,
+{
+ void *fdt = dom->devicetree_blob;
+ uint64_t region_size[GUEST_RAM_BANKS] = {0},
region_base[GUEST_RAM_BANKS],
+ bankend[GUEST_RAM_BANKS];
+ uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+ (GUEST_RAM_BANKS + 1)];
+ be32 *cells = ®s[0];
+ const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+ const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+ unsigned int i, len, nr_regions = 0;
+ libxl_dominfo info;
+ int offset, rc;
+
+ offset = fdt_path_offset(fdt, "/hypervisor");
+ assert(offset > 0);
+
+ rc = libxl_domain_info(CTX, &info, dom->guest_domid);
+ assert(!rc);
+
+ assert(info.gpaddr_bits <= 64);
Neither of the two should be assert(). They should be proper check so
we don't end up with a disaster (in particularly for the former) if
there is a problem.
I looked at the similar finalise_*(), and it looks like no one bothers
with returning an error. Of course, this is not an excuse, will add a
proper check.
This is a bit unfortunate. I would prefer if this can be avoided for new
code (the more libxl__arch_domain_finalise_hw_description() can already
propagate the error).
Cheers,
--
Julien Grall