Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state

[Thomas Huth]

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

[David Hildenbrand]

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

[Cornelia Huck]

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

[David Hildenbrand]

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

[Thomas Huth]

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

[David Hildenbrand]

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

[Cornelia Huck]

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

[David Hildenbrand]

>> +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

[David Hildenbrand]

>> +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

[Thomas Huth]

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

[David Hildenbrand]

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 **) >cpus[i],
+ object_property_allow_set_link,
+ OBJ_PROP_LINK_UNREF_ON_RELEASE,
+ _abort);
+g_free(name);
+}
+
+for (i = 0; i < smp_cpus; i++) {
+s390x_new_cpu(machine->cpu_model, i, _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 **) _states[i],
- object_property_allow_set_link,
- OBJ_PROP_LINK_UNREF_ON_RELEASE,
- _abort);
-g_free(name);
-}
-
-for (i = 0; i < smp_cpus; i++) {
-s390x_new_cpu(machine->cpu_model, i, _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