Hi Drew, On 6/3/20 11:37 AM, Andrew Jones wrote: > 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.
I fail to find where this is enforced? Do you know the place? 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.> >> 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. > >> 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'. extra note: the cpu pmu property sets the feature at vcpu level. This is what is retrieved when (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) gets called. See the cpu option setter: arm_set_pmu in target/arm/cpu.c > >> 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. agreed Thanks Eric > > Thanks, > drew > >