Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-27 Thread Zhao Liu 
On Mon, May 27, 2024 at 02:41:01PM +0800, Shaoqin Huang wrote:
> Date: Mon, 27 May 2024 14:41:01 +0800
> From: Shaoqin Huang 
> Subject: Re: [PATCH v9] arm/kvm: Enable support for
>  KVM_ARM_VCPU_PMU_V3_FILTER
> 
> Hi Zhao,
> 
> Thanks for your proposed idea. If you are willing to take the PMU Filter
> Enabling work, you can do it. I won't update this series anymore due to the
> QAPI restriction. I really appreciate if you can implement that.
>

Welcome Shaoqin, I'll cc you when I'm done with the first version (it'll
take some time).

There are also some issues that I might take up and revisit, such as
whether to place the kvm-pmu-filter in -cpu or in -kvm.

Anyway, hopefully eventually we can implement this feature for QEMU and
users can benefit from it!

Thanks,
Zhao

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-27 Thread Shaoqin Huang 

Hi Zhao,

Thanks for your proposed idea. If you are willing to take the PMU Filter 
Enabling work, you can do it. I won't update this series anymore due to 
the QAPI restriction. I really appreciate if you can implement that.


Thanks,
Shaoqin

On 5/13/24 14:52, Zhao Liu wrote:

Hi Daniel,


Please describe it in terms of a QAPI definition, as that's what we're
striving for with all QEMU public interfaces. Once the QAPI design is
agreed, then the -object mapping is trivial, as -object's JSON format
supports arbitrary QAPI structures.


Thank you for your guidance!

I rethought and and modified my previous proposal:

Let me show the command examples firstly:
   * Add a single event:
 (x86) -object kvm-pmu-event,id=e0,action=allow,format=x86-default,\
   select=0x3c,umask=0x00
 (arm or general) -object kvm-pmu-event,id=e1,action=deny,\
  format=raw,code=0x01
  
   * Add a counter bitmap:

 (x86) -object kvm-pmu-counter,id=cnt,action=allow,type=x86-fixed,\
   bitmap=0x
  
   * Add an event list (must use Json syntax format):

(x86) -object 
'{"qom-type":"kvm-pmu-event-list","id"="filter0","action"="allow","format"="x86-default","events=[{"select"=0x3c,"umask"=0x00},{"select"=0x2e,"umask"=0x4f}]'
(arm) -object 
'{"qom-type":"kvm-pmu-event-list","id"="filter1","action"="allow","format"="raw","events"=[{"code"=0x01},{"code"=0x02}]'


The specific JSON definitions are as follows (IIUC, this is "in terms of
a QAPI definition", right? ;-)):
* Define PMU event and counter bitmap with JSON format:
   - basic filter action:

   { 'enum': 'KVMPMUFilterAction',
 'prefix': 'KVM_PMU_FILTER_ACTION',
 'data': ['deny', 'allow' ] }

   - PMU counter:

   { 'enum': 'KVMPMUCounterType',
 'prefix': 'KVM_PMU_COUNTER_TYPE',
 'data': [ 'x86-fixed' ] }

   { 'struct': 'KVMPMUX86FixedCounter',
 'data': { 'bitmap': 'uint32' } }

   - PMU events (total 3 formats):

   # 3 encoding formats: "raw" is compatible with shaoqin's ARM format as
   # well as the x86 raw format, and could support other architectures in
   # the future.
   { 'enum': 'KVMPMUEventEncodeFmt',
 'prefix': 'KVM_PMU_EVENT_ENCODE_FMT',
 'data': ['raw', 'x86-default', 'x86-masked-entry' ] }

   # A general format.
   { 'struct': 'KVMPMURawEvent',
 'data': { 'code': 'uint64' } }

   # x86-specific
   { 'struct': 'KVMPMUX86DefalutEvent',
 'data': { 'select': 'uint16',
   'umask': 'uint16' } }

   # another x86 specific
   { 'struct': 'KVMPMUX86MaskedEntry',
 'data': { 'select': 'uint16',
   'match': 'uint8',
   'mask': 'uint8',
   'exclude': 'bool' } }

   # And their list wrappers:
   { 'struct': 'KVMPMURawEventList',
 'data': { 'events': ['KVMPMURawEvent'] } }

   { 'struct': 'KVMPMUX86DefalutEventList',
 'data': { 'events': ['KVMPMUX86DefalutEvent'] } }

   { 'struct': 'KVMPMUX86MaskedEntryList',
 'data': { 'events': ['KVMPMUX86MaskedEntryList'] } }


Based on the above basic structs, we could provide 3 new more qom-types:
   - 'kvm-pmu-counter': 'KVMPMUFilterCounter'

   # This is a single object option to configure PMU counter
   # bitmap filter.
   { 'union': 'KVMPMUFilterCounter',
 'base': { 'action': 'KVMPMUFilterAction',
   'type': 'KVMPMUCounterType' },
 'discriminator': 'type',
 'data': { 'x86-fixed': 'KVMPMUX86FixedCounter' } }


   - 'kvm-pmu-counter': 'KVMPMUFilterCounter'

   # This option is used to configure a single PMU event for
   # PMU filter.
   { 'union': 'KVMPMUFilterEvent',
 'base': { 'action': 'KVMPMUFilterAction',
   'format': 'KVMPMUEventEncodeFmt' },
 'discriminator': 'format',
 'data': { 'raw': 'KVMPMURawEvent',
   'x86-default': 'KVMPMUX86DefalutEvent',
   'x86-masked-entry': 'KVMPMUX86MaskedEntry' } }


   - 'kvm-pmu-event-list': 'KVMPMUFilterEventList'

   # Used to configure multiple events.
   { 'union': 'KVMPMUFilterEventList',
 'base': { 'action': 'KVMPMUFilterAction',
   'format': 'KVMPMUEventEncodeFmt' },
 'discriminator': 'format',
 'data': { 'raw': 'KVMPMURawEventList',
   'x86-default': 'KVMPMUX86DefalutEventList',
   'x86-masked-entry': 'KVMPMUX86MaskedEntryList' } }


Compared to Shaoqin's original format, kvm-pmu-event-list is not able to
enumerate events continuously (similar to 0x00-0x30 before), and now
user must enumerate events one by one individually.

What do you think about the above 3 new commands?

Thanks and Best Regards,
Zhao



--
Shaoqin

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-15 Thread Daniel P . Berrangé 
On Mon, May 13, 2024 at 02:52:14PM +0800, Zhao Liu wrote:
> Hi Daniel,
> 
> > Please describe it in terms of a QAPI definition, as that's what we're
> > striving for with all QEMU public interfaces. Once the QAPI design is
> > agreed, then the -object mapping is trivial, as -object's JSON format
> > supports arbitrary QAPI structures.
> 
> Thank you for your guidance!
> 
> I rethought and and modified my previous proposal:
> 
> Let me show the command examples firstly:
>   * Add a single event:
> (x86) -object kvm-pmu-event,id=e0,action=allow,format=x86-default,\
>   select=0x3c,umask=0x00
> (arm or general) -object kvm-pmu-event,id=e1,action=deny,\
>  format=raw,code=0x01
>  
>   * Add a counter bitmap:
> (x86) -object kvm-pmu-counter,id=cnt,action=allow,type=x86-fixed,\
>   bitmap=0x
>  
>   * Add an event list (must use Json syntax format):
>(x86) -object 
> '{"qom-type":"kvm-pmu-event-list","id"="filter0","action"="allow","format"="x86-default","events=[{"select"=0x3c,"umask"=0x00},{"select"=0x2e,"umask"=0x4f}]'
>(arm) -object 
> '{"qom-type":"kvm-pmu-event-list","id"="filter1","action"="allow","format"="raw","events"=[{"code"=0x01},{"code"=0x02}]'
> 
> 
> The specific JSON definitions are as follows (IIUC, this is "in terms of
> a QAPI definition", right? ;-)): 
> * Define PMU event and counter bitmap with JSON format:
>   - basic filter action:
> 
>   { 'enum': 'KVMPMUFilterAction',
> 'prefix': 'KVM_PMU_FILTER_ACTION',
> 'data': ['deny', 'allow' ] }
> 
>   - PMU counter:
> 
>   { 'enum': 'KVMPMUCounterType',
> 'prefix': 'KVM_PMU_COUNTER_TYPE',
> 'data': [ 'x86-fixed' ] }
> 
>   { 'struct': 'KVMPMUX86FixedCounter',
> 'data': { 'bitmap': 'uint32' } }
> 
>   - PMU events (total 3 formats):
> 
>   # 3 encoding formats: "raw" is compatible with shaoqin's ARM format as
>   # well as the x86 raw format, and could support other architectures in
>   # the future.
>   { 'enum': 'KVMPMUEventEncodeFmt',
> 'prefix': 'KVM_PMU_EVENT_ENCODE_FMT',
> 'data': ['raw', 'x86-default', 'x86-masked-entry' ] }
> 
>   # A general format.
>   { 'struct': 'KVMPMURawEvent',
> 'data': { 'code': 'uint64' } }
> 
>   # x86-specific
>   { 'struct': 'KVMPMUX86DefalutEvent',
> 'data': { 'select': 'uint16',
>   'umask': 'uint16' } }
> 
>   # another x86 specific
>   { 'struct': 'KVMPMUX86MaskedEntry',
> 'data': { 'select': 'uint16',
>   'match': 'uint8',
>   'mask': 'uint8',
>   'exclude': 'bool' } }
> 
>   # And their list wrappers:
>   { 'struct': 'KVMPMURawEventList',
> 'data': { 'events': ['KVMPMURawEvent'] } }
> 
>   { 'struct': 'KVMPMUX86DefalutEventList',
> 'data': { 'events': ['KVMPMUX86DefalutEvent'] } }
> 
>   { 'struct': 'KVMPMUX86MaskedEntryList',
> 'data': { 'events': ['KVMPMUX86MaskedEntryList'] } }
> 
> 
> Based on the above basic structs, we could provide 3 new more qom-types:
>   - 'kvm-pmu-counter': 'KVMPMUFilterCounter'
> 
>   # This is a single object option to configure PMU counter
>   # bitmap filter.
>   { 'union': 'KVMPMUFilterCounter',
> 'base': { 'action': 'KVMPMUFilterAction',
>   'type': 'KVMPMUCounterType' },
> 'discriminator': 'type',
> 'data': { 'x86-fixed': 'KVMPMUX86FixedCounter' } }
> 
> 
>   - 'kvm-pmu-counter': 'KVMPMUFilterCounter'
> 
>   # This option is used to configure a single PMU event for
>   # PMU filter.
>   { 'union': 'KVMPMUFilterEvent',
> 'base': { 'action': 'KVMPMUFilterAction',
>   'format': 'KVMPMUEventEncodeFmt' },
> 'discriminator': 'format',
> 'data': { 'raw': 'KVMPMURawEvent',
>   'x86-default': 'KVMPMUX86DefalutEvent',
>   'x86-masked-entry': 'KVMPMUX86MaskedEntry' } }
> 
> 
>   - 'kvm-pmu-event-list': 'KVMPMUFilterEventList'
> 
>   # Used to configure multiple events.
>   { 'union': 'KVMPMUFilterEventList',
> 'base': { 'action': 'KVMPMUFilterAction',
>   'format': 'KVMPMUEventEncodeFmt' },
> 'discriminator': 'format',
> 'data': { 'raw': 'KVMPMURawEventList',
>   'x86-default': 'KVMPMUX86DefalutEventList',
>   'x86-masked-entry': 'KVMPMUX86MaskedEntryList' } }
> 
> 
> Compared to Shaoqin's original format, kvm-pmu-event-list is not able to
> enumerate events continuously (similar to 0x00-0x30 before), and now
> user must enumerate events one by one individually.
> 
> What do you think about the above 3 new commands?

I don't know enough about KVM PMU to give feedback on the specific
choices, but in terms of how to do QAPI design, this looks like a
good start.


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 :|

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-13 Thread Zhao Liu 
Hi Daniel,

> Please describe it in terms of a QAPI definition, as that's what we're
> striving for with all QEMU public interfaces. Once the QAPI design is
> agreed, then the -object mapping is trivial, as -object's JSON format
> supports arbitrary QAPI structures.

Thank you for your guidance!

I rethought and and modified my previous proposal:

Let me show the command examples firstly:
  * Add a single event:
(x86) -object kvm-pmu-event,id=e0,action=allow,format=x86-default,\
  select=0x3c,umask=0x00
(arm or general) -object kvm-pmu-event,id=e1,action=deny,\
 format=raw,code=0x01
 
  * Add a counter bitmap:
(x86) -object kvm-pmu-counter,id=cnt,action=allow,type=x86-fixed,\
  bitmap=0x
 
  * Add an event list (must use Json syntax format):
   (x86) -object 
'{"qom-type":"kvm-pmu-event-list","id"="filter0","action"="allow","format"="x86-default","events=[{"select"=0x3c,"umask"=0x00},{"select"=0x2e,"umask"=0x4f}]'
   (arm) -object 
'{"qom-type":"kvm-pmu-event-list","id"="filter1","action"="allow","format"="raw","events"=[{"code"=0x01},{"code"=0x02}]'


The specific JSON definitions are as follows (IIUC, this is "in terms of
a QAPI definition", right? ;-)): 
* Define PMU event and counter bitmap with JSON format:
  - basic filter action:

  { 'enum': 'KVMPMUFilterAction',
'prefix': 'KVM_PMU_FILTER_ACTION',
'data': ['deny', 'allow' ] }

  - PMU counter:

  { 'enum': 'KVMPMUCounterType',
'prefix': 'KVM_PMU_COUNTER_TYPE',
'data': [ 'x86-fixed' ] }

  { 'struct': 'KVMPMUX86FixedCounter',
'data': { 'bitmap': 'uint32' } }

  - PMU events (total 3 formats):

  # 3 encoding formats: "raw" is compatible with shaoqin's ARM format as
  # well as the x86 raw format, and could support other architectures in
  # the future.
  { 'enum': 'KVMPMUEventEncodeFmt',
'prefix': 'KVM_PMU_EVENT_ENCODE_FMT',
'data': ['raw', 'x86-default', 'x86-masked-entry' ] }

  # A general format.
  { 'struct': 'KVMPMURawEvent',
'data': { 'code': 'uint64' } }

  # x86-specific
  { 'struct': 'KVMPMUX86DefalutEvent',
'data': { 'select': 'uint16',
  'umask': 'uint16' } }

  # another x86 specific
  { 'struct': 'KVMPMUX86MaskedEntry',
'data': { 'select': 'uint16',
  'match': 'uint8',
  'mask': 'uint8',
  'exclude': 'bool' } }

  # And their list wrappers:
  { 'struct': 'KVMPMURawEventList',
'data': { 'events': ['KVMPMURawEvent'] } }

  { 'struct': 'KVMPMUX86DefalutEventList',
'data': { 'events': ['KVMPMUX86DefalutEvent'] } }

  { 'struct': 'KVMPMUX86MaskedEntryList',
'data': { 'events': ['KVMPMUX86MaskedEntryList'] } }


Based on the above basic structs, we could provide 3 new more qom-types:
  - 'kvm-pmu-counter': 'KVMPMUFilterCounter'

  # This is a single object option to configure PMU counter
  # bitmap filter.
  { 'union': 'KVMPMUFilterCounter',
'base': { 'action': 'KVMPMUFilterAction',
  'type': 'KVMPMUCounterType' },
'discriminator': 'type',
'data': { 'x86-fixed': 'KVMPMUX86FixedCounter' } }


  - 'kvm-pmu-counter': 'KVMPMUFilterCounter'

  # This option is used to configure a single PMU event for
  # PMU filter.
  { 'union': 'KVMPMUFilterEvent',
'base': { 'action': 'KVMPMUFilterAction',
  'format': 'KVMPMUEventEncodeFmt' },
'discriminator': 'format',
'data': { 'raw': 'KVMPMURawEvent',
  'x86-default': 'KVMPMUX86DefalutEvent',
  'x86-masked-entry': 'KVMPMUX86MaskedEntry' } }


  - 'kvm-pmu-event-list': 'KVMPMUFilterEventList'

  # Used to configure multiple events.
  { 'union': 'KVMPMUFilterEventList',
'base': { 'action': 'KVMPMUFilterAction',
  'format': 'KVMPMUEventEncodeFmt' },
'discriminator': 'format',
'data': { 'raw': 'KVMPMURawEventList',
  'x86-default': 'KVMPMUX86DefalutEventList',
  'x86-masked-entry': 'KVMPMUX86MaskedEntryList' } }


Compared to Shaoqin's original format, kvm-pmu-event-list is not able to
enumerate events continuously (similar to 0x00-0x30 before), and now
user must enumerate events one by one individually.

What do you think about the above 3 new commands?

Thanks and Best Regards,
Zhao

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-09 Thread Daniel P . Berrangé 
On Thu, May 09, 2024 at 05:48:19PM +0800, Zhao Liu wrote:
> Hi Daniel & Shaoqin,
> 
> Since x86 also needs to implement PMU filter feature, though it uses
> the different KVM ioctl, we can still make the QEMU API as general as
> possible.
> 
> To move forward with both ARM and x86, I'd like to discuss my API
> thinking with you. ;-)
> 
> On Mon, Apr 15, 2024 at 06:29:25PM +0100, Daniel P. Berrangé wrote:
> > Date: Mon, 15 Apr 2024 18:29:25 +0100
> > From: "Daniel P. Berrangé" 
> > Subject: Re: [PATCH v9] arm/kvm: Enable support for
> >  KVM_ARM_VCPU_PMU_V3_FILTER
> > 
> > On Mon, Apr 08, 2024 at 10:49:40PM -0400, Shaoqin Huang wrote:
> > > The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
> > > which PMU events are provided to the guest. Add a new option
> > > `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
> > > Without the filter, all PMU events are exposed from host to guest by
> > > default. The usage of the new sub-option can be found from the updated
> > > document (docs/system/arm/cpu-features.rst).
> > > 
> > > Here is an example which shows how to use the PMU Event Filtering, when
> > > we launch a guest by use kvm, add such command line:
> > > 
> > >   # qemu-system-aarch64 \
> > > -accel kvm \
> > > -cpu host,kvm-pmu-filter="D:0x11-0x11"
> > 
> > I'm still against implementing this one-off custom parsed syntax
> > for kvm-pmu-filter values. Once this syntax exists, we're locked
> > into back-compatibility for multiple releases, and it will make
> > a conversion to QAPI/JSON harder.
> 
> Daniel, I understand you mean the new specific string format makes
> external API support more complicated, right?
> 
> What about the following options:
> 
> 1. Firstly, add a feature flag option in "-cpu" to enable kvm_filter
> feature for CPU:
> 
> -cpu host,kvm-pmu-filter
> 
> 2. Then use "-object kvm-pmu-event" to configure PMU event properties.
> Since x86's PMU filter has very complex encoding rules, we need the
> following three variants (one for general case, the other two are x86
> specific):
> 
> - General format:
>   -object kvm-pmu-event,action=[allowed|denied],events=[event-list]
> 
>   e.g, as Shaoqin's example,
>   -object kvm-pmu-event,action=allowed,events=0x11-0x11,0x23-0x23
>   -object kvm-pmu-event,action=denied,events=0x23-0x3a
> 
> - x86 raw_event encoding format (for single raw format event encoding):
>   -object kvm-pmu-event,action=[allowed|denied],mode=0,select="0x01",
>   umask="0x3c",fixed-bitmap="0x"
> 
> - x86 masked_event encoding format (for mutiple masked event encoding):
>   -object kvm-pmu-event,action=[allowed|denied],mode=masked,select="0x01",
>   mask="0x3c",match="0x11",exclude=true|false
> 
> The whole API architecture looks more complex, but has the advantage of
> being as general as possible and avoiding the introduction of new string
> format parsing.
> 
> What do you think? Because the most important thing about this feature
> is the API design, welcome your comments!

Please describe it in terms of a QAPI definition, as that's what we're
striving for with all QEMU public interfaces. Once the QAPI design is
agreed, then the -object mapping is trivial, as -object's JSON format
supports arbitrary QAPI structures.


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 :|

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-09 Thread Zhao Liu 
Hi Daniel & Shaoqin,

Since x86 also needs to implement PMU filter feature, though it uses
the different KVM ioctl, we can still make the QEMU API as general as
possible.

To move forward with both ARM and x86, I'd like to discuss my API
thinking with you. ;-)

On Mon, Apr 15, 2024 at 06:29:25PM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 15 Apr 2024 18:29:25 +0100
> From: "Daniel P. Berrangé" 
> Subject: Re: [PATCH v9] arm/kvm: Enable support for
>  KVM_ARM_VCPU_PMU_V3_FILTER
> 
> On Mon, Apr 08, 2024 at 10:49:40PM -0400, Shaoqin Huang wrote:
> > The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
> > which PMU events are provided to the guest. Add a new option
> > `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
> > Without the filter, all PMU events are exposed from host to guest by
> > default. The usage of the new sub-option can be found from the updated
> > document (docs/system/arm/cpu-features.rst).
> > 
> > Here is an example which shows how to use the PMU Event Filtering, when
> > we launch a guest by use kvm, add such command line:
> > 
> >   # qemu-system-aarch64 \
> > -accel kvm \
> > -cpu host,kvm-pmu-filter="D:0x11-0x11"
> 
> I'm still against implementing this one-off custom parsed syntax
> for kvm-pmu-filter values. Once this syntax exists, we're locked
> into back-compatibility for multiple releases, and it will make
> a conversion to QAPI/JSON harder.

Daniel, I understand you mean the new specific string format makes
external API support more complicated, right?

What about the following options:

1. Firstly, add a feature flag option in "-cpu" to enable kvm_filter
feature for CPU:

-cpu host,kvm-pmu-filter

2. Then use "-object kvm-pmu-event" to configure PMU event properties.
Since x86's PMU filter has very complex encoding rules, we need the
following three variants (one for general case, the other two are x86
specific):

- General format:
  -object kvm-pmu-event,action=[allowed|denied],events=[event-list]

  e.g, as Shaoqin's example,
  -object kvm-pmu-event,action=allowed,events=0x11-0x11,0x23-0x23
  -object kvm-pmu-event,action=denied,events=0x23-0x3a

- x86 raw_event encoding format (for single raw format event encoding):
  -object kvm-pmu-event,action=[allowed|denied],mode=0,select="0x01",
  umask="0x3c",fixed-bitmap="0x"

- x86 masked_event encoding format (for mutiple masked event encoding):
  -object kvm-pmu-event,action=[allowed|denied],mode=masked,select="0x01",
  mask="0x3c",match="0x11",exclude=true|false

The whole API architecture looks more complex, but has the advantage of
being as general as possible and avoiding the introduction of new string
format parsing.

What do you think? Because the most important thing about this feature
is the API design, welcome your comments!

Regards,
Zhao

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-05-07 Thread Shaoqin Huang 

Hi Daniel,

On 4/16/24 01:29, Daniel P. Berrangé wrote:

On Mon, Apr 08, 2024 at 10:49:40PM -0400, Shaoqin Huang wrote:

The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
which PMU events are provided to the guest. Add a new option
`kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
Without the filter, all PMU events are exposed from host to guest by
default. The usage of the new sub-option can be found from the updated
document (docs/system/arm/cpu-features.rst).

Here is an example which shows how to use the PMU Event Filtering, when
we launch a guest by use kvm, add such command line:

   # qemu-system-aarch64 \
 -accel kvm \
 -cpu host,kvm-pmu-filter="D:0x11-0x11"


I'm still against implementing this one-off custom parsed syntax
for kvm-pmu-filter values. Once this syntax exists, we're locked
into back-compatibility for multiple releases, and it will make
a conversion to QAPI/JSON harder.


Thanks for your effort of reviewing my patch. I think if I need cost 
more time about the QAPI, that's outside my initial idea and deviate 
from supporting the PMU Filter.


So I decide to not update this patch now. And wait until I have time to 
look into the QAPI or the -cpu option has been transformed to QAPI format.


Thanks,
Shaoqin



With regards,
Daniel


--
Shaoqin

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-16 Thread Cornelia Huck 
On Wed, Apr 10 2024, Thomas Huth  wrote:

> On 09/04/2024 09.47, Shaoqin Huang wrote:
>> Hi Thmoas,
>> 
>> On 4/9/24 13:33, Thomas Huth wrote:
 +    assert_has_feature(qts, "host", "kvm-pmu-filter");
>>>
>>> So you assert here that the feature is available ...
>>>
   assert_has_feature(qts, "host", "kvm-steal-time");
   assert_has_feature(qts, "host", "sve");
   resp = do_query_no_props(qts, "host");
 +    kvm_supports_pmu_filter = resp_get_feature_str(resp, 
 "kvm-pmu-filter");
   kvm_supports_steal_time = resp_get_feature(resp, 
 "kvm-steal-time");
   kvm_supports_sve = resp_get_feature(resp, "sve");
   vls = resp_get_sve_vls(resp);
   qobject_unref(resp);
 +    if (kvm_supports_pmu_filter) { >
>>> ... why do you then need to check for its availability here again?
>>> I either don't understand this part of the code, or you could drop the 
>>> kvm_supports_pmu_filter variable and simply always execute the code below.
>> 
>> Thanks for your reviewing. I did so because all other feature like 
>> "kvm-steal-time" check its availability again. I don't know the original 
>> reason why they did that. I just followed it.
>> 
>> Do you think we should delete all the checking?
>
> resp_get_feature() seems to return a boolean value, so though these feature 
> could be there, they still could be disabled, I assume? Thus we likely need 
> to keep the check for those.

This had confused me as well when I looked at it the last time -- one
thing is to check whether we have a certain prop in the cpu model, the
other one whether we actually support it. Maybe this needs some
comments?

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-15 Thread Daniel P . Berrangé 
On Mon, Apr 08, 2024 at 10:49:40PM -0400, Shaoqin Huang wrote:
> The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
> which PMU events are provided to the guest. Add a new option
> `kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
> Without the filter, all PMU events are exposed from host to guest by
> default. The usage of the new sub-option can be found from the updated
> document (docs/system/arm/cpu-features.rst).
> 
> Here is an example which shows how to use the PMU Event Filtering, when
> we launch a guest by use kvm, add such command line:
> 
>   # qemu-system-aarch64 \
> -accel kvm \
> -cpu host,kvm-pmu-filter="D:0x11-0x11"

I'm still against implementing this one-off custom parsed syntax
for kvm-pmu-filter values. Once this syntax exists, we're locked
into back-compatibility for multiple releases, and it will make
a conversion to QAPI/JSON harder.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-10 Thread Thomas Huth 

On 09/04/2024 09.47, Shaoqin Huang wrote:

Hi Thmoas,

On 4/9/24 13:33, Thomas Huth wrote:

+    assert_has_feature(qts, "host", "kvm-pmu-filter");


So you assert here that the feature is available ...


  assert_has_feature(qts, "host", "kvm-steal-time");
  assert_has_feature(qts, "host", "sve");
  resp = do_query_no_props(qts, "host");
+    kvm_supports_pmu_filter = resp_get_feature_str(resp, 
"kvm-pmu-filter");
  kvm_supports_steal_time = resp_get_feature(resp, 
"kvm-steal-time");

  kvm_supports_sve = resp_get_feature(resp, "sve");
  vls = resp_get_sve_vls(resp);
  qobject_unref(resp);
+    if (kvm_supports_pmu_filter) { >

... why do you then need to check for its availability here again?
I either don't understand this part of the code, or you could drop the 
kvm_supports_pmu_filter variable and simply always execute the code below.


Thanks for your reviewing. I did so because all other feature like 
"kvm-steal-time" check its availability again. I don't know the original 
reason why they did that. I just followed it.


Do you think we should delete all the checking?


resp_get_feature() seems to return a boolean value, so though these feature 
could be there, they still could be disabled, I assume? Thus we likely need 
to keep the check for those.


 Thomas

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-09 Thread Shaoqin Huang 

Hi Thmoas,

On 4/9/24 13:33, Thomas Huth wrote:

+    assert_has_feature(qts, "host", "kvm-pmu-filter");


So you assert here that the feature is available ...


  assert_has_feature(qts, "host", "kvm-steal-time");
  assert_has_feature(qts, "host", "sve");
  resp = do_query_no_props(qts, "host");
+    kvm_supports_pmu_filter = resp_get_feature_str(resp, 
"kvm-pmu-filter");
  kvm_supports_steal_time = resp_get_feature(resp, 
"kvm-steal-time");

  kvm_supports_sve = resp_get_feature(resp, "sve");
  vls = resp_get_sve_vls(resp);
  qobject_unref(resp);
+    if (kvm_supports_pmu_filter) { >

... why do you then need to check for its availability here again?
I either don't understand this part of the code, or you could drop the 
kvm_supports_pmu_filter variable and simply always execute the code below.


Thanks for your reviewing. I did so because all other feature like 
"kvm-steal-time" check its availability again. I don't know the original 
reason why they did that. I just followed it.


Do you think we should delete all the checking?

Thanks,
Shaoqin



  Thomas



--
Shaoqin

Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-08 Thread Thomas Huth 

On 09/04/2024 04.49, Shaoqin Huang wrote:

The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability to let the VMM decide
which PMU events are provided to the guest. Add a new option
`kvm-pmu-filter` as -cpu sub-option to set the PMU Event Filtering.
Without the filter, all PMU events are exposed from host to guest by
default. The usage of the new sub-option can be found from the updated
document (docs/system/arm/cpu-features.rst).

Here is an example which shows how to use the PMU Event Filtering, when
we launch a guest by use kvm, add such command line:

   # qemu-system-aarch64 \
 -accel kvm \
 -cpu host,kvm-pmu-filter="D:0x11-0x11"

Since the first action is deny, we have a global allow policy. This
filters out the cycle counter (event 0x11 being CPU_CYCLES).

And then in guest, use the perf to count the cycle:

   # perf stat sleep 1

Performance counter stats for 'sleep 1':

   1.22 msec task-clock   #0.001 CPUs 
utilized
  1  context-switches #  820.695 /sec
  0  cpu-migrations   #0.000 /sec
 55  page-faults  #   45.138 K/sec
  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.0 seconds sys

As we can see, the cycle counter has been disabled in the guest, but
other pmu events do still work.

Signed-off-by: Shaoqin Huang 
---
v8->v9:
   - Replace the warn_report to error_setg in some places.
   - Merge the check condition to make code more clean.
   - Try to use the QAPI format for the PMU Filter property but failed to use it
   since the -cpu option doesn't support json format yet.

v7->v8:
   - Add qtest for kvm-pmu-filter.
   - Do the kvm-pmu-filter syntax checking up-front in the kvm_pmu_filter_set()
   function. And store the filter information at there. When 
kvm_pmu_filter_get()
   reconstitute it.

v6->v7:
   - Check return value of sscanf.
   - Improve the check condition.

v5->v6:
   - Commit message improvement.
   - Remove some unused code.
   - Collect Reviewed-by, thanks Sebastian.
   - Use g_auto(Gstrv) to replace the gchar **.  [Eric]

v4->v5:
   - Change the kvm-pmu-filter as a -cpu sub-option. [Eric]
   - Comment tweak.  [Gavin]
   - Rebase to the latest branch.

v3->v4:
   - Fix the wrong check for pmu_filter_init.[Sebastian]
   - Fix multiple alignment issue.   [Gavin]
   - Report error by warn_report() instead of error_report(), and don't use
   abort() since the PMU Event Filter is an add-on and best-effort feature.
 [Gavin]
   - Add several missing {  } for single line of code.   [Gavin]
   - Use the g_strsplit() to replace strtok().   [Gavin]

v2->v3:
   - Improve commits message, use kernel doc wording, add more explaination on
 filter example, fix some typo error.[Eric]
   - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
   - Add more precise error message report.  [Eric]
   - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support 
in
 KVM.[Eric]

v1->v2:
   - Add more description for allow and deny meaning in
 commit message. [Sebastian]
   - Small improvement.  [Sebastian]
---
  docs/system/arm/cpu-features.rst |  23 +++
  target/arm/arm-qmp-cmds.c|   2 +-
  target/arm/cpu.h |   3 +
  target/arm/kvm.c | 112 +++
  tests/qtest/arm-cpu-features.c   |  51 ++
  5 files changed, 190 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index a5fb929243..f3930f34b3 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -204,6 +204,29 @@ the list of KVM VCPU features and their descriptions.
the guest scheduler behavior and/or be exposed to the guest
userspace.
  
+``kvm-pmu-filter``

+  By default kvm-pmu-filter is disabled. This means that by default all PMU
+  events will be exposed to guest.
+
+  KVM implements PMU Event Filtering to prevent a guest from being able to
+  sample certain events. It depends on the KVM_ARM_VCPU_PMU_V3_FILTER
+  attribute supported in KVM. It has the following format:
+
+  kvm-pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
+
+  The A means "allow" and D means "deny", start is the first event of the
+  range and the end is the last one. The first registered range defines
+  the global policy (global ALLOW if the

[PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

2024-04-08 Thread Shaoqin Huang 
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
 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.0 seconds sys

As we can see, the cycle counter has been disabled in the guest, but
other pmu events do still work.

Signed-off-by: Shaoqin Huang 
---
v8->v9:
  - Replace the warn_report to error_setg in some places.
  - Merge the check condition to make code more clean.
  - Try to use the QAPI format for the PMU Filter property but failed to use it
  since the -cpu option doesn't support json format yet.

v7->v8:
  - Add qtest for kvm-pmu-filter.
  - Do the kvm-pmu-filter syntax checking up-front in the kvm_pmu_filter_set()
  function. And store the filter information at there. When kvm_pmu_filter_get()
  reconstitute it.

v6->v7:
  - Check return value of sscanf.
  - Improve the check condition.

v5->v6:
  - Commit message improvement.
  - Remove some unused code.
  - Collect Reviewed-by, thanks Sebastian.
  - Use g_auto(Gstrv) to replace the gchar **.  [Eric]

v4->v5:
  - Change the kvm-pmu-filter as a -cpu sub-option. [Eric]
  - Comment tweak.  [Gavin]
  - Rebase to the latest branch.

v3->v4:
  - Fix the wrong check for pmu_filter_init.[Sebastian]
  - Fix multiple alignment issue.   [Gavin]
  - Report error by warn_report() instead of error_report(), and don't use
  abort() since the PMU Event Filter is an add-on and best-effort feature.
[Gavin]
  - Add several missing {  } for single line of code.   [Gavin]
  - Use the g_strsplit() to replace strtok().   [Gavin]

v2->v3:
  - Improve commits message, use kernel doc wording, add more explaination on
filter example, fix some typo error.[Eric]
  - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
  - Add more precise error message report.  [Eric]
  - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support in
KVM.[Eric]

v1->v2:
  - Add more description for allow and deny meaning in 
commit message. [Sebastian]
  - Small improvement.  [Sebastian]
---
 docs/system/arm/cpu-features.rst |  23 +++
 target/arm/arm-qmp-cmds.c|   2 +-
 target/arm/cpu.h |   3 +
 target/arm/kvm.c | 112 +++
 tests/qtest/arm-cpu-features.c   |  51 ++
 5 files changed, 190 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index a5fb929243..f3930f34b3 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -204,6 +204,29 @@ the list of KVM VCPU features and their descriptions.
   the guest scheduler behavior and/or be exposed to the guest
   userspace.
 
+``kvm-pmu-filter``
+  By default kvm-pmu-filter is disabled. This means that by default all PMU
+  events will be exposed to guest.
+
+  KVM implements PMU Event Filtering to prevent a guest from being able to
+  sample certain events. It depends on the KVM_ARM_VCPU_PMU_V3_FILTER
+  attribute supported in KVM. It has the following format:
+
+  kvm-pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"
+
+  The A means "allow" and D means "deny", start is the first event of the
+  range and the end is the last one. The first registered range defines
+  the global policy (global ALLOW if the first action is DENY, global DENY
+  if the first action is ALLOW). The start and end only support