Re: [PATCH RFC V6 11/24] hw/arm/acpi: MADT change to size the guest with possible vCPUs

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 7 Oct 2025 11:34:48 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:

> Hi Igor,
> 
> > From: Igor Mammedov <imammedo@redhat.com>
> > Sent: Friday, October 3, 2025 4:09 PM
> > To: salil.mehta@opnsrc.net
> > 
> > On Wed,  1 Oct 2025 01:01:14 +0000
> > salil.mehta@opnsrc.net wrote:
> >   
> > > From: Salil Mehta <salil.mehta@huawei.com>
> > >
> > > When QEMU builds the MADT table, modifications are needed to include
> > > information about possible vCPUs that are exposed as ACPI-disabled (i.e.,  
> > `_STA.Enabled=0`).  
> > > This new information will help the guest kernel pre-size its resources
> > > during boot time. Pre-sizing based on possible vCPUs will facilitate
> > > the future hot-plugging of the currently disabled vCPUs.
> > >
> > > Additionally, this change addresses updates to the ACPI MADT GIC CPU
> > > interface flags, as introduced in the UEFI ACPI 6.5 specification [1].
> > > These updates enable deferred virtual CPU onlining in the guest kernel.
> > >
> > > Reference:
> > > [1] 5.2.12.14. GIC CPU Interface (GICC) Structure (Table 5.37 GICC CPU  
> > Interface Flags)  
> > >     Link:
> > >  
> > https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.h
> > tm  
> > > l#gic-cpu-interface-gicc-structure
> > >
> > > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 40 ++++++++++++++++++++++++++++++++++-  
> > -----  
> > >  hw/core/machine.c        | 14 ++++++++++++++
> > >  include/hw/boards.h      | 20 ++++++++++++++++++++
> > >  3 files changed, 68 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > > b01fc4f8ef..7c24dd6369 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -760,6 +760,32 @@ static void build_append_gicr(GArray *table_data,  
> > uint64_t base, uint32_t size)  
> > >      build_append_int_noprefix(table_data, size, 4); /* Discovery
> > > Range Length */  }
> > >
> > > +static uint32_t virt_acpi_get_gicc_flags(CPUState *cpu) {
> > > +    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> > > +    const uint32_t GICC_FLAG_ENABLED = BIT(0);
> > > +    const uint32_t GICC_FLAG_ONLINE_CAPABLE = BIT(3);
> > > +
> > > +    /* ARM architecture does not support vCPU hotplug yet */
> > > +    if (!cpu) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    /*
> > > +     * If the machine does not support online-capable CPUs, report the  
> > GICC as  
> > > +     * 'enabled' only.
> > > +     */
> > > +    if (!mc->has_online_capable_cpus) {
> > > +        return GICC_FLAG_ENABLED;
> > > +    }
> > > +
> > > +    /*
> > > +     * ACPI 6.5, 5.2.12.14 (GICC): mark the boot CPU 'enabled' and all others
> > > +     * 'online-capable'.
> > > +     */
> > > +    return (cpu == first_cpu) ? GICC_FLAG_ENABLED :
> > > +GICC_FLAG_ONLINE_CAPABLE; }
> > > +
> > >  static void
> > >  build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState
> > > *vms)  { @@ -785,12 +811,14 @@ build_madt(GArray *table_data,
> > > BIOSLinker *linker, VirtMachineState *vms)
> > >      build_append_int_noprefix(table_data, vms->gic_version, 1);
> > >      build_append_int_noprefix(table_data, 0, 3);   /* Reserved */
> > >
> > > -    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> > > -        ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> > > +    for (i = 0; i < MACHINE(vms)->smp.max_cpus; i++) {  
> >                                      ^^^^^^^^^^^^  
> > > +        CPUState *cpu = machine_get_possible_cpu(i);  
> > ...  
> > > +        CPUArchId *archid = machine_get_possible_cpu_arch_id(i);  
> > 
> > what complexity above adds? /and then you say creating instantiating ARM
> > VM is slow./
> > 
> > I'd drop machine_get_possible_cpu/machine_get_possible_cpu_arch_id
> > altogether and mimic what acpi_build_madt() does.  
> 
> 
> We can do that here but I need this function elsewhere in the monitor code as well
> to iterate over the possible CPUs and if I remember correctly I was getting compilation
> errors there. But I will check if this can be removed.
> 
> I would like to keep machine_get_possible_cpu().

if you did iteration with this helper over CPUs, you'd basically introducing
^2 complexity at that point.
But that's details, we will sort it out eventually.

> 
> I think you've misunderstood the reason of the boot time delay mentioned to you in RFC V5.
> It is because of the realization leg i.e. qdev_relaize(), of the vCPU and not because of this
> initialization leg

I did misunderstood wrt slow vcpus creation.
I did object to lazy creation in general, and well I still dislike it.
For more on this topic see my reply to cover letter, let continue discussion there
about that.

> 
> 
> >   
> > > +        uint32_t flags = virt_acpi_get_gicc_flags(cpu);
> > > +        uint64_t mpidr = archid->arch_id;
> > >
> > >          if (vms->gic_version == VIRT_GIC_VERSION_2) {
> > >              physical_base_address = memmap[VIRT_GIC_CPU].base; @@
> > > -805,7 +833,7 @@ build_madt(GArray *table_data, BIOSLinker *linker,  
> > VirtMachineState *vms)  
> > >          build_append_int_noprefix(table_data, i, 4);    /* GIC ID */
> > >          build_append_int_noprefix(table_data, i, 4);    /* ACPI Processor UID  
> > */  
> > >          /* Flags */
> > > -        build_append_int_noprefix(table_data, 1, 4);    /* Enabled */
> > > +        build_append_int_noprefix(table_data, flags, 4);
> > >          /* Parking Protocol Version */
> > >          build_append_int_noprefix(table_data, 0, 4);
> > >          /* Performance Interrupt GSIV */ @@ -819,7 +847,7 @@
> > > build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState  
> > *vms)  
> > >          build_append_int_noprefix(table_data, vgic_interrupt, 4);
> > >          build_append_int_noprefix(table_data, 0, 8);    /* GICR Base  
> > Address*/  
> > >          /* MPIDR */
> > > -        build_append_int_noprefix(table_data,  
> > arm_cpu_mp_affinity(armcpu), 8);  
> > > +        build_append_int_noprefix(table_data, mpidr, 8);
> > >          /* Processor Power Efficiency Class */
> > >          build_append_int_noprefix(table_data, 0, 1);
> > >          /* Reserved */
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > > 69d5632464..65388d859a 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -1383,6 +1383,20 @@ CPUState *machine_get_possible_cpu(int64_t  
> > cpu_index)  
> > >      return NULL;
> > >  }
> > >
> > > +CPUArchId *machine_get_possible_cpu_arch_id(int64_t cpu_index) {
> > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > > +    CPUArchIdList *possible_cpus = ms->possible_cpus;
> > > +
> > > +    for (int i = 0; i < possible_cpus->len; i++) {
> > > +        if (possible_cpus->cpus[i].cpu &&
> > > +            possible_cpus->cpus[i].cpu->cpu_index == cpu_index) {
> > > +            return &possible_cpus->cpus[i];
> > > +        }
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > >  static char *cpu_slot_to_string(const CPUArchId *cpu)  {
> > >      GString *s = g_string_new(NULL);
> > > diff --git a/include/hw/boards.h b/include/hw/boards.h index
> > > 3ff77a8b3a..fe51ca58bf 100644
> > > --- a/include/hw/boards.h
> > > +++ b/include/hw/boards.h
> > > @@ -461,6 +461,26 @@ struct MachineState {
> > >      bool acpi_spcr_enabled;
> > >  };
> > >
> > > +/*
> > > + * machine_get_possible_cpu_arch_id:
> > > + * @cpu_index: logical cpu_index to search for
> > > + *
> > > + * Return a pointer to the CPUArchId entry matching the given
> > > +@cpu_index
> > > + * in the current machine's MachineState. The possible_cpus array
> > > +holds
> > > + * the full set of CPUs that the machine could support, including
> > > +those
> > > + * that may be created as disabled or taken offline.
> > > + *
> > > + * The slot index in ms->possible_cpus[] is always sequential, but
> > > +the
> > > + * logical cpu_index values are assigned by QEMU and may or may not
> > > +be
> > > + * sequential depending on the implementation of a particular machine.
> > > + * Direct indexing by cpu_index is therefore unsafe in general. This
> > > + * helper performs a linear search of the possible_cpus array to find
> > > + * the matching entry.
> > > + *
> > > + * Returns: pointer to the matching CPUArchId, or NULL if not found.
> > > + */
> > > +CPUArchId *machine_get_possible_cpu_arch_id(int64_t cpu_index);
> > > +
> > >  /*
> > >   * The macros which follow are intended to facilitate the
> > >   * definition of versioned machine types, using a somewhat  
> >   
>