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

[Cornelia Huck]

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...

> 
> [...]
> > 
> > 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.

I think that version may end up looking better.