On 30.08.2017 19:05, David Hildenbrand wrote: > Let's avoid global variables. While at it, move both functions using it, > so we won't have to temporarily add includes (we'll be getting rid of > s390-virtio.c soon). > > Signed-off-by: David Hildenbrand <da...@redhat.com> > --- > hw/s390x/s390-virtio-ccw.c | 39 > ++++++++++++++++++++++++++++++++++++++ > hw/s390x/s390-virtio.c | 38 ------------------------------------- > hw/s390x/s390-virtio.h | 1 - > include/hw/s390x/s390-virtio-ccw.h | 3 +++ > 4 files changed, 42 insertions(+), 39 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index dd504dd5ae..ffd56af834 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -32,6 +32,45 @@ > #include "migration/register.h" > #include "cpu_models.h" > > +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 ? [...] > 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; ... 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>