Re: [PATCH v5 04/22] x86/virt/tdx: Prevent ACPI CPU hotplug and ACPI memory hotplug

[Dave Hansen <dave.hansen@intel.com>]

On 6/24/22 04:21, Kai Huang wrote:
> Personally I don't quite like this way.  To me having separate function for host
> and guest is more clear and more flexible.  And I don't think having
> #ifdef/endif has any problem.  I would like to leave to maintainers.

It has problems.

Let's go through some of them.  First, this:

> +#ifdef CONFIG_INTEL_TDX_HOST
> +static bool intel_tdx_host_has(enum cc_attr attr)
> +{
> +	switch (attr) {
> +	case CC_ATTR_ACPI_CPU_HOTPLUG_DISABLED:
> +	case CC_ATTR_ACPI_MEMORY_HOTPLUG_DISABLED:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +#endif

What does that #ifdef get us?  I suspect you're back to trying to
silence compiler warnings with #ifdefs.  The compiler *knows* that it's
only used in this file.  It's also used all of once.  If you make it
'static inline', you'll likely get the same code generation, no
warnings, and don't need an #ifdef.

The other option is to totally lean on the compiler to figure things
out.  Compile this program, then disassemble it and see what main() does.

static void func(void)
{
	printf("I am func()\n");
}

void main(int argc, char **argv)
{
	if (0)
		func();
}

Then, do:

-	if (0)
+	if (argc)

and run it again.  What changed in the disassembly?

> +static bool intel_cc_platform_has(enum cc_attr attr)
> +{
> +#ifdef CONFIG_INTEL_TDX_GUEST
> +	if (boot_cpu_has(X86_FEATURE_TDX_GUEST))
> +		return intel_tdx_guest_has(attr);
> +#endif

Make this check cpu_feature_enabled(X86_FEATURE_TDX_GUEST).  That has an
#ifdef built in to it.  That gets rid of this #ifdef.  You have

> +#ifdef CONFIG_INTEL_TDX_HOST
> +	if (platform_tdx_enabled())
> +		return intel_tdx_host_has(attr);
> +#endif
> +	return false;
> +}

Now, let's turn our attention to platform_tdx_enabled().  Here's its
stub and declaration:

> +#ifdef CONFIG_INTEL_TDX_HOST
> +bool platform_tdx_enabled(void);
> +#else  /* !CONFIG_INTEL_TDX_HOST */
> +static inline bool platform_tdx_enabled(void) { return false; }
> +#endif /* CONFIG_INTEL_TDX_HOST */

It already has an #ifdef CONFIG_INTEL_TDX_HOST, so that #ifdef can just
go away.

Kai, the reason that we have the rule that Yuan cited:

> "Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c"
> From Documentation/process/coding-style.rst, 21) Conditional Compilation.

is not because there are *ZERO* #ifdefs in .c files.  It's because
#ifdefs in .c files hurt readability and are usually avoidable.  How do
you avoid them?  Well, you take a moment and look at the code and see
how other folks have made it readable.  It takes refactoring of code to
banish #ifdefs to headers or replace them with compiler constructs so
that the compiler can do the work behind the scenes.

Kai, could you please take the information I gave you in this message
and try to apply it across this series?  Heck, can you please take it
and use it to review others' code to make sure they don't encounter the
same pitfalls?