Re: [tip:x86/apic] x86, irq: Use cached IOAPIC entry instead of reading from hardware

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2014/11/28 3:32, Borislav Petkov wrote:
> On Wed, Nov 26, 2014 at 03:20:08PM -0800, tip-bot for Jiang Liu wrote:
>> Commit-ID:  fda7c08b1349cc4c65f8a5240b10f7e9938604b8
>> Gitweb:     http://git.kernel.org/tip/fda7c08b1349cc4c65f8a5240b10f7e9938604b8
>> Author:     Jiang Liu <jiang.liu@linux.intel.com>
>> AuthorDate: Tue, 25 Nov 2014 15:49:53 +0800
>> Committer:  Thomas Gleixner <tglx@linutronix.de>
>> CommitDate: Wed, 26 Nov 2014 23:52:49 +0100
>>
>> x86, irq: Use cached IOAPIC entry instead of reading from hardware
>>
>> Use cached IOAPIC entry instead of reading data from IOAPIC hardware
>> registers to improve performance.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
<snit>
>> @@ -1729,28 +1712,6 @@ static unsigned int startup_ioapic_irq(struct irq_data *data)
>>  	return was_pending;
>>  }
>>  
>> -static void __target_IO_APIC_irq(unsigned int irq, struct irq_cfg *cfg,
>> -				 struct mp_chip_data *data)
>> -{
>> -	int apic, pin;
>> -	struct irq_pin_list *entry;
>> -	u8 vector = cfg->vector;
>> -	unsigned int dest = SET_APIC_LOGICAL_ID(cfg->dest_apicid);
>> -
>> -	for_each_irq_pin(entry, data->irq_2_pin) {
>> -		unsigned int reg;
>> -
>> -		apic = entry->apic;
>> -		pin = entry->pin;
>> -
>> -		io_apic_write(apic, 0x11 + pin*2, dest);
>> -		reg = io_apic_read(apic, 0x10 + pin*2);
>> -		reg &= ~IO_APIC_REDIR_VECTOR_MASK;
>> -		reg |= vector;
>> -		io_apic_modify(apic, 0x10 + pin*2, reg);
>> -	}
>> -}
>> -
>>  atomic_t irq_mis_count;
>>  
>>  #ifdef CONFIG_GENERIC_PENDING_IRQ
>> @@ -1916,6 +1877,7 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
>>  {
>>  	struct irq_data *parent = irq_data->parent_data;
>>  	struct mp_chip_data *data = irq_data->chip_data;
>> +	struct irq_pin_list *entry;
>>  	struct irq_cfg *cfg;
>>  	unsigned long flags;
>>  	int ret;
>> @@ -1926,7 +1888,9 @@ static int ioapic_set_affinity(struct irq_data *irq_data,
>>  		cfg = irqd_cfg(irq_data);
>>  		data->entry.dest = SET_APIC_LOGICAL_ID(cfg->dest_apicid);
>>  		data->entry.vector = cfg->vector;
>> -		__target_IO_APIC_irq(irq_data->irq, cfg, irq_data->chip_data);
>> +		for_each_irq_pin(entry, data->irq_2_pin)
>> +			__ioapic_write_entry(entry->apic, entry->pin,
>> +					     data->entry);
>>  	}
> 
> So let me hold down what tglx and I have been staring at a whole day
> today. This commit breaks booting my AMD laptop with the error messages
> below. The machine ends up in the emergency shell, completely unusable.
> 
> When I revert the commit, x86/apic boots fine again. Btw, I was able to
> reproduce the same issue in a kvm guest so something's definitely wrong.
> 
> If I revert only the last hunk and do
> 
> 	__target_IO_APIC_irq(irq_data->irq, cfg, irq_data->chip_data);
> 
> instead and leave the loop over the irq pins in that function as the old
> icode did t, the problem disappears.
Hi Borislav,
	Thanks for tracking down to this line of change. I have no
platform to reproduce this bug, so could you please help to revert this
commit and apply following patch to get some data about IOAPIC entry?
Thanks!
Gerry
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index a915ee005148..cfada57140bd 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1736,7 +1736,9 @@ static void __target_IO_APIC_irq(unsigned int irq,
struct irq_cfg *cfg,
        struct irq_pin_list *entry;
        u8 vector = cfg->vector;
        unsigned int dest = SET_APIC_LOGICAL_ID(cfg->dest_apicid);
+       union entry_union eu;

+       eu.entry = data->entry;
        for_each_irq_pin(entry, data->irq_2_pin) {
                unsigned int reg;

@@ -1748,6 +1750,7 @@ static void __target_IO_APIC_irq(unsigned int irq,
struct irq_cfg *cfg,
                reg &= ~IO_APIC_REDIR_VECTOR_MASK;
                reg |= vector;
                io_apic_modify(apic, 0x10 + pin*2, reg);
+               pr_warn("ioapic%d pin%d, hardware reg %x, cached data
%x\n", apic, pin, reg, eu.w1);
        }
 }


> 
> I've been trying different things but nothing helped so far as to
> pinpoint where the problem lies. Thus this mail to hold down the current
> situation. More staring later, on a clear head.
> 
> [    3.465391] PM: Checking hibernation image partition /dev/sda2
> [    3.497559] PM: Hibernation image partition 8:2 present
> [    3.501859] PM: Looking for hibernation image.
> [    6.600378] usb 1-1: New USB device found, idVendor=0627, idProduct=0001
> [    6.608098] usb 1-1: New USB device strings: Mfr=1, Product=3, SerialNumber=5
> [    6.614747] usb 1-1: Product: QEMU USB Tablet
> [    6.617966] usb 1-1: Manufacturer: QEMU
> [    6.632779] usb 1-1: SerialNumber: 42
> [    7.648783] ata2.00: qc timeout (cmd 0xa0)
> [    7.655222] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
> [    7.818934] ata2.01: NODEV after polling detection
> [    7.849882] ata2.00: configured for MWDMA2
> [    8.585096] input: QEMU QEMU USB Tablet as /devices/pci0000:00/0000:00:01.2/usb1/1-1/1-1:1.0/0003:0627:0001.0001/input/input1
> [    8.598044] hid-generic 0003:0627:0001.0001: input,hidraw0: USB HID v0.01 Pointer [QEMU QEMU USB Tablet] on usb-0000:00:01.2-1/input0
> [   12.852654] ata2.00: qc timeout (cmd 0xa0)
> [   12.855798] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
> [   12.859555] ata2.00: limiting speed to MWDMA2:PIO3
> [   13.017632] ata2.01: NODEV after polling detection
> [   13.024501] ata2.00: configured for MWDMA2
> [   18.028681] ata2.00: qc timeout (cmd 0xa0)
> [   18.035697] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
> [   18.042919] ata2.00: disabled
> [   18.045671] ata2: soft resetting link
> [   18.206824] ata2.01: NODEV after polling detection
> [   18.211174] ata2: EH complete
> [   32.873120] ata1: lost interrupt (Status 0x50)
> [   32.881800] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
> [   32.886772] ata1.00: failed command: READ DMA
>