Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

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

On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>> new file mode 100644
>>>>> index 00000000000..f16e49b8956
>>>>> --- /dev/null
>>>>> +++ b/accel/accel-qmp.c
>>>>> @@ -0,0 +1,47 @@
>>>>> +/*
>>>>> + * QEMU accelerators, QMP commands
>>>>> + *
>>>>> + * Copyright (c) 2021 Red Hat Inc.
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qapi/qapi-commands-machine.h"
>>>>> +
>>>>> +static const Accelerator accel_list[] = {
>>>>> +    ACCELERATOR_QTEST,
>>>>> +#ifdef CONFIG_TCG
>>>>> +    ACCELERATOR_TCG,
>>>>> +#endif
>>>>> +#ifdef CONFIG_KVM
>>>>> +    ACCELERATOR_KVM,
>>>>> +#endif
>>>>
>>>> ...would it be worth compiling the enum to only list enum values that
>>>> were actually compiled in?  That would change it to:
>>>>
>>>> { 'enum': 'Accelerator',
>>>>   'data': [ 'qtest',
>>>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>> ...
>>
>> These accelerator definitions are supposed to be poisoned in generic
>> code... But I like the simplicity of your suggestion, so I'll give it
>> a try and see what happens with removing the poisoned definitions.
> 
> This is actually quite interesting :) Accelerator definitions are
> declared in config-target.h, but acceleration is host specific...

Thomas, I guess I hit Claudio's reported bug again...

1/ generic libqemuutil.a is built without any CONFIG_accel definition.

So this qapi-generated enum ... :

typedef enum Accelerator {
    ACCELERATOR_QTEST,
#if defined(CONFIG_TCG)
    ACCELERATOR_TCG,
#endif /* defined(CONFIG_TCG) */
#if defined(CONFIG_KVM)
    ACCELERATOR_KVM,
#endif /* defined(CONFIG_KVM) */
#if defined(CONFIG_HAX)
    ACCELERATOR_HAX,
#endif /* defined(CONFIG_HAX) */
#if defined(CONFIG_HVF)
    ACCELERATOR_HVF,
#endif /* defined(CONFIG_HVF) */
#if defined(CONFIG_WHPX)
    ACCELERATOR_WHPX,
#endif /* defined(CONFIG_WHPX) */
#if defined(CONFIG_XEN_BACKEND)
    ACCELERATOR_XEN,
#endif /* defined(CONFIG_XEN_BACKEND) */
    ACCELERATOR__MAX,
} Accelerator;

... is expanded to:

typedef enum Accelerator {
    ACCELERATOR_QTEST,
    ACCELERATOR__MAX,
} Accelerator;

2/ softmmu code and qtest do get the definition, the enum
   is different:

typedef enum Accelerator {
    ACCELERATOR_QTEST,
    ACCELERATOR_KVM,
    ACCELERATOR__MAX,
} Accelerator;

qmp_query_accels() fills AcceleratorInfoList with 2 entries

3/ trying to understand what's happening, query-qmp-schema
   returns:

        {
            "name": "206",
            "tag": "name",
            "variants": [
                {
                    "case": "qtest",
                    "type": "0"
                },
                {
                    "case": "kvm",
                    "type": "0"
                }
            ],
            "members": [
                {
                    "name": "name",
                    "type": "403"
                }
            ],
            "meta-type": "object"
        },
        {
            "name": "403",
            "meta-type": "enum",
            "values": [
                "qtest",
                "kvm"
            ]
        },

So accelerators are listed, but with the same enum index?

4/ Running 'query-accels' aborts in qapi_enum_lookup():

The first entry 'qtest' is visited correctly.
When the second entry 'kvm' is visited, this assertion fires:

    assert(val >= 0 && val < lookup->size);

because lookup->size = 1 (remember from 1/ Accelerator_lookup
has been compiled without definitions, in libqemuutil.a).

I'll keep the current patch, as it works, and address the rest
of this series comments.

Thanks,

Phil.