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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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
>