On Fri, 2 Sep 2022, Cédric Le Goater wrote: > On 8/19/22 18:55, BALATON Zoltan wrote: >> Change ppc4xx_sdram_banks() to take one Ppc4xxSdramBank array instead >> of the separate arrays and adjust ppc4xx_sdram_init() and >> ppc440_sdram_init() accordingly as well as machines using these. >> >> Signed-off-by: BALATON Zoltan >> --- >> hw/ppc/ppc405.h | 4 +--- >> hw/ppc/ppc405_uc.c | 10 +++++----- >> hw/ppc/ppc440.h | 5 ++--- >> hw/ppc/ppc440_bamboo.c | 15 ++++++--------- >> hw/ppc/ppc440_uc.c | 9 ++++----- >> hw/ppc/ppc4xx_devs.c | 21 +++++++++------------ >> hw/ppc/sam460ex.c | 15 +++++---------- >> include/hw/ppc/ppc4xx.h | 9 +++------ >> 8 files changed, 35 insertions(+), 53 deletions(-) >> >> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h >> index 756865621b..ca0972b88b 100644 >> --- a/hw/ppc/ppc405.h >> +++ b/hw/ppc/ppc405.h >> @@ -167,9 +167,7 @@ struct Ppc405SoCState { >> DeviceState parent_obj; >> /* Public */ >> - MemoryRegion ram_banks[2]; >> - hwaddr ram_bases[2], ram_sizes[2]; >> - >> + Ppc4xxSdramBank ram_banks[2]; >> MemoryRegion *dram_mr; >> hwaddr ram_size; >> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c >> index 2833d0d538..461d18c8a5 100644 >> --- a/hw/ppc/ppc405_uc.c >> +++ b/hw/ppc/ppc405_uc.c >> @@ -1071,14 +1071,14 @@ static void ppc405_soc_realize(DeviceState *dev, >> Error **errp) >> /* SDRAM controller */ >> /* XXX 405EP has no ECC interrupt */ >> - s->ram_bases[0] = 0; >> - s->ram_sizes[0] = s->ram_size; >> - memory_region_init_alias(&s->ram_banks[0], OBJECT(s), >> + s->ram_banks[0].base = 0; >> + s->ram_banks[0].size = s->ram_size; >> + memory_region_init_alias(&s->ram_banks[0].ram, OBJECT(s), >> "ppc405.sdram0", s->dram_mr, >> - s->ram_bases[0], s->ram_sizes[0]); >> + s->ram_banks[0].base, s->ram_banks[0].size); >> ppc4xx_sdram_init(env, qdev_get_gpio_in(DEVICE(&s->uic), 17), 1, >> - s->ram_banks, s->ram_bases, s->ram_sizes); >> + s->ram_banks); > > Compile fails later on : > > ../hw/ppc/ppc405_uc.c: In function ‘ppc405_soc_realize’: > ../hw/ppc/ppc405_uc.c:1083:5: error: ‘ppc4xx_sdram_init’ accessing 576 bytes > in a region of size 272 [-Werror=stringop-overflow=] > 1083 | ppc4xx_sdram_init(env, qdev_get_gpio_in(DEVICE(&s->uic), 17), 1, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1084 | s->ram_banks); > | ~~~~~~~~~~~~~ > ../hw/ppc/ppc405_uc.c:1083:5: note: referencing argument 4 of type > ‘Ppc4xxSdramBank[0]’ I don't understand this error. The 576 bytes seems to be sizeof(Ppc4xxSdramBank) and 272 is sizeof(MemoryRegion) but I don't see what the compiler means here or how this could be avoided. Also my compiler doesn't warn for this so I can't check any alternative solutions. Any ideas? Also this part is removed again two patches later so the best I could do is maybe try rearranging it to swap these patches but if there's a simpler way I'd go for that instead. Regards, BALATON Zoltan > > I am using : > > gcc version 12.2.1 20220819 (Red Hat 12.2.1-1) (GCC) > > Thanks, > > C. > > > >> /* External bus controller */ >> if (!ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(&s->ebc), &s->cpu, errp)) { >> diff --git a/hw/ppc/ppc440.h b/hw/ppc/ppc440.h >> index 7cef936125..5eb2f9a6b3 100644 >> --- a/hw/ppc/ppc440.h >> +++ b/hw/ppc/ppc440.h >> @@ -11,14 +11,13 @@ >> #ifndef PPC440_H >> #define PPC440_H >> -#include "hw/ppc/ppc.h" >> +#include "hw/ppc/ppc4xx.h" >> void ppc4xx_l2sram_init(CPUPPCState *env); >> void ppc4xx_cpr_init(CPUPPCState *env); >> void ppc4xx_sdr_init(CPUPPCState *env); >> void ppc440_sdram_init(CPUPPCState *env, int nbanks, >> - MemoryRegion *ram_memories, >> - hwaddr *ram_bases, hwaddr *ram_sizes, >> + Ppc4xxSdramBank ram_banks[], >> int do_init); >> void ppc4xx_ahb_init(CPUPPCState *env); >> void ppc4xx_dma_init(CPUPPCState *env, int dcr_base); >> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c >> index e3412c4fcd..2aac8a3fe9 100644 >> --- a/hw/ppc/ppc440_bamboo.c >> +++ b/hw/ppc/ppc440_bamboo.c >> @@ -168,9 +168,8 @@ static void bamboo_init(MachineState *machine) >> unsigned int pci_irq_nrs[4] = { 28, 27, 26, 25 }; >> MemoryRegion *address_space_mem = get_system_memory(); >> MemoryRegion *isa = g_new(MemoryRegion, 1); >> - MemoryRegion *ram_memories = g_new(MemoryRegion, >> PPC440EP_SDRAM_NR_BANKS); >> - hwaddr ram_bases[PPC440EP_SDRAM_NR_BANKS] = {0}; >> - hwaddr ram_sizes[PPC440EP_SDRAM_NR_BANKS] = {0}; >> + Ppc4xxSdramBank *ram_banks = g_new0(Ppc4xxSdramBank, >> + PPC440EP_SDRAM_NR_BANKS); >> PCIBus *pcibus; >> PowerPCCPU *cpu; >> CPUPPCState *env; >> @@ -205,13 +204,11 @@ static void bamboo_init(MachineState *machine) >> qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_CINT)); >> /* SDRAM controller */ >> - ppc4xx_sdram_banks(machine->ram, PPC440EP_SDRAM_NR_BANKS, >> ram_memories, >> - ram_bases, ram_sizes, ppc440ep_sdram_bank_sizes); >> + ppc4xx_sdram_banks(machine->ram, PPC440EP_SDRAM_NR_BANKS, ram_banks, >> + ppc440ep_sdram_bank_sizes); >> /* XXX 440EP's ECC interrupts are on UIC1, but we've only created >> UIC0. */ >> - ppc4xx_sdram_init(env, >> - qdev_get_gpio_in(uicdev, 14), >> - PPC440EP_SDRAM_NR_BANKS, ram_memories, >> - ram_bases, ram_sizes); >> + ppc4xx_sdram_init(env, qdev_get_gpio_in(uicdev, 14), >> + PPC440EP_SDRAM_NR_BANKS, ram_banks); >> /* Enable SDRAM memory regions, this should be done by the firmware >> */ >> if (ppc_dcr_write(env->dcr_env, SDRAM0_CFGADDR, 0x20) || >> ppc_dcr_write(env->dcr_env, SDRAM0_CFGDATA, 0x80000000)) { >> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c >> index 6ab0ad7985..3507c35b63 100644 >> --- a/hw/ppc/ppc440_uc.c >> +++ b/hw/ppc/ppc440_uc.c >> @@ -690,8 +690,7 @@ static void sdram_reset(void *opaque) >> } >> void ppc440_sdram_init(CPUPPCState *env, int nbanks, >> - MemoryRegion *ram_memories, >> - hwaddr *ram_bases, hwaddr *ram_sizes, >> + Ppc4xxSdramBank ram_banks[], >> int do_init) >> { >> ppc440_sdram_t *sdram; >> @@ -700,9 +699,9 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks, >> sdram = g_malloc0(sizeof(*sdram)); >> sdram->nbanks = nbanks; >> for (i = 0; i < nbanks; i++) { >> - sdram->bank[i].ram = ram_memories[i]; >> - sdram->bank[i].base = ram_bases[i]; >> - sdram->bank[i].size = ram_sizes[i]; >> + sdram->bank[i].ram = ram_banks[i].ram; >> + sdram->bank[i].base = ram_banks[i].base; >> + sdram->bank[i].size = ram_banks[i].size; >> } >> qemu_register_reset(&sdram_reset, sdram); >> ppc_dcr_register(env, SDRAM0_CFGADDR, >> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c >> index 936d6f77fe..e0b5931c04 100644 >> --- a/hw/ppc/ppc4xx_devs.c >> +++ b/hw/ppc/ppc4xx_devs.c >> @@ -343,9 +343,7 @@ static void sdram_reset(void *opaque) >> } >> void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, int nbanks, >> - MemoryRegion *ram_memories, >> - hwaddr *ram_bases, >> - hwaddr *ram_sizes) >> + Ppc4xxSdramBank ram_banks[]) >> { >> ppc4xx_sdram_t *sdram; >> int i; >> @@ -354,9 +352,9 @@ void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, >> int nbanks, >> sdram->irq = irq; >> sdram->nbanks = nbanks; >> for (i = 0; i < nbanks; i++) { >> - sdram->bank[i].ram = ram_memories[i]; >> - sdram->bank[i].base = ram_bases[i]; >> - sdram->bank[i].size = ram_sizes[i]; >> + sdram->bank[i].ram = ram_banks[i].ram; >> + sdram->bank[i].base = ram_banks[i].base; >> + sdram->bank[i].size = ram_banks[i].size; >> } >> qemu_register_reset(&sdram_reset, sdram); >> ppc_dcr_register(env, SDRAM0_CFGADDR, >> @@ -376,8 +374,7 @@ void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, >> int nbanks, >> * sizes varies by SoC. >> */ >> void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks, >> - MemoryRegion ram_memories[], >> - hwaddr ram_bases[], hwaddr ram_sizes[], >> + Ppc4xxSdramBank ram_banks[], >> const ram_addr_t sdram_bank_sizes[]) >> { >> ram_addr_t size_left = memory_region_size(ram); >> @@ -392,13 +389,13 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int >> nr_banks, >> if (bank_size <= size_left) { >> char name[32]; >> - ram_bases[i] = base; >> - ram_sizes[i] = bank_size; >> + ram_banks[i].base = base; >> + ram_banks[i].size = bank_size; >> base += bank_size; >> size_left -= bank_size; >> snprintf(name, sizeof(name), "ppc4xx.sdram%d", i); >> - memory_region_init_alias(&ram_memories[i], NULL, name, >> ram, >> - ram_bases[i], ram_sizes[i]); >> + memory_region_init_alias(&ram_banks[i].ram, NULL, name, >> ram, >> + ram_banks[i].base, >> ram_banks[i].size); >> break; >> } >> } >> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >> index 850bb3b817..f4c2a693fb 100644 >> --- a/hw/ppc/sam460ex.c >> +++ b/hw/ppc/sam460ex.c >> @@ -73,7 +73,6 @@ >> #define OPB_FREQ 115000000 >> #define EBC_FREQ 115000000 >> #define UART_FREQ 11059200 >> -#define SDRAM_NR_BANKS 4 >> /* The SoC could also handle 4 GiB but firmware does not work with >> that. */ >> /* Maybe it overflows a signed 32 bit number somewhere? */ >> @@ -274,9 +273,7 @@ static void sam460ex_init(MachineState *machine) >> { >> MemoryRegion *address_space_mem = get_system_memory(); >> MemoryRegion *isa = g_new(MemoryRegion, 1); >> - MemoryRegion *ram_memories = g_new(MemoryRegion, SDRAM_NR_BANKS); >> - hwaddr ram_bases[SDRAM_NR_BANKS] = {0}; >> - hwaddr ram_sizes[SDRAM_NR_BANKS] = {0}; >> + Ppc4xxSdramBank *ram_banks = g_new0(Ppc4xxSdramBank, 1); >> MemoryRegion *l2cache_ram = g_new(MemoryRegion, 1); >> DeviceState *uic[4]; >> int i; >> @@ -345,20 +342,18 @@ static void sam460ex_init(MachineState *machine) >> /* SDRAM controller */ >> /* put all RAM on first bank because board has one slot >> * and firmware only checks that */ >> - ppc4xx_sdram_banks(machine->ram, 1, ram_memories, ram_bases, >> ram_sizes, >> - ppc460ex_sdram_bank_sizes); >> + ppc4xx_sdram_banks(machine->ram, 1, ram_banks, >> ppc460ex_sdram_bank_sizes); >> /* FIXME: does 460EX have ECC interrupts? */ >> - ppc440_sdram_init(env, SDRAM_NR_BANKS, ram_memories, >> - ram_bases, ram_sizes, 1); >> + ppc440_sdram_init(env, 1, ram_banks, 1); >> /* IIC controllers and devices */ >> dev = sysbus_create_simple(TYPE_PPC4xx_I2C, 0x4ef600700, >> qdev_get_gpio_in(uic[0], 2)); >> i2c = PPC4xx_I2C(dev)->bus; >> /* SPD EEPROM on RAM module */ >> - spd_data = spd_data_generate(ram_sizes[0] < 128 * MiB ? DDR : DDR2, >> - ram_sizes[0]); >> + spd_data = spd_data_generate(ram_banks->size < 128 * MiB ? DDR : DDR2, >> + ram_banks->size); >> spd_data[20] = 4; /* SO-DIMM module */ >> smbus_eeprom_init_one(i2c, 0x50, spd_data); >> /* RTC */ >> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h >> index a5e6c185af..0a9781bfaf 100644 >> --- a/include/hw/ppc/ppc4xx.h >> +++ b/include/hw/ppc/ppc4xx.h >> @@ -43,14 +43,11 @@ enum { >> }; >> void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks, >> - MemoryRegion ram_memories[], >> - hwaddr ram_bases[], hwaddr ram_sizes[], >> + Ppc4xxSdramBank ram_banks[], >> const ram_addr_t sdram_bank_sizes[]); >> -void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks, >> - MemoryRegion ram_memories[], >> - hwaddr *ram_bases, >> - hwaddr *ram_sizes); >> +void ppc4xx_sdram_init(CPUPPCState *env, qemu_irq irq, int nbanks, >> + Ppc4xxSdramBank ram_banks[]); >> #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost" >> > >