> On Feb 6, 2017, at 6:56 AM, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> On Sun, Feb 05, 2017 at 01:11:57AM -0800, b...@skyportsystems.com wrote:
>> From: Ben Warren <b...@skyportsystems.com>
>>
>> This adds to the existing 'add pointer' functionality in that it
>> instructs the guest (BIOS or UEFI) to not patch memory but to instead
>> write the changes back to QEMU via a writeable fw_cfg file.
>>
>> Signed-off-by: Ben Warren <b...@skyportsystems.com>
>> ---
>> hw/acpi/aml-build.c | 2 +-
>> hw/acpi/bios-linker-loader.c | 35 ++++++++++++++++++++++++-----------
>> hw/acpi/nvdimm.c | 2 +-
>> hw/arm/virt-acpi-build.c | 4 ++--
>> hw/i386/acpi-build.c | 8 ++++----
>> include/hw/acpi/bios-linker-loader.h | 3 ++-
>> 6 files changed, 34 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 9fc54c9..03b6c6c 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1626,7 +1626,7 @@ build_rsdt(GArray *table_data, BIOSLinker *linker,
>> GArray *table_offsets,
>> /* rsdt->table_offset_entry to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, rsdt_entry_offset, rsdt_entry_size,
>> - ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, ref_tbl_offset, false);
>> }
>> build_header(linker, table_data,
>> (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
>> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
>> index d963ebe..e46bc29 100644
>> --- a/hw/acpi/bios-linker-loader.c
>> +++ b/hw/acpi/bios-linker-loader.c
>> @@ -52,10 +52,13 @@ struct BiosLinkerLoaderEntry {
>> } alloc;
>>
>> /*
>> - * 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 guest 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.
>> + * Instead of patching memory, COMMAND_WRITE_POINTER writes the
>> changes
>> + * to @dest_file in QEMU via fw_cfg DMA.
>> */
>> struct {
>> char dest_file[BIOS_LINKER_LOADER_FILESZ];
>> @@ -85,9 +88,10 @@ struct BiosLinkerLoaderEntry {
>> typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
>>
>> enum {
>> - BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
>> - BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
>> - BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + BIOS_LINKER_LOADER_COMMAND_ALLOCATE = 0x1,
>> + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER = 0x2,
>> + BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
>> + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER = 0x4,
>> };
>>
>> enum {
>> @@ -242,13 +246,15 @@ void bios_linker_loader_add_checksum(BIOSLinker
>> *linker, const char *file_name,
>> * @src_offset: location within source file blob to which
>> * @dest_file+@dst_patched_offset will point to after
>> * firmware's executed ADD_POINTER command
>> + * @write_back: guest should write change contents back to QEMU after
>> patching
>> */
>> void bios_linker_loader_add_pointer(BIOSLinker *linker,
>> const char *dest_file,
>> uint32_t dst_patched_offset,
>> uint8_t dst_patched_size,
>> const char *src_file,
>> - uint32_t src_offset)
>> + uint32_t src_offset,
>> + bool write_back)
>> {
>> uint64_t le_src_offset;
>> BiosLinkerLoaderEntry entry;
>
> Frankly I prefer a new bios_linker_loader_write_pointer.
>
OK
>
>> @@ -257,8 +263,11 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>> const BiosLinkerFileEntry *source_file =
>> bios_linker_find_file(linker, src_file);
>>
>> - assert(dst_patched_offset < dst_file->blob->len);
>> - assert(dst_patched_offset + dst_patched_size <= dst_file->blob->len);
>> + /* dst_file need not exist if writing back */
>
> Why not?
Because WRITE_POINTER can be called without having first called ALLOCATE. In
the Vm Generation ID example, there’s no reason for BIOS to allocate memory for
the address fw_cfg, since it’s a construct that only matters to QEMU. This is
something that was requested by Laszlo.
>
>> + if (!write_back) {
>> + assert(dst_patched_offset < dst_file->blob->len);
>> + assert(dst_patched_offset + dst_patched_size <=
>> dst_file->blob->len);
>> + }
>> assert(src_offset < source_file->blob->len);
>>
>> memset(&entry, 0, sizeof entry);
>> @@ -266,15 +275,19 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>> sizeof entry.pointer.dest_file - 1);
>> strncpy(entry.pointer.src_file, src_file,
>> sizeof entry.pointer.src_file - 1);
>> - entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ADD_POINTER);
>> + entry.command = cpu_to_le32(write_back ?
>> + BIOS_LINKER_LOADER_COMMAND_WRITE_POINTER :
>> + BIOS_LINKER_LOADER_COMMAND_ADD_POINTER);
>> entry.pointer.offset = cpu_to_le32(dst_patched_offset);
>> entry.pointer.size = dst_patched_size;
>> assert(dst_patched_size == 1 || dst_patched_size == 2 ||
>> dst_patched_size == 4 || dst_patched_size == 8);
>>
>> le_src_offset = cpu_to_le64(src_offset);
>> - memcpy(dst_file->blob->data + dst_patched_offset,
>> - &le_src_offset, dst_patched_size);
>> + if (!write_back) {
>> + memcpy(dst_file->blob->data + dst_patched_offset,
>> + &le_src_offset, dst_patched_size);
>> + }
>>
>> g_array_append_vals(linker->cmd_blob, &entry, sizeof entry);
>> }
>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
>> index 8e7d6ec..175996e 100644
>> --- a/hw/acpi/nvdimm.c
>> +++ b/hw/acpi/nvdimm.c
>> @@ -1266,7 +1266,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets,
>> GArray *table_data,
>> sizeof(NvdimmDsmIn), false /* high memory */);
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
>> - NVDIMM_DSM_MEM_FILE, 0);
>> + NVDIMM_DSM_MEM_FILE, 0, false);
>> build_header(linker, table_data,
>> (void *)(table_data->data + nvdimm_ssdt),
>> "SSDT", table_data->len - nvdimm_ssdt, 1, NULL, "NVDIMM");
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 07a10ac..a13f40d 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -380,7 +380,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker,
>> unsigned rsdt_tbl_offset)
>> /* Address to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
>> - ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset, false);
>>
>> /* Checksum to be filled by Guest linker */
>> bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>> @@ -684,7 +684,7 @@ static void build_fadt(GArray *table_data, BIOSLinker
>> *linker,
>> /* DSDT address to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>> - ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset, false);
>>
>> build_header(linker, table_data,
>> (void *)fadt, "FACP", sizeof(*fadt), 5, NULL, NULL);
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 1c928ab..78a1d84 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -319,13 +319,13 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
>> AcpiPmInfo *pm,
>> /* FACS address to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
>> - ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, facs_tbl_offset, false);
>>
>> /* DSDT address to be filled by Guest linker */
>> fadt_setup(fadt, pm);
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
>> - ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset, false);
>>
>> build_header(linker, table_data,
>> (void *)fadt, "FACP", sizeof(*fadt), 1, oem_id,
>> oem_table_id);
>> @@ -2262,7 +2262,7 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker,
>> GArray *tcpalog)
>> /* log area start address to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_TABLE_FILE, log_addr_offset, log_addr_size,
>> - ACPI_BUILD_TPMLOG_FILE, 0);
>> + ACPI_BUILD_TPMLOG_FILE, 0, false);
>>
>> build_header(linker, table_data,
>> (void *)tcpa, "TCPA", sizeof(*tcpa), 2, NULL, NULL);
>> @@ -2552,7 +2552,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker,
>> unsigned rsdt_tbl_offset)
>> /* Address to be filled by Guest linker */
>> bios_linker_loader_add_pointer(linker,
>> ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
>> - ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
>> + ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset, false);
>>
>> /* Checksum to be filled by Guest linker */
>> bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>> diff --git a/include/hw/acpi/bios-linker-loader.h
>> b/include/hw/acpi/bios-linker-loader.h
>> index fa1e5d1..d97e39d 100644
>> --- a/include/hw/acpi/bios-linker-loader.h
>> +++ b/include/hw/acpi/bios-linker-loader.h
>> @@ -24,7 +24,8 @@ void bios_linker_loader_add_pointer(BIOSLinker *linker,
>> uint32_t dst_patched_offset,
>> uint8_t dst_patched_size,
>> const char *src_file,
>> - uint32_t src_offset);
>> + uint32_t src_offset,
>> + bool write_back);
>>
>> void bios_linker_loader_cleanup(BIOSLinker *linker);
>> #endif
>> --
>> 2.7.4
smime.p7s
Description: S/MIME cryptographic signature
