Patchwork [2/2] s390-virtio: Factor out some initialization code.

login
register
mail settings
Submitter Cornelia Huck
Date Jan. 16, 2013, 11:57 a.m.
Message ID <1358337445-53555-3-git-send-email-cornelia.huck@de.ibm.com>
Download mbox | patch
Permalink /patch/212496/
State New
Headers show

Comments

Cornelia Huck - Jan. 16, 2013, 11:57 a.m.
Some of the machine initialization for s390-virtio will be reused
by virtio-ccw.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390-virtio.c | 134 ++++++++++++++++++++++++++++++++-----------------------
 hw/s390-virtio.h |   6 +++
 2 files changed, 85 insertions(+), 55 deletions(-)
Alexander Graf - Jan. 16, 2013, 12:19 p.m.
On 16.01.2013, at 12:57, Cornelia Huck wrote:

> Some of the machine initialization for s390-virtio will be reused
> by virtio-ccw.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

I would prefer to see this patch as part of the patch set that introduces the new s390 ccw machine, so I can check whether what it does is sound in context.


Alex
Andreas Färber - Jan. 16, 2013, 12:41 p.m.
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
Cornelia Huck - Jan. 16, 2013, 1:05 p.m.
On Wed, 16 Jan 2013 13:41:10 +0100
Andreas Färber <afaerber@suse.de> 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
>

Patch

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 91ad43b..3856ab7 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -159,62 +159,11 @@  unsigned s390_del_running_cpu(CPUS390XState *env)
     return s390_running_cpus;
 }
 
-/* PC hardware initialisation */
-static void s390_init(QEMUMachineInitArgs *args)
+CPUS390XState *s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
 {
-    ram_addr_t my_ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
     CPUS390XState *env = NULL;
-    MemoryRegion *sysmem = get_system_memory();
-    MemoryRegion *ram = g_new(MemoryRegion, 1);
-    ram_addr_t kernel_size = 0;
-    ram_addr_t initrd_offset;
-    ram_addr_t initrd_size = 0;
-    int shift = 0;
-    uint8_t *storage_keys;
-    void *virtio_region;
-    hwaddr virtio_region_len;
-    hwaddr virtio_region_start;
     int i;
 
-    /* s390x ram size detection needs a 16bit multiplier + an increment. So
-       guests > 64GB can be specified in 2MB steps etc. */
-    while ((my_ram_size >> (20 + shift)) > 65535) {
-        shift++;
-    }
-    my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
-
-    /* lets propagate the changed ram size into the global variable. */
-    ram_size = my_ram_size;
-
-    /* get a BUS */
-    s390_bus = s390_virtio_bus_init(&my_ram_size);
-    s390_sclp_init();
-
-    /* register hypercalls */
-    s390_virtio_register_hcalls();
-
-    /* allocate RAM */
-    memory_region_init_ram(ram, "s390.ram", my_ram_size);
-    vmstate_register_ram_global(ram);
-    memory_region_add_subregion(sysmem, 0, ram);
-
-    /* clear virtio region */
-    virtio_region_len = my_ram_size - ram_size;
-    virtio_region_start = ram_size;
-    virtio_region = cpu_physical_memory_map(virtio_region_start,
-                                            &virtio_region_len, true);
-    memset(virtio_region, 0, virtio_region_len);
-    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
-                              virtio_region_len);
-
-    /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
-
-    /* init CPUs */
     if (cpu_model == NULL) {
         cpu_model = "host";
     }
@@ -235,6 +184,17 @@  static void s390_init(QEMUMachineInitArgs *args)
         tmp_env->exception_index = EXCP_HLT;
         tmp_env->storage_keys = storage_keys;
     }
+    return env;
+}
+
+void s390_set_up_kernel(CPUS390XState *env,
+                        const char *kernel_filename,
+                        const char *kernel_cmdline,
+                        const char *initrd_filename)
+{
+    ram_addr_t kernel_size = 0;
+    ram_addr_t initrd_offset;
+    ram_addr_t initrd_size = 0;
 
     /* One CPU has to run */
     s390_add_running_cpu(env);
@@ -306,9 +266,13 @@  static void s390_init(QEMUMachineInitArgs *args)
         memcpy(rom_ptr(KERN_PARM_AREA), kernel_cmdline,
                strlen(kernel_cmdline) + 1);
     }
+}
 
-    /* Create VirtIO network adapters */
-    for(i = 0; i < nb_nics; i++) {
+void s390_create_virtio_net(BusState *bus, const char *name)
+{
+    int i;
+
+    for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
         DeviceState *dev;
 
@@ -321,12 +285,72 @@  static void s390_init(QEMUMachineInitArgs *args)
             exit(1);
         }
 
-        dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
+        dev = qdev_create(bus, name);
         qdev_set_nic_properties(dev, nd);
         qdev_init_nofail(dev);
     }
 }
 
+/* PC hardware initialisation */
+static void s390_init(QEMUMachineInitArgs *args)
+{
+    ram_addr_t my_ram_size = args->ram_size;
+    const char *cpu_model = args->cpu_model;
+    const char *kernel_filename = args->kernel_filename;
+    const char *kernel_cmdline = args->kernel_cmdline;
+    const char *initrd_filename = args->initrd_filename;
+    CPUS390XState *env = NULL;
+    MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *ram = g_new(MemoryRegion, 1);
+    int shift = 0;
+    uint8_t *storage_keys;
+    void *virtio_region;
+    hwaddr virtio_region_len;
+    hwaddr virtio_region_start;
+
+    /* s390x ram size detection needs a 16bit multiplier + an increment. So
+       guests > 64GB can be specified in 2MB steps etc. */
+    while ((my_ram_size >> (20 + shift)) > 65535) {
+        shift++;
+    }
+    my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
+
+    /* lets propagate the changed ram size into the global variable. */
+    ram_size = my_ram_size;
+
+    /* get a BUS */
+    s390_bus = s390_virtio_bus_init(&my_ram_size);
+    s390_sclp_init();
+
+    /* register hypercalls */
+    s390_virtio_register_hcalls();
+
+    /* allocate RAM */
+    memory_region_init_ram(ram, "s390.ram", my_ram_size);
+    vmstate_register_ram_global(ram);
+    memory_region_add_subregion(sysmem, 0, ram);
+
+    /* clear virtio region */
+    virtio_region_len = my_ram_size - ram_size;
+    virtio_region_start = ram_size;
+    virtio_region = cpu_physical_memory_map(virtio_region_start,
+                                            &virtio_region_len, true);
+    memset(virtio_region, 0, virtio_region_len);
+    cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
+                              virtio_region_len);
+
+    /* allocate storage keys */
+    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+
+    /* init CPUs */
+    env = s390_init_cpus(cpu_model, storage_keys);
+
+    s390_set_up_kernel(env, kernel_filename, kernel_cmdline, initrd_filename);
+
+    /* Create VirtIO network adapters */
+    s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
+}
+
 static QEMUMachine s390_machine = {
     .name = "s390-virtio",
     .alias = "s390",
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);
+void s390_create_virtio_net(BusState *bus, const char *name);
 #endif