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?
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
>>
>>
>>
>
>
