diff mbox

[v4,15/23] i386: add bios linker/loader

Message ID 1379857006-17451-16-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Sept. 22, 2013, 1:37 p.m. UTC 
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 <mst@redhat.com>
---
 include/hw/i386/bios-linker-loader.h |  26 ++++++
 hw/i386/bios-linker-loader.c         | 156 +++++++++++++++++++++++++++++++++++
 hw/i386/Makefile.objs                |   1 +
 3 files changed, 183 insertions(+)
 create mode 100644 include/hw/i386/bios-linker-loader.h
 create mode 100644 hw/i386/bios-linker-loader.c

Comments

Paolo Bonzini Sept. 23, 2013, 12:48 p.m. UTC | #1 
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 <mst@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".

Also, the API is (understandably) bios_linker_*, not
bios_linker_loader_*, with the exception of BiosLinkerLoaderEntry,
BIOS_LINKER_LOADER_FILESZ and perhaps a few others I've missed.  I would
just drop the loader part, and if you go for the BIOS -> ACPI rename,
make the filenames include/hw/acpi/linker.h and hw/acpi/linker.c.

Just post v5 of this patch in any case, I don't need to see the matching
changes in patch 23.

Paolo

>  3 files changed, 183 insertions(+)
>  create mode 100644 include/hw/i386/bios-linker-loader.h
>  create mode 100644 hw/i386/bios-linker-loader.c
> 
> diff --git a/include/hw/i386/bios-linker-loader.h b/include/hw/i386/bios-linker-loader.h
> new file mode 100644
> index 0000000..18c3868
> --- /dev/null
> +++ b/include/hw/i386/bios-linker-loader.h
> @@ -0,0 +1,26 @@
> +#ifndef BIOS_LINKER_LOADER_H
> +#define BIOS_LINKER_LOADER_H
> +
> +#include <glib.h>
> +#include <stdbool.h>
> +#include <inttypes.h>
> +
> +GArray *bios_linker_init(void);
> +
> +void bios_linker_alloc(GArray *linker,
> +                       const char *file,
> +                       uint32_t alloc_align,
> +                       bool alloc_fseg);
> +
> +void bios_linker_add_checksum(GArray *linker, const char *file, void *table,
> +                              void *start, unsigned size, uint8_t *checksum);
> +
> +
> +void bios_linker_add_pointer(GArray *linker,
> +                             const char *dest_file,
> +                             const char *src_file,
> +                             GArray *table, void *pointer,
> +                             uint8_t pointer_size);
> +
> +void *bios_linker_cleanup(GArray *linker);
> +#endif
> diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> new file mode 100644
> index 0000000..644016b
> --- /dev/null
> +++ b/hw/i386/bios-linker-loader.c
> @@ -0,0 +1,156 @@
> +/* Dynamic linker/loader of ACPI tables
> + *
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/i386/bios-linker-loader.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +#include <string.h>
> +#include <assert.h>
> +#include "qemu/bswap.h"
> +
> +#define BIOS_LINKER_LOADER_FILESZ FW_CFG_MAX_FILE_PATH
> +
> +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;

Also perhaps add QEMU_PACKED here too, just in case someone makes
typedefs out of the structs and forgets it.

> +        /*
> +         * 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).

> +    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 ;)
>
Michael S. Tsirkin Sept. 23, 2013, 1:36 p.m. UTC | #2 
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 <mst@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?

> Also, the API is (understandably) bios_linker_*, not
> bios_linker_loader_*, with the exception of BiosLinkerLoaderEntry,
> BIOS_LINKER_LOADER_FILESZ and perhaps a few others I've missed.  I would
> just drop the loader part, and if you go for the BIOS -> ACPI rename,
> make the filenames include/hw/acpi/linker.h and hw/acpi/linker.c.
> 
> Just post v5 of this patch in any case, I don't need to see the matching
> changes in patch 23.
> 
> Paolo

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.

> >  3 files changed, 183 insertions(+)
> >  create mode 100644 include/hw/i386/bios-linker-loader.h
> >  create mode 100644 hw/i386/bios-linker-loader.c
> > 
> > diff --git a/include/hw/i386/bios-linker-loader.h b/include/hw/i386/bios-linker-loader.h
> > new file mode 100644
> > index 0000000..18c3868
> > --- /dev/null
> > +++ b/include/hw/i386/bios-linker-loader.h
> > @@ -0,0 +1,26 @@
> > +#ifndef BIOS_LINKER_LOADER_H
> > +#define BIOS_LINKER_LOADER_H
> > +
> > +#include <glib.h>
> > +#include <stdbool.h>
> > +#include <inttypes.h>
> > +
> > +GArray *bios_linker_init(void);
> > +
> > +void bios_linker_alloc(GArray *linker,
> > +                       const char *file,
> > +                       uint32_t alloc_align,
> > +                       bool alloc_fseg);
> > +
> > +void bios_linker_add_checksum(GArray *linker, const char *file, void *table,
> > +                              void *start, unsigned size, uint8_t *checksum);
> > +
> > +
> > +void bios_linker_add_pointer(GArray *linker,
> > +                             const char *dest_file,
> > +                             const char *src_file,
> > +                             GArray *table, void *pointer,
> > +                             uint8_t pointer_size);
> > +
> > +void *bios_linker_cleanup(GArray *linker);
> > +#endif
> > diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> > new file mode 100644
> > index 0000000..644016b
> > --- /dev/null
> > +++ b/hw/i386/bios-linker-loader.c
> > @@ -0,0 +1,156 @@
> > +/* Dynamic linker/loader of ACPI tables
> > + *
> > + * Copyright (C) 2013 Red Hat Inc
> > + *
> > + * Author: Michael S. Tsirkin <mst@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "hw/i386/bios-linker-loader.h"
> > +#include "hw/nvram/fw_cfg.h"
> > +
> > +#include <string.h>
> > +#include <assert.h>
> > +#include "qemu/bswap.h"
> > +
> > +#define BIOS_LINKER_LOADER_FILESZ FW_CFG_MAX_FILE_PATH
> > +
> > +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.

> Also perhaps add QEMU_PACKED here too, just in case someone makes
> typedefs out of the structs and forgets it.

I don't see why.
Even the QEMU_PACKED at the end is for documentation purposes only:
there's no real need for it in any of the structs here.
So let's document it all in one place.

> > +        /*
> > +         * 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 ;)
> >
Paolo Bonzini Sept. 23, 2013, 1:39 p.m. UTC | #3 
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 <mst@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 ;)
>>>
Michael S. Tsirkin Sept. 23, 2013, 1:47 p.m. UTC | #4 
On Mon, Sep 23, 2013 at 03:39:10PM +0200, Paolo Bonzini wrote:
> 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 <mst@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.

Ah you mean move it outside include/. OK.

>  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

You are right, that's cleaner. I'll do this.
I don't want to sprinkle QEMU_PACKED around though,
it's just there for documentation purposes, so one
place is enough.

> >>> +        /*
> >>> +         * 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 ;)
> >>>
Paolo Bonzini Sept. 23, 2013, 1:52 p.m. UTC | #5 
Il 23/09/2013 15:47, Michael S. Tsirkin ha scritto:
> > It doesn't, the names were already prefixed with alloc_/pointer_/cksum_.
> >  All it does is change underscores to periods.
> 
> You are right, that's cleaner. I'll do this.
> I don't want to sprinkle QEMU_PACKED around though,
> it's just there for documentation purposes, so one
> place is enough.

Fair enough.  Thanks!

Paolo
diff mbox

Patch

diff --git a/include/hw/i386/bios-linker-loader.h b/include/hw/i386/bios-linker-loader.h
new file mode 100644
index 0000000..18c3868
--- /dev/null
+++ b/include/hw/i386/bios-linker-loader.h
@@ -0,0 +1,26 @@ 
+#ifndef BIOS_LINKER_LOADER_H
+#define BIOS_LINKER_LOADER_H
+
+#include <glib.h>
+#include <stdbool.h>
+#include <inttypes.h>
+
+GArray *bios_linker_init(void);
+
+void bios_linker_alloc(GArray *linker,
+                       const char *file,
+                       uint32_t alloc_align,
+                       bool alloc_fseg);
+
+void bios_linker_add_checksum(GArray *linker, const char *file, void *table,
+                              void *start, unsigned size, uint8_t *checksum);
+
+
+void bios_linker_add_pointer(GArray *linker,
+                             const char *dest_file,
+                             const char *src_file,
+                             GArray *table, void *pointer,
+                             uint8_t pointer_size);
+
+void *bios_linker_cleanup(GArray *linker);
+#endif
diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
new file mode 100644
index 0000000..644016b
--- /dev/null
+++ b/hw/i386/bios-linker-loader.c
@@ -0,0 +1,156 @@ 
+/* Dynamic linker/loader of ACPI tables
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/i386/bios-linker-loader.h"
+#include "hw/nvram/fw_cfg.h"
+
+#include <string.h>
+#include <assert.h>
+#include "qemu/bswap.h"
+
+#define BIOS_LINKER_LOADER_FILESZ FW_CFG_MAX_FILE_PATH
+
+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;
+        };
+
+        /*
+         * 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);
+    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 ;)