docs/system/arm/cpu-features.rst | 23 +++++++++ target/arm/cpu.h | 3 ++ target/arm/kvm.c | 80 ++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+)
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.
Reviewed-by: Sebastian Ott <sebott@redhat.com>
Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
---
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/cpu.h | 3 ++
target/arm/kvm.c | 80 ++++++++++++++++++++++++++++++++
3 files changed, 106 insertions(+)
diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index a5fb929243..7c8f6a60ef 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/cpu.h b/target/arm/cpu.h
index 63f31e0d98..f7f2431755 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -948,6 +948,9 @@ struct ArchCPU {
/* KVM steal time */
OnOffAuto kvm_steal_time;
+
+ /* KVM PMU Filter */
+ char *kvm_pmu_filter;
#endif /* CONFIG_KVM */
/* Uniprocessor system with MP extensions */
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 81813030a5..5c62580d34 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -496,6 +496,22 @@ 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);
+
+ return g_strdup(cpu->kvm_pmu_filter);
+}
+
+static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
+ Error **errp)
+{
+ ARMCPU *cpu = ARM_CPU(obj);
+
+ g_free(cpu->kvm_pmu_filter);
+ cpu->kvm_pmu_filter = g_strdup(pmu_filter);
+}
+
/* KVM VCPU properties should be prefixed with "kvm-". */
void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
{
@@ -517,6 +533,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 +1728,62 @@ 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_pmu_event_filter filter;
+ struct kvm_device_attr attr = {
+ .group = KVM_ARM_VCPU_PMU_V3_CTRL,
+ .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
+ .addr = (uint64_t)&filter,
+ };
+ int i;
+ g_auto(GStrv) event_filters;
+
+ if (!cpu->kvm_pmu_filter) {
+ return;
+ }
+ if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
+ warn_report("The KVM doesn't support the PMU Event Filter!");
+ return;
+ }
+
+ /*
+ * The filter only needs to be initialized through one vcpu ioctl and it
+ * will affect all other vcpu in the vm.
+ */
+ if (pmu_filter_init) {
+ return;
+ } else {
+ pmu_filter_init = true;
+ }
+
+ event_filters = g_strsplit(cpu->kvm_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) {
+ warn_report("Skipping invalid PMU filter %s", event_filters[i]);
+ continue;
+ }
+
+ if ((act != 'A' && act != 'D') || start > end) {
+ warn_report("Skipping invalid PMU filter %s", event_filters[i]);
+ continue;
+ }
+
+ filter.base_event = start;
+ filter.nevents = end - start + 1;
+ filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
+ KVM_PMU_EVENT_DENY;
+
+ if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
+ break;
+ }
+ }
+}
+
void kvm_arm_pmu_init(ARMCPU *cpu)
{
struct kvm_device_attr attr = {
@@ -1716,6 +1794,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();
base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
--
2.40.1On Wed, Feb 21, 2024 at 01:34:31AM -0500, Shaoqin Huang wrote:
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index a5fb929243..7c8f6a60ef 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.
Can you document whether the policy evaluation stops at the first
matching range, or checks all ranges
ie, if you have
kvm-pmu-filter="A:0x1-0x9;D=0x7-0x7"
will an input of '0x7' be allowed (because it matches the
first range and stops), or denied (because the second range
overrides the result of the first)
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 Wed, Feb 21, 2024 at 01:34:31AM -0500, 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"
snip
> @@ -517,6 +533,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)");
> }
Passing a string property, but....[1]
>
> bool kvm_arm_pmu_supported(void)
> @@ -1706,6 +1728,62 @@ 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_pmu_event_filter filter;
> + struct kvm_device_attr attr = {
> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> + .addr = (uint64_t)&filter,
> + };
> + int i;
> + g_auto(GStrv) event_filters;
> +
> + if (!cpu->kvm_pmu_filter) {
> + return;
> + }
> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> + warn_report("The KVM doesn't support the PMU Event Filter!");
If the user requested a filter and it can't be supported, QEMU
must exit with an error, not ignore the user's request.
> + return;
> + }
> +
> + /*
> + * The filter only needs to be initialized through one vcpu ioctl and it
> + * will affect all other vcpu in the vm.
> + */
> + if (pmu_filter_init) {
> + return;
> + } else {
> + pmu_filter_init = true;
> + }
> +
> + event_filters = g_strsplit(cpu->kvm_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) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
Warning on user syntax errors is undesirable - it should be a fatal
error of the user gets this wrong.
> + }
> +
> + if ((act != 'A' && act != 'D') || start > end) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
Likewise should be fatal.
> + }
> +
> + filter.base_event = start;
> + filter.nevents = end - start + 1;
> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> + KVM_PMU_EVENT_DENY;
> +
> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
> + break;
> + }
> + }
> +}
..[1] then implementing a custom parser is rather a QEMU design anti-pattern,
especially when the proposed syntax is incapable of being mapped into the
normal QAPI syntax for a list of structs should we want to fully convert
-cpu to QAPI parsing later. I wonder if can we model this property with
QAPI now ?
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 :|
Hi Daniel,
On 3/19/24 16:22, Daniel P. Berrangé wrote:
> On Wed, Feb 21, 2024 at 01:34:31AM -0500, 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"
>
> snip
>
>> @@ -517,6 +533,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)");
>> }
>
> Passing a string property, but....[1]
>
>>
>> bool kvm_arm_pmu_supported(void)
>> @@ -1706,6 +1728,62 @@ 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_pmu_event_filter filter;
>> + struct kvm_device_attr attr = {
>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>> + .addr = (uint64_t)&filter,
>> + };
>> + int i;
>> + g_auto(GStrv) event_filters;
>> +
>> + if (!cpu->kvm_pmu_filter) {
>> + return;
>> + }
>> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
>> + warn_report("The KVM doesn't support the PMU Event Filter!");
>
> If the user requested a filter and it can't be supported, QEMU
> must exit with an error, not ignore the user's request.
>
>> + return;
>> + }
>> +
>> + /*
>> + * The filter only needs to be initialized through one vcpu ioctl and it
>> + * will affect all other vcpu in the vm.
>> + */
>> + if (pmu_filter_init) {
>> + return;
>> + } else {
>> + pmu_filter_init = true;
>> + }
>> +
>> + event_filters = g_strsplit(cpu->kvm_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) {
>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>> + continue;
>
> Warning on user syntax errors is undesirable - it should be a fatal
> error of the user gets this wrong.
>
>> + }
>> +
>> + if ((act != 'A' && act != 'D') || start > end) {
>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>> + continue;
>
> Likewise should be fatal.
>
>> + }
>> +
>> + filter.base_event = start;
>> + filter.nevents = end - start + 1;
>> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
>> + KVM_PMU_EVENT_DENY;
>> +
>> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
>> + break;
>> + }
>> + }
>> +}
>
> ..[1] then implementing a custom parser is rather a QEMU design anti-pattern,
> especially when the proposed syntax is incapable of being mapped into the
> normal QAPI syntax for a list of structs should we want to fully convert
> -cpu to QAPI parsing later. I wonder if can we model this property with
> QAPI now ?
I guess you mean creating a new property like those in
hw/core/qdev-properties-system.c for instance and populating an array
of those at CPU object level?
Note there is v8 but most of your comments still apply
https://lore.kernel.org/all/20240312074849.71475-1-shahuang@redhat.com/
Thanks
Eric
>
> With regards,
> Daniel
On Tue, Mar 19, 2024 at 06:58:33PM +0100, Eric Auger wrote:
> Hi Daniel,
>
> On 3/19/24 16:22, Daniel P. Berrangé wrote:
> > On Wed, Feb 21, 2024 at 01:34:31AM -0500, 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"
> >
> > snip
> >
> >> @@ -517,6 +533,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)");
> >> }
> >
> > Passing a string property, but....[1]
> >
> >>
> >> bool kvm_arm_pmu_supported(void)
> >> @@ -1706,6 +1728,62 @@ 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_pmu_event_filter filter;
> >> + struct kvm_device_attr attr = {
> >> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> >> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> >> + .addr = (uint64_t)&filter,
> >> + };
> >> + int i;
> >> + g_auto(GStrv) event_filters;
> >> +
> >> + if (!cpu->kvm_pmu_filter) {
> >> + return;
> >> + }
> >> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> >> + warn_report("The KVM doesn't support the PMU Event Filter!");
> >
> > If the user requested a filter and it can't be supported, QEMU
> > must exit with an error, not ignore the user's request.
> >
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * The filter only needs to be initialized through one vcpu ioctl and it
> >> + * will affect all other vcpu in the vm.
> >> + */
> >> + if (pmu_filter_init) {
> >> + return;
> >> + } else {
> >> + pmu_filter_init = true;
> >> + }
> >> +
> >> + event_filters = g_strsplit(cpu->kvm_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) {
> >> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> >> + continue;
> >
> > Warning on user syntax errors is undesirable - it should be a fatal
> > error of the user gets this wrong.
> >
> >> + }
> >> +
> >> + if ((act != 'A' && act != 'D') || start > end) {
> >> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> >> + continue;
> >
> > Likewise should be fatal.
> >
> >> + }
> >> +
> >> + filter.base_event = start;
> >> + filter.nevents = end - start + 1;
> >> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> >> + KVM_PMU_EVENT_DENY;
> >> +
> >> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
> >> + break;
> >> + }
> >> + }
> >> +}
> >
> > ..[1] then implementing a custom parser is rather a QEMU design anti-pattern,
> > especially when the proposed syntax is incapable of being mapped into the
> > normal QAPI syntax for a list of structs should we want to fully convert
> > -cpu to QAPI parsing later. I wonder if can we model this property with
> > QAPI now ?
> I guess you mean creating a new property like those in
> hw/core/qdev-properties-system.c for instance and populating an array
> of those at CPU object level?
Yeah, something like the IOThreadVirtQueueMapping data type would
be the more QAPI like code pattern.
> Note there is v8 but most of your comments still apply
> https://lore.kernel.org/all/20240312074849.71475-1-shahuang@redhat.com/
Yes, sorry I just saw Peter's query about libvirt on this v7 and
didn't think to look for a newer version
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 :|
Hi,
On 3/19/24 19:07, Daniel P. Berrangé wrote:
> On Tue, Mar 19, 2024 at 06:58:33PM +0100, Eric Auger wrote:
>> Hi Daniel,
>>
>> On 3/19/24 16:22, Daniel P. Berrangé wrote:
>>> On Wed, Feb 21, 2024 at 01:34:31AM -0500, 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"
>>>
>>> snip
>>>
>>>> @@ -517,6 +533,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)");
>>>> }
>>>
>>> Passing a string property, but....[1]
>>>
>>>>
>>>> bool kvm_arm_pmu_supported(void)
>>>> @@ -1706,6 +1728,62 @@ 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_pmu_event_filter filter;
>>>> + struct kvm_device_attr attr = {
>>>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>>>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>>>> + .addr = (uint64_t)&filter,
>>>> + };
>>>> + int i;
>>>> + g_auto(GStrv) event_filters;
>>>> +
>>>> + if (!cpu->kvm_pmu_filter) {
>>>> + return;
>>>> + }
>>>> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
>>>> + warn_report("The KVM doesn't support the PMU Event Filter!");
>>>
>>> If the user requested a filter and it can't be supported, QEMU
>>> must exit with an error, not ignore the user's request.
>>>
>>>> + return;
>>>> + }
>>>> +
>>>> + /*
>>>> + * The filter only needs to be initialized through one vcpu ioctl and it
>>>> + * will affect all other vcpu in the vm.
>>>> + */
>>>> + if (pmu_filter_init) {
>>>> + return;
>>>> + } else {
>>>> + pmu_filter_init = true;
>>>> + }
>>>> +
>>>> + event_filters = g_strsplit(cpu->kvm_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) {
>>>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> + continue;
>>>
>>> Warning on user syntax errors is undesirable - it should be a fatal
>>> error of the user gets this wrong.
>>>
>>>> + }
>>>> +
>>>> + if ((act != 'A' && act != 'D') || start > end) {
>>>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> + continue;
>>>
>>> Likewise should be fatal.
>>>
>>>> + }
>>>> +
>>>> + filter.base_event = start;
>>>> + filter.nevents = end - start + 1;
>>>> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
>>>> + KVM_PMU_EVENT_DENY;
>>>> +
>>>> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
>>>> + break;
>>>> + }
>>>> + }
>>>> +}
>>>
>>> ..[1] then implementing a custom parser is rather a QEMU design anti-pattern,
>>> especially when the proposed syntax is incapable of being mapped into the
>>> normal QAPI syntax for a list of structs should we want to fully convert
>>> -cpu to QAPI parsing later. I wonder if can we model this property with
>>> QAPI now ?
>> I guess you mean creating a new property like those in
>> hw/core/qdev-properties-system.c for instance and populating an array
>> of those at CPU object level?
>
> Yeah, something like the IOThreadVirtQueueMapping data type would
> be the more QAPI like code pattern.
OK thank you for the confirmation. Then if we create such kind of
property it would be nice that this latter also matches the need of x86
PMU filtering. I think the uapi exists at KVM level but has never been
integrated in qemu.
>
>> Note there is v8 but most of your comments still apply
>> https://lore.kernel.org/all/20240312074849.71475-1-shahuang@redhat.com/
>
> Yes, sorry I just saw Peter's query about libvirt on this v7 and
> didn't think to look for a newer version
no problem. Thank you for your time
Eric
>
> With regards,
> Daniel
On Wed, 21 Feb 2024 at 06:34, Shaoqin Huang <shahuang@redhat.com> 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.
>
> Reviewed-by: Sebastian Ott <sebott@redhat.com>
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> ---
> 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/cpu.h | 3 ++
> target/arm/kvm.c | 80 ++++++++++++++++++++++++++++++++
> 3 files changed, 106 insertions(+)
The new syntax for the filter property seems quite complicated.
I think it would be worth testing it with a new test in
tests/qtest/arm-cpu-features.c.
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index a5fb929243..7c8f6a60ef 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
"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
Missing space before '('.
Why the '@' before 'action'?
> + 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
lowercase "the".
> + also allowed except the event 0x30 which is denied, and all the other
> + events are denied.
> +
Did you check that the documentation builds and that this new
documentation renders into HTML the way you want it?
> TCG VCPU Features
> =================
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 63f31e0d98..f7f2431755 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -948,6 +948,9 @@ struct ArchCPU {
>
> /* KVM steal time */
> OnOffAuto kvm_steal_time;
> +
> + /* KVM PMU Filter */
> + char *kvm_pmu_filter;
> #endif /* CONFIG_KVM */
>
> /* Uniprocessor system with MP extensions */
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 81813030a5..5c62580d34 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -496,6 +496,22 @@ 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);
> +
> + return g_strdup(cpu->kvm_pmu_filter);
> +}
> +
> +static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
> + Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> +
> + g_free(cpu->kvm_pmu_filter);
> + cpu->kvm_pmu_filter = g_strdup(pmu_filter);
> +}
> +
> /* KVM VCPU properties should be prefixed with "kvm-". */
> void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> {
> @@ -517,6 +533,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 +1728,62 @@ 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_pmu_event_filter filter;
> + struct kvm_device_attr attr = {
> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> + .addr = (uint64_t)&filter,
> + };
> + int i;
> + g_auto(GStrv) event_filters;
> +
> + if (!cpu->kvm_pmu_filter) {
> + return;
> + }
> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> + warn_report("The KVM doesn't support the PMU Event Filter!");
Drop "The ".
Should this really only be a warning, rather than an error?
> + return;
> + }
> +
> + /*
> + * The filter only needs to be initialized through one vcpu ioctl and it
> + * will affect all other vcpu in the vm.
Weird. Why isn't it a VM ioctl if it affects the whole VM ?
> + */
> + if (pmu_filter_init) {
> + return;
> + } else {
> + pmu_filter_init = true;
> + }
Shouldn't we do this before we do the kvm_vcpu_ioctl check
for whether the kernel supports the filter? Otherwise presumably
we'll print the warning once per vCPU, rather than only once.
> +
> + event_filters = g_strsplit(cpu->kvm_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) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
> + }
> +
> + if ((act != 'A' && act != 'D') || start > end) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
> + }
It would be better to do the syntax checking up-front when
the user tries to set the property. Then you can make the
property-setting return an error for invalid strings.
> +
> + filter.base_event = start;
> + filter.nevents = end - start + 1;
> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> + KVM_PMU_EVENT_DENY;
> +
> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
Shouldn't we arrange for an error message if this fails?
> + break;
> + }
> + }
> +}
> +
> void kvm_arm_pmu_init(ARMCPU *cpu)
> {
> struct kvm_device_attr attr = {
> @@ -1716,6 +1794,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();
>
> base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
> --
> 2.40.1
thanks
-- PMM
Hi Peter,
On 2/22/24 22:28, Peter Maydell wrote:
> On Wed, 21 Feb 2024 at 06:34, Shaoqin Huang <shahuang@redhat.com> 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.
>>
>> Reviewed-by: Sebastian Ott <sebott@redhat.com>
>> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
>> ---
>> 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/cpu.h | 3 ++
>> target/arm/kvm.c | 80 ++++++++++++++++++++++++++++++++
>> 3 files changed, 106 insertions(+)
>
> The new syntax for the filter property seems quite complicated.
> I think it would be worth testing it with a new test in
> tests/qtest/arm-cpu-features.c.
I was trying to add a test in tests/qtest/arm-cpu-features.c. But I
found all other cpu-feature is bool property.
When I use the 'query-cpu-model-expansion' to query the cpu-features,
the kvm-pmu-filter will not shown in the returned results, just like below.
{'execute': 'query-cpu-model-expansion', 'arguments': {'type': 'full',
'model': { 'name': 'host'}}}{"return": {}}
{"return": {"model": {"name": "host", "props": {"sve768": false,
"sve128": false, "sve1024": false, "sve1280": false, "sve896": false,
"sve256": false, "sve1536": false, "sve1792": false, "sve384": false,
"sve": false, "sve2048": false, "pauth": false, "kvm-no-adjvtime":
false, "sve512": false, "aarch64": true, "pmu": true, "sve1920": false,
"sve1152": false, "kvm-steal-time": true, "sve640": false, "sve1408":
false, "sve1664": false}}}}
I'm not sure if it's because the `query-cpu-model-expansion` only return
the feature which is bool. Since the kvm-pmu-filter is a str, it won't
be recognized as a feature.
So I want to ask how can I add the kvm-pmu-filter which is str property
into the cpu-feature.c test.
>
>
>> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
>> index a5fb929243..7c8f6a60ef 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
>
> "PMU"
>
Got it.
>> + 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
>
> Missing space before '('.
>
> Why the '@' before 'action'?
>
I copied the description from kvm document. And it has the @ before
action, I think I can delete @ at there.
>> + 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
>
> lowercase "the".
>
Gotta.
>> + also allowed except the event 0x30 which is denied, and all the other
>> + events are denied.
>> +
>
> Did you check that the documentation builds and that this new
> documentation renders into HTML the way you want it?
>
I can double check it.
>> TCG VCPU Features
>> =================
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 63f31e0d98..f7f2431755 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -948,6 +948,9 @@ struct ArchCPU {
>>
>> /* KVM steal time */
>> OnOffAuto kvm_steal_time;
>> +
>> + /* KVM PMU Filter */
>> + char *kvm_pmu_filter;
>> #endif /* CONFIG_KVM */
>>
>> /* Uniprocessor system with MP extensions */
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index 81813030a5..5c62580d34 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -496,6 +496,22 @@ 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);
>> +
>> + return g_strdup(cpu->kvm_pmu_filter);
>> +}
>> +
>> +static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
>> + Error **errp)
>> +{
>> + ARMCPU *cpu = ARM_CPU(obj);
>> +
>> + g_free(cpu->kvm_pmu_filter);
>> + cpu->kvm_pmu_filter = g_strdup(pmu_filter);
>> +}
>> +
>> /* KVM VCPU properties should be prefixed with "kvm-". */
>> void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
>> {
>> @@ -517,6 +533,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 +1728,62 @@ 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_pmu_event_filter filter;
>> + struct kvm_device_attr attr = {
>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>> + .addr = (uint64_t)&filter,
>> + };
>> + int i;
>> + g_auto(GStrv) event_filters;
>> +
>> + if (!cpu->kvm_pmu_filter) {
>> + return;
>> + }
>> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
>> + warn_report("The KVM doesn't support the PMU Event Filter!");
>
> Drop "The ".
>
> Should this really only be a warning, rather than an error?
>
I think this is an add-on feature, and shouldn't block the qemu init
process. If we want to set the wrong pmu filter and it doesn't take
affect to the VM, it can be detected in the VM.
>> + return;
>> + }
>> +
>> + /*
>> + * The filter only needs to be initialized through one vcpu ioctl and it
>> + * will affect all other vcpu in the vm.
>
> Weird. Why isn't it a VM ioctl if it affects the whole VM ?
>
From (kernel) commit d7eec2360e3 ("KVM: arm64: Add PMU event filtering
infrastructure"):
Note that although the ioctl is per-vcpu, the map of allowed events is
global to the VM (it can be setup from any vcpu until the vcpu PMU is
initialized).
>> + */
>> + if (pmu_filter_init) {
>> + return;
>> + } else {
>> + pmu_filter_init = true;
>> + }
>
> Shouldn't we do this before we do the kvm_vcpu_ioctl check
> for whether the kernel supports the filter? Otherwise presumably
> we'll print the warning once per vCPU, rather than only once.
>
Yes. I will move this to the beginning of the function.
>> +
>> + event_filters = g_strsplit(cpu->kvm_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) {
>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>> + continue;
>> + }
>> +
>> + if ((act != 'A' && act != 'D') || start > end) {
>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>> + continue;
>> + }
>
> It would be better to do the syntax checking up-front when
> the user tries to set the property. Then you can make the
> property-setting return an error for invalid strings.
>
Ok. I guess you mean to say move the syntax checking to the
kvm_pmu_filter_set() function. But wouldn't it cause some code
duplication? Since it should first check syntax of the string in
kvm_pmu_filter_set() and then parse the string in this function.
>> +
>> + filter.base_event = start;
>> + filter.nevents = end - start + 1;
>> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
>> + KVM_PMU_EVENT_DENY;
>> +
>> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
>
> Shouldn't we arrange for an error message if this fails?
>
If it fails, the kvm_arm_set_device_attr() function would help us to
report the error. So I think there is no need to report the error again.
Thanks,
Shaoqin
>> + break;
>> + }
>> + }
>> +}
>> +
>> void kvm_arm_pmu_init(ARMCPU *cpu)
>> {
>> struct kvm_device_attr attr = {
>> @@ -1716,6 +1794,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();
>>
>> base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
>> --
>> 2.40.1
>
> thanks
> -- PMM
>
--
Shaoqin
On Thu, 29 Feb 2024 at 02:32, Shaoqin Huang <shahuang@redhat.com> wrote:
>
> Hi Peter,
>
> On 2/22/24 22:28, Peter Maydell wrote:
> > On Wed, 21 Feb 2024 at 06:34, Shaoqin Huang <shahuang@redhat.com> 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.
> >
> > The new syntax for the filter property seems quite complicated.
> > I think it would be worth testing it with a new test in
> > tests/qtest/arm-cpu-features.c.
>
> I was trying to add a test in tests/qtest/arm-cpu-features.c. But I
> found all other cpu-feature is bool property.
>
> When I use the 'query-cpu-model-expansion' to query the cpu-features,
> the kvm-pmu-filter will not shown in the returned results, just like below.
>
> {'execute': 'query-cpu-model-expansion', 'arguments': {'type': 'full',
> 'model': { 'name': 'host'}}}{"return": {}}
>
> {"return": {"model": {"name": "host", "props": {"sve768": false,
> "sve128": false, "sve1024": false, "sve1280": false, "sve896": false,
> "sve256": false, "sve1536": false, "sve1792": false, "sve384": false,
> "sve": false, "sve2048": false, "pauth": false, "kvm-no-adjvtime":
> false, "sve512": false, "aarch64": true, "pmu": true, "sve1920": false,
> "sve1152": false, "kvm-steal-time": true, "sve640": false, "sve1408":
> false, "sve1664": false}}}}
>
> I'm not sure if it's because the `query-cpu-model-expansion` only return
> the feature which is bool. Since the kvm-pmu-filter is a str, it won't
> be recognized as a feature.
>
> So I want to ask how can I add the kvm-pmu-filter which is str property
> into the cpu-feature.c test.
It doesn't appear because the list of properties that we advertise
via query-cpu-model-expansion is set in the cpu_model_advertised_features[]
array in target/arm/arm-qmp-cmds.c, and this patch doesn't add
'kvm-pmu-filter' to it. But you have a good point about all the
others being bool properties: I don't know enough about that
mechanism to know if simply adding this to the list is right.
This does raise a more general question: do we need to advertise
the existence of this property to libvirt via QMP? Eric, Sebastian:
do you know ?
If we don't care about this being visible to libvirt then the
importance of having a test case covering the command line
syntax goes down a bit.
> >>
> >> +static void kvm_arm_pmu_filter_init(ARMCPU *cpu)
> >> +{
> >> + static bool pmu_filter_init;
> >> + struct kvm_pmu_event_filter filter;
> >> + struct kvm_device_attr attr = {
> >> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> >> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> >> + .addr = (uint64_t)&filter,
> >> + };
> >> + int i;
> >> + g_auto(GStrv) event_filters;
> >> +
> >> + if (!cpu->kvm_pmu_filter) {
> >> + return;
> >> + }
> >> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> >> + warn_report("The KVM doesn't support the PMU Event Filter!");
> >
> > Drop "The ".
> >
> > Should this really only be a warning, rather than an error?
> >
>
> I think this is an add-on feature, and shouldn't block the qemu init
> process. If we want to set the wrong pmu filter and it doesn't take
> affect to the VM, it can be detected in the VM.
But if the user explicitly asked for it, it's not optional
for them, it's something they want. We should fail if the user
passes us an option that we can't actually carry out.
> >> + return;
> >> + }
> >> +
> >> + /*
> >> + * The filter only needs to be initialized through one vcpu ioctl and it
> >> + * will affect all other vcpu in the vm.
> >
> > Weird. Why isn't it a VM ioctl if it affects the whole VM ?
> >
> From (kernel) commit d7eec2360e3 ("KVM: arm64: Add PMU event filtering
> infrastructure"):
> Note that although the ioctl is per-vcpu, the map of allowed events is
> global to the VM (it can be setup from any vcpu until the vcpu PMU is
> initialized).
That just says it is a per-vcpu ioctl, it doesn't say why...
> >> + event_filters = g_strsplit(cpu->kvm_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) {
> >> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> >> + continue;
> >> + }
> >> +
> >> + if ((act != 'A' && act != 'D') || start > end) {
> >> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> >> + continue;
> >> + }
> >
> > It would be better to do the syntax checking up-front when
> > the user tries to set the property. Then you can make the
> > property-setting return an error for invalid strings.
> >
>
> Ok. I guess you mean to say move the syntax checking to the
> kvm_pmu_filter_set() function. But wouldn't it cause some code
> duplication? Since it should first check syntax of the string in
> kvm_pmu_filter_set() and then parse the string in this function.
No, you should check syntax and parse the string in
kvm_pmu_filter_set(), and fill in a data structure so you don't
have to do any string parsing here. kvm_pmu_filter_get()
will need to reconstitute a string from the data structure.
thanks
-- PMM
Hi Peter,
On 2/29/24 12:00, Peter Maydell wrote:
> On Thu, 29 Feb 2024 at 02:32, Shaoqin Huang <shahuang@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 2/22/24 22:28, Peter Maydell wrote:
>>> On Wed, 21 Feb 2024 at 06:34, Shaoqin Huang <shahuang@redhat.com> 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.
>
>>>
>>> The new syntax for the filter property seems quite complicated.
>>> I think it would be worth testing it with a new test in
>>> tests/qtest/arm-cpu-features.c.
>>
>> I was trying to add a test in tests/qtest/arm-cpu-features.c. But I
>> found all other cpu-feature is bool property.
>>
>> When I use the 'query-cpu-model-expansion' to query the cpu-features,
>> the kvm-pmu-filter will not shown in the returned results, just like below.
>>
>> {'execute': 'query-cpu-model-expansion', 'arguments': {'type': 'full',
>> 'model': { 'name': 'host'}}}{"return": {}}
>>
>> {"return": {"model": {"name": "host", "props": {"sve768": false,
>> "sve128": false, "sve1024": false, "sve1280": false, "sve896": false,
>> "sve256": false, "sve1536": false, "sve1792": false, "sve384": false,
>> "sve": false, "sve2048": false, "pauth": false, "kvm-no-adjvtime":
>> false, "sve512": false, "aarch64": true, "pmu": true, "sve1920": false,
>> "sve1152": false, "kvm-steal-time": true, "sve640": false, "sve1408":
>> false, "sve1664": false}}}}
>>
>> I'm not sure if it's because the `query-cpu-model-expansion` only return
>> the feature which is bool. Since the kvm-pmu-filter is a str, it won't
>> be recognized as a feature.
>>
>> So I want to ask how can I add the kvm-pmu-filter which is str property
>> into the cpu-feature.c test.
>
> It doesn't appear because the list of properties that we advertise
> via query-cpu-model-expansion is set in the cpu_model_advertised_features[]
> array in target/arm/arm-qmp-cmds.c, and this patch doesn't add
> 'kvm-pmu-filter' to it. But you have a good point about all the
> others being bool properties: I don't know enough about that
> mechanism to know if simply adding this to the list is right.
>
> This does raise a more general question: do we need to advertise
> the existence of this property to libvirt via QMP? Eric, Sebastian:
> do you know ?
sorry I missed this question. yes I think it is sensible to expose that
option to libvirt. There is no good default value to be set at qemu
level so to me it really depends on the upper stack to choose the
correct value (depending on the sensitiveness of the data that justified
the kernel uapi).
>
> If we don't care about this being visible to libvirt then the
> importance of having a test case covering the command line
> syntax goes down a bit.
>
>>>>
>>>> +static void kvm_arm_pmu_filter_init(ARMCPU *cpu)
>>>> +{
>>>> + static bool pmu_filter_init;
>>>> + struct kvm_pmu_event_filter filter;
>>>> + struct kvm_device_attr attr = {
>>>> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>>>> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
>>>> + .addr = (uint64_t)&filter,
>>>> + };
>>>> + int i;
>>>> + g_auto(GStrv) event_filters;
>>>> +
>>>> + if (!cpu->kvm_pmu_filter) {
>>>> + return;
>>>> + }
>>>> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
>>>> + warn_report("The KVM doesn't support the PMU Event Filter!");
>>>
>>> Drop "The ".
>>>
>>> Should this really only be a warning, rather than an error?
>>>
>>
>> I think this is an add-on feature, and shouldn't block the qemu init
>> process. If we want to set the wrong pmu filter and it doesn't take
>> affect to the VM, it can be detected in the VM.
>
> But if the user explicitly asked for it, it's not optional
> for them, it's something they want. We should fail if the user
> passes us an option that we can't actually carry out.
>
>>>> + return;
>>>> + }
>>>> +
>>>> + /*
>>>> + * The filter only needs to be initialized through one vcpu ioctl and it
>>>> + * will affect all other vcpu in the vm.
>>>
>>> Weird. Why isn't it a VM ioctl if it affects the whole VM ?
>>>
>> From (kernel) commit d7eec2360e3 ("KVM: arm64: Add PMU event filtering
>> infrastructure"):
>> Note that although the ioctl is per-vcpu, the map of allowed events is
>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>> initialized).
>
> That just says it is a per-vcpu ioctl, it doesn't say why...
Marc said in a his cover letter
"
The filter state is global to the guest, despite the PMU being per CPU.
I'm not sure whether it would be worth it making it CPU-private."
I guess this is because Marc wanted to reuse the
KVM_ARM_VCPU_PMU_V3_CTRL group and just added a new attribute on top of
existing
KVM_ARM_VCPU_PMU_V3_IRQ, KVM_ARM_VCPU_PMU_V3_INIT
Thanks
Eric
>
>>>> + event_filters = g_strsplit(cpu->kvm_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) {
>>>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> + continue;
>>>> + }
>>>> +
>>>> + if ((act != 'A' && act != 'D') || start > end) {
>>>> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
>>>> + continue;
>>>> + }
>>>
>>> It would be better to do the syntax checking up-front when
>>> the user tries to set the property. Then you can make the
>>> property-setting return an error for invalid strings.
>>>
>>
>> Ok. I guess you mean to say move the syntax checking to the
>> kvm_pmu_filter_set() function. But wouldn't it cause some code
>> duplication? Since it should first check syntax of the string in
>> kvm_pmu_filter_set() and then parse the string in this function.
>
> No, you should check syntax and parse the string in
> kvm_pmu_filter_set(), and fill in a data structure so you don't
> have to do any string parsing here. kvm_pmu_filter_get()
> will need to reconstitute a string from the data structure.
>
> thanks
> -- PMM
>
On Tue, 19 Mar 2024 at 14:57, Eric Auger <eauger@redhat.com> wrote:
>
> Hi Peter,
>
> On 2/29/24 12:00, Peter Maydell wrote:
> > On Thu, 29 Feb 2024 at 02:32, Shaoqin Huang <shahuang@redhat.com> wrote:
> >> I was trying to add a test in tests/qtest/arm-cpu-features.c. But I
> >> found all other cpu-feature is bool property.
> >>
> >> When I use the 'query-cpu-model-expansion' to query the cpu-features,
> >> the kvm-pmu-filter will not shown in the returned results, just like below.
> >>
> >> {'execute': 'query-cpu-model-expansion', 'arguments': {'type': 'full',
> >> 'model': { 'name': 'host'}}}{"return": {}}
> >>
> >> {"return": {"model": {"name": "host", "props": {"sve768": false,
> >> "sve128": false, "sve1024": false, "sve1280": false, "sve896": false,
> >> "sve256": false, "sve1536": false, "sve1792": false, "sve384": false,
> >> "sve": false, "sve2048": false, "pauth": false, "kvm-no-adjvtime":
> >> false, "sve512": false, "aarch64": true, "pmu": true, "sve1920": false,
> >> "sve1152": false, "kvm-steal-time": true, "sve640": false, "sve1408":
> >> false, "sve1664": false}}}}
> >>
> >> I'm not sure if it's because the `query-cpu-model-expansion` only return
> >> the feature which is bool. Since the kvm-pmu-filter is a str, it won't
> >> be recognized as a feature.
> >>
> >> So I want to ask how can I add the kvm-pmu-filter which is str property
> >> into the cpu-feature.c test.
> >
> > It doesn't appear because the list of properties that we advertise
> > via query-cpu-model-expansion is set in the cpu_model_advertised_features[]
> > array in target/arm/arm-qmp-cmds.c, and this patch doesn't add
> > 'kvm-pmu-filter' to it. But you have a good point about all the
> > others being bool properties: I don't know enough about that
> > mechanism to know if simply adding this to the list is right.
> >
> > This does raise a more general question: do we need to advertise
> > the existence of this property to libvirt via QMP? Eric, Sebastian:
> > do you know ?
> sorry I missed this question. yes I think it is sensible to expose that
> option to libvirt. There is no good default value to be set at qemu
> level so to me it really depends on the upper stack to choose the
> correct value (depending on the sensitiveness of the data that justified
> the kernel uapi).
In that case we should definitely have a mechanism for libvirt
to be able to say "does this QEMU (and this CPU) implement
this property?". Unfortunately my QMP/libvirt expertise is
too low to be able to suggest what that mechanism should be...
thanks
-- PMM
On Tue, Mar 19, 2024 at 03:00:40PM +0000, Peter Maydell wrote: > On Tue, 19 Mar 2024 at 14:57, Eric Auger <eauger@redhat.com> wrote: > > > > Hi Peter, > > > > On 2/29/24 12:00, Peter Maydell wrote: > > > > > > It doesn't appear because the list of properties that we advertise > > > via query-cpu-model-expansion is set in the cpu_model_advertised_features[] > > > array in target/arm/arm-qmp-cmds.c, and this patch doesn't add > > > 'kvm-pmu-filter' to it. But you have a good point about all the > > > others being bool properties: I don't know enough about that > > > mechanism to know if simply adding this to the list is right. > > > > > > This does raise a more general question: do we need to advertise > > > the existence of this property to libvirt via QMP? Eric, Sebastian: > > > do you know ? > > sorry I missed this question. yes I think it is sensible to expose that > > option to libvirt. There is no good default value to be set at qemu > > level so to me it really depends on the upper stack to choose the > > correct value (depending on the sensitiveness of the data that justified > > the kernel uapi). > > In that case we should definitely have a mechanism for libvirt > to be able to say "does this QEMU (and this CPU) implement > this property?". Unfortunately my QMP/libvirt expertise is > too low to be able to suggest what that mechanism should be... Libvirt uses 'qom-list' on '/machine/unattached/device[0]' to identify CPU properties. If 'kvm-pmu-filter' appears with that, then detection will be fine. 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 :|
Hi Shaoqin,
On 2/21/24 07:34, 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.
>
> Reviewed-by: Sebastian Ott <sebott@redhat.com>
> Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
> ---
> 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/cpu.h | 3 ++
> target/arm/kvm.c | 80 ++++++++++++++++++++++++++++++++
> 3 files changed, 106 insertions(+)
>
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index a5fb929243..7c8f6a60ef 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/cpu.h b/target/arm/cpu.h
> index 63f31e0d98..f7f2431755 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -948,6 +948,9 @@ struct ArchCPU {
>
> /* KVM steal time */
> OnOffAuto kvm_steal_time;
> +
> + /* KVM PMU Filter */
> + char *kvm_pmu_filter;
> #endif /* CONFIG_KVM */
>
> /* Uniprocessor system with MP extensions */
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 81813030a5..5c62580d34 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -496,6 +496,22 @@ 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);
> +
> + return g_strdup(cpu->kvm_pmu_filter);
> +}
> +
> +static void kvm_pmu_filter_set(Object *obj, const char *pmu_filter,
> + Error **errp)
> +{
> + ARMCPU *cpu = ARM_CPU(obj);
> +
> + g_free(cpu->kvm_pmu_filter);
> + cpu->kvm_pmu_filter = g_strdup(pmu_filter);
> +}
> +
> /* KVM VCPU properties should be prefixed with "kvm-". */
> void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
> {
> @@ -517,6 +533,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 +1728,62 @@ 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_pmu_event_filter filter;
> + struct kvm_device_attr attr = {
> + .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> + .attr = KVM_ARM_VCPU_PMU_V3_FILTER,
> + .addr = (uint64_t)&filter,
> + };
> + int i;
> + g_auto(GStrv) event_filters;
> +
> + if (!cpu->kvm_pmu_filter) {
> + return;
> + }
> + if (kvm_vcpu_ioctl(CPU(cpu), KVM_HAS_DEVICE_ATTR, &attr)) {
> + warn_report("The KVM doesn't support the PMU Event Filter!");
> + return;
> + }
> +
> + /*
> + * The filter only needs to be initialized through one vcpu ioctl and it
> + * will affect all other vcpu in the vm.
> + */
> + if (pmu_filter_init) {
> + return;
> + } else {
> + pmu_filter_init = true;
> + }
> +
> + event_filters = g_strsplit(cpu->kvm_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) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
> + }
> +
> + if ((act != 'A' && act != 'D') || start > end) {
> + warn_report("Skipping invalid PMU filter %s", event_filters[i]);
> + continue;
> + }
> +
> + filter.base_event = start;
> + filter.nevents = end - start + 1;
> + filter.action = (act == 'A') ? KVM_PMU_EVENT_ALLOW :
> + KVM_PMU_EVENT_DENY;
> +
> + if (!kvm_arm_set_device_attr(cpu, &attr, "PMU_V3_FILTER")) {
> + break;
> + }
> + }
> +}
> +
> void kvm_arm_pmu_init(ARMCPU *cpu)
> {
> struct kvm_device_attr attr = {
> @@ -1716,6 +1794,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();
>
> base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
© 2016 - 2024 Red Hat, Inc.