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];
+}
+
+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];
--
2.13.5
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!
--
Thanks,
David
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. :)
--
Thanks,
David
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?! ;)
--
Thanks,
David
>> +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>
>
--
Thanks,
David
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
>
--
Thanks,
David
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
© 2016 - 2026 Red Hat, Inc.