| Message ID | d1ef93cefae7f103fc23046bb35864f8e493340f.1467417982.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote: > When loading ROMs allow the caller to specify an AddressSpace to use for > the load. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > V8: > - Introduce an RFC version of AddressSpace loading support > > hw/core/loader.c | 18 ++++++++++++------ > include/hw/elf_ops.h | 2 +- > include/hw/loader.h | 10 ++++++---- > 3 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 53e0e41..fcbcfbf 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -777,6 +777,7 @@ struct Rom { > > uint8_t *data; > MemoryRegion *mr; > + AddressSpace *as; > int isrom; > char *fw_dir; > char *fw_file; > @@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name) > > int rom_add_file(const char *file, const char *fw_dir, > hwaddr addr, int32_t bootindex, > - bool option_rom, MemoryRegion *mr) > + bool option_rom, MemoryRegion *mr, > + AddressSpace *as) We seem to add this argument to the function but never use it? I think specifying both mr and as should be an error. > { > MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > Rom *rom; > @@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, > * memory ownership of "data", so we don't have to allocate and copy the buffer. > */ > int rom_add_elf_program(const char *name, void *data, size_t datasize, > - size_t romsize, hwaddr addr) > + size_t romsize, hwaddr addr, AddressSpace *as) > { > Rom *rom; > > @@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, > rom->datasize = datasize; > rom->romsize = romsize; > rom->data = data; > + rom->as = as; > rom_insert(rom); > return 0; > } > > int rom_add_vga(const char *file) > { > - return rom_add_file(file, "vgaroms", 0, -1, true, NULL); > + return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL); > } > > int rom_add_option(const char *file, int32_t bootindex) > { > - return rom_add_file(file, "genroms", 0, bootindex, true, NULL); > + return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL); > } > > static void rom_reset(void *unused) > @@ -1008,7 +1011,8 @@ static void rom_reset(void *unused) > void *host = memory_region_get_ram_ptr(rom->mr); > memcpy(host, rom->data, rom->datasize); > } else { > - cpu_physical_memory_write_rom(&address_space_memory, > + cpu_physical_memory_write_rom(rom->as ? rom->as : > + &address_space_memory, > rom->addr, rom->data, rom->datasize); > } > if (rom->isrom) { > @@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void) > hwaddr addr = 0; > MemoryRegionSection section; > Rom *rom; > + AddressSpace *as = NULL; > > QTAILQ_FOREACH(rom, &roms, next) { > if (rom->fw_file) { > continue; > } > - if (addr > rom->addr) { > + if ((addr > rom->addr) && (as == rom->as)) { > fprintf(stderr, "rom: requested regions overlap " > "(rom %s. free=0x" TARGET_FMT_plx > ", addr=0x" TARGET_FMT_plx ")\n", > @@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void) > section = memory_region_find(get_system_memory(), rom->addr, 1); (An unrelated bug but I've just noticed that this code doesn't make sense for ROMs which go into a memory region rather than at an address in the system address space.) > rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr); > memory_region_unref(section.mr); > + as = rom->as; > } I don't think this attempt at checking is going to actually catch all the overlap cases if you allow multiple address spaces. The rom_insert() code orders roms in this list purely by load address, so you could get cases like [AS A, 0..0x1000], [AS B, 0..0x1000], [AS A, 0x500..0x1000] where entries 1 and 3 overlap but won't get caught. You probably need to order the list by AS first and then by address, and then adjust this code accordingly. > qemu_register_reset(rom_reset, NULL); > roms_loaded = 1; > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h > index db70c11..1339677 100644 > --- a/include/hw/elf_ops.h > +++ b/include/hw/elf_ops.h > @@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, > snprintf(label, sizeof(label), "phdr #%d: %s", i, name); > > /* rom_add_elf_program() seize the ownership of 'data' */ > - rom_add_elf_program(label, data, file_size, mem_size, addr); > + rom_add_elf_program(label, data, file_size, mem_size, addr, NULL); > > total_size += mem_size; > if (addr < low) > diff --git a/include/hw/loader.h b/include/hw/loader.h > index 4879b63..18eb0f2 100644 > --- a/include/hw/loader.h > +++ b/include/hw/loader.h > @@ -118,14 +118,14 @@ extern bool rom_file_has_mr; > > int rom_add_file(const char *file, const char *fw_dir, > hwaddr addr, int32_t bootindex, > - bool option_rom, MemoryRegion *mr); > + bool option_rom, MemoryRegion *mr, AddressSpace *as); > MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, > size_t max_len, hwaddr addr, > const char *fw_file_name, > FWCfgReadCallback fw_callback, > void *callback_opaque); > int rom_add_elf_program(const char *name, void *data, size_t datasize, > - size_t romsize, hwaddr addr); > + size_t romsize, hwaddr addr, AddressSpace *as); > int rom_check_and_register_reset(void); > void rom_set_fw(FWCfgState *f); > void rom_set_order_override(int order); > @@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr); > void hmp_info_roms(Monitor *mon, const QDict *qdict); > > #define rom_add_file_fixed(_f, _a, _i) \ > - rom_add_file(_f, NULL, _a, _i, false, NULL) > + rom_add_file(_f, NULL, _a, _i, false, NULL, NULL) > #define rom_add_blob_fixed(_f, _b, _l, _a) \ > rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL) > #define rom_add_file_mr(_f, _mr, _i) \ > - rom_add_file(_f, NULL, 0, _i, false, _mr) > + rom_add_file(_f, NULL, 0, _i, false, _mr, NULL) > +#define rom_add_file_as(_f, _as, _i) \ > + rom_add_file(_f, NULL, 0, _i, false, NULL, _as) > > #define PC_ROM_MIN_VGA 0xc0000 > #define PC_ROM_MIN_OPTION 0xc8000 > -- > 2.7.4 > thanks -- PMM
On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote: > When loading ROMs allow the caller to specify an AddressSpace to use for > the load. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Forgot to mention -- there are some typos in the subject. thanks -- PMM
On Tue, Jul 12, 2016 at 9:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 2 July 2016 at 02:07, Alistair Francis <alistair.francis@xilinx.com> wrote: >> When loading ROMs allow the caller to specify an AddressSpace to use for >> the load. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> V8: >> - Introduce an RFC version of AddressSpace loading support >> >> hw/core/loader.c | 18 ++++++++++++------ >> include/hw/elf_ops.h | 2 +- >> include/hw/loader.h | 10 ++++++---- >> 3 files changed, 19 insertions(+), 11 deletions(-) >> >> diff --git a/hw/core/loader.c b/hw/core/loader.c >> index 53e0e41..fcbcfbf 100644 >> --- a/hw/core/loader.c >> +++ b/hw/core/loader.c >> @@ -777,6 +777,7 @@ struct Rom { >> >> uint8_t *data; >> MemoryRegion *mr; >> + AddressSpace *as; >> int isrom; >> char *fw_dir; >> char *fw_file; >> @@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name) >> >> int rom_add_file(const char *file, const char *fw_dir, >> hwaddr addr, int32_t bootindex, >> - bool option_rom, MemoryRegion *mr) >> + bool option_rom, MemoryRegion *mr, >> + AddressSpace *as) > > We seem to add this argument to the function but never use it? I have fixed that. > > I think specifying both mr and as should be an error. Agreed, it is now an error. > >> { >> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); >> Rom *rom; >> @@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, >> * memory ownership of "data", so we don't have to allocate and copy the buffer. >> */ >> int rom_add_elf_program(const char *name, void *data, size_t datasize, >> - size_t romsize, hwaddr addr) >> + size_t romsize, hwaddr addr, AddressSpace *as) >> { >> Rom *rom; >> >> @@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, >> rom->datasize = datasize; >> rom->romsize = romsize; >> rom->data = data; >> + rom->as = as; >> rom_insert(rom); >> return 0; >> } >> >> int rom_add_vga(const char *file) >> { >> - return rom_add_file(file, "vgaroms", 0, -1, true, NULL); >> + return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL); >> } >> >> int rom_add_option(const char *file, int32_t bootindex) >> { >> - return rom_add_file(file, "genroms", 0, bootindex, true, NULL); >> + return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL); >> } >> >> static void rom_reset(void *unused) >> @@ -1008,7 +1011,8 @@ static void rom_reset(void *unused) >> void *host = memory_region_get_ram_ptr(rom->mr); >> memcpy(host, rom->data, rom->datasize); >> } else { >> - cpu_physical_memory_write_rom(&address_space_memory, >> + cpu_physical_memory_write_rom(rom->as ? rom->as : >> + &address_space_memory, >> rom->addr, rom->data, rom->datasize); >> } >> if (rom->isrom) { >> @@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void) >> hwaddr addr = 0; >> MemoryRegionSection section; >> Rom *rom; >> + AddressSpace *as = NULL; >> >> QTAILQ_FOREACH(rom, &roms, next) { >> if (rom->fw_file) { >> continue; >> } >> - if (addr > rom->addr) { >> + if ((addr > rom->addr) && (as == rom->as)) { >> fprintf(stderr, "rom: requested regions overlap " >> "(rom %s. free=0x" TARGET_FMT_plx >> ", addr=0x" TARGET_FMT_plx ")\n", >> @@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void) >> section = memory_region_find(get_system_memory(), rom->addr, 1); > > (An unrelated bug but I've just noticed that this code doesn't > make sense for ROMs which go into a memory region rather than > at an address in the system address space.) I'll add a patch to fix this as well. > >> rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr); >> memory_region_unref(section.mr); >> + as = rom->as; >> } > > I don't think this attempt at checking is going to actually > catch all the overlap cases if you allow multiple address > spaces. The rom_insert() code orders roms in this > list purely by load address, so you could get cases like > [AS A, 0..0x1000], [AS B, 0..0x1000], [AS A, 0x500..0x1000] > where entries 1 and 3 overlap but won't get caught. > You probably need to order the list by AS first and then > by address, and then adjust this code accordingly. I have changed the ordering now to be by AddressSpace and load address. Thanks, Alistair > >> qemu_register_reset(rom_reset, NULL); >> roms_loaded = 1; >> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h >> index db70c11..1339677 100644 >> --- a/include/hw/elf_ops.h >> +++ b/include/hw/elf_ops.h >> @@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, >> snprintf(label, sizeof(label), "phdr #%d: %s", i, name); >> >> /* rom_add_elf_program() seize the ownership of 'data' */ >> - rom_add_elf_program(label, data, file_size, mem_size, addr); >> + rom_add_elf_program(label, data, file_size, mem_size, addr, NULL); >> >> total_size += mem_size; >> if (addr < low) >> diff --git a/include/hw/loader.h b/include/hw/loader.h >> index 4879b63..18eb0f2 100644 >> --- a/include/hw/loader.h >> +++ b/include/hw/loader.h >> @@ -118,14 +118,14 @@ extern bool rom_file_has_mr; >> >> int rom_add_file(const char *file, const char *fw_dir, >> hwaddr addr, int32_t bootindex, >> - bool option_rom, MemoryRegion *mr); >> + bool option_rom, MemoryRegion *mr, AddressSpace *as); >> MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, >> size_t max_len, hwaddr addr, >> const char *fw_file_name, >> FWCfgReadCallback fw_callback, >> void *callback_opaque); >> int rom_add_elf_program(const char *name, void *data, size_t datasize, >> - size_t romsize, hwaddr addr); >> + size_t romsize, hwaddr addr, AddressSpace *as); >> int rom_check_and_register_reset(void); >> void rom_set_fw(FWCfgState *f); >> void rom_set_order_override(int order); >> @@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr); >> void hmp_info_roms(Monitor *mon, const QDict *qdict); >> >> #define rom_add_file_fixed(_f, _a, _i) \ >> - rom_add_file(_f, NULL, _a, _i, false, NULL) >> + rom_add_file(_f, NULL, _a, _i, false, NULL, NULL) >> #define rom_add_blob_fixed(_f, _b, _l, _a) \ >> rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL) >> #define rom_add_file_mr(_f, _mr, _i) \ >> - rom_add_file(_f, NULL, 0, _i, false, _mr) >> + rom_add_file(_f, NULL, 0, _i, false, _mr, NULL) >> +#define rom_add_file_as(_f, _as, _i) \ >> + rom_add_file(_f, NULL, 0, _i, false, NULL, _as) >> >> #define PC_ROM_MIN_VGA 0xc0000 >> #define PC_ROM_MIN_OPTION 0xc8000 >> -- >> 2.7.4 >> > > thanks > -- PMM >
diff --git a/hw/core/loader.c b/hw/core/loader.c index 53e0e41..fcbcfbf 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -777,6 +777,7 @@ struct Rom { uint8_t *data; MemoryRegion *mr; + AddressSpace *as; int isrom; char *fw_dir; char *fw_file; @@ -833,7 +834,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name) int rom_add_file(const char *file, const char *fw_dir, hwaddr addr, int32_t bootindex, - bool option_rom, MemoryRegion *mr) + bool option_rom, MemoryRegion *mr, + AddressSpace *as) { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; @@ -969,7 +971,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, * memory ownership of "data", so we don't have to allocate and copy the buffer. */ int rom_add_elf_program(const char *name, void *data, size_t datasize, - size_t romsize, hwaddr addr) + size_t romsize, hwaddr addr, AddressSpace *as) { Rom *rom; @@ -979,18 +981,19 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, rom->datasize = datasize; rom->romsize = romsize; rom->data = data; + rom->as = as; rom_insert(rom); return 0; } int rom_add_vga(const char *file) { - return rom_add_file(file, "vgaroms", 0, -1, true, NULL); + return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL); } int rom_add_option(const char *file, int32_t bootindex) { - return rom_add_file(file, "genroms", 0, bootindex, true, NULL); + return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL); } static void rom_reset(void *unused) @@ -1008,7 +1011,8 @@ static void rom_reset(void *unused) void *host = memory_region_get_ram_ptr(rom->mr); memcpy(host, rom->data, rom->datasize); } else { - cpu_physical_memory_write_rom(&address_space_memory, + cpu_physical_memory_write_rom(rom->as ? rom->as : + &address_space_memory, rom->addr, rom->data, rom->datasize); } if (rom->isrom) { @@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void) hwaddr addr = 0; MemoryRegionSection section; Rom *rom; + AddressSpace *as = NULL; QTAILQ_FOREACH(rom, &roms, next) { if (rom->fw_file) { continue; } - if (addr > rom->addr) { + if ((addr > rom->addr) && (as == rom->as)) { fprintf(stderr, "rom: requested regions overlap " "(rom %s. free=0x" TARGET_FMT_plx ", addr=0x" TARGET_FMT_plx ")\n", @@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void) section = memory_region_find(get_system_memory(), rom->addr, 1); rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr); memory_region_unref(section.mr); + as = rom->as; } qemu_register_reset(rom_reset, NULL); roms_loaded = 1; diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index db70c11..1339677 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -405,7 +405,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, snprintf(label, sizeof(label), "phdr #%d: %s", i, name); /* rom_add_elf_program() seize the ownership of 'data' */ - rom_add_elf_program(label, data, file_size, mem_size, addr); + rom_add_elf_program(label, data, file_size, mem_size, addr, NULL); total_size += mem_size; if (addr < low) diff --git a/include/hw/loader.h b/include/hw/loader.h index 4879b63..18eb0f2 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -118,14 +118,14 @@ extern bool rom_file_has_mr; int rom_add_file(const char *file, const char *fw_dir, hwaddr addr, int32_t bootindex, - bool option_rom, MemoryRegion *mr); + bool option_rom, MemoryRegion *mr, AddressSpace *as); MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, size_t max_len, hwaddr addr, const char *fw_file_name, FWCfgReadCallback fw_callback, void *callback_opaque); int rom_add_elf_program(const char *name, void *data, size_t datasize, - size_t romsize, hwaddr addr); + size_t romsize, hwaddr addr, AddressSpace *as); int rom_check_and_register_reset(void); void rom_set_fw(FWCfgState *f); void rom_set_order_override(int order); @@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr); void hmp_info_roms(Monitor *mon, const QDict *qdict); #define rom_add_file_fixed(_f, _a, _i) \ - rom_add_file(_f, NULL, _a, _i, false, NULL) + rom_add_file(_f, NULL, _a, _i, false, NULL, NULL) #define rom_add_blob_fixed(_f, _b, _l, _a) \ rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL) #define rom_add_file_mr(_f, _mr, _i) \ - rom_add_file(_f, NULL, 0, _i, false, _mr) + rom_add_file(_f, NULL, 0, _i, false, _mr, NULL) +#define rom_add_file_as(_f, _as, _i) \ + rom_add_file(_f, NULL, 0, _i, false, NULL, _as) #define PC_ROM_MIN_VGA 0xc0000 #define PC_ROM_MIN_OPTION 0xc8000
When loading ROMs allow the caller to specify an AddressSpace to use for the load. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- V8: - Introduce an RFC version of AddressSpace loading support hw/core/loader.c | 18 ++++++++++++------ include/hw/elf_ops.h | 2 +- include/hw/loader.h | 10 ++++++---- 3 files changed, 19 insertions(+), 11 deletions(-)