Re: [Xen-devel] [PATCH v3 09/19] acpi/hvmloader: Include file/paths adjustments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.16 at 20:59, <boris.ostrovsky@xxxxxxxxxx> wrote:
> In prepearation to moving acpi sources into generally available
> libacpi:
> 
> 1. Pass IOAPIC/LAPIC/PCI mask values via struct acpi_config
> 2. Modify include files search paths to point to acpi directory
> 3. Macro-ise include file for build.c that defines various
>    utilities used by that file. Users of libacpi will be expected
>    to define this macro when compiling build.c
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
> Changes in v3:
> * Instead of adding x86.h pass APIC/IOAPIC info via acpi_config parameter.
> * Use <> instead of "" for include directive
> 
> 
>  tools/firmware/hvmloader/Makefile             |  3 ++-
>  tools/firmware/hvmloader/acpi/README          | 16 ++++++++++++----
>  tools/firmware/hvmloader/acpi/build.c         | 19 ++++++++++---------
>  tools/firmware/hvmloader/acpi/libacpi.h       |  7 +++++++
>  tools/firmware/hvmloader/hvmloader.c          |  2 +-
>  tools/firmware/hvmloader/rombios.c            |  2 +-
>  tools/firmware/hvmloader/seabios.c            |  5 +++--
>  tools/firmware/hvmloader/util.c               | 15 +++++++++++++--
>  tools/firmware/rombios/32bit/Makefile         |  2 +-
>  tools/firmware/rombios/32bit/tcgbios/Makefile |  2 +-
>  tools/firmware/rombios/32bit/util.h           |  2 +-
>  11 files changed, 52 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/Makefile 
> b/tools/firmware/hvmloader/Makefile
> index b6c5b83..77e95f1 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -76,7 +76,8 @@ smbios.o: CFLAGS += 
> -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
>  ACPI_PATH = acpi
>  ACPI_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c
>  ACPI_OBJS = $(patsubst %.c,%.o,$(ACPI_FILES)) build.o static_tables.o
> -$(ACPI_OBJS): CFLAGS += -I$(ACPI_PATH) -I.
> +$(ACPI_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"../util.h\"
> +CFLAGS += -I$(ACPI_PATH)
>  vpath build.c $(ACPI_PATH)
>  vpath static_tables.c $(ACPI_PATH)
>  OBJS += $(ACPI_OBJS)
> diff --git a/tools/firmware/hvmloader/acpi/README 
> b/tools/firmware/hvmloader/acpi/README
> index 210d5ba..2b9d6e1 100644
> --- a/tools/firmware/hvmloader/acpi/README
> +++ b/tools/firmware/hvmloader/acpi/README
> @@ -1,11 +1,19 @@
> -ACPI Table for domain firmware
> +ACPI builder for domain firmware
>  
>  
> -INSTALL
> +BUILDING ACPI
>  -----------------
> -Simply make is OK.
> -# make 
> +Users of ACPI builder are expected to provide an include file that makes 
> available
> +the following:
> +* strncpy
> +* printf
> +* NULL
> +* test_bit
> +* offsetof
>  
> +When compiling build.c, the name of this include file should be given to
> +compiler as -DLIBACPI_STDUTILS=\"<filename>\". See 
> tools/firmware/hvmloader/Makefile
> +for an example.
>  
>  Note on DSDT Table
>  ------------------
> diff --git a/tools/firmware/hvmloader/acpi/build.c 
> b/tools/firmware/hvmloader/acpi/build.c
> index 2098920..1cd640d 100644
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -13,15 +13,13 @@
>   * GNU Lesser General Public License for more details.
>   */
>  
> +#include LIBACPI_STDUTILS
>  #include "acpi2_0.h"
>  #include "libacpi.h"
>  #include "ssdt_s3.h"
>  #include "ssdt_s4.h"
>  #include "ssdt_tpm.h"
>  #include "ssdt_pm.h"
> -#include "../config.h"
> -#include "../util.h"
> -#include "../vnuma.h"
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/params.h>
>  
> @@ -81,6 +79,9 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt 
> *ctxt,
>      struct hvm_info_table         *hvminfo = config->hvminfo;
>      int i, sz;
>  
> +    if ( config->lapic_id == NULL )
> +        return NULL;
> +
>      sz  = sizeof(struct acpi_20_madt);
>      sz += sizeof(struct acpi_20_madt_intsrcovr) * 16;
>      sz += sizeof(struct acpi_20_madt_ioapic);
> @@ -97,7 +98,7 @@ static struct acpi_20_madt *construct_madt(struct acpi_ctxt 
> *ctxt,
>      madt->header.oem_revision = ACPI_OEM_REVISION;
>      madt->header.creator_id   = ACPI_CREATOR_ID;
>      madt->header.creator_revision = ACPI_CREATOR_REVISION;
> -    madt->lapic_addr = LAPIC_BASE_ADDRESS;
> +    madt->lapic_addr = config->lapic_base_address;
>      madt->flags      = ACPI_PCAT_COMPAT;
>  
>      if ( config->table_flags & ACPI_HAS_IOAPIC )
> @@ -116,7 +117,7 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>                  intsrcovr->gsi    = 2;
>                  intsrcovr->flags  = 0x0;
>              }
> -            else if ( PCI_ISA_IRQ_MASK & (1U << i) )
> +            else if ( config->pci_isa_irq_mask & (1U << i) )
>              {
>                  /* PCI: active-low level-triggered. */
>                  intsrcovr->gsi    = i;
> @@ -135,8 +136,8 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>          memset(io_apic, 0, sizeof(*io_apic));
>          io_apic->type        = ACPI_IO_APIC;
>          io_apic->length      = sizeof(*io_apic);
> -        io_apic->ioapic_id   = IOAPIC_ID;
> -        io_apic->ioapic_addr = ioapic_base_address;
> +        io_apic->ioapic_id   = config->ioapic_id;
> +        io_apic->ioapic_addr = config->ioapic_base_address;
>  
>          lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
>      }
> @@ -152,7 +153,7 @@ static struct acpi_20_madt *construct_madt(struct 
> acpi_ctxt *ctxt,
>          lapic->length  = sizeof(*lapic);
>          /* Processor ID must match processor-object IDs in the DSDT. */
>          lapic->acpi_processor_id = i;
> -        lapic->apic_id = LAPIC_ID(i);
> +        lapic->apic_id = config->lapic_id(i);
>          lapic->flags = (test_bit(i, hvminfo->vcpu_online)
>                          ? ACPI_LOCAL_APIC_ENABLED : 0);
>          lapic++;
> @@ -240,7 +241,7 @@ static struct acpi_20_srat *construct_srat(struct 
> acpi_ctxt *ctxt,
>          processor->type     = ACPI_PROCESSOR_AFFINITY;
>          processor->length   = sizeof(*processor);
>          processor->domain   = config->numa.vcpu_to_vnode[i];
> -        processor->apic_id  = LAPIC_ID(i);
> +        processor->apic_id  = config->lapic_id(i);
>          processor->flags    = ACPI_LOCAL_APIC_AFFIN_ENABLED;
>          processor++;
>      }
> diff --git a/tools/firmware/hvmloader/acpi/libacpi.h 
> b/tools/firmware/hvmloader/acpi/libacpi.h
> index 3bcd226..d803139 100644
> --- a/tools/firmware/hvmloader/acpi/libacpi.h
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -84,6 +84,13 @@ struct acpi_config {
>  
>      /* RSDP address */
>      unsigned int rsdp;
> +
> +    /* x86-specific parameters */
> +    uint16_t (*lapic_id)(unsigned cpu);

Why uint16_t? Legacy APIC IDs in MADT are in an 8-bit field, and
x2APIC IDs require 32 bits. Depending on whether we want to be
able to re-use the same function for x2APIC support (once that
gets added) the return type should change accordingly. (But if we
were to re-use it, I guess a second parameter would then also be
needed.)

And then - down the road you're not planning to build a shared
library using this interface? Otherwise we'd need to consider some
compatibility aspects.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel