[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi,

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.