AMD mailbox driver support #7270

Akshay-Belsare · 2025-02-10T07:25:26Z

Add support for the AMD Mailbox Service for use on AMD platforms.
The Mailbox Service requires the Local IPI ID as a mandatory input,
provided via CFG_MAILBOX_LOCAL_ID.
Mailbox initialization for CFG_MAILBOX_LOCAL_ID occurs as a late init
service and will be utilized by various drivers to communicate with other
firmware.
The remote ID will be supplied by different drivers using the mailbox
service.

jenswi-linaro

Please fix the checkpatch issues.
This code is unused, do you plan to upstream code that uses the mailbox driver any time soon?

core/drivers/amd/mailbox_driver.c

core/include/drivers/amd/mailbox_driver.h

core/drivers/amd/mailbox_driver.c

Add support for the AMD Mailbox Service for use on AMD platforms. The Mailbox Service requires the Local IPI ID as a mandatory input, provided via CFG_MAILBOX_LOCAL_ID. Mailbox initialization for CFG_MAILBOX_LOCAL_ID occurs as a late init service and will be utilized by various drivers to communicate with other firmware. The remote ID will be supplied by different drivers using the mailbox service. Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

core/drivers/amd/mailbox_driver.c

etienne-lms · 2025-02-11T16:09:27Z

core/drivers/amd/mailbox_driver.c

+{
+	TEE_Result res = TEE_ERROR_GENERIC;
+
+	cache_operation(TEE_CACHEINVALIDATE, ipi_info.rsp, payload_size);


By the way, cache_operation() may return an error code. If never awaited, maybe asserting the return value is enough. That said, I see that almost all callers of that function do not check the return value :(
There are all in core/drivers/.... Note that if ipi_info.rsp is NULL, the function would have returned an error.

Let me revisit and modify accordingly.

core/drivers/amd/mailbox_driver.c

ldts · 2025-02-13T09:01:50Z

um, isnt this driver just the same than versal_mbox.c currently upstream? it seems that the only difference is the base address 0xEB3F0000 instead of 0xFF3F0000
it has less features - no tracing support, no validation of multiple buffers - but other than that I fail to see the difference.

could you comment please?

ldts · 2025-02-13T09:12:22Z

Also, if this is an init_late it means you wont be able to use some of the chip cryptographic features like the PUF or SHA (or fuses for that matter) during boot. Not sure if it applies to your platform but just a heads up.
This is the reason why on versal we chose to open the mailbox early.

ldts · 2025-02-13T10:02:10Z

Also notice that given the nature of the firmware receiving the requests, mbox requests/responses usually consist of several buffers and the payload and each buffer had alignment requirements. as @jenswi-linaro mentioned, if would be nice to have an example of usage.

Akshay-Belsare · 2025-02-13T11:22:50Z

@ldts This driver is meant for a newer AMD Versal Gen 2 family and later versions. This change is having the base infra, more will be added to it later. The use case is different from the the versal platform.

ldts · 2025-02-13T11:43:15Z

@ldts This driver is meant for a newer AMD Versal Gen 2 family and later versions. This change is having the base infra, more will be added to it later. The use case is different from the the versal platform.

What do you mean by "more will be added to it later"? Are you referring to additional changes to this mailbox driver under review? This driver is a simplified version of versal_mbox.c, and my concern is that you may eventually need to make it more complex, resulting in something similar to what already exists.

Add AMD mailbox driver support for AMD Versal Gen 2 platform. Keeping the config CFG_MAILBOX_DRIVER disabled by default. Once a driver using the mailbox service is added and enabled for this platform the configuration to be updated. Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

Address upstream review comments Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

ldts · 2025-02-13T12:02:43Z

one other piece of information, @jcorbier is doing some extensions to the versal_mbox driver (buffer memory management, mailbox timeouts etc 5418d26 ). So maybe that is something that you could also use (ie, timeouts)

Akshay-Belsare · 2025-02-13T12:13:26Z

@ldts

What do you mean by "more will be added to it later"? Are you referring to additional changes to this mailbox driver under review? This driver is a simplified version of versal_mbox.c, and my concern is that you may eventually need to make it more complex, resulting in something similar to what already exists.

As I mentioned, AMD Versal Gen 2 is new family and the underlying workings are not exactly same as Versal and Versal NET.

one other piece of information, @jcorbier is doing some extensions to the versal_mbox driver (buffer memory management, mailbox timeouts etc 5418d26 ). So maybe that is something that you could also use (ie, timeouts)

Appreciate, will have a look in to those changes as well.

ldts · 2025-02-13T12:17:49Z

thanks. but please, do take the time to ask my questions to avoid going in circles. This driver is a simplified version of what we currently have.

Address error condition cases as per review comments. Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

Akshay-Belsare · 2025-02-13T12:27:14Z

@etienne-lms @jenswi-linaro
Added new patch addressing the review comments.

ldts · 2025-02-13T18:02:07Z

um you made me curious about the IPI Gen2 now, where can I find the PLM code? should it still be in the embeddedsw repo or is it now hosted somwhere else?

core/drivers/amd/mailbox_driver.c

nathan-menhorn · 2025-02-14T16:41:50Z

it has less features - no tracing support, no validation of multiple buffers - but other than that I fail to see the difference.

could you comment please?

Hi @ldts all PLM code is in the embedded software directory. This will be using the versal_aiegen2 specific code along with the common code.

-s/id/ID -return values - brace removal - explicit header addition Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

Akshay-Belsare · 2025-02-14T17:47:19Z

@etienne-lms Addressed comments from you.

ldts · 2025-02-14T17:54:03Z

it has less features - no tracing support, no validation of multiple buffers - but other than that I fail to see the difference.
could you comment please?

Hi @ldts all PLM code is in the embedded software directory. This will be using the versal_aiegen2 specific code along with the common code.

thanks Nathan. Um so I fail to see how this is architecturally (software wise) different than Versal or Versal net: they share the same TF-A mailbox service and similar PLM services (multiple buffers on the payload). It seems to me it will ultimately need the versal_mbox features but will see.

etienne-lms

LGTM with few comments.
Could you squash the fixup commits?

etienne-lms · 2025-02-26T13:35:48Z

core/drivers/amd/mailbox_driver.c

-		/* Reset struct ipi_info.
-		 * Not checking return, already in error case
+		EMSG("Failed to open Mailbox for remote ID %"PRIu32, remote_id);
+		/* Reset IPI information structure.


- /* Reset IPI information structure. + /* + * Reset IPI information structure. * Not checking return value res, already in error case */

Could you address these coding convention issues?

etienne-lms · 2025-02-26T13:37:03Z

core/drivers/amd/mailbox_driver.c

-		/* Remove mapping for request buffer */
+		/* Remove mapping for request buffer.
+		 * Not checking return value res, since already in error case.
+		 */


- /* Remove mapping for request buffer. + /* + * Remove mapping for request buffer. * Not checking return value res, since already in error case. */

ldts · 2025-02-27T11:54:28Z

@etienne-lms could you comment my concerns before providing any form of ack?

etienne-lms

Few remaining comments.
Fell free to squash current and coming fixup commit in your next update.

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"plat-versal2: enable AMD mailbox driver".

etienne-lms · 2025-02-27T16:47:01Z

core/drivers/amd/mailbox_driver.c

+				      payload_size);
+	if (res != TEE_SUCCESS) {
+		EMSG("Failed to remove request mapping for remote ID %"PRIu32,
+		     remote_id);


goto out; for consistency?

etienne-lms · 2025-02-28T07:13:36Z

core/drivers/amd/mailbox_driver.c

-		/* Reset struct ipi_info.
-		 * Not checking return, already in error case
+		EMSG("Failed to open Mailbox for remote ID %"PRIu32, remote_id);
+		/* Reset IPI information structure.


Could you address these coding convention issues?

jenswi-linaro · 2025-02-28T07:29:51Z

core/include/drivers/amd/mailbox_driver.h

+#define IPI_NON_BLOCK	U(0x0)
+#define IPI_BLOCK	U(0x1)
+
+TEE_Result mailbox_open(uint32_t remote_id, size_t payload_size);


Please add an amd_ prefix for these functions.

jenswi-linaro · 2025-02-28T07:40:16Z

@Akshay-Belsare, please address @ldts's concerns. He has been a long-term contributor to this area. OP-TEE is a community-driven project, so the community's view matters.

Akshay-Belsare · 2025-03-06T09:35:40Z

@ldts
We sincerely appreciate your concern. However, the current implementation of the Versal Mailbox driver is highly specific to the Versal family and, in its present form, cannot be seamlessly extended to support the next generation of SoCs.
For example, the Versal Mailbox driver is designed to facilitate communication with a single component, whereas next SoC families will require a more scalable solution that supports multiple components.
Our approach focuses on developing drivers that can cater to a broader range of SoCs. Once these generic drivers are in place, the Versal family can gradually transition to them over time.
Additionally, we want to ensure that our approach preserves the existing functionality of the Versal family, allowing continued development without disruption to its current capabilities.

ldts · 2025-03-07T09:18:11Z

You are simply copying current drivers—mbox, GPIO and others will follow—while stripping functionality and removing the original copyright. As I mentioned, over time, you will inevitably need to restore some of the features you're removing (even if you haven’t realized it yet).

That said, as I requested regarding the GPIO driver you’re proposing, please retain the original copyrights from the code you’re copying. It’s a requirement of the license.

etienne-lms · 2025-03-07T10:08:12Z

Hi @ldts, sorry, I see I missed you comment #7270 (comment). I would hardly tell whether this driver is worth being factorized with the Versal drivers as I don't known the hardware and the firmwares ABI. I would rather rely on your view.

ldts · 2025-03-08T17:50:02Z

Hi @etienne-lms, no worries—it happens to me all the time.

I think the issue is best understood through a simple example that illustrates my point. Take a look at the PUF registration: PUF registration code.

As it can be observed, this specific mailbox request to the PUF firmware service consists of five separate buffers, requiring alignment and cache management for both read and write operations: Mailbox request handling.

If the AMD driver aims to one day support Versal, it will need to implement something similar to what we already have so both drivers will end up being the same on. Just partitioned a bit differently perhaps.

In any case I do not oppose to the creation of new drivers as long as the license is respected (copying and splitting doesn't void the terms of the license WRT to copyright: https://opensource.org/license/bsd-2-clause )

Akshay-Belsare changed the title ~~Amd mailbox driver support~~ AMD mailbox driver support Feb 10, 2025

jenswi-linaro reviewed Feb 10, 2025

View reviewed changes

core/drivers/amd/mailbox_driver.c Outdated Show resolved Hide resolved

core/drivers/amd/mailbox_driver.c Outdated Show resolved Hide resolved

core/drivers/amd/mailbox_driver.c Show resolved Hide resolved

etienne-lms reviewed Feb 11, 2025

View reviewed changes

Akshay-Belsare force-pushed the amd_driver_support branch from bd5d9e9 to 9ab658e Compare February 11, 2025 12:11

etienne-lms reviewed Feb 11, 2025

View reviewed changes

Akshay-Belsare added 2 commits February 13, 2025 17:32

[Review] address review comments

3e88624

Address upstream review comments Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

[Review] address error conditions

c85d468

Address error condition cases as per review comments. Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

Akshay-Belsare force-pushed the amd_driver_support branch from 9ab658e to c85d468 Compare February 13, 2025 12:26

etienne-lms reviewed Feb 14, 2025

View reviewed changes

[Review] address nitpicks

5655557

-s/id/ID -return values - brace removal - explicit header addition Signed-off-by: Akshay Belsare <akshay.belsare@amd.com>

etienne-lms reviewed Feb 26, 2025

View reviewed changes

etienne-lms reviewed Feb 28, 2025

View reviewed changes

jenswi-linaro reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMD mailbox driver support #7270

AMD mailbox driver support #7270

Akshay-Belsare commented Feb 10, 2025 •

edited

Loading

jenswi-linaro left a comment

etienne-lms Feb 11, 2025

Akshay-Belsare Feb 12, 2025

ldts commented Feb 13, 2025 •

edited

Loading

ldts commented Feb 13, 2025

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025 •

edited

Loading

ldts commented Feb 13, 2025

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025 •

edited

Loading

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025

ldts commented Feb 13, 2025

nathan-menhorn commented Feb 14, 2025

Akshay-Belsare commented Feb 14, 2025

ldts commented Feb 14, 2025

etienne-lms left a comment

etienne-lms Feb 26, 2025

etienne-lms Feb 28, 2025

etienne-lms Feb 26, 2025

ldts commented Feb 27, 2025

etienne-lms left a comment

etienne-lms Feb 27, 2025

etienne-lms Feb 28, 2025

jenswi-linaro Feb 28, 2025

jenswi-linaro commented Feb 28, 2025

Akshay-Belsare commented Mar 6, 2025

ldts commented Mar 7, 2025

etienne-lms commented Mar 7, 2025

ldts commented Mar 8, 2025

AMD mailbox driver support #7270

Are you sure you want to change the base?

AMD mailbox driver support #7270

Conversation

Akshay-Belsare commented Feb 10, 2025 • edited Loading

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts commented Feb 13, 2025 • edited Loading

ldts commented Feb 13, 2025

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025 • edited Loading

ldts commented Feb 13, 2025

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025 • edited Loading

ldts commented Feb 13, 2025

Akshay-Belsare commented Feb 13, 2025

ldts commented Feb 13, 2025

nathan-menhorn commented Feb 14, 2025

Akshay-Belsare commented Feb 14, 2025

ldts commented Feb 14, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldts commented Feb 27, 2025

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Feb 28, 2025

Akshay-Belsare commented Mar 6, 2025

ldts commented Mar 7, 2025

etienne-lms commented Mar 7, 2025

ldts commented Mar 8, 2025

Akshay-Belsare commented Feb 10, 2025 •

edited

Loading

ldts commented Feb 13, 2025 •

edited

Loading

Akshay-Belsare commented Feb 13, 2025 •

edited

Loading

Akshay-Belsare commented Feb 13, 2025 •

edited

Loading