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);
+
/**
* @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
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 :|
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
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
>
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>
© 2016 - 2025 Red Hat, Inc.