Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support

[Collin Walling]

On 1/27/20 1:21 PM, Collin Walling wrote:
> On 1/27/20 12:55 PM, David Hildenbrand wrote:
>> On 27.01.20 18:29, Cornelia Huck wrote:
>>> On Mon, 27 Jan 2020 18:09:11 +0100
>>> David Hildenbrand <address@hidden> wrote:
>>>
>>>>>>> +static void s390_diag318_reset(DeviceState *dev)
>>>>>>> +{
>>>>>>> +    if (kvm_enabled())
>>>>>>> +        kvm_s390_set_diag318_info(0);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>>>>>>> +{
>>>>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>>>>> +
>>>>>>> +    dc->reset = s390_diag318_reset;
>>>>>>> +    dc->vmsd = &vmstate_diag318;
>>>>>>> +    dc->hotpluggable = false;
>>>>>>> +    /* Reason: Created automatically during machine instantiation */
>>>>>>> +    dc->user_creatable = false;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const TypeInfo s390_diag318_info = {
>>>>>>> +    .class_init = s390_diag318_class_init,
>>>>>>> +    .parent = TYPE_DEVICE,
>>>>>>> +    .name = TYPE_S390_DIAG318,
>>>>>>> +    .instance_size = sizeof(DIAG318State),
>>>>>>> +};
>>>>>>> +
>>>>>>> +static void s390_diag318_register_types(void)
>>>>>>> +{
>>>>>>> +    type_register_static(&s390_diag318_info);
>>>>>>> +}  
>>>>>>
>>>>>> Do we really need a new device? Can't we simply glue that extended state
>>>>>> to the machine state?
>>>>>>  
>>>>>> -> target/s390x/machine.c  
>>>>>>  
>>>>>
>>>>> Those VM States relate to the CPU state... does it make sense to store the
>>>>> diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
>>>>> this info for each CPU).  
>>>>
>>>> I'm sorry, I was looking at the wrong file ...
>>>>
>>>>>
>>>>> Should we store this in the S390CcwMachineState? Or perhaps create a 
>>>>> generic
>>>>> S390MachineState for information that needs to be stored once and migrated
>>>>> once?  
>>>>
>>>> ... I actually thought we have something like this already. Personally,
>>>> I think that would make sense. At least spapr seems to have something
>>>> like this already (hw/ppc/spapr.c:spapr_machine_init().
>>>>
>>>> @Conny?
>>>
>>> What are you referring to? I only see the one with the FIXME in front
>>> of it...
>>
>> That's the one I mean. The fixme states something about qdev ... but
>> AFAIK that's only applicable if TYPE_DEVICE is involved. So maybe right
>> now there is no other way than registering the vmstate directly.
>>
> 
> Hmm okay. I'll take a look at how spapr does it. I think I've registered a
> vmstate via register_savevm_live() in an earlier version, but had difficulties
> figuring out where to store the data. I'll revisit this approach.
> 
> Thanks for the feedback!
> 

Err perhaps not entirely in this manner...

docs/devel/migration.rst declares the register_savevm_live() function as the
"legacy way" of doing things. I'll have to see how other VMStateDescriptions
are modeled. I think vmstate_register() is what I want.

Sorry for the confusion.

-- 
Respectfully,
- Collin Walling