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

[David Hildenbrand]

On 21.08.2017 12:08, Cornelia Huck wrote:
> On Fri, 18 Aug 2017 13:43:43 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  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.

-- 

Thanks,

David