qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/4] loader: Add rom_add_file_buf for adding rom


From: Jordan Justen
Subject: Re: [Qemu-devel] [PATCH 3/4] loader: Add rom_add_file_buf for adding roms from a buffer
Date: Mon, 24 Oct 2011 15:19:51 -0700

On Sun, Oct 23, 2011 at 04:27, Blue Swirl <address@hidden> wrote:
> 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.

I was discussing this change with Alex, and his opinion was that I
should not need to add the rom_add_file_buf function because the
pflash device is being used.  So, I plan to drop patches 3 & 4 from
this changeset.

Thanks for the suggestion though, and I'll keep it in mind for future changes.

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]