Re: [PATCH v6 41/60] hw/i386: add option to forcibly report edge trigger in acpi tables

Xiaoyao Li
2025-01-14T13:01:37Z

On 12/13/2024 6:39 AM, Ira Weiny wrote:
> On Tue, Nov 05, 2024 at 01:23:49AM -0500, Xiaoyao Li wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> When level trigger isn't supported on x86 platform,
>> forcibly report edge trigger in acpi tables.
>
> This commit message is pretty sparse. I was thinking of suggesting to squash
> this with patch 40 but it occurred to me that perhaps these are split to accept
> TDX specifics from general functionality. Is that the case here? Is that true
> with other patches in the series? If so what other situations would require
> this in the generic code beyond TDX?

The goal is trying to avoid adding TDX specific all around QEMU. So we
are trying to add new general interface as a patch and TDX uses the
interface as another patch.

> Ira
>
>>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> hw/i386/acpi-build.c | 99 ++++++++++++++++++++++++++++---------------
>> hw/i386/acpi-common.c | 45 +++++++++++++++-----
>> 2 files changed, 101 insertions(+), 43 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 4967aa745902..d0a5bfc69e9a 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -888,7 +888,8 @@ static void build_dbg_aml(Aml *table)
>> aml_append(table, scope);
>> }
>>
>> -static Aml *build_link_dev(const char *name, uint8_t uid, Aml *reg)
>> +static Aml *build_link_dev(const char *name, uint8_t uid, Aml *reg,
>> + bool level_trigger_unsupported)
>> {
>> Aml *dev;
>> Aml *crs;
>> @@ -900,7 +901,10 @@ static Aml *build_link_dev(const char *name, uint8_t uid, Aml *reg)
>> aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
>>
>> crs = aml_resource_template();
>> - aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>> + aml_append(crs, aml_interrupt(AML_CONSUMER,
>> + level_trigger_unsupported ?
>> + AML_EDGE : AML_LEVEL,
>> + AML_ACTIVE_HIGH,
>> AML_SHARED, irqs, ARRAY_SIZE(irqs)));
>> aml_append(dev, aml_name_decl("_PRS", crs));
>>
>> @@ -924,7 +928,8 @@ static Aml *build_link_dev(const char *name, uint8_t uid, Aml *reg)
>> return dev;
>> }
>>
>> -static Aml *build_gsi_link_dev(const char *name, uint8_t uid, uint8_t gsi)
>> +static Aml *build_gsi_link_dev(const char *name, uint8_t uid,
>> + uint8_t gsi, bool level_trigger_unsupported)
>> {
>> Aml *dev;
>> Aml *crs;
>> @@ -937,7 +942,10 @@ static Aml *build_gsi_link_dev(const char *name, uint8_t uid, uint8_t gsi)
>>
>> crs = aml_resource_template();
>> irqs = gsi;
>> - aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>> + aml_append(crs, aml_interrupt(AML_CONSUMER,
>> + level_trigger_unsupported ?
>> + AML_EDGE : AML_LEVEL,
>> + AML_ACTIVE_HIGH,
>> AML_SHARED, &irqs, 1));
>> aml_append(dev, aml_name_decl("_PRS", crs));
>>
>> @@ -956,7 +964,7 @@ static Aml *build_gsi_link_dev(const char *name, uint8_t uid, uint8_t gsi)
>> }
>>
>> /* _CRS method - get current settings */
>> -static Aml *build_iqcr_method(bool is_piix4)
>> +static Aml *build_iqcr_method(bool is_piix4, bool level_trigger_unsupported)
>> {
>> Aml *if_ctx;
>> uint32_t irqs;
>> @@ -964,7 +972,9 @@ static Aml *build_iqcr_method(bool is_piix4)
>> Aml *crs = aml_resource_template();
>>
>> irqs = 0;
>> - aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL,
>> + aml_append(crs, aml_interrupt(AML_CONSUMER,
>> + level_trigger_unsupported ?
>> + AML_EDGE : AML_LEVEL,
>> AML_ACTIVE_HIGH, AML_SHARED, &irqs, 1));
>> aml_append(method, aml_name_decl("PRR0", crs));
>>
>> @@ -998,7 +1008,7 @@ static Aml *build_irq_status_method(void)
>> return method;
>> }
>>
>> -static void build_piix4_pci0_int(Aml *table)
>> +static void build_piix4_pci0_int(Aml *table, bool level_trigger_unsupported)
>> {
>> Aml *dev;
>> Aml *crs;
>> @@ -1011,12 +1021,16 @@ static void build_piix4_pci0_int(Aml *table)
>> aml_append(sb_scope, pci0_scope);
>>
>> aml_append(sb_scope, build_irq_status_method());
>> - aml_append(sb_scope, build_iqcr_method(true));
>> + aml_append(sb_scope, build_iqcr_method(true, level_trigger_unsupported));
>>
>> - aml_append(sb_scope, build_link_dev("LNKA", 0, aml_name("PRQ0")));
>> - aml_append(sb_scope, build_link_dev("LNKB", 1, aml_name("PRQ1")));
>> - aml_append(sb_scope, build_link_dev("LNKC", 2, aml_name("PRQ2")));
>> - aml_append(sb_scope, build_link_dev("LNKD", 3, aml_name("PRQ3")));
>> + aml_append(sb_scope, build_link_dev("LNKA", 0, aml_name("PRQ0"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKB", 1, aml_name("PRQ1"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKC", 2, aml_name("PRQ2"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKD", 3, aml_name("PRQ3"),
>> + level_trigger_unsupported));
>>
>> dev = aml_device("LNKS");
>> {
>> @@ -1025,7 +1039,9 @@ static void build_piix4_pci0_int(Aml *table)
>>
>> crs = aml_resource_template();
>> irqs = 9;
>> - aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL,
>> + aml_append(crs, aml_interrupt(AML_CONSUMER,
>> + level_trigger_unsupported ?
>> + AML_EDGE : AML_LEVEL,
>> AML_ACTIVE_HIGH, AML_SHARED,
>> &irqs, 1));
>> aml_append(dev, aml_name_decl("_PRS", crs));
>> @@ -1111,7 +1127,7 @@ static Aml *build_q35_routing_table(const char *str)
>> return pkg;
>> }
>>
>> -static void build_q35_pci0_int(Aml *table)
>> +static void build_q35_pci0_int(Aml *table, bool level_trigger_unsupported)
>> {
>> Aml *method;
>> Aml *sb_scope = aml_scope("_SB");
>> @@ -1150,25 +1166,41 @@ static void build_q35_pci0_int(Aml *table)
>> aml_append(sb_scope, pci0_scope);
>>
>> aml_append(sb_scope, build_irq_status_method());
>> - aml_append(sb_scope, build_iqcr_method(false));
>> + aml_append(sb_scope, build_iqcr_method(false, level_trigger_unsupported));
>>
>> - aml_append(sb_scope, build_link_dev("LNKA", 0, aml_name("PRQA")));
>> - aml_append(sb_scope, build_link_dev("LNKB", 1, aml_name("PRQB")));
>> - aml_append(sb_scope, build_link_dev("LNKC", 2, aml_name("PRQC")));
>> - aml_append(sb_scope, build_link_dev("LNKD", 3, aml_name("PRQD")));
>> - aml_append(sb_scope, build_link_dev("LNKE", 4, aml_name("PRQE")));
>> - aml_append(sb_scope, build_link_dev("LNKF", 5, aml_name("PRQF")));
>> - aml_append(sb_scope, build_link_dev("LNKG", 6, aml_name("PRQG")));
>> - aml_append(sb_scope, build_link_dev("LNKH", 7, aml_name("PRQH")));
>> + aml_append(sb_scope, build_link_dev("LNKA", 0, aml_name("PRQA"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKB", 1, aml_name("PRQB"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKC", 2, aml_name("PRQC"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKD", 3, aml_name("PRQD"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKE", 4, aml_name("PRQE"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKF", 5, aml_name("PRQF"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKG", 6, aml_name("PRQG"),
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_link_dev("LNKH", 7, aml_name("PRQH"),
>> + level_trigger_unsupported));
>>
>> - aml_append(sb_scope, build_gsi_link_dev("GSIA", 0x10, 0x10));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIB", 0x11, 0x11));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIC", 0x12, 0x12));
>> - aml_append(sb_scope, build_gsi_link_dev("GSID", 0x13, 0x13));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIE", 0x14, 0x14));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIF", 0x15, 0x15));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIG", 0x16, 0x16));
>> - aml_append(sb_scope, build_gsi_link_dev("GSIH", 0x17, 0x17));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIA", 0x10, 0x10,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIB", 0x11, 0x11,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIC", 0x12, 0x12,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSID", 0x13, 0x13,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIE", 0x14, 0x14,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIF", 0x15, 0x15,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIG", 0x16, 0x16,
>> + level_trigger_unsupported));
>> + aml_append(sb_scope, build_gsi_link_dev("GSIH", 0x17, 0x17,
>> + level_trigger_unsupported));
>>
>> aml_append(table, sb_scope);
>> }
>> @@ -1350,6 +1382,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> PCMachineState *pcms = PC_MACHINE(machine);
>> PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>> X86MachineState *x86ms = X86_MACHINE(machine);
>> + bool level_trigger_unsupported = x86ms->eoi_intercept_unsupported;
>> AcpiMcfgInfo mcfg;
>> bool mcfg_valid = !!acpi_get_mcfg(&mcfg);
>> uint32_t nr_mem = machine->ram_slots;
>> @@ -1382,7 +1415,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> if (pm->pcihp_bridge_en || pm->pcihp_root_en) {
>> build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>> }
>> - build_piix4_pci0_int(dsdt);
>> + build_piix4_pci0_int(dsdt, level_trigger_unsupported);
>> } else if (q35) {
>> sb_scope = aml_scope("_SB");
>> dev = aml_device("PCI0");
>> @@ -1426,7 +1459,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> if (pm->pcihp_bridge_en) {
>> build_x86_acpi_pci_hotplug(dsdt, pm->pcihp_io_base);
>> }
>> - build_q35_pci0_int(dsdt);
>> + build_q35_pci0_int(dsdt, level_trigger_unsupported);
>> }
>>
>> if (misc->has_hpet) {
>> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
>> index 0cc2919bb851..ad38a6b31162 100644
>> --- a/hw/i386/acpi-common.c
>> +++ b/hw/i386/acpi-common.c
>> @@ -103,6 +103,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
>> AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = oem_id,
>> .oem_table_id = oem_table_id };
>> + bool level_trigger_unsupported = x86ms->eoi_intercept_unsupported;
>>
>> acpi_table_begin(&table, table_data);
>> /* Local APIC Address */
>> @@ -124,18 +125,42 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>> IO_APIC_SECONDARY_ADDRESS, IO_APIC_SECONDARY_IRQBASE);
>> }
>>
>> - if (x86mc->apic_xrupt_override) {
>> - build_xrupt_override(table_data, 0, 2,
>> - 0 /* Flags: Conforms to the specifications of the bus */);
>> - }
>> + if (level_trigger_unsupported) {
>> + /* Force edge trigger */
>> + if (x86mc->apic_xrupt_override) {
>> + build_xrupt_override(table_data, 0, 2,
>> + /* Flags: active high, edge triggered */
>> + 1 | (1 << 2));
>> + }
>> +
>> + for (i = x86mc->apic_xrupt_override ? 1 : 0; i < 16; i++) {
>> + build_xrupt_override(table_data, i, i,
>> + /* Flags: active high, edge triggered */
>> + 1 | (1 << 2));
>> + }
>> +
>> + if (x86ms->ioapic2) {
>> + for (i = 0; i < 16; i++) {
>> + build_xrupt_override(table_data, IO_APIC_SECONDARY_IRQBASE + i,
>> + IO_APIC_SECONDARY_IRQBASE + i,
>> + /* Flags: active high, edge triggered */
>> + 1 | (1 << 2));
>> + }
>> + }
>> + } else {
>> + if (x86mc->apic_xrupt_override) {
>> + build_xrupt_override(table_data, 0, 2,
>> + 0 /* Flags: Conforms to the specifications of the bus */);
>> + }
>>
>> - for (i = 1; i < 16; i++) {
>> - if (!(x86ms->pci_irq_mask & (1 << i))) {
>> - /* No need for a INT source override structure. */
>> - continue;
>> + for (i = 1; i < 16; i++) {
>> + if (!(x86ms->pci_irq_mask & (1 << i))) {
>> + /* No need for a INT source override structure. */
>> + continue;
>> + }
>> + build_xrupt_override(table_data, i, i,
>> + 0xd /* Flags: Active high, Level Triggered */);
>> }
>> - build_xrupt_override(table_data, i, i,
>> - 0xd /* Flags: Active high, Level Triggered */);
>> }
>>
>> if (x2apic_mode) {
>> --
>> 2.34.1
>>