The select&umask is the common way for x86 to identify the PMU event,
so support this way as the "x86-default" format in kvm-pmu-filter
object.
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Yi Lai <yi1.lai@intel.com>
---
Changes since RFC v2:
* Drop hexadecimal variants and support numeric version in QAPI
directly. (Daniel)
* Rename "x86-default" format to "x86-select-umask". (Markus)
* Add Tested-by from Yi.
* Add documentation in qemu-options.hx.
* QAPI style fix:
- KVMPMU* stuff -> KvmPmu*.
* Bump up the supported QAPI version to v10.1.
Changes since RFC v1:
* Bump up the supported QAPI version to v10.0.
---
accel/kvm/kvm-pmu.c | 20 +++++++++++++++++++-
include/system/kvm-pmu.h | 13 +++++++++++++
qapi/kvm.json | 21 +++++++++++++++++++--
qemu-options.hx | 3 +++
target/i386/kvm/kvm.c | 5 +++++
5 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
index 22f749bf9183..fa73ef428e59 100644
--- a/accel/kvm/kvm-pmu.c
+++ b/accel/kvm/kvm-pmu.c
@@ -16,6 +16,8 @@
#include "qom/object_interfaces.h"
#include "system/kvm-pmu.h"
+#define UINT12_MAX (4095)
+
static void kvm_pmu_filter_set_action(Object *obj, int value,
Error **errp G_GNUC_UNUSED)
{
@@ -53,9 +55,22 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
}
for (node = head; node; node = node->next) {
- switch (node->value->format) {
+ KvmPmuFilterEvent *event = node->value;
+
+ switch (event->format) {
case KVM_PMU_EVENT_FORMAT_RAW:
break;
+ case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
+ if (event->u.x86_select_umask.select > UINT12_MAX) {
+ error_setg(errp,
+ "Parameter 'select' out of range (%d).",
+ UINT12_MAX);
+ goto fail;
+ }
+
+ /* No need to check the range of umask since it's uint8_t. */
+ break;
+ }
default:
g_assert_not_reached();
}
@@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
filter->events = head;
qapi_free_KvmPmuFilterEventList(old_head);
return;
+
+fail:
+ qapi_free_KvmPmuFilterEventList(head);
}
static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
index 818fa309c191..6abc0d037aee 100644
--- a/include/system/kvm-pmu.h
+++ b/include/system/kvm-pmu.h
@@ -32,4 +32,17 @@ struct KVMPMUFilter {
KvmPmuFilterEventList *events;
};
+/*
+ * Stolen from Linux kernel (RAW_EVENT at tools/testing/selftests/kvm/include/
+ * x86_64/pmu.h).
+ *
+ * Encode an eventsel+umask pair into event-select MSR format. Note, this is
+ * technically AMD's format, as Intel's format only supports 8 bits for the
+ * event selector, i.e. doesn't use bits 24:16 for the selector. But, OR-ing
+ * in '0' is a nop and won't clobber the CMASK.
+ */
+#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
+ ((eventsel) & 0xff) | \
+ ((umask) & 0xff) << 8)
+
#endif /* KVM_PMU_H */
diff --git a/qapi/kvm.json b/qapi/kvm.json
index 1861d86a9726..cb151ca82e5c 100644
--- a/qapi/kvm.json
+++ b/qapi/kvm.json
@@ -36,10 +36,12 @@
#
# @raw: the encoded event code that KVM can directly consume.
#
+# @x86-select-umask: standard x86 encoding format with select and umask.
+#
# Since 10.1
##
{ 'enum': 'KvmPmuEventFormat',
- 'data': ['raw'] }
+ 'data': ['raw', 'x86-select-umask'] }
##
# @KvmPmuRawEvent:
@@ -54,6 +56,20 @@
{ 'struct': 'KvmPmuRawEvent',
'data': { 'code': 'uint64' } }
+##
+# @KvmPmuX86SelectUmaskEvent:
+#
+# @select: x86 PMU event select field, which is a 12-bit unsigned
+# number.
+#
+# @umask: x86 PMU event umask field.
+#
+# Since 10.1
+##
+{ 'struct': 'KvmPmuX86SelectUmaskEvent',
+ 'data': { 'select': 'uint16',
+ 'umask': 'uint8' } }
+
##
# @KvmPmuFilterEvent:
#
@@ -66,7 +82,8 @@
{ 'union': 'KvmPmuFilterEvent',
'base': { 'format': 'KvmPmuEventFormat' },
'discriminator': 'format',
- 'data': { 'raw': 'KvmPmuRawEvent' } }
+ 'data': { 'raw': 'KvmPmuRawEvent',
+ 'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
##
# @KvmPmuFilterProperties:
diff --git a/qemu-options.hx b/qemu-options.hx
index 51a7c61ce0b0..5dcce067d8dd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -6180,6 +6180,9 @@ SRST
((select) & 0xff) | \
((umask) & 0xff) << 8)
+ ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
+ Specify the single x86 PMU event with select and umask fields.
+
An example KVM PMU filter object would look like:
.. parsed-literal::
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index fa3a696654cb..0d36ccf250ed 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
case KVM_PMU_EVENT_FORMAT_RAW:
code = event->u.raw.code;
break;
+ case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
+ code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
+ event->u.x86_select_umask.umask);
+ break;
default:
g_assert_not_reached();
}
@@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
switch (event->format) {
case KVM_PMU_EVENT_FORMAT_RAW:
+ case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
break;
default:
error_setg(errp,
--
2.34.1Zhao Liu <zhao1.liu@intel.com> writes:
> The select&umask is the common way for x86 to identify the PMU event,
> so support this way as the "x86-default" format in kvm-pmu-filter
> object.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yi Lai <yi1.lai@intel.com>
> ---
> Changes since RFC v2:
> * Drop hexadecimal variants and support numeric version in QAPI
> directly. (Daniel)
> * Rename "x86-default" format to "x86-select-umask". (Markus)
> * Add Tested-by from Yi.
> * Add documentation in qemu-options.hx.
> * QAPI style fix:
> - KVMPMU* stuff -> KvmPmu*.
> * Bump up the supported QAPI version to v10.1.
>
> Changes since RFC v1:
> * Bump up the supported QAPI version to v10.0.
> ---
> accel/kvm/kvm-pmu.c | 20 +++++++++++++++++++-
> include/system/kvm-pmu.h | 13 +++++++++++++
> qapi/kvm.json | 21 +++++++++++++++++++--
> qemu-options.hx | 3 +++
> target/i386/kvm/kvm.c | 5 +++++
> 5 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/accel/kvm/kvm-pmu.c b/accel/kvm/kvm-pmu.c
> index 22f749bf9183..fa73ef428e59 100644
> --- a/accel/kvm/kvm-pmu.c
> +++ b/accel/kvm/kvm-pmu.c
> @@ -16,6 +16,8 @@
> #include "qom/object_interfaces.h"
> #include "system/kvm-pmu.h"
>
> +#define UINT12_MAX (4095)
> +
> static void kvm_pmu_filter_set_action(Object *obj, int value,
> Error **errp G_GNUC_UNUSED)
> {
> @@ -53,9 +55,22 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> }
>
> for (node = head; node; node = node->next) {
> - switch (node->value->format) {
> + KvmPmuFilterEvent *event = node->value;
> +
> + switch (event->format) {
> case KVM_PMU_EVENT_FORMAT_RAW:
> break;
> + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> + if (event->u.x86_select_umask.select > UINT12_MAX) {
> + error_setg(errp,
> + "Parameter 'select' out of range (%d).",
> + UINT12_MAX);
> + goto fail;
> + }
> +
> + /* No need to check the range of umask since it's uint8_t. */
> + break;
> + }
As we'll see below, the new x86-specific format is defined in the QAPI
schema regardless of target.
It is accepted here also regardless of target. Doesn't matter much
right now, as the object is effectively useless for targets other than
x86, but I understand that will change.
Should we reject it unless the target is x86?
If not, I feel the behavior should be noted in the commit message.
> default:
> g_assert_not_reached();
> }
> @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> filter->events = head;
> qapi_free_KvmPmuFilterEventList(old_head);
> return;
> +
> +fail:
> + qapi_free_KvmPmuFilterEventList(head);
> }
>
> static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> diff --git a/include/system/kvm-pmu.h b/include/system/kvm-pmu.h
> index 818fa309c191..6abc0d037aee 100644
> --- a/include/system/kvm-pmu.h
> +++ b/include/system/kvm-pmu.h
> @@ -32,4 +32,17 @@ struct KVMPMUFilter {
> KvmPmuFilterEventList *events;
> };
>
> +/*
> + * Stolen from Linux kernel (RAW_EVENT at tools/testing/selftests/kvm/include/
> + * x86_64/pmu.h).
> + *
> + * Encode an eventsel+umask pair into event-select MSR format. Note, this is
> + * technically AMD's format, as Intel's format only supports 8 bits for the
> + * event selector, i.e. doesn't use bits 24:16 for the selector. But, OR-ing
> + * in '0' is a nop and won't clobber the CMASK.
> + */
> +#define X86_PMU_RAW_EVENT(eventsel, umask) (((eventsel & 0xf00UL) << 24) | \
> + ((eventsel) & 0xff) | \
> + ((umask) & 0xff) << 8)
> +
> #endif /* KVM_PMU_H */
> diff --git a/qapi/kvm.json b/qapi/kvm.json
> index 1861d86a9726..cb151ca82e5c 100644
> --- a/qapi/kvm.json
> +++ b/qapi/kvm.json
> @@ -36,10 +36,12 @@
> #
> # @raw: the encoded event code that KVM can directly consume.
> #
> +# @x86-select-umask: standard x86 encoding format with select and umask.
> +#
> # Since 10.1
> ##
> { 'enum': 'KvmPmuEventFormat',
> - 'data': ['raw'] }
> + 'data': ['raw', 'x86-select-umask'] }
>
> ##
> # @KvmPmuRawEvent:
> @@ -54,6 +56,20 @@
> { 'struct': 'KvmPmuRawEvent',
> 'data': { 'code': 'uint64' } }
>
> +##
> +# @KvmPmuX86SelectUmaskEvent:
> +#
> +# @select: x86 PMU event select field, which is a 12-bit unsigned
> +# number.
> +#
> +# @umask: x86 PMU event umask field.
> +#
> +# Since 10.1
> +##
> +{ 'struct': 'KvmPmuX86SelectUmaskEvent',
> + 'data': { 'select': 'uint16',
> + 'umask': 'uint8' } }
> +
> ##
> # @KvmPmuFilterEvent:
> #
> @@ -66,7 +82,8 @@
> { 'union': 'KvmPmuFilterEvent',
> 'base': { 'format': 'KvmPmuEventFormat' },
> 'discriminator': 'format',
> - 'data': { 'raw': 'KvmPmuRawEvent' } }
> + 'data': { 'raw': 'KvmPmuRawEvent',
> + 'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
>
> ##
> # @KvmPmuFilterProperties:
Documentation could perhaps be more explicit about this making sense
only for x86.
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 51a7c61ce0b0..5dcce067d8dd 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -6180,6 +6180,9 @@ SRST
> ((select) & 0xff) | \
> ((umask) & 0xff) << 8)
>
> + ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> + Specify the single x86 PMU event with select and umask fields.
> +
> An example KVM PMU filter object would look like:
>
> .. parsed-literal::
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index fa3a696654cb..0d36ccf250ed 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
> case KVM_PMU_EVENT_FORMAT_RAW:
> code = event->u.raw.code;
> break;
> + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> + code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> + event->u.x86_select_umask.umask);
> + break;
> default:
> g_assert_not_reached();
> }
> @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>
> switch (event->format) {
> case KVM_PMU_EVENT_FORMAT_RAW:
> + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> break;
> default:
> error_setg(errp,
Hi Markus,
> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> > + if (event->u.x86_select_umask.select > UINT12_MAX) {
> > + error_setg(errp,
> > + "Parameter 'select' out of range (%d).",
> > + UINT12_MAX);
> > + goto fail;
> > + }
> > +
> > + /* No need to check the range of umask since it's uint8_t. */
> > + break;
> > + }
>
> As we'll see below, the new x86-specific format is defined in the QAPI
> schema regardless of target.
>
> It is accepted here also regardless of target. Doesn't matter much
> right now, as the object is effectively useless for targets other than
> x86, but I understand that will change.
>
> Should we reject it unless the target is x86?
I previously supposed that different architectures should implement
their own kvm_arch_check_pmu_filter(), which is the `check` hook of
object_class_property_add_link():
object_class_property_add_link(oc, "pmu-filter",
TYPE_KVM_PMU_FILTER,
offsetof(KVMState, pmu_filter),
kvm_arch_check_pmu_filter,
OBJ_PROP_LINK_STRONG);
For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
kvm.c and checked the supported formats (I also supposed arch-specific
PMU filter could reject the unsupported format in
kvm_arch_check_pmu_filter().)
But I think your idea is better, i.e., rejecting unsupported format
early in pmu-filter parsing.
Well, IIUC, there is no way to specify in QAPI that certain enumerations
are generic and certain enumerations are arch-specific, so rejecting
unsupported format can only happen in parsing code. For example, wrap
the above code in "#if defined(TARGET_I386)":
for (node = head; node; node = node->next) {
KvmPmuFilterEvent *event = node->value;
switch (event->format) {
case KVM_PMU_EVENT_FORMAT_RAW:
break;
#if defined(TARGET_I386)
case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
...
break;
}
case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
...
break;
}
#endif
default:
error_setg(errp,
"Unsupported format.");
goto fail;
}
...
}
EMM, do you like this idea?
> If not, I feel the behavior should be noted in the commit message.
With the above change, I think it's possible to reject x86-specific
format on non-x86 arch. And I can also note this behavior in commit
message.
> > default:
> > g_assert_not_reached();
> > }
> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> > filter->events = head;
> > qapi_free_KvmPmuFilterEventList(old_head);
> > return;
> > +
> > +fail:
> > + qapi_free_KvmPmuFilterEventList(head);
> > }
> >
> > static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
...
> > ##
> > # @KvmPmuFilterEvent:
> > #
> > @@ -66,7 +82,8 @@
> > { 'union': 'KvmPmuFilterEvent',
> > 'base': { 'format': 'KvmPmuEventFormat' },
> > 'discriminator': 'format',
> > - 'data': { 'raw': 'KvmPmuRawEvent' } }
> > + 'data': { 'raw': 'KvmPmuRawEvent',
> > + 'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
> >
> > ##
> > # @KvmPmuFilterProperties:
>
> Documentation could perhaps be more explicit about this making sense
> only for x86.
What about the following doc?
##
# @KvmPmuFilterProperties:
#
# Properties of KVM PMU Filter (only for x86).
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 51a7c61ce0b0..5dcce067d8dd 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -6180,6 +6180,9 @@ SRST
> > ((select) & 0xff) | \
> > ((umask) & 0xff) << 8)
> >
> > + ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
> > + Specify the single x86 PMU event with select and umask fields.
> > +
> > An example KVM PMU filter object would look like:
> >
> > .. parsed-literal::
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index fa3a696654cb..0d36ccf250ed 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
> > case KVM_PMU_EVENT_FORMAT_RAW:
> > code = event->u.raw.code;
> > break;
> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
> > + code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
> > + event->u.x86_select_umask.umask);
> > + break;
> > default:
> > g_assert_not_reached();
> > }
> > @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
> >
> > switch (event->format) {
> > case KVM_PMU_EVENT_FORMAT_RAW:
> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
Here's the current format check I mentioned above. But I agree your idea
and will check in the parsing of pmu-filter object.
> > break;
> > default:
> > error_setg(errp,
>
Zhao Liu <zhao1.liu@intel.com> writes:
> Hi Markus,
>
>> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
>> > + if (event->u.x86_select_umask.select > UINT12_MAX) {
>> > + error_setg(errp,
>> > + "Parameter 'select' out of range (%d).",
>> > + UINT12_MAX);
>> > + goto fail;
>> > + }
>> > +
>> > + /* No need to check the range of umask since it's uint8_t. */
>> > + break;
>> > + }
>>
>> As we'll see below, the new x86-specific format is defined in the QAPI
>> schema regardless of target.
>>
>> It is accepted here also regardless of target. Doesn't matter much
>> right now, as the object is effectively useless for targets other than
>> x86, but I understand that will change.
>>
>> Should we reject it unless the target is x86?
>
> I previously supposed that different architectures should implement
> their own kvm_arch_check_pmu_filter(), which is the `check` hook of
> object_class_property_add_link():
>
> object_class_property_add_link(oc, "pmu-filter",
> TYPE_KVM_PMU_FILTER,
> offsetof(KVMState, pmu_filter),
> kvm_arch_check_pmu_filter,
> OBJ_PROP_LINK_STRONG);
This way, the checking happens only when you actually connect the
kvm-pmu-filter object to the accelerator.
Have you considered checking in the kvm-pmu-filter object's complete()
method? Simple example of how to do that: qauthz_simple_complete() in
authz/simple.c.
> For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
> kvm.c and checked the supported formats (I also supposed arch-specific
> PMU filter could reject the unsupported format in
> kvm_arch_check_pmu_filter().)
>
> But I think your idea is better, i.e., rejecting unsupported format
> early in pmu-filter parsing.
>
> Well, IIUC, there is no way to specify in QAPI that certain enumerations
> are generic and certain enumerations are arch-specific,
Here's how to make enum values conditional:
{ 'enum': 'KvmPmuEventFormat',
'data': ['raw',
{ 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
{ 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }
However, TARGET_I386 is usable only in target-specific code. This has
two consequences here:
1. It won't compile, since QAPI schema module kvm.json is
target-independent. We'd have to put it into a target-specific
module kvm-target.json.
2. Target-specific QAPI schema mdoules are problematic for the single
binary / heterogeneous machine work. We are discussing how to best
handle that. Unclear whether adding more target-specific QAPI
definitions are a good idea.
> so rejecting
> unsupported format can only happen in parsing code. For example, wrap
> the above code in "#if defined(TARGET_I386)":
>
> for (node = head; node; node = node->next) {
> KvmPmuFilterEvent *event = node->value;
>
> switch (event->format) {
> case KVM_PMU_EVENT_FORMAT_RAW:
> break;
> #if defined(TARGET_I386)
> case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> ...
> break;
> }
> case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
> ...
> break;
> }
> #endif
> default:
> error_setg(errp,
> "Unsupported format.");
> goto fail;
> }
>
> ...
> }
>
> EMM, do you like this idea?
This is kvm_pmu_filter_set_event(), I presume.
The #if is necessary when you make the enum values conditional. The
default: code is unreachable then, so it should stay
g_assert_not_reached().
The #if is fine even when you don't make the enum values conditional.
The default: code is reachable then, unless you reject the unwanted
enums earlier some other way.
>> If not, I feel the behavior should be noted in the commit message.
>
> With the above change, I think it's possible to reject x86-specific
> format on non-x86 arch. And I can also note this behavior in commit
> message.
>
>> > default:
>> > g_assert_not_reached();
>> > }
>> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
>> > filter->events = head;
>> > qapi_free_KvmPmuFilterEventList(old_head);
>> > return;
>> > +
>> > +fail:
>> > + qapi_free_KvmPmuFilterEventList(head);
>> > }
>> >
>> > static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
>
> ...
>
>> > ##
>> > # @KvmPmuFilterEvent:
>> > #
>> > @@ -66,7 +82,8 @@
>> > { 'union': 'KvmPmuFilterEvent',
>> > 'base': { 'format': 'KvmPmuEventFormat' },
>> > 'discriminator': 'format',
>> > - 'data': { 'raw': 'KvmPmuRawEvent' } }
>> > + 'data': { 'raw': 'KvmPmuRawEvent',
>> > + 'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
>> >
>> > ##
>> > # @KvmPmuFilterProperties:
>>
>> Documentation could perhaps be more explicit about this making sense
>> only for x86.
>
> What about the following doc?
>
> ##
> # @KvmPmuFilterProperties:
> #
> # Properties of KVM PMU Filter (only for x86).
Hmm. Branch 'raw' make sense regardless of target, doesn't it? It's
actually usable only for i86 so far, because this series implements
accelerator property "pmu-filter" only for i386.
Let's not worry about this until we decided whether to use QAPI
conditionals or not.
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 51a7c61ce0b0..5dcce067d8dd 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -6180,6 +6180,9 @@ SRST
>> > ((select) & 0xff) | \
>> > ((umask) & 0xff) << 8)
>> >
>> > + ``{"format":"x86-select-umask","select":event_select,"umask":event_umask}``
>> > + Specify the single x86 PMU event with select and umask fields.
>> > +
>> > An example KVM PMU filter object would look like:
>> >
>> > .. parsed-literal::
>> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> > index fa3a696654cb..0d36ccf250ed 100644
>> > --- a/target/i386/kvm/kvm.c
>> > +++ b/target/i386/kvm/kvm.c
>> > @@ -5974,6 +5974,10 @@ static bool kvm_config_pmu_event(KVMPMUFilter *filter,
>> > case KVM_PMU_EVENT_FORMAT_RAW:
>> > code = event->u.raw.code;
>> > break;
>> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>> > + code = X86_PMU_RAW_EVENT(event->u.x86_select_umask.select,
>> > + event->u.x86_select_umask.umask);
>> > + break;
>> > default:
>> > g_assert_not_reached();
>> > }
>> > @@ -6644,6 +6648,7 @@ static void kvm_arch_check_pmu_filter(const Object *obj, const char *name,
>> >
>> > switch (event->format) {
>> > case KVM_PMU_EVENT_FORMAT_RAW:
>> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK:
>
> Here's the current format check I mentioned above. But I agree your idea
> and will check in the parsing of pmu-filter object.
>
>> > break;
>> > default:
>> > error_setg(errp,
>>
On Mon, Apr 28, 2025 at 09:19:07AM +0200, Markus Armbruster wrote:
> Date: Mon, 28 Apr 2025 09:19:07 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
> in KVM PMU filter
>
> Zhao Liu <zhao1.liu@intel.com> writes:
>
> > Hi Markus,
> >
> >> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> >> > + if (event->u.x86_select_umask.select > UINT12_MAX) {
> >> > + error_setg(errp,
> >> > + "Parameter 'select' out of range (%d).",
> >> > + UINT12_MAX);
> >> > + goto fail;
> >> > + }
> >> > +
> >> > + /* No need to check the range of umask since it's uint8_t. */
> >> > + break;
> >> > + }
> >>
> >> As we'll see below, the new x86-specific format is defined in the QAPI
> >> schema regardless of target.
> >>
> >> It is accepted here also regardless of target. Doesn't matter much
> >> right now, as the object is effectively useless for targets other than
> >> x86, but I understand that will change.
> >>
> >> Should we reject it unless the target is x86?
> >
> > I previously supposed that different architectures should implement
> > their own kvm_arch_check_pmu_filter(), which is the `check` hook of
> > object_class_property_add_link():
> >
> > object_class_property_add_link(oc, "pmu-filter",
> > TYPE_KVM_PMU_FILTER,
> > offsetof(KVMState, pmu_filter),
> > kvm_arch_check_pmu_filter,
> > OBJ_PROP_LINK_STRONG);
>
> This way, the checking happens only when you actually connect the
> kvm-pmu-filter object to the accelerator.
>
> Have you considered checking in the kvm-pmu-filter object's complete()
> method? Simple example of how to do that: qauthz_simple_complete() in
> authz/simple.c.
Thank you, I hadn't noticed it before. Now I study it carefully, and yes,
this is a better way than `check` hook. Though in the following we are
talking about other ways to handle target-specific check, this helper
may be still useful as I proposed to help check accel-specific cases in
the reply to Philip [*].
[*]: https://lore.kernel.org/qemu-devel/aA3cHIcKmt3vdkVk@intel.com/
> > For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
> > kvm.c and checked the supported formats (I also supposed arch-specific
> > PMU filter could reject the unsupported format in
> > kvm_arch_check_pmu_filter().)
> >
> > But I think your idea is better, i.e., rejecting unsupported format
> > early in pmu-filter parsing.
> >
> > Well, IIUC, there is no way to specify in QAPI that certain enumerations
> > are generic and certain enumerations are arch-specific,
>
> Here's how to make enum values conditional:
>
> { 'enum': 'KvmPmuEventFormat',
> 'data': ['raw',
> { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
> { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }
What I'm a bit hesitant about is that, if different arches add similar
"conditional" enumerations later, it could cause the enumeration values
to change under different compilation conditions (correct? :-)). Although
it might not break anything, since we don't rely on the specific numeric
values.
> However, TARGET_I386 is usable only in target-specific code. This has
> two consequences here:
>
> 1. It won't compile, since QAPI schema module kvm.json is
> target-independent. We'd have to put it into a target-specific
> module kvm-target.json.
>
> 2. Target-specific QAPI schema mdoules are problematic for the single
> binary / heterogeneous machine work. We are discussing how to best
> handle that. Unclear whether adding more target-specific QAPI
> definitions are a good idea.
And per yours feedback, CONFIG_KVM can also only be used in target-specific
code. Moreover, especially if we need to further consider multiple
accelerators, such as the HVF need mentioned by Philip, it seems that
we can't avoid target-specific issues here!
> > so rejecting
> > unsupported format can only happen in parsing code. For example, wrap
> > the above code in "#if defined(TARGET_I386)":
> >
> > for (node = head; node; node = node->next) {
> > KvmPmuFilterEvent *event = node->value;
> >
> > switch (event->format) {
> > case KVM_PMU_EVENT_FORMAT_RAW:
> > break;
> > #if defined(TARGET_I386)
> > case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
> > ...
> > break;
> > }
> > case KVM_PMU_EVENT_FORMAT_X86_MASKED_ENTRY: {
> > ...
> > break;
> > }
> > #endif
> > default:
> > error_setg(errp,
> > "Unsupported format.");
> > goto fail;
> > }
> >
> > ...
> > }
> >
> > EMM, do you like this idea?
>
> This is kvm_pmu_filter_set_event(), I presume.
>
> The #if is necessary when you make the enum values conditional. The
> default: code is unreachable then, so it should stay
> g_assert_not_reached().
>
> The #if is fine even when you don't make the enum values conditional.
> The default: code is reachable then, unless you reject the unwanted
> enums earlier some other way.
Thanks for your analysis, it's very accurate! I personally prefer the
2nd way.
> >> If not, I feel the behavior should be noted in the commit message.
> >
> > With the above change, I think it's possible to reject x86-specific
> > format on non-x86 arch. And I can also note this behavior in commit
> > message.
> >
> >> > default:
> >> > g_assert_not_reached();
> >> > }
> >> > @@ -67,6 +82,9 @@ static void kvm_pmu_filter_set_event(Object *obj, Visitor *v, const char *name,
> >> > filter->events = head;
> >> > qapi_free_KvmPmuFilterEventList(old_head);
> >> > return;
> >> > +
> >> > +fail:
> >> > + qapi_free_KvmPmuFilterEventList(head);
> >> > }
> >> >
> >> > static void kvm_pmu_filter_class_init(ObjectClass *oc, void *data)
> >
> > ...
> >
> >> > ##
> >> > # @KvmPmuFilterEvent:
> >> > #
> >> > @@ -66,7 +82,8 @@
> >> > { 'union': 'KvmPmuFilterEvent',
> >> > 'base': { 'format': 'KvmPmuEventFormat' },
> >> > 'discriminator': 'format',
> >> > - 'data': { 'raw': 'KvmPmuRawEvent' } }
> >> > + 'data': { 'raw': 'KvmPmuRawEvent',
> >> > + 'x86-select-umask': 'KvmPmuX86SelectUmaskEvent' } }
> >> >
> >> > ##
> >> > # @KvmPmuFilterProperties:
> >>
> >> Documentation could perhaps be more explicit about this making sense
> >> only for x86.
> >
> > What about the following doc?
> >
> > ##
> > # @KvmPmuFilterProperties:
> > #
> > # Properties of KVM PMU Filter (only for x86).
>
> Hmm. Branch 'raw' make sense regardless of target, doesn't it? It's
> actually usable only for i86 so far, because this series implements
> accelerator property "pmu-filter" only for i386.
The advantage of this single note is someone can easily mention other
arch in doc :-)
> Let's not worry about this until we decided whether to use QAPI
> conditionals or not.
OK, this is not a big deal (comparing with other issues).
Thanks,
Zhao
Zhao Liu <zhao1.liu@intel.com> writes:
> On Mon, Apr 28, 2025 at 09:19:07AM +0200, Markus Armbruster wrote:
>> Date: Mon, 28 Apr 2025 09:19:07 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 3/5] i386/kvm: Support event with select & umask format
>> in KVM PMU filter
>>
>> Zhao Liu <zhao1.liu@intel.com> writes:
>>
>> > Hi Markus,
>> >
>> >> > + case KVM_PMU_EVENT_FORMAT_X86_SELECT_UMASK: {
>> >> > + if (event->u.x86_select_umask.select > UINT12_MAX) {
>> >> > + error_setg(errp,
>> >> > + "Parameter 'select' out of range (%d).",
>> >> > + UINT12_MAX);
>> >> > + goto fail;
>> >> > + }
>> >> > +
>> >> > + /* No need to check the range of umask since it's uint8_t. */
>> >> > + break;
>> >> > + }
>> >>
>> >> As we'll see below, the new x86-specific format is defined in the QAPI
>> >> schema regardless of target.
>> >>
>> >> It is accepted here also regardless of target. Doesn't matter much
>> >> right now, as the object is effectively useless for targets other than
>> >> x86, but I understand that will change.
>> >>
>> >> Should we reject it unless the target is x86?
>> >
>> > I previously supposed that different architectures should implement
>> > their own kvm_arch_check_pmu_filter(), which is the `check` hook of
>> > object_class_property_add_link():
>> >
>> > object_class_property_add_link(oc, "pmu-filter",
>> > TYPE_KVM_PMU_FILTER,
>> > offsetof(KVMState, pmu_filter),
>> > kvm_arch_check_pmu_filter,
>> > OBJ_PROP_LINK_STRONG);
>>
>> This way, the checking happens only when you actually connect the
>> kvm-pmu-filter object to the accelerator.
>>
>> Have you considered checking in the kvm-pmu-filter object's complete()
>> method? Simple example of how to do that: qauthz_simple_complete() in
>> authz/simple.c.
>
> Thank you, I hadn't noticed it before. Now I study it carefully, and yes,
> this is a better way than `check` hook. Though in the following we are
> talking about other ways to handle target-specific check, this helper
> may be still useful as I proposed to help check accel-specific cases in
> the reply to Philip [*].
>
> [*]: https://lore.kernel.org/qemu-devel/aA3cHIcKmt3vdkVk@intel.com/
>
>> > For x86, I implemented kvm_arch_check_pmu_filter() in target/i386/kvm/
>> > kvm.c and checked the supported formats (I also supposed arch-specific
>> > PMU filter could reject the unsupported format in
>> > kvm_arch_check_pmu_filter().)
>> >
>> > But I think your idea is better, i.e., rejecting unsupported format
>> > early in pmu-filter parsing.
>> >
>> > Well, IIUC, there is no way to specify in QAPI that certain enumerations
>> > are generic and certain enumerations are arch-specific,
>>
>> Here's how to make enum values conditional:
>>
>> { 'enum': 'KvmPmuEventFormat',
>> 'data': ['raw',
>> { 'name': 'x86-select-umask', 'if': 'TARGET_I386' }
>> { 'name': 'x86-masked-entry', 'if': 'TARGET_I386' } ] }
>
> What I'm a bit hesitant about is that, if different arches add similar
> "conditional" enumerations later, it could cause the enumeration values
> to change under different compilation conditions (correct? :-)). Although
> it might not break anything, since we don't rely on the specific numeric
> values.
Every binary we create contains target-specific code for at most one
target. Therefore, different numerical encodings for different targets
are fine.
Same argument for struct members, by the way. Consider
{ 'struct': 'CpuModelExpansionInfo',
'data': { 'model': 'CpuModelInfo',
'deprecated-props' : { 'type': ['str'],
'if': 'TARGET_S390X' } },
'if': { 'any': [ 'TARGET_S390X',
'TARGET_I386',
'TARGET_ARM',
'TARGET_LOONGARCH64',
'TARGET_RISCV' ] } }
This generates
#if defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV)
struct CpuModelExpansionInfo {
CpuModelInfo *model;
#if defined(TARGET_S390X)
strList *deprecated_props;
#endif /* defined(TARGET_S390X) */
};
#endif /* defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV) */
The struct's size depends on the target. If we ever add members after
@deprecated_props, their offset depends on the target, too.
The single binary work will invalidate the "at most one target"
property. We need to figure out how to best deal with that, but not in
this thread.
[...]
> > What I'm a bit hesitant about is that, if different arches add similar
> > "conditional" enumerations later, it could cause the enumeration values
> > to change under different compilation conditions (correct? :-)). Although
> > it might not break anything, since we don't rely on the specific numeric
> > values.
>
> Every binary we create contains target-specific code for at most one
> target. Therefore, different numerical encodings for different targets
> are fine.
>
> Same argument for struct members, by the way. Consider
>
> { 'struct': 'CpuModelExpansionInfo',
> 'data': { 'model': 'CpuModelInfo',
> 'deprecated-props' : { 'type': ['str'],
> 'if': 'TARGET_S390X' } },
> 'if': { 'any': [ 'TARGET_S390X',
> 'TARGET_I386',
> 'TARGET_ARM',
> 'TARGET_LOONGARCH64',
> 'TARGET_RISCV' ] } }
>
> This generates
>
> #if defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV)
> struct CpuModelExpansionInfo {
> CpuModelInfo *model;
> #if defined(TARGET_S390X)
> strList *deprecated_props;
> #endif /* defined(TARGET_S390X) */
> };
> #endif /* defined(TARGET_S390X) || defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV) */
>
> The struct's size depends on the target. If we ever add members after
> @deprecated_props, their offset depends on the target, too.
Thank your for further explanation!
Regards,
Zhao
© 2016 - 2025 Red Hat, Inc.