[SeaBIOS] [PATCH v3 2/2] QEMU fw_cfg: Add command to write back address of file
Laszlo Ersek
lersek at redhat.com
Thu Feb 9 09:24:19 CET 2017
On 02/09/17 09:17, Laszlo Ersek wrote:
> Ben,
>
> On 02/05/17 18:09, ben at skyportsystems.com wrote:
>> From: Ben Warren <ben at 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 at 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.
... By that I don't necessarily mean to re-run the exact same
linker-loader logic; it should be okay to save the *results* of those
operations in a simpler array (that is, write exactly what values to
what fw_cfg files at what offsets).
And, you can detect whether this is needed at all, the
"etc/system-states" fw_cfg file can be used to see if ACPI S3 is enabled
or disabled in QEMU. (Please grep QEMU for "disable_s3" and
"etc/system-states".)
Thanks
Laszlo
>
> 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 {
>>
>
More information about the SeaBIOS
mailing list