[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when l
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs |
Date: |
Wed, 13 Jul 2016 10:14:32 -0700 |
On Tue, Jul 12, 2016 at 9:56 AM, Peter Maydell <address@hidden> wrote:
> On 2 July 2016 at 02:07, Alistair Francis <address@hidden> wrote:
>> When loading ROMs allow the caller to specify an AddressSpace to use for
>> the load.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> 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
>
- [Qemu-devel] [PATCH v8 0/5] Add a generic loader, Alistair Francis, 2016/07/01
- [Qemu-devel] [PATCH v8 1/5] loader: Allow ELF loader to auto-detect the ELF arch, Alistair Francis, 2016/07/01
- [Qemu-devel] [PATCH v8 3/5] loader: Add AddressSpace loading support to ELFs, Alistair Francis, 2016/07/01
- [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs, Alistair Francis, 2016/07/01
- [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Alistair Francis, 2016/07/01
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Peter Maydell, 2016/07/12
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Alistair Francis, 2016/07/13
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Peter Maydell, 2016/07/13
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Alistair Francis, 2016/07/13
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Peter Maydell, 2016/07/13
- Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader, Alistair Francis, 2016/07/13
[Qemu-devel] [PATCH v8 5/5] docs: Add a generic loader explanation document, Alistair Francis, 2016/07/01