Message-ID: <Z1fvkpYvqpnjPZA8@e129823.arm.com>
Date: Tue, 10 Dec 2024 07:36:50 +0000
From: Yeoreum Yun <yeoreum.yun@....com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Sudeep Holla <sudeep.holla@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	nd@....com
Subject: Re: [PATCH v2 2/2] firmware/arm_ffa: remove __le64_to_cpu() when set
 uuid for direct msg v2

Hi Arnd,

> 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.
>
Conclusionly, Yes. But the RFC 4122 said with network byte order.
to describe how uuid is saved.

but I think the endianess to load the register is not a choice.
because the spec says:

    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.

this means UUID.bytes[0] should be loaded to x2.bytes[0].
           UUID.bytes[1] should be loaded to x2,bytes[1]
           ...

That's why other software spec (i.e tf-a) doesn't loads UUID from register
wihtout swapping byte with endianess but just copy it.

The bug is "not send UUID according to spec" in kernel side
That's why it fails when I send message  with direct message version 2.
So, it''s not change code unportable to portable but it fixes according
to spec (load UUID as it is in register wihtout endianess).

> > > 'unsigned long' makes the code unnecessarily incompatible
> > > with 32-bit builds.

I don't think it should care about 32-bit for direct message 2,
Since direct message v2 is  64-bit ABI only.
that means ffa_msg_send_direct_req2() should return error before it calls smc.

Thanks.

Powered by blists - more mailing lists