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
