>> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> +{ >> + S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine()); >> + >> + if (cpu_addr >= max_cpus) { >> + return NULL; >> + } >> + >> + /* Fast lookup via CPU ID */ >> + return ms->cpus[cpu_addr]; >> +} > > I wonder whether that function should rather go into a file in > target/s390x/ instead, since it is also used there and its prototype is > in cpu.h ?
I thought about the same thing, but as it works directly on the machine, like ri_allowed() and friends. So I decided to keep it here for now. I'll think about moving the definition also into include/hw/s390x/s390-virtio-ccw.h > > [...] >> diff --git a/include/hw/s390x/s390-virtio-ccw.h >> b/include/hw/s390x/s390-virtio-ccw.h >> index 41a9d2862b..4bef28ec39 100644 >> --- a/include/hw/s390x/s390-virtio-ccw.h >> +++ b/include/hw/s390x/s390-virtio-ccw.h >> @@ -21,11 +21,14 @@ >> #define S390_MACHINE_CLASS(klass) \ >> OBJECT_CLASS_CHECK(S390CcwMachineClass, (klass), TYPE_S390_CCW_MACHINE) >> >> +struct S390CPU; > > You define a "struct S390CPU" here ... > >> typedef struct S390CcwMachineState { >> /*< private >*/ >> MachineState parent_obj; >> >> /*< public >*/ >> + S390CPU **cpus; I'll simply convert this to struct S390CPU, so this header stays independent from cpu headers. > > ... but use the typedef'ed S390CPU here ... looks somewhat suspicious, I > wonder whether the typedef is really in the right place? > >> bool aes_key_wrap; >> bool dea_key_wrap; >> uint8_t loadparm[8]; > > Anyway, that were just nits, I'm also fine with the patch as it is, so: > > Reviewed-by: Thomas Huth <th...@redhat.com> > Thanks! -- Thanks, David