Re: KVM/QEMU Community call 29/04/2025 agenda items?

[Markus Armbruster <armbru@redhat.com>]

Zhao Liu <zhao1.liu@intel.com> writes:

> On Wed, May 07, 2025 at 05:11:39PM +0200, Markus Armbruster wrote:
>> Date: Wed, 07 May 2025 17:11:39 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: KVM/QEMU Community call 29/04/2025 agenda items?
>> 
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>> > On 30/4/25 12:41, Markus Armbruster wrote:
>> 
>> [...]
>> 
>> >> Pierrick's stated goal is to have no noticable differences between the
>> >> single binary and the qemu-system-<target> it covers.
>> >> 
>> >> We have two external interfaces to worry about: QMP and the command
>> >> line.  Let's ignore the latter for now.
>> >> 
>> >> Target-specific differences in *syntax* come from QAPI schema
>> >> conditionals with target-specific conditions.  Example:
>> >> 
>> >>      { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
>> >>        'if': { 'any': [ 'TARGET_PPC',
>> >>                         'TARGET_ARM',
>> >>                         'TARGET_I386',
>> >>                         'TARGET_S390X',
>> >>                         'TARGET_MIPS',
>> >>                         'TARGET_LOONGARCH64',
>> >>                         'TARGET_RISCV' ] } }
>> >> 
>> >> This command is only defined for some targets.
>> >> 
>> >> The value of query-qmp-schema reflects this: it has
>> >> query-cpu-definitions exactly when the condition is satisfied.  The
>> >> condition is evaluated at compile-time, because that's how QAPI schema
>> >> 'if' works.
>> >> 
>> >> Say we drop the condition and instead add an equivalent run-time
>> >> condition to command registration.  This preserves behavior of command
>> >> execution.  But query-qmp-schema now has query-cpu-definitions *always*.
>> >> This is a noticable difference.  It may break management applications
>> >> that use query-qmp-schema to probe for the command.
>> >> 
>> >> Moreover, conditionals aren't limited to commands.  Example:
>> >> 
>> >>      { 'struct': 'CpuModelExpansionInfo',
>> >>        'data': { 'model': 'CpuModelInfo',
>> >>                  'deprecated-props' : { 'type': ['str'],
>> >> --->                                   'if': 'TARGET_S390X' } },
>> >>        'if': { 'any': [ 'TARGET_S390X',
>> >>                         'TARGET_I386',
>> >>                         'TARGET_ARM',
>> >>                         'TARGET_LOONGARCH64',
>> >>                         'TARGET_RISCV' ] } }
>> >> 
>> >> Here we have a conditional member.
>> 
>> [...]
>> 
>> > IMHO conditionals should only depend on host / static configuration
>> > features, not features modifiable from the command line.
>> 
>> This is how the QAPI schema works now.
>> 
>> >                                                          (I'm always
>> > confused by KVM features published in the schema, but then you start
>> > your binary with -accel=tcg and still can run KVM specific commands
>> > via QMP, returning errors).
>> 
>> Not exactly a ringing endorsement for keeping the QAPI schema work the
>> way it does now, isn't it?  ;)
>> 
>> The problem at hand is QAPI-generated files in a single binary.
>> 
>> Pierrick posted "[RFC PATCH 0/3] single-binary: make QAPI generated
>> files common".  The patches are flawed, but that's alright for RFC.
>> 
>> In review, I pointed out three possible solutions, and discussed their
>> pros and cons:
>> 
>> (1) Drop target-specific conditionals.
>> 
>> (2) Replace them by run-time checks.
>> 
>> (3) Have target-specific QAPI-generated code for multiple targets
>>     coexist in the single binary.
>
> Thank you Markus for your nice summary! I also apologize to you and
> Philippe if I didn't understand correctly (I tried to look at
> Pierrick's and Philippe's work, but still worry about I may have wrong
> understanding :-) )
>
>> Both (1) and (3) keep the QAPI schema work as it does now.
>> 
>> Pierrick's patches are an incomplete attempt at (2).
>
> I see.
>
>> Daniel made a case for (1).  You and I actually discussed (1) before,
>> and I encouraged you to explore it.
>
> And I notice Philippe has 2 patches:
>
> (1) For QAPI, this is to drop target-specific cond:
>
> https://lore.kernel.org/qemu-devel/20250429100419.20427-1-philmd@linaro.org/
>
> I feel it's a smart way to make it optional.
>
> (2) Not for QAPI, but this is try to add runtime check:
>
> https://lore.kernel.org/qemu-devel/20250502214551.80401-5-philmd@linaro.org/
>
> I am thinking about how kvm-pmu-filter could be aligned with these
> current efforts in mail list.
>
> I can drop all the target conditions for KvmPmuEventFormat:
>
>     { 'enum': 'KvmPmuEventFormat',
>       'data': ['raw', 'x86-select-umask', 'x86-masked-entry'] }

This is what you need to do if we pick solution (1).

> This is what you listed before and it is similar to the way in (1). But
> only (1) is not enough because I can't make these formats as optional
> (pls educate me if I'm wrong :-) ).
>
> Therefore, I think I need run-time check like Philippe did in (2), in
> my kvm-pmu.c file. Do you think so?

In general, not just for your KVM PMU filter work: dropping
target-specific conditionals makes the formerly conditional things exist
for targets where they didn't exist before.  This commonly breaks the
build for these targets.  To fix a target's build, we can either
implement things for the target, or make attempts to use them fail
cleanly.

> An additional question - about CONFIG_KVM - is that I see that it's all
> currently focused on solving target-specific problems, and I understand
> that it's not yet kvm-specific's turn.
>
> But kvm-pmu-filter does (or, may :-) ) need CONFIG_KVM. My initial
> thought is, is it possible to remove the CONFIG_KVM condition for
> kvm-pmu-filter in QAPI and use the same runtime check? But this would
> require accel-info as Philippe's work on target-info in [2]).

I'm afraid I don't understand your question.

>> We can certainly discuss this some more, but I'd prefer to review a
>> working solution instead.
>
> With so many options on the table, I'm a bit confused about the fate of
> kvm-pmu-filter. Should this case wait for the most appropriate option to
> come along, or could it move forward based on the status quo, and then
> consider the refactoring later with the final optimal option?

Up to you.

We'll hopefully reach consensus fairly soon.  You might want to wait a
few days for that.