Re: [RFC PATCH 00/19] accel: Introduce AccelvCPUState opaque structure

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/4/21 4:40 PM, Paolo Bonzini wrote:
> On 04/03/21 15:54, Philippe Mathieu-Daudé wrote:
>> On 3/4/21 2:56 PM, Paolo Bonzini wrote:
>>> On 03/03/21 19:22, Philippe Mathieu-Daudé wrote:
>>>> Series is organized as:
>>>> - preliminary trivial cleanups
>>>> - introduce AccelvCPUState
>>>> - move WHPX fields (build-tested)
>>>> - move HAX fields (not tested)
>>>> - move KVM fields (build-tested)
>>>> - move HVF fields (not tested)
>>>
>>> This approach prevents adding a TCG state.  Have you thought of using a
>>> union instead, or even a void pointer?
>>
>> Why does it prevent it? We can only have one accelerator per vCPU.
> 
> You're right, my misguided assumption was that there can only be one of
> WHPX/HAX/KVM/HVF.  This is true for WHPX/KVM/HVF but HAX can live with
> any of the others.

I suppose you aren't talking about build-time but runtime. There should
be no distinction related to accelerator at runtime. We should be able
to have multiple accelerators at runtime, and eventually be able to
migrate vCPU from one accelerator to another, if it is proven useful.

How accelerators are orchestrated is obviously out of the scope of this
series.

> However this means that AccelvCPUState would have multiple definitions.

Yes.

>  Did you check that gdb copes well with it?

No, I haven't, because we already use opaque pointers elsewhere.

> It's also forbidden by
> C++[1], so another thing to check would be LTO when using the C++
> compiler for linking.

OK, I have no clue about C++ (and tries to keep QEMU way from it)
or about LTO. So I'd need to investigate that.

> 
> Paolo
> 
> [1] https://en.wikipedia.org/wiki/One_Definition_Rule
> 
>> TCG state has to be declared as another AccelvCPUState implementation.
>>
>> Am I missing something?
>>
>> Preventing building different accelerator-specific code in the same
>> unit file is on purpose.
>>
>> Regards,
>>
>> Phil.
>>
>