This looks good to me after a first reading; I'm happy with it assuming it has been tested (sounds like it has if mboot.c32 now works).
CC'd hpa: if you could take a glance and see if it looks okay, that would be helpful too. As a side note, it might be nice if an IDT for COM32 would be mentioned as part of the interface in the syslinux documentation. I mostly followed doc/comboot.txt in the initial implementation, so I missed this entirely. Thanks, -- Daniel Verkamp On Tue, Jun 29, 2010 at 12:23 AM, Stefan Hajnoczi <[email protected]> wrote: > CCed Daniel Verkamp, who wrote the COMBOOT implementation. > > On Tue, Jun 29, 2010 at 2:18 AM, Geoff Lywood <[email protected]> wrote: >> COM32 binaries generally expect to run with interrupts enabled. Syslinux does >> so, and COM32 programs will execute cli/sti pairs when running a critical >> section, to provide mutual exclusion against BIOS interrupt handlers. >> Previously, under gPXE, the IDT was not valid, so any interrupt (e.g. a timer >> tick) would generally cause the machine to triple fault. >> >> This change introduces code to: >> - Create a valid IDT at the same location that syslinux uses >> - Create an "interrupt jump buffer", which contains small pieces of code that >> simply record the vector number and jump to a common handler >> - Thunk down to real mode and execute the BIOS's interrupt handler whenever >> an interrupt is received in a COM32 program >> - Switch IDTs and enable/disable interrupts when context switching to and >> from >> COM32 binaries >> >> Testing done: >> - Booted VMware ESX using a COM32 multiboot loader (mboot.c32) >> - Built with GDBSERIAL enabled, and tested breakpoints on int22 and com32_irq >> - Put the following code in a COM32 program: >> asm volatile ( "sti" ); >> while ( 1 ); >> Before this change, the machine would triple fault immediately. After this >> change, it hangs as expected. Under Bochs, it is possible to see the >> interrupt handler run, and the current time in the BIOS data area gets >> incremented. >> >> I have a feeling that Outlook is going to mangle this patch, so I'm >> attaching it as well. >> >> VMware, Inc. is providing this patch to you under the terms of the GPL >> version 2 and any later version. This patch is provided as is, with no >> warranties or support. VMware disclaims all liability in connection with the >> use/inability to use this patch. Any use of the attached is considered >> acceptance of the above. >> >> Signed-off-by: Geoff Lywood <[email protected]> >> --- >> src/arch/i386/image/com32.c | 73 >> +++++++++++++++++++++- >> src/arch/i386/include/comboot.h | 45 +++++++++++++ >> src/arch/i386/interface/syslinux/com32_call.c | 17 +++++ >> src/arch/i386/interface/syslinux/com32_wrapper.S | 28 ++++++++ >> 4 files changed, 162 insertions(+), 1 deletions(-) >> >> diff --git a/src/arch/i386/image/com32.c b/src/arch/i386/image/com32.c >> index 6ab347c..0f01027 100644 >> --- a/src/arch/i386/image/com32.c >> +++ b/src/arch/i386/image/com32.c >> @@ -42,6 +42,13 @@ FILE_LICENCE ( GPL2_OR_LATER ); >> >> struct image_type com32_image_type __image_type ( PROBE_NORMAL ); >> >> +struct idt_register com32_external_idtr = { >> + .limit = COM32_NUM_IDT_ENTRIES * sizeof ( struct idt_descriptor ) - >> 1, >> + .base = COM32_IDT >> +}; >> + >> +struct idt_register com32_internal_idtr; >> + >> /** >> * Execute COMBOOT image >> * >> @@ -89,9 +96,12 @@ static int com32_exec ( struct image *image ) { >> unregister_image ( image ); >> >> __asm__ __volatile__ ( >> + "sidt com32_internal_idtr\n\t" >> + "lidt com32_external_idtr\n\t" /* Set up IDT >> */ >> "movl %%esp, (com32_internal_esp)\n\t" /* Save >> internal virtual address space ESP */ >> "movl (com32_external_esp), %%esp\n\t" /* Switch to >> COM32 ESP (top of available memory) */ >> "call _virt_to_phys\n\t" /* Switch to >> flat physical address space */ >> + "sti\n\t" /* Enable >> interrupts */ >> "pushl %0\n\t" /* Pointer to >> CDECL helper function */ >> "pushl %1\n\t" /* Pointer to >> FAR call helper function */ >> "pushl %2\n\t" /* Size of low >> memory bounce buffer */ >> @@ -100,7 +110,9 @@ static int com32_exec ( struct image *image ) { >> "pushl %5\n\t" /* Pointer to >> the command line arguments */ >> "pushl $6\n\t" /* Number of >> additional arguments */ >> "call *%6\n\t" /* Execute >> image */ >> - "call _phys_to_virt\n\t" /* Switch >> back to internal virtual address space */ >> + "cli\n\t" /* Disable >> interrupts */ >> + "call _phys_to_virt\n\t" /* Switch >> back to internal virtual address space */ >> + "lidt com32_internal_idtr\n\t" /* Switch >> back to internal IDT (for debugging) */ >> "movl (com32_internal_esp), %%esp\n\t" /* Switch back >> to internal stack */ >> : >> : >> @@ -216,6 +228,60 @@ static int comboot_load_image ( struct image *image ) { >> } >> >> /** >> + * Prepare COM32 Interrupt Descriptor Table and Interrupt Jump Buffer >> + * @v image COM32 image >> + * @ret rc Return status code >> + */ >> +static int comboot_prepare_idt ( struct image * image ) { >> + struct idt_descriptor *idt; >> + struct ijb_entry *ijb; >> + physaddr_t com32_irq_wrapper_phys; >> + userptr_t buffer; >> + size_t memsz; >> + int i, rc; >> + >> + buffer = phys_to_user ( COM32_IDT ); >> + memsz = COM32_NUM_IDT_ENTRIES * sizeof ( struct idt_descriptor ); >> + rc = prep_segment ( buffer, memsz, memsz ); >> + if ( rc != 0 ) { >> + DBGC ( image, "COM32 %p: could not prepare IDT segment: >> %s\n", >> + image, strerror ( rc ) ); >> + return rc; >> + } >> + >> + buffer = phys_to_user ( COM32_IJB ); >> + memsz = COM32_NUM_IDT_ENTRIES * sizeof ( struct ijb_entry ); >> + rc = prep_segment ( buffer, memsz, memsz ); >> + if ( rc != 0 ) { >> + DBGC ( image, "COM32 %p: could not prepare IJB segment: >> %s\n", >> + image, strerror ( rc ) ); >> + return rc; >> + } >> + >> + idt = phys_to_virt ( COM32_IDT ); >> + ijb = phys_to_virt ( COM32_IJB ); >> + com32_irq_wrapper_phys = virt_to_phys ( com32_irq_wrapper ); >> + >> + for ( i = 0; i < COM32_NUM_IDT_ENTRIES; i++ ) { >> + uint32_t ijb_address = virt_to_phys ( &ijb[i] ); >> + >> + idt[i].offset_low = ijb_address & 0xFFFF; >> + idt[i].selector = PHYSICAL_CS; >> + idt[i].flags = IDT_INTERRUPT_GATE_FLAGS; >> + idt[i].offset_high = ijb_address >> 16; >> + >> + ijb[i].pusha_instruction = IJB_PUSHA; >> + ijb[i].mov_instruction = IJB_MOV_AL_IMM8; >> + ijb[i].mov_value = i; >> + ijb[i].jump_instruction = IJB_JMP_REL32; >> + ijb[i].jump_destination = com32_irq_wrapper_phys - >> + virt_to_phys ( &ijb[i + 1] ); >> + } >> + >> + return 0; >> +} >> + >> +/** >> * Prepare COM32 low memory bounce buffer >> * @v image COM32 image >> * @ret rc Return status code >> @@ -269,6 +335,11 @@ static int com32_load ( struct image *image ) { >> return rc; >> } >> >> + /* Set up IDT */ >> + if ( ( rc = comboot_prepare_idt ( image ) ) != 0 ) { >> + return rc; >> + } >> + >> /* Prepare bounce buffer segment */ >> if ( ( rc = comboot_prepare_bounce_buffer ( image ) ) != 0 ) { >> return rc; >> diff --git a/src/arch/i386/include/comboot.h >> b/src/arch/i386/include/comboot.h >> index 1232f0a..ce6990d 100644 >> --- a/src/arch/i386/include/comboot.h >> +++ b/src/arch/i386/include/comboot.h >> @@ -13,6 +13,50 @@ FILE_LICENCE ( GPL2_OR_LATER ); >> #include <setjmp.h> >> #include <gpxe/in.h> >> >> +/** Descriptor in a 32-bit IDT */ >> +struct idt_descriptor { >> + uint16_t offset_low; >> + uint16_t selector; >> + uint16_t flags; >> + uint16_t offset_high; >> +} PACKED; >> + >> +/** Operand for the LIDT instruction */ >> +struct idt_register { >> + uint16_t limit; >> + uint32_t base; >> +} PACKED; >> + >> +/** Entry in the interrupt jump buffer */ >> +struct ijb_entry { >> + uint8_t pusha_instruction; >> + uint8_t mov_instruction; >> + uint8_t mov_value; >> + uint8_t jump_instruction; >> + uint32_t jump_destination; >> +} PACKED; >> + >> +/** The x86 opcode for "pushal" */ >> +#define IJB_PUSHA 0x60 >> + >> +/** The x86 opcode for "movb $imm8,%al" */ >> +#define IJB_MOV_AL_IMM8 0xB0 >> + >> +/** The x86 opcode for "jmp rel32" */ >> +#define IJB_JMP_REL32 0xE9 >> + >> +/** Flags that specify a 32-bit interrupt gate with DPL=0 */ >> +#define IDT_INTERRUPT_GATE_FLAGS 0x8E00 >> + >> +/** Address of COM32 interrupt descriptor table */ >> +#define COM32_IDT 0x100000 >> + >> +/** Number of entries in a fully populated IDT */ >> +#define COM32_NUM_IDT_ENTRIES 256 >> + >> +/** Address of COM32 interrupt jump buffer */ >> +#define COM32_IJB 0x100800 >> + >> /** Segment used for COMBOOT PSP and image */ >> #define COMBOOT_PSP_SEG 0x07C0 >> >> @@ -109,6 +153,7 @@ extern void unhook_comboot_interrupts ( ); >> extern void com32_intcall_wrapper ( ); >> extern void com32_farcall_wrapper ( ); >> extern void com32_cfarcall_wrapper ( ); >> +extern void com32_irq_wrapper ( ); >> >> /* Resolve a hostname to an (IPv4) address */ >> extern int comboot_resolv ( const char *name, struct in_addr *address ); >> diff --git a/src/arch/i386/interface/syslinux/com32_call.c >> b/src/arch/i386/interface/syslinux/com32_call.c >> index d2c3f91..d271cc7 100644 >> --- a/src/arch/i386/interface/syslinux/com32_call.c >> +++ b/src/arch/i386/interface/syslinux/com32_call.c >> @@ -188,3 +188,20 @@ int __asmcall com32_cfarcall ( uint32_t proc, >> physaddr_t stack, size_t stacksz ) >> >> return eax; >> } >> + >> +/** >> + * IRQ handler >> + */ >> +void __asmcall com32_irq ( uint32_t vector ) { >> + uint32_t *ivt_entry = phys_to_virt( vector * 4 ); >> + >> + __asm__ __volatile__ ( >> + REAL_CODE ( "pushfw\n\t" >> + "pushw %%cs\n\t" >> + "pushw $com32_irq_return\n\t" >> + "pushl %0\n\t" >> + "retf\n" >> + "com32_irq_return:\n\t" ) >> + : /* no outputs */ >> + : "r" ( *ivt_entry ) ); >> +} >> diff --git a/src/arch/i386/interface/syslinux/com32_wrapper.S >> b/src/arch/i386/interface/syslinux/com32_wrapper.S >> index 5c5bd13..84d1571 100644 >> --- a/src/arch/i386/interface/syslinux/com32_wrapper.S >> +++ b/src/arch/i386/interface/syslinux/com32_wrapper.S >> @@ -22,6 +22,26 @@ FILE_LICENCE ( GPL2_OR_LATER ) >> .arch i386 >> .code32 >> >> + /* >> + * This code is entered after running the following sequence out of >> + * the interrupt jump buffer: >> + * >> + * pushal >> + * movb $vector, %al >> + * jmp com32_irq_wrapper >> + */ >> + >> + .globl com32_irq_wrapper >> +com32_irq_wrapper: >> + >> + movzxb %al,%eax >> + pushl %eax >> + movl $com32_irq, %eax >> + call com32_wrapper >> + popl %eax >> + popal >> + iret >> + >> .globl com32_farcall_wrapper >> com32_farcall_wrapper: >> >> @@ -43,10 +63,14 @@ com32_intcall_wrapper: >> /*jmp com32_wrapper*/ /* fall through */ >> >> com32_wrapper: >> + cli >> >> /* Switch to internal virtual address space */ >> call _phys_to_virt >> >> + /* Switch to internal IDT (if we have one for debugging) */ >> + lidt com32_internal_idtr >> + >> mov %eax, (com32_helper_function) >> >> /* Save external COM32 stack pointer */ >> @@ -74,9 +98,13 @@ com32_wrapper: >> movl %esp, (com32_internal_esp) >> movl (com32_external_esp), %esp >> >> + /* Switch to com32 IDT */ >> + lidt com32_external_idtr >> + >> /* Switch to external flat physical address space */ >> call _virt_to_phys >> >> + sti >> ret >> _______________________________________________ gPXE-devel mailing list [email protected] http://etherboot.org/mailman/listinfo/gpxe-devel
