Hi Andrew, Many thanks for the reply. > From: Andrew Jones [mailto:drjo...@redhat.com] > Sent: Wednesday, June 3, 2020 10:38 AM > To: Salil Mehta <salil.me...@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Peter Maydell > <peter.mayd...@linaro.org>; Igor Mammedov <imamm...@redhat.com>; > m...@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