Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
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
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
On 31.08.2017 16:38, Cornelia Huck wrote: > On Thu, 31 Aug 2017 16:30:59 +0200 > David Hildenbrand wrote: > >> On 31.08.2017 16:29, Cornelia Huck wrote: >>> On Thu, 31 Aug 2017 15:11:28 +0200 >>> David Hildenbrand 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?! ;) -- Thanks, David
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
On Thu, 31 Aug 2017 16:30:59 +0200 David Hildenbrand wrote: > On 31.08.2017 16:29, Cornelia Huck wrote: > > On Thu, 31 Aug 2017 15:11:28 +0200 > > David Hildenbrand 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.)
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
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 > -- Thanks, David
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
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
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
On 31.08.2017 16:29, Cornelia Huck wrote: > On Thu, 31 Aug 2017 15:11:28 +0200 > David Hildenbrand 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. :) -- Thanks, David
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
On Thu, 31 Aug 2017 15:11:28 +0200 David Hildenbrand 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.
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
>> +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 > -- Thanks, David
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
>> +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 > Thanks! -- Thanks, David
Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
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 > --- > 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
[Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
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 --- 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]; +} + +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_MISSING0x00 #define S390_TOD_CLOCK_VALUE_PRESENT0x01 -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]; -- 2.13.5