| Message ID | 20170830170601.15855-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <david@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 <thuth@redhat.com>
>> +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 <thuth@redhat.com> > Thanks!
>> +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? General question: how much do we care about headers that are not consistent? E.g. shall I forward declare or simply ignore if compilers don't bite me? > >> 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 <thuth@redhat.com> >
On Thu, 31 Aug 2017 15:11:28 +0200 David Hildenbrand <david@redhat.com> wrote: > >> +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 It would be a bit nicer.
On 31.08.2017 16:29, Cornelia Huck wrote: > On Thu, 31 Aug 2017 15:11:28 +0200 > David Hildenbrand <david@redhat.com> wrote: > >>>> +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 > > It would be a bit nicer. > Adding patches right now to move everything out of cpu.h that lies under the "/* outside of target/s390x/ */" section. :)
On 31.08.2017 16:23, David Hildenbrand wrote: > >>> +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? > > General question: how much do we care about headers that are not consistent? > > E.g. shall I forward declare or simply ignore if compilers don't bite me? My remark was not so much about your patch, but about the original definition instead: "struct S390CPU" is declared in target/s390x/cpu.h, but "typedef struct S390CPU S390CPU" is in target/s390x/cpu-qom.h. I think they should rather be declared in the same header file instead. Or your "struct S390CPU;" forward declaration should go into cpu-qom.h instead, right in front of the typedef. Thomas
On 31.08.2017 16:31, Thomas Huth wrote: > On 31.08.2017 16:23, David Hildenbrand wrote: >> >>>> +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? >> >> General question: how much do we care about headers that are not consistent? >> >> E.g. shall I forward declare or simply ignore if compilers don't bite me? > > My remark was not so much about your patch, but about the original > definition instead: "struct S390CPU" is declared in target/s390x/cpu.h, > but "typedef struct S390CPU S390CPU" is in target/s390x/cpu-qom.h. I > think they should rather be declared in the same header file instead. Or I agree, will have a look. > your "struct S390CPU;" forward declaration should go into cpu-qom.h > instead, right in front of the typedef. > Let me rephrase my question: include/hw/s390x/s390-virtio-ccw.h does not include cpu.h/cpu-qom.h If compilers don't complain, do we have to forward declare at all? (I think it is cleaner, but I would like to know what is suggested) > Thomas >
On Thu, 31 Aug 2017 16:30:59 +0200 David Hildenbrand <david@redhat.com> wrote: > On 31.08.2017 16:29, Cornelia Huck wrote: > > On Thu, 31 Aug 2017 15:11:28 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >>>> +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 > > > > It would be a bit nicer. > > > > Adding patches right now to move everything out of cpu.h that lies under > the "/* outside of target/s390x/ */" section. :) > Ah, you really care about your patch count, don't you? :) (I think it's a good idea.)
On 31.08.2017 16:38, Cornelia Huck wrote: > On Thu, 31 Aug 2017 16:30:59 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 31.08.2017 16:29, Cornelia Huck wrote: >>> On Thu, 31 Aug 2017 15:11:28 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>>>> +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 >>> >>> It would be a bit nicer. >>> >> >> Adding patches right now to move everything out of cpu.h that lies under >> the "/* outside of target/s390x/ */" section. :) >> > > Ah, you really care about your patch count, don't you? :) > > (I think it's a good idea.) > .... so you want me to squash everything into a single patch then?! ;)
On 31.08.2017 16:36, David Hildenbrand wrote: > On 31.08.2017 16:31, Thomas Huth wrote: >> On 31.08.2017 16:23, David Hildenbrand wrote: >>> >>>>> +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? >>> >>> General question: how much do we care about headers that are not consistent? >>> >>> E.g. shall I forward declare or simply ignore if compilers don't bite me? >> >> My remark was not so much about your patch, but about the original >> definition instead: "struct S390CPU" is declared in target/s390x/cpu.h, >> but "typedef struct S390CPU S390CPU" is in target/s390x/cpu-qom.h. I >> think they should rather be declared in the same header file instead. Or > > I agree, will have a look. > >> your "struct S390CPU;" forward declaration should go into cpu-qom.h >> instead, right in front of the typedef. >> > > Let me rephrase my question: > > include/hw/s390x/s390-virtio-ccw.h does not include cpu.h/cpu-qom.h > > If compilers don't complain, do we have to forward declare at all? (I > think it is cleaner, but I would like to know what is suggested) Well, doing a forward declaration with "struct S390CPU;" and then using the typedef'd "S390CPU" without "struct" does not make much sense - these are two "different" types. If you can use "S390CPU" here without the "struct S390CPU;" declaration, that means the cpu-qom.h header got included indirectly already - so I'd suggest to simply remove the "struct S390CPU;" forward declaration from your patch. Thomas
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]; +} + +static void s390_init_cpus(MachineState *machine) +{ + S390CcwMachineState *ms = S390_CCW_MACHINE(machine); + int i; + gchar *name; + + if (machine->cpu_model == NULL) { + machine->cpu_model = s390_default_cpu_model_name(); + } + + ms->cpus = g_new0(S390CPU *, max_cpus); + + for (i = 0; i < max_cpus; i++) { + name = g_strdup_printf("cpu[%i]", i); + object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU, + (Object **) &ms->cpus[i], + object_property_allow_set_link, + OBJ_PROP_LINK_UNREF_ON_RELEASE, + &error_abort); + g_free(name); + } + + for (i = 0; i < smp_cpus; i++) { + s390x_new_cpu(machine->cpu_model, i, &error_fatal); + } +} + static const char *const reset_dev_types[] = { TYPE_VIRTUAL_CSS_BRIDGE, "s390-sclp-event-facility", diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index da3f49e80e..464b5c71f8 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -48,18 +48,6 @@ #define S390_TOD_CLOCK_VALUE_MISSING 0x00 #define S390_TOD_CLOCK_VALUE_PRESENT 0x01 -static S390CPU **cpu_states; - -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) -{ - if (cpu_addr >= max_cpus) { - return NULL; - } - - /* Fast lookup via CPU ID */ - return cpu_states[cpu_addr]; -} - void s390_init_ipl_dev(const char *kernel_filename, const char *kernel_cmdline, const char *initrd_filename, @@ -86,32 +74,6 @@ void s390_init_ipl_dev(const char *kernel_filename, qdev_init_nofail(dev); } -void s390_init_cpus(MachineState *machine) -{ - int i; - gchar *name; - - if (machine->cpu_model == NULL) { - machine->cpu_model = s390_default_cpu_model_name(); - } - - cpu_states = g_new0(S390CPU *, max_cpus); - - for (i = 0; i < max_cpus; i++) { - name = g_strdup_printf("cpu[%i]", i); - object_property_add_link(OBJECT(machine), name, TYPE_S390_CPU, - (Object **) &cpu_states[i], - object_property_allow_set_link, - OBJ_PROP_LINK_UNREF_ON_RELEASE, - &error_abort); - g_free(name); - } - - for (i = 0; i < smp_cpus; i++) { - s390x_new_cpu(machine->cpu_model, i, &error_fatal); - } -} - void s390_create_virtio_net(BusState *bus, const char *name) { diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h index ca97fd6814..b6660e3ae9 100644 --- a/hw/s390x/s390-virtio.h +++ b/hw/s390x/s390-virtio.h @@ -19,7 +19,6 @@ typedef int (*s390_virtio_fn)(const uint64_t *args); void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn); -void s390_init_cpus(MachineState *machine); void s390_init_ipl_dev(const char *kernel_filename, const char *kernel_cmdline, const char *initrd_filename, 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; + typedef struct S390CcwMachineState { /*< private >*/ MachineState parent_obj; /*< public >*/ + S390CPU **cpus; bool aes_key_wrap; bool dea_key_wrap; uint8_t loadparm[8];
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 <david@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(-)