>> +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 <th...@redhat.com>
>
Thanks!
--
Thanks,
David
