Re: [PATCH 01/17] libacpi: Split dsdt.asl file and extract i440 specific parts

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 13, 2026 at 04:35:01PM +0000, Thierry Escande wrote:
> In order to factorize the common parts between i440 and q35 dsdt files,
> this patch splits dsdt.asl and put the i440 specific parts into
> dsdt_i400.asl.
> 
> This also makes use of #include directives instead of file
> concatenations to build the asl files.
> 
> Also, the anycpu asl files generation makes use of makefile pattern
> rules to avoid duplication for i440 and q35.
> 
> Becuase the LPC controller BDF (which differs between i440 and q35) must

s/Becuase/Because/

> be set at device declaration, it is still set in dsdt.asl by checking
> for a MACHINE_TYPE_I440 macro defined in dsdt_i400.asl.

s/i400/i440/

> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  tools/firmware/hvmloader/Makefile  |  2 +-
>  tools/firmware/hvmloader/ovmf.c    |  4 ++--
>  tools/firmware/hvmloader/seabios.c |  4 ++--
>  tools/firmware/hvmloader/util.c    |  4 ++--
>  tools/firmware/hvmloader/util.h    |  4 ++--
>  tools/libacpi/Makefile             | 10 ++++-----
>  tools/libacpi/dsdt.asl             | 25 ++++-----------------
>  tools/libacpi/dsdt_i440.asl        | 36 ++++++++++++++++++++++++++++++
>  8 files changed, 53 insertions(+), 36 deletions(-)
>  create mode 100644 tools/libacpi/dsdt_i440.asl
> 
> diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
> index 21de72187d..bdc33a877f 100644
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -78,7 +78,7 @@ rombios.o: roms.inc
>  smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\""
>  
>  ACPI_PATH = ../../libacpi
> -DSDT_FILES += dsdt_anycpu_qemu_xen.c
> +DSDT_FILES += dsdt_i440_anycpu_qemu_xen.c
>  ACPI_OBJS = $(patsubst %.c,%.o,$(DSDT_FILES)) build.o static_tables.o
>  $(ACPI_OBJS): CFLAGS += -iquote . -DLIBACPI_STDUTILS=\"$(CURDIR)/util.h\"
>  CFLAGS += -I$(ACPI_PATH)
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index 23610a0717..d264a50c73 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -119,8 +119,8 @@ static void ovmf_load(const struct bios_config *config,
>  static void ovmf_acpi_build_tables(void)
>  {
>      struct acpi_config config = {
> -        .dsdt_anycpu = dsdt_anycpu_qemu_xen,
> -        .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
> +        .dsdt_anycpu = dsdt_i440_anycpu_qemu_xen,
> +        .dsdt_anycpu_len = dsdt_i440_anycpu_qemu_xen_len,
>          .dsdt_15cpu = NULL, 
>          .dsdt_15cpu_len = 0
>      };
> diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
> index 444d118ddb..74b0406b5a 100644
> --- a/tools/firmware/hvmloader/seabios.c
> +++ b/tools/firmware/hvmloader/seabios.c
> @@ -90,8 +90,8 @@ static void seabios_acpi_build_tables(void)
>  {
>      uint32_t rsdp = (uint32_t)scratch_alloc(sizeof(struct acpi_20_rsdp), 0);
>      struct acpi_config config = {
> -        .dsdt_anycpu = dsdt_anycpu_qemu_xen,
> -        .dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len,
> +        .dsdt_anycpu = dsdt_i440_anycpu_qemu_xen,
> +        .dsdt_anycpu_len = dsdt_i440_anycpu_qemu_xen_len,
>          .dsdt_15cpu = NULL,
>          .dsdt_15cpu_len = 0,
>      };
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index e651342681..f1ed1eb48d 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -843,8 +843,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>      s = xenstore_read("platform/device-model", "");
>      if ( !strncmp(s, "qemu_xen", 9) )
>      {
> -        config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
> -        config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
> +        config->dsdt_anycpu = dsdt_i440_anycpu_qemu_xen;
> +        config->dsdt_anycpu_len = dsdt_i440_anycpu_qemu_xen_len;
>          config->dsdt_15cpu = NULL;
>          config->dsdt_15cpu_len = 0;
>      }
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 765a013ddd..3c5eeff5e7 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -381,8 +381,8 @@ extern struct e820map memory_map;
>  bool check_overlap(uint64_t start, uint64_t size,
>                     uint64_t reserved_start, uint64_t reserved_size);
>  
> -extern const unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[];
> -extern const int dsdt_anycpu_qemu_xen_len, dsdt_anycpu_len, dsdt_15cpu_len;
> +extern const unsigned char dsdt_i440_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[];
> +extern const int dsdt_i440_anycpu_qemu_xen_len, dsdt_anycpu_len, dsdt_15cpu_len;

While there you can make this unsigned int, like the dsdt_anycpu_len
field.

>  
>  unsigned long acpi_pages_allocated(void);
>  
> diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile
> index b21a64c6b4..d3d4bc9543 100644
> --- a/tools/libacpi/Makefile
> +++ b/tools/libacpi/Makefile
> @@ -11,7 +11,7 @@ endif
>  
>  MK_DSDT = $(ACPI_BUILD_DIR)/mk_dsdt
>  
> -C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c dsdt_pvh.c
> +C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_i440_anycpu_qemu_xen.c dsdt_pvh.c
>  C_SRC-$(CONFIG_ARM_64) = dsdt_anycpu_arm.c
>  DSDT_FILES ?= $(C_SRC-y)
>  C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES))
> @@ -39,18 +39,16 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl
>  $(MK_DSDT): mk_dsdt.c
>  	$(HOSTCC) $(HOSTCFLAGS) $(MKDSDT_CFLAGS-y) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c
>  
> -$(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl $(MK_DSDT)
> +$(ACPI_BUILD_DIR)/dsdt_%_anycpu_qemu_xen.asl: dsdt_%.asl dsdt.asl dsdt_acpi_info.asl $(MK_DSDT)
>  	# Remove last bracket
>  	awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
> -	cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
>  	$(MK_DSDT) --debug=$(debug) --dm-version qemu-xen >> $@.$(TMP_SUFFIX)
>  	mv -f $@.$(TMP_SUFFIX) $@
>  
>  # NB. awk invocation is a portable alternative to 'head -n -1'
> -$(ACPI_BUILD_DIR)/dsdt_%cpu.asl: dsdt.asl dsdt_acpi_info.asl  $(MK_DSDT)
> +$(ACPI_BUILD_DIR)/dsdt_%cpu.asl: dsdt_i440.asl dsdt.asl dsdt_acpi_info.asl  $(MK_DSDT)
>  	# Remove last bracket
>  	awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
> -	cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
>  	$(MK_DSDT) --debug=$(debug) --maxcpu $*  >> $@.$(TMP_SUFFIX)
>  	mv -f $@.$(TMP_SUFFIX) $@
>  
> @@ -65,7 +63,7 @@ $(ACPI_BUILD_DIR)/dsdt_anycpu_arm.asl: $(MK_DSDT)
>  	mv -f $@.$(TMP_SUFFIX) $@
>  
>  $(C_SRC): $(ACPI_BUILD_DIR)/%.c: $(ACPI_BUILD_DIR)/%.asl
> -	$(IASL) -vs -p $(ACPI_BUILD_DIR)/$*.$(TMP_SUFFIX) -tc $<
> +	$(IASL) -vs -I $(CURDIR) -p $(ACPI_BUILD_DIR)/$*.$(TMP_SUFFIX) -tc $<
>  	sed -e 's/AmlCode/$*/g' -e 's/_aml_code//g' $(ACPI_BUILD_DIR)/$*.hex > $@.$(TMP_SUFFIX)
>  	echo "int $*_len=sizeof($*);" >> $@.$(TMP_SUFFIX)
>  	mv -f $@.$(TMP_SUFFIX) $@
> diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl
> index 32b42f85ae..130826fdcc 100644
> --- a/tools/libacpi/dsdt.asl
> +++ b/tools/libacpi/dsdt.asl
> @@ -5,8 +5,6 @@
>   * Copyright (c) 2004, Intel Corporation.
>   */
>  
> -DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
> -{
>      Name (\PMBS, 0x0C00)
>      Name (\PMLN, 0x08)
>      Name (\IOB1, 0x00)
> @@ -199,7 +197,10 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
>  
>              Device (ISA)
>              {
> -                Name (_ADR, 0x00010000) /* device 1, fn 0 */
> +                /* Error will be raised if the machine type is not defined */
> +                #ifdef MACHINE_TYPE_I440
> +                    Name (_ADR, 0x00010000) /* device 1, fn 0 */
> +                #endif

I think we want the preprocessor directives not indented the same as
asl code, like we do in C, iow:

            Device (ISA)
            {
                /* Error will be raised if the machine type is not defined */
#ifdef MACHINE_TYPE_I440
                Name (_ADR, 0x00010000) /* device 1, fn 0 */
#endif

Same with the includes and defines below.

>  
>                  OperationRegion(PIRQ, PCI_Config, 0x60, 0x4)
>                  Scope(\) {
> @@ -329,23 +330,6 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
>                      })
>                  }
>  
> -                Device (FDC0)
> -                {
> -                    Name (_HID, EisaId ("PNP0700"))
> -                    Method (_STA, 0, NotSerialized)
> -                    {
> -                          Return (0x0F)
> -                    }
> -
> -                    Name (_CRS, ResourceTemplate ()
> -                    {
> -                        IO (Decode16, 0x03F0, 0x03F0, 0x01, 0x06)
> -                        IO (Decode16, 0x03F7, 0x03F7, 0x01, 0x01)
> -                        IRQNoFlags () {6}
> -                        DMA (Compatibility, NotBusMaster, Transfer8) {2}
> -                    })
> -                }

I need to look at the Q35 changes, but ff the only differences are the
Name() method at the top and this, we might as well use an #ifdef
here, and there's no need to split into so many separate files?  I
need to look at further patches.

The ACPI table generation stuff could certainly do with an overall
rework.

>                  Device (UAR1)
>                  {
>                      Name (_HID, EisaId ("PNP0501"))
> @@ -444,4 +428,3 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
>      Method(_PIC, 1) {
>          Store(Arg0, PICD)
>      }
> -}
> diff --git a/tools/libacpi/dsdt_i440.asl b/tools/libacpi/dsdt_i440.asl
> new file mode 100644
> index 0000000000..e80c454ad9
> --- /dev/null
> +++ b/tools/libacpi/dsdt_i440.asl
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-only */
> +/******************************************************************************
> + * DSDT for Xen with Qemu device model (for i440 machine)
> + *
> + * Copyright (c) 2004, Intel Corporation.
> + */
> +
> +DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
> +{
> +    #define MACHINE_TYPE_I440

IMO we probably want to define this outside of the DefinitionBlock
section, just after the copyright header.

> +
> +    #include "dsdt.asl"
> +
> +    Scope (\_SB.PCI0.ISA)
> +    {
> +         Device (FDC0)
> +         {
> +             Name (_HID, EisaId ("PNP0700"))
> +
> +             Method (_STA, 0, NotSerialized)
> +             {
> +                   Return (0x0F)
> +             }
> +
> +             Name (_CRS, ResourceTemplate ()
> +             {
> +                 IO (Decode16, 0x03F0, 0x03F0, 0x01, 0x06)
> +                 IO (Decode16, 0x03F7, 0x03F7, 0x01, 0x01)
> +                 IRQNoFlags () {6}
> +                 DMA (Compatibility, NotBusMaster, Transfer8) {2}
> +             })
> +        }
> +    }
> +
> +    #include "dsdt_acpi_info.asl"
> +}
> -- 
> 2.51.0
> 
> 
> 
> --
> Thierry Escande | Vates XCP-ng Developer
> 
> XCP-ng & Xen Orchestra - Vates solutions
> 
> web: https://vates.tech
>