Message-ID: <CALCETrVPMdZwT-NKb2VSLK8ej2+N2Bxy8g4ZiZPJG3EL31qkrA@mail.gmail.com>
Date:	Mon, 8 Feb 2016 13:04:31 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:	"Luis R. Rodriguez" <mcgrof@...e.com>,
	"Luis R. Rodriguez" <mcgrof@...nel.org>, cocci@...teme.lip6.fr,
	Juergen Gross <jgross@...e.com>, mcb30@...e.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Joerg Roedel <joro@...tes.org>,
	Robert Moore <robert.moore@...el.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Xen Devel <xen-devel@...ts.xensource.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Jan Beulich <JBeulich@...e.com>, Lv Zheng <lv.zheng@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	long.wanglong@...wei.com, Fengguang Wu <fengguang.wu@...el.com>,
	qiuxishi@...wei.com, Borislav Petkov <bp@...en8.de>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	david.e.box@...el.com, X86 ML <x86@...nel.org>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy

On Mon, Feb 8, 2016 at 7:31 AM, Boris Ostrovsky
<boris.ostrovsky@...cle.com> 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@...capital.net>
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

Powered by blists - more mailing lists