Re: [PATCH 05/13] target/arm/kvm_arm: copy definitions from kvm headers

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 30/4/25 00:02, Pierrick Bouvier wrote:
> On 4/29/25 2:14 PM, Pierrick Bouvier wrote:
>> On 4/29/25 3:28 AM, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> "linux/kvm.h" is not included for code compiled without
>>>> COMPILING_PER_TARGET, and headers are different depending architecture
>>>> (arm, arm64).
>>>> Thus we need to manually expose some definitions that will
>>>> be used by target/arm, ensuring they are the same for arm amd aarch64.
>>>>
>>>> As well, we must but prudent to not redefine things if code is already
>>>> including linux/kvm.h, thus the #ifndef COMPILING_PER_TARGET guard.
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>    target/arm/kvm_arm.h | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>>>> index c8ddf8beb2e..eedd081064c 100644
>>>> --- a/target/arm/kvm_arm.h
>>>> +++ b/target/arm/kvm_arm.h
>>>> @@ -16,6 +16,21 @@
>>>>    #define KVM_ARM_VGIC_V2   (1 << 0)
>>>>    #define KVM_ARM_VGIC_V3   (1 << 1)
>>>> +#ifndef COMPILING_PER_TARGET
>>>> +
>>>> +/* we copy those definitions from asm-arm and asm-aarch64, as they 
>>>> are the same
>>>> + * for both architectures */
>>>> +#define KVM_ARM_IRQ_CPU_IRQ 0
>>>> +#define KVM_ARM_IRQ_CPU_FIQ 1
>>>> +#define KVM_ARM_IRQ_TYPE_CPU 0
>>>> +typedef unsigned int __u32;
>>>> +struct kvm_vcpu_init {
>>>> +    __u32 target;
>>>> +    __u32 features[7];
>>>> +};
>>>> +
>>>> +#endif /* COMPILING_PER_TARGET */
>>>> +
>>>
>>> I'm not keen on the duplication. It seems to be the only reason we have
>>> struct kvm_vcpu_init is for kvm_arm_create_scratch_host_vcpu() where the
>>> only *external* user passes in a NULL.
>>>
>>
>> I'm not keen about it either, so thanks for pointing it.
>>
>>> If kvm_arm_create_scratch_host_vcpu() is made internal static to
>>> target/arm/kvm.c which will should always include the real linux headers
>>> you just need a QMP helper.
>>>
>>
>> Yes, sounds like the good approach! Thanks.
>>
> 
> Alas this function is used in target/arm/arm-qmp-cmds.c, and if we move 
> the code using it, it pulls QAPI, which is target dependent at this time.
> 
> Since struct kvm_vcpu_init is only used by pointer, I could workaround 
> this by doing a simple forward declaration in kvm_arm.h.

Correct, great!