Re: [PATCH v2 1/3] target/arm: Restrict v8M IDAU to TCG

[Claudio Fontana]

On 3/10/21 2:45 PM, Claudio Fontana wrote:
> On 3/10/21 2:42 PM, Philippe Mathieu-Daudé wrote:
>> On 3/10/21 12:46 PM, Claudio Fontana wrote:
>>> On 3/9/21 3:18 PM, Philippe Mathieu-Daudé wrote:
>>>> On 3/9/21 2:41 PM, Claudio Fontana wrote:
>>>>> On 2/21/21 11:26 PM, Philippe Mathieu-Daudé wrote:
>>>>>> IDAU is specific to M-profile. KVM only supports A-profile.
>>>>>> Restrict this interface to TCG, as it is pointless (and
>>>>>> confusing) on a KVM-only build.
>>>>>>
>>>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>
>>>>>
>>>>> This one breaks the KVM tests hard though (most of them).
>>>>>
>>>>> I will try to figure out why.
>>>>>
>>>>> Ciao,
>>>>>
>>>>> Claudio
>>>>>
>>>>>
>>>>>> ---
>>>>>>  target/arm/cpu.c     | 7 -------
>>>>>>  target/arm/cpu_tcg.c | 8 ++++++++
>>>>>>  2 files changed, 8 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>>>> index b8bc89e71fc..a772fd4926f 100644
>>>>>> --- a/target/arm/cpu.c
>>>>>> +++ b/target/arm/cpu.c
>>>>>> @@ -2380,12 +2380,6 @@ static const TypeInfo arm_cpu_type_info = {
>>>>>>      .class_init = arm_cpu_class_init,
>>>>>>  };
>>>>>>  
>>>>>> -static const TypeInfo idau_interface_type_info = {
>>>>>> -    .name = TYPE_IDAU_INTERFACE,
>>>>>> -    .parent = TYPE_INTERFACE,
>>>>
>>>> Hmm this is an interface...
>>>>
>>>> Is a CPU/machine trying to resolve it?
>>>
>>> Well, this fails horribly at any qemu-system-aarch64 startup for the 
>>> kvm-only build:
>>>
>>> in my view we cannot remove the idau interface until we have removed all 
>>> the TCG-only boards fronm the build.
>>
>> Yes, this is a similar bug to the one fixed by commit 8d0bceba24c
>> ("hw/nvram: Always register FW_CFG_DATA_GENERATOR_INTERFACE").
>>
>>>
>>> When calling qemu_init(), and we get into select_machine(),
>>>
>>> the object_class_get_list() tries to initialize all machine types.
>>>
>>> When it does that, it tries to initialize the IDAU interface, and fails.
>>>
>>> #0  0x0000ffffb9e51828 in raise () at /lib64/libc.so.6
>>> #1  0x0000ffffb9e52e4c in abort () at /lib64/libc.so.6
>>> #2  0x0000aaaae042a484 in type_initialize (ti=0xaaaaf0cb37c0) at 
>>> ../qom/object.c:333
>>> #3  0x0000aaaae042c06c in object_class_foreach_tramp (key=0xaaaaf0cb3940, 
>>> value=0xaaaaf0cb37c0, opaque=0xfffff9f2bac8)
>>>     at ../qom/object.c:1069
>>> #4  0x0000ffffbb3d4248 in g_hash_table_foreach () at 
>>> /usr/lib64/libglib-2.0.so.0
>>> #5  0x0000aaaae042c180 in object_class_foreach (fn=
>>>     0xaaaae042c324 <object_class_get_list_tramp>, 
>>> implements_type=0xaaaae089cc90 "machine", include_abstract=false, 
>>> opaque=0xfffff9f2bb10)
>>>     at ../qom/object.c:1091
>>> #6  0x0000aaaae042c3a8 in object_class_get_list 
>>> (implements_type=0xaaaae089cc90 "machine", include_abstract=false) at 
>>> ../qom/object.c:1148
>>> #7  0x0000aaaae03863d8 in select_machine () at ../softmmu/vl.c:1607
>>> #8  0x0000aaaae038ad74 in qemu_init (argc=15, argv=0xfffff9f2be08, 
>>> envp=0xfffff9f2be88) at ../softmmu/vl.c:3489
>>> #9  0x0000aaaadfdcf5a0 in main (argc=15, argv=0xfffff9f2be08, 
>>> envp=0xfffff9f2be88) at ../softmmu/main.c:49
>>>
>>>
>>> (gdb) frame 2
>>> #2  0x0000aaaae042a484 in type_initialize (ti=0xaaaaf0cb37c0) at 
>>> ../qom/object.c:333
>>> 333                     abort();
>>> (gdb) p ti[0]
>>> $1 = {name = 0xaaaaf0cb3940 "mps2tz", class_size = 408, instance_size = 
>>> 202224, instance_align = 0, class_init = 
>>>     0xaaaae0273408 <mps2tz_class_init>, class_base_init = 0x0, class_data = 
>>> 0x0, instance_init = 0x0, instance_post_init = 0x0, 
>>>   instance_finalize = 0x0, abstract = true, parent = 0xaaaaf0cb3960 
>>> "machine", parent_type = 0xaaaaf0cad860, class = 0xaaaaf0d0d830, 
>>>   num_interfaces = 1, interfaces = {{typename = 0xaaaaf0cb3980 
>>> "idau-interface"}, {typename = 0x0} <repeats 31 times>}}
>>>
>>>
>>> In my view we should revert this until all incompatible boards are disabled
>>
>> My view is this is a QOM design problem. Others might hit the
>> same issue. It is hard to debug. It should be fixed upfront.

What is the QOM design problem to fix exactly?

And in any case, I think this small change "target/arm: Restrict v8M IDAU to 
TCG",
when applied on its own, does not get us any closer to the goal, it actually 
hinders us, as we do not have a working buildable and testable kvm-only build 
to base on.

That is why I added a revert of this to my series.

My suggestion is just to postpone your change to later on,
when we have the other pieces in place (ie after we can disable incompabile 
boards).

A working kvm-only build is a good starting point I think.

After we are able to disable incompatible boards,
we can reapply "target/arm: Restrict v8M IDAU to TCG",
and we can also remove a lot of additional stubs and V7M-only code and such 
from the KVM-only build.

But I'd rather have a functional, make check-able starting point..

Ciao,

CLaudio


>>
>>> In this case, the one failing is MPS2, so the offender is
>>>
>>> devices/arm-softmmu.mak:CONFIG_MPS2=y
>>>
>>> from the point of view of the kvm-only build.
>>>
>>> What I'd suggest is (but I am open to alternatives):
>>>
>>> * revert this one
>>> * complete my arm cleanup series, with now all tests passing
>>> * disable the non-KVM boards for KVM-only builds (basically your series)
>>> * apply the accelerator classes specializations to ARM
>>
>> MPS2TZ uses a Cortex-M33 which is requires TCG.
>> The machine shouldn't be present if TCG is not built-in.
>>
>> Previous attempt which you acked :)
>> "target/arm: Restrict ARMv7 M-profile cpus to TCG accel"
>> https://www.mail-archive.com/qemu-block@nongnu.org/msg79943.html
>>
> 
> Yes, I would absolutely like to proceed with all of this,
> 
> and have only the right configuration for KVM be built,
> 
> but I am a bit lost with all these different series.
> 
> Ciao,
> 
> CLaudio
>