Re: Xen 4.1 + Linux compiled with PVH == BOOM

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 03, 2014 at 01:40:20PM +0100, Roger Pau Monné wrote:
> On 02/01/14 22:23, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 02, 2014 at 03:30:52PM -0400, Konrad Rzeszutek Wilk wrote:
> >> On Mon, Dec 30, 2013 at 02:56:48PM -0500, Konrad Rzeszutek Wilk wrote:
> >>> On Tue, Dec 24, 2013 at 01:05:10PM +0000, David Vrabel wrote:
> >>>> On 24/12/2013 12:55, Andrew Cooper wrote:
> >>>>> On 24/12/2013 12:31, David Vrabel wrote:
> >>>>>> On 20/12/2013 17:57, Konrad Rzeszutek Wilk wrote:
> >>>>>>> Hey,
> >>>>>>>
> >>>>>>> This is with Linux and
> >>>>>>>
> >>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git stable/pvh.v11
> >>>>>>>
> >>>>>>> I get Xen 4.1 (only) hypervisor to blow up with a Linux kernel that has been
> >>>>>>> compiled with PVH.
> >>>>>>>
> >>>>>>> I think the same problem would show up if I tried to launch a PV guest 
> >>>>>>> compiled as PVH under Xen 4.1 as well - as the ELF parsing code is shared
> >>>>>>> with the toolstack.
> >>>>>> If a kernel with both PVH and PV support enabled cannot boot in PV mode
> >>>>>> with a non-PVH aware hypervisor/toolstack then the kernel is broken.
> >>>>>>
> >>>>>> Hypervisor/tool-side fixes aren't the correct fix here.  Xen 4.1 and
> >>>>>> even older are still widely deployed.
> >>>>>>
> >>>>>> David
> >>>>>
> >>>>> I believe that the problem is because the elf parsing code is not
> >>>>> sufficiently forward-compatible aware, and rejects the PVH kernel
> >>>>> because it has an unrecognised Xen elf note field.  This is not a kernel
> >>>>> bug.
> >>>
> >>> It (Xen 4.1) has the logic to ignore unrecognized Xen elf note fields. But
> >>> it (all Xen versions) do not have the logic to ignore in the "SUPPORTED_FEATURES"
> >>> an unrecognized string.
> >>>
> >>>>>
> >>>>> The elf parsing should accept unrecognised fields for forward
> >>>>> compatibility, which would then allow a PV & PVH compiled kernel to run
> >>>>> in PV mode.
> >>>>
> >>>> It should but it doesn't, so a different way needs to be found for the
> >>>> kernel to report (optional) PVH support.  A method that is compatible
> >>>> with older toolstacks.
> >>>
> >>> Also known as changes to the PVH ABI.
> >>>
> >>> Mukesh, Roger, George (emailing Ian instead since he is now the Release Manager-pro-temp), Jan,
> >>>
> >>> a).  That means dropping the 'hvm_callback_vector' check from xc_dom_core.c and
> >>> just depending on: "writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel"
> >>> for PVH guests.
> >>>
> >>> b) Or dropping that altogether and introducing a new Xen elf note field, say:
> >>>
> >>> XEN_ELFNOTE_PVH_VERSION
> >>>
> >>
> >> c).
> >>
> >> Use the 'XEN_ELFNOTE_SUPPORTED_FEATURES' which says:
> >>  *
> >>  * Other than XEN_ELFNOTE_FEATURES on pre-4.2 Xen, this note allows a
> >>  * kernel to specify support for features that older hypervisors don't
> >>  * know about. The set of features 4.2 and newer hypervisors will
> >>  * consider supported by the kernel is the combination of the sets
> >>  * specified through this and the string note.
> >>
> >> for hvm_callback_vector parameter.
> >>
> >>>
> >>> Which way should we do this?
> >>
> >> The c) way looks the best. Ian, would you be OK with that idea for 4.4?
> > 
> > Seems that not only does it work without any changes in Xen 4.4 but it
> > is all in the Linux kernel, and it allows us to boot an Linux kernel
> > with PV and PVH support
> > 
> > Seems that not only does it work without any changes in Xen 4.4 but it
> > is all in the Linux kernel:
> > 
> > 
> > 
> > diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> > index 56f42c0..2ce56bf 100644
> > --- a/arch/x86/xen/xen-head.S
> > +++ b/arch/x86/xen/xen-head.S
> > @@ -11,12 +11,22 @@
> >  #include <asm/page_types.h>
> >  
> >  #include <xen/interface/elfnote.h>
> > +#include <xen/interface/features.h>
> >  #include <asm/xen/interface.h>
> >  
> >  #ifdef CONFIG_XEN_PVH
> > -#define PVH_FEATURES_STR  "|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector"
> > +#define PVH_FEATURES_STR  "|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel"
> > +/* Note the lack of 'hvm_callback_vector'. Older hypervisor will
> > + * balk at this being part of XEN_ELFNOTE_FEATURES, so we put it in
> > + * XEN_ELFNOTE_SUPPORTED_FEATURES which older hypervisors will ignore.
> > + */
> > +#define PVH_FEATURES ((1 << XENFEAT_writable_page_tables) | \
> > +		      (1 << XENFEAT_auto_translated_physmap) | \
> > +		      (1 << XENFEAT_supervisor_mode_kernel) | \
> > +		      (1 << XENFEAT_hvm_callback_vector))
> >  #else
> >  #define PVH_FEATURES_STR  ""
> > +#define PVH_FEATURES (0)
> >  #endif
> >  
> >  	__INIT
> > @@ -102,6 +112,9 @@ NEXT_HYPERCALL(arch_6)
> >  	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
> >  	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
> >  	ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii "!writable_page_tables|pae_pgdir_above_4gb"; .asciz PVH_FEATURES_STR)
> > +	ELFNOTE(Xen, XEN_ELFNOTE_SUPPORTED_FEATURES, .long (PVH_FEATURES) |
> > +						(1 << XENFEAT_writable_page_tables) |
> 
> PVH_FEATURES already contains XENFEAT_writable_page_tables, shouldn't
> you remove it from PVH_FEATURES if you are adding it unconditionally
> here? (or is this just to make clear that you need
> XENFEAT_writable_page_tables for both PVH and PV?)

Yup, that was the only reason for it. I will flesh out the comment
to make that clear. Thanks!
> 
> Roger.