| Message ID | 20250217071711.83735-2-adityag@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Implement Firmware Assisted Dump for PSeries | expand |
On Mon Feb 17, 2025 at 5:17 PM AEST, Aditya Gupta wrote: > Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. > > Currently the handler just does basic checks and handles > register/unregister/invalidate requests from kernel. > > Fadump will be enabled in a later patch. > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> > --- > hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ > 2 files changed, 158 insertions(+) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index df2e837632aa..eebdf13b1552 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, > rtas_st(rets, 0, ret); > } > > +struct fadump_metadata fadump_metadata; Can this (and other globals added in other patches) come under SpaprMachineState? And could most of the fadump code and structures go under new spapr_fadump.[ch] files? > + > +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ > +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu, > + SpaprMachineState *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, > + uint32_t nret, target_ulong rets) I don't know about adding a new unused function like this, is there a way to juggle patches around to add it when it's wired up? > +{ > + 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, Nick
Hi Nick, On 27/02/25 08:37, Nicholas Piggin wrote: > On Mon Feb 17, 2025 at 5:17 PM AEST, Aditya Gupta wrote: >> Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. >> >> Currently the handler just does basic checks and handles >> register/unregister/invalidate requests from kernel. >> >> Fadump will be enabled in a later patch. >> >> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> >> --- >> hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ >> 2 files changed, 158 insertions(+) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index df2e837632aa..eebdf13b1552 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, >> rtas_st(rets, 0, ret); >> } >> >> +struct fadump_metadata fadump_metadata; > Can this (and other globals added in other patches) come under > SpaprMachineState? > > And could most of the fadump code and structures go under new > spapr_fadump.[ch] files? Yes, i can move it inside SpaprMachineState. Will put the code in new files. >> + >> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ >> +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu, >> + SpaprMachineState *spapr, >> + uint32_t token, uint32_t nargs, >> + target_ulong args, >> + uint32_t nret, target_ulong rets) > I don't know about adding a new unused function like this, is there > a way to juggle patches around to add it when it's wired up? 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 ? >> +{ >> + 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()` ? - Aditya G > > Thanks, > Nick
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: >>> Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. >>> >>> Currently the handler just does basic checks and handles >>> register/unregister/invalidate requests from kernel. >>> >>> Fadump will be enabled in a later patch. >>> >>> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> >>> --- >>> hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ >>> include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ >>> 2 files changed, 158 insertions(+) >>> >>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >>> index df2e837632aa..eebdf13b1552 100644 >>> --- a/hw/ppc/spapr_rtas.c >>> +++ b/hw/ppc/spapr_rtas.c >>> @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, >>> rtas_st(rets, 0, ret); >>> } >>> >>> +struct fadump_metadata fadump_metadata; >> Can this (and other globals added in other patches) come under >> SpaprMachineState? >> >> And could most of the fadump code and structures go under new >> spapr_fadump.[ch] files? > Yes, i can move it inside SpaprMachineState. Will put the code in new files. >>> + >>> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ >>> +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu, >>> + SpaprMachineState *spapr, >>> + uint32_t token, uint32_t nargs, >>> + target_ulong args, >>> + uint32_t nret, target_ulong rets) >> I don't know about adding a new unused function like this, is there >> a way to juggle patches around to add it when it's wired up? > > 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. > >>> +{ >>> + 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. Thanks, Nick
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 >
On 2/17/25 12:47, Aditya Gupta wrote: > Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. > > Currently the handler just does basic checks and handles > register/unregister/invalidate requests from kernel. > > Fadump will be enabled in a later patch. Let's use FADump or fadump for consistency. > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> > --- > hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ > 2 files changed, 158 insertions(+) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index df2e837632aa..eebdf13b1552 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, > rtas_st(rets, 0, ret); > } > > +struct fadump_metadata fadump_metadata; > + > +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ > +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu, This __attribute shall be avoided if the function can be introduced when actually get used. > + SpaprMachineState *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, > + uint32_t nret, target_ulong rets) > +{ > + 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"); Some of the error cases are using hcall_dprintf below. Let's use same for consistency. Also, shouldn't this case also return RTAS_OUT_PARAM_ERROR ? > + return; > + } > + > + /* Number inputs has to be 3 */ > + if (nargs != 3) { > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); Log 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); Log error ? > + return; > + } > + > + if (fadump_metadata.fadump_dump_active == 1) { > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE); Log error? > + 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 ? */ > + if (fdm_addr <= 0) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "FADUMP: Invalid fdm address: %ld\n", fdm_addr); > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + /* Verify that we understand the fadump header version */ > + cpu_physical_memory_read(fdm_addr, &header, sizeof(header)); > + if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "FADUMP: Unknown fadump header version: 0x%x\n", > + header.dump_format_version); > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + fadump_metadata.fadump_registered = true; > + fadump_metadata.fadump_dump_active = false; > + fadump_metadata.fdm_addr = fdm_addr; > + break; > + case FADUMP_CMD_UNREGISTER: > + if (fadump_metadata.fadump_dump_active == 1) { > + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE); Log error? > + return; > + } > + > + fadump_metadata.fadump_registered = false; > + fadump_metadata.fadump_dump_active = false; > + fadump_metadata.fdm_addr = -1; > + break; > + case FADUMP_CMD_INVALIDATE: > + if (fadump_metadata.fadump_dump_active) { > + fadump_metadata.fadump_registered = false; > + fadump_metadata.fadump_dump_active = false; > + fadump_metadata.fdm_addr = -1; > + memset(&fadump_metadata.registered_fdm, 0, > + sizeof(fadump_metadata.registered_fdm)); > + } else { > + hcall_dprintf("fadump: Nothing to invalidate, no dump active.\n"); Isnt this an error case? Should it return status as error or success ? > + } > + break; > + default: > + hcall_dprintf("Unknown RTAS token 0x%x\n", token); > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > + return; > + } > + > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > +} > + > static void rtas_ibm_os_term(PowerPCCPU *cpu, > SpaprMachineState *spapr, > uint32_t token, uint32_t nargs, > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index a6c0547e313d..efa2f891a8a7 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -704,6 +704,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr); > #define RTAS_OUT_PARAM_ERROR -3 > #define RTAS_OUT_NOT_SUPPORTED -3 > #define RTAS_OUT_NO_SUCH_INDICATOR -3 > +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9 > +#define RTAS_OUT_DUMP_ACTIVE -10 > #define RTAS_OUT_NOT_AUTHORIZED -9002 > #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999 > > @@ -769,6 +771,63 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr); > > #define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D) > > +/* Fadump commands */ > +#define FADUMP_CMD_REGISTER 1 > +#define FADUMP_CMD_UNREGISTER 2 > +#define FADUMP_CMD_INVALIDATE 3 > + > +#define FADUMP_VERSION 1 > + > +/* > + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections > + * in the dump memory structure. Presently, three sections are used for > + * CPU state data, HPTE & Parameters area, while the remaining seven sections > + * can be used for boot memory regions. > + */ > +#define FADUMP_MAX_SECTIONS 10 > +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7 > + > +/* Kernel Dump section info */ > +struct rtas_fadump_section { > + __be32 request_flag; > + __be16 source_data_type; > + __be16 error_flags; > + __be64 source_address; > + __be64 source_len; > + __be64 bytes_dumped; > + __be64 destination_address; > +}; Please refer docs/devel/style.rst for Naming style. CamelCase for structs. > + > +/* ibm,configure-kernel-dump header. */ > +struct rtas_fadump_section_header { > + __be32 dump_format_version; > + __be16 dump_num_sections; > + __be16 dump_status_flag; > + __be32 offset_first_dump_section; > + > + /* Fields for disk dump option. */ > + __be32 dd_block_size; > + __be64 dd_block_offset; > + __be64 dd_num_blocks; > + __be32 dd_offset_disk_path; > + > + /* Maximum time allowed to prevent an automatic dump-reboot. */ > + __be32 max_time_auto; > +}; > + > +struct rtas_fadump_mem_struct { > + struct rtas_fadump_section_header header; > + struct rtas_fadump_section rgn[FADUMP_MAX_SECTIONS]; > +}; > + > +struct fadump_metadata { > + bool fadump_registered; > + bool fadump_dump_active; > + target_ulong fdm_addr; > + struct rtas_fadump_mem_struct registered_fdm; > +}; > +extern struct fadump_metadata fadump_metadata; > + > /* RTAS ibm,get-system-parameter token values */ > #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 > #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE 42
Hi Harsh, Thanks for your reviews. On 04/03/25 14:31, Harsh Prateek Bora wrote: > > > On 2/17/25 12:47, Aditya Gupta wrote: >> Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. >> >> Currently the handler just does basic checks and handles >> register/unregister/invalidate requests from kernel. >> >> Fadump will be enabled in a later patch. > > Let's use FADump or fadump for consistency. > Sure, will use FADump when starting the line, else fadump ? >> >> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> >> --- >> hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ >> 2 files changed, 158 insertions(+) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index df2e837632aa..eebdf13b1552 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -341,6 +341,105 @@ static void >> rtas_ibm_set_system_parameter(PowerPCCPU *cpu, >> rtas_st(rets, 0, ret); >> } >> +struct fadump_metadata fadump_metadata; >> + >> +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ >> +static __attribute((unused)) void >> rtas_configure_kernel_dump(PowerPCCPU *cpu, > > This __attribute shall be avoided if the function can be introduced > when actually get used. Will do it that way in v2, without introducing this unused attribute. > >> + SpaprMachineState *spapr, >> + uint32_t token, uint32_t nargs, >> + target_ulong args, >> + uint32_t nret, target_ulong rets) >> +{ >> + 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"); > > Some of the error cases are using hcall_dprintf below. Let's use same > for consistency. Also, shouldn't this case also return > RTAS_OUT_PARAM_ERROR ? Sure, will use qemu_log_mask then. Thanks for the catch, yes I should have returned PARAM_ERROR, missed it. Will do it. > >> + return; >> + } >> + >> + /* Number inputs has to be 3 */ >> + if (nargs != 3) { >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > > Log error ? Will add. > >> + return; >> + } >> + >> + switch (cmd) { >> + case FADUMP_CMD_REGISTER: >> + if (fadump_metadata.fadump_registered) { >> + /* Fadump already registered */ >> + rtas_st(rets, 0, RTAS_OUT_DUMP_ALREADY_REGISTERED); > > Log error ? Will do. > >> + return; >> + } >> + >> + if (fadump_metadata.fadump_dump_active == 1) { >> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE); > > Log error? Will add. > >> + 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 ? */ >> + if (fdm_addr <= 0) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "FADUMP: Invalid fdm address: %ld\n", fdm_addr); >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >> + return; >> + } >> + >> + /* Verify that we understand the fadump header version */ >> + cpu_physical_memory_read(fdm_addr, &header, sizeof(header)); >> + if (header.dump_format_version != >> cpu_to_be32(FADUMP_VERSION)) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "FADUMP: Unknown fadump header version: 0x%x\n", >> + header.dump_format_version); >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >> + return; >> + } >> + >> + fadump_metadata.fadump_registered = true; >> + fadump_metadata.fadump_dump_active = false; >> + fadump_metadata.fdm_addr = fdm_addr; >> + break; >> + case FADUMP_CMD_UNREGISTER: >> + if (fadump_metadata.fadump_dump_active == 1) { >> + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE); > > Log error? Will add. > >> + return; >> + } >> + >> + fadump_metadata.fadump_registered = false; >> + fadump_metadata.fadump_dump_active = false; >> + fadump_metadata.fdm_addr = -1; >> + break; >> + case FADUMP_CMD_INVALIDATE: >> + if (fadump_metadata.fadump_dump_active) { >> + fadump_metadata.fadump_registered = false; >> + fadump_metadata.fadump_dump_active = false; >> + fadump_metadata.fdm_addr = -1; >> + memset(&fadump_metadata.registered_fdm, 0, >> + sizeof(fadump_metadata.registered_fdm)); >> + } else { >> + hcall_dprintf("fadump: Nothing to invalidate, no dump >> active.\n"); > > Isnt this an error case? Should it return status as error or success ? Not sure. PAPR doesn't specify it any error for this situation. With this current code, software can do invalidate anytime without needing to verify if dump is active or not (shouldn't happen though), but final state should always be that there won't be any dump active and fadump registered is reset. Or should I return a HARDWARE_ERROR or PARAMETER_ERROR for this (don't think either is helpful) ? > >> + } >> + break; >> + default: >> + hcall_dprintf("Unknown RTAS token 0x%x\n", token); >> + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); >> + return; >> + } >> + >> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >> +} >> + >> static void rtas_ibm_os_term(PowerPCCPU *cpu, >> SpaprMachineState *spapr, >> uint32_t token, uint32_t nargs, >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index a6c0547e313d..efa2f891a8a7 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -704,6 +704,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr); >> #define RTAS_OUT_PARAM_ERROR -3 >> #define RTAS_OUT_NOT_SUPPORTED -3 >> #define RTAS_OUT_NO_SUCH_INDICATOR -3 >> +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9 >> +#define RTAS_OUT_DUMP_ACTIVE -10 >> #define RTAS_OUT_NOT_AUTHORIZED -9002 >> #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999 >> @@ -769,6 +771,63 @@ void push_sregs_to_kvm_pr(SpaprMachineState >> *spapr); >> #define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D) >> +/* Fadump commands */ >> +#define FADUMP_CMD_REGISTER 1 >> +#define FADUMP_CMD_UNREGISTER 2 >> +#define FADUMP_CMD_INVALIDATE 3 >> + >> +#define FADUMP_VERSION 1 >> + >> +/* >> + * The Firmware Assisted Dump Memory structure supports a maximum of >> 10 sections >> + * in the dump memory structure. Presently, three sections are used for >> + * CPU state data, HPTE & Parameters area, while the remaining seven >> sections >> + * can be used for boot memory regions. >> + */ >> +#define FADUMP_MAX_SECTIONS 10 >> +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7 >> + >> +/* Kernel Dump section info */ >> +struct rtas_fadump_section { >> + __be32 request_flag; >> + __be16 source_data_type; >> + __be16 error_flags; >> + __be64 source_address; >> + __be64 source_len; >> + __be64 bytes_dumped; >> + __be64 destination_address; >> +}; > > Please refer docs/devel/style.rst for Naming style. CamelCase for > structs. Sure, thanks, will follow it. Thanks, - Aditya Gupta > >> + >> +/* ibm,configure-kernel-dump header. */ >> +struct rtas_fadump_section_header { >> + __be32 dump_format_version; >> + __be16 dump_num_sections; >> + __be16 dump_status_flag; >> + __be32 offset_first_dump_section; >> + >> + /* Fields for disk dump option. */ >> + __be32 dd_block_size; >> + __be64 dd_block_offset; >> + __be64 dd_num_blocks; >> + __be32 dd_offset_disk_path; >> + >> + /* Maximum time allowed to prevent an automatic dump-reboot. */ >> + __be32 max_time_auto; >> +}; >> + >> +struct rtas_fadump_mem_struct { >> + struct rtas_fadump_section_header header; >> + struct rtas_fadump_section rgn[FADUMP_MAX_SECTIONS]; >> +}; >> + >> +struct fadump_metadata { >> + bool fadump_registered; >> + bool fadump_dump_active; >> + target_ulong fdm_addr; >> + struct rtas_fadump_mem_struct registered_fdm; >> +}; >> +extern struct fadump_metadata fadump_metadata; >> + >> /* RTAS ibm,get-system-parameter token values */ >> #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 >> #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE 42
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index df2e837632aa..eebdf13b1552 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -341,6 +341,105 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu, rtas_st(rets, 0, ret); } +struct fadump_metadata fadump_metadata; + +/* Papr Section 7.4.9 ibm,configure-kernel-dump RTAS call */ +static __attribute((unused)) void rtas_configure_kernel_dump(PowerPCCPU *cpu, + SpaprMachineState *spapr, + uint32_t token, uint32_t nargs, + target_ulong args, + uint32_t nret, target_ulong rets) +{ + 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 ? */ + if (fdm_addr <= 0) { + qemu_log_mask(LOG_GUEST_ERROR, + "FADUMP: Invalid fdm address: %ld\n", fdm_addr); + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + return; + } + + /* Verify that we understand the fadump header version */ + cpu_physical_memory_read(fdm_addr, &header, sizeof(header)); + if (header.dump_format_version != cpu_to_be32(FADUMP_VERSION)) { + qemu_log_mask(LOG_GUEST_ERROR, + "FADUMP: Unknown fadump header version: 0x%x\n", + header.dump_format_version); + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + return; + } + + fadump_metadata.fadump_registered = true; + fadump_metadata.fadump_dump_active = false; + fadump_metadata.fdm_addr = fdm_addr; + break; + case FADUMP_CMD_UNREGISTER: + if (fadump_metadata.fadump_dump_active == 1) { + rtas_st(rets, 0, RTAS_OUT_DUMP_ACTIVE); + return; + } + + fadump_metadata.fadump_registered = false; + fadump_metadata.fadump_dump_active = false; + fadump_metadata.fdm_addr = -1; + break; + case FADUMP_CMD_INVALIDATE: + if (fadump_metadata.fadump_dump_active) { + fadump_metadata.fadump_registered = false; + fadump_metadata.fadump_dump_active = false; + fadump_metadata.fdm_addr = -1; + memset(&fadump_metadata.registered_fdm, 0, + sizeof(fadump_metadata.registered_fdm)); + } else { + hcall_dprintf("fadump: Nothing to invalidate, no dump active.\n"); + } + break; + default: + hcall_dprintf("Unknown RTAS token 0x%x\n", token); + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); + return; + } + + rtas_st(rets, 0, RTAS_OUT_SUCCESS); +} + static void rtas_ibm_os_term(PowerPCCPU *cpu, SpaprMachineState *spapr, uint32_t token, uint32_t nargs, diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index a6c0547e313d..efa2f891a8a7 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -704,6 +704,8 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr); #define RTAS_OUT_PARAM_ERROR -3 #define RTAS_OUT_NOT_SUPPORTED -3 #define RTAS_OUT_NO_SUCH_INDICATOR -3 +#define RTAS_OUT_DUMP_ALREADY_REGISTERED -9 +#define RTAS_OUT_DUMP_ACTIVE -10 #define RTAS_OUT_NOT_AUTHORIZED -9002 #define RTAS_OUT_SYSPARM_PARAM_ERROR -9999 @@ -769,6 +771,63 @@ void push_sregs_to_kvm_pr(SpaprMachineState *spapr); #define RTAS_TOKEN_MAX (RTAS_TOKEN_BASE + 0x2D) +/* Fadump commands */ +#define FADUMP_CMD_REGISTER 1 +#define FADUMP_CMD_UNREGISTER 2 +#define FADUMP_CMD_INVALIDATE 3 + +#define FADUMP_VERSION 1 + +/* + * The Firmware Assisted Dump Memory structure supports a maximum of 10 sections + * in the dump memory structure. Presently, three sections are used for + * CPU state data, HPTE & Parameters area, while the remaining seven sections + * can be used for boot memory regions. + */ +#define FADUMP_MAX_SECTIONS 10 +#define RTAS_FADUMP_MAX_BOOT_MEM_REGS 7 + +/* Kernel Dump section info */ +struct rtas_fadump_section { + __be32 request_flag; + __be16 source_data_type; + __be16 error_flags; + __be64 source_address; + __be64 source_len; + __be64 bytes_dumped; + __be64 destination_address; +}; + +/* ibm,configure-kernel-dump header. */ +struct rtas_fadump_section_header { + __be32 dump_format_version; + __be16 dump_num_sections; + __be16 dump_status_flag; + __be32 offset_first_dump_section; + + /* Fields for disk dump option. */ + __be32 dd_block_size; + __be64 dd_block_offset; + __be64 dd_num_blocks; + __be32 dd_offset_disk_path; + + /* Maximum time allowed to prevent an automatic dump-reboot. */ + __be32 max_time_auto; +}; + +struct rtas_fadump_mem_struct { + struct rtas_fadump_section_header header; + struct rtas_fadump_section rgn[FADUMP_MAX_SECTIONS]; +}; + +struct fadump_metadata { + bool fadump_registered; + bool fadump_dump_active; + target_ulong fdm_addr; + struct rtas_fadump_mem_struct registered_fdm; +}; +extern struct fadump_metadata fadump_metadata; + /* RTAS ibm,get-system-parameter token values */ #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS 20 #define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE 42
Implement the handler for "ibm,configure-kernel-dump" rtas call in QEMU. Currently the handler just does basic checks and handles register/unregister/invalidate requests from kernel. Fadump will be enabled in a later patch. Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> --- hw/ppc/spapr_rtas.c | 99 ++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr.h | 59 +++++++++++++++++++++++++ 2 files changed, 158 insertions(+)