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

[Blue Swirl]

On Tue, Oct 18, 2011 at 21:17, Jordan Justen <address@hidden> wrote:
> 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?

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