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