|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 02/17] libacpi: new DSDT ACPI table for Q35
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?
>
>>
>> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
>
> 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.
>
>> 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
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
>
> 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?
Yes, sure.
>
>>
>> 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.
Will fix that.
> >> + */
>> + 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?
AFAIU, yes that's the goal. Since I'll revert the split and use #ifdef
directives in dsdt.asl this will be removed.
>
>> + {
>> + /*
>> + 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,
--
Thierry Escande | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |