Re: [PATCH 02/17] libacpi: new DSDT ACPI table for Q35

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 13, 2026 at 04:35:01PM +0000, Thierry Escande wrote:
> This patch adds the DSDT table for Q35 (new tools/libacpi/dsdt_q35.asl
> file). It only contains the specific Q35 parts that differ from i440).
> At the moment, these are:
> 
> - BDF location of LPC Controller
> - Minor changes related to FDC detection
> - Addition of _OSC method to inform OSPM about PCIe features supported
> 
> As we are still using 4 PCI router links and their corresponding
> device/register addresses are same (offset 0x60), no need to change PCI
> routing descriptions.
> 
> Note that '15cpu' ACPI tables are only applicable to qemu-traditional
> (which have no support for Q35), so we need to use 'anycpu' version only.

Is the above statement fully accurate?  It seems like 15cpu tables are
used with rombios, so the dependency is not on qemu-trad, but rather
rombios?

If it's truly only dependent on qemu-trad then we should remove those,
as we have removed qemu-trad.

> 
> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>

If the first SoB if from Alexey, the From: should also match.

> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
> ---
>  tools/firmware/hvmloader/Makefile |   2 +-
>  tools/libacpi/Makefile            |   2 +-
>  tools/libacpi/dsdt.asl            |   3 +
>  tools/libacpi/dsdt_q35.asl        | 130 ++++++++++++++++++++++++++++++
>  4 files changed, 135 insertions(+), 2 deletions(-)
>  create mode 100644 tools/libacpi/dsdt_q35.asl
> 
> diff --git a/tools/firmware/hvmloader/Makefile 
> b/tools/firmware/hvmloader/Makefile
> index bdc33a877f..99f045efaa 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_i440_anycpu_qemu_xen.c
> +DSDT_FILES += dsdt_i440_anycpu_qemu_xen.c dsdt_q35_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/libacpi/Makefile b/tools/libacpi/Makefile
> index d3d4bc9543..e6c4a3fd8b 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_i440_anycpu_qemu_xen.c 
> dsdt_pvh.c
> +C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c dsdt_i440_anycpu_qemu_xen.c 
> dsdt_q35_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))
> diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl
> index 130826fdcc..dc764881c9 100644
> --- a/tools/libacpi/dsdt.asl
> +++ b/tools/libacpi/dsdt.asl
> @@ -201,6 +201,9 @@
>                  #ifdef MACHINE_TYPE_I440
>                      Name (_ADR, 0x00010000) /* device 1, fn 0 */
>                  #endif
> +                #ifdef MACHINE_TYPE_Q35
> +                    Name (_ADR, 0x001f0000) /* device 31, fn 0 */
> +                #endif

You possibly want to do:

#ifdef ...
#elif defined(...)
#else
#error ...
#endif

But seeing the difference is only for the address, why not do:

#define ISA_DEV_SBDF 0x00010000
...
Name (_ADR, ISA_DEV_SBDF)
...

And avoid the ifdef mess?

>  
>                  OperationRegion(PIRQ, PCI_Config, 0x60, 0x4)
>                  Scope(\) {
> diff --git a/tools/libacpi/dsdt_q35.asl b/tools/libacpi/dsdt_q35.asl
> new file mode 100644
> index 0000000000..7cefe63506
> --- /dev/null
> +++ b/tools/libacpi/dsdt_q35.asl
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: LGPL-2.1-only */
> +/******************************************************************************
> + * DSDT for Xen with Qemu device model (for Q35 machine)
> + */
> +
> +DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
> +{
> +    #define MACHINE_TYPE_Q35
> +
> +    #include "dsdt.asl"
> +
> +    Scope (\_SB.PCI0)
> +    {
> +       /* _OSC, modified from ASL sample in ACPI spec */
> +       Name (SUPP, 0) /* PCI _OSC Support Field value */
> +       Name (CTRL, 0) /* PCI _OSC Control Field value */
> +       Method (_OSC, 4) {
> +           /* Create DWORD-addressable fields from the Capabilities Buffer */
> +           CreateDWordField (Arg3, 0, CDW1)
> +
> +           /* Switch by UUID.
> +            * Only PCI Host Bridge Device capabilities UUID used for now

Comment style, in Xen we use:

/*
 * Switch by UIID.
 * Only PCI Host Bridge Device capabilities UUID used for now.
 */

The opening and closing lines are standalone.  Also missing a full
stop on the last line.  The rest of the comments below also need
adjusting.

> +            */
> +           If (LEqual (Arg0, ToUUID 
> ("33DB4D5B-1FF7-401C-9657-7441C03DD766"))) {
> +               /* Create DWORD-addressable fields from the Capabilities 
> Buffer */
> +               CreateDWordField (Arg3, 4, CDW2)
> +               CreateDWordField (Arg3, 8, CDW3)
> +
> +               /* Save Capabilities DWORD2 & 3 */
> +               Store (CDW2, SUPP)
> +               Store (CDW3, CTRL)
> +
> +               /* Validate Revision DWORD */
> +               If (LNotEqual (Arg1, One)) {
> +                   /* Unknown revision */
> +                   /* Support and Control DWORDs will be returned anyway */
> +                   Or (CDW1, 0x08, CDW1)
> +               }
> +
> +               /* Control field bits are:
> +                * bit 0    PCI Express Native Hot Plug control
> +                * bit 1    SHPC Native Hot Plug control
> +                * bit 2    PCI Express Native Power Management Events control
> +                * bit 3    PCI Express Advanced Error Reporting control
> +                * bit 4    PCI Express Capability Structure control
> +                */
> +
> +               /* Always allow native PME, AER (no dependencies)
> +                * Never allow SHPC (no SHPC controller in this system)
> +                * Do not allow PCIe Capability Structure control for now
> +                * Also, ACPI hotplug is used for now instead of PCIe
> +                * Native Hot Plug
> +                */
> +               And (CTRL, 0x0C, CTRL)
> +
> +               If (LNotEqual (CDW3, CTRL)) {
> +                   /* Some of Capabilities bits were masked */
> +                   Or (CDW1, 0x10, CDW1)
> +               }
> +               /* Update DWORD3 in the buffer */
> +               Store (CTRL, CDW3)

This looks equal to the QEMU code FWIW.

> +           } Else {
> +               Or (CDW1, 4, CDW1) /* Unrecognized UUID */
> +           }
> +           Return (Arg3)
> +       }
> +       /* end of _OSC */
> +    }
> +
> +    /****************************************************************
> +     * LPC ISA bridge
> +     ****************************************************************/

I would use a normal one-line comment here: /* LPCI ISA Bridge */

Has any of this been picked up from the QEMU asl files?  Asking
because the above comment looks to be verbatim copied from the QEMU
file, and we then need to carry their copyright notice, which is not
done in this patch.

> +
> +    Scope (\_SB.PCI0.ISA)

AFAICT this is adding more content to the ISA device already defined
in dsdt.asl?

> +    {
> +        /*
> +         LPC ISA bridge
> +
> +         PCI Interrupt Routing Register 2 (PIRQE..PIRQH) cannot be
> +         used because of existing Xen IRQ limitations (4 PCI links
> +         only)
> +        */

Right, and PIRQA..PIRQD is already defined in the generic dsdt.asl.
Might be worth mentioning, otherwise the block looks incomplete.

> +
> +        /* LPC_I/O: I/O Decode Ranges Register */
> +        OperationRegion (LPCD, PCI_Config, 0x80, 0x2)
> +        Field (LPCD, AnyAcc, NoLock, Preserve) {
> +            COMA,   3,
> +                ,   1,
> +            COMB,   3,
> +
> +            Offset(0x01),
> +            LPTD,   2,
> +                ,   2,
> +            FDCD,   2
> +        }
> +
> +        /* LPC_EN: LPC I/F Enables Register */
> +        OperationRegion(LPCE, PCI_Config, 0x82, 0x2)
> +        Field(LPCE, AnyAcc, NoLock, Preserve) {
> +            CAEN,   1,
> +            CBEN,   1,
> +            LPEN,   1,
> +            FDEN,   1
> +        }
> +
> +        Device (FDC0)
> +        {
> +            Name (_HID, EisaId ("PNP0700"))
> +            Method (_STA, 0, NotSerialized)
> +            {
> +                Store (FDEN, Local0)
> +                If (LEqual (Local0, 0)) {
> +                    Return (0x00)
> +                } Else {
> +                    Return (0x0F)
> +                }
> +           }
> +
> +           Name (_CRS, ResourceTemplate ()
> +           {
> +               IO (Decode16, 0x03F2, 0x03F2, 0x00, 0x04)
> +               IO (Decode16, 0x03F7, 0x03F7, 0x00, 0x01)
> +               IRQNoFlags () {6}
> +               DMA (Compatibility, NotBusMaster, Transfer8) {2}
> +           })
> +        }
> +    }

This seem to match the blocks in QEMU, so it's likely fine.

Thanks, Roger.