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

[Thomas Huth <thuth@redhat.com>]

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