On Mon, 27 Jan 2020 18:09:11 +0100
David Hildenbrand <da...@redhat.com> 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.
