| Message ID | 20241203143109.1030514-3-yeoreum.yun@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | small fixes for arm_ffa driver | expand |
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
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
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
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.
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
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.
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.
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));