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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Mar 14, 2017 at 10:27:25AM +0000, Wei Liu wrote:
> 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.
> 

The more I think about this, the more I realise we're trying to do two
things at the same time: to remove PVHv1 code and repurpose toolstack
elements (xl pvh option, and b_info etc). This is the reason why this
series has been dragging on longer than necessary. And no doubt doing
the two at the same time will make this patch difficult to review.

Hence I propose to do one thing at a time. This patch should just remove
the PVHv1 implementation, and leave the adding PVHv2 toolstack support
to another patch. I have no problem with deleting pvh related stuff in
libxl and xl because we have no obligation to maintain backward
compatibility.

Roger and Ian, let me know what you think.

Wei.

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