On Wed, Jun 03, 2020 at 11:45:22AM +0000, Salil Mehta wrote: > 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?
It may not be necessary now or ever to test more than one CPU for the PMU feature, but without a good reason to change it to a machine property then I'd prefer we always to the N-1 pointless checks. The feature is a CPU feature, not a machine feature, so, IMO, it should remain something configured and tested at the CPU level, not the machine level. > > > > > > > > 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? Are you trying to configure heterogeneous mach-virt machines? Or machines that only provide PMUs to some CPUs? If not, then I'm not sure why this would be a problem. Indeed it's likely a pointless check and, instead of silently returning, it should output a warning or even assert. Otherwise, I don't see a problem with it, since we want to be sure we're dealing with the type of configuration we expect, i.e. one where each CPU has a PMU if any CPU has a PMU. Thanks, drew