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

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

On Fri, Jun 12, 2026 at 12:01:00PM +0200, Thierry Escande wrote:
> 
> On 4/28/26 12:17, Roger Pau Monné wrote:
> > 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.
> 
> You're right, the dependency is on Rombios. And I though Rombios was
> only used for qemu-trad, my bad. So it implies to use Seabios or OVMF to
> have Q35 support. Is it something acceptable?

IIRC XenServer still uses RomBIOS with upstream QEMU, for
compatibility purposes.  I'm fine with just adding Q35 support with
SeaBIOS and OVMF, it seems justified to request a newer firmware if
you want to use a newer chipset.

> > 
> >>
> >> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
> > 
> > If the first SoB if from Alexey, the From: should also match.
> 
> Ok, I'll change authorship or signed-off order depending on how close it
> is from the original patch.

I'm not an expert, but I think as long as it's based on a patch from
Alexey you need to keep is SoB first.

> > 
> >> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> >> ---
> >>  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
> 
> Well, the iasl compiler doesn't support the defined() directive (at
> least the latest Debian version from 2025) so I'll rather use something
> like:
> 
> #define MACHINE_TYPE MACHINE_TYPE_XXX
> 
> #if MACHINE_TYPE == MACHINE_TYPE_XXX
>  ...
> #elif MACHINE_TYPE == MACHINE_TYPE_YYY
>  ...
> #endif

Oh, I see, I wasn't aware of it not supporting defined() when #ifdef
is available.  Kind of weird to support one but not the other.

> > 
> >> +    {
> >> +        /*
> >> +         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.
> 
> I'll add qemu copyright header
> 
> Would that be ok under the original copyright notice ?
> /******************************************************************
>  * Q35 part heavily inspired by q35-acpi-dsdt.dsl from Qemu
>  *
>  * Copyright (c) 2010 Isaku Yamahata
>  *                    yamahata at valinux co jp
>  */
> 
> 
> Regards,

Seems fine.

Thanks, Roger.