Messages in this thread | | | From | (Eric W. Biederman) | Date | Sun, 11 Feb 2018 23:11:41 -0600 | Subject | Re: [PATCH v3 0/5] x86/apic: Fix restoring boot irq mode in reboot and kexec/kdump |
| |
Dou Liyang <douly.fnst@cn.fujitsu.com> writes:
> Hi all, > > One thing confused me. > > The disconnect_bsp_APIC() may restore the interrupt delivery mode into > virtual wire mode. it uses the vector F as the spurious interrput, But, > IMO, using the vector 0xFF(SPURIOUS_APIC_VECTOR) may more suitable and > will give us more detail. Why the disconnect_bsp_APIC() use vector F > here?
I would say this needs a documentation search before changing this.
This code originates in: 208fb93162d5 ("[PATCH] kexec: x86_64: restore apic virtual wire mode on shutdown")
The example in the Multi-Processor Specification v1.4 shows setting up the SPIV to vector 0x0f.
I don't know what is canonical and what will interact best with DOS, and that erra of setup. The vector 0x0f seems an odd choice as it is below 0x20 putting it in the range of vectors that are reserved for processor exceptions.
The constant SPURIOUS_APIC_VECTOR is definitely not something we want to be using at this point as that is a linux specific setting and used when Linux is up and running. So it is completely inapplicable.
This is all about restoring how the apics were configured at boot time so it may be appropriate to copy and store this value, if it was not architectural.
At a practical level at this point I suspect we are ok as the setting of the SPIV this way has not caused any known problems in the last decade. If someone wants to dig through the archtectural documents and the real world practice and find a better value and explain the change I would not oppose it.
All I know for certain is using the constant SPURIOUS_APIC_VECTOR is completely inappropriate (as that constant is about how linux uses vectors) and thus the patch below is wrong.
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index 25ddf02598d2..550deaad6a9a 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -2130,7 +2130,7 @@ void disconnect_bsp_APIC(int virt_wire_setup) > value = apic_read(APIC_SPIV); > value &= ~APIC_VECTOR_MASK; > value |= APIC_SPIV_APIC_ENABLED; > - value |= 0xf; > + value |= SPURIOUS_APIC_VECTOR; > apic_write(APIC_SPIV, value); > > if (!virt_wire_setup) { >
Eric
| |