[SeaBIOS] [PATCH] QEMU DMA: Add ability to write back to fw_cfg file
Laszlo Ersek
lersek at redhat.com
Fri Jan 20 17:39:58 CET 2017
On 01/20/17 17:08, Kevin O'Connor wrote:
> On Thu, Jan 19, 2017 at 10:20:50PM -0800, ben at skyportsystems.com wrote:
>> From: Ben Warren <ben at skyportsystems.com>
>>
>> This adds a new command to the DMA protocol. The command allows the
>> memory allocation of a fw_cfg file by BIOS and subsequent return of
>> the allocated address to QEMU. The initial use case is for Windows VM
>> Generation ID, where QEMU needs to change the contents of fw_cfg
>> data at runtime, while still having BIOS allocate and manage the memory.
>
> Thanks - see my comments below.
>
> [...]
>> --- a/src/fw/paravirt.c
>> +++ b/src/fw/paravirt.c
>> @@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
>> }
>>
>> static void
>> +qemu_cfg_write(void *buf, int len)
>> +{
>> + if (len == 0) {
>> + return;
>> + }
>> +
>> + if (qemu_cfg_dma_enabled()) {
>> + qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
>> + } else {
>> + outsb(PORT_QEMU_CFG_DATA, buf, len);
>> + }
>> +}
>> +
>> +static void
>> qemu_cfg_skip(int len)
>> {
>> if (len == 0) {
>> @@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
>> }
>> }
>>
>> +static void
>> +qemu_cfg_write_entry(void *buf, int e, int len)
>> +{
>> + if (qemu_cfg_dma_enabled()) {
>> + u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
>> + | QEMU_CFG_DMA_CTL_WRITE;
>> + qemu_cfg_dma_transfer(buf, len, control);
>> + } else {
>> + qemu_cfg_select(e);
>> + qemu_cfg_write(buf, len);
>> + }
>> +}
>> +
>> struct qemu_romfile_s {
>> struct romfile_s file;
>> int select, skip;
>> @@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
>> return file->size;
>> }
>>
>> +static int
>> +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
>> +{
>> + if (file->size < maxlen)
>> + return -1;
>> + struct qemu_romfile_s *qfile;
>> + qfile = container_of(file, struct qemu_romfile_s, file);
>> + if (qfile->skip == 0) {
>> + /* Do it in one transfer */
>> + qemu_cfg_write_entry(src, qfile->select, file->size);
>> + } else {
>> + qemu_cfg_select(qfile->select);
>> + qemu_cfg_skip(qfile->skip);
>> + qemu_cfg_write(src, file->size);
>> + }
>> + return file->size;
>> +}
>
> I'd prefer if we could break this patch into two parts - one part to
> add the write interface to fw_cfg and one part to add
> ALLOCATE_RET_ADDR. I'm fine with both interfaces, but the QEMU parts
> still need to be committed prior to committing to SeaBIOS. Having the
> patch in separate parts will make it both easier to review and commit
> at their appropriate times.
The fw_cfg write support is now upstream (QEMU commit
baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
blobs").
Thanks
Laszlo
>
>> +
>> static void
>> qemu_romfile_add(char *name, int select, int skip, int size)
>> {
>> @@ -317,6 +362,7 @@ qemu_romfile_add(char *name, int select, int skip, int size)
>> qfile->select = select;
>> qfile->skip = skip;
>> qfile->file.copy = qemu_cfg_read_file;
>> + qfile->file.write_back = qemu_cfg_write_file;
>> romfile_add(&qfile->file);
>> }
>
> I don't think it is worthwhile to extend the romfile mechanism with a
> write_back() method. Instead, I'd export qemu_cfg_write_file(),
> extend it to verify the passed in romfile is a fw_cfg file (file->copy
> == qemu_cfg_read_file), and change the callers to call it directly.
>
>>
>> diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h
>> index d8eb7c4..479eaf9 100644
>> --- a/src/fw/paravirt.h
>> +++ b/src/fw/paravirt.h
>> @@ -43,6 +43,7 @@ static inline int runningOnKVM(void) {
>> #define QEMU_CFG_DMA_CTL_READ 0x02
>> #define QEMU_CFG_DMA_CTL_SKIP 0x04
>> #define QEMU_CFG_DMA_CTL_SELECT 0x08
>> +#define QEMU_CFG_DMA_CTL_WRITE 0x10
>>
>> // QEMU_CFG_DMA ID bit
>> #define QEMU_CFG_VERSION_DMA 2
>> diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c
>> index f4b17ff..945572f 100644
>> --- a/src/fw/romfile_loader.c
>> +++ b/src/fw/romfile_loader.c
>> @@ -127,6 +127,38 @@ err:
>> warn_internalerror();
>> }
>>
>> +static void romfile_loader_return_addr(struct romfile_loader_entry_s *entry,
>> + struct romfile_loader_files *files)
>> +{
>> + struct romfile_loader_file *alloc_file;
>> + struct romfile_loader_file *addr_file;
>> + u64 addr;
>> + int ret;
>> +
>> + alloc_file = romfile_loader_find(entry->alloc_ret_file, files);
>> + addr_file = romfile_loader_find(entry->alloc_ret_addr_file, files);
>> +
>> + if (!alloc_file || !addr_file || !alloc_file->data || !addr_file->data ||
>> + addr_file->file->size < sizeof(addr))
>> + goto err;
>> +
>> + /* Get the address of the just-allocated file
>> + * and stuff it in the address file */
>> + memcpy(&addr, &alloc_file->data, sizeof(addr));
>> + addr = cpu_to_le64(addr);
>> + memcpy(addr_file->data, &addr, sizeof(addr));
>> +
>> + if (!addr_file->file->write_back)
>> + goto err;
>> +
>> + ret = addr_file->file->write_back(&addr, addr_file->file, sizeof(addr));
>> + if (ret != sizeof(addr))
>> + goto err;
>> + return;
>> +err:
>> + warn_internalerror();
>> +}
>> +
>> int romfile_loader_execute(const char *name)
>> {
>> struct romfile_loader_entry_s *entry;
>> @@ -161,6 +193,11 @@ int romfile_loader_execute(const char *name)
>> break;
>> case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
>> romfile_loader_add_checksum(entry, files);
>> + break;
>> + case ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR:
>> + romfile_loader_allocate(entry, files);
>> + romfile_loader_return_addr(entry, files);
>> + break;
>> default:
>> /* Skip commands that we don't recognize. */
>> break;
>> diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h
>> index 15eab2a..ea161e8 100644
>> --- a/src/fw/romfile_loader.h
>> +++ b/src/fw/romfile_loader.h
>> @@ -51,15 +51,32 @@ struct romfile_loader_entry_s {
>> u32 cksum_length;
>> };
>>
>> + /*
>> + * COMMAND_ALLOCATE_RETURN_ADDR - allocate a table from @alloc_file
>> + * subject to @alloc_ret_align alignment (must be power of 2)
>> + * and @alloc_ret_zone (can be HIGH or FSEG) requirements.
>> + * Additionally, return the address of the allocation in
>> + * @addr_file.
>> + *
>> + * This may be used instead of COMMAND_ALLOCATE
>
> Minor nit - a tab snuck in here.
>
>> + */
>> + struct {
>> + char alloc_ret_file[ROMFILE_LOADER_FILESZ];
>> + u32 alloc_ret_align;
>> + u8 alloc_ret_zone;
>> + char alloc_ret_addr_file[ROMFILE_LOADER_FILESZ];
>> + };
>> +
>> /* padding */
>> char pad[124];
>> };
>> };
>>
>> enum {
>> - ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
>> - ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
>> - ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + ROMFILE_LOADER_COMMAND_ALLOCATE = 0x1,
>> + ROMFILE_LOADER_COMMAND_ADD_POINTER = 0x2,
>> + ROMFILE_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + ROMFILE_LOADER_COMMAND_ALLOCATE_RET_ADDR = 0x4,
>> };
>>
>> enum {
>> diff --git a/src/romfile.h b/src/romfile.h
>> index c6d62a1..8694cc1 100644
>> --- a/src/romfile.h
>> +++ b/src/romfile.h
>> @@ -9,6 +9,7 @@ struct romfile_s {
>> char name[128];
>> u32 size;
>> int (*copy)(struct romfile_s *file, void *dest, u32 maxlen);
>> + int (*write_back)(void *src, struct romfile_s *file, u32 maxlen);
>> };
>> void romfile_add(struct romfile_s *file);
>> struct romfile_s *romfile_findprefix(const char *prefix, struct romfile_s *prev);
>
> -Kevin
>
More information about the SeaBIOS
mailing list