On Thu, Dec 05, 2024 at 04:05:22PM +0800, Xiaoyao Li wrote:
Date: Thu, 5 Dec 2024 16:05:22 +0800
From: Xiaoyao Li <xiaoyao.li@intel.com>
Subject: Re: [PATCH v1 0/4] Initialize nr_cores and nr_threads early and
related clearup
On 12/5/2024 3:30 PM, Zhao Liu wrote:
I'm also very sorry, but I have a slightly different opinion...
accel/tcg/user-exec-stub.c | 4 +++
hw/core/cpu-common.c | 2 +-
include/hw/core/cpu.h | 8 +++++
system/cpus.c | 6 +++-
target/alpha/cpu.c | 2 ++
target/arm/cpu.c | 2 ++
target/avr/cpu.c | 2 ++
target/hexagon/cpu.c | 2 ++
target/hppa/cpu.c | 2 ++
target/i386/cpu.c | 61 +++++++++++++++++++-------------------
target/loongarch/cpu.c | 2 ++
target/m68k/cpu.c | 2 ++
target/microblaze/cpu.c | 2 ++
target/mips/cpu.c | 2 ++
target/openrisc/cpu.c | 2 ++
target/ppc/cpu_init.c | 2 ++
target/riscv/cpu.c | 2 ++
target/rx/cpu.c | 2 ++
target/s390x/cpu.c | 2 ++
target/sh4/cpu.c | 2 ++
target/sparc/cpu.c | 2 ++
target/tricore/cpu.c | 2 ++
target/xtensa/cpu.c | 2 ++
23 files changed, 85 insertions(+), 32 deletions(-)
I have some doubts about the necessity of changing the initialization of
nr_cores/nr_threads, because you can access the machine's topology info
via machine_topo_get_threads_per_socket(), which gives the same result as
`nr_cores * nr_threads`.
yeah, it works. The goal is to maintain HT in env->features[].
The problem is, as Igor objects, accessing MachineState from CPU context.
This is what I'm working on to avoid for the next version.
:( I also noticed that patch. Adding #ifndef CONFIG_USER_ONLY is not
optimal there, so that approach is not general enough.
But your TDX case is different, TDX hook has been already under
`current_machine` context. So you can directly access any topo info
without CONFIG_USER_ONLY.
Especially, the TDX feature check hook is also within the context of
`current_machine`, so why not check if TDX's HT is consistent with QEMU's
emulation in the TDX hook?
For this reason, and based on my comment on patch 2, I think checking HT
in the TDX hook or even ignoring HT, would be a more straightforward and
less impactful solution.
Though it's motivated by TDX series, the goal is not TDX specific. The goal
is to track features in env->features[] instead of manually generating a
one-off value in cpu_x86_cpuid().
I agree, the principle of designing code should be like this. HT already
has some issues, and adding this check in x86_cpu_expand_features() is
also a one-off approach with very much effort. Checking HT in TDX is a
less costly and less impactful one-off solution.