Re: [RFC PATCH v3 6/9] hw/arm/virt-acpi-build: Use possible cpus in generation of MADT

[wangyanan (Y)]

Hi Drew,

On 2021/5/17 15:42, Andrew Jones wrote:
> On Sun, May 16, 2021 at 06:28:57PM +0800, Yanan Wang wrote:
> > When building ACPI tables regarding CPUs we should always build
> > them for the number of possible CPUs, not the number of present
> > CPUs. So we create gicc nodes in MADT for possible cpus and then
> > ensure only the present CPUs are marked ENABLED. Furthermore, it
> > also needed if we are going to support CPU hotplug in the future.
> >
> > Co-developed-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Co-developed-by: Ying Fang <fangying1@huawei.com>
> > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > Co-developed-by: Yanan Wang <wangyanan55@huawei.com>
> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> > ---
> >   hw/arm/virt-acpi-build.c | 29 +++++++++++++++++++++++++----
> >   1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index a2d8e87616..4d64aeb865 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -481,6 +481,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >       const int *irqmap = vms->irqmap;
> >       AcpiMadtGenericDistributor *gicd;
> >       AcpiMadtGenericMsiFrame *gic_msi;
> > +    MachineClass *mc = MACHINE_GET_CLASS(vms);
> > +    const CPUArchIdList *possible_cpus = 
> > mc->possible_cpu_arch_ids(MACHINE(vms));
> > +    bool pmu;
> >       int i;
> >   
> >       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
> > @@ -491,11 +494,21 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >       gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
> >       gicd->version = vms->gic_version;
> >   
> > -    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> > +    for (i = 0; i < possible_cpus->len; i++) {
> >           AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
> >                                                              sizeof(*gicc));
> >           ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> >   
> > +        /*
> > +         * PMU should have been either implemented for all CPUs or not,
> > +         * so we only get information from the first CPU, which could
> > +         * represent the others.
> > +         */
> > +        if (i == 0) {
> > +            pmu = arm_feature(&armcpu->env, ARM_FEATURE_PMU);
> > +        }
> > +        assert(!armcpu || arm_feature(&armcpu->env, ARM_FEATURE_PMU) == pmu);
> This doesn't belong in this patch. The commit message doesn't even mention
> it. Also, I don't think we should do this here at all. If we want to
> ensure that all cpus have a pmu when one does, then that should be done
> somewhere like machvirt_init(), not in ACPI generation code which doesn't
> even run for non-ACPI VMs.
Sorry, I should have stated the reason of this change in the commit message.
Actually code change here and mp_affinity part below aim to make it correct
to create gicc entries for all possible cpus.

We only initialize and realize cpuobj for present cpus in machvirt_init,
so that we will get null ARMCPU pointer here for the non-present cpus,
and consequently we won't able to check from "armcpu->env" for the
non-present cpus. The same about "armcpu->mp_affinity".

That's the reason I use PMU configuration of the first cpu to represent the
others. I assume all cpus should have a pmu when one does here since it's
how armcpu->env is initialized. And the assert seems not needed here.

Is there any better alternative way about this?
> > +
> >           gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
> >           gicc->length = sizeof(*gicc);
> >           if (vms->gic_version == 2) {
> > @@ -504,11 +517,19 @@ build_madt(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >               gicc->gicv_base_address = cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
> >           }
> >           gicc->cpu_interface_number = cpu_to_le32(i);
> > -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> > +        gicc->arm_mpidr = cpu_to_le64(possible_cpus->cpus[i].arch_id);
> Hmm, I think we may have a problem. I don't think there's any guarantee
> that possible_cpus->cpus[i].arch_id == armcpu->mp_affinity, because
> arch_id comes from virt_cpu_mp_affinity(), which is arm_cpu_mp_affinity,
> but with a variable cluster size, however mp_affinity comes from
> arm_cpu_mp_affinity with a set cluster size. Also, when KVM is used,
> then all bets are off as to what mp_affinity is.
Right! Arch_id is initialized by virt_cpu_mp_affinity() in machvirt and then
mp_affinity is initialized by arch_id. Here they two have the same value.

But mp_affinity will be overridden in kvm_arch_init_vcpu() when KVM is
enabled. Here they two won't have the same value.
> We need to add some code that ensures arch_id == mp_affinity,
Can we also update the arch_id at the same time when we change mp_affinity?
> and, for
> now, we should stick with mp_affinity, since, at least when KVM is used,
> that's the correct one.
I also prefer sticking with mp_affinity, if the problem I explain about 
ARMCPU
above can be perfectly solved.
> >           gicc->uid = cpu_to_le32(i);
> > -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> >   
> > -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> > +        /*
> > +         * ACPI spec says that LAPIC entry for non present CPU may be
> Why are we talking about LAPICs in a GICC generator?
Ah, sorry. This ought to be GICC entry. Will fix.

Thanks,
Yanan
> > +         * omitted from MADT or it must be marked as disabled. Here we
> > +         * choose to also keep the disabled ones in MADT.
> > +         */
> > +        if (possible_cpus->cpus[i].cpu != NULL) {
> > +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
> > +        }
> > +
> > +        if (pmu) {
> >               gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >           }
> >           if (vms->virt) {
> > --
> > 2.19.1
> Thanks,
> drew
>
> .