Zhao Liu <zhao1....@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 <arm...@redhat.com>
>> Subject: Re: KVM/QEMU Community call 29/04/2025 agenda items?
>>
>> Philippe Mathieu-Daudé <phi...@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-phi...@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-phi...@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.
