Message-ID: <20160126203023.GI20964@wotan.suse.de>
Date:	Tue, 26 Jan 2016 21:30:23 +0100
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:	Andrew Cooper <andrew.cooper3@...rix.com>,
	Roger Pau Monné <roger.pau@...rix.com>,
	Juergen Gross <jgross@...e.com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...capital.net>,
	David Vrabel <david.vrabel@...rix.com>,
	"H. Peter Anvin" <hpa@...or.com>, xen-devel@...ts.xenproject.org,
	Borislav Petkov <bp@...e.de>, X86 ML <x86@...nel.org>
Subject: Re: [Xen-devel] [PATCH v1 04/12] xen/hvmlite: Bootstrap HVMlite guest

On Mon, Jan 25, 2016 at 05:55:02PM -0500, Boris Ostrovsky wrote:
> On 01/25/2016 05:19 PM, Luis R. Rodriguez wrote:
> >On Sat, Jan 23, 2016 at 02:49:36PM +0000, Andrew Cooper wrote:
> >
> >
> >>it causes inappropriate linkage between the
> >>toolstack and the version of Linux wishing to be booted.
> >There are ways to do it in such a way that it does not create dependency issues
> >on Linux specific code.
> >
> >
> >0) The Xen way and EFI way
> >
> >A generic data structure is passed to the entry point on the kernel to
> >load the kernel. The kernel in turn must parse this generic specific struct
> >and interprets it and translate it to the kernel specific values.
> >
> >1) The qemu way:
> >
> >One way is to simply not refer to the boot_params data structures but IMHO that
> >produces really ugly code. The qemu folks did it this way, so qemu does not use
> >arch/x86/include/uapi/asm/bootparam.h in any way, instead it uses offsets from
> >a char *header. Refer to load_linux() in hw/i386/pc.c, for instance:
> >
> >header[0x211] |= 0x80;  /* CAN_USE_HEAP */
> >
> >2) The grub way:
> >
> >Another way, which grub uses, is to define their own data structures based
> >on arch/x86/include/uapi/asm/bootparam.h, with their own data types, and refer
> >to them in code, for instance refer to grub_cmd_linux() on the grub file
> >grub-core/loader/i386/pc/linux.c and its copy definition of the header definition
> >in include/grub/i386/linux.h.
> >
> >lh.loadflags |= GRUB_LINUX_FLAG_CAN_USE_HEAP;
> >
> >The way lguest does it in the lguest launcher is IMHO the cleanest of course,
> >but it includes asm/bootparam.h and uses that in code:
> >
> >/* Boot protocol version: 2.07 supports the fields for lguest. */
> >boot->hdr.version = 0x207;
> >
> >But that does mean copying over or using the bootparam.h file and using
> >linux data types.
> >
> >3) Merge of xen way and using subarch_data
> >
> >Only set the subarch and subarch_data pointer, and have the kernel then
> >read the generic data structure and parse it as it used to, the benefit
> >is sharing a common entry point.
> >
> >No one uses this yet. But I think its a reasonable compromise.
> >
> >Perhaps there are other ways as well.
> >
> >>>If true, then by no means, no matter how hard we try, and no matter
> >>>what we do on the Linux front to help clean things up will we be able
> >>>to have a unified bare metal / Xen entry. I'm noting it could be
> >>>possible though provided we do just set the zero page, the subarch to
> >>>Xen and subarch_data to the Xen custom data structure.
> >>All you need is a very small stub which starts per the DMLite ABI, sets
> >>up an appropriate zero_page, and jumps to the native entrypoint.
> >Right.
> 
> 
> I am trying to understand what your objection is to what is proposed
> in this patch. It is the size of the stub? If yes -- what would you
> like to leave out to be done later?

I explained that here briefly:

> >>However, this stub belongs in Linux, not in the Xen toolstack.  That
> >>way, when the Linux boot protocol is modified, both sides can be updated
> >>accordingly.
>
> >Makes sense the different entry points just seems best avoided if possible.

But let me elaborate:

We want to strive towards avoiding new entry points on Linux at all costs,
there have been historic issues with having multiple entry points, some of these
may relate to Xen, some not so much but one should not ignore history. A few
related patches, this incldues EFI entry points since Konrad brings up Xen's
approach was inspired by that as well. Read from bottom up.

Kasan: not yet fixed, the feature completely ignored Xen PV guests as a
possible candidate for x86. Xen PV guests with Kasan enabled crash. The fix
is not so trivial. What I'd prefer is for kasan to have a feature to be disabled
at run time, and with that in place we can simply at early boot disable Kasan
early on boot on Xen until Kasan is properly implemented and supported on Xen.

5054daa285beaf706f051fbd395dc36c9f0f907f - x86/xen: Initialize cr4 shadow for 64-bit PV(H) guests

Due to xen's separate entry point and the addition of a cr4 shadow, only
native had its feature added, this made Xen crash.

f3670394c29ff3730638762c1760fd2f624e6d7b - Revert "x86/efi: Fixup GOT in all boot code paths"

Fails to boot LInus' Sony Vaio Pro 11

9cb0e394234d244fe5a97e743ec9dd7ddff7e64b - x86/efi: Fixup GOT in all boot code paths

Because of the multiple boot entry points the fixup for GOT needs to be done in
different places.

7e8213c1f3acc064aef37813a39f13cbfe7c3ce7 - x86/efi: Correct EFI boot stub use of code32_start

Title says it.

b8ff87a6158886771677e6dc8139bac6e3cba717 - x86/efi: Firmware agnostic handover entry points

Fixes the issue of a mismatch on 32-bit and 64-bit between firmware and kernel run.

99f857db8857aff691c51302f93648263ed07eb1 - x86, build: Dynamically find entry points in compressed startup code

Boot loaders abuse static entry point. Perhaps not directly related to Xen but its a
lesson to learn that regardless of what you document people may abuse the entry points
in very unexpected ways.

f791620fa7517e1045742c475a7f005db9a634b8 - x86, efi: Fix 32-bit EFI handover protocol entry point

After the EFI handover protocol was added 32-bit boots had an issue.

9ca8f72a9297f2052d806bd1111e176533aa69bd  - x86, efi: Handover Protocol

*Only* use an EFI boot stub for EFI capable to avoid duplicate code in
boot loaders to set up and boot Linux.

51b26ada79b605ed709ddcedbb6012e8f8e0ebed - x86: unify arch/x86/boot/compressed/vmlinux_*.lds

Linus unifies vmlinux_*.lds and calls to unify startup_32() and startup_64().
This hasn't been done yet.

--

As someone new reading all this code / commits all I can do is cringe when I see
yet-another-entry-point being added. I think we can do better, so we should strive
to that unless there are real technical impossibilities here.

What I'm proposing?

1) Lets see if we can put a proactive stop-gap to issues such as the cr4 shadow
bug and Kasan bugs from creeping in. This should not only help PV but perhaps
HVMLite. If you'd like to help with that refer to this thread:

http://lkml.kernel.org/r/CAB=NE6VTCRCazcNpCdJ7pN1eD3=x_fcGOdH37MzVpxkKEN5esw@mail.gmail.com

2) asm code sharing prospects - rebranding of PVH to HVMlite

It sounds like HVMlite is really just a clean PVH implementation so we'll
be doing this it seems according to this approach:

a) Since PVH brand is taken add new new Xen clean solution as "HVMLite implementation"
b) Drop PVH
c) Re-brand HVMLite as PVH

Is there really no asm code to share between PV and HVMlite ? How about between
PV and other Linux asm ? Specifically I'm talking about a huge new entry point
called hvmlite_start_xen() of asm code. How much sharing or duplication
is being avoided here?

3) C code sharing prospects: rebranding of PVH to HVMlite

This code seems to share a lot from PV guest. Can we combine?

4) hardware_subarch, hardware_subarch_data and future prospects

Your patch relies on a *new* Linux entry point. Sure, we had one
for EFI, and sure there is another for Xen PV, but since you're just
rebranding PVH to HVMlite and given historic issues with any new
Linux entry points I'd like for us to take a breather and evaluate
the extent our lofty unification goals, and how the x86 boot protocol
could help with this already.

Now, perhaps today it may seem as difficult to unify asm entry points
today all over, but if we can take baby steps towards that I think that
should be seriously evaluated.

For instance, we should consider on relying on hardware_subarch and
hardware_subarch_data to fetch the hvmlite_start_info by taking advantage of
the x86 boot protocol.  The hvmlite_start_info is what Xen uses to send us info
about the guest as your patch proposes (this matches the Xen PV style entry),
we stash it into a standard Linux boot_params variable called
xen_hvmlite_boot_params for the Xen guest in hvmlite_bootparam(). This
data structure and code seems to match very much the PV guest structure,
why not just use a union and differentiate on PV subtype ? If you want to avoid
a lot of PV calls for HVMlite it seems you could just take advantage of
subarch Xen type, and differentiate on the subarch_data within Xen code
to make that code just PV sepecific.

I only see gains by using the Xen subarch, so would like to know why PC is
being pushed.

  Luis

Powered by blists - more mailing lists