Re: [Xen-devel] Xen 4.1 + Linux compiled with PVH == BOOM

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?)

Roger.

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