diff mbox

[v3,for-2.11,08/18] target/s390x: move gtod_*() declarations to s390-virtio.h

Message ID 20170818114353.13455-9-david@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Hildenbrand Aug. 18, 2017, 11:43 a.m. UTC
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio.h | 2 ++
 target/s390x/cpu.h     | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

Comments

Thomas Huth Aug. 18, 2017, 4:11 p.m. UTC | #1
Suggesting to add a patch description like: "The functions are not used
in target/s390x/ so a header in hw/s390x/ is a better place" ?

On 18.08.2017 13:43, David Hildenbrand wrote:
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio.h | 2 ++
>  target/s390x/cpu.h     | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)

Anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>
David Hildenbrand Aug. 18, 2017, 5:28 p.m. UTC | #2
On 18.08.2017 18:11, Thomas Huth wrote:
> Suggesting to add a patch description like: "The functions are not used
> in target/s390x/ so a header in hw/s390x/ is a better place" ?

Sure, I will include that, thanks!

> 
> On 18.08.2017 13:43, David Hildenbrand wrote:
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio.h | 2 ++
>>  target/s390x/cpu.h     | 3 ---
>>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> Anyway:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
Cornelia Huck Aug. 21, 2017, 10:08 a.m. UTC | #3
On Fri, 18 Aug 2017 13:43:43 +0200
David Hildenbrand <david@redhat.com> wrote:

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio.h | 2 ++
>  target/s390x/cpu.h     | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
> index f2377a3..ca97fd6 100644
> --- a/hw/s390x/s390-virtio.h
> +++ b/hw/s390x/s390-virtio.h
> @@ -30,4 +30,6 @@ void s390_create_virtio_net(BusState *bus, const char *name);
>  void s390_nmi(NMIState *n, int cpu_index, Error **errp);
>  void s390_machine_reset(void);
>  void s390_memory_init(ram_addr_t mem_size);
> +void gtod_save(QEMUFile *f, void *opaque);
> +int gtod_load(QEMUFile *f, void *opaque, int version_id);
>  #endif
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 3ce7ffc..c40d70d 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -594,9 +594,6 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu)
>      return cpu->env.cpu_state;
>  }
>  
> -void gtod_save(QEMUFile *f, void *opaque);
> -int gtod_load(QEMUFile *f, void *opaque, int version_id);
> -
>  void cpu_inject_ext(S390CPU *cpu, uint32_t code, uint32_t param,
>                      uint64_t param64);
>  

This patch prompted me to look at the contents of s390-virtio.[ch].
Many of the functions in there only made sense as an exported interface
when we still had the old s390 machine, but they can now simply be
moved to the only user (s390-virtio-ccw.c).

In s390-virtio.c, the only thing used outside of s390-virtio-ccw.c is
s390_cpuaddr2state(), and the only place that uses it for something
other than getting a dummy cpu is the kvm sigp target code. Can we
replace that last usage with a different construct?

In s390-virtio.h, the s390_register_virtio_hypercall() interface is the
only thing that still makes sense to be exported.
David Hildenbrand Aug. 21, 2017, 11:05 a.m. UTC | #4
On 21.08.2017 12:08, Cornelia Huck wrote:
> On Fri, 18 Aug 2017 13:43:43 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio.h | 2 ++
>>  target/s390x/cpu.h     | 3 ---
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
>> index f2377a3..ca97fd6 100644
>> --- a/hw/s390x/s390-virtio.h
>> +++ b/hw/s390x/s390-virtio.h
>> @@ -30,4 +30,6 @@ void s390_create_virtio_net(BusState *bus, const char *name);
>>  void s390_nmi(NMIState *n, int cpu_index, Error **errp);
>>  void s390_machine_reset(void);
>>  void s390_memory_init(ram_addr_t mem_size);
>> +void gtod_save(QEMUFile *f, void *opaque);
>> +int gtod_load(QEMUFile *f, void *opaque, int version_id);
>>  #endif
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 3ce7ffc..c40d70d 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -594,9 +594,6 @@ static inline uint8_t s390_cpu_get_state(S390CPU *cpu)
>>      return cpu->env.cpu_state;
>>  }
>>  
>> -void gtod_save(QEMUFile *f, void *opaque);
>> -int gtod_load(QEMUFile *f, void *opaque, int version_id);
>> -
>>  void cpu_inject_ext(S390CPU *cpu, uint32_t code, uint32_t param,
>>                      uint64_t param64);
>>  
> 
> This patch prompted me to look at the contents of s390-virtio.[ch].
> Many of the functions in there only made sense as an exported interface
> when we still had the old s390 machine, but they can now simply be
> moved to the only user (s390-virtio-ccw.c).

Yes, that is true.

> 
> In s390-virtio.c, the only thing used outside of s390-virtio-ccw.c is
> s390_cpuaddr2state(), and the only place that uses it for something
> other than getting a dummy cpu is the kvm sigp target code. Can we
> replace that last usage with a different construct?

As CPUs are stored in s390-virtio.c (S390CPU **cpu_states) this is not
possible. We could only get access to cpu #x via qom /machine/cpu[#x],
but I guess that won't have best performance :)

We could move that definition into the machine state (which would make
sense, as the cpus belong to a machine).

> 
> In s390-virtio.h, the s390_register_virtio_hypercall() interface is the
> only thing that still makes sense to be exported.
> 

Anyhow, I would prefer to have these cleanups in a separate series.
Nevertheless they make perfect sense.
Cornelia Huck Aug. 21, 2017, 11:14 a.m. UTC | #5
On Mon, 21 Aug 2017 13:05:12 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 21.08.2017 12:08, Cornelia Huck wrote:

> > In s390-virtio.c, the only thing used outside of s390-virtio-ccw.c is
> > s390_cpuaddr2state(), and the only place that uses it for something
> > other than getting a dummy cpu is the kvm sigp target code. Can we
> > replace that last usage with a different construct?  
> 
> As CPUs are stored in s390-virtio.c (S390CPU **cpu_states) this is not
> possible. We could only get access to cpu #x via qom /machine/cpu[#x],
> but I guess that won't have best performance :)
> 
> We could move that definition into the machine state (which would make
> sense, as the cpus belong to a machine).

The machine state looks like a better place than s390-virtio.c.

> 
> > 
> > In s390-virtio.h, the s390_register_virtio_hypercall() interface is the
> > only thing that still makes sense to be exported.
> >   
> 
> Anyhow, I would prefer to have these cleanups in a separate series.
> Nevertheless they make perfect sense.

This is certainly material for a different series :) Just thought I'd
write down what I noticed.
Cornelia Huck Aug. 24, 2017, 12:08 p.m. UTC | #6
On Fri, 18 Aug 2017 19:28:31 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 18.08.2017 18:11, Thomas Huth wrote:
> > Suggesting to add a patch description like: "The functions are not used
> > in target/s390x/ so a header in hw/s390x/ is a better place" ?  
> 
> Sure, I will include that, thanks!

No worries, I'm including that on applying.

> 
> > 
> > On 18.08.2017 13:43, David Hildenbrand wrote:  
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/s390x/s390-virtio.h | 2 ++
> >>  target/s390x/cpu.h     | 3 ---
> >>  2 files changed, 2 insertions(+), 3 deletions(-)  
> > 
> > Anyway:
> > 
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >   
> 
>
diff mbox

Patch

diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index f2377a3..ca97fd6 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -30,4 +30,6 @@  void s390_create_virtio_net(BusState *bus, const char *name);
 void s390_nmi(NMIState *n, int cpu_index, Error **errp);
 void s390_machine_reset(void);
 void s390_memory_init(ram_addr_t mem_size);
+void gtod_save(QEMUFile *f, void *opaque);
+int gtod_load(QEMUFile *f, void *opaque, int version_id);
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 3ce7ffc..c40d70d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -594,9 +594,6 @@  static inline uint8_t s390_cpu_get_state(S390CPU *cpu)
     return cpu->env.cpu_state;
 }
 
-void gtod_save(QEMUFile *f, void *opaque);
-int gtod_load(QEMUFile *f, void *opaque, int version_id);
-
 void cpu_inject_ext(S390CPU *cpu, uint32_t code, uint32_t param,
                     uint64_t param64);