Re: [PATCH v1 0/4] Initialize nr_cores and nr_threads early and related clearup

[Zhao Liu <zhao1.liu@intel.com>]

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.

Even, why not ignore HT in the TDX check? Isn't placing the TDX feature
check before build_cpuid() implicitly assuming that any subsequent
updates to CPUID by QEMU are valid?

Based on this, why can't TDX trust that QEMU will set HT correctly?
(However, this assumes that HT cannot be arbitrarily set by the user.)

I'm not sure how the next version will be, but it would definitely be
better if it helps with generalization.

Based on the current approach, affecting all architectures, it would be
better to directly obtain the topology information from current_machine
in TDX.

-Zhao