Re: [Xen-devel] [PATCH v3 2/4] x86: remove PVHv1 code

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Mar 13, 2017 at 04:50:12PM +0000, Wei Liu wrote:
> On Fri, Mar 03, 2017 at 12:25:06PM +0000, Roger Pau Monne wrote:
> > This removal applies to both the hypervisor and the toolstack side of PVHv1.
> > 
> > Note that on the toolstack side a new PVH domain type is introduced to 
> > libxl.
> > The "none" device model version is removed, together with the "pvh" field in
> > the create info struct (the defines announcing those features are also 
> > removed
> > from libxl.h). The canonical way to create a PVH guest in libxl is to add
> > "pvh=1" to the guest config file.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> > Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Acked-by: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> > Changes since v1:
> >  - Remove dom0pvh option from the command line docs.
> >  - Bump domctl interface version due to the removed CDF flag.
> >  - Introduce LIBXL_DOMAIN_TYPE_PVH.
> >  - Remove "none" from the valid device model version options.
> >  - Update the xl.cfg(5) man page to reflect the changes.
> > 
> > ---
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> > Cc: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> > ---
> >  docs/man/xl.cfg.pod.5.in            |  16 +-
> >  docs/misc/pvh-readme.txt            |  63 --------
> >  docs/misc/xen-command-line.markdown |   7 -
> >  tools/debugger/gdbsx/xg/xg_main.c   |   4 +-
> >  tools/libxc/include/xc_dom.h        |   1 -
> >  tools/libxc/include/xenctrl.h       |   2 +-
> >  tools/libxc/xc_cpuid_x86.c          |  13 +-
> >  tools/libxc/xc_dom_core.c           |   9 --
> >  tools/libxc/xc_dom_x86.c            |  49 +++---
> >  tools/libxc/xc_domain.c             |   1 -
> >  tools/libxl/libxl.h                 |  22 +--
> >  tools/libxl/libxl_console.c         |   1 +
> >  tools/libxl/libxl_create.c          |  64 +++-----
> >  tools/libxl/libxl_disk.c            |  10 +-
> >  tools/libxl/libxl_dm.c              |   2 +
> >  tools/libxl/libxl_dom.c             |  86 ++++++-----
> >  tools/libxl/libxl_dom_save.c        |   7 +-
> >  tools/libxl/libxl_dom_suspend.c     |   4 +-
> >  tools/libxl/libxl_domain.c          |  18 +--
> >  tools/libxl/libxl_internal.h        |   1 -
> >  tools/libxl/libxl_mem.c             |   1 +
> >  tools/libxl/libxl_nic.c             |   7 +-
> >  tools/libxl/libxl_pci.c             |   9 +-
> >  tools/libxl/libxl_stream_read.c     |   8 +-
> >  tools/libxl/libxl_stream_write.c    |  14 +-
> >  tools/libxl/libxl_types.idl         | 115 ++++++++-------
> >  tools/libxl/libxl_usb.c             |   4 +-
> >  tools/libxl/libxl_x86.c             |  31 ++--
> >  tools/libxl/libxl_x86_acpi.c        |   3 +-
> >  tools/xl/xl_parse.c                 |   8 +-
> [...]
> > +                [("hvm", libxl_domain_build_info_hvm),
> > +                 ("pvh", libxl_domain_build_info_hvm),
> 
> This "pvh" field is never used in code - instead you fill in the hvm
> fields. It is problematic because people don't know how to use this.
> 
> Either we don't want this, or you would have to (tediously) fill this
> in. I think the latter is what Ian has asked for. Unfortunately C
> doesn't have good support for meta-programming.

On second thought, let's look at this from user's perspective. A user of
libxl would certainly use .u.pvh field to construct a pvh guest, so I
think we need the pvh struct.

Since .u.pvh and .u.hvm share the same layout and type-punning via union
is allowed even when strict-aliasing is enabled, this patch *might* just
work. (Can someone familiar with C standard check this?)

Ultimately I would like to avoid tricky code like this even it conforms
to C standard. Let me check if there is a way forward.

Wei.

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