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

[Luis R. Rodriguez]

On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky 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.

OK this is being discussed.

> 
> 
> >
> >         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).

Great, but we won't know unless we can test both. Until then we need the check,
but I'm noting a solution is to use the subarch and my series can address this.

> >
> >         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.

Think is very different than knowing, so without a test we won't know.
Until we test it, we could safely replace this check with the subarch
stuff. Both lguest and PV Xen would opt-out as they do now.

> >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.

This is about the BIOS Etra BDA space that the is reserved, we reserve
this on bare metal to give the OS access to this, but on a PV guest
this is not emulated AFAICT -- we skip reserving this space as its
not needed to do so for PV guests. This is what was explained
to me when I asked about this during the session I put together
over dead code concerns and I brought this one up. Konrad and
Andrew might be able to chime in more.

Anyway, with the subarch stuff I'm doing we can keep it and peg
the check with the subarch.

> >
> >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.

Again, *think* is different than knowing. If we want to keep it just
opting out of it for Xen and lguest subarch should suffice here.

> >
> >         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.

Seems Andy is on this.

  Luis