[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] s390-virtio: Factor out some initialization code. |
Date: |
Wed, 16 Jan 2013 14:05:07 +0100 |
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
>