Message-ID: <20160223085119.GA10182@gmail.com>
Date:	Tue, 23 Feb 2016 09:51:19 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	"Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:	bp@...en8.de, hpa@...or.com, tglx@...utronix.de, mingo@...hat.com,
	rusty@...tcorp.com.au, x86@...nel.org,
	linux-kernel@...r.kernel.org, luto@...capital.net,
	boris.ostrovsky@...cle.com, david.vrabel@...rix.com,
	konrad.wilk@...cle.com, xen-devel@...ts.xensource.com,
	lguest@...ts.ozlabs.org,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v3 01/11] x86/boot: enumerate documentation for the x86
 hardware_subarch


* Luis R. Rodriguez <mcgrof@...nel.org> wrote:

> 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.
> 
> v2: fix typos
> 
> Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
> ---
>  arch/x86/include/uapi/asm/bootparam.h | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index 329254373479..50d5009cf276 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -157,7 +157,36 @@ 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_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 Internet 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,

No, this is really backwards.

While I agree that we want to get rid of paravirt_enabled(), we _dont_ want to 
spread the use of (arguably broken) boot flags like this! This is one of the cases 
where the cure is worse than the disease.

The 'modern' way to handle platform quirks is to have hardware drivers with 
auto-detection, and drivers know how to detect whether the hardware is present or 
not. For legacy cases where no auto-detection is possible, we have per hardware 
capability flags to turn it off.

The 'hardware subarch' flag of the bootloader can be used to install certain 
quirks, in a single early bootup function - but that should be all: ideally no 
quirks are needed. We don't want to spread 'subarch flags' into various unrelated 
code.

Let's go over your series and see whether and how that principle can be applied:

 - patch #1, #2: should be dropped

 - patch #3, #4: EBDA support.

   The EBDA BIOS signature is an ancient data structure, starting off at
   physical memory 0x40E - which is the very first physical memory page of the
   system.

   We should add an x86_ebda_bios flag that is set to 1 by default, but which
   paravirt bootup can set to 0. That would avoid the reservation of the BIOS 
   area and will save a bit of RAM.

 - patch #5, #6, #7: looks good, does not use a subarch flag

 - patch #8: f00f workaround. Subarch flag use is wrong. The complication with 
   this workaround is that it uses MM tricks to install an IDT. Could you check 
   whether Xen truly needs this quirk? If yes then there should be a new flag, 
   something like x86_idt_readonly, which is set to 0 but Xen can set it to 1. If 
   that flag is set then the F00F workaround does not have to be installed.

   Or something like that: the point is to use a specific flag.

 - Patch #9, #10: RTC support. The problem with RTC platform driver is that it's 
   not possible to detect the RTC reliably - so we sometimes have to quirk it off.

   Instead of using bitflags, add something like x86_platform.rtc_available, which 
   defaults to 1. Don't add negation to the name and don't use bitflags - use a 
   byte flag.

 - Patch #11: this patch wants to disable the PNP BIOS code.

   The complication with the PNP BIOS code is that the PNP BIOS is defined 
   in physical memory, in the 0xf0000-0xffff0 memory range - a 64kB large area. 
   Paravirt images don't want to waste 64kB of RAM just to tell the kernel that no
   PNP BIOS is available.

   But instead of using subarch flags, please define a pnpbios_disable() API call 
   that paravirt bootup can call into. That can disable the PNP BIOS code.

Also, feel free to define a single 'disable PC legacies' quirk function that 
disables all the usual PC legacies that paravirt does not care about or does not 
want to define via RAM. Specific paravirt initialization functions would only have 
to call this one (shared) function.

Thanks,

	Ingo

Powered by blists - more mailing lists