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

[David Hildenbrand]

>>> +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?

[...]
> 
> How about we introduce a union in the ReadInfo struct. Something like:
> 
>     union {
>         uint8_t  byte_134;
>         struct CPUEntry entries[0];
>     } x;

Or drop the "entries" pointer completely and introduce

static int cpu_entries_offset(void)
{
    /*
     * When we have to indicate features in byte 134, we have to move
     * the start of the cpu entries.
     */
    if (s390_has_feat(S390_FEAT_DIAG318)) {
        return 144;
    }
    return 128;
}

struct CPUEntry *cpu_entries(ReadInfo *ri)
{
    return (struct CPUEntry *)((void *)ri + cpu_entries_offset());
}

unsigned int cpu_entries)count(ReadInfo *ri)
{
    return (SCCB_SIZE - cpu_entries_offset()) / sizeof(CPUEntry);
}

etc. (might take some tweaking to make it compile) and a comment for the
struct. Not sure what's better. Having two struct CPUEntry entries[0] is
also confusing.


Thanks!


-- 
Thanks,

David / dhildenb