[PATCH 0/4] Miscellaneous Apple RTKit fixes

[PATCH 0/4] Miscellaneous Apple RTKit fixes

[Sven Peter via B4 Relay]

Hi,

This series contains four unrelated minor fixes to our RTKit driver.
These are mainly required for drivers which aren't upstream yet but
have been part of our downstream tree for quite a while now.

Best,

Sven

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
Hector Martin (1):
      soc: apple: rtkit: Implement OSLog buffers properly

Janne Grunau (3):
      soc: apple: rtkit: Add APPLE_RTKIT_PWR_STATE_INIT
      soc: apple: rtkit: Use high prio work queue
      soc: apple: rtkit: Cut syslog messages after the first '\0'

 drivers/soc/apple/rtkit-internal.h |  1 +
 drivers/soc/apple/rtkit.c          | 62 +++++++++++++++++++++++---------------
 2 files changed, 38 insertions(+), 25 deletions(-)
---
base-commit: 00834971f0d9e38beae37e92055b1432782827d0
change-id: 20250220-apple-soc-misc-55ec124184fd

Best regards,
-- 
Sven Peter <sven@svenpeter.dev>

[PATCH 1/4] soc: apple: rtkit: Add APPLE_RTKIT_PWR_STATE_INIT

[Sven Peter via B4 Relay]

From: Janne Grunau <j@jannau.net>

This state is needed to wake the dcp IOP after m1n1 shut it down.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/soc/apple/rtkit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
index 2f5f878bf899..be0d08861168 100644
--- a/drivers/soc/apple/rtkit.c
+++ b/drivers/soc/apple/rtkit.c
@@ -12,6 +12,7 @@ enum {
 	APPLE_RTKIT_PWR_STATE_IDLE = 0x201, /* sleeping, retain state */
 	APPLE_RTKIT_PWR_STATE_QUIESCED = 0x10, /* running but no communication */
 	APPLE_RTKIT_PWR_STATE_ON = 0x20, /* normal operating state */
+	APPLE_RTKIT_PWR_STATE_INIT = 0x220, /* init after starting the coproc */
 };
 
 enum {
@@ -898,7 +899,7 @@ int apple_rtkit_wake(struct apple_rtkit *rtk)
 	 * Use open-coded apple_rtkit_set_iop_power_state since apple_rtkit_boot
 	 * will wait for the completion anyway.
 	 */
-	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_ON);
+	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_INIT);
 	ret = apple_rtkit_management_send(rtk, APPLE_RTKIT_MGMT_SET_IOP_PWR_STATE,
 					  msg);
 	if (ret)

-- 
2.34.1

[PATCH 2/4] soc: apple: rtkit: Implement OSLog buffers properly

[Sven Peter via B4 Relay]

From: Hector Martin <marcan@marcan.st>

Apparently nobody can figure out where the old logic came from, but it
seems like it has never been actually used on any supported firmware to
this day. OSLog buffers were apparently never requested.

But starting with 13.3, we actually need this implemented properly for
MTP (and later AOP) to work, so let's actually do that.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/soc/apple/rtkit-internal.h |  1 +
 drivers/soc/apple/rtkit.c          | 55 +++++++++++++++++++++++---------------
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/apple/rtkit-internal.h b/drivers/soc/apple/rtkit-internal.h
index 27c9fa745fd5..b8d5244678f0 100644
--- a/drivers/soc/apple/rtkit-internal.h
+++ b/drivers/soc/apple/rtkit-internal.h
@@ -44,6 +44,7 @@ struct apple_rtkit {
 
 	struct apple_rtkit_shmem ioreport_buffer;
 	struct apple_rtkit_shmem crashlog_buffer;
+	struct apple_rtkit_shmem oslog_buffer;
 
 	struct apple_rtkit_shmem syslog_buffer;
 	char *syslog_msg_buffer;
diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
index be0d08861168..35734ae8c9ce 100644
--- a/drivers/soc/apple/rtkit.c
+++ b/drivers/soc/apple/rtkit.c
@@ -67,8 +67,9 @@ enum {
 #define APPLE_RTKIT_SYSLOG_MSG_SIZE  GENMASK_ULL(31, 24)
 
 #define APPLE_RTKIT_OSLOG_TYPE GENMASK_ULL(63, 56)
-#define APPLE_RTKIT_OSLOG_INIT	1
-#define APPLE_RTKIT_OSLOG_ACK	3
+#define APPLE_RTKIT_OSLOG_BUFFER_REQUEST 1
+#define APPLE_RTKIT_OSLOG_SIZE GENMASK_ULL(55, 36)
+#define APPLE_RTKIT_OSLOG_IOVA GENMASK_ULL(35, 0)
 
 #define APPLE_RTKIT_MIN_SUPPORTED_VERSION 11
 #define APPLE_RTKIT_MAX_SUPPORTED_VERSION 12
@@ -259,15 +260,20 @@ static int apple_rtkit_common_rx_get_buffer(struct apple_rtkit *rtk,
 					    struct apple_rtkit_shmem *buffer,
 					    u8 ep, u64 msg)
 {
-	size_t n_4kpages = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg);
 	u64 reply;
 	int err;
 
+	if (ep == APPLE_RTKIT_EP_OSLOG) {
+		buffer->size = FIELD_GET(APPLE_RTKIT_OSLOG_SIZE, msg);
+		buffer->iova = FIELD_GET(APPLE_RTKIT_OSLOG_IOVA, msg) << 12;
+	} else {
+		buffer->size = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg) << 12;
+		buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
+	}
+
 	buffer->buffer = NULL;
 	buffer->iomem = NULL;
 	buffer->is_mapped = false;
-	buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
-	buffer->size = n_4kpages << 12;
 
 	dev_dbg(rtk->dev, "RTKit: buffer request for 0x%zx bytes at %pad\n",
 		buffer->size, &buffer->iova);
@@ -292,11 +298,21 @@ static int apple_rtkit_common_rx_get_buffer(struct apple_rtkit *rtk,
 	}
 
 	if (!buffer->is_mapped) {
-		reply = FIELD_PREP(APPLE_RTKIT_SYSLOG_TYPE,
-				   APPLE_RTKIT_BUFFER_REQUEST);
-		reply |= FIELD_PREP(APPLE_RTKIT_BUFFER_REQUEST_SIZE, n_4kpages);
-		reply |= FIELD_PREP(APPLE_RTKIT_BUFFER_REQUEST_IOVA,
-				    buffer->iova);
+		/* oslog uses different fields */
+		if (ep == APPLE_RTKIT_EP_OSLOG) {
+			reply = FIELD_PREP(APPLE_RTKIT_OSLOG_TYPE,
+					   APPLE_RTKIT_OSLOG_BUFFER_REQUEST);
+			reply |= FIELD_PREP(APPLE_RTKIT_OSLOG_SIZE, buffer->size);
+			reply |= FIELD_PREP(APPLE_RTKIT_OSLOG_IOVA,
+					    buffer->iova >> 12);
+		} else {
+			reply = FIELD_PREP(APPLE_RTKIT_SYSLOG_TYPE,
+					   APPLE_RTKIT_BUFFER_REQUEST);
+			reply |= FIELD_PREP(APPLE_RTKIT_BUFFER_REQUEST_SIZE,
+					    buffer->size >> 12);
+			reply |= FIELD_PREP(APPLE_RTKIT_BUFFER_REQUEST_IOVA,
+					    buffer->iova);
+		}
 		apple_rtkit_send_message(rtk, ep, reply, NULL, false);
 	}
 
@@ -494,25 +510,18 @@ static void apple_rtkit_syslog_rx(struct apple_rtkit *rtk, u64 msg)
 	}
 }
 
-static void apple_rtkit_oslog_rx_init(struct apple_rtkit *rtk, u64 msg)
-{
-	u64 ack;
-
-	dev_dbg(rtk->dev, "RTKit: oslog init: msg: 0x%llx\n", msg);
-	ack = FIELD_PREP(APPLE_RTKIT_OSLOG_TYPE, APPLE_RTKIT_OSLOG_ACK);
-	apple_rtkit_send_message(rtk, APPLE_RTKIT_EP_OSLOG, ack, NULL, false);
-}
-
 static void apple_rtkit_oslog_rx(struct apple_rtkit *rtk, u64 msg)
 {
 	u8 type = FIELD_GET(APPLE_RTKIT_OSLOG_TYPE, msg);
 
 	switch (type) {
-	case APPLE_RTKIT_OSLOG_INIT:
-		apple_rtkit_oslog_rx_init(rtk, msg);
+	case APPLE_RTKIT_OSLOG_BUFFER_REQUEST:
+		apple_rtkit_common_rx_get_buffer(rtk, &rtk->oslog_buffer,
+						 APPLE_RTKIT_EP_OSLOG, msg);
 		break;
 	default:
-		dev_warn(rtk->dev, "RTKit: Unknown oslog message: %llx\n", msg);
+		dev_warn(rtk->dev, "RTKit: Unknown oslog message: %llx\n",
+			 msg);
 	}
 }
 
@@ -729,6 +738,7 @@ int apple_rtkit_reinit(struct apple_rtkit *rtk)
 
 	apple_rtkit_free_buffer(rtk, &rtk->ioreport_buffer);
 	apple_rtkit_free_buffer(rtk, &rtk->crashlog_buffer);
+	apple_rtkit_free_buffer(rtk, &rtk->oslog_buffer);
 	apple_rtkit_free_buffer(rtk, &rtk->syslog_buffer);
 
 	kfree(rtk->syslog_msg_buffer);
@@ -916,6 +926,7 @@ void apple_rtkit_free(struct apple_rtkit *rtk)
 
 	apple_rtkit_free_buffer(rtk, &rtk->ioreport_buffer);
 	apple_rtkit_free_buffer(rtk, &rtk->crashlog_buffer);
+	apple_rtkit_free_buffer(rtk, &rtk->oslog_buffer);
 	apple_rtkit_free_buffer(rtk, &rtk->syslog_buffer);
 
 	kfree(rtk->syslog_msg_buffer);

-- 
2.34.1

[PATCH 3/4] soc: apple: rtkit: Use high prio work queue

[Sven Peter via B4 Relay]

From: Janne Grunau <j@jannau.net>

rtkit messages as communication with the DCP firmware for framebuffer
swaps or input events are time critical so use WQ_HIGHPRI to prevent
user space CPU load to increase latency.
With kwin_wayland 6's explicit sync mode user space load was able to
delay the IOMFB rtkit communication enough to miss vsync for surface
swaps. Minimal test scenario is constantly resizing a glxgears
Xwayland window.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/soc/apple/rtkit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
index 35734ae8c9ce..b7f4654c3341 100644
--- a/drivers/soc/apple/rtkit.c
+++ b/drivers/soc/apple/rtkit.c
@@ -695,7 +695,7 @@ struct apple_rtkit *apple_rtkit_init(struct device *dev, void *cookie,
 	rtk->mbox->rx = apple_rtkit_rx;
 	rtk->mbox->cookie = rtk;
 
-	rtk->wq = alloc_ordered_workqueue("rtkit-%s", WQ_MEM_RECLAIM,
+	rtk->wq = alloc_ordered_workqueue("rtkit-%s", WQ_HIGHPRI | WQ_MEM_RECLAIM,
 					  dev_name(rtk->dev));
 	if (!rtk->wq) {
 		ret = -ENOMEM;

-- 
2.34.1

[PATCH 4/4] soc: apple: rtkit: Cut syslog messages after the first '\0'

[Sven Peter via B4 Relay]

From: Janne Grunau <j@jannau.net>

Certain messages from DCP contain NUL bytes in the random data after the
NUL terminated syslog message. Since the syslog message ends with '\n'
this results in a dev_info() message terminated with two newlines and an
empty printed line in the kernel log.

Signed-off-by: Janne Grunau <j@jannau.net>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/soc/apple/rtkit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
index b7f4654c3341..595efce265ce 100644
--- a/drivers/soc/apple/rtkit.c
+++ b/drivers/soc/apple/rtkit.c
@@ -476,7 +476,7 @@ static void apple_rtkit_syslog_rx_log(struct apple_rtkit *rtk, u64 msg)
 
 	log_context[sizeof(log_context) - 1] = 0;
 
-	msglen = rtk->syslog_msg_size - 1;
+	msglen = strnlen(rtk->syslog_msg_buffer, rtk->syslog_msg_size - 1);
 	while (msglen > 0 &&
 		   should_crop_syslog_char(rtk->syslog_msg_buffer[msglen - 1]))
 		msglen--;

-- 
2.34.1

Re: [PATCH 1/4] soc: apple: rtkit: Add APPLE_RTKIT_PWR_STATE_INIT

[Alyssa Rosenzweig]

> -	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_ON);
> +	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_INIT);

Commit message doesn't match the code. Just adding the state doesn't
imply a behaviour change but the patch is one. How about "Use INIT state
instead of ON" or something to make explicit the behaviour change?

Re: [PATCH 2/4] soc: apple: rtkit: Implement OSLog buffers properly

[Alyssa Rosenzweig]

> +	if (ep == APPLE_RTKIT_EP_OSLOG) {
> +		buffer->size = FIELD_GET(APPLE_RTKIT_OSLOG_SIZE, msg);
> +		buffer->iova = FIELD_GET(APPLE_RTKIT_OSLOG_IOVA, msg) << 12;
> +	} else {
> +		buffer->size = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg) << 12;
> +		buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
> +	}

The shifts are suspiciously asymmetric. Are we really sure this is
correct? My guess is that both size & iova for both oslog & buffer need
to be page-aligned, so all 4 lines should be shifted, and the bit
offsets should be adjusted in turn, and the lower 12-bits in oslog_size
and buffer_iova are reserved. But that's just a guess.

Anyway if this logic is really what we want it deserves a comment
because it looks like a typo.

(Likewise later in the patch)

Re: [PATCH 3/4] soc: apple: rtkit: Use high prio work queue

[Alyssa Rosenzweig]

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Le Sat , Feb 22, 2025 at 04:56:48PM +0000, Sven Peter via B4 Relay a écrit :
> From: Janne Grunau <j@jannau.net>
> 
> rtkit messages as communication with the DCP firmware for framebuffer
> swaps or input events are time critical so use WQ_HIGHPRI to prevent
> user space CPU load to increase latency.
> With kwin_wayland 6's explicit sync mode user space load was able to
> delay the IOMFB rtkit communication enough to miss vsync for surface
> swaps. Minimal test scenario is constantly resizing a glxgears
> Xwayland window.
> 
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  drivers/soc/apple/rtkit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
> index 35734ae8c9ce..b7f4654c3341 100644
> --- a/drivers/soc/apple/rtkit.c
> +++ b/drivers/soc/apple/rtkit.c
> @@ -695,7 +695,7 @@ struct apple_rtkit *apple_rtkit_init(struct device *dev, void *cookie,
>  	rtk->mbox->rx = apple_rtkit_rx;
>  	rtk->mbox->cookie = rtk;
>  
> -	rtk->wq = alloc_ordered_workqueue("rtkit-%s", WQ_MEM_RECLAIM,
> +	rtk->wq = alloc_ordered_workqueue("rtkit-%s", WQ_HIGHPRI | WQ_MEM_RECLAIM,
>  					  dev_name(rtk->dev));
>  	if (!rtk->wq) {
>  		ret = -ENOMEM;
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 4/4] soc: apple: rtkit: Cut syslog messages after the first '\0'

[Alyssa Rosenzweig]

Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>

Le Sat , Feb 22, 2025 at 04:56:49PM +0000, Sven Peter via B4 Relay a écrit :
> From: Janne Grunau <j@jannau.net>
> 
> Certain messages from DCP contain NUL bytes in the random data after the
> NUL terminated syslog message. Since the syslog message ends with '\n'
> this results in a dev_info() message terminated with two newlines and an
> empty printed line in the kernel log.
> 
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  drivers/soc/apple/rtkit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/apple/rtkit.c b/drivers/soc/apple/rtkit.c
> index b7f4654c3341..595efce265ce 100644
> --- a/drivers/soc/apple/rtkit.c
> +++ b/drivers/soc/apple/rtkit.c
> @@ -476,7 +476,7 @@ static void apple_rtkit_syslog_rx_log(struct apple_rtkit *rtk, u64 msg)
>  
>  	log_context[sizeof(log_context) - 1] = 0;
>  
> -	msglen = rtk->syslog_msg_size - 1;
> +	msglen = strnlen(rtk->syslog_msg_buffer, rtk->syslog_msg_size - 1);
>  	while (msglen > 0 &&
>  		   should_crop_syslog_char(rtk->syslog_msg_buffer[msglen - 1]))
>  		msglen--;
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/4] soc: apple: rtkit: Add APPLE_RTKIT_PWR_STATE_INIT

[Sven Peter]

On Mon, Feb 24, 2025, at 19:03, Alyssa Rosenzweig wrote:
>> -	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_ON);
>> +	msg = FIELD_PREP(APPLE_RTKIT_MGMT_PWR_STATE, APPLE_RTKIT_PWR_STATE_INIT);
>
> Commit message doesn't match the code. Just adding the state doesn't
> imply a behaviour change but the patch is one. How about "Use INIT state
> instead of ON" or something to make explicit the behaviour change?

Good point, will change the commit message to something like this for v2:

    soc: apple: rtkit: Add and use PWR_STATE_INIT instead of _ON

    This state is needed to wake the dcp IOP after m1n1 shut it down
    and works for all other co-processors as well.



Thanks,


Sven

Re: [PATCH 2/4] soc: apple: rtkit: Implement OSLog buffers properly

[Sven Peter]

On Mon, Feb 24, 2025, at 19:09, Alyssa Rosenzweig wrote:
>> +	if (ep == APPLE_RTKIT_EP_OSLOG) {
>> +		buffer->size = FIELD_GET(APPLE_RTKIT_OSLOG_SIZE, msg);
>> +		buffer->iova = FIELD_GET(APPLE_RTKIT_OSLOG_IOVA, msg) << 12;
>> +	} else {
>> +		buffer->size = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg) << 12;
>> +		buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
>> +	}
>
> The shifts are suspiciously asymmetric. Are we really sure this is
> correct? My guess is that both size & iova for both oslog & buffer need
> to be page-aligned, so all 4 lines should be shifted, and the bit
> offsets should be adjusted in turn, and the lower 12-bits in oslog_size
> and buffer_iova are reserved. But that's just a guess.
>
> Anyway if this logic is really what we want it deserves a comment
> because it looks like a typo.

That guess can't be true for syslog since there's no change in behavior here
and the syslog endpoint has been working fine so far. This common code is
also used for other endpoints that request buffers and there haven't been
any issues there either. The size is just passed in "number of 4k chunks"
and the IOVA needs no additional fixups.


The entire reason for this commit is because this common logic just didn't
work for oslog. Our u-boot fork uses the same logic as used here [1]. We're stealing
a chunk of MTP's SRAM to make hand-off to Linux easier there. If either size or
IOVA was off by a factor 0x4000 this would've never worked in the first
place.

I'll add a comment that this is really what we want even though it looks odd.


Sven


[1] https://github.com/AsahiLinux/u-boot/blob/582f851413c3fd1fcd60d701ed54fff2e840b9cf/arch/arm/mach-apple/rtkit.c#L144

Re: [PATCH 2/4] soc: apple: rtkit: Implement OSLog buffers properly

[Alyssa Rosenzweig]

> >> +	if (ep == APPLE_RTKIT_EP_OSLOG) {
> >> +		buffer->size = FIELD_GET(APPLE_RTKIT_OSLOG_SIZE, msg);
> >> +		buffer->iova = FIELD_GET(APPLE_RTKIT_OSLOG_IOVA, msg) << 12;
> >> +	} else {
> >> +		buffer->size = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg) << 12;
> >> +		buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
> >> +	}
> >
> > The shifts are suspiciously asymmetric. Are we really sure this is
> > correct? My guess is that both size & iova for both oslog & buffer need
> > to be page-aligned, so all 4 lines should be shifted, and the bit
> > offsets should be adjusted in turn, and the lower 12-bits in oslog_size
> > and buffer_iova are reserved. But that's just a guess.
> >
> > Anyway if this logic is really what we want it deserves a comment
> > because it looks like a typo.
> 
> That guess can't be true for syslog since there's no change in behavior here
> and the syslog endpoint has been working fine so far. This common code is
> also used for other endpoints that request buffers and there haven't been
> any issues there either. The size is just passed in "number of 4k chunks"
> and the IOVA needs no additional fixups.
> 
> 
> The entire reason for this commit is because this common logic just didn't
> work for oslog. Our u-boot fork uses the same logic as used here [1]. We're stealing
> a chunk of MTP's SRAM to make hand-off to Linux easier there. If either size or
> IOVA was off by a factor 0x4000 this would've never worked in the first
> place.

I'm not suggesting things are off by a factor of 4k. Rather I'm
questioning what the behaviour is when we're not 4k aligned. (I.e.
the syslog or oslog buffer does not both start and end at 4k
boundaries.)

If we're aligned, all our bottom bits are 0, and hypothetically we're
putting 0 into "reserved-must be zero" bits.

I guess it's inconsequential if everything is 4k aligned in practice.
But .. is it? I don't know.

Re: [PATCH 2/4] soc: apple: rtkit: Implement OSLog buffers properly

[Sven Peter]

On Wed, Feb 26, 2025, at 18:05, Alyssa Rosenzweig wrote:
>> >> +	if (ep == APPLE_RTKIT_EP_OSLOG) {
>> >> +		buffer->size = FIELD_GET(APPLE_RTKIT_OSLOG_SIZE, msg);
>> >> +		buffer->iova = FIELD_GET(APPLE_RTKIT_OSLOG_IOVA, msg) << 12;
>> >> +	} else {
>> >> +		buffer->size = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_SIZE, msg) << 12;
>> >> +		buffer->iova = FIELD_GET(APPLE_RTKIT_BUFFER_REQUEST_IOVA, msg);
>> >> +	}
>> >
>> > The shifts are suspiciously asymmetric. Are we really sure this is
>> > correct? My guess is that both size & iova for both oslog & buffer need
>> > to be page-aligned, so all 4 lines should be shifted, and the bit
>> > offsets should be adjusted in turn, and the lower 12-bits in oslog_size
>> > and buffer_iova are reserved. But that's just a guess.
>> >
>> > Anyway if this logic is really what we want it deserves a comment
>> > because it looks like a typo.
>> 
>> That guess can't be true for syslog since there's no change in behavior here
>> and the syslog endpoint has been working fine so far. This common code is
>> also used for other endpoints that request buffers and there haven't been
>> any issues there either. The size is just passed in "number of 4k chunks"
>> and the IOVA needs no additional fixups.
>> 
>> 
>> The entire reason for this commit is because this common logic just didn't
>> work for oslog. Our u-boot fork uses the same logic as used here [1]. We're stealing
>> a chunk of MTP's SRAM to make hand-off to Linux easier there. If either size or
>> IOVA was off by a factor 0x4000 this would've never worked in the first
>> place.
>
> I'm not suggesting things are off by a factor of 4k. Rather I'm
> questioning what the behaviour is when we're not 4k aligned. (I.e.
> the syslog or oslog buffer does not both start and end at 4k
> boundaries.)
>
> If we're aligned, all our bottom bits are 0, and hypothetically we're
> putting 0 into "reserved-must be zero" bits.
>
> I guess it's inconsequential if everything is 4k aligned in practice.
> But .. is it? I don't know.


For the common buffers at least we sometimes _get_ the address from the
co-processor when it e.g. points to its internal SRAM. We could access any
buffer aligned to a 4 byte boundary in that case because it's just MMIO
access then and I'm not even sure how strongly the buffer passed to us will be aligned.
I'd rather not zero out bits there because we just cannot be sure those really
are reserved.
If the co-processor only asks for a buffer we'll grab it using dma_alloc_coherent
and it's going to be aligned anyway then.


For oslog there aren't even any "reserved lower bits" in the message. It really
expects the down-shifted address on the wire starting at bit 0.



Sven