Re: [PATCH v3 19/35] x86/io_apic: Cleanup trigger/polarity helpers
On 10/11/20 09:59, David Woodhouse wrote:
Hm, attempting to reproduce this shows something else. Ever since
commit be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-
iommu api") in 5.5 the following stops working for me:
$ qemu-system-x86_64 -serial mon:stdio -kernel bzImage -machine
q35,accel=kvm,kernel-irqchip=split -m 2G -device amd-iommu,intremap=off -append
"console=ttyS0 apic=verbose debug" -display none
It hasn't got a hard drive but I can watch the SATA interrupts fail as
it probes the CD-ROM:
[7.403327] ata3.00: qc timeout (cmd 0xa1)
[7.405980] ata3.00: failed to IDENTIFY (I/O error, err_mask=0x4)
Adding 'iommu=off' to the kernel command line makes it work again, in
that it correctly panics at the lack of a root file system, quickly.
That might well be a QEMU bug though, AMD emulation is kinda experimental.
Paolo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 19/35] x86/io_apic: Cleanup trigger/polarity helpers
On Tue, 2020-11-10 at 01:31 -0500, Qian Cai wrote:
> On Sat, 2020-10-24 at 22:35 +0100, David Woodhouse wrote:
> > From: Thomas Gleixner
> >
> > 'trigger' and 'polarity' are used throughout the I/O-APIC code for handling
> > the trigger type (edge/level) and the active low/high configuration. While
> > there are defines for initializing these variables and struct members, they
> > are not used consequently and the meaning of 'trigger' and 'polarity' is
> > opaque and confusing at best.
> >
> > Rename them to 'is_level' and 'active_low' and make them boolean in various
> > structs so it's entirely clear what the meaning is.
> >
> > Signed-off-by: Thomas Gleixner
> > Signed-off-by: David Woodhouse
> > ---
> > arch/x86/include/asm/hw_irq.h | 6 +-
> > arch/x86/kernel/apic/io_apic.c | 244 +---
> > arch/x86/pci/intel_mid_pci.c| 8 +-
> > drivers/iommu/amd/iommu.c | 10 +-
> > drivers/iommu/intel/irq_remapping.c | 9 +-
> > 5 files changed, 130 insertions(+), 147 deletions(-)
>
> Reverting the rest of patchset up to this commit on next-20201109 fixed an
> endless soft-lockups issue booting an AMD server below. I noticed that the
> failed boots always has this IOMMU IO_PAGE_FAULT before those soft-lockups:
Hm, attempting to reproduce this shows something else. Ever since
commit be62dbf554c5 ("iommu/amd: Convert AMD iommu driver to the dma-
iommu api") in 5.5 the following stops working for me:
$ qemu-system-x86_64 -serial mon:stdio -kernel bzImage -machine
q35,accel=kvm,kernel-irqchip=split -m 2G -device amd-iommu,intremap=off -append
"console=ttyS0 apic=verbose debug" -display none
It hasn't got a hard drive but I can watch the SATA interrupts fail as
it probes the CD-ROM:
[7.403327] ata3.00: qc timeout (cmd 0xa1)
[7.405980] ata3.00: failed to IDENTIFY (I/O error, err_mask=0x4)
Adding 'iommu=off' to the kernel command line makes it work again, in
that it correctly panics at the lack of a root file system, quickly.
smime.p7s
Description: S/MIME cryptographic signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 19/35] x86/io_apic: Cleanup trigger/polarity helpers
On Sat, 2020-10-24 at 22:35 +0100, David Woodhouse wrote: > From: Thomas Gleixner > > 'trigger' and 'polarity' are used throughout the I/O-APIC code for handling > the trigger type (edge/level) and the active low/high configuration. While > there are defines for initializing these variables and struct members, they > are not used consequently and the meaning of 'trigger' and 'polarity' is > opaque and confusing at best. > > Rename them to 'is_level' and 'active_low' and make them boolean in various > structs so it's entirely clear what the meaning is. > > Signed-off-by: Thomas Gleixner > Signed-off-by: David Woodhouse > --- > arch/x86/include/asm/hw_irq.h | 6 +- > arch/x86/kernel/apic/io_apic.c | 244 +--- > arch/x86/pci/intel_mid_pci.c| 8 +- > drivers/iommu/amd/iommu.c | 10 +- > drivers/iommu/intel/irq_remapping.c | 9 +- > 5 files changed, 130 insertions(+), 147 deletions(-) Reverting the rest of patchset up to this commit on next-20201109 fixed an endless soft-lockups issue booting an AMD server below. I noticed that the failed boots always has this IOMMU IO_PAGE_FAULT before those soft-lockups: [ 3404.093354][T1] AMD-Vi: Interrupt remapping enabled [ 3404.098593][T1] AMD-Vi: Virtual APIC enabled [ 3404.107783][ T340] pci :00:14.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x address=0xfffdf8020200 flags=0x0008] [ 3404.120644][T1] AMD-Vi: Lazy IO/TLB flushing enabled [ 3404.126011][T1] PCI-DMA: Using software bounce buffering for IO (SWIOTLB) [ 3404.133173][T1] software IO TLB: mapped [mem 0x68dcf000-0x6cdcf000] (64MB) .config (if ever matters): https://cailca.coding.net/public/linux/mm/git/files/master/x86.config good boot dmesg (with the commits reverted): http://people.redhat.com/qcai/dmesg.txt == system info == Dell Poweredge R6415 AMD EPYC 7401P 24-Core Processor 24576 MB memory, 239 GB disk space [ OK ] Started Flush Journal to Persistent Storage. [ OK ] Started udev Kernel Device Manager. [ OK ] Started udev Coldplug all Devices. [ OK ] Started Monitoring of LVM2 mirrors,…sing dmeventd or progress polling. [ OK ] Reached target Local File Systems (Pre). Mounting /boot... [ OK ] Created slice system-lvm2\x2dpvscan.slice. [ 3740.376500][ T1058] XFS (sda1): Mounting V5 Filesystem [ 3740.438474][ T1058] XFS (sda1): Ending clean mount [ 3765.159433][C0] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [systemd:1] [ 3765.166929][C0] Modules linked in: acpi_cpufreq(+) ip_tables x_tables sd_mod ahci libahci tg3 bnxt_en megaraid_sas libata firmware_class libphy dm_mirror dm_region_hash dm_log dm_mod [ 3765.183576][C0] irq event stamp: 26230104 [ 3765.187954][C0] hardirqs last enabled at (26230103): [] asm_common_interrupt+0x1e/0x40 [ 3765.197873][C0] hardirqs last disabled at (26230104): [] sysvec_apic_timer_interrupt+0xa/0xa0 [ 3765.208303][C0] softirqs last enabled at (26202664): [] __do_softirq+0x61b/0x95d [ 3765.217699][C0] softirqs last disabled at (26202591): [] asm_call_irq_on_stack+0x12/0x20 [ 3765.227702][C0] CPU: 0 PID: 1 Comm: systemd Not tainted 5.10.0-rc2-next-20201109+ #2 [ 3765.235793][C0] Hardware name: Dell Inc. PowerEdge R6415/07YXFK, BIOS 1.9.3 06/25/2019 [ 3765.244065][C0] RIP: 0010:lock_acquire+0x1f4/0x820 lock_acquire at kernel/locking/lockdep.c:5404 [ 3765.249211][C0] Code: ff ff ff 48 83 c4 20 65 0f c1 05 a7 ba 9e 7e 83 f8 01 4c 8b 54 24 08 0f 85 60 04 00 00 41 52 9d 48 b8 00 00 00 00 00 fc ff df <48> 01 c3 c7 03 00 00 00 00 c7 43 08 00 00 00 00 48 8b0 [ 3765.268657][C0] RSP: 0018:c906f9f8 EFLAGS: 0246 [ 3765.274587][C0] RAX: dc00 RBX: 1920df42 RCX: 1920df28 [ 3765.282420][C0] RDX: 111020645769 RSI: RDI: 0001 [ 3765.290256][C0] RBP: 0001 R08: fbfff164cb10 R09: fbfff164cb10 [ 3765.298090][C0] R10: 0246 R11: fbfff164cb0f R12: 88812be555b0 [ 3765.305922][C0] R13: R14: R15: [ 3765.313750][C0] FS: 7f12bb8c59c0() GS:8881b700() knlGS: [ 3765.322537][C0] CS: 0010 DS: ES: CR0: 80050033 [ 3765.328985][C0] CR2: 7f0c2d828fd0 CR3: 00011868a000 CR4: 003506f0 [ 3765.336820][C0] Call Trace: [ 3765.339979][C0] ? rcu_read_unlock+0x40/0x40 [ 3765.344609][C0] __d_move+0x2a2/0x16f0 __seqprop_spinlock_assert at include/linux/seqlock.h:277 (inlined by) __d_move at fs/dcache.c:2861 [ 3765.348711][C0] ? d_move+0x47/0x70 [ 3765.352560][C0] ? _raw_spin_unlock+0x1a/0x30 [ 3765.357275][C0] d_move+0x47/0x70 write_seqcount_t_end at include/linux/seqlock.h:560 (inlined by) write_sequnlock at include/linux/seqlock.h:901 (inlined by) d_move at fs/dcache.c:2916 [ 3765.360951][C0] ? vfs_rename+0x9ac/0x1270
[PATCH v3 19/35] x86/io_apic: Cleanup trigger/polarity helpers
From: Thomas Gleixner
'trigger' and 'polarity' are used throughout the I/O-APIC code for handling
the trigger type (edge/level) and the active low/high configuration. While
there are defines for initializing these variables and struct members, they
are not used consequently and the meaning of 'trigger' and 'polarity' is
opaque and confusing at best.
Rename them to 'is_level' and 'active_low' and make them boolean in various
structs so it's entirely clear what the meaning is.
Signed-off-by: Thomas Gleixner
Signed-off-by: David Woodhouse
---
arch/x86/include/asm/hw_irq.h | 6 +-
arch/x86/kernel/apic/io_apic.c | 244 +---
arch/x86/pci/intel_mid_pci.c| 8 +-
drivers/iommu/amd/iommu.c | 10 +-
drivers/iommu/intel/irq_remapping.c | 9 +-
5 files changed, 130 insertions(+), 147 deletions(-)
diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index a4aeeaace040..517847a94dbe 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -47,9 +47,9 @@ enum irq_alloc_type {
struct ioapic_alloc_info {
int pin;
int node;
- u32 trigger : 1;
- u32 polarity : 1;
- u32 valid : 1;
+ u32 is_level: 1;
+ u32 active_low : 1;
+ u32 valid : 1;
struct IO_APIC_route_entry *entry;
};
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c6d92d2570d0..24a7bba7cbf4 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -89,12 +89,12 @@ struct irq_pin_list {
};
struct mp_chip_data {
- struct list_head irq_2_pin;
- struct IO_APIC_route_entry entry;
- int trigger;
- int polarity;
+ struct list_headirq_2_pin;
+ struct IO_APIC_route_entry entry;
+ boolis_level;
+ boolactive_low;
+ boolisa_irq;
u32 count;
- bool isa_irq;
};
struct mp_ioapic_gsi {
@@ -745,44 +745,7 @@ static int __init find_isa_irq_apic(int irq, int type)
return -1;
}
-#ifdef CONFIG_EISA
-/*
- * EISA Edge/Level control register, ELCR
- */
-static int EISA_ELCR(unsigned int irq)
-{
- if (irq < nr_legacy_irqs()) {
- unsigned int port = 0x4d0 + (irq >> 3);
- return (inb(port) >> (irq & 7)) & 1;
- }
- apic_printk(APIC_VERBOSE, KERN_INFO
- "Broken MPtable reports ISA irq %d\n", irq);
- return 0;
-}
-
-#endif
-
-/* ISA interrupts are always active high edge triggered,
- * when listed as conforming in the MP table. */
-
-#define default_ISA_trigger(idx) (IOAPIC_EDGE)
-#define default_ISA_polarity(idx) (IOAPIC_POL_HIGH)
-
-/* EISA interrupts are always polarity zero and can be edge or level
- * trigger depending on the ELCR value. If an interrupt is listed as
- * EISA conforming in the MP table, that means its trigger type must
- * be read in from the ELCR */
-
-#define default_EISA_trigger(idx) (EISA_ELCR(mp_irqs[idx].srcbusirq))
-#define default_EISA_polarity(idx) default_ISA_polarity(idx)
-
-/* PCI interrupts are always active low level triggered,
- * when listed as conforming in the MP table. */
-
-#define default_PCI_trigger(idx) (IOAPIC_LEVEL)
-#define default_PCI_polarity(idx) (IOAPIC_POL_LOW)
-
-static int irq_polarity(int idx)
+static bool irq_active_low(int idx)
{
int bus = mp_irqs[idx].srcbus;
@@ -791,90 +754,139 @@ static int irq_polarity(int idx)
*/
switch (mp_irqs[idx].irqflag & MP_IRQPOL_MASK) {
case MP_IRQPOL_DEFAULT:
- /* conforms to spec, ie. bus-type dependent polarity */
- if (test_bit(bus, mp_bus_not_pci))
- return default_ISA_polarity(idx);
- else
- return default_PCI_polarity(idx);
+ /*
+* Conforms to spec, ie. bus-type dependent polarity. PCI
+* defaults to low active. [E]ISA defaults to high active.
+*/
+ return !test_bit(bus, mp_bus_not_pci);
case MP_IRQPOL_ACTIVE_HIGH:
- return IOAPIC_POL_HIGH;
+ return false;
case MP_IRQPOL_RESERVED:
pr_warn("IOAPIC: Invalid polarity: 2, defaulting to low\n");
fallthrough;
case MP_IRQPOL_ACTIVE_LOW:
default: /* Pointless default required due to do gcc stupidity */
- return IOAPIC_POL_LOW;
+ return true;
}
}
#ifdef CONFIG_EISA
-static int eisa_irq_trigger(int idx, int bus, int trigger)
+/*
+ * EISA Edge/Level control register, EL
