Il 23/09/2013 15:36, Michael S. Tsirkin ha scritto:
> On Mon, Sep 23, 2013 at 02:48:57PM +0200, Paolo Bonzini wrote:
>> Il 22/09/2013 15:37, Michael S. Tsirkin ha scritto:
>>> This adds a dynamic bios linker/loader.
>>> This will be used by acpi table generation
>>> code to:
>>> - load each table in the appropriate memory segment
>>> - link tables to each other
>>> - fix up checksums after said linking
>>>
>>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
>>> ---
>>> include/hw/i386/bios-linker-loader.h | 26 ++++++
>>> hw/i386/bios-linker-loader.c | 156
>>> +++++++++++++++++++++++++++++++++++
>>> hw/i386/Makefile.objs | 1 +
>>
>> Just a naming question...
>>
>> If this is ACPI-specific (or if there are no other uses in sight), it
>> should go in include/hw/acpi and hw/acpi, and renamed with s/bios/acpi/ etc.
>>
>> If this is i386-specific, it should go entirely in hw/i386 and the
>> headers should be included as "bios-linker-loader.h".
>
> Definitely not ACPI specific.
> It's only used on i386 so I put it all under i386.
> You think we should replace
> #include "hw/i386/bios-linker-loader.h" with
> #include "bios-linker-loader.h"?
>
> I don't really care, even though this makes headers
> hard to find. What's the benefit?
Private headers are outside include/, public headers within it. Same
conventions as Linux. But if it's not ACPI-specific, I don't feel too
strongly about it.
> It's not easy to find a good name for it.
> For now I'll just make all names bios_linker_loader_ consistently.
> We can always rename it later, it's not set in stone.
Ok.
>>> +struct BiosLinkerLoaderEntry {
>>> + uint32_t 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.
>>> + *
>>> + * Must appear exactly once for each file, and before
>>> + * this file is referenced by any other command.
>>> + */
>>> + struct {
>>> + char alloc_file[BIOS_LINKER_LOADER_FILESZ];
>>> + uint32_t alloc_align;
>>> + uint8_t alloc_zone;
>>> + };
>>
>> I would prefer to have named structs, like
>>
>> struct {
>> char file[BIOS_LINKER_LOADER_FILESZ];
>> uint32_t align;
>> uint8_t zone;
>> } alloc;
>
> Why do you prefer this? It just makes code more verbose.
It doesn't, the names were already prefixed with alloc_/pointer_/cksum_.
All it does is change underscores to periods.
Paolo
>>> + /*
>>> + * COMMAND_ADD_POINTER - patch the table (originating from
>>> + * @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.
>>> + */
>>> + struct {
>>> + char pointer_dest_file[BIOS_LINKER_LOADER_FILESZ];
>>> + char pointer_src_file[BIOS_LINKER_LOADER_FILESZ];
>>> + uint32_t pointer_offset;
>>> + uint8_t pointer_size;
>>> + };
>>> +
>>> + /*
>>> + * COMMAND_ADD_CHECKSUM - calculate checksum of the range
>>> specified by
>>> + * @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[BIOS_LINKER_LOADER_FILESZ];
>>> + uint32_t cksum_offset;
>>> + uint32_t cksum_start;
>>> + uint32_t cksum_length;
>>> + };
>>> +
>>> + /* padding */
>>> + char pad[124];
>>> + };
>>> +} QEMU_PACKED;
>>> +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,
>>> +};
>>> +
>>> +enum {
>>> + BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1,
>>> + BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
>>> +};
>>> +
>>> +GArray *bios_linker_init(void)
>>> +{
>>> + return g_array_new(false, true /* clear */,
>>> sizeof(BiosLinkerLoaderEntry));
>>> +}
>>> +
>>> +/* Free linker wrapper and return the linker array. */
>>> +void *bios_linker_cleanup(GArray *linker)
>>> +{
>>> + return g_array_free(linker, false);
>>> +}
>>> +
>>> +void bios_linker_alloc(GArray *linker,
>>> + const char *file,
>>> + uint32_t alloc_align,
>>> + bool alloc_fseg)
>>> +{
>>> + BiosLinkerLoaderEntry entry;
>>> +
>>> + memset(&entry, 0, sizeof entry);
>>> + strncpy(entry.alloc_file, file, sizeof entry.alloc_file - 1);
>>
>> Please use pstrcpy (also below).
>
> OK.
>
>>> + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
>>> + entry.alloc_align = cpu_to_le32(alloc_align);
>>> + entry.alloc_zone = cpu_to_le32(alloc_fseg ?
>>> + BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
>>> + BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
>>> +
>>> + /* Alloc entries must come first, so prepend them */
>>> + g_array_prepend_val(linker, entry);
>>> +}
>>> +
>>> +void bios_linker_add_checksum(GArray *linker, const char *file, void
>>> *table,
>>> + void *start, unsigned size, uint8_t
>>> *checksum)
>>> +{
>>> + BiosLinkerLoaderEntry entry;
>>> +
>>> + memset(&entry, 0, sizeof entry);
>>> + strncpy(entry.cksum_file, file, sizeof entry.cksum_file - 1);
>>> + entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM);
>>> + entry.cksum_offset = cpu_to_le32(checksum - (uint8_t *)table);
>>> + entry.cksum_start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
>>> + entry.cksum_length = cpu_to_le32(size);
>>> +
>>> + g_array_append_val(linker, entry);
>>> +}
>>> +
>>> +void bios_linker_add_pointer(GArray *linker,
>>> + const char *dest_file,
>>> + const char *src_file,
>>> + GArray *table, void *pointer,
>>> + uint8_t pointer_size)
>>> +{
>>> + BiosLinkerLoaderEntry entry;
>>> +
>>> + memset(&entry, 0, sizeof entry);
>>> + strncpy(entry.pointer_dest_file, dest_file,
>>> + 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.pointer_offset = cpu_to_le32((gchar *)pointer - table->data);
>>> + entry.pointer_size = pointer_size;
>>> + assert(pointer_size == 1 || pointer_size == 2 ||
>>> + pointer_size == 4 || pointer_size == 8);
>>> +
>>> + g_array_append_val(linker, entry);
>>> +}
>>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>>> index f950707..b9ca380 100644
>>> --- a/hw/i386/Makefile.objs
>>> +++ b/hw/i386/Makefile.objs
>>> @@ -5,6 +5,7 @@ obj-y += pc_sysfw.o
>>> obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
>>>
>>> obj-y += kvmvapic.o
>>> +obj-y += bios-linker-loader.o
>>>
>>> iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
>>> ; then echo "$(2)"; else echo "$(3)"; fi ;)
>>>
