| Message ID | 20230211091302.4576-1-lina@asahilina.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: apple: rtkit: Do not copy the reg state structure to the stack | expand |
On Sat, 11 Feb 2023 at 09:21, Asahi Lina <lina@asahilina.net> wrote: > > The register state struct is 848 bytes, which ends up bloating the > apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some > 32-bit platforms, triggering compile warnings. > > This doesn't matter for 64BIT/ARM64, but there's also no good reason to > copy the structure to the stack in this case. We can use __packed to > avoid alignment issues, there are no double-read hazards, and this is a > fatal error path so performance does not matter. > > Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog") > Signed-off-by: Asahi Lina <lina@asahilina.net> LGTM, less copies too. Reviewed-by: Eric Curtin <ecurtin@redhat.com> Is mise le meas/Regards, Eric Curtin > --- > drivers/soc/apple/rtkit-crashlog.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/soc/apple/rtkit-crashlog.c b/drivers/soc/apple/rtkit-crashlog.c > index dfa74b32eda2..8319e365110b 100644 > --- a/drivers/soc/apple/rtkit-crashlog.c > +++ b/drivers/soc/apple/rtkit-crashlog.c > @@ -57,7 +57,7 @@ struct apple_rtkit_crashlog_regs { > u64 unk_X; > u64 esr; > u64 unk_Z; > -}; > +} __packed; > static_assert(sizeof(struct apple_rtkit_crashlog_regs) == 0x350); > > static void apple_rtkit_crashlog_dump_str(struct apple_rtkit *rtk, u8 *bfr, > @@ -126,18 +126,18 @@ static void apple_rtkit_crashlog_dump_mailbox(struct apple_rtkit *rtk, u8 *bfr, > static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, > size_t size) > { > - struct apple_rtkit_crashlog_regs regs; > + struct apple_rtkit_crashlog_regs *regs; > const char *el; > int i; > > - if (size < sizeof(regs)) { > + if (size < sizeof(*regs)) { > dev_warn(rtk->dev, "RTKit: Regs section too small: 0x%zx", size); > return; > } > > - memcpy(®s, bfr, sizeof(regs)); > + regs = (struct apple_rtkit_crashlog_regs *)bfr; > > - switch (regs.psr & PSR_MODE_MASK) { > + switch (regs->psr & PSR_MODE_MASK) { > case PSR_MODE_EL0t: > el = "EL0t"; > break; > @@ -160,11 +160,11 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, > > dev_warn(rtk->dev, "RTKit: Exception dump:"); > dev_warn(rtk->dev, " == Exception taken from %s ==", el); > - dev_warn(rtk->dev, " PSR = 0x%llx", regs.psr); > - dev_warn(rtk->dev, " PC = 0x%llx\n", regs.pc); > - dev_warn(rtk->dev, " ESR = 0x%llx\n", regs.esr); > - dev_warn(rtk->dev, " FAR = 0x%llx\n", regs.far); > - dev_warn(rtk->dev, " SP = 0x%llx\n", regs.sp); > + dev_warn(rtk->dev, " PSR = 0x%llx", regs->psr); > + dev_warn(rtk->dev, " PC = 0x%llx\n", regs->pc); > + dev_warn(rtk->dev, " ESR = 0x%llx\n", regs->esr); > + dev_warn(rtk->dev, " FAR = 0x%llx\n", regs->far); > + dev_warn(rtk->dev, " SP = 0x%llx\n", regs->sp); > dev_warn(rtk->dev, "\n"); > > for (i = 0; i < 31; i += 4) { > @@ -172,12 +172,12 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, > dev_warn(rtk->dev, > " x%02d-x%02d = %016llx %016llx %016llx %016llx\n", > i, i + 3, > - regs.regs[i], regs.regs[i + 1], > - regs.regs[i + 2], regs.regs[i + 3]); > + regs->regs[i], regs->regs[i + 1], > + regs->regs[i + 2], regs->regs[i + 3]); > else > dev_warn(rtk->dev, > " x%02d-x%02d = %016llx %016llx %016llx\n", i, i + 3, > - regs.regs[i], regs.regs[i + 1], regs.regs[i + 2]); > + regs->regs[i], regs->regs[i + 1], regs->regs[i + 2]); > } > > dev_warn(rtk->dev, "\n"); > -- > 2.35.1 > >
On Mon, Feb 13, 2023, at 18:15, Eric Curtin wrote: > On Sat, 11 Feb 2023 at 09:21, Asahi Lina <lina@asahilina.net> wrote: >> >> The register state struct is 848 bytes, which ends up bloating the >> apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some >> 32-bit platforms, triggering compile warnings. >> >> This doesn't matter for 64BIT/ARM64, but there's also no good reason to >> copy the structure to the stack in this case. We can use __packed to >> avoid alignment issues, there are no double-read hazards, and this is a >> fatal error path so performance does not matter. >> >> Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog") >> Signed-off-by: Asahi Lina <lina@asahilina.net> > Reviewed-by: Eric Curtin <ecurtin@redhat.com> Merged now, thanks a lot for the fix! Arnd
diff --git a/drivers/soc/apple/rtkit-crashlog.c b/drivers/soc/apple/rtkit-crashlog.c index dfa74b32eda2..8319e365110b 100644 --- a/drivers/soc/apple/rtkit-crashlog.c +++ b/drivers/soc/apple/rtkit-crashlog.c @@ -57,7 +57,7 @@ struct apple_rtkit_crashlog_regs { u64 unk_X; u64 esr; u64 unk_Z; -}; +} __packed; static_assert(sizeof(struct apple_rtkit_crashlog_regs) == 0x350); static void apple_rtkit_crashlog_dump_str(struct apple_rtkit *rtk, u8 *bfr, @@ -126,18 +126,18 @@ static void apple_rtkit_crashlog_dump_mailbox(struct apple_rtkit *rtk, u8 *bfr, static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, size_t size) { - struct apple_rtkit_crashlog_regs regs; + struct apple_rtkit_crashlog_regs *regs; const char *el; int i; - if (size < sizeof(regs)) { + if (size < sizeof(*regs)) { dev_warn(rtk->dev, "RTKit: Regs section too small: 0x%zx", size); return; } - memcpy(®s, bfr, sizeof(regs)); + regs = (struct apple_rtkit_crashlog_regs *)bfr; - switch (regs.psr & PSR_MODE_MASK) { + switch (regs->psr & PSR_MODE_MASK) { case PSR_MODE_EL0t: el = "EL0t"; break; @@ -160,11 +160,11 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, dev_warn(rtk->dev, "RTKit: Exception dump:"); dev_warn(rtk->dev, " == Exception taken from %s ==", el); - dev_warn(rtk->dev, " PSR = 0x%llx", regs.psr); - dev_warn(rtk->dev, " PC = 0x%llx\n", regs.pc); - dev_warn(rtk->dev, " ESR = 0x%llx\n", regs.esr); - dev_warn(rtk->dev, " FAR = 0x%llx\n", regs.far); - dev_warn(rtk->dev, " SP = 0x%llx\n", regs.sp); + dev_warn(rtk->dev, " PSR = 0x%llx", regs->psr); + dev_warn(rtk->dev, " PC = 0x%llx\n", regs->pc); + dev_warn(rtk->dev, " ESR = 0x%llx\n", regs->esr); + dev_warn(rtk->dev, " FAR = 0x%llx\n", regs->far); + dev_warn(rtk->dev, " SP = 0x%llx\n", regs->sp); dev_warn(rtk->dev, "\n"); for (i = 0; i < 31; i += 4) { @@ -172,12 +172,12 @@ static void apple_rtkit_crashlog_dump_regs(struct apple_rtkit *rtk, u8 *bfr, dev_warn(rtk->dev, " x%02d-x%02d = %016llx %016llx %016llx %016llx\n", i, i + 3, - regs.regs[i], regs.regs[i + 1], - regs.regs[i + 2], regs.regs[i + 3]); + regs->regs[i], regs->regs[i + 1], + regs->regs[i + 2], regs->regs[i + 3]); else dev_warn(rtk->dev, " x%02d-x%02d = %016llx %016llx %016llx\n", i, i + 3, - regs.regs[i], regs.regs[i + 1], regs.regs[i + 2]); + regs->regs[i], regs->regs[i + 1], regs->regs[i + 2]); } dev_warn(rtk->dev, "\n");
The register state struct is 848 bytes, which ends up bloating the apple_rtkit_crashlog_dump_regs stack frame beyond 1024 on some 32-bit platforms, triggering compile warnings. This doesn't matter for 64BIT/ARM64, but there's also no good reason to copy the structure to the stack in this case. We can use __packed to avoid alignment issues, there are no double-read hazards, and this is a fatal error path so performance does not matter. Fixes: 22991d8d5725 ("soc: apple: rtkit: Add register dump decoding to crashlog") Signed-off-by: Asahi Lina <lina@asahilina.net> --- drivers/soc/apple/rtkit-crashlog.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)