From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [RFC][PATCH] xen/apic: refactor error_interrupt
Date: Mon, 04 May 2015 19:14:59 +0800
Message-ID: <554754B3.8090104@intel.com>
References: <1430704984-6238-1-git-send-email-tiejun.chen@intel.com>
	<554744EC0200007800076298@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <554744EC0200007800076298@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: andrew.cooper3@citrix.com, keir@xen.org, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On 2015/5/4 16:07, Jan Beulich wrote:
>>>> On 04.05.15 at 04:03, <tiejun.chen@intel.com> wrote:
>> --- 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[] =
>
> If at all, then this should be const. But...
>
>> +{
>> +    "Send CS error",
>> +    "Receive CS error",
>> +    "Send accept error",
>> +    "Receive accept error",
>> +    "Reserved",
>> +    "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];
>> +}
>> +
>>   void error_interrupt(struct cpu_user_regs *regs)
>>   {
>>       unsigned long v, v1;
>> +    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);
>
> ... I'm not convinced - you're treating errors represented by lower
> bits "better" than higher ones. And if there are multiple errors, then
> spelling out one but not the other may end up being confusing.

I just think APIC shouldn't record these multiple errors at the same 
time because these kinds of errors seem be exclusive... But I don't find 
this is described in SDM so I think you're right.

Fortunately, looks Andrew guides me a approach to cover this point, so 
just let me go there.

Thanks
Tiejun

> These errors should be rare enough to warrant manually looking up
> the individual bits' meanings.
>
> Jan
>
>> -    /* 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",
>> +            smp_processor_id(), v , v1, reason);
>>   }
>>
>>   /*
>
>
>
>