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

[Roger Pau Monne]

On Tue, Mar 14, 2017 at 04:29:22PM +, Wei Liu wrote:
> On Tue, Mar 14, 2017 at 10:27:25AM +, Wei Liu wrote:
> > On Mon, Mar 13, 2017 at 04:50:12PM +, Wei Liu wrote:
> > > On Fri, Mar 03, 2017 at 12:25:06PM +, 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é 
> > > > Reviewed-by: Andrew Cooper 
> > > > Acked-by: George Dunlap 
> > > > Reviewed-by: Paul Durrant 
> > > > Acked-by: Elena Ufimtseva 
> > > > Reviewed-by: Kevin Tian 
> > > > ---
> > > > 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 
> > > > Cc: Wei Liu 
> > > > Cc: Elena Ufimtseva 
> > > > Cc: Jan Beulich 
> > > > Cc: Andrew Cooper 
> > > > Cc: Paul Durrant 
> > > > Cc: Jun Nakajima 
> > > > Cc: Kevin Tian 
> > > > Cc: George Dunlap 
> > > > Cc: Razvan Cojocaru 
> > > > Cc: Tamas K Lengyel 
> > > > ---
> > > >  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 

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

[Wei Liu]

On Tue, Mar 14, 2017 at 10:27:25AM +, Wei Liu wrote:
> On Mon, Mar 13, 2017 at 04:50:12PM +, Wei Liu wrote:
> > On Fri, Mar 03, 2017 at 12:25:06PM +, 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é 
> > > Reviewed-by: Andrew Cooper 
> > > Acked-by: George Dunlap 
> > > Reviewed-by: Paul Durrant 
> > > Acked-by: Elena Ufimtseva 
> > > Reviewed-by: Kevin Tian 
> > > ---
> > > 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 
> > > Cc: Wei Liu 
> > > Cc: Elena Ufimtseva 
> > > Cc: Jan Beulich 
> > > Cc: Andrew Cooper 
> > > Cc: Paul Durrant 
> > > Cc: Jun Nakajima 
> > > Cc: Kevin Tian 
> > > Cc: George Dunlap 
> > > Cc: Razvan Cojocaru 
> > > Cc: Tamas K Lengyel 
> > > ---
> > >  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 

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

[Wei Liu]

On Mon, Mar 13, 2017 at 04:50:12PM +, Wei Liu wrote:
> On Fri, Mar 03, 2017 at 12:25:06PM +, 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é 
> > Reviewed-by: Andrew Cooper 
> > Acked-by: George Dunlap 
> > Reviewed-by: Paul Durrant 
> > Acked-by: Elena Ufimtseva 
> > Reviewed-by: Kevin Tian 
> > ---
> > 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 
> > Cc: Wei Liu 
> > Cc: Elena Ufimtseva 
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> > Cc: Paul Durrant 
> > Cc: Jun Nakajima 
> > Cc: Kevin Tian 
> > Cc: George Dunlap 
> > Cc: Razvan Cojocaru 
> > Cc: Tamas K Lengyel 
> > ---
> >  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Wei Liu]

On Fri, Mar 03, 2017 at 12:25:06PM +, 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é 
> Reviewed-by: Andrew Cooper 
> Acked-by: George Dunlap 
> Reviewed-by: Paul Durrant 
> Acked-by: Elena Ufimtseva 
> Reviewed-by: Kevin Tian 
> ---
> 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 
> Cc: Wei Liu 
> Cc: Elena Ufimtseva 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Paul Durrant 
> Cc: Jun Nakajima 
> Cc: Kevin Tian 
> Cc: George Dunlap 
> Cc: Razvan Cojocaru 
> Cc: Tamas K Lengyel 
> ---
>  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.

Wei.

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

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

[Elena Ufimtseva]

On Fri, Mar 03, 2017 at 12:25:06PM +, 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é 
> Reviewed-by: Andrew Cooper 
> Acked-by: George Dunlap 
> Reviewed-by: Paul Durrant 
> Acked-by: Elena Ufimtseva 
> Reviewed-by: Kevin Tian 
> ---
> 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.
> 

For gdbsx bits:
Acked-by: Elena Ufimtseva 
> ---
> Cc: Ian Jackson 
> Cc: Wei Liu 
> Cc: Elena Ufimtseva 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Paul Durrant 
> Cc: Jun Nakajima 
> Cc: Kevin Tian 
> Cc: George Dunlap 
> Cc: Razvan Cojocaru 
> Cc: Tamas K Lengyel 
> ---
>  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 +-
>  xen/arch/x86/cpu/vpmu.c |   3 +-
>  xen/arch/x86/domain.c   |  42 +-
>  xen/arch/x86/domain_build.c | 287 
> +---
>  xen/arch/x86/domctl.c   |   7 +-
>  xen/arch/x86/hvm/hvm.c  |  81 +-
>  xen/arch/x86/hvm/hypercall.c|   4 +-
>  xen/arch/x86/hvm/io.c   |   2 -
>  xen/arch/x86/hvm/ioreq.c|   3 +-
>  xen/arch/x86/hvm/irq.c  |   3 -
>  xen/arch/x86/hvm/vmx/vmcs.c |  35 +
>  xen/arch/x86/hvm/vmx/vmx.c  |  12 +-
>  xen/arch/x86/mm.c   |   2 +-
>  xen/arch/x86/mm/p2m-pt.c|   2 +-
>  xen/arch/x86/mm/p2m.c   |   6 +-
>  xen/arch/x86/physdev.c  |   8 -
>  xen/arch/x86/setup.c|   7 -
>  xen/arch/x86/time.c |  27 
>  xen/common/domain.c |   2 -
>  xen/common/domctl.c |  10 --
>  xen/common/kernel.c |   5 -
>  xen/common/vm_event.c   |   8 +-
>  xen/include/asm-x86/domain.h|   1 -
>  xen/include/asm-x86/hvm/hvm.h   |   3 -
>  xen/include/public/domctl.h |  14 +-
>  xen/include/xen/sched.h |   9 +-
>  55 files changed, 252 insertions(+), 911 deletions(-)
>  delete mode 100644 docs/misc/pvh-readme.txt
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 505c111..8e4eb97 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1064,6 +1064,13 @@ FIFO-based event channel ABI support up to 131,071 
> event channels.
>  Other guests are limited to 4095 (64-bit x86 and ARM) or 1023 (32-bit
>  x86).
>  
> +=item