RE: [PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest with possible vCPUs

[Salil Mehta]

Hi Gavin,

> From: Gavin Shan <gshan@redhat.com>
> Sent: Friday, September 29, 2023 12:44 AM
> To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; 
> qemu-arm@nongnu.org
> Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>; lpieralisi@kernel.org;
> peter.maydell@linaro.org; richard.henderson@linaro.org;
> imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com;
> philmd@linaro.org; eric.auger@redhat.com; will@kernel.org; ardb@kernel.org;
> oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com;
> rafael@kernel.org; borntraeger@linux.ibm.com; alex.bennee@linaro.org;
> linux@armlinux.org.uk; darren@os.amperecomputing.com;
> ilkka@os.amperecomputing.com; vishnu@os.amperecomputing.com;
> karl.heubaum@oracle.com; miguel.luis@oracle.com; salil.mehta@opnsrc.net;
> zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng (C)
> <wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>;
> jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn
> Subject: Re: [PATCH RFC V2 21/37] hw/arm: MADT Tbl change to size the guest
> with possible vCPUs
> 
> Hi Salil,
> 
> On 9/26/23 20:04, Salil Mehta wrote:
> > Changes required during building of MADT Table by QEMU to accommodate 
> > disabled
> > possible vCPUs. This info shall be used by the guest kernel to size up its
> > resources during boot time. This pre-sizing of the guest kernel done on
> > possible vCPUs will facilitate hotplug of the disabled vCPUs.
> >
> > This change also caters ACPI MADT GIC CPU Interface flag related changes
> > recently introduced in the UEFI ACPI 6.5 Specification which allows deferred
> > virtual CPU online'ing in the Guest Kernel.
> >
> > Link:
> https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#gic-cpu-interface-gicc-structure
> >
> > Co-developed-by: Salil Mehta <salil.mehta@huawei.com>
> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> > 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 | 36 ++++++++++++++++++++++++++++++------
> >   1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index d27df5030e..cbccd2ca2d 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -700,6 +700,29 @@ 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());
> > +
> > +    /* can only exist in 'enabled' state */
> > +    if (!mc->has_hotpluggable_cpus) {
> > +        return 1;
> > +    }
> > +
> > +    /*
> > +     * ARM GIC CPU Interface can be 'online-capable' or 'enabled' at boot
> > +     * We MUST set 'online-capable' Bit for all hotpluggable CPUs except 
> > the
>                                         ^^^
>                                         bit

:)

> > +     * first/boot CPU. Cold-booted CPUs without 'Id' can also be unplugged.
> > +     * Though as-of-now this is only used as a debugging feature.
> > +     *
> > +     *   UEFI ACPI Specification 6.5
> > +     *   Section: 5.2.12.14. GIC CPU Interface (GICC) Structure
> > +     *   Table:   5.37 GICC CPU Interface Flags
> > +     *   Link: https://uefi.org/specs/ACPI/6.5
> > +     */
> > +    return cpu && !cpu->cpu_index ? 1 : (1 << 3);
> > +}
> > +
> 
> I don't understand how a cold-booted CPU can be hot removed if it doesn't
> have a ID? Besides, how cpu->cpu_index is zero for the first cold-booted
> CPU?

Some cold-booted CPUs can be 'pluggable'. Hence, can have 'ID' specified
as part of the QEMU command line. This 'ID' can be used to hot(un)plug
later (if supported). 

You can also start QEMU with '-s' option which will pause the QEMU and
then you can cold-plug the CPUs during VM initialization time.

Good point about boot CPU.
But, it is a default assumption to have boot CPU as 0 on ARM - I think?

I will need to cross this part.

Thanks for pointing this though.

> 
> >   static void
> >   build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState
> *vms)
> >   {
> > @@ -726,12 +749,13 @@ 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 = qemu_get_possible_cpu(i);
> >           uint64_t physical_base_address = 0, gich = 0, gicv = 0;
> >           uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) :
> 0;
> > -        uint32_t pmu_interrupt = arm_feature(&armcpu->env, 
> > ARM_FEATURE_PMU) ?
> > -                                             PPI(VIRTUAL_PMU_IRQ) : 0;
> > +        uint32_t pmu_interrupt = vms->pmu ? PPI(VIRTUAL_PMU_IRQ) : 0;
> > +        uint32_t flags = virt_acpi_get_gicc_flags(cpu);
> > +        uint64_t mpidr = qemu_get_cpu_archid(i);
> >
> 
> qemu_get_cpu_archid() can be dropped since it's called for once. MPIDR
> can be fetched from ms->possible_cpus->cpus[i].arch_id, which has been
> initialized pre-hand.

I want expose this API to other parts of QEMU and other architectures as
well. It is an accessor API and should be encouraged all the time. It
reduces the unnecessary boiler plate code.

So would like to keep it but can move to board.h if you wish?


Thanks
Salil.