RE: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs

[Salil Mehta]

Hello,
Any comments on this would be really helpful & much appreciated.


Thanks in anticipation!

Best regards
Salil
> -----Original Message-----
> From: Salil Mehta
> Sent: Monday, June 1, 2020 4:00 PM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: Peter Maydell <peter.maydell@linaro.org>; mst@redhat.com; Igor Mammedov
> <imammedo@redhat.com>
> Subject: [Question] Regarding PMU initialization within the QEMU for ARM VCPUs
> 
> Hello,
> I could see below within function fdt_add_pmu_nodes() part of
> hw/arm/virt.c during virt machine initialization time:
> 
> Observation:
> In below function, support of PMU feature is being checked for
> each vcpu and if the PMU is found part of the features then PMU
> is initialized with in the host/KVM. But if there is even one
> vcpu which is found to not support the PMU then loop is exited
> and PMU is not initialized for the rest of the vcpus as well.
> 
> static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> {
>     CPUState *cpu;
>     ARMCPU *armcpu;
>     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> 
>     CPU_FOREACH(cpu) {
>         armcpu = ARM_CPU(cpu);
>         if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>             return; ------> Here, loop exits & function returns
>         }
>         if (kvm_enabled()) {
>             if (kvm_irqchip_in_kernel()) {
>                 kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
>             }
>             kvm_arm_pmu_init(cpu);
>         }
>     }
> 
>     if (vms->gic_version == VIRT_GIC_VERSION_2) {
>         irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
>                              GIC_FDT_IRQ_PPI_CPU_WIDTH,
>                              (1 << vms->smp_cpus) - 1);
>     }
> 
>     armcpu = ARM_CPU(qemu_get_cpu(0));
>     qemu_fdt_add_subnode(vms->fdt, "/pmu");
>     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
>         const char compat[] = "arm,armv8-pmuv3";
>         qemu_fdt_setprop(vms->fdt, "/pmu", "compatible",
>                          compat, sizeof(compat));
>         qemu_fdt_setprop_cells(vms->fdt, "/pmu", "interrupts",
>                                GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, 
> irqflags);
>     }
> }
> 
> Questions:
> Q1. Not sure what is the logic of the premature exit and not
>     continuing with further checks and initialization of other
>     VCPU PMUs?
> Q2. Does it even makes sense to have PMUs initialized for some
>     vcpus and not for others unless we have heterogeneous system?
> Q3. Also, there is a per virt machine knob of vcc->no_pmu.
>     This is something which user could specify at the init time
>     and perhaps only once but we don't use it for ARM. Perhaps
>     should have been used even before entering this function
>     to enable or disable the support as per user config?
> Q4. This function  fdt_* looks to be wrongly named. The info
>     being initialized here shall be used even when ACPI is
>     being used. Initialization part and FDT info looked
>     mixed up here if I am right?
> 
> 
> Best regards
> Salil
>