[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH
Hi Roger,
On 22/08/18 16:18, Roger Pau Monné wrote:
On Wed, Aug 22, 2018 at 04:00:45PM +0100, Julien Grall wrote:
Currently, the toolstack is considering Arm guest always PV. However,
they are very similar to PVH because HW virtualization extension are used
and QEMU is not started. So switch Arm guest type to PVH.
To keep compatibility with toolstack creating Arm guest with PV type
(e.g libvirt), libxl will now convert those guests to PVH.
Furthermore, the default type for Arm in xl will now be PVH to allow
smooth transition for user.
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
---
This was discussed at Xen Summit and also in various thread on
xen-devel. The latest one was when Andrew sent a patch to deny guest creation
on Arm with XEN_DOMCTL_CDF_hap unset.
I suspect we first implemented Arm guest as PV in libxl because PVH was
non-existent and the type was easier to avoid spawning QEMU. Note that
Linux and Xen are already considering Arm guest as PVH.
Changes in v2:
- Rather than denying PV guest, convert them to PVH.
---
docs/man/xl.cfg.pod.5.in | 3 ++-
tools/libxl/libxl_arch.h | 3 ++-
tools/libxl/libxl_arm.c | 39 +++++++++++++++++++++++++++++++++++++--
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_x86.c | 3 ++-
tools/xl/xl_parse.c | 4 ++++
6 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b72718151b..a0568e397d 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -86,7 +86,8 @@ guest operating systems. This is the default.
Specifies that this is to be an PVH domain. That is a lightweight HVM-like
guest without a device model and without many of the emulated devices
-available to HVM guests. Note that this mode requires a PVH aware kernel.
+available to HVM guests. Note that this mode requires a PVH aware kernel on
+x86.
Should you add "This is the default on ARM". And likewise modify
'type="pv"' so it's last sentence is "This is the default on x86"?
Good idea. I will update the doc.
=item B<type="hvm">
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 5ab0c95974..930570ef1e 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -65,7 +65,8 @@ _hidden
int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq);
_hidden
-void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info);
+void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info);
_hidden
int libxl__arch_extra_memory(libxl__gc *gc,
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 18c41f4ee9..0cf92d7af4 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -978,7 +978,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
int rc;
uint64_t val;
- assert(info->type == LIBXL_DOMAIN_TYPE_PV);
+ if (info->type != LIBXL_DOMAIN_TYPE_PVH) {
+ LOG(ERROR, "Unsupported Arm guest type %s",
+ libxl_domain_type_to_string(info->type));
+ return ERROR_FAIL;
Nit: ERROR_INVAL might be better here.
+ }
/* Set the value of domain param HVM_PARAM_CALLBACK_IRQ. */
val = MASK_INSR(HVM_PARAM_CALLBACK_TYPE_PPI,
@@ -1135,10 +1139,41 @@ int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t
domid, int irq)
return xc_domain_bind_pt_spi_irq(CTX->xch, domid, irq, irq);
}
-void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info)
+void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info)
{
/* ACPI is disabled by default */
libxl_defbool_setdefault(&b_info->acpi, false);
+
+ /*
+ * Arm guest are now considered as PVH by the toolstack. To allow
+ * compatibility with previous toolstack, PV guest are automatically
+ * converted to PVH.
+ */
+ if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
+ return;
+
+ LOG(WARN, "Converting PV guest to PVH");
+ LOG(WARN, "ARM guest are now PVH. Please update your toolstack.");
"Please update your configuration file.". Updating the toolstack won't
make this message go away AFAICT :).
In the case of libvirt, you don't have PVH support. So you would need to
update your toolstack here.
How about "Please fix your configuration file/toolstack"?
+
+ b_info->type = LIBXL_DOMAIN_TYPE_PVH;
+
+ /*
+ * They only field in u.pv that matters on Arm are: kernel, cmdline,
+ * ramdisk.
+ */
+
+ if (!b_info->kernel && b_info->u.pv.kernel)
+ b_info->kernel = b_info->u.pv.kernel;
+
+ if (!b_info->ramdisk && b_info->u.pv.ramdisk)
+ b_info->ramdisk = b_info->u.pv.ramdisk;
+
+ if (!b_info->cmdline && b_info->u.pv.cmdline)
+ b_info->cmdline = b_info->u.pv.cmdline;
+
+ /* Reset b_info->u.pvh to default values */
+ memset(&b_info->u.pvh, 0, sizeof(b_info->u.pvh));
I'm afraid that's not correct. The default values for u.pvh are set
by libxl__domain_build_info_setdefault.
I thought that this should be covered by the switch right after the call
of libxl__arch_domain_build_info_setdefault. Did I miss anything?
What I wanted to do here is resetting the union to 0 so you don't get
data mangled by the pv fields.
}
/*
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d4fa06daea..a6431c5d3f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -215,7 +215,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
- libxl__arch_domain_build_info_setdefault(b_info);
+ libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
switch (b_info->type) {
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 81523a568f..8b6759c089 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -613,7 +613,8 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc
*gc,
return rc;
}
-void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info)
+void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info)
{
libxl_defbool_setdefault(&b_info->acpi, true);
}
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 971ec1bc56..0bda28152b 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1286,7 +1286,11 @@ void parse_config_data(const char *config_source,
}
if (c_info->type == LIBXL_DOMAIN_TYPE_INVALID)
+#if defined(__arm__) || defined(__aarch64__)
I think #ifdef CONFIG_ARM should DTRT and it's cleaner IMO.
CONFIG_ARM is not defined in the tools C source. So that's the only way
to know if you are on Arm. This follows what is done in libxc.
I would be happy to introduce CONFIG_ARM/CONFIG_X86 if people thinks
this would be useful in other places.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
