Re: [PATCH v2 2/3] cpus: Introduce SysemuCPUOps::qmp_dump_skeys() callback

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 10, 2025 at 02:31:17PM +0100, Philippe Mathieu-Daudé wrote:
> Allow generic CPUs to dump the architecture storage keys.
> 
> Being specific to s390x, it is only implemented there.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/core/sysemu-cpu-ops.h | 6 ++++++
>  target/s390x/cpu-system.c        | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
> index 877892373f9..d3534cba65c 100644
> --- a/include/hw/core/sysemu-cpu-ops.h
> +++ b/include/hw/core/sysemu-cpu-ops.h
> @@ -47,6 +47,12 @@ typedef struct SysemuCPUOps {
>       *       a memory access with the specified memory transaction attributes.
>       */
>      int (*asidx_from_attrs)(CPUState *cpu, MemTxAttrs attrs);
> +
> +    /**
> +     * @qmp_dump_skeys: Callback to dump guest's storage keys to @filename.
> +     */
> +    void (*qmp_dump_skeys)(const char *filename, Error **errp);

Is it right to hook this onto the CPU object ? In the next patch
the code arbitrarily picks the 1st CPU and adds a "FIXME" annotation,
but the actual impl of dump code doesn't seem to be tied to any CPU
object at all, it is getting what looks like a global singleton
object holding the keys.

IOW, should this hook be against the machine type instead, if it
is dumping global state, not tied to a specific CPU ?

> +
>      /**
>       * @get_crash_info: Callback for reporting guest crash information in
>       * GUEST_PANICKED events.
> diff --git a/target/s390x/cpu-system.c b/target/s390x/cpu-system.c
> index 9b380e343c2..ab7bb8d5cf5 100644
> --- a/target/s390x/cpu-system.c
> +++ b/target/s390x/cpu-system.c
> @@ -38,6 +38,7 @@
>  #include "system/system.h"
>  #include "system/tcg.h"
>  #include "hw/core/sysemu-cpu-ops.h"
> +#include "hw/s390x/storage-keys.h"
>  
>  bool s390_cpu_has_work(CPUState *cs)
>  {
> @@ -179,6 +180,7 @@ static const struct SysemuCPUOps s390_sysemu_ops = {
>      .get_phys_page_debug = s390_cpu_get_phys_page_debug,
>      .get_crash_info = s390_cpu_get_crash_info,
>      .write_elf64_note = s390_cpu_write_elf64_note,
> +    .qmp_dump_skeys = s390_qmp_dump_skeys,
>      .legacy_vmsd = &vmstate_s390_cpu,
>  };
>  
> -- 
> 2.47.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|