[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.21 00:22, Julien Grall wrote:
Hi Oleksandr,
Hi Julien, Ian.
Julien, thank you for the detailed answer, I will analyze it tomorrow.
Ian, I think, there is no reason in providing git branch with the acks
folded in + my minor fix for the debug message as it was discussed
before, it sounds like there is more work to do, so it is going to be a
new version anyway.
On 12/10/2021 18:42, Oleksandr wrote:
On 12.10.21 19:05, Julien Grall wrote:
On 11/10/2021 18:48, Oleksandr Tyshchenko wrote:
---
tools/libs/light/libxl_arm.c | 76
++++++++++++++++++++++++++++++++++++++++---
xen/include/public/arch-arm.h | 2 ++
2 files changed, 73 insertions(+), 5 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c
b/tools/libs/light/libxl_arm.c
index e3140a6..c0e8415 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -598,9 +598,20 @@ static int make_timer_node(libxl__gc *gc, void
*fdt,
return 0;
}
+#define ALIGN_UP_TO_2MB(x) (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
static int make_hypervisor_node(libxl__gc *gc, void *fdt,
- const libxl_version_info *vers)
+ const libxl_version_info *vers,
+ const libxl_domain_build_info
*b_info,
+ const struct xc_dom_image *dom)
{
+ uint64_t region_size[GUEST_RAM_BANKS] = {0},
region_base[GUEST_RAM_BANKS],
+ bank1end, ramsize;
+ uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS +
GUEST_ROOT_SIZE_CELLS) *
+ (GUEST_RAM_BANKS + 1)];
+ be32 *cells = ®s[0];
+ unsigned int i, len, nr_regions = 0;
+ libxl_dominfo info;
int res;
gic_interrupt intr;
@@ -615,9 +626,64 @@ static int make_hypervisor_node(libxl__gc
*gc, void *fdt,
"xen,xen");
if (res) return res;
- /* reg 0 is grant table space */
- res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
GUEST_ROOT_SIZE_CELLS,
- 1,GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
+ if (strcmp(dom->guest_type, "xen-3.0-aarch64")) {
+ LOG(WARN, "The extended regions are only supported for
64-bit guest currently");
+ goto out;
+ }
I understand why we want to limit to 64-bit domain for dom0. But I
am not sure this is warrant for 32-bit domain. At worse, the guest
will ignore the bank because it is not usable. So could we drop the
check?
Yes.
+
+ res = libxl_domain_info(CTX, &info, dom->guest_domid);
+ if (res) return res;
+
+ assert(info.gpaddr_bits >= 32 && info.gpaddr_bits <= 48);
What could go wrong below if gpaddr_bits is not within this range?
if info.gpaddr_bits is less than 64, then nothing bad, otherwise, I
assume we will get shift count overflow.
So I think the assert() is not suitable here because even if the
gpaddr_bits is provided by the hypervisor (and therefore should be
trusted), this is a different component so hardening the code is a
good practice.
In this case, I would check that info.gpaddr_bits <= 64 and return an
error. The reason I am suggesting <= 64 and not 48 is because Arm
already supports 52 bits address space. Yet, I still like to avoid
this assumption in the code. Something like below should work:
bank1end = GUEST_RAM1_BASE + GUEST_RAM1_SIZE - 1;
bank1end = min(bank1end, ~(0ULL) >> (64 - info.gpaddr_bits);
+
+ /*
+ * Try to allocate separate 2MB-aligned extended regions from
the first
+ * (below 4GB) and second (above 4GB) RAM banks taking into
the account
+ * the maximum supported guest physical address space size and
the amount
+ * of memory assigned to the guest.
+ * As the guest memory layout is not populated yet we cannot
rely on
+ * dom->rambank_size[], so calculate the actual size of both
banks using
+ * "max_memkb" value.
+ */
At the moment, libxl doesn't know how libxc will allocate the
memory. We may decide in the future to have only a small amount of
memory below 4GB and then the rest above 4GB. With this approach it
would be more difficult to modify the memory layout. Instead, I
think we should create a placeholder that is updated once we know
the banks in libxl__arch_domain_finalise_hw_description.
If I got your point correctly, this is close to how it was done from
the beginning. Yes, we can create placeholder(s) here and then update
them once the memory layout is populated. The problem is that we
won't be able to remove the placeholder(s) if we fail to allocate
region(s) for some reasons. So, we should know for sure in advance
how many region(s) we will be able to allocate later on in order to
create the required number of placeholders right now... Please, look
at the TODO I wrote in finalise_ext_region() [1]. Or I misread your
point?
You read correctly my point. However, I disagree that it is a problem
to remove the placeholder if we fail to allocate the amount of regions
expected.
Looking at libfdt, I can see two ways to deal with it:
1) Use fdt_setprop()
2) Delete the property using fdt_delprop() and then recreate it with
fdt_appendprop()
The first solution is ideal and I think can work here to downsize the
property. At worse, the second solution should work as the FDT blob
will not increase.
We also probably want to mention in the memory layout in
public/arch-arm.h this decision as the suggested way to find
extended regions will definitely impact our decision to re-order the
memory layout or shrink some regions in the future (I have in mind
the PCI Passthrough work).
Sorry, I couldn't parse.
So this patch is relying on the fact that the regions reserved for the
RAM are big enough to also accommodate the extended regions.
I am happy with this approach. However, I would like the approach to
be documented in arch-arm.h because this is the first place one would
look to understand the memory layout. This will be helpful if/when we
need to modify the guest memory layout.
+ ramsize = b_info->max_memkb * 1024;
+ if (ramsize <= GUEST_RAM0_SIZE) {
+ region_base[0] = GUEST_RAM0_BASE + ALIGN_UP_TO_2MB(ramsize);
+ region_size[0] = GUEST_RAM0_SIZE - ALIGN_UP_TO_2MB(ramsize);
+ region_base[1] = GUEST_RAM1_BASE;
+ } else
+ region_base[1] = GUEST_RAM1_BASE +
+ ALIGN_UP_TO_2MB(ramsize - GUEST_RAM0_SIZE);
+
+ bank1end = min(1ULL << info.gpaddr_bits, GUEST_RAM1_BASE +
GUEST_RAM1_SIZE);
+ if (bank1end > region_base[1])
+ region_size[1] = bank1end - region_base[1];
It would be best to not rely on the fact that Bank on is always
below 4GB. If the code is too complex then we should look to add a
BUILD_BUG_ON() to avoid any surprise.
Yes, I can add:
BUILD_BUG_ON((GUEST_RAM0_BASE + GUEST_RAM0_SIZE) > GB(4));
I am OK with that. But I wonder if we could simply use min(..., ) to
avoid the BUILD_BUG_ON().
Cheers,
--
Regards,
Oleksandr Tyshchenko
|
