Re: [PATCH 2/5] x86/virt/tdx: Move TDMR metadata fields map table to local variable

From: "Huang, Kai" <kai.huang@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: "jgross@suse.com" <jgross@suse.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"x86@kernel.org" <x86@kernel.org>, "bp@alien8.de" <bp@alien8.de>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>
Subject: Re: [PATCH 2/5] x86/virt/tdx: Move TDMR metadata fields map table to local variable
Date: Mon, 6 May 2024 08:04:51 +0000	[thread overview]
Message-ID: <aeccabf25c5c4e5e721f0ee5a83892061a6b2d76.camel@intel.com> (raw)
In-Reply-To: <b78a383d-27bb-43e3-8e61-f8a6b25b2708@intel.com>

On Fri, 2024-05-03 at 09:01 -0700, Dave Hansen wrote:
> On 3/1/24 03:20, Kai Huang wrote:
> > The kernel reads all TDMR related global metadata fields based on a
> > table which maps the metadata fields to the corresponding members of
> > 'struct tdx_tdmr_sysinfo'.
> > 
> > Currently this table is a static variable.  But this table is only used
> > by the function which reads these metadata fields and becomes useless
> > after reading is done.
> 
> Is this intended to be a problem statement?  _How_ is this a problem?
> 
> > Change the table to function local variable.  This also saves the
> > storage of the table from the kernel image.
> 
> I'm confused how this would happen.  Could you please explain your logic
> a bit here?

I think I failed to notice one thing, that although this patch can
eliminate the (static) @fields[] array in the data section, it generates
more code in the function get_tdx_tdmr_sysinfo() in order to build the
same array in the stack.

I did experiment and compared the generated code with or without the code
change in this patch:

before:

	fields:
	        .quad   -7998392933915033592	/* metadata field ID */
	        .long   0
	        .zero   4
	        .quad   -7998392933915033591
	        .long   2
	        .zero   4
	        .quad   -7998392933915033584
	        .long   4
	        .zero   4
	        .quad   -7998392933915033583
	        .long   6
	        .zero   4
	        .quad   -7998392933915033582
	        .long   8
	        .zero   4
	get_tdx_tdmr_sysinfo:
	        pushq   %rbp
	        movq    %rsp, %rbp
	        subq    $24, %rsp
	        movq    %rdi, -24(%rbp)
	        movl    $0, -4(%rbp)
	        jmp     .L8

		......

after:

	get_tdx_tdmr_sysinfo:
	        pushq   %rbp
	        movq    %rsp, %rbp
	        subq    $112, %rsp
	        movq    %rdi, -104(%rbp)
	        movabsq $-7998392933915033592, %rax
	        movq    %rax, -96(%rbp)
	        movl    $0, -88(%rbp)
	        movabsq $-7998392933915033591, %rax
	        movq    %rax, -80(%rbp)
	        movl    $2, -72(%rbp)
	        movabsq $-7998392933915033584, %rax
	        movq    %rax, -64(%rbp)
	        movl    $4, -56(%rbp)
	        movabsq $-7998392933915033583, %rax
	        movq    %rax, -48(%rbp)
	        movl    $6, -40(%rbp)
	        movabsq $-7998392933915033582, %rax
	        movq    %rax, -32(%rbp)
	        movl    $8, -24(%rbp)
	        movl    $0, -4(%rbp)
	        jmp     .L8

		......

So looks we cannot assume moving the static array to function local
variable can always save the storage.

I think the point is the compiler has to keep those constants (metadata
field ID and offset) somewhere in the object file, no matter they are in
the data section or in the code in text section, and no matter how does
the compiler generate the code.

The more reasonable benefit of this patch is to make the name scope of the
@fields[] array be only visible in the get_tdx_tdmr_sysinfo() but not the
entire file.

Thanks for the insight.  I hope the above is all I missed, or am I still
missing anything?

Anyway as replied to Rick I'll drop this patch from this series.