Message-ID: <CAB=NE6WZeP2EfFyE64ya14LYFuCQt+a-YS9CsDQwM5WDj_TfAQ@mail.gmail.com>
Date:	Tue, 26 Jan 2016 11:14:39 -0800
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:	Andy Lutomirski <luto@...capital.net>,
	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>,
	David Vrabel <david.vrabel@...rix.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
	Borislav Petkov <bp@...e.de>,
	Roger Pau Monné <roger.pau@...rix.com>,
	X86 ML <x86@...nel.org>,
	Jörg Rödel <jroedel@...e.de>
Subject: Re: [Xen-devel] [PATCH v1 04/12] xen/hvmlite: Bootstrap HVMlite guest

On Tue, Jan 26, 2016 at 11:00 AM, Boris Ostrovsky
<boris.ostrovsky@...cle.com> wrote:
> On 01/26/2016 01:46 PM, Andy Lutomirski wrote:
>>
>> On Tue, Jan 26, 2016 at 10:34 AM, Luis R. Rodriguez <mcgrof@...e.com>
>> wrote:
>>>
>>> On Mon, Jan 25, 2016 at 05:28:08PM -0500, Boris Ostrovsky wrote:
>>>>
>>>> On 01/25/2016 04:21 PM, H. Peter Anvin wrote:
>>>>>
>>>>> On 01/25/16 13:12, Luis R. Rodriguez wrote:
>>>>>>>
>>>>>>> Perhaps, but someone would still have to set hardware_subarch. And
>>>>>>> it's hvmlite_bootparams() that does it.
>>>>>>
>>>>>> No, Xen would do it as well, essentially all of hvmlite_bootparams()
>>>>>> could be
>>>>>> done in Xen.
>>>>>>
>>>>> Or a stub code.
>>>>
>>>> This patch in fact is the stub for Xen HVMlite guests, after we are
>>>> done with it we jump to bare-metal startup code (i.e startup_32|64)
>>>
>>> Right the point is the stub need not be in Linux, I'll explain in the
>>> other
>>> thread where I provided more details on the different known approaches.
>>>
>> ISTM if the Xen ABI-specified entry point has a different convention
>> than the Linux native entry, then the stub should live in Linux.  It
>> would be just a couple if lines of code, right?
>
>
> It's not exactly a couple of lines but it's not large neither. It mainly
> sets up boot_params (similar to what make_boot_params() does for EFI). Plus,
> for 64-bit, it loads identity page tables and switches to long mode. And
> then jumps to bare-meta startup code.

This terse summary provides an example of the issue I'm highlighting.
Even though the stub is small we undermine its impact!

Although the stub is small the different entry point also enables
subtle additions which are required, although they are minimal they
are important and if not considered for new x86 features causes
regressions. I don't care what people tell me about "this should have
been caught by code review, and no one CC'd me -- whaa!" -- this is a
silly expectation and I think we should do better.

Case in point:

xen_start_kernel() has seen regressions now on both cr4_init_shadow()
which you forgot to add to Xen Andy -- and later Boris fixed. Another
example: the latest one is a kasan init -- which to this day remains
fucked up -- a Kasan enabled PV guest crashes, and fixing is no where
in sight. I flagged this a while ago, and I think we should put a
proactive measure in place for that.

The linker table stuff I'm doing was not for kicks -- its a means to
an end here, and although I can't yet read subarch at early init, if
we had it, then things link xen_start_kernel() could just be an x86
init stub, it'd be the first stub Xen runs. You can keep whatever
boot_params setup stub it does today, even though I think its cleaner
done in Xen, but at the very least it could at least instead just
*read* the Xen custom generic structure to parse and set boot_params
by using subarch_data pointer.

I also have been reading the history of code changes on the other
entry points in Linux and even though the code is small I see tons of
commits in there for minor but critical fixes all over, likewise
comments and recommendations and visions to unify / share code. I
refuse to accept that we should undermine the issues of a new entry
point or leaving the situation as-is with dual entry points for x86_64
 / xen if we can instead just cleanly unify even asm entry points.

>> The issue that caused headaches in the past isn't that there's code
>> that's executed only on native, it's that there are whole big
>> functions that are executed only on native for no good reason and that
>> aren't clearly marked.

Its more than that, as I noted.

> This won't happen with HVMlite.

And that's fine, its just I think we can avoid yet-another entry --
even if we still are coming into a common C entry later.

>> If we had native_start_kernel and xen_start_kernel, and they both
>> called very quickly in to common_start_kernel, it would be very clear
>> what's going on.
>
> What is now xen_start_kernel() is no longer the entry point for HVMlite. It
> is called as x86_init.oem.arch_setup() (or I may even move it to
> x86_hyper_xen.init_platform() or something like that).

And that's a huge win! Yet I invite us to consider other prospects to
even merge more and simplify more.

 Luis

Powered by blists - more mailing lists