On 27/02/25 14:18, Nicholas Piggin wrote:
On Thu Feb 27, 2025 at 4:49 PM AEST, Aditya Gupta wrote:
Hi Nick,
On 27/02/25 08:37, Nicholas Piggin wrote:
On Mon Feb 17, 2025 at 5:17 PM AEST, Aditya Gupta wrote:
<...snip...>
Ah, that is problematic agreed. I tried to move around things, but
arrived at this.
I will spend some time thinking how to arrange this.
Will need some guidance. How should I approach arranging the code in
such situations ?
My idea was to
* First one is the skeleton: mentions the steps, but doesn't implement
the steps
* Middle patches implement the steps one by one
* Last patch enables it all. So in future if someone checks out the
"Enable fadump" commit they would have all the support ready.
The major problem is "everything" remains unused till this last patch.
But this 1st patch gave me the chance to logically build upon this, eg.
first implement preserving memory regions, then add the fadump_trigger
in os-term rtas call, etc.
Any advice to approach this ?
Yeah, sometimes it's difficult to avoid. Especially with a new
feature like this. If you can't find a better way, that's okay.
One thing could be to return errors from calls. RTAS is a little
bit tricky since there is no general "unsupported" error because
the presence of the token implies some support. You could return
-1 hardware error perhaps.
Another option is implement the call but not all functionality.
E.g., permit dump register/unregister, but don't actually provide
a valid dump on reboot (you could ignore, or provide empty or
invalid format). Downside of that is that if you bisect, a kernel
test case could go bad because it appears to be supported but
produces invalid result.
To avoid that, perhaps you could trip an assert or just log an
error message when performing a reboot with crash dump registered.
But as I said, don't make it too convoluted or lots more work if
it's not easy to rework.
Thanks for the ideas Nick. I guess the first one makes sense if we want
to not need the unused functions.
Only thing I want to check there is, since the
"ibm,configure-kernel-dump" rtas call is registered, kernel will think
fadump is supported, and might try registering fadump (if "fadump=on"
passed in kernel), will see what the kernel does on a failure to
register fadump in earlyboot.
It generally falls back to kdump, will check.
+{
+ struct rtas_fadump_section_header header;
+ target_ulong cmd = rtas_ld(args, 0);
+ target_ulong fdm_addr = rtas_ld(args, 1);
+ target_ulong fdm_size = rtas_ld(args, 2);
+
+ /* Number outputs has to be 1 */
+ if (nret != 1) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADUMP: ibm,configure-kernel-dump RTAS called with nret !=
1.\n");
+ return;
+ }
+
+ /* Number inputs has to be 3 */
+ if (nargs != 3) {
+ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+ return;
+ }
+
+ switch (cmd) {
+ case FADUMP_CMD_REGISTER:
+ if (fadump_metadata.fadump_registered) {
+ /* Fadump already registered */
+ rtas_st(rets, 0, RTAS_OUT_DUMP_ALREADY_REGISTERED);
+ return;
+ }
+
+ if (fadump_metadata.fadump_dump_active == 1) {
+ rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE);
+ return;
+ }
+
+ if (fdm_size < sizeof(struct rtas_fadump_section_header)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "FADUMP: Header size is invalid: %lu\n", fdm_size);
+ rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+ return;
+ }
+
+ /* XXX: Can we ensure fdm_addr points to a valid RMR-memory buffer ? */
RMR memory? There is spapr_rma_size() if that's what you need?
Thanks, will use `spapr_rma_size`. The PAPR says fdm_addr should point
to a valid RMR buffer, I guess that means it should be in the RMA, ie.
`< spapr_rma_size()` ?
Ah yes, PAPR glossray says:
Real Mode Region. This is an obsolete term that is deprecated in favor of RMA.
So that should do what you want.
Sure, thanks !
- Aditya Gupta
Thanks,
Nick
