paravirt: rebrand paravirt_enabled as paravirt_legacy

paravirt_enabled conveys the idea that if this is set or if paravirt_enabled() returns true you are in a paravirtualized environment. This is not true by any means, and left as-is is just causing confusion and is prone to be misused and abused.

This primitive is really only useful to determine if you have a paravirtualization hypervisor that supports legacy paravirtualized guests. At run time, this tells us if we've booted into a Linux guest with support for legacy devices and features.

To avoid further issues with semantics on this we loosely borrow the definition of "legacy" from both the ACPI 5.2.9.3 "IA-PC Boot Architecture Flags" section and the PC 2001 definition in the PC Systems design guide [0]:

paravirt_legacy() is true if this hypervisor supports legacy
x86 paravirtualized guests.

+/**
+ * struct pv_info - paravirt hypervisor information
+ *
+ * @supports_x86_legacy: true if this hypervisor supports legacy x86

+ *     paravirtualized guests.  The definition of legacy here adheres
+ *     *loosely* to both the notion of legacy in the ACPI 5.2.9.3 "IA-PC Boot
+ *     Architecture Flags" section and the PC 2001 "legacy free" concept [1]
+ *     referred to in the PC System Design Guide [2] [3] on Chapter 3, Page 50
+ *     [4].  Legacy x86 guests systems are guest systems which are not "legacy
+ *     free" as per the PC 2001 definition, and in the ACPI sense could have
+ *     any of the legacy ACPI IA-PC Boot architecture flags set. These are x86
+ *     systems with any type of legacy peripherals or requirements.

+ *
+ * Examples of some popular legacy peripherals:
+ *
+ * a) Floppy drive
+ * b) Legacy ports [1] such as such as parallel ports, PS/2 connectors,
+ * serial ports / RS-232, game ports Parallel ATA, and IEEE 1394
+ * c) ISA bus
+ *
+ * Examples of features required to support such type of legacy guests
+ * are the need for APM and a PNP BIOS.

Quoted 16 lines · Luis R. Rodriguez

paravirt_enabled conveys the idea that if this is set or if paravirt_enabled() returns true you are in a paravirtualized environment. This is not true by any means, and left as-is is just causing confusion and is prone to be misused and abused.

This primitive is really only useful to determine if you have a paravirtualization hypervisor that supports legacy paravirtualized guests. At run time, this tells us if we've booted into a Linux guest with support for legacy devices and features.

To avoid further issues with semantics on this we loosely borrow the definition of "legacy" from both the ACPI 5.2.9.3 "IA-PC Boot Architecture Flags" section and the PC 2001 definition in the PC Systems design guide [0]:

Quoted 2 lines

paravirt_legacy() is true if this hypervisor supports legacy
x86 paravirtualized guests.

This needs to be far more concrete. I'm reasonably well versed in x86 details relevant to kernels ans I have *no clue* what your semantics mean.

Quoted 12 lines

+/**
+ * struct pv_info - paravirt hypervisor information
+ *
+ * @supports_x86_legacy: true if this hypervisor supports legacy x86

+ *     paravirtualized guests.  The definition of legacy here adheres
+ *     *loosely* to both the notion of legacy in the ACPI 5.2.9.3 "IA-PC Boot
+ *     Architecture Flags" section and the PC 2001 "legacy free" concept [1]
+ *     referred to in the PC System Design Guide [2] [3] on Chapter 3, Page 50
+ *     [4].  Legacy x86 guests systems are guest systems which are not "legacy
+ *     free" as per the PC 2001 definition, and in the ACPI sense could have
+ *     any of the legacy ACPI IA-PC Boot architecture flags set. These are x86
+ *     systems with any type of legacy peripherals or requirements.

Quoted 10 lines

+ *
+ * Examples of some popular legacy peripherals:
+ *
+ * a) Floppy drive
+ * b) Legacy ports [1] such as such as parallel ports, PS/2 connectors,
+ * serial ports / RS-232, game ports Parallel ATA, and IEEE 1394
+ * c) ISA bus
+ *
+ * Examples of features required to support such type of legacy guests
+ * are the need for APM and a PNP BIOS.

Seriously?

I think you just defined every standard native x86 system as well as QEMU/KVM as "legacy".

Can we just enumerate this crap? I propose:

Xen PV and lguest are paravirt_legacy. Nothing else is paravirt_legacy.

The addition of new paravirt_legacy support is
strongly discouraged.

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Luis R. Rodriguez

paravirt_enabled conveys the idea that if this is set or if paravirt_enabled() returns true you are in a paravirtualized environment. This is not true by any means, and left as-is is just causing confusion and is prone to be misused and abused.

This primitive is really only useful to determine if you have a paravirtualization hypervisor that supports legacy paravirtualized guests. At run time, this tells us if we've booted into a Linux guest with support for legacy devices and features.

To avoid further issues with semantics on this we loosely borrow the definition of "legacy" from both the ACPI 5.2.9.3 "IA-PC Boot Architecture Flags" section and the PC 2001 definition in the PC Systems design guide [0]:

Quoted 6 lines

Quoted 2 lines

paravirt_legacy() is true if this hypervisor supports legacy
x86 paravirtualized guests.

This needs to be far more concrete. I'm reasonably well versed in x86 details relevant to kernels ans I have *no clue* what your semantics mean.

Interesting. I'm glad you're chiming in :)

Quoted 23 lines

Quoted 12 lines

+/**
+ * struct pv_info - paravirt hypervisor information
+ *
+ * @supports_x86_legacy: true if this hypervisor supports legacy x86

+ *     paravirtualized guests.  The definition of legacy here adheres
+ *     *loosely* to both the notion of legacy in the ACPI 5.2.9.3 "IA-PC Boot
+ *     Architecture Flags" section and the PC 2001 "legacy free" concept [1]
+ *     referred to in the PC System Design Guide [2] [3] on Chapter 3, Page 50
+ *     [4].  Legacy x86 guests systems are guest systems which are not "legacy
+ *     free" as per the PC 2001 definition, and in the ACPI sense could have
+ *     any of the legacy ACPI IA-PC Boot architecture flags set. These are x86
+ *     systems with any type of legacy peripherals or requirements.

Quoted 10 lines

+ *
+ * Examples of some popular legacy peripherals:
+ *
+ * a) Floppy drive
+ * b) Legacy ports [1] such as such as parallel ports, PS/2 connectors,
+ * serial ports / RS-232, game ports Parallel ATA, and IEEE 1394
+ * c) ISA bus
+ *
+ * Examples of features required to support such type of legacy guests
+ * are the need for APM and a PNP BIOS.

Quoted 4 lines

Seriously?

I think you just defined every standard native x86 system as well as QEMU/KVM as "legacy".

I was a afraid it was too broad, but its why I used "loosely".

Quoted 4 lines

Can we just enumerate this crap? I propose:

Xen PV and lguest are paravirt_legacy. Nothing else is paravirt_legacy.

Fine by me!

Quoted 2 lines

The addition of new paravirt_legacy support is
strongly discouraged.

Fine by me, but Boris is re-using that for HVMLite on his recently proposed patches. Do we want that ? I'll also note that the goal is to ensure its hardware_subarch will be 0 (PC), meanwhile old PV will be Xen (although this hasn't been set yet). This mess is part of the reason why I do think stronger semantics and clearer definitions will help here. Without strong semantics I can't help address the dead code concerns I've been rambling over.

HVMLite is just a rebranding for PVH done right, and then it seems PVH will be dropped and we'll have HVMLite rebranded back to PVH I guess it seems.

The enumeration of legacy crap by ACPI boot flags seems to provide enough details to suit our needs if we really wanted to zero down on the specifics of what paravirt_legacy() means, there are these flags:

/* Masks for FADT IA-PC Boot Architecture Flags (boot_flags) [Vx]=Introduced in this FADT revision */

#define ACPI_FADT_LEGACY_DEVICES    (1)         /* 00: [V2] System has LPC or ISA bus devices */
#define ACPI_FADT_8042              (1<<1)      /* 01: [V3] System has an 8042 controller on port 60/64 */
#define ACPI_FADT_NO_VGA            (1<<2)      /* 02: [V4] It is not safe to probe for VGA hardware */
#define ACPI_FADT_NO_MSI            (1<<3)      /* 03: [V4] Message Signaled Interrupts (MSI) must not be enabled */
#define ACPI_FADT_NO_ASPM           (1<<4)      /* 04: [V4] PCIe ASPM control must not be enabled */
#define ACPI_FADT_NO_CMOS_RTC       (1<<5)      /* 05: [V5] No CMOS real-time clock present */

I checked and I didn't see qemu using any of the ACPI boot flags, but I suspected qemu instances must use a series of legacy crap. Likewise for KVM.

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.

Quoted 8 lines · Andy Lutomirski

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

There was some breakage on 32-bit pvops with that.

Quoted 6 lines

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.

So there are two aspects:

- the guest loading the microcode driver. Xen should behave like qemu+kvm does: emulate the MSR accesses the microcode loader does.

- microcode application on Xen: we've had this before. The hypervisor should do that (if it doesn't do so already).

So yes, that paravirt_enabled() thing should go away. Even more so if we have CPUID leaf 0x4... reserved for hypervisors.

It does. Very much IIRC, the problem was not caused by an access to MSR but rather some sort of address not being available somewhere.

Quoted 2 lines

- microcode application on Xen: we've had this before. The hypervisor should do that (if it doesn't do so already).

it does.

Quoted 2 lines

So yes, that paravirt_enabled() thing should go away. Even more so if we have CPUID leaf 0x4... reserved for hypervisors.

I actually think this was the original proposal until we realized we had paravirt_enabled(). So we can go back to checking CPUID 0x40000000.

We might also be able to test for (x86_hyper!=NULL) and have guests that do microcode management prior to init_hypervisor() rely on hypervisors ignoring MSR accesses (as they do today).

Quoted 2 lines · Boris Ostrovsky

It does. Very much IIRC, the problem was not caused by an access to MSR but rather some sort of address not being available somewhere.

See below.

Quoted 3 lines

Quoted 2 lines

- microcode application on Xen: we've had this before. The hypervisor should do that (if it doesn't do so already).

it does.

Good.

Quoted 8 lines

Quoted 2 lines

So yes, that paravirt_enabled() thing should go away. Even more so if we have CPUID leaf 0x4... reserved for hypervisors.

I actually think this was the original proposal until we realized we had paravirt_enabled(). So we can go back to checking CPUID 0x40000000.

We might also be able to test for (x86_hyper!=NULL) and have guests that do microcode management prior to init_hypervisor() rely on hypervisors ignoring MSR accesses (as they do today).

Right, so the early loader can't do that as on 32-bit it runs even before paging has been enabled. So I *think* the thing with CPUID would be best. What does the xen hypervisor return in regs when I do CPUID(4)? I.e., how do I reliably detect it in the guest?

I can whip up a quick patch and get rid of paravirt_enabled() while at it...

For compatibility with other virtualisation specs, Xen's cpuid leaves shift depending on configuration.

Spec at
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/arch-x86/cpuid.h;h=d709340f18d089560b959835eabb7b6609542c7e;hb=HEAD#l33

Basically, they are either at 0x40000000, or 0x40000100 if viridian or vmware compatibility has been enabled.

Quoted 4 lines · Andrew Cooper

For compatibility with other virtualisation specs, Xen's cpuid leaves shift depending on configuration.

Quoted 6 lines

Spec at
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/arch-x86/cpuid.h;h=d709340f18d089560b959835eabb7b6609542c7e;hb=HEAD#l33

Basically, they are either at 0x40000000, or 0x40000100 if viridian or vmware compatibility has been enabled.

(Also in arch/x86/include/asm/xen/cpuid.h in Linux tree).

Quoted 9 lines · Boris Ostrovsky

Quoted 7 lines · Andrew Cooper

For compatibility with other virtualisation specs, Xen's cpuid leaves shift depending on configuration.

Spec at
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/arch-x86/cpuid.h;h=d709340f18d089560b959835eabb7b6609542c7e;hb=HEAD#l33

Quoted 3 lines

Quoted 2 lines

Basically, they are either at 0x40000000, or 0x40000100 if viridian or vmware compatibility has been enabled.

(Also in arch/x86/include/asm/xen/cpuid.h in Linux tree).

The other complication is that, for a PV guest, you must use the forced emulation prefix for the cpuid instruction to trap into the hypervisor.

This either means you must be sure you are under Xen to start with, or use an extable entry to catch the #UD fault if you are not under Xen.

Quoted 14 lines · Andrew Cooper

Quoted 13 lines · Boris Ostrovsky

Quoted 10 lines · Andrew Cooper

For compatibility with other virtualisation specs, Xen's cpuid leaves shift depending on configuration.

Spec at
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/arch-x86/cpuid.h;h=d709340f18d089560b959835eabb7b6609542c7e;hb=HEAD#l33

Basically, they are either at 0x40000000, or 0x40000100 if viridian or vmware compatibility has been enabled.

(Also in arch/x86/include/asm/xen/cpuid.h in Linux tree).

Quoted 5 lines

The other complication is that, for a PV guest, you must use the forced emulation prefix for the cpuid instruction to trap into the hypervisor.

This either means you must be sure you are under Xen to start with, or use an extable entry to catch the #UD fault if you are not under Xen.

I think we are OK for PV because this code will be executed after pvops are set and so we will be calling xen_cpuid().

I think we are OK for PV because this code will be executed after pvops are set and so we will be calling xen_cpuid().

Quoted 2 lines · Boris Ostrovsky

I think we are OK for PV because this code will be executed after pvops are set and so we will be calling xen_cpuid().

Not for the early loader - it is too early for pvops then. So you're saying something like that won't work?

Does the early loader have extable support? If so, this is fairly easy to fix. If not, we have a problem.

Quoted 2 lines · Andrew Cooper

Does the early loader have extable support? If so, this is fairly easy to fix. If not, we have a problem.

It doesn't and regardless, you want to have this CPUID querying as simple as possible. No special handling, no special prefixes as it should be able to run on other hypervisors too.

If one can't execute a simple CPUID(0x4...) on a xen guest and get the results back, then for early, we will have to do what we've done until now and simply emulate the MSR accesses.

Later code can use then xen_cpuid() and all is fine. We should still get rid of paravirt_enabled() though.

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Andrew Cooper

Does the early loader have extable support? If so, this is fairly easy to fix. If not, we have a problem.

It doesn't and regardless, you want to have this CPUID querying as simple as possible. No special handling, no special prefixes as it should be able to run on other hypervisors too.

Quoted 3 lines

If one can't execute a simple CPUID(0x4...) on a xen guest and get the results back, then for early, we will have to do what we've done until now and simply emulate the MSR accesses.

I think xen_hypervisor check can be done in microcode_init() as this is first time PV kernel deals with microcode.

Let me try it --- I want to see what happens on hotplug and resume but I am reasonably certain this should work during boot.

So it looks like we can just simply revert a18a0f6850 because the very next patch to microcode code (fbae4ba8c4a) makes the original problem (of using __pa_nodebug, which we shouldn't use on PV) go away: we don't call load_ucode_ap from resume path anymore.

Quoted 2 lines · Andrew Cooper

Does the early loader have extable support? If so, this is fairly easy to fix. If not, we have a problem.

It doesn't and regardless, you want to have this CPUID querying as simple as possible. No special handling, no special prefixes as it should be able to run on other hypervisors too.

If one can't execute a simple CPUID(0x4...) on a xen guest and get the results back, then for early, we will have to do what we've done until now and simply emulate the MSR accesses.

Later code can use then xen_cpuid() and all is fine. We should still get rid of paravirt_enabled() though.

As an alternative check which should be doable this early on, peeking in the head of hypercall_page should work. If Linux was booted as a PV guest, the hypercall_page will have been constructed by the domain builder, and won't have 0x90's in it.

Quoted 7 lines · Borislav Petkov

Quoted 2 lines · Andrew Cooper

Does the early loader have extable support? If so, this is fairly easy to fix. If not, we have a problem.

It doesn't and regardless, you want to have this CPUID querying as simple as possible. No special handling, no special prefixes as it should be able to run on other hypervisors too.

Quoted 7 lines

If one can't execute a simple CPUID(0x4...) on a xen guest and get the results back, then for early, we will have to do what we've done until now and simply emulate the MSR accesses.

Later code can use then xen_cpuid() and all is fine. We should still get rid of paravirt_enabled() though.

The force emulation prefix starts with a ud2a instruction, so extable is to prevent it breaking on non-Xen systems. However, if extable isn't available, this point is moot.

As an alternative check which should be doable this early on, peeking in the head of hypercall_page should work. If Linux was booted as a PV guest, the hypercall_page will have been constructed by the domain builder, and won't have 0x90's in it.

Quoted 2 lines · Boris Ostrovsky

I think we are OK for PV because this code will be executed after pvops are set and so we will be calling xen_cpuid().

Not for the early loader - it is too early for pvops then. So you're saying something like that won't work?

Keep in mind that Xen PV doesn't go through startup_32|64(). It starts at xen_start_kernel (save for a small stub before that), which sets pvops. It "joins" regular/baremetal code in i386_start_kernel/x86_64_start_reservation().

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?

         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?

         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.

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.

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

         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.

So yeah, I think the right solution is to delete paravirt_enabled.

--Andy

This range is reserved for hypervisors but the only hypervisor that uses it is Xen PV (lguest doesn't run in 64-bit mode).

Quoted 2 lines · Boris Ostrovsky

This range is reserved for hypervisors but the only hypervisor that uses it is Xen PV (lguest doesn't run in 64-bit mode).

Yeah, this is mentioned in arch/x86/include/asm/page_64_types.h:

/*
* Set __PAGE_OFFSET to the most negative possible address +
* PGDIR_SIZE*16 (pgd slot 272). The gap is to allow a space for a
* hypervisor to fit. Choosing 16 slots here is arbitrary, but it's
* what Xen requires.
*/
#define __PAGE_OFFSET _AC(0xffff880000000000, UL)

It is also in Documentation/x86/x86_64/mm.txt:

ffff800000000000 - ffff87ffffffffff (=43 bits) guard hole, reserved for hypervisor

albeit a bit laconic.

Quoted 16 lines · Andy Lutomirski

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));
}

Quoted 3 lines

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.

Quoted 10 lines

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

Quoted 9 lines

         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.

Quoted 16 lines

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.

Quoted 7 lines

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.

Quoted 10 lines

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;
         }

Quoted 3 lines

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.

Quoted 9 lines

         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:

Quoted 5 lines

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.

Quoted 15 lines · Andy Lutomirski

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));
}

Quoted 3 lines

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.

Quoted 9 lines

        if (apm_info.bios.version == 0 || paravirt_enabled() ||
machine_is_olpc()) {
                printk(KERN_INFO "apm: BIOS not found.\n");
                return -ENODEV;
        }

Quoted 3 lines

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

Quoted 9 lines

        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.

Quoted 21 lines

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;

Quoted 1 line

I don't know what the point of this one is.

Not sure about this one neither.

Quoted 10 lines

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;
        }

Quoted 3 lines

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.

Quoted 9 lines

        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:

Quoted 5 lines

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.

OK so here's a wiki to keep track of progress of the difference uses:

http://kernelnewbies.org/KernelProjects/remove-paravirt-enabled

It seems we have a resolution one way or another for all except for the use on arch/x86/mm/dump_pagetables.c, is that right?

Quoted 4 lines · Luis R. Rodriguez

OK so here's a wiki to keep track of progress of the difference uses:

http://kernelnewbies.org/KernelProjects/remove-paravirt-enabled

Quoted 2 lines

It seems we have a resolution one way or another for all except for the use on arch/x86/mm/dump_pagetables.c, is that right?

Why not?

I think we should simply check the range as ffff800000000000 - ffff87ffffffffff is practically an ABI and nothing should be mapped there anyway. No need for paravirt_enabled() there either.

Quoted 4 lines · Luis R. Rodriguez

OK so here's a wiki to keep track of progress of the difference uses:

http://kernelnewbies.org/KernelProjects/remove-paravirt-enabled

Quoted 2 lines

It seems we have a resolution one way or another for all except for the use on arch/x86/mm/dump_pagetables.c, is that right?

Why not?

I think we should simply check the range as ffff800000000000 - ffff87ffffffffff is practically an ABI and nothing should be mapped

there anyway. No need for paravirt_enabled() there either.

That's exactly the point: if something is mapped it's an error for a non-PV kernel.

By removing paravirt_enabled() we may miss those errors. Worse, I think we may even crash while doing pagetable walk (although it's probably better to crash here than to use an unexpected translation in real code somewhere)

Quoted 2 lines · Boris Ostrovsky

That's exactly the point: if something is mapped it's an error for a non-PV kernel.

How would something be mapped there? __PAGE_OFFSET is 0xffff880000000000.

Or are you thinking about some insanely b0rked kernel code mapping stuff in there?

Quoted 3 lines

By removing paravirt_enabled() we may miss those errors. Worse, I think we may even crash while doing pagetable walk (although it's probably better to crash here than to use an unexpected translation in real code somewhere)

Well, if this is the only site which keeps paravirt_enabled() from being buried, we need to think about a better way to detect a hypervisor. Maybe we should look at x86_hyper, use CPUID(0x4...) or something else.

What's your preference?

Quoted 12 lines · Borislav Petkov

Quoted 2 lines · Boris Ostrovsky

That's exactly the point: if something is mapped it's an error for a non-PV kernel.

How would something be mapped there? __PAGE_OFFSET is 0xffff880000000000.

Or are you thinking about some insanely b0rked kernel code mapping stuff in there?

Quoted 3 lines

By removing paravirt_enabled() we may miss those errors. Worse, I think we may even crash while doing pagetable walk (although it's probably better to crash here than to use an unexpected translation in real code somewhere)

Quoted 5 lines

Well, if this is the only site which keeps paravirt_enabled() from being buried, we need to think about a better way to detect a hypervisor. Maybe we should look at x86_hyper, use CPUID(0x4...) or something else.

What's your preference?

I'm confused. Isn't it the other way around? That is, checking for the hypervisor range on all systems should be safer, right? Or am I missing something?

Hmm. I think you are right --- I was following wrong branch of the 'if' statement. We are always going straight to note_page().

Then yes, we should be able to remove paravirt_enabled(). Sorry for the noise.

Quoted 2 lines · Boris Ostrovsky

That's exactly the point: if something is mapped it's an error for a non-PV kernel.

How would something be mapped there? __PAGE_OFFSET is 0xffff880000000000.

Or are you thinking about some insanely b0rked kernel code mapping stuff in there?

Quoted 4 lines

By removing paravirt_enabled() we may miss those errors. Worse, I think we may even crash while doing pagetable walk (although it's probably better to crash here than to use an unexpected translation in real code somewhere)

Well, if this is the only site which keeps paravirt_enabled() from being buried, we need to think about a better way to detect a hypervisor. Maybe we should look at x86_hyper, use CPUID(0x4...)

or something else.

What's your preference?

Quoted 2 lines · Boris Ostrovsky

That's exactly the point: if something is mapped it's an error for a non-PV kernel.

How would something be mapped there? __PAGE_OFFSET is 0xffff880000000000.

Or are you thinking about some insanely b0rked kernel code mapping stuff in there?

Quoted 3 lines

By removing paravirt_enabled() we may miss those errors. Worse, I think we may even crash while doing pagetable walk (although it's probably better to crash here than to use an unexpected translation in real code somewhere)

Well, if this is the only site which keeps paravirt_enabled() from being buried, we need to think about a better way to detect a hypervisor. Maybe we should look at x86_hyper, use CPUID(0x4...) or something else.

What's your preference?

Quoted 75 lines · Luis R. Rodriguez

Quoted 17 lines · Andy Lutomirski

Quoted 16 lines · Luis R. Rodriguez

paravirt_enabled conveys the idea that if this is set or if paravirt_enabled() returns true you are in a paravirtualized environment. This is not true by any means, and left as-is is just causing confusion and is prone to be misused and abused.

This primitive is really only useful to determine if you have a paravirtualization hypervisor that supports legacy paravirtualized guests. At run time, this tells us if we've booted into a Linux guest with support for legacy devices and features.

To avoid further issues with semantics on this we loosely borrow the definition of "legacy" from both the ACPI 5.2.9.3 "IA-PC Boot Architecture Flags" section and the PC 2001 definition in the PC Systems design guide [0]:

Quoted 6 lines

Quoted 2 lines

paravirt_legacy() is true if this hypervisor supports legacy
x86 paravirtualized guests.

This needs to be far more concrete. I'm reasonably well versed in x86 details relevant to kernels ans I have *no clue* what your semantics mean.

Interesting. I'm glad you're chiming in :)

Quoted 23 lines

Quoted 12 lines

+/**
+ * struct pv_info - paravirt hypervisor information
+ *
+ * @supports_x86_legacy: true if this hypervisor supports legacy x86

+ *     paravirtualized guests.  The definition of legacy here adheres
+ *     *loosely* to both the notion of legacy in the ACPI 5.2.9.3 "IA-PC Boot
+ *     Architecture Flags" section and the PC 2001 "legacy free" concept [1]
+ *     referred to in the PC System Design Guide [2] [3] on Chapter 3, Page 50
+ *     [4].  Legacy x86 guests systems are guest systems which are not "legacy
+ *     free" as per the PC 2001 definition, and in the ACPI sense could have
+ *     any of the legacy ACPI IA-PC Boot architecture flags set. These are x86
+ *     systems with any type of legacy peripherals or requirements.

Quoted 10 lines

+ *
+ * Examples of some popular legacy peripherals:
+ *
+ * a) Floppy drive
+ * b) Legacy ports [1] such as such as parallel ports, PS/2 connectors,
+ * serial ports / RS-232, game ports Parallel ATA, and IEEE 1394
+ * c) ISA bus
+ *
+ * Examples of features required to support such type of legacy guests
+ * are the need for APM and a PNP BIOS.

Quoted 4 lines

Seriously?

I think you just defined every standard native x86 system as well as QEMU/KVM as "legacy".

I was a afraid it was too broad, but its why I used "loosely".

Quoted 4 lines

Can we just enumerate this crap? I propose:

Xen PV and lguest are paravirt_legacy. Nothing else is paravirt_legacy.

Fine by me!

Quoted 2 lines

The addition of new paravirt_legacy support is
strongly discouraged.

Fine by me, but Boris is re-using that for HVMLite on his recently proposed patches. Do we want that ? I'll also note that the goal is to ensure its hardware_subarch will be 0 (PC), meanwhile old PV will be Xen (although this hasn't been set yet). This mess is part of the reason why I do think stronger semantics and clearer definitions will help here. Without strong semantics I can't help address the dead code concerns I've been rambling over.

Quoted 2 lines

HVMLite is just a rebranding for PVH done right, and then it seems PVH will be dropped and we'll have HVMLite rebranded back to PVH I guess it seems.

PVH/HVMlite had better not be "paravirt" in this sense, I hope.

Quoted 12 lines

The enumeration of legacy crap by ACPI boot flags seems to provide enough details to suit our needs if we really wanted to zero down on the specifics of what paravirt_legacy() means, there are these flags:

/* Masks for FADT IA-PC Boot Architecture Flags (boot_flags) [Vx]=Introduced in this FADT revision */

#define ACPI_FADT_LEGACY_DEVICES    (1)         /* 00: [V2] System has LPC or ISA bus devices */
#define ACPI_FADT_8042              (1<<1)      /* 01: [V3] System has an 8042 controller on port 60/64 */
#define ACPI_FADT_NO_VGA            (1<<2)      /* 02: [V4] It is not safe to probe for VGA hardware */
#define ACPI_FADT_NO_MSI            (1<<3)      /* 03: [V4] Message Signaled Interrupts (MSI) must not be enabled */
#define ACPI_FADT_NO_ASPM           (1<<4)      /* 04: [V4] PCIe ASPM control must not be enabled */
#define ACPI_FADT_NO_CMOS_RTC       (1<<5)      /* 05: [V5] No CMOS real-time clock present */

Quoted 3 lines

I checked and I didn't see qemu using any of the ACPI boot flags, but I suspected qemu instances must use a series of legacy crap. Likewise for KVM.

So shouldn't Linux find out things from the FADT by reading the FADT rather than squishing them into a confusing single function?

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?

        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?

        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.

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.

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

        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.

So yeah, I think the right solution is to delete paravirt_enabled.

The enumeration of legacy crap by ACPI boot flags seems to provide enough details to suit our needs if we really wanted to zero down on the specifics of what paravirt_legacy() means, there are these flags:

/* Masks for FADT IA-PC Boot Architecture Flags (boot_flags) [Vx]=Introduced in this FADT revision */

#define ACPI_FADT_LEGACY_DEVICES    (1)         /* 00: [V2] System has LPC or ISA bus devices */
#define ACPI_FADT_8042              (1<<1)      /* 01: [V3] System has an 8042 controller on port 60/64 */
#define ACPI_FADT_NO_VGA            (1<<2)      /* 02: [V4] It is not safe to probe for VGA hardware */
#define ACPI_FADT_NO_MSI            (1<<3)      /* 03: [V4] Message Signaled Interrupts (MSI) must not be enabled */
#define ACPI_FADT_NO_ASPM           (1<<4)      /* 04: [V4] PCIe ASPM control must not be enabled */
#define ACPI_FADT_NO_CMOS_RTC       (1<<5)      /* 05: [V5] No CMOS real-time clock present */

I checked and I didn't see qemu using any of the ACPI boot flags, but I suspected qemu instances must use a series of legacy crap. Likewise for KVM.

coreboot defines legacy free when you don't have any of the above flags set:
#define ACPI_FADT_LEGACY_FREE 0x00 /* No legacy devices (including 8042) *

Would it be sufficient to just stick to "pv legacy" equivalent of requiring just ACPI_FADT_LEGACY_DEVICES and ACPI_FADT_8042 ?

I should point out It turns out ACPI_FADT_NO_CMOS_RTC matches lguest's and it seems that's the only reason we have that RTC PV flag and the features pv field... with the linker table + x86 subarch use it should be relatively simple to remove paravirt_has_feature() paravirt_has() and PV_SUPPORTED_RTC. lguest would just be the only subarch that opts out.

Luis