On 10/16/2026 4:08 AM, Philippe Mathieu-Daudé wrote: > On 16/10/26 08:39, Pierrick Bouvier wrote: >> On 10/15/26 10:43 PM, Philippe Mathieu-Daudé wrote: >>> On 15/10/26 23:32, Pierrick Bouvier wrote: >>>> For now, we keep the global cpu_get_dump_info symbol defined. >>>> >>>> Signed-off-by: Pierrick Bouvier <[email protected]> >>>> --- >>>> target/i386/arch_dump.c | 17 ++++++++++++++--- >>>> target/i386/cpu-internal.h | 5 +++++ >>>> target/i386/cpu.c | 1 + >>>> 3 files changed, 20 insertions(+), 3 deletions(-) >>> >>> >>>> -int cpu_get_dump_info(ArchDumpInfo *info, >>>> +int x86_get_dump_info(const CPUState *cs, ArchDumpInfo *info, >>>> const GuestPhysBlockList *guest_phys_blocks) >>>> { >>>> bool lma = false; >>>> GuestPhysBlock *block; >>>> #ifdef TARGET_X86_64 >>>> - X86CPU *first_x86_cpu = X86_CPU(first_cpu); >>>> - lma = first_cpu && (first_x86_cpu->env.hflags & HF_LMA_MASK); >>> >>> I know this is pre-existing, but I'd like to understand this check, ... >>> >>>> + const X86CPU *cpu = X86_CPU(cs); >>>> + lma = !!(cpu->env.hflags & HF_LMA_MASK); >>>> #endif >>>> if (lma) { >>>> @@ -429,6 +430,16 @@ int cpu_get_dump_info(ArchDumpInfo *info, >>>> return 0; >>>> } >>>> +int cpu_get_dump_info(ArchDumpInfo *info, >>>> + const GuestPhysBlockList *guest_phys_blocks) >>>> +{ >>>> + if (first_cpu == NULL) { >>> >>> ... because you kept it. Why do we need it? QTest something? >>> >> >> It's here to ensure it doesn't segfault when dump is called for none >> machine: >> https://gitlab.com/qemu-project/qemu/-/commit/ >> fd5d23babf9838c1b099a9e8020e778aac0ebb4d >> >> It's a corner case, but doesn't hurt to keep it IMHO. > > I don't see it as a corner case but a bad API design. Why get called > if a machine doesn't have any CPU? The caller (dump_init in dump/dump.c) > should do the &first_cpu check, and either call with a valid CPU or not > call at all. The callees should abort for NULL vcpu (programming error). > Actually dump_init() already does the check by assigning nr_cpus. >
That's what we'll have now with this series: dump_init checks first_cpu is not NULL systematically now. (see patch 9). > Anyway your changes look correct since you pass a @cpu argument to the > methods added in SysemuCPUOps. > >>> (goal is to understand when this can be called on a non-vCPU thread) >>> >> >> I don't think it matters where this code runs, first_cpu is just the >> first cpu available and it can be accessed from any thread, vcpu or not. >> Now, the day we'll cleanup this TARGET_X86_64 guard, then it will be >> time to revisit this question. > > No need to revisit, add the target_heterogenenous() check (this code is > completely broken in this setup) then we are good / safe. Ok, will add this for v3. Regards, Pierrick
