Re: [PATCH v2 2/2] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2

["Arnd Bergmann" <arnd@arndb.de>]

On Mon, Dec 9, 2024, at 17:59, Sudeep Holla wrote:
> On Mon, Dec 09, 2024 at 04:27:14PM +0100, Arnd Bergmann wrote:
>
>> > That means, we don't need to swap the uuid when it send via direct
>> > message request version 2, just send it as saved in memory.
>>
>> "As saved in memory" does not sound like a useful description
>> when passing arguments through registers, as the register
>> contents are not defined in terms of byte offsets.
>>
>
> Well I didn't know how to term it. The structure UUID is a raw buffer
> and it provide helpers to import/export the data in/out of it. So in LE
> kernel IIUC, it is stored in LE format itself which was my initial
> confusion and hence though what you fixed was correct previously.

The way I would phrase it, the UUID is never "stored" in
big-endian or little-endian format, it's just remains a string
of bytes. The endianess becomes a choice only when loading it
into registers for passing the argument to firmware, and it's
the firmware that mandates little-endian in the specification.

>> Can you describe what bug you found? If the byteorder on
>> big-endian kernels is wrong in the current version and your
>> patch fixes it, it sounds like the specification needs to
>> be updated describe both big-endian and little-endian
>> byte-order, and how the firmware detects which one is used.
>>
>
> The firmware interface understands only LE format. And by default UUID
> is stored in LE format itself in the structure which I got confused
> initially. We may need endian conversion at places(found few when trying
> to get it working with BE kernel).
>
> I wanted to check with you about this. The current driver doesn't
> work with BE. I tried to cook up patches but then the upstream user
> of this driver OPTEE doesn't work in BE, so I hit a roadblock to fully
> validate my changes. I don't see any driver adding endianness dependency
> in the Kconfig if they can't work with BE, not sure if that
> is intentional or just don't care. I was thinking if we can disable
> it to build in BE kernel until the actual support was added.

I think as long big-endian kernels remain an option on arm64, we
should try to to write portable code and implement the specification
The reality of course is that very few people care these days, and
it's getting harder to test over time.

> So the current FF-A driver just supports LE and the bug was found just
> in LE kernel itself.

What is the bug and how was it found? The only thing I see in
the patch here is to change the code from portable to nonportable,
but not actually change behavior on little-endian 64-bit.

Looking through the other functions in drivers/firmware/arm_ffa/driver.c,
I see that most of them just match the specification. One exception
is ffa_notification_info_get(), which incorrectly casts the
argument response arguments to an array of 'u16' values. Using
the correct bit shifts according to the specification would
make that work on big-endian and also more readable and
robust. Another one is __ffa_partition_info_get_regs(), which
does an incorrect memcpy() instead of decoding the values.


     Arnd