diff mbox series

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

Message ID 20250310133118.3881-3-philmd@linaro.org
State New
Headers show
Series qapi/machine: Make @dump-skeys command generic | expand

Commit Message

Philippe Mathieu-Daudé March 10, 2025, 1:31 p.m. UTC 
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(+)

Comments

Thomas Huth March 10, 2025, 1:43 p.m. UTC | #1 
On 10/03/2025 14.31, 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(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>
Daniel P. Berrangé March 10, 2025, 1:45 p.m. UTC | #2 
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
Thomas Huth March 10, 2025, 1:48 p.m. UTC | #3 
On 10/03/2025 14.45, Daniel P. Berrangé wrote:
> 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 ?

Hmm, you've got a point - the storage keys are part of the memory, not of 
the CPU, so they might rather belong to the machine instead, indeed.

  Thomas
Philippe Mathieu-Daudé March 10, 2025, 3:06 p.m. UTC | #4 
On 10/3/25 14:48, Thomas Huth wrote:
> On 10/03/2025 14.45, Daniel P. Berrangé wrote:
>> 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 ?

Great analysis!

> Hmm, you've got a point - the storage keys are part of the memory, not 
> of the CPU, so they might rather belong to the machine instead, indeed.
> 
>   Thomas
>
diff mbox series

Patch

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);
+
     /**
      * @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,
 };