| Message ID | 20250310133118.3881-3-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/machine: Make @dump-skeys command generic | expand |
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>
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
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 >
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, };
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(+)