Re: [PATCH 03/12] tests/plugin: add test plugin for inline operations

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 1/12/24 21:20, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 1/11/24 19:57, Philippe Mathieu-Daudé wrote:
>>> Hi Pierrick,
>>> On 11/1/24 15:23, Pierrick Bouvier wrote:
>>>> For now, it simply performs instruction, bb and mem count, and ensure
>>>> that inline vs callback versions have the same result. Later, we'll
>>>> extend it when new inline operations are added.
>>>>
>>>> Use existing plugins to test everything works is a bit cumbersome, as
>>>> different events are treated in different plugins. Thus, this new one.
>>>>
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> ---
>>>>     tests/plugin/inline.c    | 183 +++++++++++++++++++++++++++++++++++++++
>>>>     tests/plugin/meson.build |   2 +-
>>>>     2 files changed, 184 insertions(+), 1 deletion(-)
>>>>     create mode 100644 tests/plugin/inline.c
>>>
>>>> +#define MAX_CPUS 8
>>> Where does this value come from?
>>>
>>
>> The plugin tests/plugin/insn.c had this constant, so I picked it up
>> from here.
>>
>>> Should the pluggin API provide a helper to ask TCG how many
>>> vCPUs are created?
>>
>> In user mode, we can't know how many simultaneous threads (and thus
>> vcpu) will be triggered by advance. I'm not sure if additional cpus
>> can be added in system mode.
>>
>> One problem though, is that when you register an inline op with a
>> dynamic array, when you resize it (when detecting a new vcpu), you
>> can't change it afterwards. So, you need a storage statically sized
>> somewhere.
>>
>> Your question is good, and maybe we should define a MAX constant that
>> plugins should rely on, instead of a random amount.
> 
> For user-mode it can be infinite. The existing plugins do this by
> ensuring vcpu_index % max_vcpu. Perhaps we just ensure that for the
> scoreboard as well? Of course that does introduce a trap for those using
> user-mode...
> 

The problem with vcpu-index % max_vcpu is that it reintroduces race 
condition, though it's probably less frequent than on a single variable. 
IMHO, yes it solves memory error, but does not solve the initial problem 
itself.

The simplest solution would be to have a size "big enough" for most 
cases, and abort when it's reached.

Another solution, much more complicated, but correct, would be to move 
memory management of plugin scoreboard to plugin runtime, and add a 
level of indirection to access it. Every time a new vcpu is added, we 
can grow dynamically. This way, the array can grow, and ultimately, 
plugin can poke its content/size. I'm not sure this complexity is what 
we want though.