Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <292182cbe779aade47580ac23dc304856619c799.camel@intel.com>
Subject: Re: [PATCH v5 04/22] x86/virt/tdx: Prevent ACPI CPU hotplug and
 ACPI memory hotplug
From:   Kai Huang <kai.huang@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>, Chao Gao <chao.gao@intel.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        seanjc@google.com, pbonzini@redhat.com, len.brown@intel.com,
        tony.luck@intel.com, rafael.j.wysocki@intel.com,
        reinette.chatre@intel.com, dan.j.williams@intel.com,
        peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        isaku.yamahata@intel.com, thomas.lendacky@amd.com,
        Tianyu.Lan@microsoft.com
Date:   Thu, 30 Jun 2022 11:02:31 +1200
In-Reply-To: <a2277c2f-91a1-871f-08f1-42950bca53b3@intel.com>
References: <cover.1655894131.git.kai.huang@intel.com>
         <3a1c9807d8c140bdd550cd5736664f86782cca64.1655894131.git.kai.huang@intel.com>
         <20220624014112.GA15566@gao-cwp>
         <951da5eeb4214521635602ce3564246ad49018f5.camel@intel.com>
         <a2277c2f-91a1-871f-08f1-42950bca53b3@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

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.
>=20
> It has problems.
>=20
> Let's go through some of them.  First, this:
>=20
> > +#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
>=20
> 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 turne=
d 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.=20

>=20
> 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.
>=20
> static void func(void)
> {
> 	printf("I am func()\n");
> }
>=20
> void main(int argc, char **argv)
> {
> 	if (0)
> 		func();
> }
>=20
> Then, do:
>=20
> -	if (0)
> +	if (argc)
>=20
> 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.=
=20
I'll try when I got some spare time.  Thanks for the info.

>=20
> > +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
>=20
> 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
>=20
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > +	if (platform_tdx_enabled())
> > +		return intel_tdx_host_has(attr);
> > +#endif
> > +	return false;
> > +}
>=20
> Now, let's turn our attention to platform_tdx_enabled().  Here's its
> stub and declaration:
>=20
> > +#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 */
>=20
> It already has an #ifdef CONFIG_INTEL_TDX_HOST, so that #ifdef can just
> go away.
>=20
> Kai, the reason that we have the rule that Yuan cited:
>=20
> > "Wherever possible, don't use preprocessor conditionals (#if, #ifdef) i=
n .c"
> > From Documentation/process/coding-style.rst, 21) Conditional Compilatio=
n.
>=20
> 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.
=20
>=20
> 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.

--=20
Thanks,
-Kai