| Message ID | 20170818114353.13455-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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> >
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.
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.
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.
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 --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);
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(-)