Re: [PATCH v4 03/16] libxl/arm: Generate static ACPI DSDT table

[Julien Grall <julien.grall@arm.com>]

Hi Shannon,

On 16/08/2016 06:25, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> It uses static DSDT table like the way x86 uses. Currently the DSDT
> table only contains processor device objects and it generates the
> maximal objects which so far is 128.
>
> Also only check iasl for aarch64 in configure since ACPI on ARM32 is not
> supported.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  tools/configure               |  2 +-

The file tools/configure should not be modified manually. Instead you 
have to modify tools/configure.ac.

You can regenerate tools/configure, you can call ./autegen.sh. However, 
I would recommend you to not include the changes of configure and ask 
the committer to regenerate. This is because we use always use the same 
version of autotools to do generation in order to avoid spurious change.

>  tools/libacpi/Makefile        | 15 ++++++++++++-
>  tools/libacpi/mk_dsdt.c       | 51 ++++++++++++++++++++++++++++++++-----------
>  tools/libxl/Makefile          |  5 ++++-
>  tools/libxl/libxl_arm_acpi.c  |  5 +++++
>  xen/include/public/arch-arm.h |  3 +++
>  6 files changed, 65 insertions(+), 16 deletions(-)
>
> diff --git a/tools/configure b/tools/configure
> index 5b5dcce..48239c0 100755
> --- a/tools/configure
> +++ b/tools/configure
> @@ -7458,7 +7458,7 @@ then
>      as_fn_error $? "Unable to find xgettext, please install xgettext" "$LINENO" 5
>  fi
>  case "$host_cpu" in
> -i[3456]86|x86_64)
> +i[3456]86|x86_64|aarch64)
>      # Extract the first word of "iasl", so it can be a program name with args.
>  set dummy iasl; ac_word=$2
>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
> diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
> index d741ac5..7f50a33 100644
> --- a/tools/libacpi/Makefile
> +++ b/tools/libacpi/Makefile
> @@ -19,6 +19,7 @@ MK_DSDT = $(ACPI_BUILD_DIR)/mk_dsdt
>
>  # Sources to be generated
>  C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, dsdt_anycpu.c dsdt_15cpu.c  dsdt_anycpu_qemu_xen.c dsdt_pvh.c)
> +C_SRC += $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.c

Do we really want to generate dsdt_anycpu_arm.c even for x86? Similarly, 
do we want to generate x86 dsdt for ARM?

>  H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h)
>
>  vpath iasl $(PATH)
> @@ -32,7 +33,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl
>  	cd $(CURDIR)
>
>  $(MK_DSDT): mk_dsdt.c
> -	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c
> +	$(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c

It would be useful to mention either in the code or in the commit 
message why you added __XEN_TOOLS__ here.

>
>  $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl $(MK_DSDT)
>  	awk 'NR > 1 {print s} {s=$$0}' $< > $@
> @@ -62,6 +63,18 @@ $(ACPI_BUILD_DIR)/dsdt_pvh.c: iasl $(ACPI_BUILD_DIR)/dsdt_pvh.asl
>  	echo "int dsdt_pvh_len=sizeof(dsdt_pvh);" >>$@
>  	rm -f $(ACPI_BUILD_DIR)/$*.aml $(ACPI_BUILD_DIR)/$*.hex
>
> +$(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl: $(MK_DSDT)
> +	printf "DefinitionBlock (\"DSDT.aml\", \"DSDT\", 3, \"XenARM\", \"Xen DSDT\", 1)\n{" > $@
> +	$(MK_DSDT) --debug=$(debug) --arch arm >> $@
> +
> +$(ACPI_BUILD_DIR)/dsdt_anycpu_arm.c: iasl $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl
> +	cd $(ACPI_BUILD_DIR)
> +	iasl -vs -p $* -tc $(ACPI_BUILD_DIR)/$*.asl
> +	sed -e 's/AmlCode/$*/g' $*.hex >$@
> +	echo "int $*_len=sizeof($*);" >>$@
> +	rm -f $*.aml $*.hex
> +	cd $(CURDIR)
> +
>  iasl:
>  	@echo
>  	@echo "ACPI ASL compiler (iasl) is needed"
> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 7d76784..f3ab28f 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -18,6 +18,7 @@
>  #include <stdlib.h>
>  #include <stdbool.h>
>  #include <xen/hvm/hvm_info_table.h>
> +#include <xen/arch-arm.h>

arch-arm.h defines a lot of ARM specific constant. This is a call to 
misused them when built for x86.

Similarly, xen/hvm/hvm_info_table.h should not be included for ARM.

>
>  static unsigned int indent_level;
>  static bool debug = false;
> @@ -99,6 +100,7 @@ static struct option options[] = {
>      { "dm-version", 1, 0, 'q' },
>      { "debug", 1, 0, 'd' },
>      { "no-dm", 0, 0, 'n' },
> +    { "arch", 1, 0, 'a' },
>      { 0, 0, 0, 0 }
>  };
>
> @@ -106,7 +108,7 @@ int main(int argc, char **argv)
>  {
>      unsigned int slot, dev, intx, link, cpu, max_cpus = HVM_MAX_VCPUS;

Here an example why we should avoid to include x86 header for ARM. 
HVM_MAX_VCPUS is x86 specific.

>      dm_version dm_version = QEMU_XEN_TRADITIONAL;
> -    bool no_dm = 0;
> +    bool no_dm = 0, arch_is_arm = false;
>
>      for ( ; ; )
>      {
> @@ -145,6 +147,10 @@ int main(int argc, char **argv)
>          case 'n':
>              no_dm = 1;
>              break;
> +        case 'a':
> +            if (strcmp(optarg, "arm") == 0)
> +                arch_is_arm = true;
> +            break;
>          case 'd':
>              if (*optarg == 'y')
>                  debug = true;
> @@ -154,6 +160,9 @@ int main(int argc, char **argv)
>          }
>      }
>
> +    if (arch_is_arm)
> +        max_cpus = GUEST_MAX_VCPUS;
> +

And here GUEST_MAX_VCPUS is arm specific. I actually don't much 
understand why you added an option to mk_dsdt in order to generate ARM 
table. We will never build ARM table on x86, and vice versa.

>      /**** DSDT DefinitionBlock start ****/
>      /* (we append to existing DSDT definition block) */
>      indent_level++;
> @@ -161,19 +170,21 @@ int main(int argc, char **argv)
>      /**** Processor start ****/
>      push_block("Scope", "\\_SB");
>
> -    /* MADT checksum */
> -    stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1");
> -    push_block("Field", "MSUM, ByteAcc, NoLock, Preserve");
> -    indent(); printf("MSU, 8\n");
> -    pop_block();
> +    if (!arch_is_arm) {
> +        /* MADT checksum */
> +        stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1");
> +        push_block("Field", "MSUM, ByteAcc, NoLock, Preserve");
> +        indent(); printf("MSU, 8\n");
> +        pop_block();
>
> -    /* Processor object helpers. */
> -    push_block("Method", "PMAT, 2");
> -    push_block("If", "LLess(Arg0, NCPU)");
> -    stmt("Return", "ToBuffer(Arg1)");
> -    pop_block();
> -    stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
> -    pop_block();
> +        /* Processor object helpers. */
> +        push_block("Method", "PMAT, 2");
> +        push_block("If", "LLess(Arg0, NCPU)");
> +        stmt("Return", "ToBuffer(Arg1)");
> +        pop_block();
> +        stmt("Return", "Buffer() {0, 8, 0xff, 0xff, 0, 0, 0, 0}");
> +        pop_block();
> +    }
>
>      /* Define processor objects and control methods. */
>      for ( cpu = 0; cpu < max_cpus; cpu++)
> @@ -182,6 +193,12 @@ int main(int argc, char **argv)
>
>          stmt("Name", "_HID, \"ACPI0007\"");
>
> +        if (arch_is_arm) {
> +            stmt("Name", "_UID, %d", cpu);

_UID is not ARM specific. I am not sure why it is no added for x86.

> +            pop_block();
> +            continue;
> +        }
> +
>          /* Name this processor's MADT LAPIC descriptor. */
>          stmt("OperationRegion",
>               "MATR, SystemMemory, Add(\\_SB.MAPA, %d), 8", cpu*8);
> @@ -220,6 +237,14 @@ int main(int argc, char **argv)
>          pop_block();
>      }
>
> +    if (arch_is_arm) {
> +        pop_block();
> +        /**** Processor end ****/
> +        pop_block();
> +        /**** DSDT DefinitionBlock end ****/
> +        return 0;
> +    }
> +
>      /* Operation Region 'PRST': bitmask of online CPUs. */
>      stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
>      push_block("Field", "PRST, ByteAcc, NoLock, Preserve");
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 6139bed..ce356d0 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -90,7 +90,10 @@ acpi:
>
>  LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o libxl_x86_acpi.o
>  LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
> -LIBXL_OBJS-$(CONFIG_ARM_64) += libxl_arm_acpi.o
> +LIBXL_OBJS-$(CONFIG_ARM_64) += libxl_arm_acpi.o dsdt_anycpu_arm.o
> +
> +dsdt_anycpu_arm.c:
> +	$(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(shell pwd)
>
>  libxl_arm_acpi.o: libxl_arm_acpi.c
>  	$(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c
> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> index ec6cf08..087d028 100644
> --- a/tools/libxl/libxl_arm_acpi.c
> +++ b/tools/libxl/libxl_arm_acpi.c
> @@ -28,6 +28,11 @@ typedef uint64_t u64;
>  #include <acpi/acconfig.h>
>  #include <acpi/actbl.h>
>
> +_hidden
> +extern const unsigned char dsdt_anycpu_arm[];
> +_hidden
> +extern const int dsdt_anycpu_arm_len;
> +
>  int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>                          libxl__domain_build_state *state,
>                          struct xc_dom_image *dom)
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 0afd654..008a2a0 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -435,6 +435,9 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_RAM_BANK_BASES   { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>  #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>
> +/* Current supported guest VCPUs */
> +#define GUEST_MAX_VCPUS 128

The number of vCPUS per guest supported depends whether Xen has been 
built for ARM32 or ARM64.

Also, because now we have two different place to define the number of 
vCPUS (here and include/asm-arm/config.h) it might be possible to have 
them differ by mistake.

I am not sure how to avoid the 2 definitions, so I would add a 
BUILD_BUG_ON in Xen to make sure that MAX_VIRT_CPUS is always <= to 
GUEST_MAX_VCPUS.

> +
>  /* Interrupts */
>  #define GUEST_TIMER_VIRT_PPI    27
>  #define GUEST_TIMER_PHYS_S_PPI  29
>

Regards,

-- 
Julien Grall

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