Re: [Xen-devel] [PATCH RFC v1 00/74] Run PV guest in PVH container

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[Xen-devel] [PATCH RFC v1 00/74] Run PV guest in PVH 
container"):
> 1. ARM build and some Clang build are broken by this series.
> 2. The host will see a lot over-allocation messages, nothing too harmful and
>    will be fixed once toolstack is ready.

My revised toolstack part of this, which I passed to Wei on Friday,
uses a different approach.  The caller must specify type=pvh
and some additional pvshim options (minimally, they must set a new
defbool to true).

Since we are intending to backport something like this, I thought I
would write down my API/ABI compatibility analysis, and our
discussions as I remember them.

I'm talking here about the libxl ABI/API; xl's config file handling is
easier and I won't analyse it here.


Xen 4.10
========

Backport strategy:
------------------

The ABI/API changes at the libxl layer are a few new fields in
b_info.u.pvh.  That substructure is empty in earlier 4.10.  It's part
of a union, so the overall struct size and layout does not increase.

Old callers with new libxl on 4.10:
-----------------------------------

If the caller is creating a guest other than a PVH one there is no
change to the ABI.

If the caller that is creating a PVH guest uses the proper
libxl_*init* functions those new fields will be initialised to the
default values.  The default is to disable the new mode (this is right
because we don't want to turn existing PVH guests into PV-in-PVH
ones).  With the patch as I passed to Wei, the extra string
parameters are initialised unconditionally.  This is not desirable
in the backport.

 => code change: the backport should set the default non-NULL
    values only if the pvhshim boolean is true after defaulting

When the caller disposes a domain config, the libxl dispose function
will free any of these fields.

If the caller did not call libxl_*init*, nor initialise the struct
with memset, but _does_ call libxl_*dispose*, the dispose will read
uninitialised memory and crash.  However, that is not a supported
approach and such a caller would have to manually initialise all the
myriad fields instead of calling *init* so it seems unlikely.

If the caller is examining existing guests: The returned struct will
have additional information, for any PVH guests (including PV-in-PVH
ones) which will be ignored by the caller.  The caller may
misunderstand and misreport the guest type.  The additional
information may come from the heap, but *dispose* will free it, so
there should be no leak.

Overall: this is safe, with the code change I propose.

New callers with old libxl on 4.10:
-----------------------------------

If the caller is creating a guest other than a PVH one there is no
change to the ABI.

When a caller creates a PVH non-shim guest, it will probably not set
any of these fields.  Things will work properly.

If a caller tries to create a shim guest, the attempt to do so will be
ignored and the guest will be created as PV.  Probably, the guest will
not boot.  Additionally, if the caller filled in pvshim cmdline or
path information, it will probably expect *dispose* to free those
values - and the result will be a memory leak.

If the caller is examining existing guests, PV and HVM guests will
work fine.  If the caller is examining existing PVH guests, the
library will not initialise the new fields.  The result may include an
uninitialised read by the caller.

Overall: this is not safe and should be prevented.

 => code change: the 4.10 backport should use symbol versioning or
    another technique to prevent expecting callers whose source code
    understands *shim* guests from using libxl versions which don't.



Xen 4.9 and 4.8
===============

Backport strategy:
------------------

These have some kind of PVH toolstack support but only half-baked and
specified with "device_model_version=none" which breaks some PV device
availability etc.

We have considered three backport strategies:

 (i) Make the shim run in HVM mode and use that.  My understanding
     from experts in the relevant area is that this is quite awkward:
     a lot of work, and risks producing a program which does not work
     well.

 (ii) Have libxl callers specify the new mode as a variant on the PV
     guest type.  Inside libxl this is a real pain because it involves
     touching every place the domain type is considered, to decide
     whether this new mode should be like PV or like HVM.  This is
     essentially redoing the work done for PVH in 4.10, but afresh and
     therefore with new bugs.

 (iii) Backport the first-class PVH guest type patches to libxl from
     4.10.  This is a large backport but these patches have been
     around for a while and we regard them as stable.  Doing this
     means we provide users of 4.8 and 4.9 with the same bugs as are
     in 4.10 (which we think are fairly few) rather than doing a lot
     of work to write an essentially equivalent amount of new code
     with a lot of new bugs.

From the above it should be evident that we think strategy (iii) is
the best answer.  Howwever, there are some wrinkles.

Some of the libxl pvh patches move various parameters for specifying
guest kernels from the pv-specific part of the domain config struct to
the "build info", ie from d_config.b_info.u.pv to d_config.b_info.

We cannot do that in 4.8 and 4.9 because doing so would increase the
size of d_config.b_info.  b_info is not the last field in d_config, so
adding fields to it would move other fields in b_info which is total
ABI break.

Accordingly, we will have to provide copies of the kernel related
parameters in the pvh struct.

Updated callers that want to create shim guests (or, indeed PVH
guests) will have to specify the kernel in the new fields.

We should provide a macro to access the "right" field for each of
the kernel fields.  We should provide that macro in 4.10 and unstable
too, so that the API is forward-compatible.

 => code change: define the macro just discussed

Old callers with new libxl on <=4.9:
------------------------------------

If the caller is creating a guest there is no change to the ABI.

If the caller is examining existing guests: If any PVH guests have
been created already, the caller will receive a domain type that it is
not expecting.  It may crash or produce an error, but undefined
behaviour seems unlikely.  And this can only happen when old
higher-level programs are operating on a host that already has new
guests.  Furthermore, if the caller properly handles the unexpected
guest type, and calls *dispose*, there is even no memory leak.

New callers with old libxl on <=4.9:
------------------------------------

If the caller is creating a guest other than a PVH one there is no
change to the ABI.

If the caller is creating a PVH guest, it will call *init* but that
call will not initialise the PVH type.  (*init* does not return
errors.)  The caller may attempt boolean setting, string replacement,
and so on, on the undefined data.

 => code change: the <=4.9 backport should use symbol versioning or
    another technique to prevent callers whose source code understands
    *PVH* guests from using libxl versions which don't.

If the caller is examining guests, for non-PVH guests there is no
problem.  For PVH guests, libxl will return wrong information but
there will be no serious problem.


Xen 4.7 and earlier:
====================

These lack appropriate hypervisor support for PVHv2 guests.
It may be possible to make the shim run in HVM - but see above.


Ian.

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