Re: [PATCH v8 05/32] libxc: introduce a domain loader for HVM guest firmware

["Roger Pau Monné" <roger.pau@citrix.com>]

El 08/10/15 a les 12.12, Ian Campbell ha escrit:
> On Wed, 2015-10-07 at 18:55 +0200, Roger Pau Monne wrote:
>> Introduce a very simple (and dummy) domain loader to be used to load the
>> firmware (hvmloader) into HVM guests. Since hmvloader is just a 32bit elf
>> executable the loader is fairly simple.
>>
>> Since the order in which loaders are tested cannot be arranged, prevent the
>> current elfloader from trying to boot a kernel that doesn't contain Xen
>> ELFNOTES.
> 
> I think it relies (probably implicitly and probably not very defined) on
> the link order.
> 
> It is possible to control this (somewhat) because the __init used on
> register_loader turns into __attribute__((constructor)), which takes an
> (optional) priority. You can also (I think) use __attribute__
> ((init_priority (2000))).
> 
> All of which is enormous faff. Having xc_dom_register_loader() take a
> priority, or putting one in struct xc_dom_loader would be less so.
> 
> Apart fromall that, I'm happy with the idea that the elfloader shouldn't be
> selected to load things which it cannot work with.
> 
> However I'm unsure that the presence or absence of ELF notes is sufficient,
> since there is at least the legacy SHT_NOTE section and then __xen_guest
> section (see the tail of elf_xen_parse) as well.

AFAICT NetBSD still uses the __xen_guest section, and it works fine with
this change:

http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/sys/arch/amd64/amd64/locore.S?rev=1.78&content-type=text/plain&only_with_tag=MAIN

elfloader recognizes the kernel and it's able to load it without issues.

xc_dom_parse_elf_kernel already calls elf_xen_parse later on, while
trying to load the kernel, so the functionality it's exactly the same,
it's just that we now simply refuse to try to load a kernel without
elfnotes with elfloader.

Without this change the elfloader would try to load a kernel without
elfnotes just to fail later while parsing it.

> It may well be that the code you've got actually covers these cases and
> it's just the commentary which doesn't. I think this is probably the case
> (since elf_xen_parse calls elf_xen_note_check after handling all those
> cases).
> 
> There is an additional subtlety with ARM not having notes, but I think your
> checks will pass and therefore to the right thing.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> Changes since v7:
>>  - Prevent elfloader from loading kernels that don't have Xen elfnotes.
>>    Another solution is to force an order in the way loaders are tested,
>>    but that's a more complex solution.
>>
>> Changes since v3:
>>  - s/__FUNCTION__/__func__/g
>>  - Fix style errors in xc_dom_hvmloader.c.
>>  - Add Andrew Cooper Reviewed-by.
>>  - Add Wei Acked-by.
>> ---
>>  tools/libxc/Makefile           |   1 +
>>  tools/libxc/include/xc_dom.h   |   8 ++
>>  tools/libxc/xc_dom_elfloader.c |  22 ++-
>>  tools/libxc/xc_dom_hvmloader.c | 313
>> +++++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/libelf.h       |   1 +
>>  5 files changed, 344 insertions(+), 1 deletion(-)
>>  create mode 100644 tools/libxc/xc_dom_hvmloader.c
>>
>> diff --git a/tools/libxc/Makefile b/tools/libxc/Makefile
>> index a0f899b..baaadd6 100644
>> --- a/tools/libxc/Makefile
>> +++ b/tools/libxc/Makefile
>> @@ -84,6 +84,7 @@ GUEST_SRCS-y                 += xc_dom_core.c
>> xc_dom_boot.c
>>  GUEST_SRCS-y                 += xc_dom_elfloader.c
>>  GUEST_SRCS-$(CONFIG_X86)     += xc_dom_bzimageloader.c
>>  GUEST_SRCS-$(CONFIG_X86)     += xc_dom_decompress_lz4.c
>> +GUEST_SRCS-$(CONFIG_X86)     += xc_dom_hvmloader.c
>>  GUEST_SRCS-$(CONFIG_ARM)     += xc_dom_armzimageloader.c
>>  GUEST_SRCS-y                 += xc_dom_binloader.c
>>  GUEST_SRCS-y                 += xc_dom_compat_linux.c
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 30fa8c5..88c5c80 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -14,6 +14,7 @@
>>   */
>>  
>>  #include <xen/libelf/libelf.h>
>> +#include <xenguest.h>
>>  
>>  #define INVALID_P2M_ENTRY   ((xen_pfn_t)-1)
>>  
>> @@ -183,6 +184,13 @@ struct xc_dom_image {
>>          XC_DOM_PV_CONTAINER,
>>          XC_DOM_HVM_CONTAINER,
>>      } container_type;
>> +
>> +    /* HVM specific fields. */
>> +    /* Extra ACPI tables passed to HVMLOADER */
>> +    struct xc_hvm_firmware_module acpi_module;
>> +
>> +    /* Extra SMBIOS structures passed to HVMLOADER */
>> +    struct xc_hvm_firmware_module smbios_module;
>>  };
>>  
>>  /* --- pluggable kernel loader ------------------------------------- */
>> diff --git a/tools/libxc/xc_dom_elfloader.c
>> b/tools/libxc/xc_dom_elfloader.c
>> index 82524c9..36a115e 100644
>> --- a/tools/libxc/xc_dom_elfloader.c
>> +++ b/tools/libxc/xc_dom_elfloader.c
>> @@ -106,7 +106,27 @@ static elf_negerrnoval check_elf_kernel(struct
>> xc_dom_image *dom, bool verbose)
>>  
>>  static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
>>  {
>> -    return check_elf_kernel(dom, 0);
>> +    struct elf_binary elf;
>> +    int rc;
>> +
>> +    rc = check_elf_kernel(dom, 0);
>> +    if ( rc != 0 )
>> +        return rc;
>> +
>> +    rc = elf_init(&elf, dom->kernel_blob, dom->kernel_size);
>> +    if ( rc != 0 )
>> +        return rc;
>> +
>> +    /*
>> +     * We need to check that it contains Xen ELFNOTES,
>> +     * or else we might be trying to load a plain ELF.
>> +     */
>> +    elf_parse_binary(&elf);
>> +    rc = elf_xen_parse(&elf, &dom->parms);
>> +    if ( rc != 0 )
>> +        return rc;
> 
> Can you confirm that I'm right and there is no need to cleanup of free this
> elf object?

AFAICT yes, there's no function to free an elf object, and I haven't
spotted any memory allocations in the functions called
(elf_init/elf_parse_binary/elf_xen_parse).

> It's a bit of a shame to now have to parse the ELF twice. How abusive would
> to be to declare that when a xc_dom ->probe hook returns success it is
> entitled to rely on the contents of dom->loader_private being preserved
> until ->parse is called. In turn this would imply that the first successful
> probe would be used rather than e.g. probing everything and then picking a
> winner from the successful applicants.

We already do this, we stop testing once a probe returns 0 (see
xc_dom_find_loader), and use that as the loader, so there's no
functional change in this aspect. IMHO, this approach is not very
intrusive, would you like me implement it in a followup patch or rather
do it in the series?

Roger.


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