Messages in this thread | | | Subject | Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest | From | Boris Ostrovsky <> | Date | Thu, 4 Feb 2016 14:54:15 -0500 |
| |
On 02/03/2016 06:40 PM, Luis R. Rodriguez wrote: > On Wed, Feb 03, 2016 at 03:11:56PM -0500, Boris Ostrovsky wrote: >> On 02/03/2016 01:55 PM, Luis R. Rodriguez wrote: >>> I saw no considerations for the recommendations I had made last on your v1: >>> >>> https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com >>> >>> Of importance: >>> >>> 1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this >>> is for legacy x86: >>> >>> Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed >>> this is wrong. It will be renamed to x86_legacy_free() to align with what folks >>> are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff. >>> This also means re-thinking all use cases and ensuring subarch is used then >>> instead when the goal was to avoid Xen from entering that code. Today Xen does >>> not use this but with my work it does and it helps clean and brush up a lot of >>> these checks with future prospects to even help unify entry points. >> As I said earlier, I am not sure I understand what subarch buys us >> for HVMlite guests. > I accepted subarch may not be the right thing, so proposed a hypervisor type.
I don't see much difference between having an HV-specific subarch and a hypervisor type.
> What it buys you is a strong semantics association between code designed > for a purpose. > >> As for using paravirt_enabled -- this is really only used to >> differentiate HVM from HVMlite and I think (although I'd need to >> check) is only needed by Xen-specific code in a couple of places. > That sounds like a Xen specific use case as such an interface that is > pointed out as going to renamed to reflect its actual use case should not > be abused for that purpose. > >> So if/when it is removed we will switch to something else. Since your work is >> WIP I decided to keep using it until it's clear what other options may be >> available. > And your work is not WIP? I'll be splitting my patches up and the rename > will be atomic, it likely can go in first than yours, so not sure why you > are simply brushing this off.
I didn't mean to imply anything by saying that your patches are a WIP. It's just that I can only write and test my patches against existing code, not the future one.
I am sorry if you felt I was trying to say something else, it certainly was not my intent.
> >>> 2) We should avoid more hypervisor type hacks, and just consider a new >>> hypervisor type to close the gap: >>> >>> Using x86_legacy_free() and friends in a unified way for all systems means it >>> should only be used after init_hypervisor_platform() which is called during >>> setup_arch(). This means we have a semantic gap for checks on "are we on >>> hypervisor type and which one?". >> In this particular case we don't need any information about >> hypervisor until init_hypervisor_platform(). > I pointed out in your v1 patchset how microcode loading was not blocked, you > then asked how KVM does it, and that was explained as well, and that they > don't enable it as well. You need a solution for this.
Not really. Xen will ignore writes to microcode-specific MSRs, just like KVM.
This is exact same behavior we have now with regular HVM guests.
> As-is the x86 boot protocol would not allow an easy way for this, I'm > suggesting we consider extending the boot protocol to add a hypervisor > type and data pointer much as with subarch and subarch_data for the
Who will set hypervisor type and where? It won't be Xen as Andrew mentioned in another email.
> particular purpose of both enabling entry into the same startup_32() > but also a clean way for modifications of stubs both at the beginning > and at the end of startup_32(). > > Pseudo code: > > startup_32() startup_64() > | | > | | > V V > pre_hypervisor_stub_32() pre_hypervisor_stub_64() > | | > | | > V V > [existing startup_32()] [existing startup_64()] > | | > | | > V V > post_hypervisor_stub_32() post_hypervisor_stub_64() > > The pre_hypervisor_stub_32() would have much of the code in > hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64() > would have the 64-bits.
Sure. When the protocol is agreed upon and this code is written we will just move hvmlite_start_xen() to pre_hypervisor_stub_32().
> +int xen_hvmlite __attribute__((section(".data"))) = 0; > +struct hvm_start_info hvmlite_start_info __attribute__((section(".data"))); > +uint hvmlite_start_info_sz = sizeof(hvmlite_start_info); > +struct boot_params xen_hvmlite_boot_params __attribute__((section(".data"))); > +#endif > + >>> The section annotations seems very special use case but likely worth documenting >>> and defining a new macro for in include/linux/compiler.h. This would make it >>> easier to change should we want to change the section used here later and >>> enable others to easily look for the reason for these annotations in a >>> single place. >> I wonder whether __initdata would be a good attribute. We only need >> this early in the boot. > I could not find other users of .data other than some specific driver. > Using anything with *init* alludes you can free the data later but if we > want to keep it I suggest a different prefix, up to you.
That's why I said that we only need this info early in the boot.
-boris
| |