SeaBIOS

seabios@seabios.org

3783 discussions

Re: [SeaBIOS] [PATCH v5 4/5] QEMU fw_cfg: Add functions for accessing files by key
by Igor Mammedov 21 Feb '17

21 Feb '17

On Fri, 17 Feb 2017 22:21:58 -0800 ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > When resuming from S3, only fw_cfg file keys are known. maybe something like this: When resuming from S3 due to memory constrains fw_cfg files API isn't available, so add simple API to get file 'key' and write to file by using saved at boot time 'key'. + with Laszlo's suggestions on code consolidation Reviewed-by: Igor Mammedov <imammedo(a)redhat.com> > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/paravirt.c | 23 +++++++++++++++++++++++ > src/fw/paravirt.h | 2 ++ > 2 files changed, 25 insertions(+) > > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c > index 4618647..e513dd5 100644 > --- a/src/fw/paravirt.c > +++ b/src/fw/paravirt.c > @@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len) > return len; > } > > +// Bare-bones function for writing a file knowing only its unique > +// identifying key (select) > +int > +qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len) > +{ > + qemu_cfg_select(key); > + qemu_cfg_skip(offset); > + qemu_cfg_write(src, len); > + return len; > +} > + > static void > qemu_romfile_add(char *name, int select, int skip, int size) > { > @@ -370,6 +381,18 @@ qemu_romfile_add(char *name, int select, int skip, int size) > } > > u16 > +qemu_get_romfile_key(struct romfile_s *file) > +{ > + struct qemu_romfile_s *qfile; > + qfile = container_of(file, struct qemu_romfile_s, file); > + if (!qfile) { > + warn_internalerror(); > + return 0; > + } > + return qfile->select; > +} > + > +u16 > qemu_get_present_cpus_count(void) > { > u16 smp_count = 0; > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h > index fb220d8..16f3d9a 100644 > --- a/src/fw/paravirt.h > +++ b/src/fw/paravirt.h > @@ -56,5 +56,7 @@ void qemu_cfg_init(void); > > u16 qemu_get_present_cpus_count(void); > int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len); > +int qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len); > +u16 qemu_get_romfile_key(struct romfile_s *file); > > #endif

1 0

Re: [SeaBIOS] [PATCH v5 3/5] QEMU fw_cfg: Add command to write back address of file
by Laszlo Ersek 21 Feb '17

21 Feb '17

On 02/18/17 07:21, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This command is similar to ADD_POINTER, but instead of patching > memory, it writes the pointer back to QEMU over the DMA interface. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/romfile_loader.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > src/fw/romfile_loader.h | 23 ++++++++++++++++++++--- > 2 files changed, 65 insertions(+), 3 deletions(-) Reviewed-by: Laszlo Ersek <lersek(a)redhat.com> Thanks Laszlo > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c > index 7737453..30e7b58 100644 > --- a/src/fw/romfile_loader.c > +++ b/src/fw/romfile_loader.c > @@ -5,6 +5,7 @@ > #include "romfile.h" // struct romfile_s > #include "malloc.h" // Zone*, _malloc > #include "output.h" // warn_* > +#include "paravirt.h" // qemu_cfg_write_file > > struct romfile_loader_file { > struct romfile_s *file; > @@ -127,6 +128,46 @@ err: > warn_internalerror(); > } > > +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry, > + struct romfile_loader_files *files) > +{ > + struct romfile_s *dest_file; > + struct romfile_loader_file *src_file; > + unsigned dst_offset = le32_to_cpu(entry->wr_pointer.dst_offset); > + unsigned src_offset = le32_to_cpu(entry->wr_pointer.src_offset); > + u64 pointer = 0; > + > + /* Writing back to a file that may not be loaded in RAM */ > + dest_file = romfile_find(entry->wr_pointer.dest_file); > + src_file = romfile_loader_find(entry->wr_pointer.src_file, files); > + > + if (!dest_file || !src_file || !src_file->data || > + dst_offset + entry->wr_pointer.size < dst_offset || > + dst_offset + entry->wr_pointer.size > dest_file->size || > + src_offset >= src_file->file->size || > + entry->wr_pointer.size < 1 || entry->wr_pointer.size > 8 || > + entry->wr_pointer.size & (entry->wr_pointer.size - 1)) { > + goto err; > + } > + > + pointer = (unsigned long)src_file->data + src_offset; > + /* Make sure the pointer fits within wr_pointer.size */ > + if ((entry->wr_pointer.size != sizeof(u64)) && > + ((pointer >> (entry->wr_pointer.size * 8)) > 0)) { > + goto err; > + } > + pointer = cpu_to_le64(pointer); > + > + /* Only supported on QEMU */ > + if (qemu_cfg_write_file(&pointer, dest_file, dst_offset, > + entry->wr_pointer.size) != entry->wr_pointer.size) { > + goto err; > + } > + return; > + err: > + warn_internalerror(); > +} > + > int romfile_loader_execute(const char *name) > { > struct romfile_loader_entry_s *entry; > @@ -161,6 +202,10 @@ 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_WRITE_POINTER: > + romfile_loader_write_pointer(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 bce3719..4dc50ab 100644 > --- a/src/fw/romfile_loader.h > +++ b/src/fw/romfile_loader.h > @@ -51,15 +51,32 @@ struct romfile_loader_entry_s { > u32 length; > } cksum; > > + /* > + * COMMAND_WRITE_POINTER - Write back to a host file via DMA, > + * @wr_pointer.dest_file at offset @wr_pointer.dst_offset, a pointer > + * to the table originating from @wr_pointer.src_file at offset > + * @wr_pointer.src_offset. > + * 1,2,4 or 8 byte unsigned addition is used depending on > + * @wr_pointer.size. > + */ > + struct { > + char dest_file[ROMFILE_LOADER_FILESZ]; > + char src_file[ROMFILE_LOADER_FILESZ]; > + u32 dst_offset; > + u32 src_offset; > + u8 size; > + } wr_pointer; > + > /* 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_WRITE_POINTER = 0x4, > }; > > enum { >

2 1

Re: [SeaBIOS] [PATCH v5 2/5] romfile-loader: Switch to using named structs
by Igor Mammedov 21 Feb '17

21 Feb '17

On Fri, 17 Feb 2017 22:21:56 -0800 ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This is a little cleaner and matches how the command structures are > defined in QEMU. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > Reviewed-by: Laszlo Ersek <lersek(a)redhat.com> Reviewed-by: Igor Mammedov <imammedo(a)redhat.com> > --- > src/fw/romfile_loader.c | 34 +++++++++++++++++----------------- > src/fw/romfile_loader.h | 40 ++++++++++++++++++++-------------------- > 2 files changed, 37 insertions(+), 37 deletions(-) > > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c > index f4b17ff..7737453 100644 > --- a/src/fw/romfile_loader.c > +++ b/src/fw/romfile_loader.c > @@ -35,12 +35,12 @@ static void romfile_loader_allocate(struct romfile_loader_entry_s *entry, > struct romfile_loader_file *file = &files->files[files->nfiles]; > void *data; > int ret; > - unsigned alloc_align = le32_to_cpu(entry->alloc_align); > + unsigned alloc_align = le32_to_cpu(entry->alloc.align); > > if (alloc_align & (alloc_align - 1)) > goto err; > > - switch (entry->alloc_zone) { > + switch (entry->alloc.zone) { > case ROMFILE_LOADER_ALLOC_ZONE_HIGH: > zone = &ZoneHigh; > break; > @@ -52,9 +52,9 @@ static void romfile_loader_allocate(struct romfile_loader_entry_s *entry, > } > if (alloc_align < MALLOC_MIN_ALIGN) > alloc_align = MALLOC_MIN_ALIGN; > - if (entry->alloc_file[ROMFILE_LOADER_FILESZ - 1]) > + if (entry->alloc.file[ROMFILE_LOADER_FILESZ - 1]) > goto err; > - file->file = romfile_find(entry->alloc_file); > + file->file = romfile_find(entry->alloc.file); > if (!file->file || !file->file->size) > return; > data = _malloc(zone, file->file->size, alloc_align); > @@ -80,24 +80,24 @@ static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry, > { > struct romfile_loader_file *dest_file; > struct romfile_loader_file *src_file; > - unsigned offset = le32_to_cpu(entry->pointer_offset); > + unsigned offset = le32_to_cpu(entry->pointer.offset); > u64 pointer = 0; > > - dest_file = romfile_loader_find(entry->pointer_dest_file, files); > - src_file = romfile_loader_find(entry->pointer_src_file, files); > + dest_file = romfile_loader_find(entry->pointer.dest_file, files); > + src_file = romfile_loader_find(entry->pointer.src_file, files); > > if (!dest_file || !src_file || !dest_file->data || !src_file->data || > - offset + entry->pointer_size < offset || > - offset + entry->pointer_size > dest_file->file->size || > - entry->pointer_size < 1 || entry->pointer_size > 8 || > - entry->pointer_size & (entry->pointer_size - 1)) > + offset + entry->pointer.size < offset || > + offset + entry->pointer.size > dest_file->file->size || > + entry->pointer.size < 1 || entry->pointer.size > 8 || > + entry->pointer.size & (entry->pointer.size - 1)) > goto err; > > - memcpy(&pointer, dest_file->data + offset, entry->pointer_size); > + memcpy(&pointer, dest_file->data + offset, entry->pointer.size); > pointer = le64_to_cpu(pointer); > pointer += (unsigned long)src_file->data; > pointer = cpu_to_le64(pointer); > - memcpy(dest_file->data + offset, &pointer, entry->pointer_size); > + memcpy(dest_file->data + offset, &pointer, entry->pointer.size); > > return; > err: > @@ -108,12 +108,12 @@ static void romfile_loader_add_checksum(struct romfile_loader_entry_s *entry, > struct romfile_loader_files *files) > { > struct romfile_loader_file *file; > - unsigned offset = le32_to_cpu(entry->cksum_offset); > - unsigned start = le32_to_cpu(entry->cksum_start); > - unsigned len = le32_to_cpu(entry->cksum_length); > + unsigned offset = le32_to_cpu(entry->cksum.offset); > + unsigned start = le32_to_cpu(entry->cksum.start); > + unsigned len = le32_to_cpu(entry->cksum.length); > u8 *data; > > - file = romfile_loader_find(entry->cksum_file, files); > + file = romfile_loader_find(entry->cksum.file, files); > > if (!file || !file->data || offset >= file->file->size || > start + len < start || start + len > file->file->size) > diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h > index 15eab2a..bce3719 100644 > --- a/src/fw/romfile_loader.h > +++ b/src/fw/romfile_loader.h > @@ -11,45 +11,45 @@ struct romfile_loader_entry_s { > u32 command; > union { > /* > - * COMMAND_ALLOCATE - allocate a table from @alloc_file > - * subject to @alloc_align alignment (must be power of 2) > - * and @alloc_zone (can be HIGH or FSEG) requirements. > + * COMMAND_ALLOCATE - allocate a table from @alloc.file > + * subject to @alloc.align alignment (must be power of 2) > + * and @alloc.zone (can be HIGH or FSEG) requirements. > * > * Must appear exactly once for each file, and before > * this file is referenced by any other command. > */ > struct { > - char alloc_file[ROMFILE_LOADER_FILESZ]; > - u32 alloc_align; > - u8 alloc_zone; > - }; > + char file[ROMFILE_LOADER_FILESZ]; > + u32 align; > + u8 zone; > + } alloc; > > /* > * COMMAND_ADD_POINTER - patch the table (originating from > - * @dest_file) at @pointer_offset, by adding a pointer to the table > + * @dest_file) at @pointer.offset, by adding a pointer to the table > * originating from @src_file. 1,2,4 or 8 byte unsigned > - * addition is used depending on @pointer_size. > + * addition is used depending on @pointer.size. > */ > struct { > - char pointer_dest_file[ROMFILE_LOADER_FILESZ]; > - char pointer_src_file[ROMFILE_LOADER_FILESZ]; > - u32 pointer_offset; > - u8 pointer_size; > - }; > + char dest_file[ROMFILE_LOADER_FILESZ]; > + char src_file[ROMFILE_LOADER_FILESZ]; > + u32 offset; > + u8 size; > + } pointer; > > /* > * COMMAND_ADD_CHECKSUM - calculate checksum of the range specified by > - * @cksum_start and @cksum_length fields, > + * @cksum.start and @cksum.length fields, > * and then add the value at @cksum_offset. > * Checksum simply sums -X for each byte X in the range > * using 8-bit math. > */ > struct { > - char cksum_file[ROMFILE_LOADER_FILESZ]; > - u32 cksum_offset; > - u32 cksum_start; > - u32 cksum_length; > - }; > + char file[ROMFILE_LOADER_FILESZ]; > + u32 offset; > + u32 start; > + u32 length; > + } cksum; > > /* padding */ > char pad[124];

1 0

Re: [SeaBIOS] [PATCH v5 1/5] QEMU DMA: Add DMA write capability
by Igor Mammedov 21 Feb '17

21 Feb '17

On Fri, 17 Feb 2017 22:21:55 -0800 ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This allows BIOS to write data back to QEMU using the DMA interface and > provides a higher-level abstraction to write to a fw_cfg file > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> Reviewed-by: Igor Mammedov <imammedo(a)redhat.com> > --- > src/fw/paravirt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > src/fw/paravirt.h | 3 +++ > 2 files changed, 52 insertions(+) > > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c > index 6de70f6..4618647 100644 > --- 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 { > + warn_internalerror(); > + } > +} > + > +static void > qemu_cfg_skip(int len) > { > if (len == 0) { > @@ -280,6 +294,18 @@ 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 { > + warn_internalerror(); > + } > +} > + > struct qemu_romfile_s { > struct romfile_s file; > int select, skip; > @@ -303,6 +329,29 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen) > return file->size; > } > > +int > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len) > +{ > + if ((offset + len) > file->size) > + return -1; > + > + if (!qemu_cfg_dma_enabled() || (file->copy != qemu_cfg_read_file)) { > + warn_internalerror(); > + return -1; > + } > + struct qemu_romfile_s *qfile; > + qfile = container_of(file, struct qemu_romfile_s, file); > + if (offset == 0) { > + /* Do it in one transfer */ > + qemu_cfg_write_entry(src, qfile->select, len); > + } else { > + qemu_cfg_select(qfile->select); > + qemu_cfg_skip(offset); > + qemu_cfg_write(src, len); > + } > + return len; > +} > + > static void > qemu_romfile_add(char *name, int select, int skip, int size) > { > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h > index d8eb7c4..fb220d8 100644 > --- a/src/fw/paravirt.h > +++ b/src/fw/paravirt.h > @@ -3,6 +3,7 @@ > > #include "config.h" // CONFIG_* > #include "biosvar.h" // GET_GLOBAL > +#include "romfile.h" // struct romfile_s > > // Types of paravirtualized platforms. > #define PF_QEMU (1<<0) > @@ -43,6 +44,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 > @@ -53,5 +55,6 @@ void qemu_platform_setup(void); > void qemu_cfg_init(void); > > u16 qemu_get_present_cpus_count(void); > +int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len); > > #endif

1 0

Re: [SeaBIOS] [PATCH v5 5/5] QEMU fw_cfg: Write fw_cfg back on S3 resume
by Laszlo Ersek 20 Feb '17

20 Feb '17

On 02/18/17 07:21, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > Any pointers to BIOS-allocated memory that were written back to QEMU > fw_cfg files are replayed when resuming from S3 sleep. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/romfile_loader.c | 35 +++++++++++++++++++++++++++++++++++ > src/fw/romfile_loader.h | 2 ++ > src/resume.c | 4 ++++ > 3 files changed, 41 insertions(+) > > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c > index 30e7b58..33aaec4 100644 > --- a/src/fw/romfile_loader.c > +++ b/src/fw/romfile_loader.c > @@ -4,6 +4,7 @@ > #include "string.h" // strcmp > #include "romfile.h" // struct romfile_s > #include "malloc.h" // Zone*, _malloc > +#include "list.h" // struct hlist_node > #include "output.h" // warn_* > #include "paravirt.h" // qemu_cfg_write_file > > @@ -16,6 +17,16 @@ struct romfile_loader_files { > struct romfile_loader_file files[]; > }; > > +// Data structures for storing "write pointer" entries for possible replay > +struct romfile_wr_pointer_entry { > + u64 pointer; > + u32 offset; > + u16 key; > + u8 ptr_size; > + struct hlist_node node; > +}; > +static struct hlist_head romfile_pointer_list; > + > static struct romfile_loader_file * > romfile_loader_find(const char *name, > struct romfile_loader_files *files) > @@ -29,6 +40,16 @@ romfile_loader_find(const char *name, > return NULL; > } > > +// Replay "write pointer" entries back to QEMU > +void romfile_fw_cfg_resume(void) > +{ > + struct romfile_wr_pointer_entry *entry; > + hlist_for_each_entry(entry, &romfile_pointer_list, node) { > + qemu_cfg_write_file_simple(&entry->pointer, entry->key, > + entry->offset, entry->ptr_size); > + } > +} > + > static void romfile_loader_allocate(struct romfile_loader_entry_s *entry, > struct romfile_loader_files *files) > { > @@ -163,6 +184,20 @@ static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry, > entry->wr_pointer.size) != entry->wr_pointer.size) { > goto err; > } > + > + /* Store the info so it can replayed later if necessary */ > + struct romfile_wr_pointer_entry *store = _malloc(&ZoneHigh, > + sizeof(*store), 4); I don't know enough of the SeaBIOS memory allocation system to know if this is safe. I assume this will place the allocation in reserved memory. > + struct hlist_node **pprev = &romfile_pointer_list.first; > + store->pointer = pointer; > + store->key = qemu_get_romfile_key(dest_file); > + if (store->key == 0) { > + goto err; > + } Based on my comment on container_of() in patch #4, qemu_get_romfile_key() shouldn't be able to fail, so this check should be unnecessary. > + store->offset = dst_offset; > + store->ptr_size = entry->wr_pointer.size; > + hlist_add(&store->node, pprev); I think the code can be simplified a bit, by calling hlist_add_head() instead (you can drop the local "pprev" variable then): hlist_add_head(&store->node, &romfile_pointer_list); > + > return; > err: > warn_internalerror(); > diff --git a/src/fw/romfile_loader.h b/src/fw/romfile_loader.h > index 4dc50ab..fcd4ab2 100644 > --- a/src/fw/romfile_loader.h > +++ b/src/fw/romfile_loader.h > @@ -86,4 +86,6 @@ enum { > > int romfile_loader_execute(const char *name); > > +void romfile_fw_cfg_resume(void); > + > #endif > diff --git a/src/resume.c b/src/resume.c > index e67cfce..99fa34f 100644 > --- a/src/resume.c > +++ b/src/resume.c > @@ -17,6 +17,7 @@ > #include "string.h" // memset > #include "util.h" // dma_setup > #include "tcgbios.h" // tpm_s3_resume > +#include "fw/romfile_loader.h" // romfile_fw_cfg_resume > > // Handler for post calls that look like a resume. > void VISIBLE16 > @@ -105,6 +106,9 @@ s3_resume(void) > tpm_s3_resume(); > s3_resume_vga(); > > + /* Replay any fw_cfg entries that go back to the host */ > + romfile_fw_cfg_resume(); > + The functionality in "romfile_loader.c" is conditional on CONFIG_QEMU: qemu_platform_setup() [src/fw/paravirt.c] // returns immediately if !CONFIG_QEMU romfile_loader_execute() [src/fw/romfile_loader.c] So we shouldn't make the replay unconditional either. Although "romfile_pointer_list" will be empty, if romfile_loader_execute() never runs, we should also save on code size (-> build time optimization) if CONFIG_QEMU is false. Please add the check if (!CONFIG_QEMU) return; to the top of romfile_fw_cfg_resume(), similarly to the example in smp_resume() [src/fw/smp.c], which is also called from s3_resume(). (Comments from others are most welcome, of course; this is just how I see things.) Thanks! Laszlo > make_bios_readonly(); > > // Invoke the resume vector. >

1 0

Re: [SeaBIOS] [PATCH v5 4/5] QEMU fw_cfg: Add functions for accessing files by key
by Laszlo Ersek 20 Feb '17

20 Feb '17

On 02/18/17 07:21, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > When resuming from S3, only fw_cfg file keys are known. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/paravirt.c | 23 +++++++++++++++++++++++ > src/fw/paravirt.h | 2 ++ > 2 files changed, 25 insertions(+) > > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c > index 4618647..e513dd5 100644 > --- a/src/fw/paravirt.c > +++ b/src/fw/paravirt.c > @@ -352,6 +352,17 @@ qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len) > return len; > } > > +// Bare-bones function for writing a file knowing only its unique > +// identifying key (select) > +int > +qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len) > +{ > + qemu_cfg_select(key); > + qemu_cfg_skip(offset); > + qemu_cfg_write(src, len); > + return len; > +} Does anything counter-indicate the customization seen in qemu_cfg_write_file() as well, that is, call qemu_cfg_write_entry() if offset is zero? If not, then I think you could even split out that part of qemu_cfg_write_file() as qemu_cfg_write_file_simple(), and call it from qemu_cfg_write_file(). > + > static void > qemu_romfile_add(char *name, int select, int skip, int size) > { > @@ -370,6 +381,18 @@ qemu_romfile_add(char *name, int select, int skip, int size) > } > > u16 > +qemu_get_romfile_key(struct romfile_s *file) > +{ > + struct qemu_romfile_s *qfile; > + qfile = container_of(file, struct qemu_romfile_s, file); If the input pointer "file" was valid, then container_of() cannot produce a NULL pointer. So I suggest to drop the code that depends on that. > + if (!qfile) { > + warn_internalerror(); > + return 0; > + } > + return qfile->select; > +} This could be reused in qemu_cfg_write_file() too, so that that function would remain: - initial checks - qemu_cfg_write_file_simple( ..., qemu_get_romfile_key(), ...) I think patch #1 is fine as is, so I'd keep this refactoring in patch #4. Thanks Laszlo > + > +u16 > qemu_get_present_cpus_count(void) > { > u16 smp_count = 0; > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h > index fb220d8..16f3d9a 100644 > --- a/src/fw/paravirt.h > +++ b/src/fw/paravirt.h > @@ -56,5 +56,7 @@ void qemu_cfg_init(void); > > u16 qemu_get_present_cpus_count(void); > int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len); > +int qemu_cfg_write_file_simple(void *src, u16 key, u32 offset, u32 len); > +u16 qemu_get_romfile_key(struct romfile_s *file); > > #endif >

1 0

Re: [SeaBIOS] [PATCH v5 1/5] QEMU DMA: Add DMA write capability
by Laszlo Ersek 20 Feb '17

20 Feb '17

On 02/18/17 07:21, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This allows BIOS to write data back to QEMU using the DMA interface and > provides a higher-level abstraction to write to a fw_cfg file > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/paravirt.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > src/fw/paravirt.h | 3 +++ > 2 files changed, 52 insertions(+) Reviewed-by: Laszlo Ersek <lersek(a)redhat.com> Thanks Laszlo > diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c > index 6de70f6..4618647 100644 > --- 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 { > + warn_internalerror(); > + } > +} > + > +static void > qemu_cfg_skip(int len) > { > if (len == 0) { > @@ -280,6 +294,18 @@ 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 { > + warn_internalerror(); > + } > +} > + > struct qemu_romfile_s { > struct romfile_s file; > int select, skip; > @@ -303,6 +329,29 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen) > return file->size; > } > > +int > +qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len) > +{ > + if ((offset + len) > file->size) > + return -1; > + > + if (!qemu_cfg_dma_enabled() || (file->copy != qemu_cfg_read_file)) { > + warn_internalerror(); > + return -1; > + } > + struct qemu_romfile_s *qfile; > + qfile = container_of(file, struct qemu_romfile_s, file); > + if (offset == 0) { > + /* Do it in one transfer */ > + qemu_cfg_write_entry(src, qfile->select, len); > + } else { > + qemu_cfg_select(qfile->select); > + qemu_cfg_skip(offset); > + qemu_cfg_write(src, len); > + } > + return len; > +} > + > static void > qemu_romfile_add(char *name, int select, int skip, int size) > { > diff --git a/src/fw/paravirt.h b/src/fw/paravirt.h > index d8eb7c4..fb220d8 100644 > --- a/src/fw/paravirt.h > +++ b/src/fw/paravirt.h > @@ -3,6 +3,7 @@ > > #include "config.h" // CONFIG_* > #include "biosvar.h" // GET_GLOBAL > +#include "romfile.h" // struct romfile_s > > // Types of paravirtualized platforms. > #define PF_QEMU (1<<0) > @@ -43,6 +44,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 > @@ -53,5 +55,6 @@ void qemu_platform_setup(void); > void qemu_cfg_init(void); > > u16 qemu_get_present_cpus_count(void); > +int qemu_cfg_write_file(void *src, struct romfile_s *file, u32 offset, u32 len); > > #endif >

1 0

[PATCH v5 0/5] QEMU: Add capability to write back fw_cfg address to QEMU
by ben＠skyportsystems.com 18 Feb '17

18 Feb '17

From: Ben Warren <ben(a)skyportsystems.com> This patch set adds the capability to write to QEMU across the fw_cfg DMA link. It adds a higher-level command to write the BIOS-allocated address of one fw_cfg file into an arbitrary point within another, writeable fw_cfg file across the above-mentioned link. 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. The final corresponding QEMU patch can be found here: https://www.mail-archive.com/qemu-devel@nongnu.org/msg430490.html v4->v5: - Minor changes to range checking and data validation - Added code for replaying COMMAND_WRITE_POINTER commands on S3 resume v3->v4: - Added a 'src_offset' field in the message so the pointer need not point to the beginning of the file. - Changed the command struct definitions from anonymous to named. v2->v3: - fixed up the qemu_cfg_write* functions to allow writing to an arbitrary offset within the destination file. - Changed function name to COMMAND_WRITE_POINTER and its functionality to not patch memory at all, but write back to a specified offset. v1->v2: - separated patch into two functional units. - changed so writes only occur over the DMA interface. - fixed coding style. - removed change to romfile struct definition (removed new write_back method). Ben Warren (5): QEMU DMA: Add DMA write capability romfile-loader: Switch to using named structs QEMU fw_cfg: Add command to write back address of file QEMU fw_cfg: Add functions for accessing files by key QEMU fw_cfg: Write fw_cfg back on S3 resume src/fw/paravirt.c | 72 ++++++++++++++++++++++++++++++ src/fw/paravirt.h | 5 +++ src/fw/romfile_loader.c | 114 ++++++++++++++++++++++++++++++++++++++++-------- src/fw/romfile_loader.h | 65 +++++++++++++++++---------- src/resume.c | 4 ++ 5 files changed, 220 insertions(+), 40 deletions(-) -- 2.7.4

1 5

Re: [SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address of file
by Laszlo Ersek 18 Feb '17

18 Feb '17

Ben, On 02/05/17 18:09, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This command is similar to ADD_POINTER, but instead of patching > memory, it writes the pointer back to QEMU over the DMA interface. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/romfile_loader.c | 37 +++++++++++++++++++++++++++++++++++++ > src/fw/romfile_loader.h | 16 ++++++++++------ > 2 files changed, 47 insertions(+), 6 deletions(-) while working on the OVMF patches for WRITE_POINTER, something else occurred to me. As I mentioned in the QEMU thread, https://www.mail-archive.com/qemu-devel@nongnu.org/msg428495.html the VMGENID device (or more generally, each device that produces WRITE_POINTER command(s)) should have a reset handler that makes the device forget the GPA(s) that the firmware communicated to it, via WRITE_POINTER. This is because after system reset, all earlier GPAs become meaningless. What I want to mention here is the corollary to the above. ACPI S3 resume is basically a reset, but the GPAs remain valid, because system memory is preserved. Therefore, at S3 resume, the guest firmware has to replay all the WRITE_POINTER commands. The QEMU reset handler will forcibly forget about the GPAs (which is correct), so the firmware has to re-store them. I think it can be a separate patch for SeaBIOS (but preferably in this series). I plan to do this in OVMF as well. Thanks! Laszlo > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c > index f4b17ff..d0ae42b 100644 > --- a/src/fw/romfile_loader.c > +++ b/src/fw/romfile_loader.c > @@ -5,6 +5,7 @@ > #include "romfile.h" // struct romfile_s > #include "malloc.h" // Zone*, _malloc > #include "output.h" // warn_* > +#include "paravirt.h" // qemu_cfg_write_file > > struct romfile_loader_file { > struct romfile_s *file; > @@ -98,7 +99,39 @@ static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry, > pointer += (unsigned long)src_file->data; > pointer = cpu_to_le64(pointer); > memcpy(dest_file->data + offset, &pointer, entry->pointer_size); > + return; > +err: > + warn_internalerror(); > +} > + > +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry, > + struct romfile_loader_files *files) > +{ > + struct romfile_s *dest_file; > + struct romfile_loader_file *src_file; > + unsigned offset = le32_to_cpu(entry->pointer_offset); > + u64 pointer = 0; > + > + /* Writing back to a file that may not be loaded in RAM */ > + dest_file = romfile_find(entry->pointer_dest_file); > + src_file = romfile_loader_find(entry->pointer_src_file, files); > > + if (!dest_file || !src_file || !src_file->data || > + offset + entry->pointer_size < offset || > + offset + entry->pointer_size > dest_file->size || > + entry->pointer_size < 1 || entry->pointer_size > 8 || > + entry->pointer_size & (entry->pointer_size - 1)) { > + goto err; > + } > + > + pointer = (unsigned long)src_file->data; > + pointer = cpu_to_le64(pointer); > + > + /* Only supported on QEMU */ > + if (qemu_cfg_write_file(&pointer, dest_file, offset, > + entry->pointer_size) != entry->pointer_size) { > + goto err; > + } > return; > err: > warn_internalerror(); > @@ -161,6 +194,10 @@ 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_WRITE_POINTER: > + romfile_loader_write_pointer(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..0c0782c 100644 > --- a/src/fw/romfile_loader.h > +++ b/src/fw/romfile_loader.h > @@ -25,10 +25,13 @@ struct romfile_loader_entry_s { > }; > > /* > - * COMMAND_ADD_POINTER - patch the table (originating from > - * @dest_file) at @pointer_offset, by adding a pointer to the table > + * COMMAND_ADD_POINTER & > + * COMMAND_WRITE_POINTER - patch memory (originating from > + * @dest_file) at @pointer.offset, by adding a pointer to the memory > * originating from @src_file. 1,2,4 or 8 byte unsigned > - * addition is used depending on @pointer_size. > + * addition is used depending on @pointer.size. > + * Instead of patching memory, COMMAND_WRITE_POINTER writes the changes > + * to @dest_file back to the host via DMA > */ > struct { > char pointer_dest_file[ROMFILE_LOADER_FILESZ]; > @@ -57,9 +60,10 @@ struct romfile_loader_entry_s { > }; > > 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_WRITE_POINTER = 0x4, > }; > > enum { >

3 6

Re: [SeaBIOS] [PATCH v4 3/3] QEMU fw_cfg: Add command to write back address of file
by Laszlo Ersek 17 Feb '17

17 Feb '17

On 02/17/17 07:26, ben(a)skyportsystems.com wrote: > From: Ben Warren <ben(a)skyportsystems.com> > > This command is similar to ADD_POINTER, but instead of patching > memory, it writes the pointer back to QEMU over the DMA interface. > > Signed-off-by: Ben Warren <ben(a)skyportsystems.com> > --- > src/fw/romfile_loader.c | 40 ++++++++++++++++++++++++++++++++++++++++ > src/fw/romfile_loader.h | 23 ++++++++++++++++++++--- > 2 files changed, 60 insertions(+), 3 deletions(-) > > diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c > index 7737453..1ddcb2a 100644 > --- a/src/fw/romfile_loader.c > +++ b/src/fw/romfile_loader.c > @@ -5,6 +5,7 @@ > #include "romfile.h" // struct romfile_s > #include "malloc.h" // Zone*, _malloc > #include "output.h" // warn_* > +#include "paravirt.h" // qemu_cfg_write_file > > struct romfile_loader_file { > struct romfile_s *file; > @@ -127,6 +128,41 @@ err: > warn_internalerror(); > } > > +static void romfile_loader_write_pointer(struct romfile_loader_entry_s *entry, > + struct romfile_loader_files *files) Whitespace problem, the second line is not correctly aligned. > +{ > + struct romfile_s *dest_file; > + struct romfile_loader_file *src_file; > + unsigned dst_offset = le32_to_cpu(entry->wr_pointer.dst_offset); > + unsigned src_offset = le32_to_cpu(entry->wr_pointer.src_offset); > + u64 pointer = 0; > + > + /* Writing back to a file that may not be loaded in RAM */ > + dest_file = romfile_find(entry->wr_pointer.dest_file); > + src_file = romfile_loader_find(entry->wr_pointer.src_file, files); > + > + if (!dest_file || !src_file || !src_file->data || > + dst_offset + entry->wr_pointer.size < dst_offset || > + dst_offset + entry->wr_pointer.size > dest_file->size || > + src_offset > src_file->file->size || This is off by one (paralleling the earlier assert() off-by-one in QEMU: <https://www.mail-archive.com/qemu-devel@nongnu.org/msg430195.html>); namely src_offset == src_file->file->size would mean that the pointer points one past src_file in memory. While such pointers are allowed in C (for evaluation, not dereferencing), in our case it would mean a zero-sized field in src_file for QEMU to access, and that's wrong. IOW, equality too should be rejected. > + entry->wr_pointer.size < 1 || entry->wr_pointer.size > 8 || > + entry->wr_pointer.size & (entry->wr_pointer.size - 1)) { > + goto err; > + } > + > + pointer = (unsigned long)src_file->data + src_offset; This is good, but right after you should range-check the value in "pointer" against "entry->wr_pointer.size". There might be a better way for this in SeaBIOS, but the way I do it in OVMF is like if ((pointer >> (entry->wr_pointer.size * 8 - 1)) >> 1) { goto err; } The idea is to shift right "entry->wr_pointer.size" bytes out of "pointer", and see if anything remains (which we won't be able to represent, so err out then). And the funky double-shift (separating out the final bit shift) is due to the fact that shifting a uint64_t by 64 bits is undefined behavior, so we shift 63 + 1 bits. > + pointer = cpu_to_le64(pointer); > + > + /* Only supported on QEMU */ > + if (qemu_cfg_write_file(&pointer, dest_file, dst_offset, > + entry->wr_pointer.size) != entry->wr_pointer.size) { > + goto err; > + } > + return; > + err: > + warn_internalerror(); > +} > + > int romfile_loader_execute(const char *name) > { > struct romfile_loader_entry_s *entry; > @@ -161,6 +197,10 @@ 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_WRITE_POINTER: > + romfile_loader_write_pointer(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 bce3719..8b557a3 100644 > --- a/src/fw/romfile_loader.h > +++ b/src/fw/romfile_loader.h > @@ -51,15 +51,32 @@ struct romfile_loader_entry_s { > u32 length; > } cksum; > > + /* > + * COMMAND_WRITE_POINTER - Write back to a host file via DMA, > + * wr_pointer@dest_file at offset @wr_pointer.dst_offset, a pointer "wr_pointer@dest_file" should be "@wr_pointer.dest_file". The rest looks good. Thanks! > + * to the table originating from @wr_pointer.src_file at offset > + * @wr_pointer.src_offset. > + * 1,2,4 or 8 byte unsigned addition is used depending on > + * @wr_pointer.size. > + */ > + struct { > + char dest_file[ROMFILE_LOADER_FILESZ]; > + char src_file[ROMFILE_LOADER_FILESZ]; > + u32 dst_offset; > + u32 src_offset; > + u8 size; > + } wr_pointer; > + > /* 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_WRITE_POINTER = 0x4, > }; > > enum { >

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS