* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-06 20:05 ` Andy Lutomirski
0 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-06 20:05 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Luis R. Rodriguez, cocci, Boris Ostrovsky, Juergen Gross, mcb30,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, Konrad Rzeszutek Wilk,
david.e.box
On Sat, Feb 6, 2016 at 12:59 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Feb 05, 2016 at 11:11:34PM -0800, Andy Lutomirski wrote:
>> On Feb 5, 2016 8:30 PM, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>> >
>> > 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.
>>
>> 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 :)
>
>> > +/**
>> > + * 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.
>>
>> 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".
>
>> Can we just enumerate this crap? I propose:
>>
>> Xen PV and lguest are paravirt_legacy. Nothing else is
>> paravirt_legacy.
>
> Fine by me!
>
>> 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.
PVH/HVMlite had better not be "paravirt" in this sense, I hope.
>
> 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.
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.
--Andy
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-06 20:05 ` Andy Lutomirski
0 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-06 20:05 UTC (permalink / raw)
To: cocci
On Sat, Feb 6, 2016 at 12:59 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Fri, Feb 05, 2016 at 11:11:34PM -0800, Andy Lutomirski wrote:
>> On Feb 5, 2016 8:30 PM, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>> >
>> > 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.
>>
>> 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 :)
>
>> > +/**
>> > + * 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.
>>
>> 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".
>
>> Can we just enumerate this crap? I propose:
>>
>> Xen PV and lguest are paravirt_legacy. Nothing else is
>> paravirt_legacy.
>
> Fine by me!
>
>> 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.
PVH/HVMlite had better not be "paravirt" in this sense, I hope.
>
> 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.
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.
--Andy
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-06 20:05 ` [Cocci] " Andy Lutomirski
(?)
@ 2016-02-06 22:04 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-06 22:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Boris Ostrovsky,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box,
X86 ML, Ingo Molnar
On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
> 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.
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-06 22:04 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-06 22:04 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Boris Ostrovsky,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.bo
On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
> 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.
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-06 22:04 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-06 22:04 UTC (permalink / raw)
To: cocci
On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
> 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.
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-06 22:04 ` [Cocci] " Borislav Petkov
(?)
@ 2016-02-08 15:39 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:39 UTC (permalink / raw)
To: Borislav Petkov, Andy Lutomirski
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Juergen Gross,
mcb30, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box, X86 ML,
Ingo Molnar
On 02/06/2016 05:04 PM, Borislav Petkov wrote:
> On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
>> 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.
>
>> 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.
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.
>
> - microcode application on Xen: we've had this before. The hypervisor
> should do that (if it doesn't do so already).
it does.
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:39 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:39 UTC (permalink / raw)
To: Borislav Petkov, Andy Lutomirski
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Juergen Gross,
mcb30, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box, X86 ML,
Ingo
On 02/06/2016 05:04 PM, Borislav Petkov wrote:
> On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
>> 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.
>
>> 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.
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.
>
> - microcode application on Xen: we've had this before. The hypervisor
> should do that (if it doesn't do so already).
it does.
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:39 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:39 UTC (permalink / raw)
To: cocci
On 02/06/2016 05:04 PM, Borislav Petkov wrote:
> On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
>> 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.
>
>> 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.
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.
>
> - microcode application on Xen: we've had this before. The hypervisor
> should do that (if it doesn't do so already).
it does.
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:39 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 15:55 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:55 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box,
X86 ML, Ingo Molnar
On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
> 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.
> >- microcode application on Xen: we've had this before. The hypervisor
> >should do that (if it doesn't do so already).
>
> it does.
Good.
> >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...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:55 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:55 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box
On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
> 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.
> >- microcode application on Xen: we've had this before. The hypervisor
> >should do that (if it doesn't do so already).
>
> it does.
Good.
> >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...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:55 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:55 UTC (permalink / raw)
To: cocci
On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
> 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.
> >- microcode application on Xen: we've had this before. The hypervisor
> >should do that (if it doesn't do so already).
>
> it does.
Good.
> >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...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:55 ` [Cocci] " Borislav Petkov
(?)
@ 2016-02-08 16:05 ` Andrew Cooper
-1 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:05 UTC (permalink / raw)
To: Borislav Petkov, Boris Ostrovsky
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 08/02/16 15:55, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
>> 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.
>
>>> - microcode application on Xen: we've had this before. The hypervisor
>>> should do that (if it doesn't do so already).
>> it does.
> Good.
>
>>> 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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:05 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:05 UTC (permalink / raw)
To: Borislav Petkov, Boris Ostrovsky
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fenggu
On 08/02/16 15:55, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
>> 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.
>
>>> - microcode application on Xen: we've had this before. The hypervisor
>>> should do that (if it doesn't do so already).
>> it does.
> Good.
>
>>> 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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:05 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:05 UTC (permalink / raw)
To: cocci
On 08/02/16 15:55, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 10:39:43AM -0500, Boris Ostrovsky wrote:
>> 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.
>
>>> - microcode application on Xen: we've had this before. The hypervisor
>>> should do that (if it doesn't do so already).
>> it does.
> Good.
>
>>> 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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:05 ` [Cocci] " Andrew Cooper
(?)
@ 2016-02-08 16:12 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:12 UTC (permalink / raw)
To: Andrew Cooper, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:12 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:12 UTC (permalink / raw)
To: Andrew Cooper, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:12 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:12 UTC (permalink / raw)
To: cocci
On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>
> 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).
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:12 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 16:26 ` Andrew Cooper
-1 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:26 UTC (permalink / raw)
To: Boris Ostrovsky, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 08/02/16 16:12, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>
>> 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).
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:26 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:26 UTC (permalink / raw)
To: Boris Ostrovsky, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fenggu
On 08/02/16 16:12, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>
>> 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).
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:26 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:26 UTC (permalink / raw)
To: cocci
On 08/02/16 16:12, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>
>> 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).
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:26 ` [Cocci] " Andrew Cooper
(?)
@ 2016-02-08 16:31 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:31 UTC (permalink / raw)
To: Andrew Cooper, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:26 AM, Andrew Cooper wrote:
> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>
>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>> 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).
> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:31 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:31 UTC (permalink / raw)
To: Andrew Cooper, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:26 AM, Andrew Cooper wrote:
> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>
>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>> 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).
> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:31 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:31 UTC (permalink / raw)
To: cocci
On 02/08/2016 11:26 AM, Andrew Cooper wrote:
> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>
>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>> 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).
> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:31 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 16:32 ` Andrew Cooper
-1 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:32 UTC (permalink / raw)
To: Boris Ostrovsky, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 08/02/16 16:31, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:26 AM, Andrew Cooper wrote:
>> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>>
>>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>>> 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).
>> 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().
Great.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:32 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:32 UTC (permalink / raw)
To: Boris Ostrovsky, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fenggu
On 08/02/16 16:31, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:26 AM, Andrew Cooper wrote:
>> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>>
>>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>>> 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).
>> 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().
Great.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:32 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:32 UTC (permalink / raw)
To: cocci
On 08/02/16 16:31, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:26 AM, Andrew Cooper wrote:
>> On 08/02/16 16:12, Boris Ostrovsky wrote:
>>>
>>> On 02/08/2016 11:05 AM, Andrew Cooper wrote:
>>>> 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).
>> 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().
Great.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:31 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 16:35 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:35 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
> 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?
---
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 055ea9941dd5..f3e563e8b5c3 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -56,9 +56,11 @@ extern const struct hypervisor_x86 x86_hyper_kvm;
extern void init_hypervisor(struct cpuinfo_x86 *c);
extern void init_hypervisor_platform(void);
extern bool hypervisor_x2apic_available(void);
+bool is_xen_hypervisor(void);
#else
static inline void init_hypervisor(struct cpuinfo_x86 *c) { }
static inline void init_hypervisor_platform(void) { }
static inline bool hypervisor_x2apic_available(void) { return false; }
+static inline bool is_xen_hypervisor(void) { return false; }
#endif /* CONFIG_HYPERVISOR_GUEST */
#endif /* _ASM_X86_HYPERVISOR_H */
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index d820d8eae96b..bda29017f946 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -85,3 +85,24 @@ bool __init hypervisor_x2apic_available(void)
x86_hyper->x2apic_available &&
x86_hyper->x2apic_available();
}
+
+bool is_xen_hypervisor(void)
+{
+ u32 eax, ebx, ecx, edx;
+
+ eax = 0x4000000;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ eax = 0x40000100;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ return false;
+}
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index cea8552e2b3a..0a941ff8095c 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -35,6 +35,7 @@
#include <asm/microcode_intel.h>
#include <asm/cpu_device_id.h>
#include <asm/microcode_amd.h>
+#include <asm/hypervisor.h>
#include <asm/perf_event.h>
#include <asm/microcode.h>
#include <asm/processor.h>
@@ -86,7 +87,7 @@ static bool __init check_loader_disabled_bsp(void)
bool *res = &dis_ucode_ldr;
#endif
- if (cmdline_find_option_bool(cmdline, option))
+ if (cmdline_find_option_bool(cmdline, option) || is_xen_hypervisor())
*res = true;
return *res;
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply related [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:35 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:35 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wan
On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
> 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?
---
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 055ea9941dd5..f3e563e8b5c3 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -56,9 +56,11 @@ extern const struct hypervisor_x86 x86_hyper_kvm;
extern void init_hypervisor(struct cpuinfo_x86 *c);
extern void init_hypervisor_platform(void);
extern bool hypervisor_x2apic_available(void);
+bool is_xen_hypervisor(void);
#else
static inline void init_hypervisor(struct cpuinfo_x86 *c) { }
static inline void init_hypervisor_platform(void) { }
static inline bool hypervisor_x2apic_available(void) { return false; }
+static inline bool is_xen_hypervisor(void) { return false; }
#endif /* CONFIG_HYPERVISOR_GUEST */
#endif /* _ASM_X86_HYPERVISOR_H */
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index d820d8eae96b..bda29017f946 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -85,3 +85,24 @@ bool __init hypervisor_x2apic_available(void)
x86_hyper->x2apic_available &&
x86_hyper->x2apic_available();
}
+
+bool is_xen_hypervisor(void)
+{
+ u32 eax, ebx, ecx, edx;
+
+ eax = 0x4000000;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ eax = 0x40000100;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ return false;
+}
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index cea8552e2b3a..0a941ff8095c 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -35,6 +35,7 @@
#include <asm/microcode_intel.h>
#include <asm/cpu_device_id.h>
#include <asm/microcode_amd.h>
+#include <asm/hypervisor.h>
#include <asm/perf_event.h>
#include <asm/microcode.h>
#include <asm/processor.h>
@@ -86,7 +87,7 @@ static bool __init check_loader_disabled_bsp(void)
bool *res = &dis_ucode_ldr;
#endif
- if (cmdline_find_option_bool(cmdline, option))
+ if (cmdline_find_option_bool(cmdline, option) || is_xen_hypervisor())
*res = true;
return *res;
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply related [flat|nested] 104+ messages in thread* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:35 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:35 UTC (permalink / raw)
To: cocci
On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
> 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?
---
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 055ea9941dd5..f3e563e8b5c3 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -56,9 +56,11 @@ extern const struct hypervisor_x86 x86_hyper_kvm;
extern void init_hypervisor(struct cpuinfo_x86 *c);
extern void init_hypervisor_platform(void);
extern bool hypervisor_x2apic_available(void);
+bool is_xen_hypervisor(void);
#else
static inline void init_hypervisor(struct cpuinfo_x86 *c) { }
static inline void init_hypervisor_platform(void) { }
static inline bool hypervisor_x2apic_available(void) { return false; }
+static inline bool is_xen_hypervisor(void) { return false; }
#endif /* CONFIG_HYPERVISOR_GUEST */
#endif /* _ASM_X86_HYPERVISOR_H */
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index d820d8eae96b..bda29017f946 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -85,3 +85,24 @@ bool __init hypervisor_x2apic_available(void)
x86_hyper->x2apic_available &&
x86_hyper->x2apic_available();
}
+
+bool is_xen_hypervisor(void)
+{
+ u32 eax, ebx, ecx, edx;
+
+ eax = 0x4000000;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ eax = 0x40000100;
+ ecx = 0;
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ if (ebx == 0x566e6558 && ecx == 0x65584d4d && edx == 0x4d4d566e)
+ return true;
+
+ return false;
+}
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index cea8552e2b3a..0a941ff8095c 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -35,6 +35,7 @@
#include <asm/microcode_intel.h>
#include <asm/cpu_device_id.h>
#include <asm/microcode_amd.h>
+#include <asm/hypervisor.h>
#include <asm/perf_event.h>
#include <asm/microcode.h>
#include <asm/processor.h>
@@ -86,7 +87,7 @@ static bool __init check_loader_disabled_bsp(void)
bool *res = &dis_ucode_ldr;
#endif
- if (cmdline_find_option_bool(cmdline, option))
+ if (cmdline_find_option_bool(cmdline, option) || is_xen_hypervisor())
*res = true;
return *res;
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply related [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:35 ` [Cocci] " Borislav Petkov
(?)
@ 2016-02-08 16:38 ` Andrew Cooper
-1 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:38 UTC (permalink / raw)
To: Borislav Petkov, Boris Ostrovsky
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 08/02/16 16:35, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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?
Correct. PV guests are ring-deprivilelged so the cpuid instruction
doesn't trap in general. (It does on more modern Intel hardware with
cpuid faulting enabled, but that is only IvyBridge and newer).
Does the early loader have extable support? If so, this is fairly easy
to fix. If not, we have a problem.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:38 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:38 UTC (permalink / raw)
To: Borislav Petkov, Boris Ostrovsky
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguan
On 08/02/16 16:35, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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?
Correct. PV guests are ring-deprivilelged so the cpuid instruction
doesn't trap in general. (It does on more modern Intel hardware with
cpuid faulting enabled, but that is only IvyBridge and newer).
Does the early loader have extable support? If so, this is fairly easy
to fix. If not, we have a problem.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:38 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:38 UTC (permalink / raw)
To: cocci
On 08/02/16 16:35, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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?
Correct. PV guests are ring-deprivilelged so the cpuid instruction
doesn't trap in general. (It does on more modern Intel hardware with
cpuid faulting enabled, but that is only IvyBridge and newer).
Does the early loader have extable support? If so, this is fairly easy
to fix. If not, we have a problem.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:38 ` [Cocci] " Andrew Cooper
(?)
@ 2016-02-08 16:45 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:45 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:45 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:45 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long
On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:45 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:45 UTC (permalink / raw)
To: cocci
On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
> 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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:45 ` [Cocci] " Borislav Petkov
@ 2016-02-08 16:52 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:52 UTC (permalink / raw)
To: Borislav Petkov, Andrew Cooper
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:45 AM, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>> 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.
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.
-boris
>
> Later code can use then xen_cpuid() and all is fine. We should still get
> rid of paravirt_enabled() though.
>
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:52 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:52 UTC (permalink / raw)
To: Borislav Petkov, Andrew Cooper
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:45 AM, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>> 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.
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.
-boris
>
> Later code can use then xen_cpuid() and all is fine. We should still get
> rid of paravirt_enabled() though.
>
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:52 ` Boris Ostrovsky
@ 2016-02-08 20:45 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 20:45 UTC (permalink / raw)
To: Borislav Petkov, Andrew Cooper
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:52 AM, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:45 AM, Borislav Petkov wrote:
>> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>>> 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.
>
> 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.
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 20:45 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 20:45 UTC (permalink / raw)
To: Borislav Petkov, Andrew Cooper
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, cocci, Xen Devel, Joerg Roedel,
Rafael J. Wysocki, Robert Moore, Ingo Molnar, Andrey Ryabinin,
Mauro Carvalho Chehab, Luis R. Rodriguez, Rusty Russell,
Thomas Gleixner, mcb30, Juergen Gross, linux-kernel,
Andy Lutomirski, Luis R. Rodriguez, long.wanglong, Fengguang Wu
On 02/08/2016 11:52 AM, Boris Ostrovsky wrote:
>
>
> On 02/08/2016 11:45 AM, Borislav Petkov wrote:
>> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>>> 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.
>
> 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.
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 20:45 ` Boris Ostrovsky
@ 2016-02-08 21:06 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 21:06 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 03:45:21PM -0500, Boris Ostrovsky wrote:
> 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.
Even better.
Care to cut a proper patch explaining why we don't need the
paravirt_enabled() check anymore, and prep it ontop of
http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=tip-microcode
?
I'll queue it for 4.6.
Thanks!
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 21:06 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 21:06 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wan
On Mon, Feb 08, 2016 at 03:45:21PM -0500, Boris Ostrovsky wrote:
> 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.
Even better.
Care to cut a proper patch explaining why we don't need the
paravirt_enabled() check anymore, and prep it ontop of
http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git/log/?h=tip-microcode
?
I'll queue it for 4.6.
Thanks!
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:45 ` [Cocci] " Borislav Petkov
(?)
@ 2016-02-08 16:53 ` Andrew Cooper
-1 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On 08/02/16 16:45, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>> 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.
>
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:53 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:53 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez
On 08/02/16 16:45, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>> 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.
>
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:53 ` Andrew Cooper
0 siblings, 0 replies; 104+ messages in thread
From: Andrew Cooper @ 2016-02-08 16:53 UTC (permalink / raw)
To: cocci
On 08/02/16 16:45, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
>> 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.
>
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.
~Andrew
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:53 ` [Cocci] " Andrew Cooper
@ 2016-02-08 17:13 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 17:13 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 04:53:00PM +0000, Andrew Cooper wrote:
> 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.
Good to know, we might need it for something. :)
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 17:13 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 17:13 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long
On Mon, Feb 08, 2016 at 04:53:00PM +0000, Andrew Cooper wrote:
> 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.
Good to know, we might need it for something. :)
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:53 ` [Cocci] " Andrew Cooper
@ 2016-02-09 6:22 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:22 UTC (permalink / raw)
To: Andrew Cooper
Cc: Borislav Petkov, Boris Ostrovsky, X86 ML, david.e.box,
Andrey Ryabinin, Lv Zheng, Jan Beulich, H. Peter Anvin, qiuxishi,
cocci, Xen Devel, Joerg Roedel, Rafael J. Wysocki, Robert Moore,
Ingo Molnar, Andrey Ryabinin, Mauro Carvalho Chehab,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 04:53:00PM +0000, Andrew Cooper wrote:
> On 08/02/16 16:45, Borislav Petkov wrote:
> > On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
> >> 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.
> >
>
> 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.
Most of the paravirt_enabled() checks can be replaced with a hardware_subarch
check once we modify the xen_start_kernel() to add that, today XEN is unused
even though it was added eons ago. Part of my work was to remove as many
paravirt_enabled() checks. I'll reply to Andy's original e-mail now indicating
which ones I could address, it sounds like with this nugget and some other
work we might be able to address all.
I should note, in the future the check for subarch would be an explicit part of
the x86 early init init routines to avoid further issues but only in between
x86_64_start_reservations() and setup_arch(). How *early* can such a hypercall
_page check be *safely* be called? I say safely here as if we're not on Xen are
we OK to muck around and check the same address space?
Provided we use the subarch for PV to remove a lot of the paravirt_enabled()
checks, is HVMLite still OK if it ends up using PC subarch and there not being
a paravirt_enabled() anyamore? Boris O's HVMLite series added paravirt_enabled
= 1 for the new HVMLite.
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-09 6:22 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:22 UTC (permalink / raw)
To: Andrew Cooper
Cc: Borislav Petkov, Boris Ostrovsky, X86 ML, david.e.box,
Andrey Ryabinin, Lv Zheng, Jan Beulich, H. Peter Anvin, qiuxishi,
cocci, Xen Devel, Joerg Roedel, Rafael J. Wysocki, Robert Moore,
Ingo Molnar, Andrey Ryabinin, Mauro Carvalho Chehab,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglo
On Mon, Feb 08, 2016 at 04:53:00PM +0000, Andrew Cooper wrote:
> On 08/02/16 16:45, Borislav Petkov wrote:
> > On Mon, Feb 08, 2016 at 04:38:40PM +0000, Andrew Cooper wrote:
> >> 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.
> >
>
> 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.
Most of the paravirt_enabled() checks can be replaced with a hardware_subarch
check once we modify the xen_start_kernel() to add that, today XEN is unused
even though it was added eons ago. Part of my work was to remove as many
paravirt_enabled() checks. I'll reply to Andy's original e-mail now indicating
which ones I could address, it sounds like with this nugget and some other
work we might be able to address all.
I should note, in the future the check for subarch would be an explicit part of
the x86 early init init routines to avoid further issues but only in between
x86_64_start_reservations() and setup_arch(). How *early* can such a hypercall
_page check be *safely* be called? I say safely here as if we're not on Xen are
we OK to muck around and check the same address space?
Provided we use the subarch for PV to remove a lot of the paravirt_enabled()
checks, is HVMLite still OK if it ends up using PC subarch and there not being
a paravirt_enabled() anyamore? Boris O's HVMLite series added paravirt_enabled
= 1 for the new HVMLite.
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:35 ` [Cocci] " Borislav Petkov
(?)
@ 2016-02-08 16:41 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:41 UTC (permalink / raw)
To: Borislav Petkov
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On 02/08/2016 11:35 AM, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:41 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:41 UTC (permalink / raw)
To: Borislav Petkov
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wan
On 02/08/2016 11:35 AM, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:41 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 16:41 UTC (permalink / raw)
To: cocci
On 02/08/2016 11:35 AM, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 11:31:04AM -0500, Boris Ostrovsky wrote:
>> 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().
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 16:41 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 16:52 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:52 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wanglong,
Fengguang Wu
On Mon, Feb 08, 2016 at 11:41:16AM -0500, Boris Ostrovsky wrote:
> 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().
Ah, so one more reason not to do the CPUID thing for the early loader.
So it's up to you how to replace that paravirt_enabled() thing - CPUID
or test static_cpu_has(X86_FEATURE_XENPV)...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:52 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:52 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andrew Cooper, X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng,
Jan Beulich, H. Peter Anvin, qiuxishi, cocci, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Mauro Carvalho Chehab, Luis R. Rodriguez,
Rusty Russell, Thomas Gleixner, mcb30, Juergen Gross,
linux-kernel, Andy Lutomirski, Luis R. Rodriguez, long.wan
On Mon, Feb 08, 2016 at 11:41:16AM -0500, Boris Ostrovsky wrote:
> 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().
Ah, so one more reason not to do the CPUID thing for the early loader.
So it's up to you how to replace that paravirt_enabled() thing - CPUID
or test static_cpu_has(X86_FEATURE_XENPV)...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [Xen-devel] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 16:52 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 16:52 UTC (permalink / raw)
To: cocci
On Mon, Feb 08, 2016 at 11:41:16AM -0500, Boris Ostrovsky wrote:
> 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().
Ah, so one more reason not to do the CPUID thing for the early loader.
So it's up to you how to replace that paravirt_enabled() thing - CPUID
or test static_cpu_has(X86_FEATURE_XENPV)...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-06 20:05 ` [Cocci] " Andy Lutomirski
(?)
@ 2016-02-08 15:31 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:31 UTC (permalink / raw)
To: Andy Lutomirski, Luis R. Rodriguez
Cc: Luis R. Rodriguez, cocci, Juergen Gross, mcb30, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, Konrad Rzeszutek Wilk,
david.e.box, X86 ML, Ingo Molnar
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.
-boris
>
> So yeah, I think the right solution is to delete paravirt_enabled.
>
> --Andy
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:31 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:31 UTC (permalink / raw)
To: Andy Lutomirski, Luis R. Rodriguez
Cc: Luis R. Rodriguez, cocci, Juergen Gross, mcb30, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, Konrad Rzeszutek Wilk,
david.e.box, X86 ML, Ingo Molnar
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.
-boris
>
> So yeah, I think the right solution is to delete paravirt_enabled.
>
> --Andy
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:31 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-08 15:31 UTC (permalink / raw)
To: cocci
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.
-boris
>
> So yeah, I think the right solution is to delete paravirt_enabled.
>
> --Andy
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:31 ` [Cocci] " Boris Ostrovsky
(?)
@ 2016-02-08 15:46 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:46 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box,
X86 ML, Ingo Molnar
On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky wrote:
> 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.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:46 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:46 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box
On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky wrote:
> 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.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* [Cocci] [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 15:46 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-08 15:46 UTC (permalink / raw)
To: cocci
On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky wrote:
> 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.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:46 ` [Cocci] " Borislav Petkov
@ 2016-02-09 6:59 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:59 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Andy Lutomirski, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.box,
X86 ML, Ingo Molnar
On Mon, Feb 08, 2016 at 04:46:08PM +0100, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky wrote:
> > 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.
It doesn't sound like we have a replacement strategy for this yet?
If not, for PV we can rely on the subarch, but only once its set,
I can take this on as part of my work if agreeable given I'm adding
the use of the Xen subarch. If we want this in before linker table
stuff gets fleshed out this I can just split out setting the subarch
on xen_start_kernel() and replacing paravirt_enabled() where
applicable. But -- what about HVMLite for this, given as-is currently
HVMLite is to use PC subarch?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-09 6:59 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:59 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Andy Lutomirski, Luis R. Rodriguez, cocci,
Juergen Gross, mcb30, Thomas Gleixner, Andrey Ryabinin,
Joerg Roedel, Robert Moore, Mauro Carvalho Chehab,
Rafael J. Wysocki, Xen Devel, H. Peter Anvin, Rusty Russell,
Jan Beulich, Lv Zheng, linux-kernel, long.wanglong, Fengguang Wu,
qiuxishi, Andrey Ryabinin, Konrad Rzeszutek Wilk, david.e.bo
On Mon, Feb 08, 2016 at 04:46:08PM +0100, Borislav Petkov wrote:
> On Mon, Feb 08, 2016 at 10:31:36AM -0500, Boris Ostrovsky wrote:
> > 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.
It doesn't sound like we have a replacement strategy for this yet?
If not, for PV we can rely on the subarch, but only once its set,
I can take this on as part of my work if agreeable given I'm adding
the use of the Xen subarch. If we want this in before linker table
stuff gets fleshed out this I can just split out setting the subarch
on xen_start_kernel() and replacing paravirt_enabled() where
applicable. But -- what about HVMLite for this, given as-is currently
HVMLite is to use PC subarch?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:31 ` [Cocci] " Boris Ostrovsky
@ 2016-02-08 21:04 ` Andy Lutomirski
-1 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-08 21:04 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Juergen Gross,
mcb30, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, Konrad Rzeszutek Wilk,
david.e.box, X86 ML, Ingo Molnar
On Mon, Feb 8, 2016 at 7:31 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.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@amacapital.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
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-08 21:04 ` Andy Lutomirski
0 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-08 21:04 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Luis R. Rodriguez, Luis R. Rodriguez, cocci, Juergen Gross,
mcb30, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, Konrad Rzeszutek Wilk,
david.e.box
On Mon, Feb 8, 2016 at 7:31 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.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@amacapital.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
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-08 15:31 ` [Cocci] " Boris Ostrovsky
@ 2016-02-09 7:06 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 7:06 UTC (permalink / raw)
To: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper
Cc: Andy Lutomirski, Luis R. Rodriguez, cocci, Juergen Gross, mcb30,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, david.e.box, X86 ML,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-09 7:06 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 7:06 UTC (permalink / raw)
To: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper
Cc: Andy Lutomirski, Luis R. Rodriguez, cocci, Juergen Gross, mcb30,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, david.e.box, X86 ML,
Ingo Molnar
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
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-09 7:06 ` Luis R. Rodriguez
(?)
@ 2016-02-17 20:07 ` Luis R. Rodriguez
2016-02-17 20:49 ` Borislav Petkov
-1 siblings, 1 reply; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-17 20:07 UTC (permalink / raw)
To: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Borislav Petkov
Cc: Luis R. Rodriguez, Juergen Gross, Michael Brown, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
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? So to be
clear we should not merge more users of paravirt_enabled.
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 20:07 ` Luis R. Rodriguez
@ 2016-02-17 20:49 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 20:49 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
> 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?
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.
---
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index 4a6f1d9b5106..de1ee3a40250 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -358,20 +358,20 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
#define pgd_none(a) pud_none(__pud(pgd_val(a)))
#endif
-#ifdef CONFIG_X86_64
static inline bool is_hypervisor_range(int idx)
{
+#ifdef CONFIG_X86_64
+
/*
* ffff800000000000 - ffff87ffffffffff is reserved for
* the hypervisor.
*/
- return paravirt_enabled() &&
- (idx >= pgd_index(__PAGE_OFFSET) - 16) &&
+ return (idx >= pgd_index(__PAGE_OFFSET) - 16) &&
(idx < pgd_index(__PAGE_OFFSET));
-}
#else
-static inline bool is_hypervisor_range(int idx) { return false; }
+ return false;
#endif
+}
static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
bool checkwx)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply related [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 20:49 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 20:49 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi
On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
> 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?
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.
---
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index 4a6f1d9b5106..de1ee3a40250 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -358,20 +358,20 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
#define pgd_none(a) pud_none(__pud(pgd_val(a)))
#endif
-#ifdef CONFIG_X86_64
static inline bool is_hypervisor_range(int idx)
{
+#ifdef CONFIG_X86_64
+
/*
* ffff800000000000 - ffff87ffffffffff is reserved for
* the hypervisor.
*/
- return paravirt_enabled() &&
- (idx >= pgd_index(__PAGE_OFFSET) - 16) &&
+ return (idx >= pgd_index(__PAGE_OFFSET) - 16) &&
(idx < pgd_index(__PAGE_OFFSET));
-}
#else
-static inline bool is_hypervisor_range(int idx) { return false; }
+ return false;
#endif
+}
static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
bool checkwx)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply related [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 20:49 ` Borislav Petkov
@ 2016-02-17 21:12 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-17 21:12 UTC (permalink / raw)
To: Borislav Petkov, Fengguang Wu
Cc: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, qiuxishi, Andrey Ryabinin,
david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 12:49 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
>> 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?
>
> 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.
Provided someone on the xen side acks, then great! We'd have full
coverage to remove all uses soon and kill paravirt_enabled() for good.
It may take some time to run tests of this to get a full sense of
correctness but perhaps in the future it may be easier if 0-day gets
some basic Xen tests (or embraces the Xen test suite) as was discussed
as possible a while ago. lguest may need some basic tests too, but I'm
not even sure what type of tests we'd run against lguest.
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 21:12 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-17 21:12 UTC (permalink / raw)
To: Borislav Petkov, Fengguang Wu
Cc: Boris Ostrovsky, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, qiuxishi, Andrey Ryabinin,
david.e.box
On Wed, Feb 17, 2016 at 12:49 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
>> 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?
>
> 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.
Provided someone on the xen side acks, then great! We'd have full
coverage to remove all uses soon and kill paravirt_enabled() for good.
It may take some time to run tests of this to get a full sense of
correctness but perhaps in the future it may be easier if 0-day gets
some basic Xen tests (or embraces the Xen test suite) as was discussed
as possible a while ago. lguest may need some basic tests too, but I'm
not even sure what type of tests we'd run against lguest.
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 20:49 ` Borislav Petkov
@ 2016-02-17 21:21 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 21:21 UTC (permalink / raw)
To: Borislav Petkov, Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Andrew Cooper, David Vrabel,
Andy Lutomirski, Juergen Gross, Michael Brown, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On 02/17/2016 03:49 PM, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
>> 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?
>
> Why not?
>
> I think we should simply check the range as ffff800000000000 -
> ffff87ffffffffff is practically an ABI and nothing should be mapped
^^^^^^^^^
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)
-boris
> there anyway. No need for paravirt_enabled() there either.
>
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 21:21 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 21:21 UTC (permalink / raw)
To: Borislav Petkov, Luis R. Rodriguez
Cc: Konrad Rzeszutek Wilk, Andrew Cooper, David Vrabel,
Andy Lutomirski, Juergen Gross, Michael Brown, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box
On 02/17/2016 03:49 PM, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 12:07:13PM -0800, Luis R. Rodriguez wrote:
>> 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?
>
> Why not?
>
> I think we should simply check the range as ffff800000000000 -
> ffff87ffffffffff is practically an ABI and nothing should be mapped
^^^^^^^^^
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)
-boris
> there anyway. No need for paravirt_enabled() there either.
>
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 21:21 ` Boris Ostrovsky
@ 2016-02-17 22:03 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 22:03 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Luis R. Rodriguez, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
> 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?
> 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?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 22:03 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 22:03 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Luis R. Rodriguez, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi
On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
> 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?
> 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?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 22:03 ` Borislav Petkov
@ 2016-02-17 22:18 ` Andy Lutomirski
-1 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-17 22:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 2:03 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>> 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?
>
>> 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?
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?
--Andy
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 22:18 ` Andy Lutomirski
0 siblings, 0 replies; 104+ messages in thread
From: Andy Lutomirski @ 2016-02-17 22:18 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi
On Wed, Feb 17, 2016 at 2:03 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>> 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?
>
>> 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?
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?
--Andy
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 22:18 ` Andy Lutomirski
@ 2016-02-17 22:39 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 22:39 UTC (permalink / raw)
To: Andy Lutomirski, Borislav Petkov
Cc: Luis R. Rodriguez, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Juergen Gross, Michael Brown, Thomas Gleixner,
Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On 02/17/2016 05:18 PM, Andy Lutomirski wrote:
> On Wed, Feb 17, 2016 at 2:03 PM, Borislav Petkov <bp@alien8.de> wrote:
>> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>>> 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?
>>
>>> 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?
> 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.
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 22:39 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 22:39 UTC (permalink / raw)
To: Andy Lutomirski, Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, Mauro Carvalho Chehab, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Rusty Russell, Thomas Gleixner, Michael Brown,
Juergen Gross, Andrew Cooper, linux-kernel, Luis R. Rodriguez,
David Vrabel, long.wanglong, Fengguang Wu
On 02/17/2016 05:18 PM, Andy Lutomirski wrote:
> On Wed, Feb 17, 2016 at 2:03 PM, Borislav Petkov <bp@alien8.de> wrote:
>> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>>> 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?
>>
>>> 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?
> 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.
-boris
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 22:39 ` Boris Ostrovsky
@ 2016-02-17 23:39 ` Borislav Petkov
-1 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 23:39 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 05:39:23PM -0500, Boris Ostrovsky wrote:
> Hmm. I think you are right --- I was following wrong branch of the 'if'
> statement. We are always going straight to note_page().
Yap. The is_hypervisor_range() - without the "!" - does note_page().
> Then yes, we should be able to remove paravirt_enabled(). Sorry for
> the noise.
I'll do a formal patch tomorrow.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 23:39 ` Borislav Petkov
0 siblings, 0 replies; 104+ messages in thread
From: Borislav Petkov @ 2016-02-17 23:39 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Andy Lutomirski, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi
On Wed, Feb 17, 2016 at 05:39:23PM -0500, Boris Ostrovsky wrote:
> Hmm. I think you are right --- I was following wrong branch of the 'if'
> statement. We are always going straight to note_page().
Yap. The is_hypervisor_range() - without the "!" - does note_page().
> Then yes, we should be able to remove paravirt_enabled(). Sorry for
> the noise.
I'll do a formal patch tomorrow.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 22:03 ` Borislav Petkov
@ 2016-02-17 22:19 ` Boris Ostrovsky
-1 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 22:19 UTC (permalink / raw)
To: Borislav Petkov
Cc: Luis R. Rodriguez, Konrad Rzeszutek Wilk, Andrew Cooper,
David Vrabel, Andy Lutomirski, Juergen Gross, Michael Brown,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On 02/17/2016 05:03 PM, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>> 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?
The latter. This is to detect things that clearly shouldn't be happening.
>
>> 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...)
Can't use CPUID 0x40000000 because it will return hypervisor (Xen or
otherwise) for non-PV guests as well. In Xen's case, you can't determine
guest type from hypervisor leaves.
> or something else.
We could say xen_pv_domain(). But this means using Xen-specific code in
x86-generic file to detect things specified by ABI. I don't know if I'd
like that.
-boris
>
> What's your preference?
>
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 22:19 ` Boris Ostrovsky
0 siblings, 0 replies; 104+ messages in thread
From: Boris Ostrovsky @ 2016-02-17 22:19 UTC (permalink / raw)
To: Borislav Petkov
Cc: X86 ML, david.e.box, Andrey Ryabinin, Lv Zheng, Jan Beulich,
H. Peter Anvin, qiuxishi, Mauro Carvalho Chehab, Xen Devel,
Joerg Roedel, Rafael J. Wysocki, Robert Moore, Ingo Molnar,
Andrey Ryabinin, Rusty Russell, Thomas Gleixner, Michael Brown,
Juergen Gross, Andrew Cooper, linux-kernel, Andy Lutomirski,
Luis R. Rodriguez, David Vrabel, long.wanglong
On 02/17/2016 05:03 PM, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
>> 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?
The latter. This is to detect things that clearly shouldn't be happening.
>
>> 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...)
Can't use CPUID 0x40000000 because it will return hypervisor (Xen or
otherwise) for non-PV guests as well. In Xen's case, you can't determine
guest type from hypervisor leaves.
> or something else.
We could say xen_pv_domain(). But this means using Xen-specific code in
x86-generic file to detect things specified by ABI. I don't know if I'd
like that.
-boris
>
> What's your preference?
>
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-17 22:03 ` Borislav Petkov
@ 2016-02-17 22:35 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-17 22:35 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Andy Lutomirski, Juergen Gross,
Michael Brown, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Andrey Ryabinin, david.e.box, X86 ML, Ingo Molnar
On Wed, Feb 17, 2016 at 11:03:04PM +0100, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
> > 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?
>
> > 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?
I can think of two possibilities but lets also address _why_ we can't
replace it: the semantic gap.
1) Can't we just set_memory_np() to cause a page fault on that range
to catch invalid access?
2) Add hypervisor type
I've been lobbying for a new boot protocol hypervisor type and hypervisor data
pointer extensions, much in the same way subarch and subarch_data was added for
x86 boot protocol 2.07. This would be an extension to help fill in the gaps,
it would also make it accessible to super early code and could help those of
us who care about super clean inits to work towards these goals. Right now we
can rely on the subarch for most Xen concerns but with Xen HVM and future Xen
HVMLite things get more complex and as noted by hpa the subarch was not
designed as a 'hypervisor type', such a thing should be considered separateley.
So we'd knock about 2-3 birds with 1 stone here:
a) there is a semantic gap between early access to hypervisor type of
code between asm boot and when setup_arch() is called, only after
init_hypervisor_platform() is called (in setup_arch()) can things
such as cpu_has_hypervisor() or derivatives such as kvm_para_available()
be correctly used, as only then will that information be correct across
the board. Part of this issue is what gave rise to paravirt_enabled()
hackeries in the first place. We have subarch but that is not to be used
to set things such as hypervisor type, I'll soon post a patch to clarify
the exact use case for the subarch.
Since we have no uniform way to detect hypervisor types we are starting
to see custom hacks. For instance of a hack propagated to drivers now, see
sound/pci/intel8x0.c use of kvm_para_available().
b) help put an end to paravirt_enabled() for cases we can't replace
c) provide an early access mechanism to hypervisor type. This should
help towards unifying inits by enabling further stubs on early and
post routine calls. This is future long term possible work:
With a hypervisor type and hypervisor custom data pointer, we'd strive
to work to make xen use the same startup_32() and startup_64() entries,
with stubs possible using the hypervisor type at the start /end as
follows:
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 of
the newly proposed hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64()
would have the 64-bits.
I realize Andrew has not been a fan over the idea of Xen setting on the
zero page *any* parameter but he's also noted an alternative is to just
lobby for Grub to boot Xen kernels and then we can rely on Grub to
set it for Linux. The only issue with this is it seems this doesn't
address stubdomains which don't use grub?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-17 22:35 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-17 22:35 UTC (permalink / raw)
To: Borislav Petkov
Cc: Boris Ostrovsky, Luis R. Rodriguez, Konrad Rzeszutek Wilk,
Andrew Cooper, David Vrabel, Andy Lutomirski, Juergen Gross,
Michael Brown, Thomas Gleixner, Andrey Ryabinin, Joerg Roedel,
Robert Moore, Mauro Carvalho Chehab, Rafael J. Wysocki,
Xen Devel, H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu
On Wed, Feb 17, 2016 at 11:03:04PM +0100, Borislav Petkov wrote:
> On Wed, Feb 17, 2016 at 04:21:56PM -0500, Boris Ostrovsky wrote:
> > 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?
>
> > 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?
I can think of two possibilities but lets also address _why_ we can't
replace it: the semantic gap.
1) Can't we just set_memory_np() to cause a page fault on that range
to catch invalid access?
2) Add hypervisor type
I've been lobbying for a new boot protocol hypervisor type and hypervisor data
pointer extensions, much in the same way subarch and subarch_data was added for
x86 boot protocol 2.07. This would be an extension to help fill in the gaps,
it would also make it accessible to super early code and could help those of
us who care about super clean inits to work towards these goals. Right now we
can rely on the subarch for most Xen concerns but with Xen HVM and future Xen
HVMLite things get more complex and as noted by hpa the subarch was not
designed as a 'hypervisor type', such a thing should be considered separateley.
So we'd knock about 2-3 birds with 1 stone here:
a) there is a semantic gap between early access to hypervisor type of
code between asm boot and when setup_arch() is called, only after
init_hypervisor_platform() is called (in setup_arch()) can things
such as cpu_has_hypervisor() or derivatives such as kvm_para_available()
be correctly used, as only then will that information be correct across
the board. Part of this issue is what gave rise to paravirt_enabled()
hackeries in the first place. We have subarch but that is not to be used
to set things such as hypervisor type, I'll soon post a patch to clarify
the exact use case for the subarch.
Since we have no uniform way to detect hypervisor types we are starting
to see custom hacks. For instance of a hack propagated to drivers now, see
sound/pci/intel8x0.c use of kvm_para_available().
b) help put an end to paravirt_enabled() for cases we can't replace
c) provide an early access mechanism to hypervisor type. This should
help towards unifying inits by enabling further stubs on early and
post routine calls. This is future long term possible work:
With a hypervisor type and hypervisor custom data pointer, we'd strive
to work to make xen use the same startup_32() and startup_64() entries,
with stubs possible using the hypervisor type at the start /end as
follows:
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 of
the newly proposed hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64()
would have the 64-bits.
I realize Andrew has not been a fan over the idea of Xen setting on the
zero page *any* parameter but he's also noted an alternative is to just
lobby for Grub to boot Xen kernels and then we can rely on Grub to
set it for Linux. The only issue with this is it seems this doesn't
address stubdomains which don't use grub?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread
* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
2016-02-06 20:05 ` [Cocci] " Andy Lutomirski
@ 2016-02-09 6:41 ` Luis R. Rodriguez
-1 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:41 UTC (permalink / raw)
To: Andy Lutomirski, Konrad Rzeszutek Wilk, David Vrabel
Cc: Luis R. Rodriguez, cocci, Boris Ostrovsky, Juergen Gross, mcb30,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, david.e.box, X86 ML,
Ingo Molnar
On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
> On Sat, Feb 6, 2016 at 12:59 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Feb 05, 2016 at 11:11:34PM -0800, Andy Lutomirski wrote:
> >> On Feb 5, 2016 8:30 PM, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> >> >
> >> > 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.
> >>
> >> 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 :)
> >
> >> > +/**
> >> > + * 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.
> >>
> >> 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".
> >
> >> Can we just enumerate this crap? I propose:
> >>
> >> Xen PV and lguest are paravirt_legacy. Nothing else is
> >> paravirt_legacy.
> >
> > Fine by me!
> >
> >> 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.
>
> PVH/HVMlite had better not be "paravirt" in this sense, I hope.
Cc'ing David Vrabel so he's aware of this ongoing discussion, which
relates to the HVMLite series.
As far as I follow both this patch series discussion and the HVMLite patch
series by Boris O, it doesn't sound like we yet have a solution for this given
that as-is the patch series for HVMLite still uses paravirt_enabled = 1.
Perhaps the discussion ongoing on this thread will lead us to a safe way to
distinguish.
> > 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.
>
> So shouldn't Linux find out things from the FADT by reading the FADT
> rather than squishing them into a confusing single function?
Boris O replied saying it seems some Xen PV guests can exist without ACPI,
but I was in the same hope as you were as it would seem the architecturally
sane way to do this. PV legacy should ultimately die, so I'm not too
concerned over it, I think we just need a clean way to deprecate it without
causing regressions, but also enabling the best architecture forward.
> Anyway, this is all ridiculous. I propose that rather than trying to
> clean up paravirt_enabled, you just delete it.
Yay.
> 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?
It seems Boris and others have replied to this and a solution to replace
it is in sight without paravirt_enabled().
> 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?
With my patch series I am pretty sure I can remove this check by
relying on the subarch.
> 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.
Likewise. This is avoiding the foof workaround on PV guests as the
hypervisor would have done that itself. Also I was told by Konrad
at the last Xen Developer summit that it seems like we might even
be able to remove the foof workaround from upstream, but this was
just speculation. We could not confirm it.
If we need it I think I can address this with my work.
> 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.
Likewise. I can remove it but it sounds like others can remove it
through other means as replied to this thread.
> 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.
Either way my set of patches address this by relying on the subarch.
> /* 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 really needed likewise, I think the subarch can be used...
>
> 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.
I actually want to remove paravirt_has(NO_RTC), it turns out
that only lguest needs to avoid this, then we not only remove
paravirt_has(NO_RTC) but the RTF flag and the features paravirt
crap. I am pretty sure I can remove this by just relying on
the subarch, LGUEST would just not opt-in for this.
> 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.
Not sure yet as this is early code and that's where I'm having issues,
and its why I've been asking for a simple x86 hypervisr type and seeing
if we can just access it in really early code.
> So yeah, I think the right solution is to delete paravirt_enabled.
Lets do it. Given this is all work we all seem to agree I can just
drop this rename for now, and the other 2 patches can be merged
for now (or ignored, let me know, and then we can just drop things
as we go forward with either the subarch for Xen being set and used
with the linker table stuff, and/or with the other alternatives that
are being discussed so far on this thread by others.
As noted though, we still need the HVMLite paravirt_enabled() thing
sorted out, if we drop it, will HVMLite have a way to ensure it
also gets a bail on the same routines?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread* Re: [PATCH v2 3/3] paravirt: rename paravirt_enabled to paravirt_legacy
@ 2016-02-09 6:41 ` Luis R. Rodriguez
0 siblings, 0 replies; 104+ messages in thread
From: Luis R. Rodriguez @ 2016-02-09 6:41 UTC (permalink / raw)
To: Andy Lutomirski, Konrad Rzeszutek Wilk, David Vrabel
Cc: Luis R. Rodriguez, cocci, Boris Ostrovsky, Juergen Gross, mcb30,
Thomas Gleixner, Andrey Ryabinin, Joerg Roedel, Robert Moore,
Mauro Carvalho Chehab, Rafael J. Wysocki, Xen Devel,
H. Peter Anvin, Rusty Russell, Jan Beulich, Lv Zheng,
linux-kernel, long.wanglong, Fengguang Wu, qiuxishi,
Borislav Petkov, Andrey Ryabinin, david.e.box, X86 ML,
Ingo Molnar
On Sat, Feb 06, 2016 at 12:05:32PM -0800, Andy Lutomirski wrote:
> On Sat, Feb 6, 2016 at 12:59 AM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Fri, Feb 05, 2016 at 11:11:34PM -0800, Andy Lutomirski wrote:
> >> On Feb 5, 2016 8:30 PM, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
> >> >
> >> > 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.
> >>
> >> 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 :)
> >
> >> > +/**
> >> > + * 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.
> >>
> >> 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".
> >
> >> Can we just enumerate this crap? I propose:
> >>
> >> Xen PV and lguest are paravirt_legacy. Nothing else is
> >> paravirt_legacy.
> >
> > Fine by me!
> >
> >> 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.
>
> PVH/HVMlite had better not be "paravirt" in this sense, I hope.
Cc'ing David Vrabel so he's aware of this ongoing discussion, which
relates to the HVMLite series.
As far as I follow both this patch series discussion and the HVMLite patch
series by Boris O, it doesn't sound like we yet have a solution for this given
that as-is the patch series for HVMLite still uses paravirt_enabled = 1.
Perhaps the discussion ongoing on this thread will lead us to a safe way to
distinguish.
> > 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.
>
> So shouldn't Linux find out things from the FADT by reading the FADT
> rather than squishing them into a confusing single function?
Boris O replied saying it seems some Xen PV guests can exist without ACPI,
but I was in the same hope as you were as it would seem the architecturally
sane way to do this. PV legacy should ultimately die, so I'm not too
concerned over it, I think we just need a clean way to deprecate it without
causing regressions, but also enabling the best architecture forward.
> Anyway, this is all ridiculous. I propose that rather than trying to
> clean up paravirt_enabled, you just delete it.
Yay.
> 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?
It seems Boris and others have replied to this and a solution to replace
it is in sight without paravirt_enabled().
> 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?
With my patch series I am pretty sure I can remove this check by
relying on the subarch.
> 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.
Likewise. This is avoiding the foof workaround on PV guests as the
hypervisor would have done that itself. Also I was told by Konrad
at the last Xen Developer summit that it seems like we might even
be able to remove the foof workaround from upstream, but this was
just speculation. We could not confirm it.
If we need it I think I can address this with my work.
> 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.
Likewise. I can remove it but it sounds like others can remove it
through other means as replied to this thread.
> 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.
Either way my set of patches address this by relying on the subarch.
> /* 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 really needed likewise, I think the subarch can be used...
>
> 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.
I actually want to remove paravirt_has(NO_RTC), it turns out
that only lguest needs to avoid this, then we not only remove
paravirt_has(NO_RTC) but the RTF flag and the features paravirt
crap. I am pretty sure I can remove this by just relying on
the subarch, LGUEST would just not opt-in for this.
> 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.
Not sure yet as this is early code and that's where I'm having issues,
and its why I've been asking for a simple x86 hypervisr type and seeing
if we can just access it in really early code.
> So yeah, I think the right solution is to delete paravirt_enabled.
Lets do it. Given this is all work we all seem to agree I can just
drop this rename for now, and the other 2 patches can be merged
for now (or ignored, let me know, and then we can just drop things
as we go forward with either the subarch for Xen being set and used
with the linker table stuff, and/or with the other alternatives that
are being discussed so far on this thread by others.
As noted though, we still need the HVMLite paravirt_enabled() thing
sorted out, if we drop it, will HVMLite have a way to ensure it
also gets a bail on the same routines?
Luis
^ permalink raw reply [flat|nested] 104+ messages in thread