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

[Sudeep Holla]

On Mon, Dec 09, 2024 at 04:27:14PM +0100, Arnd Bergmann wrote:
> On Tue, Dec 3, 2024, at 15:31, Yeoreum Yun wrote:
> > From: Levi Yun <yeoreum.yun@xxxxxxx>
> 
> I just saw this commit in the pull request, and I'm very
> confused because the description does not match the
> patch contents.
>

Sorry for that, I tried to reword to improve it but it is obvious now that I
didn't do a good job there.

> > Accoding to FF-A specification[0] 15.4 FFA_MSG_SEND_DRIECT_REQ2,
> > then UUID is saved in register:
> >     UUID Lo  x2  Bytes[0...7] of UUID with byte 0 in the low-order bits.
> >     UUID Hi  x3  Bytes[8...15] of UUID with byte 8 in the low-order bits.
>
> The specification you cite here clearly describes little-endian
> format, i.e. the low-order byte corresponds to the first
> memory address.
>


> > 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.

> 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.

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

> > Remove le64_to_cpu() for uuid in direct message request version 2,
> > and change uuid_regs' type to unsigned long.
>
> 'unsigned long' makes the code unnecessarily incompatible
> with 32-bit builds.
>

Understood we may need some typecasting to avoid compiler warnings.

Just a note not related to your comment though: FFA_MSG_SEND_DIRECT_REQ2
is 64-bit only as it uses full 64-bit register to pass UUID.

--
Regards,
Sudeep