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

["wangyanan (Y)" <wangyanan55@huawei.com>]

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
>
> .