x86/init: replace paravirt_enabled() were possible
Subsystems (8)
cover x86/init: replace paravirt_enabled() were possible
I originally set out to rename paravirt_enabled() to paravirt_legacy() but we instead decided to remove paravirt_enabled() completely. Although I have some linker table work which will help make this cleaner, instead of waiting for that to go in, just remove the known cases that should be safe for paravirt_enabled() and also replace it for the subarch use.
While at it, document the subarch to avoid any possible misuses.
Luis R. Rodriguez (9):
x86/boot: enumerate documentation for the x86 hardware_subarch tools/lguest: make lguest launcher use X86_SUBARCH_LGUEST explicitly x86/xen: use X86_SUBARCH_XEN for PV guest boots
x86/init: make ebda depend on PC subarch
apm32: remove paravirt_enabled() use
x86/tboot: remove paravirt_enabled()
x86/cpu/intel: replace paravirt_enabled() for f00f work around x86/rtc: replace paravirt_enabled() check with subarch check
pnpbios: replace paravirt_enabled() check with subarch checks
arch/x86/include/asm/paravirt.h | 6 ------ arch/x86/include/asm/paravirt_types.h | 5 ----- arch/x86/include/asm/processor.h | 1 - arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- arch/x86/kernel/apm_32.c | 2 +- arch/x86/kernel/cpu/intel.c | 4 +++- arch/x86/kernel/head.c | 2 +- arch/x86/kernel/rtc.c | 3 ++- arch/x86/kernel/tboot.c | 6 ------ arch/x86/xen/enlighten.c | 4 +--- drivers/pnp/pnpbios/core.c | 4 +++- tools/lguest/lguest.c | 4 ++-- 12 files changed, 44 insertions(+), 29 deletions(-)
Signature · 1 line
2.7.0
PATCH 1/9 x86/boot: enumerate documentation for the x86 hardware_subarch
Although hardware_subarch has been in place since the x86 boot protocol 2.07 it hasn't been used much. Enumerate current possible values to avoid misuses and help with semantics later at boot time should this be used further.
--- arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
arch/x86/include/uapi/asm/bootparam.h +31 -1
@@ -157,7 +157,37 @@ struct boot_params {__u8 _pad9[276 ];/* 0xeec */ } __attribute__((packed)); -enum { +/** +* enum x86_hardware_subarch - x86 hardware subarchitecture +* +* The x86 hardware_subarch and hardware_subarch_data were added as of the x86 +* boot protocol 2.07 to help distinguish and supports custom x86 boot +* sequences. This enum represents accepted values for the x86 +* hardware_subarch. Custom x86 boot sequences (not X86_SUBARCH_PC) do not have +* or simply do not make use of natural stubs like BIOS or EFI, the +* hardware_subarch can be used on the Linux entry path to revector to a +* subarchitecture stub when needed. This subarchitecture stub can be used to +* set up Linux boot parameters or for special care to account for nonstandard +* handling of page tables. +* +* KVM and Xen HVM do not have a subarch as these are expected to follow +* standard x86 boot entries. If there is a genuine need for "hypervisor" type +* that should be considered separately in the future. +* +* @X86_SUBARCH_PC: Should be used if the hardware is enumerable using standard +* PC mechanisms (PCI, ACPI) and doesn't need a special boot flow. +* @X86_SUBARCH_LGUEST: Used for x86 hypervisor demo, lguest +* @X86_SUBARCH_LGUEST: Used for x86 hypervisor demo, lguest +* @X86_SUBARCH_XEN: Used for Xen guest types which follow the PV boot path, +* which start at asm startup_xen() entry point and later jump to the C +* xen_start_kernel() entry point. +* @X86_SUBARCH_INTEL_MID: Used for Intel MID (Mobile nternet Device) platform +* systems which do not have the PCI legacy interfaces. +* @X86_SUBARCH_CE4100: Used for Intel CE media processor (CE4100) SOC for +* for settop boxes and media devices, the use of a subarch for CE4100 +* is more of a hack... +*/ +enum x86_hardware_subarch {X86_SUBARCH_PC =0 ,X86_SUBARCH_LGUEST ,X86_SUBARCH_XEN ,
Signature · 1 line
2.7.0
Juergen Gross
maintainerRe: [Xen-devel] [PATCH 1/9] x86/boot: enumerate documentation for the x86 hardware_subarch
Quoted 31 lines · Luis R. Rodriguez
Although
hardware_subarchhas been in place since the x86 boot protocol 2.07 it hasn't been used much. Enumerate current possible values to avoid misuses and help with semantics later at boot time should this be used further.Cc Andy Shevchenko <[email protected]>Signed-off-by Luis R. Rodriguez <[email protected]>--- arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)arch/x86/include/uapi/asm/bootparam.h +11
-1@@ -157,7 +157,37 @@ struct boot_params {__u8 _pad9[276 ];/* 0xeec */ } __attribute__((packed)); -enum { +/** +* enum x86_hardware_subarch - x86 hardware subarchitecture +* +* The x86 hardware_subarch and hardware_subarch_data were added as of the x86 +* boot protocol 2.07 to help distinguish and supports custom x86 boot +* sequences. This enum represents accepted values for the x86 +* hardware_subarch. Custom x86 boot sequences (not X86_SUBARCH_PC) do not have +* or simply do not make use of natural stubs like BIOS or EFI, the +* hardware_subarch can be used on the Linux entry path to revector to a +* subarchitecture stub when needed. This subarchitecture stub can be used to +* set up Linux boot parameters or for special care to account for nonstandard
arch/x86/include/uapi/asm/bootparam.h struct boot_params { · line 180
Quoted 10 lines
arch/x86/include/uapi/asm/bootparam.h +10
-0+* handling of page tables. +* +* KVM and Xen HVM do not have a subarch as these are expected to follow +* standard x86 boot entries. If there is a genuine need for "hypervisor" type +* that should be considered separately in the future. +* +* @X86_SUBARCH_PC: Should be used if the hardware is enumerable using standard +* PC mechanisms (PCI, ACPI) and doesn't need a special boot flow. +* @X86_SUBARCH_LGUEST: Used for x86 hypervisor demo, lguest +* @X86_SUBARCH_LGUEST: Used for x86 hypervisor demo, lguest
Double line
arch/x86/include/uapi/asm/bootparam.h struct boot_params { · line 184
Quoted 4 lines
arch/x86/include/uapi/asm/bootparam.h +4
-0+* @X86_SUBARCH_XEN: Used for Xen guest types which follow the PV boot path, +* which start at asm startup_xen() entry point and later jump to the C +* xen_start_kernel() entry point. +* @X86_SUBARCH_INTEL_MID: Used for Intel MID (Mobile nternet Device) platform
s/nternet/Internet/
arch/x86/include/uapi/asm/bootparam.h struct boot_params { · line 193
Quoted 10 lines
arch/x86/include/uapi/asm/bootparam.h +6
-0+* systems which do not have the PCI legacy interfaces. +* @X86_SUBARCH_CE4100: Used for Intel CE media processor (CE4100) SOC for +* for settop boxes and media devices, the use of a subarch for CE4100 +* is more of a hack... +*/ +enum x86_hardware_subarch {X86_SUBARCH_PC =0 ,X86_SUBARCH_LGUEST ,X86_SUBARCH_XEN ,
Juergen
Re: [Xen-devel] [PATCH 1/9] x86/boot: enumerate documentation for the x86 hardware_subarch
In reply to Luis R. RodriguezQuoted 22 lines · Luis R. Rodriguez
Although
hardware_subarchhas been in place since the x86 boot protocol 2.07 it hasn't been used much. Enumerate current possible values to avoid misuses and help with semantics later at boot time should this be used further.Cc Andy Shevchenko <[email protected]>Signed-off-by Luis R. Rodriguez <[email protected]>--- arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)arch/x86/include/uapi/asm/bootparam.h +2
-1@@ -157,7 +157,37 @@ struct boot_params {__u8 _pad9[276 ];/* 0xeec */ } __attribute__((packed)); -enum { +/** +* enum x86_hardware_subarch - x86 hardware subarchitecture
arch/x86/include/uapi/asm/bootparam.h struct boot_params { · line 171
Quoted 10 lines
arch/x86/include/uapi/asm/bootparam.h +10
-0+* +* The x86 hardware_subarch and hardware_subarch_data were added as of the x86 +* boot protocol 2.07 to help distinguish and supports custom x86 boot +* sequences. This enum represents accepted values for the x86 +* hardware_subarch. Custom x86 boot sequences (not X86_SUBARCH_PC) do not have +* or simply do not make use of natural stubs like BIOS or EFI, the +* hardware_subarch can be used on the Linux entry path to revector to a +* subarchitecture stub when needed. This subarchitecture stub can be used to +* set up Linux boot parameters or for special care to account for nonstandard +* handling of page tables.
This documentation reads like a plan for future implementation. Is this the level of documentation that is needed here?
Also, "revector to a subarchitecture stub" is a rather odd way of saying "call a subarch-specific stub".
David
Re: [Xen-devel] [PATCH 1/9] x86/boot: enumerate documentation for the x86 hardware_subarch
Quoted 37 lines · David Vrabel
Quoted 22 lines · Luis R. Rodriguez
Although
hardware_subarchhas been in place since the x86 boot protocol 2.07 it hasn't been used much. Enumerate current possible values to avoid misuses and help with semantics later at boot time should this be used further.Cc Andy Shevchenko <[email protected]>Signed-off-by Luis R. Rodriguez <[email protected]>--- arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)arch/x86/include/uapi/asm/bootparam.h +2
-1@@ -157,7 +157,37 @@ struct boot_params {__u8 _pad9[276 ];/* 0xeec */ } __attribute__((packed)); -enum { +/** +* enum x86_hardware_subarch - x86 hardware subarchitecture arch/x86/include/uapi/asm/bootparam.h struct boot_params { · line 171
Quoted 10 lines
arch/x86/include/uapi/asm/bootparam.h +10
-0+* +* The x86 hardware_subarch and hardware_subarch_data were added as of the x86 +* boot protocol 2.07 to help distinguish and supports custom x86 boot +* sequences. This enum represents accepted values for the x86 +* hardware_subarch. Custom x86 boot sequences (not X86_SUBARCH_PC) do not have +* or simply do not make use of natural stubs like BIOS or EFI, the +* hardware_subarch can be used on the Linux entry path to revector to a +* subarchitecture stub when needed. This subarchitecture stub can be used to +* set up Linux boot parameters or for special care to account for nonstandard +* handling of page tables. This documentation reads like a plan for future implementation. Is this the level of documentation that is needed here?
Quoted 2 lines
Also, "revector to a subarchitecture stub" is a rather odd way of saying "call a subarch-specific stub".
I took feedback directly from hpa's replies about what the subarch is from my original linker table series. So its not me concocting this. It also shows how your own lack of clarify on this isn't doing us any good either, and its precisely why I am documenting this now.
The lack of semantics here has gotten us intro trouble as it is not allowing us to tool in proactive solutions to prevent bugs. My goal is first to clarify some definitions, and then use them and tool them to proactiveley avoid bugs.
Luis
PATCH 2/9 tools/lguest: make lguest launcher use X86_SUBARCH_LGUEST explicitly
In reply to Luis R. RodriguezBe explicit and make use of X86_SUBARCH_LGUEST directly.
--- tools/lguest/lguest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tools/lguest/lguest.c +2 -2
@@ -3351,8 +3351,8 @@ int main(int argc, char *argv[])/* Boot protocol version: 2.07 supports the fields for lguest. */ boot->hdr.version =0x207 ; -/* The hardware_subarch value of "1" tells the Guest it's an lguest. */ - boot->hdr.hardware_subarch =1 ; +/* X86_SUBARCH_LGUEST tells the Guest it's an lguest. */ + boot->hdr.hardware_subarch =X86_SUBARCH_LGUEST ;/* Tell the entry path not to try to reload segment registers. */ boot->hdr.loadflags |=KEEP_SEGMENTS ;
Signature · 1 line
2.7.0
PATCH 3/9 x86/xen: use X86_SUBARCH_XEN for PV guest boots
In reply to Luis R. RodriguezThe use of subarch should have no current effect on Xen PV guests, as such this should have no current functional effects.
--- arch/x86/xen/enlighten.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/xen/enlighten.c +1 -0
@@ -1669,6 +1669,7 @@ asmlinkage __visible void __init xen_start_kernel(void) boot_params.hdr.ramdisk_image = initrd_start; boot_params.hdr.ramdisk_size = xen_start_info->mod_len; boot_params.hdr.cmd_line_ptr =__pa (xen_start_info->cmd_line); + boot_params.hdr.hardware_subarch =X86_SUBARCH_XEN ;if (!xen_initial_domain ()) {add_preferred_console ("xenboot" ,0 ,NULL );
Signature · 1 line
2.7.0
PATCH 4/9 x86/init: make ebda depend on PC subarch
In reply to Luis R. RodriguezThis lets us remove its use of paravirt_enabled(). The other subarchs are not needed here given that on 32-bit there is a switch already that negates access to this code on X86_SUBARCH_INTEL_MID, and X86_SUBARCH_CE4100. Both lguest and Xen had paravirt_enabled so that excludes them.
--- arch/x86/kernel/head.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/head.c +1 -1
@@ -38,7 +38,7 @@ void __init reserve_ebda_region(void)* that the paravirt case can handle memory setup * correctly, without our help. */ -if (paravirt_enabled ()) +if (boot_params.hdr.hardware_subarch !=X86_SUBARCH_PC )return ;/* end of low (conventional) memory */
Signature · 1 line
2.7.0
PATCH 5/9 apm32: remove paravirt_enabled() use
In reply to Luis R. RodriguezThere is already a check for apm_info.bios == 0, the apm_info.bios is set from the boot_params.apm_bios_info. Both Xen and lguest, which are also the only ones that set paravirt_enabled to true) do never set the apm_bios_info, the paravirt_enabled() check is simply not needed.
--- arch/x86/kernel/apm_32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/apm_32.c +1 -1
@@ -2267,7 +2267,7 @@ static int __init apm_init(void)dmi_check_system (apm_dmi_table); -if (apm_info.bios.version ==0 ||paravirt_enabled () ||machine_is_olpc ()) { +if (apm_info.bios.version ==0 ||machine_is_olpc ()) {printk (KERN_INFO "apm: BIOS not found.\n" );return -ENODEV ; }
Signature · 1 line
2.7.0
Boris Ostrovsky
reviewerRe: [PATCH 5/9] apm32: remove paravirt_enabled() use
Quoted 5 lines · Luis R. Rodriguez
There is already a check for apm_info.bios == 0, the apm_info.bios is set from the boot_params.apm_bios_info. Both Xen and lguest, which are also the only ones that set
paravirt_enabledto true) do never set theapm_bios_info, theparavirt_enabled()check is simply not needed.
We need to guarantee that boot_params is filled with zeroes. On baremetal path we clear .bss (which is where boot_params live) before copying data from zero page.
So we need to at least memset(&boot_params, 0, sz) in xen_start_kernel(). Better yet, clear whole .bss.
(This applies to the next patch as well).
-boris
Re: [PATCH 5/9] apm32: remove paravirt_enabled() use
Quoted 9 lines · Boris Ostrovsky
Quoted 5 lines · Luis R. Rodriguez
There is already a check for apm_info.bios == 0, the apm_info.bios is set from the boot_params.apm_bios_info. Both Xen and lguest, which are also the only ones that set
paravirt_enabledto true) do never set theapm_bios_info, theparavirt_enabled()check is simply not needed.
Quoted 3 lines
We need to guarantee that
boot_paramsis filled with zeroes. On baremetal path we clear .bss (which is whereboot_paramslive) before copying data from zero page.
To be clear Xen is the only user of this that just sets it outright instead of poking at the file and using that as reference, and setting what it needs. Its a good point though that things like this which are 0 still should probably be set.
Quoted 1 line
So we need to at least
memset(&boot_params, 0, sz)
This use case would be just Xen specific, for lguest we can just set the things we need clearly on the launcher. So like this (I can fold in a separate patch):
tools/lguest/lguest.c +6 -0
@@ -3357,6 +3357,12 @@ int main(int argc, char *argv[])/* Tell the entry path not to try to reload segment registers. */ boot->hdr.loadflags |=KEEP_SEGMENTS ; +/* We don't support tboot */ + boot->tboot_addr =0 ; + +/* Ensure this is 0 to prevent apm from loading */ + boot->apm_bios_info.version =0 ; +/* We tell the kernel to initialize the Guest. */ tell_kernel (start);
Quoted 3 lines
in
xen_start_kernel(). Better yet, clear whole .bss.(This applies to the next patch as well).
So clear_bss() -- oh look, another call that xen_start_kernel() could have made good use of. :) Can you send a respective patch I can old into this series? I'm afraid it is by no means obvious to me where it would be safe to do this on xen_start_kernel().
Luis
Boris Ostrovsky
reviewerRe: [PATCH 5/9] apm32: remove paravirt_enabled() use
Quoted 9 lines · Luis R. Rodriguez
Quoted 3 lines · Boris Ostrovsky
in
xen_start_kernel(). Better yet, clear whole .bss.(This applies to the next patch as well).
So
clear_bss()-- oh look, another call thatxen_start_kernel()could have made good use of. :) Can you send a respective patch I can old into this series? I'm afraid it is by no means obvious to me where it would be safe to do this onxen_start_kernel().
OK, I'll test it next week, it really should be done irrespective of what you are doing here.
-boris
Re: [PATCH 5/9] apm32: remove paravirt_enabled() use
Quoted 7 lines · Boris Ostrovsky
Quoted 4 lines · Luis R. Rodriguez
Quoted 2 lines · Boris Ostrovsky
in
xen_start_kernel(). Better yet, clear whole .bss.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
(This applies to the next patch as well).
So
clear_bss()-- oh look, another call thatxen_start_kernel()could have made good use of. :) Can you send a respective patch I can old into this series? I'm afraid it is by no means obvious to me where it would be safe to do this onxen_start_kernel().OK, I'll test it next week, it really should be done irrespective of what you are doing here.
OK so it can go in separately?
Luis
Boris Ostrovsky
reviewerRe: [PATCH 5/9] apm32: remove paravirt_enabled() use
Quoted 7 lines · Luis R. Rodriguez
Quoted 6 lines · Boris Ostrovsky
Quoted 4 lines · Luis R. Rodriguez
Quoted 2 lines · Boris Ostrovsky
in
xen_start_kernel(). Better yet, clear whole .bss.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
(This applies to the next patch as well).
So
clear_bss()-- oh look, another call thatxen_start_kernel()could have made good use of. :) Can you send a respective patch I can old into this series? I'm afraid it is by no means obvious to me where it would be safe to do this onxen_start_kernel().OK, I'll test it next week, it really should be done irrespective of what you are doing here.
OK so it can go in separately?
Yes, I'll send it as a separate patch.
-boris
PATCH 6/9 x86/tboot: remove paravirt_enabled()
In reply to Luis R. RodriguezThere is already a check for boot_params.tboot_addr prior to paravirt_enabled() and both Xen and lguest never set this. This check is not needed.
--- arch/x86/kernel/tboot.c | 6 ------ 1 file changed, 6 deletions(-)
arch/x86/kernel/tboot.c +0 -6
@@ -74,12 +74,6 @@ void __init tboot_probe(void)return ; } -/* 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 ; - } -/* Map and check for tboot UUID. */ set_fixmap (FIX_TBOOT_BASE , boot_params.tboot_addr); tboot = (struct tboot *)fix_to_virt (FIX_TBOOT_BASE );
Signature · 1 line
2.7.0
PATCH 7/9 x86/cpu/intel: replace paravirt_enabled() for f00f work around
In reply to Luis R. RodriguezUse the harware subarch instead now that they are set. If we want to test removing this work around on Xen we can do so separatley as another independent change, for now we just want to remove paravirt_enabled().
--- arch/x86/kernel/cpu/intel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
arch/x86/kernel/cpu/intel.c +3 -1
@@ -220,7 +220,9 @@ static void intel_workarounds(struct cpuinfo_x86 *c)* The Quark is also family 5, but does not have the same bug. */ clear_cpu_bug (c,X86_BUG_F00F ); -if (!paravirt_enabled () && c->x86 ==5 && c->x86_model <9 ) { +if (boot_params.hdr.hardware_subarch !=X86_SUBARCH_XEN && + boot_params.hdr.hardware_subarch !=X86_SUBARCH_LGUEST && + c->x86 ==5 && c->x86_model <9 ) {static int f00f_workaround_enabled;set_cpu_bug (c,X86_BUG_F00F );
Signature · 1 line
2.7.0
PATCH 8/9 x86/rtc: replace paravirt_enabled() check with subarch check
In reply to Luis R. RodriguezThe current check is a super long winded way of asking if this is on lguest. The flags is used for legacy features, this is likely inspired by the ACPI IA-PC boot architecture flags, where as for RTC it annotates No CMOS real-time clock present. I don't expect we will be implementing more legacy features, its simply pointless so just remove this legacy flag feature thing as well.
--- arch/x86/include/asm/paravirt.h | 6 ------ arch/x86/include/asm/paravirt_types.h | 5 ----- arch/x86/include/asm/processor.h | 1 - arch/x86/kernel/rtc.c | 3 ++- arch/x86/xen/enlighten.c | 3 --- 5 files changed, 2 insertions(+), 16 deletions(-)
arch/x86/include/asm/paravirt.h +0 -6
@@ -19,12 +19,6 @@ static inline int paravirt_enabled(void)return pv_info.paravirt_enabled; } -static inline int paravirt_has_feature (unsigned int feature) -{ -WARN_ON_ONCE (!pv_info.paravirt_enabled); -return (pv_info.features & feature); -} -static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread) {
arch/x86/include/asm/paravirt_types.h +0 -5
@@ -70,14 +70,9 @@ struct pv_info {#endif int paravirt_enabled; -unsigned int features;/* valid only if paravirt_enabled is set */ const char *name; }; -#define paravirt_has (x) paravirt_has_feature(PV_SUPPORTED_##x) -/* Supported features */ -#define PV_SUPPORTED_RTC (1<<0) -struct pv_init_ops {/* * Patch may replace one of the defined code sequences with
arch/x86/include/asm/processor.h +0 -1
@@ -472,7 +472,6 @@ static inline unsigned long current_top_of_stack(void)#else #define __cpuid native_cpuid#define paravirt_enabled () 0 -#define paravirt_has (x) 0static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread)
arch/x86/kernel/rtc.c +2 -1
@@ -14,6 +14,7 @@#include <asm/time.h> #include <asm/intel-mid.h> #include <asm/rtc.h> +#include <asm/setup.h> #ifdef CONFIG_X86_32 /*
@@ -200,7 +201,7 @@ static __init int add_rtc_cmos(void) }#endif -if (paravirt_enabled () && !paravirt_has (RTC )) +if (boot_params.hdr.hardware_subarch ==X86_SUBARCH_LGUEST )return -ENODEV ;platform_device_register (&rtc_device);
arch/x86/xen/enlighten.c +0 -3
@@ -1192,7 +1192,6 @@ static const struct pv_info xen_info __initconst = {#ifdef CONFIG_X86_64 .extra_user_64bit_cs = FLAT_USER_CS64, #endif - .features = 0, .name = "Xen", };
@@ -1526,8 +1525,6 @@ asmlinkage __visible void __init xen_start_kernel(void)/* Install Xen paravirt ops */ pv_info = xen_info; -if (xen_initial_domain ()) - pv_info.features |=PV_SUPPORTED_RTC ; pv_init_ops = xen_init_ops;if (!xen_pvh_domain ()) { pv_cpu_ops = xen_cpu_ops;
Signature · 1 line
2.7.0
Juergen Gross
maintainerRe: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 2 lines · Luis R. Rodriguez
The current check is a super long winded way of asking if this is on lguest. The flags is used for legacy features, this is
What about Xen pv-domU? I wouldn't expect those to have PV_SUPPORTED_RTC set.
Juergen
Quoted 105 lines
likely inspired by the ACPI IA-PC boot architecture flags, where as for RTC it annotates No CMOS real-time clock present. I don't expect we will be implementing more legacy features, its simply pointless so just remove this legacy flag feature thing as well.
Signed-off-by Luis R. Rodriguez <[email protected]>--- arch/x86/include/asm/paravirt.h | 6 ------ arch/x86/include/asm/paravirt_types.h | 5 ----- arch/x86/include/asm/processor.h | 1 - arch/x86/kernel/rtc.c | 3 ++- arch/x86/xen/enlighten.c | 3 --- 5 files changed, 2 insertions(+), 16 deletions(-)arch/x86/include/asm/paravirt.h +0
-6@@ -19,12 +19,6 @@ static inline int paravirt_enabled(void)return pv_info.paravirt_enabled; } -static inline int paravirt_has_feature (unsigned int feature) -{ -WARN_ON_ONCE (!pv_info.paravirt_enabled); -return (pv_info.features & feature); -} -static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread) {arch/x86/include/asm/paravirt_types.h +0
-5@@ -70,14 +70,9 @@ struct pv_info {#endif int paravirt_enabled; -unsigned int features;/* valid only if paravirt_enabled is set */ const char *name; }; -#define paravirt_has (x) paravirt_has_feature(PV_SUPPORTED_##x) -/* Supported features */ -#define PV_SUPPORTED_RTC (1<<0) -struct pv_init_ops {/* * Patch may replace one of the defined code sequences with arch/x86/include/asm/processor.h +0
-1@@ -472,7 +472,6 @@ static inline unsigned long current_top_of_stack(void)#else #define __cpuid native_cpuid#define paravirt_enabled () 0 -#define paravirt_has (x) 0static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread)arch/x86/kernel/rtc.c +2
-1@@ -14,6 +14,7 @@#include <asm/time.h> #include <asm/intel-mid.h> #include <asm/rtc.h> +#include <asm/setup.h> #ifdef CONFIG_X86_32 /* @@ -200,7 +201,7 @@ static __init int add_rtc_cmos(void) }#endif -if (paravirt_enabled () && !paravirt_has (RTC )) +if (boot_params.hdr.hardware_subarch ==X86_SUBARCH_LGUEST )return -ENODEV ;platform_device_register (&rtc_device);arch/x86/xen/enlighten.c +0
-3@@ -1192,7 +1192,6 @@ static const struct pv_info xen_info __initconst = {#ifdef CONFIG_X86_64 .extra_user_64bit_cs = FLAT_USER_CS64, #endif - .features = 0, .name = "Xen", };@@ -1526,8 +1525,6 @@ asmlinkage __visible void __init xen_start_kernel(void)/* Install Xen paravirt ops */ pv_info = xen_info; -if (xen_initial_domain ()) - pv_info.features |=PV_SUPPORTED_RTC ; pv_init_ops = xen_init_ops;if (!xen_pvh_domain ()) { pv_cpu_ops = xen_cpu_ops;
Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 6 lines · Juergen Gross
Quoted 2 lines · Luis R. Rodriguez
The current check is a super long winded way of asking if this is on lguest. The flags is used for legacy features, this is
What about Xen pv-domU? I wouldn't expect those to have
PV_SUPPORTED_RTCset.
Hrm, I see -- how do we check for that in a standard more clean way?
Luis
Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 9 lines · Luis R. Rodriguez
Quoted 6 lines · Juergen Gross
Quoted 2 lines · Luis R. Rodriguez
The current check is a super long winded way of asking if this is on lguest. The flags is used for legacy features, this is
What about Xen pv-domU? I wouldn't expect those to have
PV_SUPPORTED_RTCset.Hrm, I see -- how do we check for that in a standard more clean way?
OK we have 4 types of x86 platforms that disable RTC:
* Intel MID
* Lguest
* Xen dom-U
* x86 on legacy systems annotated with an ACPI legacy flag
So it would seem its best to just generalize the disabling of the RTC for any x86 platform, we could also fold the ACPI check into the FADT parse table routine, which is called during setup_arch().
So how about this:
arch/x86/include/asm/paravirt.h +0 -6
@@ -19,12 +19,6 @@ static inline int paravirt_enabled(void)return pv_info.paravirt_enabled; } -static inline int paravirt_has_feature (unsigned int feature) -{ -WARN_ON_ONCE (!pv_info.paravirt_enabled); -return (pv_info.features & feature); -} -static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread) {
arch/x86/include/asm/paravirt_types.h +0 -5
@@ -70,14 +70,9 @@ struct pv_info {#endif int paravirt_enabled; -unsigned int features;/* valid only if paravirt_enabled is set */ const char *name; }; -#define paravirt_has (x) paravirt_has_feature(PV_SUPPORTED_##x) -/* Supported features */ -#define PV_SUPPORTED_RTC (1<<0) -struct pv_init_ops {/* * Patch may replace one of the defined code sequences with
arch/x86/include/asm/processor.h +0 -1
@@ -472,7 +472,6 @@ static inline unsigned long current_top_of_stack(void)#else #define __cpuid native_cpuid#define paravirt_enabled () 0 -#define paravirt_has (x) 0static inline void load_sp0 (struct tss_struct *tss,struct thread_struct *thread)
arch/x86/include/asm/x86_init.h +3 -0
@@ -84,11 +84,14 @@ struct x86_init_paging {* boot cpu * @timer_init: initialize the platform timer (default PIT/HPET) * @wallclock_init: init the wallclock device +* @no_cmos_rtc: set when platform has no CMOS real-time clock +* present */ struct x86_init_timers {void (*setup_percpu_clockev)(void );void (*timer_init)(void );void (*wallclock_init)(void ); +bool no_cmos_rtc; };/**
arch/x86/kernel/acpi/boot.c +4 -0
@@ -913,6 +913,10 @@ late_initcall(hpet_insert_resource);static int __init acpi_parse_fadt (struct acpi_table_header *table) { +if (acpi_gbl_FADT.boot_flags &ACPI_FADT_NO_CMOS_RTC ) { +pr_debug ("ACPI: not registering RTC platform device\n" ); + x86_init.timers.no_cmos_rtc = true; + }#ifdef CONFIG_X86_PM_TIMER /* detect the location of the ACPI PM Timer */
arch/x86/kernel/rtc.c +2 -13
@@ -14,6 +14,7 @@#include <asm/time.h> #include <asm/intel-mid.h> #include <asm/rtc.h> +#include <asm/setup.h> #ifdef CONFIG_X86_32 /*
@@ -188,19 +189,7 @@ static __init int add_rtc_cmos(void)if (of_have_populated_dt ())return 0 ; -/* Intel MID platforms don't have ioport rtc */ -if (intel_mid_identify_cpu ()) -return -ENODEV ; - -#ifdef CONFIG_ACPI -if (acpi_gbl_FADT.boot_flags &ACPI_FADT_NO_CMOS_RTC ) { -/* This warning can likely go away again in a year or two. */ -pr_info ("ACPI: not registering RTC platform device\n" ); -return -ENODEV ; - } -#endif - -if (paravirt_enabled () && !paravirt_has (RTC )) +if (x86_init.timers.no_cmos_rtc)return -ENODEV ;platform_device_register (&rtc_device);
arch/x86/lguest/boot.c +1 -1
@@ -1414,7 +1414,6 @@ __init void lguest_init(void) pv_info.kernel_rpl =1 ;/* Everyone except Xen runs with this set. */ pv_info.shared_kernel_pmd =1 ; - pv_info.features =0 ;/* * We set up all the lguest overrides for sensitive operations. These
@@ -1482,6 +1481,7 @@ __init void lguest_init(void) x86_init.resources.memory_setup = lguest_memory_setup; x86_init.irqs.intr_init = lguest_init_IRQ; x86_init.timers.timer_init = lguest_time_init; + x86_init.timers.no_cmos_rtc = true; x86_platform.calibrate_tsc = lguest_tsc_khz; x86_platform.get_wallclock = lguest_get_wallclock;
arch/x86/platform/intel-mid/intel-mid.c +3 -0
@@ -175,6 +175,9 @@ void __init x86_intel_mid_early_setup(void) x86_init.timers.timer_init = intel_mid_time_init; x86_init.timers.setup_percpu_clockev = x86_init_noop; +/* Intel MID platforms don't have ioport rtc */ + x86_init.timers.no_cmos_rtc = true; + x86_init.irqs.pre_vector_init = x86_init_noop; x86_init.oem.arch_setup = intel_mid_arch_setup;
arch/x86/xen/enlighten.c +0 -3
@@ -1192,7 +1192,6 @@ static const struct pv_info xen_info __initconst = {#ifdef CONFIG_X86_64 .extra_user_64bit_cs = FLAT_USER_CS64, #endif - .features = 0, .name = "Xen", };
@@ -1526,8 +1525,6 @@ asmlinkage __visible void __init xen_start_kernel(void)/* Install Xen paravirt ops */ pv_info = xen_info; -if (xen_initial_domain ()) - pv_info.features |=PV_SUPPORTED_RTC ; pv_init_ops = xen_init_ops;if (!xen_pvh_domain ()) { pv_cpu_ops = xen_cpu_ops;
arch/x86/xen/time.c +3 -1
@@ -446,8 +446,10 @@ void __init xen_init_time_ops(void) x86_platform.calibrate_tsc = xen_tsc_khz; x86_platform.get_wallclock = xen_get_wallclock;/* Dom0 uses the native method to set the hardware RTC. */ -if (!xen_initial_domain ()) +if (!xen_initial_domain ()) { + x86_init.timers.no_cmos_rtc = true; x86_platform.set_wallclock = xen_set_wallclock; + } }#ifdef CONFIG_XEN_PVHVM
Borislav Petkov
maintainerRe: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 6 lines · Luis R. Rodriguez
arch/x86/include/asm/x86_init.h +0
-0@@ -84,11 +84,14 @@ struct x86_init_paging {* boot cpu
arch/x86/include/asm/x86_init.h struct x86_init_paging { · line 94
Quoted 10 lines
arch/x86/include/asm/x86_init.h +3
-0* @timer_init: initialize the platform timer (default PIT/HPET) * @wallclock_init: init the wallclock device +* @no_cmos_rtc: set when platform has no CMOS real-time clock +* present */ struct x86_init_timers {void (*setup_percpu_clockev)(void );void (*timer_init)(void );void (*wallclock_init)(void ); +bool no_cmos_rtc;
I'd add
u64 flags;
to x86_init_ops and then set X86_PLATFORM_NO_RTC or so in there. The reason being, others could use that flags field too, for other stuff and define more bits.
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.Boris Ostrovsky
reviewerRe: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 19 lines · Borislav Petkov
Quoted 6 lines · Luis R. Rodriguez
arch/x86/include/asm/x86_init.h +0
-0@@ -84,11 +84,14 @@ struct x86_init_paging {* boot cpu arch/x86/include/asm/x86_init.h struct x86_init_paging { · line 94
Quoted 10 lines
arch/x86/include/asm/x86_init.h +3
-0* @timer_init: initialize the platform timer (default PIT/HPET) * @wallclock_init: init the wallclock device +* @no_cmos_rtc: set when platform has no CMOS real-time clock +* present */ struct x86_init_timers {void (*setup_percpu_clockev)(void );void (*timer_init)(void );void (*wallclock_init)(void ); +bool no_cmos_rtc;I'd add
Quoted 5 lines
u64 flags;to
x86_init_opsand then setX86_PLATFORM_NO_RTCor so in there. The reason being, others could use that flags field too, for other stuff and define more bits.
Maybe timer_flags or platform_flags (or something else) to be a little more cscope-friendly?
-boris
Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
Quoted 20 lines · Boris Ostrovsky
Quoted 19 lines · Borislav Petkov
Quoted 6 lines · Luis R. Rodriguez
arch/x86/include/asm/x86_init.h +0
-0@@ -84,11 +84,14 @@ struct x86_init_paging {* boot cpu arch/x86/include/asm/x86_init.h struct x86_init_paging { · line 94
Quoted 10 lines
arch/x86/include/asm/x86_init.h +3
-0* @timer_init: initialize the platform timer (default PIT/HPET) * @wallclock_init: init the wallclock device +* @no_cmos_rtc: set when platform has no CMOS real-time clock +* present */ struct x86_init_timers {void (*setup_percpu_clockev)(void );void (*timer_init)(void );void (*wallclock_init)(void ); +bool no_cmos_rtc;I'd add
Quoted 8 lines
Quoted 5 lines
u64 flags;to
x86_init_opsand then setX86_PLATFORM_NO_RTCor so in there. The reason being, others could use that flags field too, for other stuff and define more bits.Maybe
timer_flagsorplatform_flags(or something else) to be a little more cscope-friendly?
Sure, I'll go with platform_flags on x86_init_ops. Will repost a new series after 0-day testing.
Luis
Re: [Xen-devel] [PATCH 8/9] x86/rtc: replace paravirt_enabled() check with subarch check
In reply to Luis R. RodriguezQuoted 6 lines · Luis R. Rodriguez
The current check is a super long winded way of asking if this is on lguest. The flags is used for legacy features, this is likely inspired by the ACPI IA-PC boot architecture flags, where as for RTC it annotates No CMOS real-time clock present. I don't expect we will be implementing more legacy features, its simply pointless so just remove this legacy flag feature thing as well.
No. This check is needed for Xen PV domU guests and without it PV guests don't get a console (as I think the history of the change that introduced this makes clear).
I think this trend towards a coarse-grained subarch instead of a set of feature bits is the wrong direction.
David
PATCH 9/9 pnpbios: replace paravirt_enabled() check with subarch checks
In reply to Luis R. RodriguezSince we are removing paravirt_enabled() replace it with a logical equivalent.
--- drivers/pnp/pnpbios/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/pnp/pnpbios/core.c +3 -1
@@ -66,6 +66,7 @@#include <asm/page.h> #include <asm/desc.h> #include <asm/byteorder.h> +#include <asm/setup.h> #include "../base.h" #include "pnpbios.h"
@@ -521,7 +522,8 @@ static int __init pnpbios_init(void)int ret;if (pnpbios_disabled ||dmi_check_system (pnpbios_dmi_table) || -paravirt_enabled ()) { + boot_params.hdr.hardware_subarch ==X86_SUBARCH_LGUEST || + boot_params.hdr.hardware_subarch ==X86_SUBARCH_XEN ) {printk (KERN_INFO "PnPBIOS: Disabled\n" );return -ENODEV ; }
Signature · 1 line
2.7.0
Quoted 6 lines · Luis R. Rodriguez
I originally set out to rename
paravirt_enabled()toparavirt_legacy()but we instead decided to removeparavirt_enabled()completely. Although I have some linker table work which will help make this cleaner, instead of waiting for that to go in, just remove the known cases that should be safe forparavirt_enabled()and also replace it for the subarch use.
I don't understand the purpose of this series. How is replacing "if (A)" with "if (B)" an improvement?
Using the hardware subarch in this way looks like paravirt_enabled() with a slightly different flavour and I don't think this is that useful to determine if certain hardware features should be used.
I think you should consider a finer-grained set of feature bits. e.g., platform_has_pc_rtc() and platform_has_tboot() etc.
David
Quoted 28 lines
While at it, document the subarch to avoid any possible misuses.
Luis R. Rodriguez (9):
x86/boot: enumerate documentation for the x86hardware_subarchtools/lguest: make lguest launcher useX86_SUBARCH_LGUESTexplicitly x86/xen: useX86_SUBARCH_XENfor PV guest boots
x86/init: make ebda depend on PC subarch
apm32: removeparavirt_enabled()use
x86/tboot: removeparavirt_enabled()
x86/cpu/intel: replaceparavirt_enabled()for f00f work around x86/rtc: replaceparavirt_enabled()check with subarch check
pnpbios: replaceparavirt_enabled()check with subarch checksarch/x86/include/asm/paravirt.h | 6 ------ arch/x86/include/asm/paravirt_types.h | 5 ----- arch/x86/include/asm/processor.h | 1 - arch/x86/include/uapi/asm/bootparam.h | 32 +++++++++++++++++++++++++++++++- arch/x86/kernel/apm_32.c | 2 +- arch/x86/kernel/cpu/intel.c | 4 +++- arch/x86/kernel/head.c | 2 +- arch/x86/kernel/rtc.c | 3 ++- arch/x86/kernel/tboot.c | 6 ------ arch/x86/xen/enlighten.c | 4 +--- drivers/pnp/pnpbios/core.c | 4 +++- tools/lguest/lguest.c | 4 ++-- 12 files changed, 44 insertions(+), 29 deletions(-)
Quoted 10 lines · David Vrabel
Quoted 6 lines · Luis R. Rodriguez
I originally set out to rename
paravirt_enabled()toparavirt_legacy()but we instead decided to removeparavirt_enabled()completely. Although I have some linker table work which will help make this cleaner, instead of waiting for that to go in, just remove the known cases that should be safe forparavirt_enabled()and also replace it for the subarch use.I don't understand the purpose of this series. How is replacing "if (A)" with "if (B)" an improvement?
Its about semantics. Let's recall I first set out to simply document paravirt_enabled() as per Konrad's recommendation I also renamed it to avoid confusion. What we've learned is that paravirt_enabled()'s semantics were a mishap due to two main factors:
1) Industry moving way from legacy:
a) Exhibit A: PC system design guide [0] - Move by Intel and Microsoft to help build legacy free systems.
b) EXhibit B: ACPI IA-PC boot architecture flags (all for legacy)
2) The array of different guest types during this time.
The best thing we had was subarch added via boot protocol 2.07 but as I've learned only lguest and a few hacks picked it up, even though Xen clearly had a subarch added for it. The current "grey" area of different guest types does not help, but this just means we lack way to further describe things.
[0] http://tech-insider.org/windows/research/2000/1102.html
Quoted 3 lines
Using the hardware subarch in this way looks like
paravirt_enabled()with a slightly different flavour and I don't think this is that useful to determine if certain hardware features should be used.
I agree, and it was not my goal to just leave it like that. The *real* way I am targeting use of the subarch is reflected in how I am proposing we use linker tables and dependency relationships on x86. Please refer to that series where such annotations added here are removed. I posted this series separately as otherwise the clutch to removal of paravirt_enabled() is the progress on linker tables. That may take time.
Quoted 2 lines
I think you should consider a finer-grained set of feature bits. e.g.,
platform_has_pc_rtc()andplatform_has_tboot()etc.
Are you suggesting as further x86 boot protocol changes?
Luis