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

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

Powered by blists - more mailing lists