On Tue, Oct 18, 2011 at 21:17, Jordan Justen <jljus...@gmail.com> wrote: > On Tue, Oct 18, 2011 at 11:05, Blue Swirl <blauwir...@gmail.com> wrote: >> On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen >> <jordan.l.jus...@intel.com> wrote: >>> rom_add_file_buf is similar to rom_add_file, except the rom's >>> contents are provided in a buffer. >>> >>> rom_add_file is modified to call rom_add_file_buf after >>> reading the rom's contents from the file. >>> >>> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> >>> --- >>> hw/loader.c | 71 >>> +++++++++++++++++++++++++++++++++++++++------------------- >>> hw/loader.h | 5 ++++ >>> 2 files changed, 53 insertions(+), 23 deletions(-) >>> >>> diff --git a/hw/loader.c b/hw/loader.c >>> index 5676c18..d1a4a98 100644 >>> --- a/hw/loader.c >>> +++ b/hw/loader.c >>> @@ -557,11 +557,11 @@ static void rom_insert(Rom *rom) >>> QTAILQ_INSERT_TAIL(&roms, rom, next); >>> } >>> >>> -int rom_add_file(const char *file, const char *fw_dir, >>> - target_phys_addr_t addr, int32_t bootindex) >>> +int rom_add_file_buf(const char *file, const void *data, size_t size, >>> + const char *fw_dir, >>> + target_phys_addr_t addr, int32_t bootindex) >>> { >>> Rom *rom; >>> - int rc, fd = -1; >>> char devpath[100]; >>> >>> rom = g_malloc0(sizeof(*rom)); >>> @@ -571,28 +571,16 @@ int rom_add_file(const char *file, const char *fw_dir, >>> rom->path = g_strdup(file); >>> } >>> >>> - fd = open(rom->path, O_RDONLY | O_BINARY); >>> - if (fd == -1) { >>> - fprintf(stderr, "Could not open option rom '%s': %s\n", >>> - rom->path, strerror(errno)); >>> - goto err; >>> - } >>> - >>> if (fw_dir) { >>> rom->fw_dir = g_strdup(fw_dir); >>> rom->fw_file = g_strdup(file); >>> } >>> rom->addr = addr; >>> - rom->romsize = lseek(fd, 0, SEEK_END); >>> + rom->romsize = size; >>> rom->data = g_malloc0(rom->romsize); >>> - lseek(fd, 0, SEEK_SET); >>> - rc = read(fd, rom->data, rom->romsize); >>> - if (rc != rom->romsize) { >>> - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected >>> %zd)\n", >>> - rom->name, rc, rom->romsize); >>> - goto err; >>> - } >>> - close(fd); >>> + >>> + memcpy(rom->data, data, rom->romsize); >> >> This is not optimal, instead the data should be used directly. That >> way also mmap()ed, deduplicated ROM files are possible. > > In my 4th patch I use a buffer from a memory region via > memory_region_get_ram_ptr. Comments for memory_region_get_ram_ptr say > 'Use with care'. > > So, would the best thing be for me to allocate a new buffer in my 4th > patch, do the memcpy there, and then use the pointer directly here?
No, instead of memcpy just do rom->data = data; Then also the corresponding g_free(data) below should be removed. The line g_free(rom->data) in the error path would be a problem for the future mmap() case though. Should be solvable with with some refactoring then, we'd need to be able to munmap() anyway. > Thanks, > > -Jordan > >> >>> + >>> rom_insert(rom); >>> if (rom->fw_file && fw_cfg) { >>> const char *basename; >>> @@ -614,14 +602,51 @@ int rom_add_file(const char *file, const char *fw_dir, >>> >>> add_boot_device_path(bootindex, NULL, devpath); >>> return 0; >>> +} >>> + >>> +int rom_add_file(const char *file, const char *fw_dir, >>> + target_phys_addr_t addr, int32_t bootindex) >>> +{ >>> + char *filename; >>> + void *data = NULL; >>> + size_t size; >>> + int rc, fd = -1; >>> + >>> + filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file); >>> + if (filename == NULL) { >>> + filename = g_strdup(file); >>> + } >>> + >>> + fd = open(filename, O_RDONLY | O_BINARY); >>> + if (fd == -1) { >>> + fprintf(stderr, "Could not open option rom '%s': %s\n", >>> + filename, strerror(errno)); >>> + goto err; >>> + } >>> + >>> + size = lseek(fd, 0, SEEK_END); >>> + data = g_malloc0(size); >>> + lseek(fd, 0, SEEK_SET); >>> + rc = read(fd, data, size); >> >> It should be easy to replace this with mmap(), maybe later. >> >>> + if (rc != size) { >>> + fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected >>> %zd)\n", >>> + filename, rc, size); >>> + goto err; >>> + } >>> + close(fd); >>> + >>> + rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex); >>> + if (rc != 0) { >>> + goto err; >>> + } >>> + >>> + g_free(data); >>> + return 0; >>> >>> err: >>> if (fd != -1) >>> close(fd); >>> - g_free(rom->data); >>> - g_free(rom->path); >>> - g_free(rom->name); >>> - g_free(rom); >>> + g_free(data); >>> return -1; >>> } >>> >>> diff --git a/hw/loader.h b/hw/loader.h >>> index fc6bdff..9efe64a 100644 >>> --- a/hw/loader.h >>> +++ b/hw/loader.h >>> @@ -21,6 +21,9 @@ void pstrcpy_targphys(const char *name, >>> const char *source); >>> >>> >>> +int rom_add_file_buf(const char *file, const void *data, size_t size, >>> + const char *fw_dir, >>> + target_phys_addr_t addr, int32_t bootindex); >>> int rom_add_file(const char *file, const char *fw_dir, >>> target_phys_addr_t addr, int32_t bootindex); >>> int rom_add_blob(const char *name, const void *blob, size_t len, >>> @@ -31,6 +34,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, >>> size_t size); >>> void *rom_ptr(target_phys_addr_t addr); >>> void do_info_roms(Monitor *mon); >>> >>> +#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i) \ >>> + rom_add_file_buf(_f, _d, _s, NULL, _a, _i) >>> #define rom_add_file_fixed(_f, _a, _i) \ >>> rom_add_file(_f, NULL, _a, _i) >>> #define rom_add_blob_fixed(_f, _b, _l, _a) \ >>> -- >>> 1.7.1 >>> >>> >>> >> >> >