docs/system/arm/cpu-features.rst | 23 +++++++ target/arm/arm-qmp-cmds.c | 2 +- target/arm/cpu.h | 3 + target/arm/kvm.c | 112 +++++++++++++++++++++++++++++++ tests/qtest/arm-cpu-features.c | 51 ++++++++++++++ 5 files changed, 190 insertions(+), 1 deletion(-)
The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
which PMU events are provided to the guest. Add a new option
`kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
Without the filter, all PMU events are exposed from host to guest by
default. The usage of the new sub-option can be found from the updated
document (docs/system/arm/cpu-features.rst).
Here is an example which shows how to use the PMU Event Filtering, when
we launch a guest by use kvm, add such command line:
# qemu-system-aarch64 \
-accel kvm \
-cpu host,kvm-pmu-filter="D:0x11-0x11"
Since the first action is deny, we have a global allow policy. This
filters out the cycle counter (event 0x11 being CPU_CYCLES).
And then in guest, use the perf to count the cycle:
# perf stat sleep 1
Performance counter stats for 'sleep 1':
1.22 msec task-clock # 0.001 CPUs utilized
1 context-switches # 820.695 /sec
0 cpu-migrations # 0.000 /sec
55 page-faults # 45.138 K/sec
<not supported> cycles
1128954 instructions
227031 branches # 186.323 M/sec
8686 branch-misses # 3.83% of all branches
1.002492480 seconds time elapsed
0.001752000 seconds user
0.000000000 seconds sys
As we can see, the cycle counter has been disabled in the guest, but
other pmu events do still work.
Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
v8->v9:
- Replace the warn_report to error_setg in some places.
- Merge the check condition to make code more clean.
- Try to use the QAPI format for the PMU Filter property but failed to use it
since the -cpu option doesn't support json format yet.
v7->v8:
- Add qtest for kvm-pmu-filter.
- Do the kvm-pmu-filter syntax checking up-front in the kvm_pmu_filter_set()
function. And store the filter information at there. When kvm_pmu_filter_get()
reconstitute it.
v6->v7:
- Check return value of sscanf.
- Improve the check condition.
v5->v6:
- Commit message improvement.
- Remove some unused code.
- Collect Reviewed-by, thanks Sebastian.
- Use g_auto(Gstrv) to replace the gchar **. [Eric]
v4->v5:
- Change the kvm-pmu-filter as a -cpu sub-option. [Eric]
- Comment tweak. [Gavin]
- Rebase to the latest branch.
v3->v4:
- Fix the wrong check for pmu_filter_init. [Sebastian]
- Fix multiple alignment issue. [Gavin]
- Report error by warn_report() instead of error_report(), and don't use
abort() since the PMU Event Filter is an add-on and best-effort feature.
[Gavin]
- Add several missing { } for single line of code. [Gavin]
- Use the g_strsplit() to replace strtok(). [Gavin]
v2->v3:
- Improve commits message, use kernel doc wording, add more explaination on
filter example, fix some typo error. [Eric]
- Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
- Add more precise error message report. [Eric]
- In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support in
KVM. [Eric]
v1->v2:
- Add more description for allow and deny meaning in
commit message. [Sebastian]
- Small improvement. [Sebastian]
---
docs/system/arm/cpu-features.rst | 23 +++++++
target/arm/arm-qmp-cmds.c | 2 +-
target/arm/cpu.h | 3 +
target/arm/kvm.c | 112 +++++++++++++++++++++++++++++++
tests/qtest/arm-cpu-features.c | 51 ++++++++++++++
5 files changed, 190 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index a5fb929243..f3930f34b3 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -204,6 +204,29 @@ the list of KVM VCPU features and their descriptions.
the guest scheduler behavior and/or be exposed to the guest
userspace.
+``kvm-pmu-filter``
+ By default kvm-pmu-filter is disabled. This means that by default all PMU
+ events will be exposed to guest.
+
+ KVM implements PMU Event Filtering to prevent a guest from being able to
+ sample certain events. It depends on the KVM_ARM_VCPU_PMU_V3_FILTER
+ attribute supported in KVM. It has the following format:
+
+ kvm-pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
+
+ The A means "allow" and D means "deny", start is the first event of the
+ range and the end is the last one. The first registered range defines
+ the global policy (global ALLOW if the first action is DENY, global DENY
+ if the first action is ALLOW). The start and end only support hexadecimal
+ format. For example:
+
+ kvm-pmu-filter="A:0x11-0x11;A:0x23-0x3a;D:0x30-0x30"
+
+ Since the first action is allow, we have a global deny policy. It
+ will allow event 0x11 (the cycle counter), events 0x23 to 0x3a are
+ also allowed except the event 0x30 which is denied, and all the other
+ events are denied.
+
TCG VCPU Features
=================
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 3cc8cc738b..6ec1d3ea3c 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -93,7 +93,7 @@ static const char *cpu_model_advertised_features[] = {
"sve128", "sve256", "sve384", "sve512",
"sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
"sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
- "kvm-no-adjvtime", "kvm-steal-time",
+ "kvm-no-adjvtime", "kvm-steal-time", "kvm-pmu-filter",
"pauth", "pauth-impdef", "pauth-qarma3",
NULL
};
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index bc0c84873f..996754a9a7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -943,6 +943,9 @@ struct ArchCPU {
/* KVM steal time */
OnOffAuto kvm_steal_time;
+
+ /* KVM PMU Filter */
+ GArray *kvm_pmu_filter;
#endif /* CONFIG_KVM */
/* Uniprocessor system with MP extensions */
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index ab85d628a8..7a363131fe 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -496,6 +496,68 @@ static void kvm_steal_time_set(Object *obj, bool value, Error **errp)
ARM_CPU(obj)->kvm_steal_time = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
+static char *kvm_pmu_filter_get(Object *obj, Error **errp)
+{
+ ARMCPU *cpu = ARM_CPU(obj);
+ g_autoptr(GString) pmu_filter = g_string_new(NULL);
+ struct kvm_pmu_event_filter *filter;
+ char action;
+ int i;
+
+ if (!cpu->kvm_pmu_filter) {
+ return NULL;
+ }
+
+ for (i = 0; i < cpu->kvm_pmu_filter->len; i++) {
+ filter = &g_array_index(cpu->kvm_pmu_filter,
+ struct kvm_pmu_event_filter, i);
+ if (i) {
+ g_string_append_c(pmu_filter, ';');
+ }
+ action = filter->action == KVM_PMU_EVENT_ALLOW ? 'A' : 'D';
+ g_string_append_printf(pmu_filter, "%c:0x%hx-0x%hx", action,
+ filter->base_event,
+ filter->base_event + filter->nevents - 1);
+ }
+
+ return g_strdup(pmu_filter->str);
+}
+
+static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
+ Error **errp)
+{
+ ARMCPU *cpu = ARM_CPU(obj);
+ struct kvm_pmu_event_filter filter;
+ g_auto(GStrv) event_filters;
+ int i;
+
+ if (cpu->kvm_pmu_filter) {
+ g_array_free(cpu->kvm_pmu_filter, true);
+ }
+
+ cpu->kvm_pmu_filter = g_array_new(false, false,
+ sizeof(struct kvm_pmu_event_filter));
+
+ event_filters = g_strsplit(pmu_filter, ";", -1);
+ for (i = 0; event_filters[i]; i++) {
+ unsigned short start = 0, end = 0;
+ char act;
+
+ if (sscanf(event_filters[i], "%c:%hx-%hx", &act, &start, &end) != 3 ||
+ (act != 'A' && act != 'D') || start > end) {
+ error_setg(errp, "Invalid PMU filter %s", event_filters[i]);
+ return;
+ }
+
+ filter.base_event = start;
+ filter.nevents = end - start + 1;
+ filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
+ KVM_PMU_EVENT_DENY;
+
+ g_array_append_vals(cpu->kvm_pmu_filter, &filter, 1);
+ }
+}
+
/* KVM VCPU properties should be prefixed with "kvm-". */
void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
{
@@ -517,6 +579,12 @@ void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
kvm_steal_time_set);
object_property_set_description(obj, "kvm-steal-time",
"Set off to disable KVM steal time.");
+
+ object_property_add_str(obj, "kvm-pmu-filter", kvm_pmu_filter_get,
+ kvm_pmu_filter_set);
+ object_property_set_description(obj, "kvm-pmu-filter",
+ "PMU Event Filtering description for "
+ "guest PMU. (default: NULL, disabled)");
}
bool kvm_arm_pmu_supported(void)
@@ -1706,6 +1774,48 @@ static bool kvm_arm_set_device_attr(ARMCPU *cpu, struct kvm_device_attr *attr,
return true;
}
+static void kvm_arm_pmu_filter_init(ARMCPU *cpu)
+{
+ static bool pmu_filter_init;
+ struct kvm_device_attr attr = {
+ .group = KVM_ARM_VCPU_PMU_V3_CTRL,
+ .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
+ };
+ int i;
+
+ /*
+ * The filter only needs to be initialized through one vcpu ioctl and it
+ * will affect all other vcpu in the vm.
+ * It can be referred from kernel commit d7eec2360e3 ("KVM: arm64: Add PMU
+ * event filtering infrastructure"):
+ * Although the ioctl is per-vcpu, the map of allowed events is global to
+ * the VM (and can be setup from any vcpu until the vcpu PMU is
+ * initialized).
+ */
+ if (pmu_filter_init) {
+ return;
+ } else {
+ pmu_filter_init = true;
+ }
+
+ if (!cpu->kvm_pmu_filter) {
+ return;
+ }
+ if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
+ error_report("KVM doesn't support the PMU Event Filter!");
+ return;
+ }
+
+ for (i = 0; i < cpu->kvm_pmu_filter->len; i++) {
+ attr.addr = (uint64_t)&g_array_index(cpu->kvm_pmu_filter,
+ struct kvm_pmu_event_filter, i);
+ if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
+ error_report("KVM set the PMU Event Filter failed!");
+ break;
+ }
+ }
+}
+
void kvm_arm_pmu_init(ARMCPU *cpu)
{
struct kvm_device_attr attr = {
@@ -1716,6 +1826,8 @@ void kvm_arm_pmu_init(ARMCPU *cpu)
if (!cpu->has_pmu) {
return;
}
+
+ kvm_arm_pmu_filter_init(cpu);
if (!kvm_arm_set_device_attr(cpu, &attr, "PMU")) {
error_report("failed to init PMU");
abort();
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 9d6e6190d5..0b99939af3 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -127,6 +127,17 @@ static bool resp_get_feature(QDict *resp, const char *feature)
return qdict_get_bool(props, feature);
}
+static const char *resp_get_feature_str(QDict *resp, const char *feature)
+{
+ QDict *props;
+
+ g_assert(resp);
+ g_assert(resp_has_props(resp));
+ props = resp_get_props(resp);
+ g_assert(qdict_get(props, feature));
+ return qdict_get_str(props, feature);
+}
+
#define assert_has_feature(qts, cpu_type, feature) \
({ \
QDict *_resp = do_query_no_props(qts, cpu_type); \
@@ -156,6 +167,18 @@ static bool resp_get_feature(QDict *resp, const char *feature)
g_assert(qdict_get_bool(_props, feature) == (expected_value)); \
})
+#define resp_assert_feature_str(resp, feature, expected_value) \
+({ \
+ QDict *_props; \
+ \
+ g_assert(_resp); \
+ g_assert(resp_has_props(_resp)); \
+ _props = resp_get_props(_resp); \
+ g_assert(qdict_get(_props, feature)); \
+ g_assert_cmpstr(qdict_get_str(_props, feature), \
+ ==, (expected_value)); \
+})
+
#define assert_feature(qts, cpu_type, feature, expected_value) \
({ \
QDict *_resp; \
@@ -177,6 +200,17 @@ static bool resp_get_feature(QDict *resp, const char *feature)
qobject_unref(_resp); \
})
+#define assert_set_feature_str(qts, cpu_type, feature, value) \
+({ \
+ const char *_fmt = "{ %s: %s }"; \
+ QDict *_resp; \
+ \
+ _resp = do_query(qts, cpu_type, _fmt, feature, value); \
+ g_assert(_resp); \
+ resp_assert_feature_str(_resp, feature, value); \
+ qobject_unref(_resp); \
+})
+
#define assert_has_feature_enabled(qts, cpu_type, feature) \
assert_feature(qts, cpu_type, feature, true)
@@ -462,6 +496,7 @@ static void test_query_cpu_model_expansion(const void *data)
assert_has_not_feature(qts, "max", "kvm-no-adjvtime");
assert_has_not_feature(qts, "max", "kvm-steal-time");
+ assert_has_not_feature(qts, "max", "kvm-pmu-filter");
if (g_str_equal(qtest_get_arch(), "aarch64")) {
assert_has_feature_enabled(qts, "max", "aarch64");
@@ -509,6 +544,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
assert_set_feature(qts, "host", "kvm-no-adjvtime", false);
if (g_str_equal(qtest_get_arch(), "aarch64")) {
+ const char *kvm_supports_pmu_filter;
bool kvm_supports_steal_time;
bool kvm_supports_sve;
char max_name[8], name[8];
@@ -547,15 +583,29 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
* because this instance of KVM doesn't support them. Test that the
* features are present, and, when enabled, issue further tests.
*/
+ assert_has_feature(qts, "host", "kvm-pmu-filter");
assert_has_feature(qts, "host", "kvm-steal-time");
assert_has_feature(qts, "host", "sve");
resp = do_query_no_props(qts, "host");
+ kvm_supports_pmu_filter = resp_get_feature_str(resp, "kvm-pmu-filter");
kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
kvm_supports_sve = resp_get_feature(resp, "sve");
vls = resp_get_sve_vls(resp);
qobject_unref(resp);
+ if (kvm_supports_pmu_filter) {
+ assert_set_feature_str(qts, "host", "kvm-pmu-filter", "");
+ assert_set_feature_str(qts, "host", "kvm-pmu-filter",
+ "A:0x11-0x11");
+ assert_set_feature_str(qts, "host", "kvm-pmu-filter",
+ "D:0x11-0x11");
+ assert_set_feature_str(qts, "host", "kvm-pmu-filter",
+ "A:0x11-0x11;A:0x12-0x20");
+ assert_set_feature_str(qts, "host", "kvm-pmu-filter",
+ "D:0x11-0x11;A:0x12-0x20;D:0x12-0x15");
+ }
+
if (kvm_supports_steal_time) {
/* If we have steal-time then we should be able to toggle it. */
assert_set_feature(qts, "host", "kvm-steal-time", false);
@@ -623,6 +673,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
assert_has_not_feature(qts, "host", "pmu");
assert_has_not_feature(qts, "host", "sve");
assert_has_not_feature(qts, "host", "kvm-steal-time");
+ assert_has_not_feature(qts, "host", "kvm-pmu-filter");
}
qtest_quit(qts);
--
2.40.1On Mon, Apr 08, 2024 at 10:49:40PM -0400, Shaoqin Huang wrote: > The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide > which PMU events are provided to the guest. Add a new option > `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering. > Without the filter, all PMU events are exposed from host to guest by > default. The usage of the new sub-option can be found from the updated > document (docs/system/arm/cpu-features.rst). > > Here is an example which shows how to use the PMU Event Filtering, when > we launch a guest by use kvm, add such command line: > > # qemu-system-aarch64 \ > -accel kvm \ > -cpu host,kvm-pmu-filter="D:0x11-0x11" I'm still against implementing this one-off custom parsed syntax for kvm-pmu-filter values. Once this syntax exists, we're locked into back-compatibility for multiple releases, and it will make a conversion to QAPI/JSON harder. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 09/04/2024 04.49, Shaoqin Huang wrote:
> The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
> which PMU events are provided to the guest. Add a new option
> `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
> Without the filter, all PMU events are exposed from host to guest by
> default. The usage of the new sub-option can be found from the updated
> document (docs/system/arm/cpu-features.rst).
>
> Here is an example which shows how to use the PMU Event Filtering, when
> we launch a guest by use kvm, add such command line:
>
> # qemu-system-aarch64 \
> -accel kvm \
> -cpu host,kvm-pmu-filter="D:0x11-0x11"
>
> Since the first action is deny, we have a global allow policy. This
> filters out the cycle counter (event 0x11 being CPU_CYCLES).
>
> And then in guest, use the perf to count the cycle:
>
> # perf stat sleep 1
>
> Performance counter stats for 'sleep 1':
>
> 1.22 msec task-clock # 0.001 CPUs utilized
> 1 context-switches # 820.695 /sec
> 0 cpu-migrations # 0.000 /sec
> 55 page-faults # 45.138 K/sec
> <not supported> cycles
> 1128954 instructions
> 227031 branches # 186.323 M/sec
> 8686 branch-misses # 3.83% of all branches
>
> 1.002492480 seconds time elapsed
>
> 0.001752000 seconds user
> 0.000000000 seconds sys
>
> As we can see, the cycle counter has been disabled in the guest, but
> other pmu events do still work.
>
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
> v8->v9:
> - Replace the warn_report to error_setg in some places.
> - Merge the check condition to make code more clean.
> - Try to use the QAPI format for the PMU Filter property but failed to use it
> since the -cpu option doesn't support json format yet.
>
> v7->v8:
> - Add qtest for kvm-pmu-filter.
> - Do the kvm-pmu-filter syntax checking up-front in the kvm_pmu_filter_set()
> function. And store the filter information at there. When kvm_pmu_filter_get()
> reconstitute it.
>
> v6->v7:
> - Check return value of sscanf.
> - Improve the check condition.
>
> v5->v6:
> - Commit message improvement.
> - Remove some unused code.
> - Collect Reviewed-by, thanks Sebastian.
> - Use g_auto(Gstrv) to replace the gchar **. [Eric]
>
> v4->v5:
> - Change the kvm-pmu-filter as a -cpu sub-option. [Eric]
> - Comment tweak. [Gavin]
> - Rebase to the latest branch.
>
> v3->v4:
> - Fix the wrong check for pmu_filter_init. [Sebastian]
> - Fix multiple alignment issue. [Gavin]
> - Report error by warn_report() instead of error_report(), and don't use
> abort() since the PMU Event Filter is an add-on and best-effort feature.
> [Gavin]
> - Add several missing { } for single line of code. [Gavin]
> - Use the g_strsplit() to replace strtok(). [Gavin]
>
> v2->v3:
> - Improve commits message, use kernel doc wording, add more explaination on
> filter example, fix some typo error. [Eric]
> - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
> - Add more precise error message report. [Eric]
> - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support in
> KVM. [Eric]
>
> v1->v2:
> - Add more description for allow and deny meaning in
> commit message. [Sebastian]
> - Small improvement. [Sebastian]
> ---
> docs/system/arm/cpu-features.rst | 23 +++++++
> target/arm/arm-qmp-cmds.c | 2 +-
> target/arm/cpu.h | 3 +
> target/arm/kvm.c | 112 +++++++++++++++++++++++++++++++
> tests/qtest/arm-cpu-features.c | 51 ++++++++++++++
> 5 files changed, 190 insertions(+), 1 deletion(-)
>
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index a5fb929243..f3930f34b3 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -204,6 +204,29 @@ the list of KVM VCPU features and their descriptions.
> the guest scheduler behavior and/or be exposed to the guest
> userspace.
>
> +``kvm-pmu-filter``
> + By default kvm-pmu-filter is disabled. This means that by default all PMU
> + events will be exposed to guest.
> +
> + KVM implements PMU Event Filtering to prevent a guest from being able to
> + sample certain events. It depends on the KVM_ARM_VCPU_PMU_V3_FILTER
> + attribute supported in KVM. It has the following format:
> +
> + kvm-pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
> +
> + The A means "allow" and D means "deny", start is the first event of the
> + range and the end is the last one. The first registered range defines
> + the global policy (global ALLOW if the first action is DENY, global DENY
> + if the first action is ALLOW). The start and end only support hexadecimal
> + format. For example:
> +
> + kvm-pmu-filter="A:0x11-0x11;A:0x23-0x3a;D:0x30-0x30"
> +
> + Since the first action is allow, we have a global deny policy. It
> + will allow event 0x11 (the cycle counter), events 0x23 to 0x3a are
> + also allowed except the event 0x30 which is denied, and all the other
> + events are denied.
> +
> TCG VCPU Features
> =================
>
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index 3cc8cc738b..6ec1d3ea3c 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -93,7 +93,7 @@ static const char *cpu_model_advertised_features[] = {
> "sve128", "sve256", "sve384", "sve512",
> "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
> "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> - "kvm-no-adjvtime", "kvm-steal-time",
> + "kvm-no-adjvtime", "kvm-steal-time", "kvm-pmu-filter",
> "pauth", "pauth-impdef", "pauth-qarma3",
> NULL
> };
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bc0c84873f..996754a9a7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -943,6 +943,9 @@ struct ArchCPU {
>
> /* KVM steal time */
> OnOffAuto kvm_steal_time;
> +
> + /* KVM PMU Filter */
> + GArray *kvm_pmu_filter;
> #endif /* CONFIG_KVM */
>
> /* Uniprocessor system with MP extensions */
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index ab85d628a8..7a363131fe 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -496,6 +496,68 @@ static void kvm_steal_time_set(Object *obj, bool value, Error **errp)
> ARM_CPU(obj)->kvm_steal_time = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> }
>
> +static char *kvm_pmu_filter_get(Object *obj, Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> + g_autoptr(GString) pmu_filter = g_string_new(NULL);
> + struct kvm_pmu_event_filter *filter;
> + char action;
> + int i;
> +
> + if (!cpu->kvm_pmu_filter) {
> + return NULL;
> + }
> +
> + for (i = 0; i < cpu->kvm_pmu_filter->len; i++) {
> + filter = &g_array_index(cpu->kvm_pmu_filter,
> + struct kvm_pmu_event_filter, i);
> + if (i) {
> + g_string_append_c(pmu_filter, ';');
> + }
> + action = filter->action == KVM_PMU_EVENT_ALLOW ? 'A' : 'D';
> + g_string_append_printf(pmu_filter, "%c:0x%hx-0x%hx", action,
> + filter->base_event,
> + filter->base_event + filter->nevents - 1);
> + }
> +
> + return g_strdup(pmu_filter->str);
> +}
> +
> +static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
> + Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> + struct kvm_pmu_event_filter filter;
> + g_auto(GStrv) event_filters;
> + int i;
> +
> + if (cpu->kvm_pmu_filter) {
> + g_array_free(cpu->kvm_pmu_filter, true);
> + }
> +
> + cpu->kvm_pmu_filter = g_array_new(false, false,
> + sizeof(struct kvm_pmu_event_filter));
> +
> + event_filters = g_strsplit(pmu_filter, ";", -1);
> + for (i = 0; event_filters[i]; i++) {
> + unsigned short start = 0, end = 0;
> + char act;
> +
> + if (sscanf(event_filters[i], "%c:%hx-%hx", &act, &start, &end) != 3 ||
> + (act != 'A' && act != 'D') || start > end) {
> + error_setg(errp, "Invalid PMU filter %s", event_filters[i]);
> + return;
> + }
> +
> + filter.base_event = start;
> + filter.nevents = end - start + 1;
> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> + KVM_PMU_EVENT_DENY;
> +
> + g_array_append_vals(cpu->kvm_pmu_filter, &filter, 1);
> + }
> +}
> +
> /* KVM VCPU properties should be prefixed with "kvm-". */
> void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> {
> @@ -517,6 +579,12 @@ void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> kvm_steal_time_set);
> object_property_set_description(obj, "kvm-steal-time",
> "Set off to disable KVM steal time.");
> +
> + object_property_add_str(obj, "kvm-pmu-filter", kvm_pmu_filter_get,
> + kvm_pmu_filter_set);
> + object_property_set_description(obj, "kvm-pmu-filter",
> + "PMU Event Filtering description for "
> + "guest PMU. (default: NULL, disabled)");
> }
>
> bool kvm_arm_pmu_supported(void)
> @@ -1706,6 +1774,48 @@ static bool kvm_arm_set_device_attr(ARMCPU *cpu, struct kvm_device_attr *attr,
> return true;
> }
>
> +static void kvm_arm_pmu_filter_init(ARMCPU *cpu)
> +{
> + static bool pmu_filter_init;
> + struct kvm_device_attr attr = {
> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> + };
> + int i;
> +
> + /*
> + * The filter only needs to be initialized through one vcpu ioctl and it
> + * will affect all other vcpu in the vm.
> + * It can be referred from kernel commit d7eec2360e3 ("KVM: arm64: Add PMU
> + * event filtering infrastructure"):
> + * Although the ioctl is per-vcpu, the map of allowed events is global to
> + * the VM (and can be setup from any vcpu until the vcpu PMU is
> + * initialized).
> + */
> + if (pmu_filter_init) {
> + return;
> + } else {
> + pmu_filter_init = true;
> + }
> +
> + if (!cpu->kvm_pmu_filter) {
> + return;
> + }
> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> + error_report("KVM doesn't support the PMU Event Filter!");
> + return;
> + }
> +
> + for (i = 0; i < cpu->kvm_pmu_filter->len; i++) {
> + attr.addr = (uint64_t)&g_array_index(cpu->kvm_pmu_filter,
> + struct kvm_pmu_event_filter, i);
> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
> + error_report("KVM set the PMU Event Filter failed!");
> + break;
> + }
> + }
> +}
> +
> void kvm_arm_pmu_init(ARMCPU *cpu)
> {
> struct kvm_device_attr attr = {
> @@ -1716,6 +1826,8 @@ void kvm_arm_pmu_init(ARMCPU *cpu)
> if (!cpu->has_pmu) {
> return;
> }
> +
> + kvm_arm_pmu_filter_init(cpu);
> if (!kvm_arm_set_device_attr(cpu, &attr, "PMU")) {
> error_report("failed to init PMU");
> abort();
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 9d6e6190d5..0b99939af3 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -127,6 +127,17 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> return qdict_get_bool(props, feature);
> }
>
> +static const char *resp_get_feature_str(QDict *resp, const char *feature)
> +{
> + QDict *props;
> +
> + g_assert(resp);
> + g_assert(resp_has_props(resp));
> + props = resp_get_props(resp);
> + g_assert(qdict_get(props, feature));
> + return qdict_get_str(props, feature);
> +}
> +
> #define assert_has_feature(qts, cpu_type, feature) \
> ({ \
> QDict *_resp = do_query_no_props(qts, cpu_type); \
> @@ -156,6 +167,18 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> g_assert(qdict_get_bool(_props, feature) == (expected_value)); \
> })
>
> +#define resp_assert_feature_str(resp, feature, expected_value) \
> +({ \
> + QDict *_props; \
> + \
> + g_assert(_resp); \
> + g_assert(resp_has_props(_resp)); \
> + _props = resp_get_props(_resp); \
> + g_assert(qdict_get(_props, feature)); \
> + g_assert_cmpstr(qdict_get_str(_props, feature), \
> + ==, (expected_value)); \
> +})
> +
> #define assert_feature(qts, cpu_type, feature, expected_value) \
> ({ \
> QDict *_resp; \
> @@ -177,6 +200,17 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> qobject_unref(_resp); \
> })
>
> +#define assert_set_feature_str(qts, cpu_type, feature, value) \
> +({ \
> + const char *_fmt = "{ %s: %s }"; \
> + QDict *_resp; \
> + \
> + _resp = do_query(qts, cpu_type, _fmt, feature, value); \
> + g_assert(_resp); \
> + resp_assert_feature_str(_resp, feature, value); \
> + qobject_unref(_resp); \
> +})
> +
> #define assert_has_feature_enabled(qts, cpu_type, feature) \
> assert_feature(qts, cpu_type, feature, true)
>
> @@ -462,6 +496,7 @@ static void test_query_cpu_model_expansion(const void *data)
>
> assert_has_not_feature(qts, "max", "kvm-no-adjvtime");
> assert_has_not_feature(qts, "max", "kvm-steal-time");
> + assert_has_not_feature(qts, "max", "kvm-pmu-filter");
>
> if (g_str_equal(qtest_get_arch(), "aarch64")) {
> assert_has_feature_enabled(qts, "max", "aarch64");
> @@ -509,6 +544,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
> assert_set_feature(qts, "host", "kvm-no-adjvtime", false);
>
> if (g_str_equal(qtest_get_arch(), "aarch64")) {
> + const char *kvm_supports_pmu_filter;
> bool kvm_supports_steal_time;
> bool kvm_supports_sve;
> char max_name[8], name[8];
> @@ -547,15 +583,29 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
> * because this instance of KVM doesn't support them. Test that the
> * features are present, and, when enabled, issue further tests.
> */
> + assert_has_feature(qts, "host", "kvm-pmu-filter");
So you assert here that the feature is available ...
> assert_has_feature(qts, "host", "kvm-steal-time");
> assert_has_feature(qts, "host", "sve");
>
> resp = do_query_no_props(qts, "host");
> + kvm_supports_pmu_filter = resp_get_feature_str(resp, "kvm-pmu-filter");
> kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
> kvm_supports_sve = resp_get_feature(resp, "sve");
> vls = resp_get_sve_vls(resp);
> qobject_unref(resp);
>
> + if (kvm_supports_pmu_filter) {
... why do you then need to check for its availability here again?
I either don't understand this part of the code, or you could drop the
kvm_supports_pmu_filter variable and simply always execute the code below.
Thomas
> + assert_set_feature_str(qts, "host", "kvm-pmu-filter", "");
> + assert_set_feature_str(qts, "host", "kvm-pmu-filter",
> + "A:0x11-0x11");
> + assert_set_feature_str(qts, "host", "kvm-pmu-filter",
> + "D:0x11-0x11");
> + assert_set_feature_str(qts, "host", "kvm-pmu-filter",
> + "A:0x11-0x11;A:0x12-0x20");
> + assert_set_feature_str(qts, "host", "kvm-pmu-filter",
> + "D:0x11-0x11;A:0x12-0x20;D:0x12-0x15");
> + }
> +
> if (kvm_supports_steal_time) {
> /* If we have steal-time then we should be able to toggle it. */
> assert_set_feature(qts, "host", "kvm-steal-time", false);
> @@ -623,6 +673,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
> assert_has_not_feature(qts, "host", "pmu");
> assert_has_not_feature(qts, "host", "sve");
> assert_has_not_feature(qts, "host", "kvm-steal-time");
> + assert_has_not_feature(qts, "host", "kvm-pmu-filter");
> }
>
> qtest_quit(qts);
Hi Thmoas,
On 4/9/24 13:33, Thomas Huth wrote:
>> + assert_has_feature(qts, "host", "kvm-pmu-filter");
>
> So you assert here that the feature is available ...
>
>> assert_has_feature(qts, "host", "kvm-steal-time");
>> assert_has_feature(qts, "host", "sve");
>> resp = do_query_no_props(qts, "host");
>> + kvm_supports_pmu_filter = resp_get_feature_str(resp,
>> "kvm-pmu-filter");
>> kvm_supports_steal_time = resp_get_feature(resp,
>> "kvm-steal-time");
>> kvm_supports_sve = resp_get_feature(resp, "sve");
>> vls = resp_get_sve_vls(resp);
>> qobject_unref(resp);
>> + if (kvm_supports_pmu_filter) { >
> ... why do you then need to check for its availability here again?
> I either don't understand this part of the code, or you could drop the
> kvm_supports_pmu_filter variable and simply always execute the code below.
Thanks for your reviewing. I did so because all other feature like
"kvm-steal-time" check its availability again. I don't know the original
reason why they did that. I just followed it.
Do you think we should delete all the checking?
Thanks,
Shaoqin
>
> Thomas
>
--
Shaoqin
On 09/04/2024 09.47, Shaoqin Huang wrote:
> Hi Thmoas,
>
> On 4/9/24 13:33, Thomas Huth wrote:
>>> + assert_has_feature(qts, "host", "kvm-pmu-filter");
>>
>> So you assert here that the feature is available ...
>>
>>> assert_has_feature(qts, "host", "kvm-steal-time");
>>> assert_has_feature(qts, "host", "sve");
>>> resp = do_query_no_props(qts, "host");
>>> + kvm_supports_pmu_filter = resp_get_feature_str(resp,
>>> "kvm-pmu-filter");
>>> kvm_supports_steal_time = resp_get_feature(resp,
>>> "kvm-steal-time");
>>> kvm_supports_sve = resp_get_feature(resp, "sve");
>>> vls = resp_get_sve_vls(resp);
>>> qobject_unref(resp);
>>> + if (kvm_supports_pmu_filter) { >
>> ... why do you then need to check for its availability here again?
>> I either don't understand this part of the code, or you could drop the
>> kvm_supports_pmu_filter variable and simply always execute the code below.
>
> Thanks for your reviewing. I did so because all other feature like
> "kvm-steal-time" check its availability again. I don't know the original
> reason why they did that. I just followed it.
>
> Do you think we should delete all the checking?
resp_get_feature() seems to return a boolean value, so though these feature
could be there, they still could be disabled, I assume? Thus we likely need
to keep the check for those.
Thomas
On Wed, Apr 10 2024, Thomas Huth <thuth@redhat.com> wrote:
> On 09/04/2024 09.47, Shaoqin Huang wrote:
>> Hi Thmoas,
>>
>> On 4/9/24 13:33, Thomas Huth wrote:
>>>> + assert_has_feature(qts, "host", "kvm-pmu-filter");
>>>
>>> So you assert here that the feature is available ...
>>>
>>>> assert_has_feature(qts, "host", "kvm-steal-time");
>>>> assert_has_feature(qts, "host", "sve");
>>>> resp = do_query_no_props(qts, "host");
>>>> + kvm_supports_pmu_filter = resp_get_feature_str(resp,
>>>> "kvm-pmu-filter");
>>>> kvm_supports_steal_time = resp_get_feature(resp,
>>>> "kvm-steal-time");
>>>> kvm_supports_sve = resp_get_feature(resp, "sve");
>>>> vls = resp_get_sve_vls(resp);
>>>> qobject_unref(resp);
>>>> + if (kvm_supports_pmu_filter) { >
>>> ... why do you then need to check for its availability here again?
>>> I either don't understand this part of the code, or you could drop the
>>> kvm_supports_pmu_filter variable and simply always execute the code below.
>>
>> Thanks for your reviewing. I did so because all other feature like
>> "kvm-steal-time" check its availability again. I don't know the original
>> reason why they did that. I just followed it.
>>
>> Do you think we should delete all the checking?
>
> resp_get_feature() seems to return a boolean value, so though these feature
> could be there, they still could be disabled, I assume? Thus we likely need
> to keep the check for those.
This had confused me as well when I looked at it the last time -- one
thing is to check whether we have a certain prop in the cpu model, the
other one whether we actually support it. Maybe this needs some
comments?
© 2016 - 2024 Red Hat, Inc.