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

[Cornelia Huck]

On Wed, 16 Jan 2013 13:41:10 +0100
Andreas Färber <address@hidden> wrote:

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

An alternative might be to use s390_cpu_addr2state(0) to effectively
get to the same cpu.

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

s390_set_up_kernel() uses it to specify the initial psw, and for
virtio-ccw, it will be needed to issue an ioctl. But both use cases
could be covered by grabbing cpu 0.

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