[PATCH 0/2] tools/libxl: Switch Arm guest type to PVH

[PATCH 0/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

Hi all,

This small patch series adds switch Arm guest type to PVH in libxl. See
patch #2 for the rationale.

Cheers,

Julien Grall (2):
  tools/libxl: Rename libxl__arch_domain_build_info_acpi_setdefault
    to...
  tools/libxl: Switch Arm guest type to PVH

 docs/man/xl.cfg.pod.5.in   |  3 ++-
 tools/libxl/libxl_arch.h   |  4 ++--
 tools/libxl/libxl_arm.c    | 41 ++++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_create.c |  2 +-
 tools/libxl/libxl_x86.c    |  4 ++--
 tools/xl/xl_parse.c        |  4 ++++
 6 files changed, 49 insertions(+), 9 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/2] tools/libxl: Rename libxl__arch_domain_build_info_acpi_setdefault to...

[Julien Grall]

libxl__arch_domain_build_info_setdefault

A follow-up will require to modify default of multiple fields of
build_info. So rename the function accordingly.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 tools/libxl/libxl_arch.h   | 3 +--
 tools/libxl/libxl_arm.c    | 4 ++--
 tools/libxl/libxl_create.c | 2 +-
 tools/libxl/libxl_x86.c    | 3 +--
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index c8ccaaf14c..5ab0c95974 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -65,8 +65,7 @@ _hidden
 int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq);
 
 _hidden
-void libxl__arch_domain_build_info_acpi_setdefault(
-                                        libxl_domain_build_info *b_info);
+void libxl__arch_domain_build_info_setdefault(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 3ee37d2d12..18c41f4ee9 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1135,9 +1135,9 @@ 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_acpi_setdefault(
-                                        libxl_domain_build_info *b_info)
+void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info)
 {
+    /* ACPI is disabled by default */
     libxl_defbool_setdefault(&b_info->acpi, false);
 }
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b7b44e280b..d4fa06daea 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_acpi_setdefault(b_info);
+    libxl__arch_domain_build_info_setdefault(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 6f670b03b5..81523a568f 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -613,8 +613,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
     return rc;
 }
 
-void libxl__arch_domain_build_info_acpi_setdefault(
-                                        libxl_domain_build_info *b_info)
+void libxl__arch_domain_build_info_setdefault(libxl_domain_build_info *b_info)
 {
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] tools/libxl: Rename libxl__arch_domain_build_info_acpi_setdefault to...

[Roger Pau Monné]

On Wed, Aug 22, 2018 at 04:00:44PM +0100, Julien Grall wrote:
> libxl__arch_domain_build_info_setdefault
> 
> A follow-up will require to modify default of multiple fields of
> build_info. So rename the function accordingly.

No functional change.

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] tools/libxl: Rename libxl__arch_domain_build_info_acpi_setdefault to...

[Wei Liu]

On Wed, Aug 22, 2018 at 05:08:18PM +0200, Roger Pau Monné wrote:
> On Wed, Aug 22, 2018 at 04:00:44PM +0100, Julien Grall wrote:
> > libxl__arch_domain_build_info_setdefault
> > 
> > A follow-up will require to modify default of multiple fields of
> > build_info. So rename the function accordingly.
> 
> No functional change.
> 
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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.
 
 =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);
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Wei Liu]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Wei Liu]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

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

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] tools/libxl: Switch Arm guest type to PVH

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel