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

[Kai Huang]

On Wed, 2022-06-29 at 07:22 -0700, Dave Hansen wrote:
> 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 purpose is not to avoid warning, but to make intel_cc_platform_has(enum
cc_attr attr) simple that when neither TDX host and TDX guest code is turned on,
it can be simple:

	static bool  intel_cc_platform_has(enum cc_attr attr)
	{
		return false;
	}

So I don't need to depend on how internal functions are implemented in the
header files and I don't need to guess how does compiler generate code.

And also because I personally believe it doesn't hurt readability. 

> 
> 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?

You mean compile it again?  I have to confess I never tried and don't know. 
I'll try when I got some spare time.  Thanks for the info.

> 
> > +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.

Yes I understand the purpose of this rule. Thanks for explaining.
 
> 
> 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?

Thanks for the tip.  Will do.

Btw this patch is the only one in this series that has this #ifdef problem, and
it will be gone in next version based on feedbacks that I received.  But I'll
check again.

-- 
Thanks,
-Kai