[PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Andrew Cooper]

c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_* helpers"
introduced a regression building HVM domains in combination with the libxl
"firmware_override=" option.

The older HVM building code (now removed) had no 32bit ELF check, so would
happily load ELF64 images which contained a stub to switch into long mode.

It is convenient for the ELF file to match the intended runmode rather
than the starting runmode.  As such, don't make an arbitrary restriction.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

My Xen Test Framework (which is almost ready to be formally presented on
xen-devel) uses ELF32/ELF64 for the intended runmode to make
compiling/disassembling easier.

At the point that the developer is specifying firmware_override, they are into
"just do what I tell you" territory, and can keep both pieces if they have
actually passed a broken firmware.

This change has actually regressed XenServers automated testing against
xen-upstream, which does make use of the Test Framework.
---
 tools/libxc/xc_dom_hvmloader.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 79a3b99..0cf9887 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -107,13 +107,6 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
         return rc;
     }
 
-    if ( !elf_32bit(elf) )
-    {
-        xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image is not 32bit",
-                     __func__);
-        return -EINVAL;
-    }
-
     /* parse binary and get xen meta info */
     elf_parse_binary(elf);
 
-- 
2.1.4


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

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Roger Pau Monné]

El 11/11/15 a les 21.18, Andrew Cooper ha escrit:
> c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_* helpers"
> introduced a regression building HVM domains in combination with the libxl
> "firmware_override=" option.
> 
> The older HVM building code (now removed) had no 32bit ELF check, so would
> happily load ELF64 images which contained a stub to switch into long mode.
> 
> It is convenient for the ELF file to match the intended runmode rather
> than the starting runmode.  As such, don't make an arbitrary restriction.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Sorry for the breakage, I actually added that check to make the domain
building more sane, but didn't know there where such use cases, so:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

TBH, I though that ELF64 binaries that have a 32bit entry point would
always specify their type as ELF32 (IIRC that's what Xen itself does).

Roger.


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

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Ian Campbell]

On Wed, 2015-11-11 at 20:18 +0000, Andrew Cooper wrote:
> c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_* helpers"
> introduced a regression building HVM domains in combination with the
> libxl
> "firmware_override=" option.
> 
> The older HVM building code (now removed) had no 32bit ELF check, so would
> happily load ELF64 images which contained a stub to switch into long mode.

IOW a ELF64 with 32-bit code at its entry point? Is that entry point the
ELF entry point or the special Xen entry point located via the notes?

I think you likely mean the latter, in which case I'm ok with this change
if that entry point is explicitly documented to be 32-bit irrespective of
the containing ELF file (either the commit message should mention this is 
already the case or the patch should update the docs to make it so). 

I'm sure I saw this getting documented in patches at some point but I can't
actually find them. All I found was docs/misc/pvh-readme.txt which
discusses original pvh and not dmlite, so I'm pretty sure that is the wrong
place to be looking.

In any case I think for the purposes of supporting users who have done
something odd by accident rather than design it would be worthwhile to log
the type of ELF file somewhere, perhaps we already do, but I couldn't find
it if so.

> It is convenient for the ELF file to match the intended runmode rather
> than the starting runmode.  As such, don't make an arbitrary restriction.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> My Xen Test Framework (which is almost ready to be formally presented on
> xen-devel) uses ELF32/ELF64 for the intended runmode to make
> compiling/disassembling easier.
> 
> At the point that the developer is specifying firmware_override, they are into
> "just do what I tell you" territory, and can keep both pieces if they have
> actually passed a broken firmware.
> 
> This change has actually regressed XenServers automated testing against
> xen-upstream, which does make use of the Test Framework.
> ---
>  tools/libxc/xc_dom_hvmloader.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_hvmloader.c
> b/tools/libxc/xc_dom_hvmloader.c
> index 79a3b99..0cf9887 100644
> --- a/tools/libxc/xc_dom_hvmloader.c
> +++ b/tools/libxc/xc_dom_hvmloader.c
> @@ -107,13 +107,6 @@ static elf_errorstatus
> xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
>          return rc;
>      }
>  
> -    if ( !elf_32bit(elf) )
> -    {
> -        xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image is not
> 32bit",
> -                     __func__);
> -        return -EINVAL;
> -    }
> -
>      /* parse binary and get xen meta info */
>      elf_parse_binary(elf);
>  

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

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Andrew Cooper]

On 12/11/15 09:41, Ian Campbell wrote:
> On Wed, 2015-11-11 at 20:18 +0000, Andrew Cooper wrote:
>> c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_* helpers"
>> introduced a regression building HVM domains in combination with the
>> libxl
>> "firmware_override=" option.
>>
>> The older HVM building code (now removed) had no 32bit ELF check, so would
>> happily load ELF64 images which contained a stub to switch into long mode.
> IOW a ELF64 with 32-bit code at its entry point? Is that entry point the
> ELF entry point or the special Xen entry point located via the notes?
>
> I think you likely mean the latter, in which case I'm ok with this change
> if that entry point is explicitly documented to be 32-bit irrespective of
> the containing ELF file (either the commit message should mention this is 
> already the case or the patch should update the docs to make it so).

I mean the former.  This has nothing to do with DMLite guests, so no
elfnotes are involved.

I realise that strictly speaking the elf check should match.  However,
it always used to work, and is sufficiently convenient for development
purposes that I feel the check is more problematic than helpful. 
(Postprocessing the linked binary from 64bit to 32bit elf is an extra
step which also makes it harder to disassemble.)

~Andrew

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Ian Campbell]

On Thu, 2015-11-12 at 12:37 +0000, Andrew Cooper wrote:
> On 12/11/15 09:41, Ian Campbell wrote:
> > On Wed, 2015-11-11 at 20:18 +0000, Andrew Cooper wrote:
> > > c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_*
> > > helpers"
> > > introduced a regression building HVM domains in combination with the
> > > libxl
> > > "firmware_override=" option.
> > > 
> > > The older HVM building code (now removed) had no 32bit ELF check, so
> > > would
> > > happily load ELF64 images which contained a stub to switch into long
> > > mode.
> > IOW a ELF64 with 32-bit code at its entry point? Is that entry point
> > the
> > ELF entry point or the special Xen entry point located via the notes?
> > 
> > I think you likely mean the latter, in which case I'm ok with this
> > change
> > if that entry point is explicitly documented to be 32-bit irrespective
> > of
> > the containing ELF file (either the commit message should mention this
> > is 
> > already the case or the patch should update the docs to make it so).
> 
> I mean the former.  This has nothing to do with DMLite guests, so no
> elfnotes are involved.
> 
> I realise that strictly speaking the elf check should match.  However,
> it always used to work, and is sufficiently convenient for development
> purposes that I feel the check is more problematic than helpful. 
> (Postprocessing the linked binary from 64bit to 32bit elf is an extra
> step which also makes it harder to disassemble.)

An ELF file which says it is class ELF32 but has 64bit code at the entry
point is IMHO malformed.

I think that the fact this used to work is a misfeature, however convenient
you may have found it.

Kernels which wish to do this should IMHO do as Xen does and have a
mkelf32.c step. You can still run gdb or whatever using the original ELF
file.

Ian.

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

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Andrew Cooper]

On 12/11/15 12:47, Ian Campbell wrote:
> On Thu, 2015-11-12 at 12:37 +0000, Andrew Cooper wrote:
>> On 12/11/15 09:41, Ian Campbell wrote:
>>> On Wed, 2015-11-11 at 20:18 +0000, Andrew Cooper wrote:
>>>> c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_*
>>>> helpers"
>>>> introduced a regression building HVM domains in combination with the
>>>> libxl
>>>> "firmware_override=" option.
>>>>
>>>> The older HVM building code (now removed) had no 32bit ELF check, so
>>>> would
>>>> happily load ELF64 images which contained a stub to switch into long
>>>> mode.
>>> IOW a ELF64 with 32-bit code at its entry point? Is that entry point
>>> the
>>> ELF entry point or the special Xen entry point located via the notes?
>>>
>>> I think you likely mean the latter, in which case I'm ok with this
>>> change
>>> if that entry point is explicitly documented to be 32-bit irrespective
>>> of
>>> the containing ELF file (either the commit message should mention this
>>> is 
>>> already the case or the patch should update the docs to make it so).
>> I mean the former.  This has nothing to do with DMLite guests, so no
>> elfnotes are involved.
>>
>> I realise that strictly speaking the elf check should match.  However,
>> it always used to work, and is sufficiently convenient for development
>> purposes that I feel the check is more problematic than helpful. 
>> (Postprocessing the linked binary from 64bit to 32bit elf is an extra
>> step which also makes it harder to disassemble.)
> An ELF file which says it is class ELF32 but has 64bit code at the entry
> point is IMHO malformed.
>
> I think that the fact this used to work is a misfeature, however convenient
> you may have found it.
>
> Kernels which wish to do this should IMHO do as Xen does and have a
> mkelf32.c step. You can still run gdb or whatever using the original ELF
> file.

It turns out that

OUTPUT_FORMAT("elf32-x86-64")

is a thing for linker scripts.

I have updated the framework to use that for 64bit hvm tests, and
everything appears to be working.

~Andrew

Re: [PATCH] tools/libxc: Fix construction of HVM guests with non-default firmware

[Ian Campbell]

On Thu, 2015-11-12 at 16:02 +0000, Andrew Cooper wrote:
> On 12/11/15 12:47, Ian Campbell wrote:
> > On Thu, 2015-11-12 at 12:37 +0000, Andrew Cooper wrote:
> > > On 12/11/15 09:41, Ian Campbell wrote:
> > > > On Wed, 2015-11-11 at 20:18 +0000, Andrew Cooper wrote:
> > > > > c/s 1ee15d7 "libxl: switch HVM domain building to use xc_dom_*
> > > > > helpers"
> > > > > introduced a regression building HVM domains in combination with
> > > > > the
> > > > > libxl
> > > > > "firmware_override=" option.
> > > > > 
> > > > > The older HVM building code (now removed) had no 32bit ELF check,
> > > > > so
> > > > > would
> > > > > happily load ELF64 images which contained a stub to switch into
> > > > > long
> > > > > mode.
> > > > IOW a ELF64 with 32-bit code at its entry point? Is that entry
> > > > point
> > > > the
> > > > ELF entry point or the special Xen entry point located via the
> > > > notes?
> > > > 
> > > > I think you likely mean the latter, in which case I'm ok with this
> > > > change
> > > > if that entry point is explicitly documented to be 32-bit
> > > > irrespective
> > > > of
> > > > the containing ELF file (either the commit message should mention
> > > > this
> > > > is 
> > > > already the case or the patch should update the docs to make it
> > > > so).
> > > I mean the former.  This has nothing to do with DMLite guests, so no
> > > elfnotes are involved.
> > > 
> > > I realise that strictly speaking the elf check should
> > > match.  However,
> > > it always used to work, and is sufficiently convenient for
> > > development
> > > purposes that I feel the check is more problematic than helpful. 
> > > (Postprocessing the linked binary from 64bit to 32bit elf is an extra
> > > step which also makes it harder to disassemble.)
> > An ELF file which says it is class ELF32 but has 64bit code at the
> > entry
> > point is IMHO malformed.
> > 
> > I think that the fact this used to work is a misfeature, however
> > convenient
> > you may have found it.
> > 
> > Kernels which wish to do this should IMHO do as Xen does and have a
> > mkelf32.c step. You can still run gdb or whatever using the original
> > ELF
> > file.
> 
> It turns out that
> 
> OUTPUT_FORMAT("elf32-x86-64")
> 
> is a thing for linker scripts.
> 
> I have updated the framework to use that for 64bit hvm tests, and
> everything appears to be working.

Great. I shall consider this patch withdrawn then.

Thanks for letting us know.

Ian.


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