[PATCH v2 0/2] small fixes for arm_ffa driver

[PATCH v2 0/2] small fixes for arm_ffa driver

[Yeoreum Yun]

This patchset small fixes for arm_ffa driver:
    - change ffa_device_register()'s declaration and its return value.
    - fix direct message request version 2 uuid setup.


v1 -> v2:
    - Restore error return in ffa_device_register().
    - change type __be64 to unsigned long to suppress warning.

Levi Yun (2):
  firmware/arm_ffa: change ffa_device_register()'s parameters and return
  firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg
    v2

 drivers/firmware/arm_ffa/bus.c    | 15 +++++++++++----
 drivers/firmware/arm_ffa/driver.c | 13 ++++---------
 include/linux/arm_ffa.h           | 12 ++++++++----
 3 files changed, 23 insertions(+), 17 deletions(-)

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

[PATCH v2 1/2] firmware/arm_ffa: change ffa_device_register()'s parameters and return

[Yeoreum Yun]

From: Levi Yun <yeoreum.yun@arm.com>

Currently, ffa_dev->properties is set after ffa_device_register() in
ffa_setup_partitions().
This means it couldn't validate partition's properties
while probing ffa_device.

Change parameter of ffa_device_register() to receive ffa_partition_info
so that before device_register(), ffa_device->properties can be setup
and any other data.

Also, change return value of ffa_device_register() from NULL to ERR_PTR
so that it passes error code.

Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 drivers/firmware/arm_ffa/bus.c    | 15 +++++++++++----
 drivers/firmware/arm_ffa/driver.c |  7 +------
 include/linux/arm_ffa.h           | 12 ++++++++----
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
index eb17d03b66fe..a4ab6d14afef 100644
--- a/drivers/firmware/arm_ffa/bus.c
+++ b/drivers/firmware/arm_ffa/bus.c
@@ -187,13 +187,18 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev)
 	return valid;
 }
 
-struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
-				       const struct ffa_ops *ops)
+struct ffa_device *ffa_device_register(
+		const struct ffa_partition_info *part_info,
+		const struct ffa_ops *ops)
 {
 	int id, ret;
+	uuid_t uuid;
 	struct device *dev;
 	struct ffa_device *ffa_dev;
 
+	if (!part_info)
+		return NULL;
+
 	id = ida_alloc_min(&ffa_bus_id, 1, GFP_KERNEL);
 	if (id < 0)
 		return NULL;
@@ -210,9 +215,11 @@ struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
 	dev_set_name(&ffa_dev->dev, "arm-ffa-%d", id);
 
 	ffa_dev->id = id;
-	ffa_dev->vm_id = vm_id;
+	ffa_dev->vm_id = part_info->id;
+	ffa_dev->properties = part_info->properties;
 	ffa_dev->ops = ops;
-	uuid_copy(&ffa_dev->uuid, uuid);
+	import_uuid(&uuid, (u8 *)part_info->uuid);
+	uuid_copy(&ffa_dev->uuid, &uuid);
 
 	ret = device_register(&ffa_dev->dev);
 	if (ret) {
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index b14cbdae94e8..2c2ec3c35f15 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1387,7 +1387,6 @@ static struct notifier_block ffa_bus_nb = {
 static int ffa_setup_partitions(void)
 {
 	int count, idx, ret;
-	uuid_t uuid;
 	struct ffa_device *ffa_dev;
 	struct ffa_dev_part_info *info;
 	struct ffa_partition_info *pbuf, *tpbuf;
@@ -1406,23 +1405,19 @@ static int ffa_setup_partitions(void)
 
 	xa_init(&drv_info->partition_info);
 	for (idx = 0, tpbuf = pbuf; idx < count; idx++, tpbuf++) {
-		import_uuid(&uuid, (u8 *)tpbuf->uuid);
-
 		/* Note that if the UUID will be uuid_null, that will require
 		 * ffa_bus_notifier() to find the UUID of this partition id
 		 * with help of ffa_device_match_uuid(). FF-A v1.1 and above
 		 * provides UUID here for each partition as part of the
 		 * discovery API and the same is passed.
 		 */
-		ffa_dev = ffa_device_register(&uuid, tpbuf->id, &ffa_drv_ops);
+		ffa_dev = ffa_device_register(tpbuf, &ffa_drv_ops);
 		if (!ffa_dev) {
 			pr_err("%s: failed to register partition ID 0x%x\n",
 			       __func__, tpbuf->id);
 			continue;
 		}
 
-		ffa_dev->properties = tpbuf->properties;
-
 		if (drv_info->version > FFA_VERSION_1_0 &&
 		    !(tpbuf->properties & FFA_PARTITION_AARCH64_EXEC))
 			ffa_mode_32bit_set(ffa_dev);
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index a28e2a6a13d0..3fb9c7a3453b 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -166,9 +166,12 @@ static inline void *ffa_dev_get_drvdata(struct ffa_device *fdev)
 	return dev_get_drvdata(&fdev->dev);
 }
 
+struct ffa_partition_info;
+
 #if IS_REACHABLE(CONFIG_ARM_FFA_TRANSPORT)
-struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
-				       const struct ffa_ops *ops);
+struct ffa_device *ffa_device_register(
+		const struct ffa_partition_info *part_info,
+		const struct ffa_ops *ops);
 void ffa_device_unregister(struct ffa_device *ffa_dev);
 int ffa_driver_register(struct ffa_driver *driver, struct module *owner,
 			const char *mod_name);
@@ -177,8 +180,9 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev);
 
 #else
 static inline
-struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
-				       const struct ffa_ops *ops)
+struct ffa_device *ffa_device_register(
+		const struct ffa_partition_info *part_info,
+		const struct ffa_ops *ops)
 {
 	return NULL;
 }
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

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

[Yeoreum Yun]

From: Levi Yun <yeoreum.yun@arm.com>

UUID is saved in big endian format.
i.e) For uuid "378daedc-f06b-4446-8314-40ab933c87a3",

It should be saved in memory like:
    37 8d ae dc
    f0 6b 44 46
    83 14 40 ab
    93 3c 87 a3

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.

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.

Remove le64_to_cpu() for uuid in direct message request version 2,
and change uuid_regs' type to unsigned long.

Link: https://developer.arm.com/documentation/den0077/latest [0]
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 2c2ec3c35f15..8bfeca9d0d2a 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -483,13 +483,13 @@ static int ffa_msg_send_direct_req2(u16 src_id, u16 dst_id, const uuid_t *uuid,
 	u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
 	union {
 		uuid_t uuid;
-		__le64 regs[2];
+		unsigned long regs[2];
 	} uuid_regs = { .uuid = *uuid };
 	ffa_value_t ret, args = {
 		.a0 = FFA_MSG_SEND_DIRECT_REQ2,
 		.a1 = src_dst_ids,
-		.a2 = le64_to_cpu(uuid_regs.regs[0]),
-		.a3 = le64_to_cpu(uuid_regs.regs[1]),
+		.a2 = uuid_regs.regs[0],
+		.a3 = uuid_regs.regs[1],
 	};
 	memcpy((void *)&args + offsetof(ffa_value_t, a4), data, sizeof(*data));

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH v2 0/2] small fixes for arm_ffa driver

[Sudeep Holla]

On Tue, 03 Dec 2024 14:31:07 +0000, Yeoreum Yun wrote:
> This patchset small fixes for arm_ffa driver:
>     - change ffa_device_register()'s declaration and its return value.
>     - fix direct message request version 2 uuid setup.
>
>
> v1 -> v2:
>     - Restore error return in ffa_device_register().
>     - change type __be64 to unsigned long to suppress warning.
>
> [...]

Applied with some reformating/reworking of the commit messages including
$subject change to sudeep.holla/linux (for-next/ffa/fixes), thanks!

[1/2] firmware/arm_ffa: change ffa_device_register()'s parameters and return
      https://git.kernel.org/sudeep.holla/c/6fe437cfe2cd
[2/2] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2
      https://git.kernel.org/sudeep.holla/c/81c8b1c2ef7a
--
Regards,
Sudeep

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

[Arnd Bergmann]

On Tue, Dec 3, 2024, at 15:31, Yeoreum Yun wrote:
> From: Levi Yun <yeoreum.yun@arm.com>

I just saw this commit in the pull request, and I'm very
confused because the description does not match the
patch contents.

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

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.

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

     Arnd

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@arm.com>
> 
> 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

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

[Arnd Bergmann]

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

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

[Yeoreum Yun]

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.

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

[Arnd Bergmann]

On Tue, Dec 10, 2024, at 08:36, Yeoreum Yun wrote:
>> On Mon, Dec 9, 2024, at 17:59, Sudeep Holla wrote:
>> > On Mon, Dec 09, 2024 at 04:27:14PM +0100, Arnd Bergmann wrote:
>>
>> 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]
>            ...

I meant they had the choice and chose to specify little-endian
64-bit word to encode the sequence of bytes of the standard
in-memory representation of UUIDs. 

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

If the uuid is transferred in memory, you obviously don't want to
swap it. If they pass it in registers across different endianess
code without specifying the byteorder in the caller, then they
would have the same bug.

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

Sorry, but you are not making sense here.

Sudeep, should I just cherry-pick your other fix from the pull
request and ignore this patch?

      Arnd

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 09:04:30PM +0100, Arnd Bergmann wrote:
> 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.
>

Thanks, I will add such a note when I get BE support fixed so that it is
clear.

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

Indeed. I do run SCMI once in a while but hadn't tried FF-A so far for no
particular reasons. I will get that sorted this 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.
>

OK you are right, I clearly got confused. There should be something
else messed up in the setup. I think fixing BE support will avoid such
things in the future, I will get on that ASAP. Sorry for the confusion.

I just dumped the buffers and UUID and it works as expected, so I blindly
assumed the firmware setup is correct and there is a bug.

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

Yes these are 2 main changes I have. I think I had one more but I need to
go back and check. I plan to post them once I have done the testing with
OPTEE. I just want to run xtest they have and see if everything works for
which I may need to spend sometime.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On Tue, Dec 10, 2024 at 09:45:51AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 10, 2024, at 08:36, Yeoreum Yun wrote:
> >> On Mon, Dec 9, 2024, at 17:59, Sudeep Holla wrote:
> >> > On Mon, Dec 09, 2024 at 04:27:14PM +0100, Arnd Bergmann wrote:
> >>
> >> 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]
> >            ...
> 
> I meant they had the choice and chose to specify little-endian
> 64-bit word to encode the sequence of bytes of the standard
> in-memory representation of UUIDs. 
> 
> > 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.
> 
> If the uuid is transferred in memory, you obviously don't want to
> swap it. If they pass it in registers across different endianess
> code without specifying the byteorder in the caller, then they
> would have the same bug.
> 
> > 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).
> 
> Sorry, but you are not making sense here.
>

Agreed. The patch is clearly wrong. Thanks for refreshing my knowledge and
helping me to clear my confusion.

> Sudeep, should I just cherry-pick your other fix from the pull
> request and ignore this patch?
>

I will remove the wrong patch and retag and send PR if that helps. It should
be quick.

-- 
Regards,
Sudeep