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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Tue, Mar 14, 2017 at 04:29:22PM +0000, Wei Liu wrote:
> 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.

Right, at first I though that I should provide a suitable replacement so that
the "pvh=1" option is always working, but since this is experimental I don't
see a need for that. I will update this patch to just remove the PVHv1 stuff
(including "pvh=1"), and then I can implement that option for PVHv2.

> 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.

Right, I think this is correct.

Roger.

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