| Message ID | 20180822150045.22864-3-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | tools/libxl: Switch Arm guest type to PVH | expand |
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@arm.com> > > --- > > 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"? > > =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 :). > + > + 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. > } > > /* > 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. Thanks, Roger.
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@arm.com> >> >> --- >> >> 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,
On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: > 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: > > > -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"? I would use "fix your configuration or update your toolstack", but I'm fine with the above. > > > > > + > > > + 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? Oh right, libxl__arch_domain_build_info_setdefault is called by libxl__domain_build_info_setdefault. > What I wanted to do here is resetting the union to 0 so you don't get data > mangled by the pv fields. Another possible option I think would be to mark those fields as deprecated in the IDL, and libxl__domain_build_info_copy_deprecated will take care of copying them to the new place. In fact I think all guest types should be using the top level kernel, ramdisk and cmdline fields. I'm not specially comfortable with changing the guest type in the middle of libxl__domain_build_info_setdefault, but I also don't have a much better suggestion apart from using the deprecation helper. From what you say above I assume bootloader or bootloader arguments are not used by ARM? > > > > > } > > > /* > > > 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. The tools makefile already uses CONFIG_ARM/X86, so I think it would make sense to have this for the code as well. In any case, I don't feel this should be done just for this patch, so I'm fine as-is. Thanks, Roger.
On Thu, Aug 23, 2018 at 09:58:57AM +0200, Roger Pau Monné wrote: [...] > > > What I wanted to do here is resetting the union to 0 so you don't get data > > mangled by the pv fields. > > Another possible option I think would be to mark those fields as > deprecated in the IDL, and libxl__domain_build_info_copy_deprecated I think this is a better approach. > will take care of copying them to the new place. In fact I think all > guest types should be using the top level kernel, ramdisk and cmdline > fields. > > I'm not specially comfortable with changing the guest type in the > middle of libxl__domain_build_info_setdefault, but I also don't have a > much better suggestion apart from using the deprecation helper. > > From what you say above I assume bootloader or bootloader arguments > are not used by ARM? > > > > > > > > } > > > > /* > > > > 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. > > The tools makefile already uses CONFIG_ARM/X86, so I think it would > make sense to have this for the code as well. In any case, I don't > feel this should be done just for this patch, so I'm fine as-is. > I think CONFIG_ARM should already work. There are several CONFIG_ARM* in toolstack code, though not in libxl. In any case, I think the code is fine as-is. Wei. > Thanks, Roger.
On 23/08/18 08:58, Roger Pau Monné wrote: > On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: >>> >>>> + >>>> + 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? > > Oh right, libxl__arch_domain_build_info_setdefault is called by > libxl__domain_build_info_setdefault. > >> What I wanted to do here is resetting the union to 0 so you don't get data >> mangled by the pv fields. > > Another possible option I think would be to mark those fields as > deprecated in the IDL, and libxl__domain_build_info_copy_deprecated > will take care of copying them to the new place. In fact I think all > guest types should be using the top level kernel, ramdisk and cmdline > fields. I will have a look at it. > > I'm not specially comfortable with changing the guest type in the > middle of libxl__domain_build_info_setdefault, but I also don't have a > much better suggestion apart from using the deprecation helper. > > From what you say above I assume bootloader or bootloader arguments > are not used by ARM? That's correct. Cheers,
Hi Wei, On 28/08/18 17:45, Wei Liu wrote: > On Thu, Aug 23, 2018 at 09:58:57AM +0200, Roger Pau Monné wrote: > [...] >> >>> What I wanted to do here is resetting the union to 0 so you don't get data >>> mangled by the pv fields. >> >> Another possible option I think would be to mark those fields as >> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated > > I think this is a better approach. > >> will take care of copying them to the new place. In fact I think all >> guest types should be using the top level kernel, ramdisk and cmdline >> fields. >> >> I'm not specially comfortable with changing the guest type in the >> middle of libxl__domain_build_info_setdefault, but I also don't have a >> much better suggestion apart from using the deprecation helper. >> >> From what you say above I assume bootloader or bootloader arguments >> are not used by ARM? >> >>>> >>>>> } >>>>> /* >>>>> 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. >> >> The tools makefile already uses CONFIG_ARM/X86, so I think it would >> make sense to have this for the code as well. In any case, I don't >> feel this should be done just for this patch, so I'm fine as-is. >> > > I think CONFIG_ARM should already work. I don't think so. The top makefile does not pass -DCONFIG_ARM on compiler command line. This is only done in sub-directory such as console and libacpi. > > There are several CONFIG_ARM* in toolstack code, though not in libxl. In > any case, I think the code is fine as-is. I am happy to introduce CONFIG_ARM/CONFIG_X86 if you don't mind adding -DCONFIG_ARM on the command line. Cheers,
On 03/09/18 12:09, Julien Grall wrote: > > > On 23/08/18 08:58, Roger Pau Monné wrote: >> On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: >>>> >>>>> + >>>>> + 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? >> >> Oh right, libxl__arch_domain_build_info_setdefault is called by >> libxl__domain_build_info_setdefault. >> >>> What I wanted to do here is resetting the union to 0 so you don't get >>> data >>> mangled by the pv fields. >> >> Another possible option I think would be to mark those fields as >> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated >> will take care of copying them to the new place. In fact I think all >> guest types should be using the top level kernel, ramdisk and cmdline >> fields. > > I will have a look at it. > >> >> I'm not specially comfortable with changing the guest type in the >> middle of libxl__domain_build_info_setdefault, but I also don't have a >> much better suggestion apart from using the deprecation helper. I forgot to answer to this bit. I don't think the deprecation helper will do all the jobs. There are still PV specific parameters: slack_memkb, features, e820_host. Those are not necessary for Arm, if you don't zero them then you will not initialize the PVH structure with default values. How do you suggest to handle them? Cheers,
On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: > On 03/09/18 12:09, Julien Grall wrote: > > > > > > On 23/08/18 08:58, Roger Pau Monné wrote: > > > On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: > > > > > > > > > > > + > > > > > > + 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? > > > > > > Oh right, libxl__arch_domain_build_info_setdefault is called by > > > libxl__domain_build_info_setdefault. > > > > > > > What I wanted to do here is resetting the union to 0 so you > > > > don't get data > > > > mangled by the pv fields. > > > > > > Another possible option I think would be to mark those fields as > > > deprecated in the IDL, and libxl__domain_build_info_copy_deprecated > > > will take care of copying them to the new place. In fact I think all > > > guest types should be using the top level kernel, ramdisk and cmdline > > > fields. > > > > I will have a look at it. > > > > > > > > I'm not specially comfortable with changing the guest type in the > > > middle of libxl__domain_build_info_setdefault, but I also don't have a > > > much better suggestion apart from using the deprecation helper. > > I forgot to answer to this bit. I don't think the deprecation helper will do > all the jobs. There are still PV specific parameters: slack_memkb, features, > e820_host. Those can be left inside the PV sub-struct and shouldn't be marked as deprecated. > Those are not necessary for Arm, if you don't zero them then you will not > initialize the PVH structure with default values. How do you suggest to > handle them? But I guess ARM doesn't use any of those fields (or else they should be moved to the pvh sub-struct anyway)? In which case allowing the user to set them in the first place seems like an error to me. Roger.
On Mon, Sep 03, 2018 at 12:11:27PM +0100, Julien Grall wrote: > I don't think so. The top makefile does not pass -DCONFIG_ARM on compiler > command line. This is only done in sub-directory such as console and > libacpi. Indeed. That how console code did it. > > > > > There are several CONFIG_ARM* in toolstack code, though not in libxl. In > > any case, I think the code is fine as-is. > > I am happy to introduce CONFIG_ARM/CONFIG_X86 if you don't mind adding > -DCONFIG_ARM on the command line. I'm not too fussed. Keeping your code as-is is fine. Wei. > > Cheers, > > -- > Julien Grall
Hi Roger, Sorry for the late reply. On 09/03/2018 03:40 PM, Roger Pau Monné wrote: > On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: >> On 03/09/18 12:09, Julien Grall wrote: >>> >>> >>> On 23/08/18 08:58, Roger Pau Monné wrote: >>>> On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: >>>>>> >>>>>>> + >>>>>>> + b_info->type = LIBXL_DOMAIN_TYPE_PVH; >>>>>>> + >>>>>>> + /* >>>>>>> + * They only fie ld 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? >>>> >>>> Oh right, libxl__arch_domain_build_info_setdefault is called by >>>> libxl__domain_build_info_setdefault. >>>> >>>>> What I wanted to do here is resetting the union to 0 so you >>>>> don't get data >>>>> mangled by the pv fields. >>>> >>>> Another possible option I think would be to mark those fields as >>>> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated >>>> will take care of copying them to the new place. In fact I think all >>>> guest types should be using the top level kernel, ramdisk and cmdline >>>> fields. >>> >>> I will have a look at it. >>> >>>> >>>> I'm not specially comfortable with changing the guest type in the >>>> middle of libxl__domain_build_info_setdefault, but I also don't have a >>>> much better suggestion apart from using the deprecation helper. >> >> I forgot to answer to this bit. I don't think the deprecation helper will do >> all the jobs. There are still PV specific parameters: slack_memkb, features, >> e820_host. > > Those can be left inside the PV sub-struct and shouldn't be marked as > deprecated. > >> Those are not necessary for Arm, if you don't zero them then you will not >> initialize the PVH structure with default values. How do you suggest to >> handle them? > > But I guess ARM doesn't use any of those fields (or else they should > be moved to the pvh sub-struct anyway)? Those fields should not be used by Arm. Looking at the current fields in pv, they all should be zeroed. However, I am not sure if we can assume that will always be the case. If we can assume it, then I think it would just be sufficient to have b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm. Any thoughts? Cheers, > > In which case allowing the user to set them in the first place seems > like an error to me. Agreed. Cheers,
On Tue, Sep 25, 2018 at 08:36:36PM +0100, Julien Grall wrote: > Hi Roger, > > Sorry for the late reply. > > On 09/03/2018 03:40 PM, Roger Pau Monné wrote: > > On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: > > > On 03/09/18 12:09, Julien Grall wrote: > > > > > > > > > > > > On 23/08/18 08:58, Roger Pau Monné wrote: > > > > > On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: > > > > > > > > > > > > > > > + > > > > > > > > + b_info->type = LIBXL_DOMAIN_TYPE_PVH; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * They only fie ld 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? > > > > > > > > > > Oh right, libxl__arch_domain_build_info_setdefault is called by > > > > > libxl__domain_build_info_setdefault. > > > > > > > > > > > What I wanted to do here is resetting the union to 0 so you > > > > > > don't get data > > > > > > mangled by the pv fields. > > > > > > > > > > Another possible option I think would be to mark those fields as > > > > > deprecated in the IDL, and libxl__domain_build_info_copy_deprecated > > > > > will take care of copying them to the new place. In fact I think all > > > > > guest types should be using the top level kernel, ramdisk and cmdline > > > > > fields. > > > > > > > > I will have a look at it. > > > > > > > > > > > > > > I'm not specially comfortable with changing the guest type in the > > > > > middle of libxl__domain_build_info_setdefault, but I also don't have a > > > > > much better suggestion apart from using the deprecation helper. > > > > > > I forgot to answer to this bit. I don't think the deprecation helper will do > > > all the jobs. There are still PV specific parameters: slack_memkb, features, > > > e820_host. > > > > Those can be left inside the PV sub-struct and shouldn't be marked as > > deprecated. > > > > > Those are not necessary for Arm, if you don't zero them then you will not > > > initialize the PVH structure with default values. How do you suggest to > > > handle them? > > > > But I guess ARM doesn't use any of those fields (or else they should > > be moved to the pvh sub-struct anyway)? > > Those fields should not be used by Arm. Looking at the current fields in pv, > they all should be zeroed. > > However, I am not sure if we can assume that will always be the case. > If we can assume it, then I think it would just be sufficient to have > b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm. > > Any thoughts? If we go down this route for ARM I think you should add to xl.cfg that the `type` option only applies to x86, and that there's only one guest type on ARM. IMO it's best to not use type if there's only one type available, so that it can be intruded later on ARM if required. Thanks, Roger.
Hi, On 10/01/2018 04:27 PM, Roger Pau Monné wrote: > On Tue, Sep 25, 2018 at 08:36:36PM +0100, Julien Grall wrote: >> Hi Roger, >> >> Sorry for the late reply. >> >> On 09/03/2018 03:40 PM, Roger Pau Monné wrote: >>> On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: >>>> On 03/09/18 12:09, Julien Grall wrote: >>>>> >>>>> >>>>> On 23/08/18 08:58, Roger Pau Monné wrote: >>>>>> On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: >>>>>>>> >>>>>>>>> + >>>>>>>>> + b_info->type = LIBXL_DOMAIN_TYPE_PVH; >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * They only fie ld 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? >>>>>> >>>>>> Oh right, libxl__arch_domain_build_info_setdefault is called by >>>>>> libxl__domain_build_info_setdefault. >>>>>> >>>>>>> What I wanted to do here is resetting the union to 0 so you >>>>>>> don't get data >>>>>>> mangled by the pv fields. >>>>>> >>>>>> Another possible option I think would be to mark those fields as >>>>>> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated >>>>>> will take care of copying them to the new place. In fact I think all >>>>>> guest types should be using the top level kernel, ramdisk and cmdline >>>>>> fields. >>>>> >>>>> I will have a look at it. >>>>> >>>>>> >>>>>> I'm not specially comfortable with changing the guest type in the >>>>>> middle of libxl__domain_build_info_setdefault, but I also don't have a >>>>>> much better suggestion apart from using the deprecation helper. >>>> >>>> I forgot to answer to this bit. I don't think the deprecation helper will do >>>> all the jobs. There are still PV specific parameters: slack_memkb, features, >>>> e820_host. >>> >>> Those can be left inside the PV sub-struct and shouldn't be marked as >>> deprecated. >>> >>>> Those are not necessary for Arm, if you don't zero them then you will not >>>> initialize the PVH structure with default values. How do you suggest to >>>> handle them? >>> >>> But I guess ARM doesn't use any of those fields (or else they should >>> be moved to the pvh sub-struct anyway)? >> >> Those fields should not be used by Arm. Looking at the current fields in pv, >> they all should be zeroed. >> >> However, I am not sure if we can assume that will always be the case. >> If we can assume it, then I think it would just be sufficient to have >> b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm. >> >> Any thoughts? > > If we go down this route for ARM I think you should add to xl.cfg that > the `type` option only applies to x86, and that there's only one guest > type on ARM. IMO it's best to not use type if there's only one type > available, so that it can be intruded later on ARM if required. That's not what we discussed before. The idea is to default Arm guest to PVH for xl. But "type=" would still be available. We still have to support other type as other toolstack (e.g libvirt) may use PV for Arm. So the idea here is libxl will check if it was PV and convert to PVH. In an earlier reply, you said that we can use deprecated to copy most of the options over. However, can we always assume the pv.* fields will always be zero after the value was copied over? If not, then we would have to memset them. I asked opinion on the last bits. Cheers,
On Mon, Oct 01, 2018 at 04:33:45PM +0100, Julien Grall wrote: > Hi, > > On 10/01/2018 04:27 PM, Roger Pau Monné wrote: > > On Tue, Sep 25, 2018 at 08:36:36PM +0100, Julien Grall wrote: > > > Hi Roger, > > > > > > Sorry for the late reply. > > > > > > On 09/03/2018 03:40 PM, Roger Pau Monné wrote: > > > > On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: > > > > > On 03/09/18 12:09, Julien Grall wrote: > > > > > > > > > > > > > > > > > > On 23/08/18 08:58, Roger Pau Monné wrote: > > > > > > > On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > + b_info->type = LIBXL_DOMAIN_TYPE_PVH; > > > > > > > > > > + > > > > > > > > > > + /* > > > > > > > > > > + * They only fie ld 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? > > > > > > > > > > > > > > Oh right, libxl__arch_domain_build_info_setdefault is called by > > > > > > > libxl__domain_build_info_setdefault. > > > > > > > > > > > > > > > What I wanted to do here is resetting the union to 0 so you > > > > > > > > don't get data > > > > > > > > mangled by the pv fields. > > > > > > > > > > > > > > Another possible option I think would be to mark those fields as > > > > > > > deprecated in the IDL, and libxl__domain_build_info_copy_deprecated > > > > > > > will take care of copying them to the new place. In fact I think all > > > > > > > guest types should be using the top level kernel, ramdisk and cmdline > > > > > > > fields. > > > > > > > > > > > > I will have a look at it. > > > > > > > > > > > > > > > > > > > > I'm not specially comfortable with changing the guest type in the > > > > > > > middle of libxl__domain_build_info_setdefault, but I also don't have a > > > > > > > much better suggestion apart from using the deprecation helper. > > > > > > > > > > I forgot to answer to this bit. I don't think the deprecation helper will do > > > > > all the jobs. There are still PV specific parameters: slack_memkb, features, > > > > > e820_host. > > > > > > > > Those can be left inside the PV sub-struct and shouldn't be marked as > > > > deprecated. > > > > > > > > > Those are not necessary for Arm, if you don't zero them then you will not > > > > > initialize the PVH structure with default values. How do you suggest to > > > > > handle them? > > > > > > > > But I guess ARM doesn't use any of those fields (or else they should > > > > be moved to the pvh sub-struct anyway)? > > > > > > Those fields should not be used by Arm. Looking at the current fields in pv, > > > they all should be zeroed. > > > > > > However, I am not sure if we can assume that will always be the case. > > > If we can assume it, then I think it would just be sufficient to have > > > b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm. > > > > > > Any thoughts? > > > > If we go down this route for ARM I think you should add to xl.cfg that > > the `type` option only applies to x86, and that there's only one guest > > type on ARM. IMO it's best to not use type if there's only one type > > available, so that it can be intruded later on ARM if required. > > That's not what we discussed before. The idea is to default Arm guest to PVH > for xl. But "type=" would still be available. > > We still have to support other type as other toolstack (e.g libvirt) may use > PV for Arm. So the idea here is libxl will check if it was PV and convert to > PVH. > > In an earlier reply, you said that we can use deprecated to copy most of the > options over. However, can we always assume the pv.* fields will always be > zero after the value was copied over? If not, then we would have to memset > them. Yes, the field is cleared/disposed after the value is copied over, see libxl_C_type_copy_deprecated in gentypes.py. Roger.
Hi Roger, On 10/01/2018 05:37 PM, Roger Pau Monné wrote: > On Mon, Oct 01, 2018 at 04:33:45PM +0100, Julien Grall wrote: >> Hi, >> >> On 10/01/2018 04:27 PM, Roger Pau Monné wrote: >>> On Tue, Sep 25, 2018 at 08:36:36PM +0100, Julien Grall wrote: >>>> Hi Roger, >>>> >>>> Sorry for the late reply. >>>> >>>> On 09/03/2018 03:40 PM, Roger Pau Monné wrote: >>>>> On Mon, Sep 03, 2018 at 12:15:16PM +0100, Julien Grall wrote: >>>>>> On 03/09/18 12:09, Julien Grall wrote: >>>>>>> >>>>>>> >>>>>>> On 23/08/18 08:58, Roger Pau Monné wrote: >>>>>>>> On Wed, Aug 22, 2018 at 06:48:05PM +0100, Julien Grall wrote: >>>>>>>>>> >>>>>>>>>>> + >>>>>>>>>>> + b_info->type = LIBXL_DOMAIN_TYPE_PVH; >>>>>>>>>>> + >>>>>>>>>>> + /* >>>>>>>>>>> + * They only fie ld 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? >>>>>>>> >>>>>>>> Oh right, libxl__arch_domain_build_info_setdefault is called by >>>>>>>> libxl__domain_build_info_setdefault. >>>>>>>> >>>>>>>>> What I wanted to do here is resetting the union to 0 so you >>>>>>>>> don't get data >>>>>>>>> mangled by the pv fields. >>>>>>>> >>>>>>>> Another possible option I think would be to mark those fields as >>>>>>>> deprecated in the IDL, and libxl__domain_build_info_copy_deprecated >>>>>>>> will take care of copying them to the new place. In fact I think all >>>>>>>> guest types should be using the top level kernel, ramdisk and cmdline >>>>>>>> fields. >>>>>>> >>>>>>> I will have a look at it. >>>>>>> >>>>>>>> >>>>>>>> I'm not specially comfortable with changing the guest type in the >>>>>>>> middle of libxl__domain_build_info_setdefault, but I also don't have a >>>>>>>> much better suggestion apart from using the deprecation helper. >>>>>> >>>>>> I forgot to answer to this bit. I don't think the deprecation helper will do >>>>>> all the jobs. There are still PV specific parameters: slack_memkb, features, >>>>>> e820_host. >>>>> >>>>> Those can be left inside the PV sub-struct and shouldn't be marked as >>>>> deprecated. >>>>> >>>>>> Those are not necessary for Arm, if you don't zero them then you will not >>>>>> initialize the PVH structure with default values. How do you suggest to >>>>>> handle them? >>>>> >>>>> But I guess ARM doesn't use any of those fields (or else they should >>>>> be moved to the pvh sub-struct anyway)? >>>> >>>> Those fields should not be used by Arm. Looking at the current fields in pv, >>>> they all should be zeroed. >>>> >>>> However, I am not sure if we can assume that will always be the case. >>>> If we can assume it, then I think it would just be sufficient to have >>>> b_info->type = LIBXL_DOMAIN_TYPE_PVH in Arm. >>>> >>>> Any thoughts? >>> >>> If we go down this route for ARM I think you should add to xl.cfg that >>> the `type` option only applies to x86, and that there's only one guest >>> type on ARM. IMO it's best to not use type if there's only one type >>> available, so that it can be intruded later on ARM if required. >> >> That's not what we discussed before. The idea is to default Arm guest to PVH >> for xl. But "type=" would still be available. >> >> We still have to support other type as other toolstack (e.g libvirt) may use >> PV for Arm. So the idea here is libxl will check if it was PV and convert to >> PVH. >> >> In an earlier reply, you said that we can use deprecated to copy most of the >> options over. However, can we always assume the pv.* fields will always be >> zero after the value was copied over? If not, then we would have to memset >> them. > > Yes, the field is cleared/disposed after the value is copied over, see > libxl_C_type_copy_deprecated in gentypes.py. That's answer part of my question :). AFAIU, nothing promise that a default value will always be 0 (see libxl_devid). I am concerned that someone may decide to add such field in either PV or PVH in the future. Anyway, I think I find a way to address that, I will resend a new version later today. Cheers,
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. =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; + } /* 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."); + + 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)); } /* 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__) + c_info->type = LIBXL_DOMAIN_TYPE_PVH; +#else c_info->type = LIBXL_DOMAIN_TYPE_PV; +#endif xlu_cfg_get_defbool(config, "hap", &c_info->hap, 0);
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@arm.com> --- 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(-)