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

[Thomas Huth]

On 30.08.2017 19:05, David Hildenbrand wrote:
> Let's avoid global variables. While at it, move both functions using it,
> so we won't have to temporarily add includes (we'll be getting rid of
> s390-virtio.c soon).
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 39 
> ++++++++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio.c             | 38 -------------------------------------
>  hw/s390x/s390-virtio.h             |  1 -
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  4 files changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index dd504dd5ae..ffd56af834 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -32,6 +32,45 @@
>  #include "migration/register.h"
>  #include "cpu_models.h"
>  
> +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 ?

[...]
> 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;

... 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>