On 2015/5/4 16:25, Andrew Cooper wrote:
On 04/05/2015 03:03, Tiejun Chen wrote:
Just make this readable while debug.
"debugging"
Fixed.
Signed-off-by: Tiejun Chen <tiejun.c...@intel.com>
In principle, I fully agree with the change. (I had an item on my todo
list to make a change like this anyway).
---
xen/arch/x86/apic.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 3217bdf..0b21ed1 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1319,28 +1319,37 @@ out: ;
* This interrupt should never happen with our APIC/SMP architecture
*/
+static const char *apic_fault_reasons[] =
static const char * const
It might be better to have "esr" in the name to make it clear that these
are string representations of the esr fields.
Sure.
+{
+ "Send CS error",
+ "Receive CS error",
+ "Send accept error",
+ "Receive accept error",
+ "Reserved",
"Redirectable IPI"
This field is not reserved on all processors which can run Xen.
Changed.
+ "Send illegal vector",
+ "Received illegal vector",
+ "Illegal register address",
+};
+
+static const char *apic_get_fault_reason(u8 fault_reason)
+{
+ return apic_fault_reasons[fault_reason];
This can easily overflow the array. I don't see much value in the wrapper.
Right.
+}
+
void error_interrupt(struct cpu_user_regs *regs)
{
unsigned long v, v1;
These can be unsigned int rather than unsigned long, saving a bit of space.
Okay.
+ const char *reason;
/* First tickle the hardware, only then report what went on. -- REW */
v = apic_read(APIC_ESR);
apic_write(APIC_ESR, 0);
v1 = apic_read(APIC_ESR);
ack_APIC_irq();
+ reason = apic_get_fault_reason(ffs(v1) - 1);
All but the bottom byte of v1 is reserved, and not guaranteed to be read
as 0.
Also, more than a single error can be latched at once, which this code
discards.
- /* Here is what the APIC error bits mean:
- 0: Send CS error
- 1: Receive CS error
- 2: Send accept error
- 3: Receive accept error
- 4: Reserved
- 5: Send illegal vector
- 6: Received illegal vector
- 7: Illegal register address
- */
- printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx)\n",
- smp_processor_id(), v , v1);
+ printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx):%s.\n",
Please can we avoid adding redundant punctuation to error messages. The
full stop serves no semantic purpose.
Also, please correct %d to %u as smp_processor_id() is an unsigned integer.
+ smp_processor_id(), v , v1, reason);
A better approach might be:
printk(KERN_DEBUG "APIC error on CPU%u: %02lx(%02lx)", ...)
for ( i = (1<<7); i; i >>= 1 )
if ( v1 & i )
printk(", %s", apic_fault_reasons[i]);
I guess this should be apic_fault_reasons[ffs(i) - 1].
printk("\n");
Which will decode all set bits, and guarantee not to access outside the
bounds of apic_fault_reasons.
Thanks you for all comments. So overall, what about this?
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 3217bdf..87684a2 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1319,9 +1319,21 @@ out: ;
* This interrupt should never happen with our APIC/SMP architecture
*/
+static const char * const fault_reasons_from_esr[] =
+{
+ "Send CS error",
+ "Receive CS error",
+ "Send accept error",
+ "Receive accept error",
+ "Redirectable IPI",
+ "Send illegal vector",
+ "Received illegal vector",
+ "Illegal register address",
+};
+
void error_interrupt(struct cpu_user_regs *regs)
{
- unsigned long v, v1;
+ unsigned int v, v1, i;
/* First tickle the hardware, only then report what went on. -- REW */
v = apic_read(APIC_ESR);
@@ -1329,18 +1341,12 @@ void error_interrupt(struct cpu_user_regs *regs)
v1 = apic_read(APIC_ESR);
ack_APIC_irq();
- /* Here is what the APIC error bits mean:
- 0: Send CS error
- 1: Receive CS error
- 2: Send accept error
- 3: Receive accept error
- 4: Reserved
- 5: Send illegal vector
- 6: Received illegal vector
- 7: Illegal register address
- */
- printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx)\n",
+ printk (KERN_DEBUG "APIC error on CPU%u: %02x(%02x)",
smp_processor_id(), v , v1);
+ for (i = (1<<7); i; i >>= 1)
+ if(v1 & i)
+ printk (KERN_DEBUG ", %s", fault_reasons_from_esr[ffs(i) - 1]);
+ printk (KERN_DEBUG ".\n");
}
/*
Thanks
Tiejun
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
