Re: [PATCH v3 01/11] x86/boot: enumerate documentation for the x86 hardware_subarch

[Ingo Molnar]

* Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:

> > > > +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!
> > 
> > I agree that we should not see the spread of boot flags around general x86 
> > code, its not my goal to spread it though, the code that uses it here though 
> > is *early boot code* (although in retrospect the pnpbios use was a fuckup), 
> > and I have some special considerations for early boot code which I think does 
> > give merit to it use. But also keep in mind my goal is to rather fold the boot 
> > flag so its more just an architectural consideration eventually. More on this 
> > below.
> 
> It seems I TL;DR suck; all this is a long winded way of asking, can we keep the 
> subarch just for EBDA and use the flags for the other things as you noted?

No, even for EBDA we should make the flag EBDA specific, because that really tells 
us what it's about.

The EBDA code could not care less about what subarch it's running on - it only 
cares about whether it's safe to search for the EBDA signature in the hardware's 
low RAM.

So instead of this:

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

I'd suggest adding an EBDA search flag, something like this:

	if (!x86_platform.legacy.ebda_search)
		return;

Note that the 'legacy' intermediate structure can be used to group various 
legacies, while making it clear that this is not something modern hardware should 
care about.

The x86_platform.legacy.ebda_search flag can then be set up during (very) early 
bootup:

	if (boot_params.hdr.hardware_subarch == X86_SUBARCH_PC)
		x86_platform.legacy.ebda_search = 1;

This might look like an unnecessary indirection but it isn't: beyond the 
documentation value it also makes things a lot clearer should we introduce other 
legacy flags in x86_platform.legacy.

For hard coded platform quirks I'd suggest we add x86_platform.quirks flags. For 
example the F00F hack for Xen could be done via:

	x86_platform.quirks.idt_remap = 0;

and would be set like this during early init:

void early_init_platform_quirks(void)
{
	x86_platform.legacy.ebda_search = 0;
	x86_platform.quirks.idt_remap = 1;

	switch (boot_params.hdr.hardware_subarch) {
		case X86_SUBARCH_PC:
			x86_platform.legacy.ebda_search = 1;
			break;
		case X86_SUBARCH_XEN:
			x86_platform.quirks.idt_remap = 0;
			break;
		case X86_SUBARCH_LGUEST:
			x86_platform.quirks.idt_remap = 0;
			break;
	}
}

And if also add the legacy RTC flag, it becomes:

void early_init_hardcoded_platform_quirks(void)
{
	x86_platform.legacy.ebda_search = 0;
	x86_platform.quirks.idt_remap = 1;
	x86_platform.legacy.rtc = 1;

	switch (boot_params.hdr.hardware_subarch) {
		case X86_SUBARCH_PC:
			x86_platform.legacy.ebda_search = 1;
			break;
		case X86_SUBARCH_XEN:
			x86_platform.quirks.idt_remap = 0;
			x86_platform.legacy.rtc = 0;
			break;
		case X86_SUBARCH_LGUEST:
			x86_platform.quirks.idt_remap = 0;
			x86_platform.legacy.rtc = 0;
			break;
	}
}

Note that both opt-in and opt-out quirks/legacies are possible this way, and note 
how cleanly and consistently it's all organized: setup of all hard coded 
legacies/quirks is concentrated in a single function, and the actual usage sites 
don't know anything about subarchitectures.

Ok? Functionally this approach is equivalent to your series, but it's cleaner, and 
it's also easier to maintain in the long run: there's only a single place to look 
to check all hard coded legacies/quirks - instead of the code being spread out all 
around the x86 code.

Furthermore we should probably move a few other existing legacies to this flag 
space as well, to make all this more consistent.

Thanks,

	Ingo