Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code.

[Andreas Färber]

Am 16.01.2013 12:57, schrieb Cornelia Huck:
> diff --git a/hw/s390-virtio.h b/hw/s390-virtio.h
> index cd88179..acd4846 100644
> --- a/hw/s390-virtio.h
> +++ b/hw/s390-virtio.h
> @@ -20,4 +20,10 @@ typedef int (*s390_virtio_fn)(uint64_t reg2, uint64_t 
> reg3, uint64_t reg4,
>                                uint64_t reg5, uint64_t reg6, uint64_t reg7);
>  void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
>  
> +CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
> +void s390_set_up_kernel(CPUS390XState *env,
> +                        const char *kernel_filename,
> +                        const char *kernel_cmdline,
> +                        const char *initrd_filename);

I don't like this interface: It reads "cpus" but appears to return a
single CPUS390XState. Can't you at least use S390CPU* instead?

Alternatively it would be possible (although at some point to be
changed) to use global first_cpu and to iterate over the CPUs rather
than returning one from one function to the other.

However since the only usage I spot in the patch without looking up the
file myself is s390_add_running_cpu(), can the call be moved out of the
kernel setup function to avoid this dependency?

Andreas

> +void s390_create_virtio_net(BusState *bus, const char *name);
>  #endif

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg