Re: [PATCH v2 09/10] x86/irqs: Move interrupt-stub generation out of C

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/05/14 16:06, Jan Beulich wrote:
>>>> On 16.05.14 at 12:39, <andrew.cooper3@citrix.com> wrote:
>>>>
>>>>
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -475,6 +475,12 @@ ENTRY(common_interrupt)
>>          callq do_IRQ
>>          jmp ret_from_intr
>>  
>> +ENTRY(reserved_exception)
>> +        SAVE_ALL CLAC
>> +        movq %rsp,%rdi
>> +        callq do_reserved_exception
>> +        jmp ret_from_intr
> Sadly this still doesn't take care of auto-detecting whether an error
> code got pushed.

It is not supposed to.

Each reserved exception has its own automatically generated stub which
pushes its trap value, so do_reserved_exception() can identify which
trap has been unexpectedly triggered.

Currently there are no reserved exceptions which push an error code.  I
will leave a warning comment by the generation code to take care with
the first pushq $0.

>
> Also - is there a particular reason you don't have this go though
> handle_exception?

I hadn't considered that.  Its a good idea.  We can:

* Automatically generate the reserved entry point, directed at
handle_exception
* Fully populate the exception_table, with several labels going towards
do_reserved_exception.
* Treat the double fault entry in exception_table as reserved.  We
certainly don't want to enter through it.

>
>> @@ -717,13 +713,13 @@ ENTRY(exception_table)
>>          .quad do_invalid_op
>>          .quad do_device_not_available
>>          .quad BAD_VIRT_ADDR         /* double_fault, IST entry */
>> -        .quad do_coprocessor_segment_overrun
>> +        .quad BAD_VIRT_ADDR         /* coproc_seg_overrun, reserved */
>>          .quad do_invalid_TSS
>>          .quad do_segment_not_present
>>          .quad do_stack_segment
>>          .quad do_general_protection
>>          .quad do_page_fault
>> -        .quad do_spurious_interrupt_bug
>> +        .quad BAD_VIRT_ADDR         /* PIC IRQ7 spurious, reserved */
> And the comment here didn't get much (if at all) adjusted either.

What about "Default PIC spurious interrupt, architecturally reserved" ?

>
>> --- /dev/null
>> +++ b/xen/arch/x86/x86_64/irqgen.S
>> @@ -0,0 +1,52 @@
>> +/* Automatically generated interrupt entry points */
>> +#include <asm/processor.h>
>> +#include <irq_vectors.h>
>> +
>> +.section ".init.rodata", "a", @progbits
>> +
>> +/* Table of automatically generated entry points.  One per vector. */
>> +GLOBAL(autogen_entrypoints)
>> +
>> +/* pop into the .init.rodata section and record an entry point. */
>> +.macro entrypoint ent
>> +    .pushsection ".init.rodata", "a", @progbits
> Actually it's quite pointless to repeat the attributes - they can't be
> different than in the first section declaration anyway.
>
> Also I think this more legible version would now better go into entry.S
> rather than having a new file for it.
>
> Jan
>

Sure.

~Andrew