Re: [PATCH 16/18] arm/altp2m: Extended libxl to activate altp2m on ARM.

From: Wei Liu <wei.liu2@citrix.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Sergej Proskurin <proskurin@sec.in.tum.de>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel@lists.xen.org
Subject: Re: [PATCH 16/18] arm/altp2m: Extended libxl to activate altp2m on ARM.
Date: Mon, 25 Jul 2016 11:08:26 +0100	[thread overview]
Message-ID: <20160725100826.GI1008@citrix.com> (raw)
In-Reply-To: <50734ad7-cbdb-6614-0d68-b7ada5982335@arm.com>

On Mon, Jul 25, 2016 at 10:49:41AM +0100, Julien Grall wrote:
> Hello,
> 
> On 25/07/16 10:04, Sergej Proskurin wrote:
> >On 07/25/2016 10:32 AM, Wei Liu wrote:
> >>On Sun, Jul 24, 2016 at 06:06:00PM +0200, Sergej Proskurin wrote:
> >>>>>+        xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2M,
> >>>>>+                         libxl_defbool_val(info->u.pv.altp2m));
> >>>>>+}
> >>>>>+#endif
> >>>>>+
> >>>>> static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
> >>>>>                                 libxl_domain_build_info *const info)
> >>>>> {
> >>>>>@@ -433,6 +443,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >>>>>             return rc;
> >>>>> #endif
> >>>>>     }
> >>>>>+#if defined(__arm__) || defined(__aarch64__)
> >>>>>+    else /* info->type == LIBXL_DOMAIN_TYPE_PV */
> >>>>>+        pv_set_conf_params(ctx->xch, domid, info);
> >>>>>+#endif
> >>>>>
> >>>>>     rc = libxl__arch_domain_create(gc, d_config, domid);
> >>>>>
> >>>>>diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >>>>>index ef614be..0a164f9 100644
> >>>>>--- a/tools/libxl/libxl_types.idl
> >>>>>+++ b/tools/libxl/libxl_types.idl
> >>>>>@@ -554,6 +554,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>>>>                                       ("features", string, {'const': True}),
> >>>>>                                       # Use host's E820 for PCI passthrough.
> >>>>>                                       ("e820_host", libxl_defbool),
> >>>>>+                                      ("altp2m", libxl_defbool),
> >>>>Why is this placed in PV instead of arch_arm?
> >>>>
> >>>The current implementation mirrors the x86's altp2m, where the altp2m
> >>>field represents a property of HVM guests in b_info->u.hvm.altp2m. Since
> >>>guests on ARM are marked as PV, I have placed the field altp2m into
> >>>b_info->u.pv.altp2m.
> >>>
> >>>However, if you believe that it would make more sense to place altp2m
> >>>into b_info->arch_arm.altp2m, I will try to adapt the affected fields.
> >>>
> >>OK. I don't think that one should be placed in u.pv because that union
> >>is for x86. However it is also not suitable to just promote that to
> >>a common field because x86 pv doesn't have altp2m support.
> >>
> >>I think arch_arm would be a better location.
> >>
> >
> >Alright, I will move the field altp2m to arch_arm and adopt the
> >initialization routines.
> 
> Would not it make more sense to introduce a generic field 'altp2m' (i.e
> outside of hvm and arch_arm)?
> 
> Otherwise, toolstack such as libvirt will need to have specific code to
> handle ARM and x86.
> 

Hmm... this is a compelling reason.

After mulling over this issue a bit more, I think I'm fine with
promoting altp2m to a common field.

Sergej, this is a patch to get you started. Please make sure setting the
old field still works. If I'm not clear enough, please ask.


Wei.

---8<---

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ef614be..81d3ae5 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -494,6 +494,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     # Note that the partial device tree should avoid to use the phandle
     # 65000 which is reserved by the toolstack.
     ("device_tree",      string),
+    ("altp2m",           libxl_defbool),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
@@ -512,7 +513,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("mmio_hole_memkb",  MemKB),
                                        ("timer_mode",       libxl_timer_mode),
                                        ("nested_hvm",       libxl_defbool),
-                                       ("altp2m",           libxl_defbool),
+                                       ("altp2m",           libxl_defbool), # deprecated, please use common field
                                        ("smbios_firmware",  string),
                                        ("acpi_firmware",    string),
                                        ("hdtype",           libxl_hdtype),

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