Re: [Qemu-devel] [RFC 42/47] acpi: make tables linker-loader available to other targets

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 19, 2015 at 01:36:36PM +0100, Paolo Bonzini wrote:
> 
> 
> On 19/12/2014 03:02, Igor Mammedov wrote:
> > keeping bios-linker-loader.c i386 specific would break build
> > of mips target which is built with CONFIG_ACPI which would
> > dependend on it in following patch that adds acpi_def_block()
> > term. Also UEFI for ARM target is going to use linker as well
> > so it makes sense to move it into generic target independent
> > ACPI part.
> 
> Makes sense now that fw_cfg was added "officially" to -M virt.
> 
> Paolo

I'm not sure why doesn't git detect this as a move.
Did you also change files at the same time?  Please don't: this makes
the patch extremely hard to check. You can use tricks e.g. using the
preprocessor to split move away from code changes.


> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/Makefile.objs                |   2 +-
> >  hw/acpi/acpi_gen_utils.c             |   1 +
> >  hw/acpi/bios-linker-loader.c         | 157 +++++++++++++++++++++++++++++++++++
> >  hw/i386/Makefile.objs                |   1 -
> >  hw/i386/acpi-build.c                 |   2 +-
> >  hw/i386/bios-linker-loader.c         | 157 -----------------------------------
> >  hw/i386/bios-linker-loader.h         |  27 ------
> >  include/hw/acpi/bios-linker-loader.h |  27 ++++++
> >  8 files changed, 187 insertions(+), 187 deletions(-)
> >  create mode 100644 hw/acpi/bios-linker-loader.c
> >  delete mode 100644 hw/i386/bios-linker-loader.c
> >  delete mode 100644 hw/i386/bios-linker-loader.h
> >  create mode 100644 include/hw/acpi/bios-linker-loader.h
> > 
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index 4237232..02ca4ed 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,4 +1,4 @@
> >  common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > -common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> > +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o bios-linker-loader.o
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > index 39cbab7..cc2fa03 100644
> > --- a/hw/acpi/acpi_gen_utils.c
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -5,6 +5,7 @@
> >  #include <string.h>
> >  #include "hw/acpi/acpi_gen_utils.h"
> >  #include "qemu/bswap.h"
> > +#include "hw/acpi/bios-linker-loader.h"
> >  
> >  GArray *build_alloc_array(void)
> >  {
> > diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> > new file mode 100644
> > index 0000000..5cc4d90
> > --- /dev/null
> > +++ b/hw/acpi/bios-linker-loader.c
> > @@ -0,0 +1,157 @@
> > +/* 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 "qemu-common.h"
> > +#include "hw/acpi/bios-linker-loader.h"
> > +#include "hw/nvram/fw_cfg.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 file[BIOS_LINKER_LOADER_FILESZ];
> > +            uint32_t align;
> > +            uint8_t zone;
> > +        } alloc;
> > +
> > +        /*
> > +         * 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 dest_file[BIOS_LINKER_LOADER_FILESZ];
> > +            char src_file[BIOS_LINKER_LOADER_FILESZ];
> > +            uint32_t offset;
> > +            uint8_t size;
> > +        } pointer;
> > +
> > +        /*
> > +         * 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 file[BIOS_LINKER_LOADER_FILESZ];
> > +            uint32_t offset;
> > +            uint32_t start;
> > +            uint32_t length;
> > +        } cksum;
> > +
> > +        /* 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_loader_init(void)
> > +{
> > +    return g_array_new(false, true /* clear */, 1);
> > +}
> > +
> > +/* Free linker wrapper and return the linker array. */
> > +void *bios_linker_loader_cleanup(GArray *linker)
> > +{
> > +    return g_array_free(linker, false);
> > +}
> > +
> > +void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > +}
> > +
> > +void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > +}
> > +
> > +void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > +}
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index 790a56c..6c8705d 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -7,7 +7,6 @@ obj-$(CONFIG_XEN) += ../xenpv/ xen/
> >  
> >  obj-y += kvmvapic.o
> >  obj-y += acpi-build.o
> > -obj-y += bios-linker-loader.o
> >  hw/i386/acpi-build.o: hw/i386/acpi-build.c hw/i386/acpi-dsdt.hex \
> >  	hw/i386/ssdt-misc.hex hw/i386/q35-acpi-dsdt.hex \
> >  	hw/i386/ssdt-tpm.hex
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b425156..e0fa30a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -36,7 +36,7 @@
> >  #include "hw/i386/acpi-defs.h"
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/nvram/fw_cfg.h"
> > -#include "bios-linker-loader.h"
> > +#include "hw/acpi/bios-linker-loader.h"
> >  #include "hw/loader.h"
> >  #include "hw/isa/isa.h"
> >  #include "hw/acpi/memory_hotplug.h"
> > diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> > deleted file mode 100644
> > index aa56184..0000000
> > --- a/hw/i386/bios-linker-loader.c
> > +++ /dev/null
> > @@ -1,157 +0,0 @@
> > -/* 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 "qemu-common.h"
> > -#include "bios-linker-loader.h"
> > -#include "hw/nvram/fw_cfg.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 file[BIOS_LINKER_LOADER_FILESZ];
> > -            uint32_t align;
> > -            uint8_t zone;
> > -        } alloc;
> > -
> > -        /*
> > -         * 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 dest_file[BIOS_LINKER_LOADER_FILESZ];
> > -            char src_file[BIOS_LINKER_LOADER_FILESZ];
> > -            uint32_t offset;
> > -            uint8_t size;
> > -        } pointer;
> > -
> > -        /*
> > -         * 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 file[BIOS_LINKER_LOADER_FILESZ];
> > -            uint32_t offset;
> > -            uint32_t start;
> > -            uint32_t length;
> > -        } cksum;
> > -
> > -        /* 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_loader_init(void)
> > -{
> > -    return g_array_new(false, true /* clear */, 1);
> > -}
> > -
> > -/* Free linker wrapper and return the linker array. */
> > -void *bios_linker_loader_cleanup(GArray *linker)
> > -{
> > -    return g_array_free(linker, false);
> > -}
> > -
> > -void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > -}
> > -
> > -void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > -}
> > -
> > -void bios_linker_loader_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_vals(linker, &entry, sizeof entry);
> > -}
> > diff --git a/hw/i386/bios-linker-loader.h b/hw/i386/bios-linker-loader.h
> > deleted file mode 100644
> > index 498c0af..0000000
> > --- a/hw/i386/bios-linker-loader.h
> > +++ /dev/null
> > @@ -1,27 +0,0 @@
> > -#ifndef BIOS_LINKER_LOADER_H
> > -#define BIOS_LINKER_LOADER_H
> > -
> > -#include <glib.h>
> > -#include <stdbool.h>
> > -#include <inttypes.h>
> > -
> > -GArray *bios_linker_loader_init(void);
> > -
> > -void bios_linker_loader_alloc(GArray *linker,
> > -                              const char *file,
> > -                              uint32_t alloc_align,
> > -                              bool alloc_fseg);
> > -
> > -void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> > -                                     void *table,
> > -                                     void *start, unsigned size,
> > -                                     uint8_t *checksum);
> > -
> > -void bios_linker_loader_add_pointer(GArray *linker,
> > -                                    const char *dest_file,
> > -                                    const char *src_file,
> > -                                    GArray *table, void *pointer,
> > -                                    uint8_t pointer_size);
> > -
> > -void *bios_linker_loader_cleanup(GArray *linker);
> > -#endif
> > diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> > new file mode 100644
> > index 0000000..498c0af
> > --- /dev/null
> > +++ b/include/hw/acpi/bios-linker-loader.h
> > @@ -0,0 +1,27 @@
> > +#ifndef BIOS_LINKER_LOADER_H
> > +#define BIOS_LINKER_LOADER_H
> > +
> > +#include <glib.h>
> > +#include <stdbool.h>
> > +#include <inttypes.h>
> > +
> > +GArray *bios_linker_loader_init(void);
> > +
> > +void bios_linker_loader_alloc(GArray *linker,
> > +                              const char *file,
> > +                              uint32_t alloc_align,
> > +                              bool alloc_fseg);
> > +
> > +void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> > +                                     void *table,
> > +                                     void *start, unsigned size,
> > +                                     uint8_t *checksum);
> > +
> > +void bios_linker_loader_add_pointer(GArray *linker,
> > +                                    const char *dest_file,
> > +                                    const char *src_file,
> > +                                    GArray *table, void *pointer,
> > +                                    uint8_t pointer_size);
> > +
> > +void *bios_linker_loader_cleanup(GArray *linker);
> > +#endif
> >