| Message ID | 1301980331-21179-2-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 05.04.2011, at 07:12, David Gibson wrote: > Currently the pseries machine init code builds up an array, envs, of > CPUState pointers for all the cpus in the system. This is kind of > pointless, given the generic code already has a perfectly good linked list > of the cpus. > > In addition, there are a number of places which assume that the cpu's > cpu_index field is equal to its index in this array. This is true in > practice, because cpu_index values are just assigned sequentially, but > it's conceptually incorrect and may not always be true. > > Therefore, this patch abolishes the envs array, and explicitly uses the > generic cpu linked list and cpu_index values throughout. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/spapr.c | 49 ++++++++++++++++++++++++------------------------- > hw/xics.c | 32 +++++++++++++++++++++----------- > hw/xics.h | 3 +-- > 3 files changed, 46 insertions(+), 38 deletions(-) > > diff --git a/hw/spapr.c b/hw/spapr.c > index 1152a25..f80873c 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -57,7 +57,7 @@ > sPAPREnvironment *spapr; > > static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, > - const char *cpu_model, CPUState *envs[], > + const char *cpu_model, > sPAPREnvironment *spapr, > target_phys_addr_t initrd_base, > target_phys_addr_t initrd_size, > @@ -68,6 +68,7 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, > long hash_shift) > { > void *fdt; > + CPUState *env; > uint64_t mem_reg_property[] = { 0, cpu_to_be64(ramsize) }; > uint32_t start_prop = cpu_to_be32(initrd_base); > uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); > @@ -135,14 +136,14 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, > modelname[i] = toupper(modelname[i]); > } > > - for (i = 0; i < smp_cpus; i++) { > - CPUState *env = envs[i]; > - uint32_t gserver_prop[] = {cpu_to_be32(i), 0}; /* HACK! */ > + for (env = first_cpu; env != NULL; env = env->next_cpu) { > + int index = env->cpu_index; > + uint32_t gserver_prop[] = {cpu_to_be32(index), 0}; /* HACK! */ > char *nodename; > uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40), > 0xffffffff, 0xffffffff}; > > - if (asprintf(&nodename, "%s@%x", modelname, i) < 0) { > + if (asprintf(&nodename, "%s@%x", modelname, index) < 0) { > fprintf(stderr, "Allocation failure\n"); > exit(1); > } > @@ -151,7 +152,7 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, > > free(nodename); > > - _FDT((fdt_property_cell(fdt, "reg", i))); > + _FDT((fdt_property_cell(fdt, "reg", index))); > _FDT((fdt_property_string(fdt, "device_type", "cpu"))); > > _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR]))); > @@ -168,11 +169,11 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, > pft_size_prop, sizeof(pft_size_prop)))); > _FDT((fdt_property_string(fdt, "status", "okay"))); > _FDT((fdt_property(fdt, "64-bit", NULL, 0))); > - _FDT((fdt_property_cell(fdt, "ibm,ppc-interrupt-server#s", i))); > + _FDT((fdt_property_cell(fdt, "ibm,ppc-interrupt-server#s", index))); > _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s", > gserver_prop, sizeof(gserver_prop)))); > > - if (envs[i]->mmu_model & POWERPC_MMU_1TSEG) { > + if (env->mmu_model & POWERPC_MMU_1TSEG) { > _FDT((fdt_property(fdt, "ibm,processor-segment-sizes", > segs, sizeof(segs)))); > } > @@ -261,8 +262,8 @@ static void ppc_spapr_init(ram_addr_t ram_size, > const char *initrd_filename, > const char *cpu_model) > { > - CPUState *envs[MAX_CPUS]; > void *fdt, *htab; > + CPUState *env; > int i; > ram_addr_t ram_offset; > target_phys_addr_t fdt_addr, rtas_addr; > @@ -288,7 +289,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, > cpu_model = "POWER7"; > } > for (i = 0; i < smp_cpus; i++) { > - CPUState *env = cpu_init(cpu_model); > + env = cpu_init(cpu_model); > > if (!env) { > fprintf(stderr, "Unable to find PowerPC CPU definition\n"); > @@ -300,9 +301,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, > > env->hreset_vector = 0x60; > env->hreset_excp_prefix = 0; > - env->gpr[3] = i; > - > - envs[i] = env; > + env->gpr[3] = env->cpu_index; > } > > /* allocate RAM */ > @@ -315,10 +314,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, > htab_size = 1ULL << (pteg_shift + 7); > htab = qemu_mallocz(htab_size); > > - for (i = 0; i < smp_cpus; i++) { > - envs[i]->external_htab = htab; > - envs[i]->htab_base = -1; > - envs[i]->htab_mask = htab_size - 1; > + for (env = first_cpu; env != NULL; env = env->next_cpu) { > + env->external_htab = htab; > + env->htab_base = -1; > + env->htab_mask = htab_size - 1; > } > > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin"); > @@ -330,7 +329,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, > qemu_free(filename); > > /* Set up Interrupt Controller */ > - spapr->icp = xics_system_init(smp_cpus, envs, XICS_IRQS); > + spapr->icp = xics_system_init(XICS_IRQS); > > /* Set up VIO bus */ > spapr->vio_bus = spapr_vio_bus_init(); > @@ -416,13 +415,13 @@ static void ppc_spapr_init(ram_addr_t ram_size, > > /* SLOF will startup the secondary CPUs using RTAS, > rather than expecting a kexec() style entry */ > - for (i = 0; i < smp_cpus; i++) { > - envs[i]->halted = 1; > + for (env = first_cpu; env != NULL; env = env->next_cpu) { > + env->halted = 1; > } > } > > /* Prepare the device tree */ > - fdt = spapr_create_fdt(&fdt_size, ram_size, cpu_model, envs, spapr, > + fdt = spapr_create_fdt(&fdt_size, ram_size, cpu_model, spapr, > initrd_base, initrd_size, > boot_device, kernel_cmdline, > rtas_addr, rtas_size, pteg_shift + 7); > @@ -432,10 +431,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, > > qemu_free(fdt); > > - envs[0]->gpr[3] = fdt_addr; > - envs[0]->gpr[5] = 0; > - envs[0]->hreset_vector = kernel_base; > - envs[0]->halted = 0; > + first_cpu->gpr[3] = fdt_addr; > + first_cpu->gpr[5] = 0; > + first_cpu->hreset_vector = kernel_base; > + first_cpu->halted = 0; > } > > static QEMUMachine spapr_machine = { > diff --git a/hw/xics.c b/hw/xics.c > index 66047a6..13a1d25 100644 > --- a/hw/xics.c > +++ b/hw/xics.c > @@ -425,27 +425,39 @@ static void rtas_int_on(sPAPREnvironment *spapr, uint32_t token, > rtas_st(rets, 0, 0); /* Success */ > } > > -struct icp_state *xics_system_init(int nr_servers, CPUState *servers[], > - int nr_irqs) > +struct icp_state *xics_system_init(int nr_irqs) > { > + CPUState *env; > + int max_server_num; > int i; > struct icp_state *icp; > struct ics_state *ics; > > + max_server_num = -1; > + for (env = first_cpu; env != NULL; env = env->next_cpu) { > + if (env->cpu_index > max_server_num) { > + max_server_num = env->cpu_index; > + } > + } > + > icp = qemu_mallocz(sizeof(*icp)); > - icp->nr_servers = nr_servers; > - icp->ss = qemu_mallocz(nr_servers * sizeof(struct icp_server_state)); > + icp->nr_servers = max_server_num + 1; > + icp->ss = qemu_mallocz(icp->nr_servers*sizeof(struct icp_server_state)); > + > + for (i = 0; i < icp->nr_servers; i++) { > + icp->ss[i].mfrr = 0xff; Is ss always big enough to hold all CPUs + 1? Alex
On Tue, Apr 05, 2011 at 10:05:12AM +0200, Alexander Graf wrote: > On 05.04.2011, at 07:12, David Gibson wrote: [snip] > > +struct icp_state *xics_system_init(int nr_irqs) > > { > > + CPUState *env; > > + int max_server_num; > > int i; > > struct icp_state *icp; > > struct ics_state *ics; > > > > + max_server_num = -1; > > + for (env = first_cpu; env != NULL; env = env->next_cpu) { > > + if (env->cpu_index > max_server_num) { > > + max_server_num = env->cpu_index; > > + } > > + } > > + > > icp = qemu_mallocz(sizeof(*icp)); > > - icp->nr_servers = nr_servers; > > - icp->ss = qemu_mallocz(nr_servers * sizeof(struct icp_server_state)); > > + icp->nr_servers = max_server_num + 1; > > + icp->ss = qemu_mallocz(icp->nr_servers*sizeof(struct icp_server_state)); > > + > > + for (i = 0; i < icp->nr_servers; i++) { > > + icp->ss[i].mfrr = 0xff; > > Is ss always big enough to hold all CPUs + 1? Well, it should be. We allocated it just above based on icp->nr_servers, and nr_servers is the largest cpu_index + 1, computer just above that.
On 04/05/2011 03:16 PM, David Gibson wrote: > On Tue, Apr 05, 2011 at 10:05:12AM +0200, Alexander Graf wrote: >> On 05.04.2011, at 07:12, David Gibson wrote: > [snip] >>> +struct icp_state *xics_system_init(int nr_irqs) >>> { >>> + CPUState *env; >>> + int max_server_num; >>> int i; >>> struct icp_state *icp; >>> struct ics_state *ics; >>> >>> + max_server_num = -1; >>> + for (env = first_cpu; env != NULL; env = env->next_cpu) { >>> + if (env->cpu_index> max_server_num) { >>> + max_server_num = env->cpu_index; >>> + } >>> + } >>> + >>> icp = qemu_mallocz(sizeof(*icp)); >>> - icp->nr_servers = nr_servers; >>> - icp->ss = qemu_mallocz(nr_servers * sizeof(struct icp_server_state)); >>> + icp->nr_servers = max_server_num + 1; >>> + icp->ss = qemu_mallocz(icp->nr_servers*sizeof(struct icp_server_state)); >>> + >>> + for (i = 0; i< icp->nr_servers; i++) { >>> + icp->ss[i].mfrr = 0xff; >> Is ss always big enough to hold all CPUs + 1? > Well, it should be. We allocated it just above based on > icp->nr_servers, and nr_servers is the largest cpu_index + 1, computer > just above that Ah, there it is :). I missed that part. Will apply the patches to my tree. Alex
diff --git a/hw/spapr.c b/hw/spapr.c index 1152a25..f80873c 100644 --- a/hw/spapr.c +++ b/hw/spapr.c @@ -57,7 +57,7 @@ sPAPREnvironment *spapr; static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, - const char *cpu_model, CPUState *envs[], + const char *cpu_model, sPAPREnvironment *spapr, target_phys_addr_t initrd_base, target_phys_addr_t initrd_size, @@ -68,6 +68,7 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, long hash_shift) { void *fdt; + CPUState *env; uint64_t mem_reg_property[] = { 0, cpu_to_be64(ramsize) }; uint32_t start_prop = cpu_to_be32(initrd_base); uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); @@ -135,14 +136,14 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, modelname[i] = toupper(modelname[i]); } - for (i = 0; i < smp_cpus; i++) { - CPUState *env = envs[i]; - uint32_t gserver_prop[] = {cpu_to_be32(i), 0}; /* HACK! */ + for (env = first_cpu; env != NULL; env = env->next_cpu) { + int index = env->cpu_index; + uint32_t gserver_prop[] = {cpu_to_be32(index), 0}; /* HACK! */ char *nodename; uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40), 0xffffffff, 0xffffffff}; - if (asprintf(&nodename, "%s@%x", modelname, i) < 0) { + if (asprintf(&nodename, "%s@%x", modelname, index) < 0) { fprintf(stderr, "Allocation failure\n"); exit(1); } @@ -151,7 +152,7 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, free(nodename); - _FDT((fdt_property_cell(fdt, "reg", i))); + _FDT((fdt_property_cell(fdt, "reg", index))); _FDT((fdt_property_string(fdt, "device_type", "cpu"))); _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR]))); @@ -168,11 +169,11 @@ static void *spapr_create_fdt(int *fdt_size, ram_addr_t ramsize, pft_size_prop, sizeof(pft_size_prop)))); _FDT((fdt_property_string(fdt, "status", "okay"))); _FDT((fdt_property(fdt, "64-bit", NULL, 0))); - _FDT((fdt_property_cell(fdt, "ibm,ppc-interrupt-server#s", i))); + _FDT((fdt_property_cell(fdt, "ibm,ppc-interrupt-server#s", index))); _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s", gserver_prop, sizeof(gserver_prop)))); - if (envs[i]->mmu_model & POWERPC_MMU_1TSEG) { + if (env->mmu_model & POWERPC_MMU_1TSEG) { _FDT((fdt_property(fdt, "ibm,processor-segment-sizes", segs, sizeof(segs)))); } @@ -261,8 +262,8 @@ static void ppc_spapr_init(ram_addr_t ram_size, const char *initrd_filename, const char *cpu_model) { - CPUState *envs[MAX_CPUS]; void *fdt, *htab; + CPUState *env; int i; ram_addr_t ram_offset; target_phys_addr_t fdt_addr, rtas_addr; @@ -288,7 +289,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, cpu_model = "POWER7"; } for (i = 0; i < smp_cpus; i++) { - CPUState *env = cpu_init(cpu_model); + env = cpu_init(cpu_model); if (!env) { fprintf(stderr, "Unable to find PowerPC CPU definition\n"); @@ -300,9 +301,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, env->hreset_vector = 0x60; env->hreset_excp_prefix = 0; - env->gpr[3] = i; - - envs[i] = env; + env->gpr[3] = env->cpu_index; } /* allocate RAM */ @@ -315,10 +314,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, htab_size = 1ULL << (pteg_shift + 7); htab = qemu_mallocz(htab_size); - for (i = 0; i < smp_cpus; i++) { - envs[i]->external_htab = htab; - envs[i]->htab_base = -1; - envs[i]->htab_mask = htab_size - 1; + for (env = first_cpu; env != NULL; env = env->next_cpu) { + env->external_htab = htab; + env->htab_base = -1; + env->htab_mask = htab_size - 1; } filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin"); @@ -330,7 +329,7 @@ static void ppc_spapr_init(ram_addr_t ram_size, qemu_free(filename); /* Set up Interrupt Controller */ - spapr->icp = xics_system_init(smp_cpus, envs, XICS_IRQS); + spapr->icp = xics_system_init(XICS_IRQS); /* Set up VIO bus */ spapr->vio_bus = spapr_vio_bus_init(); @@ -416,13 +415,13 @@ static void ppc_spapr_init(ram_addr_t ram_size, /* SLOF will startup the secondary CPUs using RTAS, rather than expecting a kexec() style entry */ - for (i = 0; i < smp_cpus; i++) { - envs[i]->halted = 1; + for (env = first_cpu; env != NULL; env = env->next_cpu) { + env->halted = 1; } } /* Prepare the device tree */ - fdt = spapr_create_fdt(&fdt_size, ram_size, cpu_model, envs, spapr, + fdt = spapr_create_fdt(&fdt_size, ram_size, cpu_model, spapr, initrd_base, initrd_size, boot_device, kernel_cmdline, rtas_addr, rtas_size, pteg_shift + 7); @@ -432,10 +431,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, qemu_free(fdt); - envs[0]->gpr[3] = fdt_addr; - envs[0]->gpr[5] = 0; - envs[0]->hreset_vector = kernel_base; - envs[0]->halted = 0; + first_cpu->gpr[3] = fdt_addr; + first_cpu->gpr[5] = 0; + first_cpu->hreset_vector = kernel_base; + first_cpu->halted = 0; } static QEMUMachine spapr_machine = { diff --git a/hw/xics.c b/hw/xics.c index 66047a6..13a1d25 100644 --- a/hw/xics.c +++ b/hw/xics.c @@ -425,27 +425,39 @@ static void rtas_int_on(sPAPREnvironment *spapr, uint32_t token, rtas_st(rets, 0, 0); /* Success */ } -struct icp_state *xics_system_init(int nr_servers, CPUState *servers[], - int nr_irqs) +struct icp_state *xics_system_init(int nr_irqs) { + CPUState *env; + int max_server_num; int i; struct icp_state *icp; struct ics_state *ics; + max_server_num = -1; + for (env = first_cpu; env != NULL; env = env->next_cpu) { + if (env->cpu_index > max_server_num) { + max_server_num = env->cpu_index; + } + } + icp = qemu_mallocz(sizeof(*icp)); - icp->nr_servers = nr_servers; - icp->ss = qemu_mallocz(nr_servers * sizeof(struct icp_server_state)); + icp->nr_servers = max_server_num + 1; + icp->ss = qemu_mallocz(icp->nr_servers*sizeof(struct icp_server_state)); + + for (i = 0; i < icp->nr_servers; i++) { + icp->ss[i].mfrr = 0xff; + } - for (i = 0; i < nr_servers; i++) { - servers[i]->cpu_index = i; + for (env = first_cpu; env != NULL; env = env->next_cpu) { + struct icp_server_state *ss = &icp->ss[env->cpu_index]; - switch (PPC_INPUT(servers[i])) { + switch (PPC_INPUT(env)) { case PPC_FLAGS_INPUT_POWER7: - icp->ss[i].output = servers[i]->irq_inputs[POWER7_INPUT_INT]; + ss->output = env->irq_inputs[POWER7_INPUT_INT]; break; case PPC_FLAGS_INPUT_970: - icp->ss[i].output = servers[i]->irq_inputs[PPC970_INPUT_INT]; + ss->output = env->irq_inputs[PPC970_INPUT_INT]; break; default: @@ -453,8 +465,6 @@ struct icp_state *xics_system_init(int nr_servers, CPUState *servers[], "model\n"); exit(1); } - - icp->ss[i].mfrr = 0xff; } ics = qemu_mallocz(sizeof(*ics)); diff --git a/hw/xics.h b/hw/xics.h index 096eeb3..83c1182 100644 --- a/hw/xics.h +++ b/hw/xics.h @@ -33,7 +33,6 @@ struct icp_state; qemu_irq xics_find_qirq(struct icp_state *icp, int irq); -struct icp_state *xics_system_init(int nr_servers, CPUState *servers[], - int nr_irqs); +struct icp_state *xics_system_init(int nr_irqs); #endif /* __XICS_H__ */
Currently the pseries machine init code builds up an array, envs, of CPUState pointers for all the cpus in the system. This is kind of pointless, given the generic code already has a perfectly good linked list of the cpus. In addition, there are a number of places which assume that the cpu's cpu_index field is equal to its index in this array. This is true in practice, because cpu_index values are just assigned sequentially, but it's conceptually incorrect and may not always be true. Therefore, this patch abolishes the envs array, and explicitly uses the generic cpu linked list and cpu_index values throughout. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/spapr.c | 49 ++++++++++++++++++++++++------------------------- hw/xics.c | 32 +++++++++++++++++++++----------- hw/xics.h | 3 +-- 3 files changed, 46 insertions(+), 38 deletions(-)