Re: [Qemu-devel] [PATCH 3/4] loader: Add rom_add_file_buf for adding roms from a buffer

[Jordan Justen]

On Tue, Oct 18, 2011 at 11:05, Blue Swirl <address@hidden> wrote:
> On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen
> <address@hidden> 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 <address@hidden>
>> ---
>>  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
>>
>>
>>
>
>