Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy

From: Andy Lutomirski
Date: Mon Feb 08 2016 - 16:04:58 EST


On Mon, Feb 8, 2016 at 7:31 AM, Boris Ostrovsky
<boris.ostrovsky@xxxxxxxxxx> wrote:
>
>
> On 02/06/2016 03:05 PM, Andy Lutomirski wrote:
>>
>>
>> Anyway, this is all ridiculous. I propose that rather than trying to
>> clean up paravirt_enabled, you just delete it. Here are its users:
>>
>> static inline bool is_hypervisor_range(int idx)
>> {
>> /*
>> * ffff800000000000 - ffff87ffffffffff is reserved for
>> * the hypervisor.
>> */
>> return paravirt_enabled() &&
>> (idx >= pgd_index(__PAGE_OFFSET) - 16) &&
>> (idx < pgd_index(__PAGE_OFFSET));
>> }
>>
>> Nope, wrong. I don't really know what this code is trying to do, but
>> I'm pretty sure it's wrong. Did this mean to check "is Xen PV"? Or
>> was it "is Xen PV or lgeust"? Or what?
>
>
> This range is reserved for hypervisors but the only hypervisor that uses it
> is Xen PV (lguest doesn't run in 64-bit mode).
>
> The check is intended to catch mappings on baremetal kernels that shouldn't
> be there. In other words it's not Xen PV that needs it, it's baremetal that
> wants to catch errors.
>
>
>>
>> if (apm_info.bios.version == 0 || paravirt_enabled() ||
>> machine_is_olpc()) {
>> printk(KERN_INFO "apm: BIOS not found.\n");
>> return -ENODEV;
>> }
>>
>> I assume that is trying to avoid checking for APM on systems that are
>> known to be too new. How about cleanup up the condition to check
>> something sensible?
>
>
> The check here I suspect is unnecessary since version is taken from
> boot_params and thus should be zero for Xen PV (don't know about lguest but
> if it's not then we could just set it there).
>
>>
>> if (!paravirt_enabled() && c->x86 == 5 && c->x86_model < 9) {
>> static int f00f_workaround_enabled;
>> [...]
>>
>> This is asking "are we the natively booted kernel?". This has nothing
>> to do with paravirt in particular. How about just deleting that code?
>> It seems pointless. Sure, it's the responsibility of the real root
>> kernel, but nothing will break if a guest kernel also does the fixup.
>
>
> Yes, I also think so.
>
>>
>> int __init microcode_init(void)
>> {
>> [...]
>> if (paravirt_enabled() || dis_ucode_ldr)
>> return -EINVAL;
>>
>> This is also asking "are we the natively booted kernel?" This is
>> plausibly useful for real. (Borislav, is this actually necessary?)
>> Seems to me there should be a function is_native_root_kernel() or
>> similar. Obviously it could have false positives and code will have
>> to deal with that. (This also could be entirely wrong. What code is
>> responsible for CPU microcode updates on Xen? For all I know, dom0 is
>> *supposed* to apply microcode updates, in which case that check really
>> should be deleted.
>>
>> void __init reserve_ebda_region(void)
>> {
>> [...]
>> if (paravirt_enabled())
>> return;
>>
>> I don't know what the point of this one is.
>
>
> Not sure about this one neither.
>
>>
>> pnpbios turns itself off if paravirt_enabled(). I'm not convinced
>> that's correct.
>>
>> /* only a natively booted kernel should be using TXT */
>> if (paravirt_enabled()) {
>> pr_warning("non-0 tboot_addr but pv_ops is enabled\n");
>> return;
>> }
>>
>> Er, what's wrong with trying to talk to tboot on paravirt? It won't
>> be there unless something is rather wrong. In any case, this could
>> use is_native_root_kernel().
>
>
> As in apm_info case, I don't think this check is necessary.
>
>>
>> if (paravirt_enabled() && !paravirt_has(RTC))
>> return -ENODEV;
>>
>> This actually seems legit. But how about reversing it: if
>> paravirt_has(NO_RTC) return -ENODEV? Problem solved.
>>
>> paravirt_enabled is also used in entry_32.S:
>>
>> cmpl $0, pv_info+PARAVIRT_enabled
>>
>> This is actually trying to check whether pv_cpu_ops.iret ==
>> native_iret. I sincerely hope that no additional support is *ever*
>> added to x86 Linux for systems on which this is not the case.
>
>
> I think we can use ALTERNATIVE(... X86_FEATURE_XENPV) here.
>

I think we can do one better and rearrange the code a bit to look more
like the 64-bit version. See:

commit 7209a75d2009dbf7745e2fd354abf25c3deb3ca3
Author: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
Date: Wed Jul 23 08:34:11 2014 -0700

x86_64/entry/xen: Do not invoke espfix64 on Xen


I'll give this a try soon.

--Andy