Re: [Xen-devel] [PATCH v1 04/12] xen/hvmlite: Bootstrap HVMlite guest

[Luis R. Rodriguez]

Bleh moving forward please use mcgrof@xxxxxxxxxx, that will be sent to
my employer and my personal address. More below.

On Wed, Jan 27, 2016 at 8:15 AM, Boris Ostrovsky
<boris.ostrovsky@xxxxxxxxxx> wrote:
> On 01/27/2016 10:29 AM, Konrad Rzeszutek Wilk wrote:
>> On Wed, Jan 27, 2016 at 10:17:56AM -0500, Boris Ostrovsky wrote:
>>> On 01/27/2016 10:09 AM, David Vrabel wrote:
>>>> On 27/01/16 15:06, Boris Ostrovsky wrote:
>>> Why not continue relying on paravirt_enabled()? We are going to keep this
>>> in
>>> some form for HVMlite.
>>
>> And this is where Luis comes in. He has posted an patchset which removes
>> the
>> paravirt_enabled with .. Here is the link
>> https://lkml.org/lkml/2015/12/15/772
>
>
> Yes, I saw that and this will be renamed as paravirt_legacy() (which I am
> not sure is really what it should be called.)

Given Konrad's pointers about some folks pushing for BIOS to have a
"legacy free option" where all legacy crap (PS/2, PnP, serial port,
parallel port) are all disabled [0], I'm inclined to respin the rename
patch to use x86_legacy_free(). This can later then be extended
provided such BIOS check becomes available.

[0] http://lkml.kernel.org/r/20160120193241.GA4769@xxxxxxxxxxxxxxxxxx

But -- as I also pointed out to hpa recently, there is an issue with
using things like cpu_has_hypervisor() (or in this case
paravirt_enabled()) for early code, given that it relies on
init_hypervisor_platform() having been called first which is called on
setup_arch() [1]. So paravirt_enabled() really can only be used prior
to setup_arch() correctly (specifically after,
init_hypervisor_platform()), and anything else would be a bug. I
learned the hard way while doing some linker table work and trying to
find a good use / solution to the dead code concerns I've been aiming
to address due to Xen's separate entry point and the nature of pvops
[2] [3]. hpa's response to this issue was that we cannot and should
not abuse the boot_params hardware_subarch for this purpose (in this
case I was suggesting perhaps KVM could use it if it in the future
needed it for a kvm check earlier than setup_arch()), but he noted
that:

  "If you have a genuine need for a "hypervisor type" then that is a
separate thing and should be
   treated separately from subarch.  However, you need to consider
that some hypervisors can
  emulate other hypervisors and you may have more than one hypervisor
API available."

This goes along with my suggestion here earlier where I mentioned that
subarch is already used nicely to pivot off some specific subarch
entry after startup_32(), if we want to avoid yet-another-entry for
HVMlite (the PVH rebrand done cleanly) and still use startup_32() for
it we could perhaps follow similar strategy as with subarch but
instead add a hypervisor type and use that for a stub call the
hypervisor type if needed early on in startup_32(). This could perhaps
in turn also be used as a generic hypervisor type / and more robust /
easier / not-so-convoluted check. For instance of what I think is a
convoluted check refer to snd_intel8x0_inside_vm() in
sound/pci/intel8x0.c with its use of kvm_para_available(), #ifdefery
over boot_cpu_has(X86_FEATURE_HYPERVISOR) (which is just
cpu_has_hypervisor()). If we had a hypervisor type easily accessible
it could both be used by early init code and perhaps driver code to
replace these convoluted checks.

If this seems a bit sensible the next question to ask if -- how we'd
*set* the hypervisor type, do we extend the x86 boot protocol and
enable a hypervisor type on the zero page? That would clearly only
make sense if:

1) Its reasonable to x86 maintainers (perhaps pending this
discussion's outcome? If its preemptively known to not reasonable an
alternative suggestion would be appreciated)
2) Xen is willing to actually set this (and perhaps a new
hypervisor_type_data for the private data structure), but not much
more would be needed, and incorporate this as part of the DmLite
protocol, or whatever its called as an option for some OSes
3) The kernel could get access to this value from the zero page really
early on, immediately following startup_32()

Worth mentioning here also is hpa's clarification on when subarch type
PC (0) should be used: [it should be used if the hardware is]
"enumerable using standard PC mechanisms (PCI, ACPI) and doesn't need
a special boot flow" -- does that fit HVMLite's description so far? If
so then The Xen subarch may need to be redefined as well to be clear
what it means. I don't think we need to be precise but at the very
least cover grounds to enable the definitions to meet its actual use
to not confuse users.

[1] http://lkml.kernel.org/r/CAB=NE6WoKsP8+KGnJEtigWYktCMjg6iherCOcq-jskxi4P2QqA@xxxxxxxxxxxxxx
[2] http://www.do-not-panic.com/2015/12/avoiding-dead-code-pvops-not-silver-bullet.html
[3] http://www.do-not-panic.com/2015/12/xen-and-x86-linux-zero-page.html
[4] http://lkml.kernel.org/r/56A17B01.9040708@xxxxxxxxx

> Another option is to have early microcode code query CPUID to see whether we
> are running on a hypervisor (this in fact is what we originally thought of
> doing before realizing that we have paravirt_enabled()).

Please keep in mind we want to consider use and setting of something
generic, not just a solution for Xen, and if we want to use this to
help avoid a new entry point then strive for something we actually
could get access to very early on the first x86 entry point, both for
32-bit and 64-bit. If its accessible for both 32-bit and 64-bit well
hell, we could unify not only the entry points for HVMLite thing, but
I'm pretty sure also unify the entry points for PV and x86 as well (if
we wanted that) !

  Luis