Re: [Qemu-devel] [PATCH V4 1/2] arm64: Add an option to turn on/off vPMU support
On Wed, Sep 21, 2016 at 08:35:41AM -0500, Wei Huang wrote: > On 09/21/2016 02:53 AM, Andrew Jones wrote: > > On Tue, Sep 20, 2016 at 10:33:53PM -0400, Wei Huang wrote: [snip] > >> +if (cpu->has_pmu == ON_OFF_AUTO_ON && !kvm_enabled()) { > >> +cpu->has_pmu = ON_OFF_AUTO_OFF; > >> +if (!pmu_warned && !qtest_enabled()) { > > > > Why do we need this qtest_enabled thing? > > Without this, "make check" will print out the warning message (using a > qtest machine type). This seems to be a common practice across QEMU code. I see. Too bad it's so ugly. Maybe we need a new error_report that hides that ugliness, i.e. an error_report that only reports when !qtest_enabled(). CC'ing Markus to get his opinion, but I'm off topic now and wouldn't expect anything like that to be worked out before accepting this patch. > > > > >> +error_report("warning: pmu can't be enabled without KVM > >> acceleration"); > >> +pmu_warned = true; > >> +} > >> +} > >> +if (cpu->has_pmu == ON_OFF_AUTO_OFF) { > >> +unset_feature(env, ARM_FEATURE_PMU); > >> +} > >> + [snip] Thanks, drew
Re: [Qemu-devel] [PATCH V4 1/2] arm64: Add an option to turn on/off vPMU support
On 09/21/2016 02:53 AM, Andrew Jones wrote: > On Tue, Sep 20, 2016 at 10:33:53PM -0400, Wei Huang wrote: >> This patch adds a pmu=[on/off] option to enable/disable vPMU support >> in guest vCPU. This option is only available for cortex-a57/cortex-53/ >> host under both TCG and KVM modes, but unavailable on ARMv7 and other >> processors. It allows virt tools, such as libvirt, to determine the >> exsitence of vPMU and configure it. Note that, if nothing specified, >> the pmu option is set to AUTO as default, allowing machine-level PMU >> property to override it. Also when pmu is turned on under non-KVM mode, >> a warning message will be printed. >> >> Signed-off-by: Wei Huang >> --- >> hw/arm/virt-acpi-build.c | 2 +- >> hw/arm/virt.c| 2 +- >> target-arm/cpu.c | 23 +++ >> target-arm/cpu.h | 5 +++-- >> target-arm/cpu64.c | 2 ++ >> target-arm/kvm64.c | 17 ++--- >> 6 files changed, 44 insertions(+), 7 deletions(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 295ec86..8b3083e 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, >> VirtGuestInfo *guest_info) >> gicc->uid = i; >> gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); >> >> -if (armcpu->has_pmu) { >> +if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { >> gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); >> } >> } >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a193b5a..a781ad0 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, >> int gictype) >> >> CPU_FOREACH(cpu) { >> armcpu = ARM_CPU(cpu); >> -if (!armcpu->has_pmu || >> +if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) || >> !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { >> return; >> } >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index ce8b8f4..ed7b884 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -19,6 +19,7 @@ >> */ >> >> #include "qemu/osdep.h" >> +#include "qemu/error-report.h" >> #include "qapi/error.h" >> #include "cpu.h" >> #include "internals.h" >> @@ -31,6 +32,7 @@ >> #include "hw/arm/arm.h" >> #include "sysemu/sysemu.h" >> #include "sysemu/kvm.h" >> +#include "sysemu/qtest.h" >> #include "kvm_arm.h" >> >> static void arm_cpu_set_pc(CPUState *cs, vaddr value) >> @@ -509,6 +511,10 @@ static Property arm_cpu_rvbar_property = >> static Property arm_cpu_has_el3_property = >> DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true); >> >> +/* use property name "pmu" to match other archs and virt tools */ >> +static Property arm_cpu_has_pmu_property = >> +DEFINE_PROP_ON_OFF_AUTO("pmu", ARMCPU, has_pmu, ON_OFF_AUTO_AUTO); >> + >> static Property arm_cpu_has_mpu_property = >> DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); >> >> @@ -552,6 +558,11 @@ static void arm_cpu_post_init(Object *obj) >> #endif >> } >> >> +if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) { >> +qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property, >> + &error_abort); >> +} >> + >> if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) { >> qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property, >> &error_abort); >> @@ -576,6 +587,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error >> **errp) >> ARMCPU *cpu = ARM_CPU(dev); >> ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); >> CPUARMState *env = &cpu->env; >> +static bool pmu_warned; >> >> /* Some features automatically imply others: */ >> if (arm_feature(env, ARM_FEATURE_V8)) { >> @@ -648,6 +660,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error >> **errp) >> cpu->id_aa64pfr0 &= ~0xf000; >> } >> >> +if (cpu->has_pmu == ON_OFF_AUTO_ON && !kvm_enabled()) { >> +cpu->has_pmu = ON_OFF_AUTO_OFF; >> +if (!pmu_warned && !qtest_enabled()) { > > Why do we need this qtest_enabled thing? Without this, "make check" will print out the warning message (using a qtest machine type). This seems to be a common practice across QEMU code. > >> +error_report("warning: pmu can't be enabled without KVM >> acceleration"); >> +pmu_warned = true; >> +} >> +} >> +if (cpu->has_pmu == ON_OFF_AUTO_OFF) { >> +unset_feature(env, ARM_FEATURE_PMU); >> +} >> + >> if (!arm_feature(env, ARM_FEATURE_EL2)) { >> /* Disable the hypervisor feature bits in the processor feature >> * registers if we don't have EL2. These are id_pfr1[15:12] and >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 76d824d..c8625eb 100644 >> --- a/tar
Re: [Qemu-devel] [PATCH V4 1/2] arm64: Add an option to turn on/off vPMU support
On Tue, Sep 20, 2016 at 10:33:53PM -0400, Wei Huang wrote: > This patch adds a pmu=[on/off] option to enable/disable vPMU support > in guest vCPU. This option is only available for cortex-a57/cortex-53/ > host under both TCG and KVM modes, but unavailable on ARMv7 and other > processors. It allows virt tools, such as libvirt, to determine the > exsitence of vPMU and configure it. Note that, if nothing specified, > the pmu option is set to AUTO as default, allowing machine-level PMU > property to override it. Also when pmu is turned on under non-KVM mode, > a warning message will be printed. > > Signed-off-by: Wei Huang > --- > hw/arm/virt-acpi-build.c | 2 +- > hw/arm/virt.c| 2 +- > target-arm/cpu.c | 23 +++ > target-arm/cpu.h | 5 +++-- > target-arm/cpu64.c | 2 ++ > target-arm/kvm64.c | 17 ++--- > 6 files changed, 44 insertions(+), 7 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 295ec86..8b3083e 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, > VirtGuestInfo *guest_info) > gicc->uid = i; > gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); > > -if (armcpu->has_pmu) { > +if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > } > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a193b5a..a781ad0 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, > int gictype) > > CPU_FOREACH(cpu) { > armcpu = ARM_CPU(cpu); > -if (!armcpu->has_pmu || > +if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) || > !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > return; > } > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index ce8b8f4..ed7b884 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -19,6 +19,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/error-report.h" > #include "qapi/error.h" > #include "cpu.h" > #include "internals.h" > @@ -31,6 +32,7 @@ > #include "hw/arm/arm.h" > #include "sysemu/sysemu.h" > #include "sysemu/kvm.h" > +#include "sysemu/qtest.h" > #include "kvm_arm.h" > > static void arm_cpu_set_pc(CPUState *cs, vaddr value) > @@ -509,6 +511,10 @@ static Property arm_cpu_rvbar_property = > static Property arm_cpu_has_el3_property = > DEFINE_PROP_BOOL("has_el3", ARMCPU, has_el3, true); > > +/* use property name "pmu" to match other archs and virt tools */ > +static Property arm_cpu_has_pmu_property = > +DEFINE_PROP_ON_OFF_AUTO("pmu", ARMCPU, has_pmu, ON_OFF_AUTO_AUTO); > + > static Property arm_cpu_has_mpu_property = > DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true); > > @@ -552,6 +558,11 @@ static void arm_cpu_post_init(Object *obj) > #endif > } > > +if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) { > +qdev_property_add_static(DEVICE(obj), &arm_cpu_has_pmu_property, > + &error_abort); > +} > + > if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) { > qdev_property_add_static(DEVICE(obj), &arm_cpu_has_mpu_property, > &error_abort); > @@ -576,6 +587,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error > **errp) > ARMCPU *cpu = ARM_CPU(dev); > ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); > CPUARMState *env = &cpu->env; > +static bool pmu_warned; > > /* Some features automatically imply others: */ > if (arm_feature(env, ARM_FEATURE_V8)) { > @@ -648,6 +660,17 @@ static void arm_cpu_realizefn(DeviceState *dev, Error > **errp) > cpu->id_aa64pfr0 &= ~0xf000; > } > > +if (cpu->has_pmu == ON_OFF_AUTO_ON && !kvm_enabled()) { > +cpu->has_pmu = ON_OFF_AUTO_OFF; > +if (!pmu_warned && !qtest_enabled()) { Why do we need this qtest_enabled thing? > +error_report("warning: pmu can't be enabled without KVM > acceleration"); > +pmu_warned = true; > +} > +} > +if (cpu->has_pmu == ON_OFF_AUTO_OFF) { > +unset_feature(env, ARM_FEATURE_PMU); > +} > + > if (!arm_feature(env, ARM_FEATURE_EL2)) { > /* Disable the hypervisor feature bits in the processor feature > * registers if we don't have EL2. These are id_pfr1[15:12] and > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 76d824d..c8625eb 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -579,8 +579,8 @@ struct ARMCPU { > bool powered_off; > /* CPU has security extension */ > bool has_el3; > -/* CPU has PMU (Performance Monitor Unit) */ > -bool has_pmu; > +/* CPU PMU (Performance Monitor Unit) config, tri-state */ The comment didn't