On 1/23/2025 8:53 PM, Igor Mammedov wrote:
On Tue, 14 Jan 2025 21:01:27 +0800 Xiaoyao Li <xiaoyao.li@xxxxxxxxx> wrote: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@xxxxxxxxx> When level trigger isn't supported on x86 platform,it used to be level before this patch like for forever, so either we have a bug or above statement isn't correct.
This patch originated from Isaku.As you said, almost all of the ACPI info tell Level triggere before. I'm not sure just changing Level trigger to Edge trigger in ACPI would be sufficient. I need to learn and think carefully on it.
So I will drop this patch and the one before, for next sereis submission. And I will submit the new version of this one later separately.
Thanks!
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.in other words level trigger is not supported when TDX is enable, do I get it right?
yes.
If yes, then mention it in commit message and also mention followup patch which would use this. see my other comments below.IraSigned-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx> Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx> Acked-by: Gerd Hoffmann <kraxel@xxxxxxxxxx> --- 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)do not use negative naming, or even better pass AML_EDGE || AML_LEVEL as an argument here. the same applies to other places that use 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)do we really need piix4 machine to work with TDX?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,MADT change should be its own patch.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) {maybe, try to set flags as local var first, and then use it in build_xrupt_override() instead of rewriting/shifting existing blocks+ /* Force edge trigger */ + if (x86mc->apic_xrupt_override) { + build_xrupt_override(table_data, 0, 2, + /* Flags: active high, edge triggered */ + 1 | (1 << 2));pls point to spec where it comes from+ } + + for (i = x86mc->apic_xrupt_override ? 1 : 0; i < 16; i++) {^^^^^^^ before patch it was always starting from 1, so above does come from?+ 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)); + } + }and this is absolutely new hunk, perhaps its own patch with explanation why it's need+ } 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
