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

[Salil Mehta <salil.mehta@huawei.com>]

Hi Andrew,
Many thanks for the reply.

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Wednesday, June 3, 2020 10:38 AM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org; Peter Maydell
> <peter.maydell@linaro.org>; Igor Mammedov <imammedo@redhat.com>;
> mst@redhat.com
> Subject: Re: [Question] Regarding PMU initialization within the QEMU for ARM
> VCPUs
> 
> On Mon, Jun 01, 2020 at 03:04:33PM +0000, Salil Mehta wrote:
> > 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.
> >
> > Questions:
> > Q1. Not sure what is the logic of the premature exit and not
> >     continuing with further checks and initialization of other
> >     VCPU PMUs?
> 
> KVM requires all VCPUs to have a PMU if one does. If the ARM ARM
> says it's possible to have PMUs for only some CPUs, then, for TCG,
> the restriction could be relaxed. I expect it will take more than
> just removing the check for things to work though.

Got it. Many thanks for this info.

During virt machine init we take cpu type from (-cpu <cpu-type>)
option and it should apply evenly to all of the vcpus. Therefore,
I can assume all of the processors to be identical for now. This
combined with the KVM restriction you mentioned above means for
PMU we could only have Enable-for-All OR Enable-for-none config
for all of the vcpus being booted even though we at different
places do have per-vcpu specific check like below available

/* MADT */
static void
build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
[...]

    for (i = 0; i < vms->smp_cpus; i++) {
        AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                           sizeof(*gicc));
        [...]

        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {---> This check
            gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
        }
 [...]
}

Do per-vcpu feature check for PMU even makes sense till we allow
heterogeneous support of processors or relax the PMU enablement
on the per-vcpu basis within the KVM?



> 
> > Q2. Does it even makes sense to have PMUs initialized for some
> >     vcpus and not for others unless we have heterogeneous system?
> 
> I don't know, but it doesn't sound like a configuration I'd like
> to see.


sure. but in the existing code we do prematurely exit after we
discover first vcpu amongst the possible vcpus not supporting
PMU feature. This looks abnormal as well?


> 
> > 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?
> 
> It's purpose is to keep users from doing 'pmu=on' on 2.6 machine
> types. On 2.7 and later machine types if you don't want a PMU
> you should use 'pmu=off'.

sure. so by default on latest machines PMU is on. 

> 
> > 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?
> 
> Agreed. The function has the wrong name. mach-virt has many functions that
> mix the initialization and fdt building together, but those functions are
> named something like create_foo(). Patches welcome.


Will do. I have created one already. Will float soon.


> 
> Thanks,
> drew