All of lore.kernel.org
 help / color / mirror / Atom feed
* Xen 4.1 + Linux compiled with PVH == BOOM
@ 2013-12-20 17:57 Konrad Rzeszutek Wilk
  2013-12-21  1:47 ` Mukesh Rathor
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-20 17:57 UTC (permalink / raw)
  To: xen-devel, Mukesh Rathor, JBeulich

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.

The issue is that Xen 4.1 is missing git
commit 30832c06a8d1f9caff0987654ef9e24d59469d9a
Author: Mukesh Rathor <mukesh.rathor@oracle.com>
Date:   Tue Sep 10 16:38:43 2013 +0200

    libelf: add hvm callback vector feature
    
    Add XENFEAT_hvm_callback_vector to elf_xen_feature_names so we can
    ensure the kernel supports all features required for PVH mode when
    building a PVH domU here. Note, hvm callback is required for PVH.
    
    Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

where the message is a bit misleading. It also allows the hypervisor
to boot a PVH guest in PV mode.

Here is what the Xen serial log shows (with extra debugging sprinkled):

(XEN) elf_xen_parse_note: GUEST_OS = "linux"
(XEN) elf_xen_parse_note: GUEST_VERSION = "2.6"
(XEN) elf_xen_parse_note: XEN_VERSION = "xen-3.0"
(XEN) elf_xen_parse_note: VIRT_BASE = 0xffffffff80000000
(XEN) elf_xen_parse_note: ENTRY = 0xffffffff81cd51e0
(XEN) elf_xen_parse_note: HYPERCALL_PAGE = 0xffffffff81001000
(XEN) elf_xen_parse_note: FEATURES = "!writable_page_tables|pae_pgdir_above_4gb|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector"
(XEN) elf_xen_parse_features:70 r[writable_page_tables]
(XEN) elf_xen_parse_features:81 s[pae_pgdir_above_4gb]
(XEN) elf_xen_parse_features:81 s[writable_descriptor_tables]
(XEN) elf_xen_parse_features:81 s[auto_translated_physmap]
(XEN) elf_xen_parse_features:81 s[supervisor_mode_kernel]
(XEN) elf_xen_parse_features:88 5
(XEN) elf_xen_parse_note:206
(XEN) elf_xen_parse_notes:245 on Xen


We end up bailing out:

 87         if ( i == elf_xen_features ) {
 88             elf_msg(elf,"%s:%d %d\n", __func__, __LINE__, i);
 89             return -1;
 90         }

because the elf_xen_features (or rather elf_xen_feature_names)
does not have the "hvm_callback_vector parameter" and it can't parse
it - so it returns -1.

And that means:
203     case XEN_ELFNOTE_FEATURES:
204         if ( elf_xen_parse_features(elf, str, parms->f_supported,
205                                     parms->f_required) ) {
206             elf_msg(elf, "%s:%d\n", __func__, __LINE__);
207             return -1;
208         }

Which means we return at:
244         if ( elf_xen_parse_note(elf, parms, note) ) {
245             elf_msg(elf, "%s:%d on %s\n", __func__, __LINE__, note_name);
246             return ELF_NOTE_INVALID;
247         }

and never finish the construct_dom0.

Thought on how to fix it?

My thought was that we should return 0, that is in line with
some of the other code that deals with elf, such as elf_xen_parse_note
which returns 0:

130     if ( (type >= sizeof(note_desc) / sizeof(note_desc[0])) ||
131          (note_desc[type].name == NULL) )
132     {
133         elf_msg(elf, "%s: unknown xen elf note (0x%x)\n",
134                 __FUNCTION__, type);
135         return 0;
136     }


Like this:

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index fda19e7..c9ff61e 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -83,7 +83,9 @@ elf_errorstatus elf_xen_parse_features(const char *features,
             }
         }
         if ( i == elf_xen_features )
-            return -1;
+            return 0; /* We don't recognize this feature, and let the
+                       * caller figure out if it has all of the needed parts.
+                       */
     }
 
     return 0;



But perhaps that is not the way to do it and we should just cherry-pick
30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?

cThoughts?

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-20 17:57 Xen 4.1 + Linux compiled with PVH == BOOM Konrad Rzeszutek Wilk
@ 2013-12-21  1:47 ` Mukesh Rathor
  2013-12-21 11:09 ` Ian Campbell
  2013-12-24 12:31 ` David Vrabel
  2 siblings, 0 replies; 21+ messages in thread
From: Mukesh Rathor @ 2013-12-21  1:47 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: xen-devel, JBeulich

On Fri, 20 Dec 2013 12:57:35 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> Hey,
> 
> This is with Linux and
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git
.......

> But perhaps that is not the way to do it and we should just
> cherry-pick 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
> 
> cThoughts?

IMO, that seems to be the easiest and safest solution. 

thanks,
Mukesh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-20 17:57 Xen 4.1 + Linux compiled with PVH == BOOM Konrad Rzeszutek Wilk
  2013-12-21  1:47 ` Mukesh Rathor
@ 2013-12-21 11:09 ` Ian Campbell
  2013-12-21 16:17   ` Konrad Rzeszutek Wilk
  2013-12-23  9:37   ` Jan Beulich
  2013-12-24 12:31 ` David Vrabel
  2 siblings, 2 replies; 21+ messages in thread
From: Ian Campbell @ 2013-12-21 11:09 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: xen-devel, JBeulich

On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:

> My thought was that we should return 0[...]
> Like this:
> 
> diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
> index fda19e7..c9ff61e 100644
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -83,7 +83,9 @@ elf_errorstatus elf_xen_parse_features(const char *features,
>              }
>          }
>          if ( i == elf_xen_features )
> -            return -1;
> +            return 0; /* We don't recognize this feature, and let the
> +                       * caller figure out if it has all of the needed parts.
> +                       */

WRT the comment, if this code doesn't know about the feature then the
caller is unlikely to know about it either, so silently ignoring it is
more than likely completely fine.

[...]
> But perhaps that is not the way to do it and we should just cherry-pick
> 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?

I think we should do both, i.e. backport 30832c06a8d1 now to solve the
immediate problem and then look at fixing unstable to be more accepting
of new features which it doesn't yet know about. Ultimately perhaps we'd
want to backport that patch too.

Ian.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-21 11:09 ` Ian Campbell
@ 2013-12-21 16:17   ` Konrad Rzeszutek Wilk
  2013-12-23  9:37   ` Jan Beulich
  1 sibling, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-21 16:17 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel, JBeulich

On Sat, Dec 21, 2013 at 11:09:54AM +0000, Ian Campbell wrote:
> On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
> 
> > My thought was that we should return 0[...]
> > Like this:
> > 
> > diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
> > index fda19e7..c9ff61e 100644
> > --- a/xen/common/libelf/libelf-dominfo.c
> > +++ b/xen/common/libelf/libelf-dominfo.c
> > @@ -83,7 +83,9 @@ elf_errorstatus elf_xen_parse_features(const char *features,
> >              }
> >          }
> >          if ( i == elf_xen_features )
> > -            return -1;
> > +            return 0; /* We don't recognize this feature, and let the
> > +                       * caller figure out if it has all of the needed parts.
> > +                       */
> 
> WRT the comment, if this code doesn't know about the feature then the
> caller is unlikely to know about it either, so silently ignoring it is
> more than likely completely fine.

<nods>
> 
> [...]
> > But perhaps that is not the way to do it and we should just cherry-pick
> > 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
> 
> I think we should do both, i.e. backport 30832c06a8d1 now to solve the
> immediate problem and then look at fixing unstable to be more accepting
> of new features which it doesn't yet know about. Ultimately perhaps we'd
> want to backport that patch too.

Ok, will crank out a patch after New Year.

Thanks!
> 
> Ian.
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-21 11:09 ` Ian Campbell
  2013-12-21 16:17   ` Konrad Rzeszutek Wilk
@ 2013-12-23  9:37   ` Jan Beulich
  2013-12-23 11:49     ` Jan Beulich
  1 sibling, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-12-23  9:37 UTC (permalink / raw)
  To: Ian.Campbell, konrad.wilk; +Cc: xen-devel

>>> Ian Campbell <Ian.Campbell@citrix.com> 12/21/13 12:10 PM >>>
>On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
>> But perhaps that is not the way to do it and we should just cherry-pick
>> 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
>
>I think we should do both, i.e. backport 30832c06a8d1 now to solve the
>immediate problem and then look at fixing unstable to be more accepting
>of new features which it doesn't yet know about.

Hmm, not sure - without a split between necessary to be understood
and acceptable to be unknown ones, I'm not sure either model will be
the right thing.

>Ultimately perhaps we'd want to backport that patch too.

But not to 4.1, which is only getting security fixes backported.

Jan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-23  9:37   ` Jan Beulich
@ 2013-12-23 11:49     ` Jan Beulich
  2013-12-24  1:56       ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2013-12-23 11:49 UTC (permalink / raw)
  To: Ian.Campbell, konrad.wilk; +Cc: xen-devel

>>> "Jan Beulich" <jbeulich@suse.com> 12/23/13 10:39 AM >>>
>>>> Ian Campbell <Ian.Campbell@citrix.com> 12/21/13 12:10 PM >>>
>>On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
>>> But perhaps that is not the way to do it and we should just cherry-pick
>>> 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
>>
>>I think we should do both, i.e. backport 30832c06a8d1 now to solve the
>>immediate problem and then look at fixing unstable to be more accepting
>>of new features which it doesn't yet know about.
>
>Hmm, not sure - without a split between necessary to be understood
>and acceptable to be unknown ones, I'm not sure either model will be
>the right thing.

And actually, in the case at hand the "BOOM" is correct: If the kernel tells
the hypervisor that it needs a feature the hypervisor doesn't even recognize,
it's surely wrong to ignore this. The mistake here is for the kernel to require
that feature statically in the first place - that should be done only if the kernel
could _only_ boot in PVH mode.

Jan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-23 11:49     ` Jan Beulich
@ 2013-12-24  1:56       ` Konrad Rzeszutek Wilk
  2014-01-07  8:23         ` Jan Beulich
  0 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-24  1:56 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Ian.Campbell

On Mon, Dec 23, 2013 at 11:49:49AM +0000, Jan Beulich wrote:
> >>> "Jan Beulich" <jbeulich@suse.com> 12/23/13 10:39 AM >>>
> >>>> Ian Campbell <Ian.Campbell@citrix.com> 12/21/13 12:10 PM >>>
> >>On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
> >>> But perhaps that is not the way to do it and we should just cherry-pick
> >>> 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
> >>
> >>I think we should do both, i.e. backport 30832c06a8d1 now to solve the
> >>immediate problem and then look at fixing unstable to be more accepting
> >>of new features which it doesn't yet know about.
> >
> >Hmm, not sure - without a split between necessary to be understood
> >and acceptable to be unknown ones, I'm not sure either model will be
> >the right thing.
> 
> And actually, in the case at hand the "BOOM" is correct: If the kernel tells
> the hypervisor that it needs a feature the hypervisor doesn't even recognize,
> it's surely wrong to ignore this. The mistake here is for the kernel to require

But it does not ignore it. It checks later on in construct_dom0 whether
the 'required' parameters are present, like:

if ( test_bit(XENFEAT_supervisor_mode_kernel, parms.f_required) )



> that feature statically in the first place - that should be done only if the kernel
> could _only_ boot in PVH mode.

The feature is not marked as "required" but rather - it can utilize said
extension (so supported). I am advocating that the calleer checks that
all of the required pieces are correct - and it can ignore the ones it
has no idea off (which it does for some of the Xen ELF notes - ignores
them if it has no idea of what they are).


> 
> Jan
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-20 17:57 Xen 4.1 + Linux compiled with PVH == BOOM Konrad Rzeszutek Wilk
  2013-12-21  1:47 ` Mukesh Rathor
  2013-12-21 11:09 ` Ian Campbell
@ 2013-12-24 12:31 ` David Vrabel
  2013-12-24 12:55   ` Andrew Cooper
  2 siblings, 1 reply; 21+ messages in thread
From: David Vrabel @ 2013-12-24 12:31 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, xen-devel, Mukesh Rathor, JBeulich

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-24 12:31 ` David Vrabel
@ 2013-12-24 12:55   ` Andrew Cooper
  2013-12-24 13:05     ` David Vrabel
  0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2013-12-24 12:55 UTC (permalink / raw)
  To: David Vrabel, Konrad Rzeszutek Wilk, xen-devel, Mukesh Rathor, JBeulich

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.

The elf parsing should accept unrecognised fields for forward
compatibility, which would then allow a PV & PVH compiled kernel to run
in PV mode.

If once the kernel starts running, it (for some reason) is unable to
boot in PV mode when it has PVH also compiled in, then this is certainly
a kernel bug.

~Andrew

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-24 12:55   ` Andrew Cooper
@ 2013-12-24 13:05     ` David Vrabel
  2013-12-30 19:56       ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 21+ messages in thread
From: David Vrabel @ 2013-12-24 13:05 UTC (permalink / raw)
  To: Andrew Cooper, Konrad Rzeszutek Wilk, xen-devel, Mukesh Rathor, JBeulich

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

David

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-24 13:05     ` David Vrabel
@ 2013-12-30 19:56       ` Konrad Rzeszutek Wilk
  2014-01-02 19:30         ` Konrad Rzeszutek Wilk
  2014-01-03  0:27         ` Mukesh Rathor
  0 siblings, 2 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-12-30 19:56 UTC (permalink / raw)
  To: David Vrabel, Mukesh Rathor, george.dunlap, ian.campbell,
	jbeulich, roger.pau
  Cc: Andrew Cooper, xen-devel, JBeulich

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


Which way should we do this?

P.S.
It looks like that the git tree is not accessible so I cannot update to the
latest git tree to produce an RFC patchset :-(

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-30 19:56       ` Konrad Rzeszutek Wilk
@ 2014-01-02 19:30         ` Konrad Rzeszutek Wilk
  2014-01-02 21:23           ` Konrad Rzeszutek Wilk
  2014-01-03  0:27         ` Mukesh Rathor
  1 sibling, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-01-02 19:30 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: xen-devel, ian.campbell, george.dunlap, Andrew Cooper,
	David Vrabel, jbeulich, roger.pau

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-02 19:30         ` Konrad Rzeszutek Wilk
@ 2014-01-02 21:23           ` Konrad Rzeszutek Wilk
  2014-01-03 12:40             ` Roger Pau Monné
  0 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-01-02 21:23 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: xen-devel, ian.campbell, george.dunlap, Andrew Cooper,
	David Vrabel, jbeulich, roger.pau

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) |
+						(1 << XENFEAT_dom0))
 	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
 	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
 	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,
diff --git a/include/xen/interface/elfnote.h b/include/xen/interface/elfnote.h
index 0360b15..6f4eae3 100644
--- a/include/xen/interface/elfnote.h
+++ b/include/xen/interface/elfnote.h
@@ -140,6 +140,19 @@
  */
 #define XEN_ELFNOTE_SUSPEND_CANCEL 14
 
+/*
+ * The features supported by this kernel (numeric).
+ *
+ * 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.
+ *
+ * LEGACY: FEATURES
+ */
+#define XEN_ELFNOTE_SUPPORTED_FEATURES 17
+
 #endif /* __XEN_PUBLIC_ELFNOTE_H__ */
 
 /*

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-30 19:56       ` Konrad Rzeszutek Wilk
  2014-01-02 19:30         ` Konrad Rzeszutek Wilk
@ 2014-01-03  0:27         ` Mukesh Rathor
  2014-01-06 11:01           ` Ian Campbell
  1 sibling, 1 reply; 21+ messages in thread
From: Mukesh Rathor @ 2014-01-03  0:27 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: xen-devel, ian.campbell, george.dunlap, Andrew Cooper,
	David Vrabel, jbeulich, roger.pau

On Mon, 30 Dec 2013 14:56:48 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Tue, Dec 24, 2013 at 01:05:10PM +0000, David Vrabel wrote:
......
> > > 
> > > 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.

I"m not sure what the state of auto xlated with shadow and Ian's work
for supervisor mode kernel is. If they are obsolete, then we can drop
the hvm_callback_vector check.

thanks
mukesh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-02 21:23           ` Konrad Rzeszutek Wilk
@ 2014-01-03 12:40             ` Roger Pau Monné
  2014-01-03 14:35               ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monné @ 2014-01-03 12:40 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Konrad Rzeszutek Wilk
  Cc: xen-devel, ian.campbell, george.dunlap, Andrew Cooper,
	David Vrabel, jbeulich

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-03 12:40             ` Roger Pau Monné
@ 2014-01-03 14:35               ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-01-03 14:35 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: xen-devel, ian.campbell, george.dunlap, Andrew Cooper,
	David Vrabel, jbeulich, Konrad Rzeszutek Wilk

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-03  0:27         ` Mukesh Rathor
@ 2014-01-06 11:01           ` Ian Campbell
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Campbell @ 2014-01-06 11:01 UTC (permalink / raw)
  To: Mukesh Rathor
  Cc: xen-devel, george.dunlap, Andrew Cooper, David Vrabel, jbeulich,
	roger.pau

On Thu, 2014-01-02 at 16:27 -0800, Mukesh Rathor wrote:
> On Mon, 30 Dec 2013 14:56:48 -0500
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Tue, Dec 24, 2013 at 01:05:10PM +0000, David Vrabel wrote:
> ......
> > > > 
> > > > 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.
> 
> I"m not sure what the state of auto xlated with shadow and Ian's work
> for supervisor mode kernel is. If they are obsolete, then we can drop
> the hvm_callback_vector check.

supervisor mode kernel is a decade old stunt, I don't think it has any
relevance any more -- or at least I've no intention to work on it any
further and no objections to it being removed (I must confess I thought
it already had been...).

Ian.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2013-12-24  1:56       ` Konrad Rzeszutek Wilk
@ 2014-01-07  8:23         ` Jan Beulich
  2014-01-07 11:31           ` Ian Campbell
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-01-07  8:23 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: xen-devel, Ian.Campbell

>>> On 24.12.13 at 02:56, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Dec 23, 2013 at 11:49:49AM +0000, Jan Beulich wrote:
>> >>> "Jan Beulich" <jbeulich@suse.com> 12/23/13 10:39 AM >>>
>> >>>> Ian Campbell <Ian.Campbell@citrix.com> 12/21/13 12:10 PM >>>
>> >>On Fri, 2013-12-20 at 12:57 -0500, Konrad Rzeszutek Wilk wrote:
>> >>> But perhaps that is not the way to do it and we should just cherry-pick
>> >>> 30832c06a8d1f9caff0987654ef9e24d59469d9a in Xen 4.1?
>> >>
>> >>I think we should do both, i.e. backport 30832c06a8d1 now to solve the
>> >>immediate problem and then look at fixing unstable to be more accepting
>> >>of new features which it doesn't yet know about.
>> >
>> >Hmm, not sure - without a split between necessary to be understood
>> >and acceptable to be unknown ones, I'm not sure either model will be
>> >the right thing.
>> 
>> And actually, in the case at hand the "BOOM" is correct: If the kernel tells
>> the hypervisor that it needs a feature the hypervisor doesn't even 
> recognize,
>> it's surely wrong to ignore this. The mistake here is for the kernel to 
> require
> 
> But it does not ignore it. It checks later on in construct_dom0 whether
> the 'required' parameters are present, like:

That's what I said: It is / would be wrong to ignore a feature th
kernel requires.

> if ( test_bit(XENFEAT_supervisor_mode_kernel, parms.f_required) )

This is even more confusing: 30832c06 is about hvm_callback_vector,
not supervisor_mode_kernel.

>> that feature statically in the first place - that should be done only if the 
> kernel
>> could _only_ boot in PVH mode.
> 
> The feature is not marked as "required" but rather - it can utilize said
> extension (so supported). I am advocating that the calleer checks that
> all of the required pieces are correct - and it can ignore the ones it
> has no idea off (which it does for some of the Xen ELF notes - ignores
> them if it has no idea of what they are).

What would be to point of telling the hypervisor that the kernel
can utilize a certain extension? The kernel could just utilize it, and
the hypervisor would know by that simple fact.

Jan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-07  8:23         ` Jan Beulich
@ 2014-01-07 11:31           ` Ian Campbell
  2014-01-07 11:50             ` Jan Beulich
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Campbell @ 2014-01-07 11:31 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel

On Tue, 2014-01-07 at 08:23 +0000, Jan Beulich wrote:
> >> that feature statically in the first place - that should be done only if the 
> > kernel
> >> could _only_ boot in PVH mode.
> > 
> > The feature is not marked as "required" but rather - it can utilize said
> > extension (so supported). I am advocating that the calleer checks that
> > all of the required pieces are correct - and it can ignore the ones it
> > has no idea off (which it does for some of the Xen ELF notes - ignores
> > them if it has no idea of what they are).
> 
> What would be to point of telling the hypervisor that the kernel
> can utilize a certain extension? The kernel could just utilize it, and
> the hypervisor would know by that simple fact.

But for PVH doesn't the hypervisor need to now at dom0 build time
whether to build a PV or PVH domain? So it needs to know upfront if the
kernel could do PVH or not, and then pick, but once it has picked the
kernel had best follow that choice.

So in the PVH case it's not just a simple case of the kernel deciding to
utilize an optional feature, the optional feature has already been
enabled.

Ian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-07 11:31           ` Ian Campbell
@ 2014-01-07 11:50             ` Jan Beulich
  2014-01-07 13:39               ` Ian Campbell
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2014-01-07 11:50 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel

>>> On 07.01.14 at 12:31, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-01-07 at 08:23 +0000, Jan Beulich wrote:
>> >> that feature statically in the first place - that should be done only if 
> the 
>> > kernel
>> >> could _only_ boot in PVH mode.
>> > 
>> > The feature is not marked as "required" but rather - it can utilize said
>> > extension (so supported). I am advocating that the calleer checks that
>> > all of the required pieces are correct - and it can ignore the ones it
>> > has no idea off (which it does for some of the Xen ELF notes - ignores
>> > them if it has no idea of what they are).
>> 
>> What would be to point of telling the hypervisor that the kernel
>> can utilize a certain extension? The kernel could just utilize it, and
>> the hypervisor would know by that simple fact.
> 
> But for PVH doesn't the hypervisor need to now at dom0 build time
> whether to build a PV or PVH domain?

Which needs to be communicated via hypervisor command line option
anyway. Specifying the option without have a suitable kernel is (of
course) a user error (generally expected to result in a kernel crash).

Jan

> So it needs to know upfront if the
> kernel could do PVH or not, and then pick, but once it has picked the
> kernel had best follow that choice.
> 
> So in the PVH case it's not just a simple case of the kernel deciding to
> utilize an optional feature, the optional feature has already been
> enabled.
> 
> Ian

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Xen 4.1 + Linux compiled with PVH == BOOM
  2014-01-07 11:50             ` Jan Beulich
@ 2014-01-07 13:39               ` Ian Campbell
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Campbell @ 2014-01-07 13:39 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel

On Tue, 2014-01-07 at 11:50 +0000, Jan Beulich wrote:
> >>> On 07.01.14 at 12:31, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2014-01-07 at 08:23 +0000, Jan Beulich wrote:
> >> >> that feature statically in the first place - that should be done only if 
> > the 
> >> > kernel
> >> >> could _only_ boot in PVH mode.
> >> > 
> >> > The feature is not marked as "required" but rather - it can utilize said
> >> > extension (so supported). I am advocating that the calleer checks that
> >> > all of the required pieces are correct - and it can ignore the ones it
> >> > has no idea off (which it does for some of the Xen ELF notes - ignores
> >> > them if it has no idea of what they are).
> >> 
> >> What would be to point of telling the hypervisor that the kernel
> >> can utilize a certain extension? The kernel could just utilize it, and
> >> the hypervisor would know by that simple fact.
> > 
> > But for PVH doesn't the hypervisor need to now at dom0 build time
> > whether to build a PV or PVH domain?
> 
> Which needs to be communicated via hypervisor command line option
> anyway.

I would expect that the plan is to eventually enable PVH by default if
the kernel can cope with it.

>  Specifying the option without have a suitable kernel is (of
> course) a user error (generally expected to result in a kernel crash).

In the case where the user has specified the option sure.

Note that the original issue was a PVH capable kernel under a non-PVH
capable Xen, although we've strayed a bit from that topic.

Ian.

> 
> Jan
> 
> > So it needs to know upfront if the
> > kernel could do PVH or not, and then pick, but once it has picked the
> > kernel had best follow that choice.
> > 
> > So in the PVH case it's not just a simple case of the kernel deciding to
> > utilize an optional feature, the optional feature has already been
> > enabled.
> > 
> > Ian
> 
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2014-01-07 13:39 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-20 17:57 Xen 4.1 + Linux compiled with PVH == BOOM Konrad Rzeszutek Wilk
2013-12-21  1:47 ` Mukesh Rathor
2013-12-21 11:09 ` Ian Campbell
2013-12-21 16:17   ` Konrad Rzeszutek Wilk
2013-12-23  9:37   ` Jan Beulich
2013-12-23 11:49     ` Jan Beulich
2013-12-24  1:56       ` Konrad Rzeszutek Wilk
2014-01-07  8:23         ` Jan Beulich
2014-01-07 11:31           ` Ian Campbell
2014-01-07 11:50             ` Jan Beulich
2014-01-07 13:39               ` Ian Campbell
2013-12-24 12:31 ` David Vrabel
2013-12-24 12:55   ` Andrew Cooper
2013-12-24 13:05     ` David Vrabel
2013-12-30 19:56       ` Konrad Rzeszutek Wilk
2014-01-02 19:30         ` Konrad Rzeszutek Wilk
2014-01-02 21:23           ` Konrad Rzeszutek Wilk
2014-01-03 12:40             ` Roger Pau Monné
2014-01-03 14:35               ` Konrad Rzeszutek Wilk
2014-01-03  0:27         ` Mukesh Rathor
2014-01-06 11:01           ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.