qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Hildenbrand
Subject: Re: [Qemu-devel] [PATCH v1 03/11] s390x: store cpu states inside machine state
Date: Thu, 31 Aug 2017 15:11:28 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

>> +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 <address@hidden>
> 

Thanks!

-- 

Thanks,

David



reply via email to

[Prev in Thread] Current Thread [Next in Thread]