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

[Kai Huang]

On Thu, 2022-06-30 at 08:44 -0700, Dave Hansen wrote:
> On 6/29/22 16:02, Kai Huang wrote:
> > On Wed, 2022-06-29 at 07:22 -0700, Dave Hansen wrote:
> > > On 6/24/22 04:21, Kai Huang wrote:
> > > 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.
> 
> I hate to break it to you, but you actually need to know how the
> compiler works for you to be able to write good code.  Ignoring all the
> great stuff that the compiler does for you makes your code worse.

Agreed.

> 
> > And also because I personally believe it doesn't hurt readability. 
> 
> Are you saying that you're ignoring long-established kernel coding style
> conventions because of your personal beliefs?  That seem, um, like an
> approach that's unlikely to help your code get accepted.

Agreed.  Will keep this in mind.  Thanks.

> 
> > > 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.
> 
> Yes, compile it again and run it again.
> 
> But, seriously, it's a quick exercise.  I can help make you some spare
> time if you wish.  Just let me know.

So I tried.  Took me less than 5 mins:)

The
	if (0)
		func();

never generates the code to actually call the func():

0000000000401137 <main>: 
  401137:       55                      push   %rbp      
  401138:       48 89 e5                mov    %rsp,%rbp 
  40113b:       89 7d fc                mov    %edi,-0x4(%rbp)
  40113e:       48 89 75 f0             mov    %rsi,-0x10(%rbp)
  401142:       90                      nop
  401143:       5d                      pop    %rbp    
  401144:       c3                      ret    

While
	if (argc)
		func();

generates the code to check argc and call func():

0000000000401137 <main>: 
  401137:       55                      push   %rbp     
  401138:       48 89 e5                mov    %rsp,%rbp  
  40113b:       48 83 ec 10             sub    $0x10,%rsp
  40113f:       89 7d fc                mov    %edi,-0x4(%rbp)
  401142:       48 89 75 f0             mov    %rsi,-0x10(%rbp)
  401146:       83 7d fc 00             cmpl   $0x0,-0x4(%rbp)
  40114a:       74 05                   je     401151 <main+0x1a>
  40114c:       e8 d5 ff ff ff          call   401126 <func>
  401151:       90                      nop                      
  401152:       c9                      leave                
  401153:       c3                      ret   

This is kinda no surprise.

Were you trying to make point that

	if (false)
		func();

doesn't generate any additional code?

I get your point now.  Thanks :)