On 04/03/2026 20:38, Pierrick Bouvier wrote:
> On 3/4/26 9:56 AM, Pierrick Bouvier wrote:
>> On 3/4/26 8:18 AM, Florian Hofhammer wrote:
>>>>>>> +static bool vcpu_syscall_filter(qemu_plugin_id_t id, unsigned int
>>>>>>> vcpu_index,
>>>>>>> + int64_t num, uint64_t a1, uint64_t a2,
>>>>>>> + uint64_t a3, uint64_t a4, uint64_t a5,
>>>>>>> + uint64_t a6, uint64_t a7, uint64_t a8,
>>>>>>> + uint64_t *sysret)
>>>>>>> +{
>>>>>>> + if (num == 4095) {
>>>>>>> + qemu_plugin_outs("Communication syscall detected, set
>>>>>>> target_pc / "
>>>>>>> + "target_vaddr\n");
>>>>>>> + source_pc = a1;
>>>>>>> + target_pc = a2;
>>>>>>> + target_vaddr = a3;
>>>>>>> + if (source_pc >> 63 || target_pc >> 63 || target_vaddr >> 63) {
>>>>>>> + /*
>>>>>>> + * Some architectures (e.g., m68k) use 32-bit addresses
>>>>>>> with the
>>>>>>> + * top bit set, which causes them to get sign-extended
>>>>>>> somewhere in
>>>>>>> + * the chain to this callback. We mask the top bits off
>>>>>>> here to get
>>>>>>> + * the actual addresses.
>>>>>>> + */
>>>>>>
>>>>>> This looks like an actual bug. Using a backtrace here, can you find
>>>>>> which function extended it?
>>>>>> Parameter should be treated as unsigned everywhere, else something is
>>>>>> really going wrong.
>>>>>
>>>>> tl;dr: register values are considered signed in the syscall handling
>>>>> code, which seems incorrect to me. Details below.
>>>>>
>>>>> This seems to be a bigger issue in the syscall handling code and I'm
>>>>> surprised this never surfaced before. Generally, the CPUArchState struct
>>>>> in target/*/cpu.h defines the registers as unsigned types. When a
>>>>> syscall is encountered, the main cpu loop calls do_syscall() from
>>>>> linux-user/syscall.c, which takes and returns abi_long values. abi_long
>>>>> is defined as either int32_t or target_long in include/user/abitypes.h
>>>>> and therefore a signed type, so the register values get converted from
>>>>> unsigned to signed types here. do_syscall() passes those abi_long values
>>>>> on, e.g., to record_syscall_start() and send_through_syscall_filters(),
>>>>> which still take the register values in as abi_long. Those in turn
>>>>> however pass the values on to qemu_plugin_vcpu_syscall() and
>>>>> qemu_plugin_vcpu_syscall_filter(), respectively. Those two functions now
>>>>> take uint64_t values for the arguments, so for 32-bit architectures the
>>>>> conversion chain is uint32_t (CPUArchState) -> int32_t (do_syscall) ->
>>>>> uint64_t (callbacks).
>>>>>
>>>>> This seems to be some bigger refactoring changing all those abi_longs to
>>>>> abi_ulongs. Would you like me to prepare a separate patch series for
>>>>> this?
>>>>
>>
>> Indeed, they are signed in linux-user, so maybe my original assumption
>> that it *should* be unsigned is wrong. Let me take a look to see what
>> should be the correct semantic here.
>>
>>>> As a follow-up on this, changing this seems to be a bit more complicated
>>>> than I initially thought. The arguments could be simply made unsigned,
>>>> but the return value (which is also just a register content and should
>>>> therefore be unsigned if I understand correctly) can't easily be made
>>>> unsigned. The per-target main loop relies on the return value of
>>>> do_syscall() to be signed to determine whether one of the QEMU-specific
>>>> errors (e.g., QEMU_ERESTARTSYS) was returned.
>>>
>>> Note: some architectures already define the return value as unsigned in
>>> the main cpu loop and implicitly cast the return value of do_syscall().
>>> Given that in theory, a syscall's return value could always coincide
>>> with the values of QEMU's special errors, I'm wondering whether it would
>>> make sense to make do_syscall()'s return value unsigned and signal those
>>> special error codes out-of-band instead of in the syscall return value,
>>> e.g., either via a separate pointer argument to do_syscall() or by
>>> making do_syscall() return a struct containing both the return value and
>>> a potential error code.
>>>
>>
>> It could be a solution. I'm just a little bit relunctant to make such a
>> change now because we have limited time (soft freeze is coming next
>> week), so not the right time to make such deep changes and... it has
>> been working like that for long time.
>>
>>> But maybe I'm getting ahead of myself here, please let me know what you
>>> think!
>>>
>> Thanks for your very good analysis. I'll come back once I have more
>> information about what should be the correct fix.
>>
>> Meanwhile, and because we are close from softfreeze, just keep the
>> current hack in the test. This way, we can focus on sending your series
>> and the additional comment patch you sent.
>>
>> Thanks,
>> Pierrick
>
> The attached patch fixes the problem by clamping syscall arguments for 32-bit
> targets. Return type has to be signed anyway, so no change is needed on this
> side.
>
> You can add it as the first patch on your next version, and remove the hack
> checking the highest bit in this test, now that the problem is solved.
>
> Regards,
> Pierrick
Ack, thanks for the patch!
Best regards,
Florian
