Skip to contentSkip to conversation

X86: Probe for PIC and set legacy_pic appropriately

PATCH In review K. Y. Srinivasan 2 participants 3 messages Apr 11, 2014

Subsystems (2)
x86 Hyper-V/Azure core and drivers
lore
Share & tools
lore

K. Y. Srinivasan

author
#0 lore raw

PATCH 1/1 X86: Probe for PIC and set legacy_pic appropriately

Probe for the existance of legacy PIC, if one does not exist, use the null_legacy_pic.

This patch implements the proposal put forth by H. Peter Anvin <[email protected]>.

Signed-off-by K. Y. Srinivasan <[email protected]>
---
 arch/x86/kernel/cpu/mshyperv.c |    2 --
 arch/x86/kernel/i8259.c        |   11 +++++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mshyperv.c +0 -2
index 47359f7..9ca9587 100644
@@ -133,8 +133,6 @@ static void __init ms_hyperv_init_platform(void)
 		printk(KERN_INFO "HyperV: LAPIC Timer Frequency: %#x\n",
 				lapic_timer_frequency);
 
-		printk(KERN_INFO "HyperV: Using null_legacy_pic\n");
-		legacy_pic = &null_legacy_pic;
 	}
 #endif
arch/x86/kernel/i8259.c +11 -0
index 2e977b5..0a57a19 100644
@@ -299,11 +299,22 @@ static void unmask_8259A(void)
 static void init_8259A(int auto_eoi)
 {
 	unsigned long flags;
+	unsigned char probe_val = 0xfb;
 
 	i8259A_auto_eoi = auto_eoi;
 
 	raw_spin_lock_irqsave(&i8259A_lock, flags);
 
+	/* Check to see if we have a PIC */
+	outb(probe_val, PIC_MASTER_IMR);
+	probe_val = inb(PIC_MASTER_IMR);
+	if (probe_val == 0xff) {
+		printk(KERN_INFO "Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
+		return;
+	}
+
 	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
 	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
Signature · 1 line
1.7.4.1
reviewer
#1 lore raw
Quoted 18 lines · K. Y. Srinivasan
arch/x86/kernel/i8259.c +4 -0
index 2e977b5..0a57a19 100644
@@ -299,11 +299,22 @@ static void unmask_8259A(void)
 static void init_8259A(int auto_eoi)
 {
 	unsigned long flags;
+	unsigned char probe_val = 0xfb;
 
 	i8259A_auto_eoi = auto_eoi;
 
 	raw_spin_lock_irqsave(&i8259A_lock, flags);
 
+	/* Check to see if we have a PIC */
+	outb(probe_val, PIC_MASTER_IMR);
+	probe_val = inb(PIC_MASTER_IMR);

arch/x86/kernel/i8259.c static void unmask_8259A(void) · line 320

Quoted 10 lines
arch/x86/kernel/i8259.c +7 -0
+	if (probe_val == 0xff) {
+		printk(KERN_INFO "Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
+		return;
+	}
+
 	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
 	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */

I think it is important to document what this actually means here. 0xfb is "mask all except for the cascade."

Arguably, we should do this after masking the slave PIC.

On a whole other subject... the whole __byte() logic is a just plain horrific piece of crap. Clearly whomever wrote it had never heard of a union.

I'm still horrifically confused, though, why we mask IRQ 2 (the cascade interrupt) in i8259.c, but we explicitly do *not* mask IRQ 2 in boot/pm.c (nor in the assembly code on which boot/pm.c was based.) In fact, it looks like we never unmask it. I'm guessing that the masking status of the cascade input simply doesn't matter.

At the very least, though, we shouldn't have 0xfb as a magic number, but use ~(1U << PIC_CASCADE_IR).

-hpa

author
#2 lore raw
Quoted 49 lines

-----Original Message-----
From: H. Peter Anvin [mailto:[email protected]]
Sent: Friday, April 11, 2014 3:36 PM
To: KY Srinivasan; [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: Thomas Gleixner
Subject: Re: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

Quoted 17 lines · K. Y. Srinivasan
arch/x86/kernel/i8259.c index renamed from arch/x86/kernel/i8259.c +0 -0

2e977b5..0a57a19 100644

arch/x86/kernel/i8259.c +4 -0
@@ -299,11 +299,22 @@ static void unmask_8259A(void)  static void
 init_8259A(int auto_eoi)  {
 	unsigned long flags;
+	unsigned char probe_val = 0xfb;
 
 	i8259A_auto_eoi = auto_eoi;
 
 	raw_spin_lock_irqsave(&i8259A_lock, flags);
 
+	/* Check to see if we have a PIC */
+	outb(probe_val, PIC_MASTER_IMR);
+	probe_val = inb(PIC_MASTER_IMR);

arch/x86/kernel/i8259.c static void unmask_8259A(void) static void · line 319

Quoted 10 lines
arch/x86/kernel/i8259.c +7 -0
+	if (probe_val == 0xff) {
+		printk(KERN_INFO "Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
+		return;
+	}
+
 	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
 	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */

I think it is important to document what this actually means here. 0xfb is "mask all except for the cascade."

Arguably, we should do this after masking the slave PIC.

On a whole other subject... the whole __byte() logic is a just plain horrific piece of crap. Clearly whomever wrote it had never heard of a union.

Quoted 8 lines

I'm still horrifically confused, though, why we mask IRQ 2 (the cascade interrupt) in i8259.c, but we explicitly do *not* mask IRQ 2 in boot/pm.c (nor in the assembly code on which boot/pm.c was based.) In fact, it looks like we never unmask it. I'm guessing that the masking status of the cascade input simply doesn't matter.

At the very least, though, we shouldn't have 0xfb as a magic number, but use ~(1U << PIC_CASCADE_IR).

I will get rid of this magic number as you have suggested.

K. Y

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�