Re: [PATCH] Proof of concept interrupt wrapping

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3005 bytes --]

Robert Millan wrote:
> On Thu, Dec 31, 2009 at 01:28:30PM +0100, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>> Hello. We were discussing with Robert how to move BIOS interrupt
>> routines out of kernel. There are following possibilities:
>> 1) Have a .lowmem section in every concerned module which will always be
>> placed in low memory. Currently in experimental.
>> Advantages:
>>   a) moving functions to modules is straightforward
>>   b) functions grow neither in size nor in complexity
>> Disadvantages:
>>   c) needs lowmem allocators in core
>> 2) Make every function needing bios interrupts setup its own trampoline.
>> Due to complexity of trampolines it's not a real option
>> 3) Have an universal routine grub_interrupt (int intno, struct
>> grub_cpu_interrupt_regs *regs) which will be used by C routine to do the
>> interrupt calls. This would move the complexity from asm to C.
>> Advantages:
>> a) simplicity in core
>> b) complexity moved to a more readable language
>> c)  we can also rename grub_interrupt to grub_interrupt_real and make
>> grub_interrupt dprintf registers before and after the call. This would
>> make debugging BIOS quirks easier.
>> Disadvantages:
>> a) Moving functions needs effort
>> b) C functions are probably bigger but it may be offset by possibility
>> of inlining functions
>> c) repeadetly changing from/to real mode is an overhead when executing
>> multiple interrupts in series. Fortunately this condition is rare in our
>> codebase and is only on non performance-critical parts like halting.
>> d) Some functions aren't covered by this. At least grub_pxe_call is in
>> this case. But we can use method 2 for them
>>     
>
> We could diminish #1.c with ifdef GRUB_MACHINE_PCBIOS, but it's still ugly.
> I like #3 a lot more.
>
> As for C being bigger than asm, it's argueable, taking into account that
> we have regparm, function alignment hacks, and gcc size optimizations :-)
>
> In any case #3 looks a lot cleaner.
>
> Some comments about the patch:
>
>   
>> +struct grub_cpu_int_registers
>> +{
>> +  grub_uint16_t bx;
>> +  grub_uint16_t es;
>> +  grub_uint16_t cx;
>> +  grub_uint16_t ax;
>> +  grub_uint16_t dx;
>> +  grub_uint16_t ds;
>> +  grub_uint16_t di;
>> +  grub_uint16_t flags;
>> +  grub_uint16_t si;
>> +};
>>     
>
> This structure is named in generic way, but its member names are CPU-specific.
> Is it useful to make this generic?  In practice, it will be impossible for
> CPU-independant code to use it.
>   
I changed it to _bios_
> I think it would be better to admit that this is a pure ad-hoc kludge.  It's
> not beautiful, but it's *much* better than what we have now.
>
>   
I added 32-bit register support since it's needed for APM. Can wrapper
be merged to mainline or is it better for it to be in a separate branch
and be merged with first bunch of BIOS code going out of kernel?


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: intwrap.diff --]
[-- Type: text/x-diff; name="intwrap.diff", Size: 9156 bytes --]

=== modified file 'conf/i386-pc.rmk'
--- conf/i386-pc.rmk	2010-01-15 20:11:51 +0000
+++ conf/i386-pc.rmk	2010-01-17 20:16:27 +0000
@@ -64,7 +64,7 @@
 	partition.h msdos_partition.h reader.h symbol.h term.h time.h types.h \
 	machine/biosdisk.h machine/boot.h machine/console.h machine/init.h \
 	machine/memory.h machine/loader.h machine/vga.h machine/vbe.h \
-	machine/kernel.h machine/pxe.h i386/pit.h list.h handler.h command.h i18n.h
+	machine/kernel.h machine/pxe.h i386/pit.h list.h handler.h command.h i18n.h machine/int.h
 kernel_img_CFLAGS = $(COMMON_CFLAGS)  $(TARGET_IMG_CFLAGS)
 kernel_img_ASFLAGS = $(COMMON_ASFLAGS)
 kernel_img_LDFLAGS = $(COMMON_LDFLAGS) $(TARGET_IMG_LDFLAGS)$(GRUB_KERNEL_MACHINE_LINK_ADDR) $(COMMON_CFLAGS)

=== modified file 'disk/i386/pc/biosdisk.c'
--- disk/i386/pc/biosdisk.c	2010-01-04 23:30:27 +0000
+++ disk/i386/pc/biosdisk.c	2010-01-17 20:16:27 +0000
@@ -19,6 +19,7 @@
 #include <grub/machine/biosdisk.h>
 #include <grub/machine/memory.h>
 #include <grub/machine/kernel.h>
+#include <grub/machine/int.h>
 #include <grub/disk.h>
 #include <grub/dl.h>
 #include <grub/mm.h>
@@ -28,6 +29,59 @@
 #include <grub/term.h>
 
 static int cd_drive = 0;
+static int grub_biosdisk_rw_int13_extensions (int ah, int drive, void *dap);
+
+static int grub_biosdisk_get_num_floppies (void)
+{
+  struct grub_bios_int_registers regs;
+  int drive;
+
+  /* reset the disk system first */
+  regs.eax = 0;
+  regs.edx = 0;
+  regs.flags = GRUB_CPU_INT_FLAGS_DEFAULT;
+
+  grub_bios_interrupt (0x13, &regs);
+
+  for (drive = 0; drive < 2; drive++)
+    {
+      regs.flags = GRUB_CPU_INT_FLAGS_DEFAULT | GRUB_CPU_INT_FLAGS_CARRY;
+      regs.edx = drive;
+
+      /* call GET DISK TYPE */
+      regs.eax = 0x1500;
+      grub_bios_interrupt (0x13, &regs);
+      if (regs.flags & GRUB_CPU_INT_FLAGS_CARRY)
+	break;
+
+      /* check if this drive exists */
+      if (!(regs.eax & 0x300))
+	break;
+    }
+
+  return drive;
+}
+
+/*
+ *   Call IBM/MS INT13 Extensions (int 13 %ah=AH) for DRIVE. DAP
+ *   is passed for disk address packet. If an error occurs, return
+ *   non-zero, otherwise zero.
+ */
+
+static int 
+grub_biosdisk_rw_int13_extensions (int ah, int drive, void *dap)
+{
+  struct grub_bios_int_registers regs;
+  regs.eax = ah << 8;
+  /* compute the address of disk_address_packet */
+  regs.ds = (((grub_addr_t) dap) & 0xffff0000) >> 4;
+  regs.esi = (((grub_addr_t) dap) & 0xffff);
+  regs.edx = drive;
+  regs.flags = GRUB_CPU_INT_FLAGS_DEFAULT;
+
+  grub_bios_interrupt (0x13, &regs);
+  return (regs.eax >> 8) & 0xff;
+}
 
 static int
 grub_biosdisk_get_drive (const char *name)

=== modified file 'include/grub/i386/pc/biosdisk.h'
--- include/grub/i386/pc/biosdisk.h	2009-06-10 21:04:23 +0000
+++ include/grub/i386/pc/biosdisk.h	2009-12-31 11:49:34 +0000
@@ -106,7 +106,6 @@
   grub_uint64_t block;
 } __attribute__ ((packed));
 
-int EXPORT_FUNC(grub_biosdisk_rw_int13_extensions) (int ah, int drive, void *dap);
 int EXPORT_FUNC(grub_biosdisk_rw_standard) (int ah, int drive, int coff, int hoff,
 			       int soff, int nsec, int segment);
 int EXPORT_FUNC(grub_biosdisk_check_int13_extensions) (int drive);
@@ -118,7 +117,6 @@
 					 unsigned long *cylinders,
 					 unsigned long *heads,
 					 unsigned long *sectors);
-int EXPORT_FUNC(grub_biosdisk_get_num_floppies) (void);
 
 void grub_biosdisk_init (void);
 void grub_biosdisk_fini (void);

=== added file 'include/grub/i386/pc/int.h'
--- include/grub/i386/pc/int.h	1970-01-01 00:00:00 +0000
+++ include/grub/i386/pc/int.h	2010-01-17 19:23:55 +0000
@@ -0,0 +1,52 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_INTERRUPT_MACHINE_HEADER
+#define GRUB_INTERRUPT_MACHINE_HEADER	1
+
+#include <grub/symbol.h>
+
+struct grub_bios_int_registers
+{
+  grub_uint32_t eax;
+  grub_uint16_t es;
+  grub_uint16_t ds;
+  grub_uint16_t flags;
+  grub_uint16_t dummy;
+  grub_uint32_t ebx;
+  grub_uint32_t ecx;
+  grub_uint32_t edi;
+  grub_uint32_t esi;
+  grub_uint32_t edx;
+};
+
+#define  GRUB_CPU_INT_FLAGS_CARRY     0x1
+#define  GRUB_CPU_INT_FLAGS_PARITY    0x4
+#define  GRUB_CPU_INT_FLAGS_ADJUST    0x10
+#define  GRUB_CPU_INT_FLAGS_ZERO      0x40
+#define  GRUB_CPU_INT_FLAGS_SIGN      0x80
+#define  GRUB_CPU_INT_FLAGS_TRAP      0x100
+#define  GRUB_CPU_INT_FLAGS_INTERRUPT 0x200
+#define  GRUB_CPU_INT_FLAGS_DIRECTION 0x400
+#define  GRUB_CPU_INT_FLAGS_OVERFLOW  0x800
+#define  GRUB_CPU_INT_FLAGS_DEFAULT   GRUB_CPU_INT_FLAGS_INTERRUPT
+
+void EXPORT_FUNC (grub_bios_interrupt) (grub_uint8_t intno,
+					struct grub_bios_int_registers *regs);
+
+#endif

=== modified file 'kern/i386/pc/startup.S'
--- kern/i386/pc/startup.S	2010-01-03 22:05:07 +0000
+++ kern/i386/pc/startup.S	2010-01-17 20:16:27 +0000
@@ -567,44 +567,6 @@
 #include "../loader.S"
 
 /*
- *   int grub_biosdisk_rw_int13_extensions (int ah, int drive, void *dap)
- *
- *   Call IBM/MS INT13 Extensions (int 13 %ah=AH) for DRIVE. DAP
- *   is passed for disk address packet. If an error occurs, return
- *   non-zero, otherwise zero.
- */
-
-FUNCTION(grub_biosdisk_rw_int13_extensions)
-	pushl	%ebp
-	pushl	%esi
-
-	/* compute the address of disk_address_packet */
-	movw	%cx, %si
-	xorw	%cx, %cx
-	shrl	$4, %ecx	/* save the segment to cx */
-
-	/* ah */
-	movb	%al, %dh
-	/* enter real mode */
-	call	prot_to_real
-
-	.code16
-	movb	%dh, %ah
-	movw	%cx, %ds
-	int	$0x13		/* do the operation */
-	movb	%ah, %dl	/* save return value */
-	/* back to protected mode */
-	DATA32	call	real_to_prot
-	.code32
-
-	movb	%dl, %al	/* return value in %eax */
-
-	popl	%esi
-	popl	%ebp
-
-	ret
-
-/*
  *   int grub_biosdisk_rw_standard (int ah, int drive, int coff, int hoff,
  *                                  int soff, int nsec, int segment)
  *
@@ -862,43 +824,6 @@
 
 
 /*
- * int grub_biosdisk_get_num_floppies (void)
- */
-FUNCTION(grub_biosdisk_get_num_floppies)
-	pushl	%ebp
-
-	xorl	%edx, %edx
-	call	prot_to_real
-
-	.code16
-	/* reset the disk system first */
-	int	$0x13
-1:
-	stc
-
-	/* call GET DISK TYPE */
-	movb	$0x15, %ah
-	int	$0x13
-
-	jc	2f
-
-	/* check if this drive exists */
-	testb	$0x3, %ah
-	jz	2f
-
-	incb	%dl
-	cmpb	$2, %dl
-	jne	1b
-2:
-	DATA32	call	real_to_prot
-	.code32
-
-	movl	%edx, %eax
-	popl	%ebp
-	ret
-
-
-/*
  *
  * grub_get_memsize(i) :  return the memory size in KB. i == 0 for conventional
  *		memory, i == 1 for extended memory
@@ -2142,3 +2067,97 @@
 	popl	%esi
 	popl	%ebp
 	ret
+
+FUNCTION(grub_bios_interrupt)
+	pushl    %ebp
+	pushl    %ecx
+	pushl    %eax
+	pushl    %edx
+	
+	movb     %al, intno
+	movl	 (%edx), %eax
+	movl	 %eax, LOCAL(bios_register_eax)
+	movw	 4(%edx), %ax
+	movw	 %ax, LOCAL(bios_register_es)
+	movw	 6(%edx), %ax
+	movw	 %ax, LOCAL(bios_register_ds)
+	movw	 8(%edx), %ax
+	movw	 %ax, LOCAL(bios_register_flags)
+
+	movl 	12(%edx), %ebx
+	movl 	16(%edx), %ecx
+	movl 	20(%edx), %edi
+	movl 	24(%edx), %esi
+	movl 	28(%edx), %edx
+
+	call    prot_to_real
+	.code16
+
+	mov	%ds, %ax
+	push	%ax
+
+	/* movw imm16, %ax*/
+	.byte	0xb8
+LOCAL(bios_register_es):
+	.short 	0
+	movw	%ax, %es
+	/* movw imm16, %ax*/
+	.byte	0xb8
+LOCAL(bios_register_ds):
+	.short 	0
+	movw	%ax, %ds
+
+	/* movw imm16, %ax*/
+	.byte	0xb8
+LOCAL(bios_register_flags):
+	.short 	0
+	push	%ax
+	popf
+
+	/* movl imm32, %eax*/
+	.byte	0x66, 0xb8
+LOCAL(bios_register_eax):
+	.long 	0
+	
+	/* int imm8.  */
+	.byte   0xcd
+intno:	
+	.byte   0
+
+	movl 	%eax, %cs:LOCAL(bios_register_eax)
+	movw	%ds, %ax
+	movw 	%ax, %cs:LOCAL(bios_register_ds)
+	pop 	%ax
+	mov	%ax, %ds
+	pushf
+	pop	%ax
+	movw	%ax, LOCAL(bios_register_flags)
+	mov 	%es, %ax
+	movw	%ax, LOCAL(bios_register_es)
+
+	DATA32  call	real_to_prot
+	.code32
+
+	popl    %eax
+
+	movl 	%ebx, 12(%eax)
+	movl 	%ecx, 16(%eax)
+	movl 	%edi, 20(%eax)
+	movl 	%esi, 24(%eax)
+	movl 	%edx, 28(%eax)
+
+	movl     %eax, %edx
+
+	movl	 LOCAL(bios_register_eax), %eax
+	movl	 %eax, (%edx)
+	movw	 LOCAL(bios_register_es), %ax
+	movw	 %ax, 4(%edx)
+	movw	 LOCAL(bios_register_ds), %ax
+	movw	 %ax, 6(%edx)
+	movw	 LOCAL(bios_register_flags), %ax
+	movw	 %ax, 8(%edx)
+
+	popl    %eax
+	popl    %ecx
+	popl    %ebp
+	ret


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]