| Message ID | c4ab709b684bf6505a9721163564d2223d06c49d.1596768588.git.haibo.xu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Add vSPE support to KVM guest | expand |
Hi Haibo, On 8/7/20 10:10 AM, Haibo Xu wrote: > Adds a spe=[on/off] option to enable/disable vSPE support in > guest vCPU. Note this option is only available for "-cpu host" > with KVM mode, and default value is on. > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > --- > target/arm/cpu.c | 28 ++++++++++++++++++++++++++++ > target/arm/cpu.h | 3 +++ > 2 files changed, 31 insertions(+) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 111579554f..40768b4d19 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp) > cpu->has_pmu = value; > } > > +static bool arm_get_spe(Object *obj, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + return cpu->has_spe; > +} > + > +static void arm_set_spe(Object *obj, bool value, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + if (value) { > + if (kvm_enabled() && !kvm_arm_spe_supported()) { > + error_setg(errp, "'spe' feature not supported by KVM on this host"); > + return; > + } > + set_feature(&cpu->env, ARM_FEATURE_SPE); > + } else { > + unset_feature(&cpu->env, ARM_FEATURE_SPE); > + } > + cpu->has_spe = value; > +} > + > unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > { > /* > @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj) > object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); > } > > + if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) { > + cpu->has_spe = true; Being a profiling feature, are you sure you want it to be ON by default? I'd expect the opposite, either being turned on via command line at startup or by a management layer at runtime, when someone is ready to record the perf events and analyze them. > + object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe); > + } > + > /* > * Allow user to turn off VFP and Neon support, but only for TCG -- > * KVM does not currently allow us to lie to the guest about its > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 9e8ed423ea..fe0ac14386 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -822,6 +822,8 @@ struct ARMCPU { > bool has_el3; > /* CPU has PMU (Performance Monitor Unit) */ > bool has_pmu; > + /* CPU has SPE (Statistical Profiling Extension) */ > + bool has_spe; > /* CPU has VFP */ > bool has_vfp; > /* CPU has Neon */ > @@ -1959,6 +1961,7 @@ enum arm_features { > ARM_FEATURE_VBAR, /* has cp15 VBAR */ > ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ > ARM_FEATURE_M_MAIN, /* M profile Main Extension */ > + ARM_FEATURE_SPE, /* has SPE support */ > }; > > static inline int arm_feature(CPUARMState *env, int feature) >
On Fri, 7 Aug 2020 at 16:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Hi Haibo, > > On 8/7/20 10:10 AM, Haibo Xu wrote: > > Adds a spe=[on/off] option to enable/disable vSPE support in > > guest vCPU. Note this option is only available for "-cpu host" > > with KVM mode, and default value is on. > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > --- > > target/arm/cpu.c | 28 ++++++++++++++++++++++++++++ > > target/arm/cpu.h | 3 +++ > > 2 files changed, 31 insertions(+) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 111579554f..40768b4d19 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp) > > cpu->has_pmu = value; > > } > > > > +static bool arm_get_spe(Object *obj, Error **errp) > > +{ > > + ARMCPU *cpu = ARM_CPU(obj); > > + > > + return cpu->has_spe; > > +} > > + > > +static void arm_set_spe(Object *obj, bool value, Error **errp) > > +{ > > + ARMCPU *cpu = ARM_CPU(obj); > > + > > + if (value) { > > + if (kvm_enabled() && !kvm_arm_spe_supported()) { > > + error_setg(errp, "'spe' feature not supported by KVM on this host"); > > + return; > > + } > > + set_feature(&cpu->env, ARM_FEATURE_SPE); > > + } else { > > + unset_feature(&cpu->env, ARM_FEATURE_SPE); > > + } > > + cpu->has_spe = value; > > +} > > + > > unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > > { > > /* > > @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj) > > object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); > > } > > > > + if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) { > > + cpu->has_spe = true; > > Being a profiling feature, are you sure you want it to be ON by default? > > I'd expect the opposite, either being turned on via command line at > startup or by a management layer at runtime, when someone is ready > to record the perf events and analyze them. > I'm not sure whether it's proper to follow the 'pmu' setting here which has it on by default. To be honest, I also prefer to turn it off by default(Please refer to the comments in the cover letter). > > + object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe); > > + } > > + > > /* > > * Allow user to turn off VFP and Neon support, but only for TCG -- > > * KVM does not currently allow us to lie to the guest about its > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 9e8ed423ea..fe0ac14386 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -822,6 +822,8 @@ struct ARMCPU { > > bool has_el3; > > /* CPU has PMU (Performance Monitor Unit) */ > > bool has_pmu; > > + /* CPU has SPE (Statistical Profiling Extension) */ > > + bool has_spe; > > /* CPU has VFP */ > > bool has_vfp; > > /* CPU has Neon */ > > @@ -1959,6 +1961,7 @@ enum arm_features { > > ARM_FEATURE_VBAR, /* has cp15 VBAR */ > > ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ > > ARM_FEATURE_M_MAIN, /* M profile Main Extension */ > > + ARM_FEATURE_SPE, /* has SPE support */ > > }; > > > > static inline int arm_feature(CPUARMState *env, int feature) > > >
On 8/10/20 5:03 AM, Haibo Xu wrote: > On Fri, 7 Aug 2020 at 16:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Hi Haibo, >> >> On 8/7/20 10:10 AM, Haibo Xu wrote: >>> Adds a spe=[on/off] option to enable/disable vSPE support in >>> guest vCPU. Note this option is only available for "-cpu host" >>> with KVM mode, and default value is on. You are right, we don't know whether if the feature is announced to the guest, the guest will use the SPE registers, so similarly to PMU have it default ON if available. >>> >>> Signed-off-by: Haibo Xu <haibo.xu@linaro.org> >>> --- >>> target/arm/cpu.c | 28 ++++++++++++++++++++++++++++ >>> target/arm/cpu.h | 3 +++ >>> 2 files changed, 31 insertions(+) >>> >>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >>> index 111579554f..40768b4d19 100644 >>> --- a/target/arm/cpu.c >>> +++ b/target/arm/cpu.c >>> @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp) >>> cpu->has_pmu = value; >>> } >>> >>> +static bool arm_get_spe(Object *obj, Error **errp) >>> +{ >>> + ARMCPU *cpu = ARM_CPU(obj); >>> + >>> + return cpu->has_spe; >>> +} >>> + >>> +static void arm_set_spe(Object *obj, bool value, Error **errp) >>> +{ >>> + ARMCPU *cpu = ARM_CPU(obj); >>> + >>> + if (value) { >>> + if (kvm_enabled() && !kvm_arm_spe_supported()) { >>> + error_setg(errp, "'spe' feature not supported by KVM on this host"); >>> + return; >>> + } >>> + set_feature(&cpu->env, ARM_FEATURE_SPE); >>> + } else { >>> + unset_feature(&cpu->env, ARM_FEATURE_SPE); >>> + } >>> + cpu->has_spe = value; >>> +} >>> + >>> unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) >>> { >>> /* >>> @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj) >>> object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); >>> } >>> >>> + if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) { >>> + cpu->has_spe = true; >> >> Being a profiling feature, are you sure you want it to be ON by default? >> >> I'd expect the opposite, either being turned on via command line at >> startup or by a management layer at runtime, when someone is ready >> to record the perf events and analyze them. >> > > I'm not sure whether it's proper to follow the 'pmu' setting here > which has it on by default. > To be honest, I also prefer to turn it off by default(Please refer to > the comments in the cover letter). > >>> + object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe); >>> + } >>> + >>> /* >>> * Allow user to turn off VFP and Neon support, but only for TCG -- >>> * KVM does not currently allow us to lie to the guest about its >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >>> index 9e8ed423ea..fe0ac14386 100644 >>> --- a/target/arm/cpu.h >>> +++ b/target/arm/cpu.h >>> @@ -822,6 +822,8 @@ struct ARMCPU { >>> bool has_el3; >>> /* CPU has PMU (Performance Monitor Unit) */ >>> bool has_pmu; >>> + /* CPU has SPE (Statistical Profiling Extension) */ >>> + bool has_spe; >>> /* CPU has VFP */ >>> bool has_vfp; >>> /* CPU has Neon */ >>> @@ -1959,6 +1961,7 @@ enum arm_features { >>> ARM_FEATURE_VBAR, /* has cp15 VBAR */ >>> ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ >>> ARM_FEATURE_M_MAIN, /* M profile Main Extension */ >>> + ARM_FEATURE_SPE, /* has SPE support */ >>> }; >>> >>> static inline int arm_feature(CPUARMState *env, int feature) >>> >> >
On Fri, Aug 07, 2020 at 08:10:33AM +0000, Haibo Xu wrote: > Adds a spe=[on/off] option to enable/disable vSPE support in > guest vCPU. Note this option is only available for "-cpu host" > with KVM mode, and default value is on. > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > --- > target/arm/cpu.c | 28 ++++++++++++++++++++++++++++ > target/arm/cpu.h | 3 +++ > 2 files changed, 31 insertions(+) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 111579554f..40768b4d19 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp) > cpu->has_pmu = value; > } > > +static bool arm_get_spe(Object *obj, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + return cpu->has_spe; > +} > + > +static void arm_set_spe(Object *obj, bool value, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + if (value) { > + if (kvm_enabled() && !kvm_arm_spe_supported()) { > + error_setg(errp, "'spe' feature not supported by KVM on this host"); > + return; > + } > + set_feature(&cpu->env, ARM_FEATURE_SPE); > + } else { > + unset_feature(&cpu->env, ARM_FEATURE_SPE); See below, we should be using ID register bits instead. > + } > + cpu->has_spe = value; > +} > + > unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > { > /* > @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj) > object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); > } > > + if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) { > + cpu->has_spe = true; > + object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe); The property should be available even when the host doesn't support the feature. How else can one migrate from a host where the feature is available, but disabled, to a host that doesn't support the feature? > + } > + > /* > * Allow user to turn off VFP and Neon support, but only for TCG -- > * KVM does not currently allow us to lie to the guest about its > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 9e8ed423ea..fe0ac14386 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -822,6 +822,8 @@ struct ARMCPU { > bool has_el3; > /* CPU has PMU (Performance Monitor Unit) */ > bool has_pmu; > + /* CPU has SPE (Statistical Profiling Extension) */ > + bool has_spe; > /* CPU has VFP */ > bool has_vfp; > /* CPU has Neon */ > @@ -1959,6 +1961,7 @@ enum arm_features { > ARM_FEATURE_VBAR, /* has cp15 VBAR */ > ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ > ARM_FEATURE_M_MAIN, /* M profile Main Extension */ > + ARM_FEATURE_SPE, /* has SPE support */ We shouldn't need to add this feature bit. SPE should have an ID register bit to use instead. > }; > > static inline int arm_feature(CPUARMState *env, int feature) > -- > 2.17.1 > > Thanks, drew
On 8/10/20 3:50 AM, Andrew Jones wrote: >> @@ -1959,6 +1961,7 @@ enum arm_features { >> ARM_FEATURE_VBAR, /* has cp15 VBAR */ >> ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ >> ARM_FEATURE_M_MAIN, /* M profile Main Extension */ >> + ARM_FEATURE_SPE, /* has SPE support */ > > We shouldn't need to add this feature bit. SPE should have an ID register > bit to use instead. Yes indeed: ID_AA64DFR0_EL1.PMSVer. r~
On 8/7/20 1:10 AM, Haibo Xu wrote: > +static void arm_set_spe(Object *obj, bool value, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + if (value) { > + if (kvm_enabled() && !kvm_arm_spe_supported()) { > + error_setg(errp, "'spe' feature not supported by KVM on this host"); > + return; > + } > + set_feature(&cpu->env, ARM_FEATURE_SPE); > + } else { > + unset_feature(&cpu->env, ARM_FEATURE_SPE); > + } > + cpu->has_spe = value; > +} I think you want to simply set cpu->has_spe here, and leave the adjustment of ID_AA64DFR0 to a finalize routine. Because there are multiple values that PMSVer can take. Once the get/set routines are only setting a flag on ARMCPU, you can use a simpler property interface: static Property arm_cpu_spe_property = DEFINE_PROP_BOOL("spe", ARMCPU, has_spe, true); qdev_property_add_static(DEVICE(obj), &arm_cpu_spe_property); The finalize routine would be called from arm_cpu_finalize_features(), much like the existing arm_cpu_sve_finalize(). Since you're only registering the spe property when the selected cpu supports spe, the finalize routine only needs to set PMSVer to 0 to turn it off, preserving the initial enabled value of 1 or 2. r~
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 111579554f..40768b4d19 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp) cpu->has_pmu = value; } +static bool arm_get_spe(Object *obj, Error **errp) +{ + ARMCPU *cpu = ARM_CPU(obj); + + return cpu->has_spe; +} + +static void arm_set_spe(Object *obj, bool value, Error **errp) +{ + ARMCPU *cpu = ARM_CPU(obj); + + if (value) { + if (kvm_enabled() && !kvm_arm_spe_supported()) { + error_setg(errp, "'spe' feature not supported by KVM on this host"); + return; + } + set_feature(&cpu->env, ARM_FEATURE_SPE); + } else { + unset_feature(&cpu->env, ARM_FEATURE_SPE); + } + cpu->has_spe = value; +} + unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) { /* @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj) object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); } + if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) { + cpu->has_spe = true; + object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe); + } + /* * Allow user to turn off VFP and Neon support, but only for TCG -- * KVM does not currently allow us to lie to the guest about its diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 9e8ed423ea..fe0ac14386 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -822,6 +822,8 @@ struct ARMCPU { bool has_el3; /* CPU has PMU (Performance Monitor Unit) */ bool has_pmu; + /* CPU has SPE (Statistical Profiling Extension) */ + bool has_spe; /* CPU has VFP */ bool has_vfp; /* CPU has Neon */ @@ -1959,6 +1961,7 @@ enum arm_features { ARM_FEATURE_VBAR, /* has cp15 VBAR */ ARM_FEATURE_M_SECURITY, /* M profile Security Extension */ ARM_FEATURE_M_MAIN, /* M profile Main Extension */ + ARM_FEATURE_SPE, /* has SPE support */ }; static inline int arm_feature(CPUARMState *env, int feature)
Adds a spe=[on/off] option to enable/disable vSPE support in guest vCPU. Note this option is only available for "-cpu host" with KVM mode, and default value is on. Signed-off-by: Haibo Xu <haibo.xu@linaro.org> --- target/arm/cpu.c | 28 ++++++++++++++++++++++++++++ target/arm/cpu.h | 3 +++ 2 files changed, 31 insertions(+) -- 2.17.1