[PATCH] Centralizing understanding of far pointers

[PATCH] Centralizing understanding of far pointers

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

This patch modifies the machine-specific memory.h (currently, just the
i386-specific file), adding a new type grub_machine_farptr and two
functions to convert between such far pointers and normal C pointers.

The code performing the mapping between realmode and pmode addresses is
simple, and thus is repeated in many source files like drivemap, vbe*,
mmap, etc. However, as simple as it is, its ad-hoc application tends to
be quite unwieldy, generating long code that is verges close to the tag
of write-only code.

The i386 farptr type has been implemented as an union between the uint32
that was used until now (.raw_bits) and a structure separating the
uint16 segment and offset parts for easy access and debug printing.

This post has three attachments: the patch to memory.h itself, a patch
to the i386 mmap.c and its helper asm file, to show the impact of this
patch (I've also taken the liberty of adding an offset macro just like
in drivemap, and make the changed code more elegant in my opinion), and
another patch doing the same for drivemap.

NOTE: this patch depends on the PTR_TO_UINT macro added by another patch
still on discussion, [1].

[1] http://lists.gnu.org/archive/html/grub-devel/2009-07/msg00398.html

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: rmode.patch --]
[-- Type: text/x-patch, Size: 972 bytes --]

Index: include/grub/i386/pc/memory.h
===================================================================
--- include/grub/i386/pc/memory.h	(revisión: 2447)
+++ include/grub/i386/pc/memory.h	(copia de trabajo)
@@ -126,6 +126,32 @@
 }
 #endif
 
+typedef union
+{
+  struct
+  {
+    /* Given that i386 is little-endian, this order matches the in-memory
+       format of CPU realmode far pointers.  */
+    grub_uint16_t offset;
+    grub_uint16_t segment;
+  } __attribute__ ((packed));
+  grub_uint32_t raw_bits;
+} grub_machine_farptr;
+
+static inline void *
+grub_machine_real2linear (grub_machine_farptr ptr)
+{
+  return UINT_TO_PTR ((((grub_uint32_t) ptr.segment) << 4) + ptr.offset);
+}
+
+static inline grub_machine_farptr
+grub_machine_linear2real (void *addr)
+{
+  grub_machine_farptr ret = { { PTR_TO_UINT32 (addr) & 0x0000000F,
+			       (PTR_TO_UINT32 (addr) & 0x000FFFF0) >> 4 } };
+  return ret;
+}
+
 #endif
 
 #endif /* ! GRUB_MEMORY_MACHINE_HEADER */

[-- Attachment #3: example_mmap.patch --]
[-- Type: text/x-patch, Size: 4918 bytes --]

Index: mmap/i386/pc/mmap_helper.S
===================================================================
--- mmap/i386/pc/mmap_helper.S	(revisión: 2447)
+++ mmap/i386/pc/mmap_helper.S	(copia de trabajo)
@@ -48,9 +48,8 @@
 	popf
 	/* ljmp */
 	.byte	0xea
-VARIABLE (grub_machine_mmaphook_int15offset)
+VARIABLE (grub_machine_mmaphook_oldint15)
 	.word	0
-VARIABLE (grub_machine_mmaphook_int15segment)
 	.word	0
 
 e801:
Index: mmap/i386/pc/mmap.c
===================================================================
--- mmap/i386/pc/mmap.c	(revisión: 2447)
+++ mmap/i386/pc/mmap.c	(copia de trabajo)
@@ -25,17 +25,20 @@
 
 #define min(a,b) (((a) < (b)) ? (a) : (b))
 
+#define OFFSET_IN_ASM(addr) ( PTR_TO_UINT (addr) \
+                          - PTR_TO_UINT (&grub_machine_mmaphook_start) )
+
 static void *hooktarget = 0;
+/* Not a null pointer, the real mode IVT is actually at address 0:0 */
+static grub_machine_farptr *grub_machine_ivt = 0;
 
-extern grub_uint8_t grub_machine_mmaphook_start;
-extern grub_uint8_t grub_machine_mmaphook_end;
-extern grub_uint8_t grub_machine_mmaphook_int12;
-extern grub_uint8_t grub_machine_mmaphook_int15;
+extern const void grub_machine_mmaphook_start;
+extern const void grub_machine_mmaphook_end;
+extern const void grub_machine_mmaphook_int12;
+extern const void grub_machine_mmaphook_int15;
 
-static grub_uint16_t grub_machine_mmaphook_int12offset = 0;
-static grub_uint16_t grub_machine_mmaphook_int12segment = 0;
-extern grub_uint16_t grub_machine_mmaphook_int15offset;
-extern grub_uint16_t grub_machine_mmaphook_int15segment;
+static grub_machine_farptr grub_machine_mmaphook_oldint12;
+extern grub_machine_farptr grub_machine_mmaphook_oldint15;
 
 extern grub_uint16_t grub_machine_mmaphook_mmap_num;
 extern grub_uint16_t grub_machine_mmaphook_kblow;
@@ -73,9 +76,8 @@
 
   grub_dprintf ("mmap", "installing preboot handlers\n");
 
-  hookmmapcur = hookmmap = (struct grub_e820_mmap_entry *)
-    ((grub_uint8_t *) hooktarget + (&grub_machine_mmaphook_end
-				    - &grub_machine_mmaphook_start));
+  hookmmapcur = hookmmap = UINT_TO_PTR (PTR_TO_UINT (hooktarget)
+                                   + OFFSET_IN_ASM(&grub_machine_mmaphook_end));
 
   grub_mmap_iterate (fill_hook);
   grub_machine_mmaphook_mmap_num = hookmmapcur - hookmmap;
@@ -90,24 +92,23 @@
   *((grub_uint16_t *) 0x413) = grub_mmap_get_lower () >> 10;
 
   /* Save old interrupt handlers. */
-  grub_machine_mmaphook_int12offset = *((grub_uint16_t *) 0x48);
-  grub_machine_mmaphook_int12segment = *((grub_uint16_t *) 0x4a);
-  grub_machine_mmaphook_int15offset = *((grub_uint16_t *) 0x54);
-  grub_machine_mmaphook_int15segment = *((grub_uint16_t *) 0x56);
+  grub_machine_mmaphook_oldint12 = grub_machine_ivt[0x12];
+  grub_machine_mmaphook_oldint15 = grub_machine_ivt[0x15];
 
   grub_dprintf ("mmap", "hooktarget = %p\n", hooktarget);
 
   /* Install the interrupt handlers. */
   grub_memcpy (hooktarget, &grub_machine_mmaphook_start,
-	       &grub_machine_mmaphook_end - &grub_machine_mmaphook_start);
+	       OFFSET_IN_ASM(&grub_machine_mmaphook_end));
 
-  *((grub_uint16_t *) 0x4a) = PTR_TO_UINT32 (hooktarget) >> 4;
-  *((grub_uint16_t *) 0x56) = PTR_TO_UINT32 (hooktarget) >> 4;
-  *((grub_uint16_t *) 0x48) = &grub_machine_mmaphook_int12
-    - &grub_machine_mmaphook_start;
-  *((grub_uint16_t *) 0x54) = &grub_machine_mmaphook_int15
-    - &grub_machine_mmaphook_start;
+  void* newint12 = UINT_TO_PTR (PTR_TO_UINT (hooktarget)
+                                + OFFSET_IN_ASM(&grub_machine_mmaphook_int12));
+  void* newint15 = UINT_TO_PTR (PTR_TO_UINT (hooktarget)
+                                + OFFSET_IN_ASM(&grub_machine_mmaphook_int15));
 
+  grub_machine_ivt[0x12] = grub_machine_linear2real(newint12);
+  grub_machine_ivt[0x15] = grub_machine_linear2real(newint15);
+
   return GRUB_ERR_NONE;
 }
 
@@ -115,10 +116,8 @@
 preboot_rest (void)
 {
   /* Restore old interrupt handlers. */
-  *((grub_uint16_t *) 0x48) = grub_machine_mmaphook_int12offset;
-  *((grub_uint16_t *) 0x4a) = grub_machine_mmaphook_int12segment;
-  *((grub_uint16_t *) 0x54) = grub_machine_mmaphook_int15offset;
-  *((grub_uint16_t *) 0x56) = grub_machine_mmaphook_int15segment;
+  grub_machine_ivt[0x12] = grub_machine_mmaphook_oldint12;
+  grub_machine_ivt[0x15] = grub_machine_mmaphook_oldint15;
 
   return GRUB_ERR_NONE;
 }
@@ -161,12 +160,11 @@
       hooktarget = 0;
     }
 
-  hooksize = &grub_machine_mmaphook_end - &grub_machine_mmaphook_start
+  hooksize = OFFSET_IN_ASM(&grub_machine_mmaphook_end)
     + regcount * sizeof (struct grub_e820_mmap_entry);
   /* Allocate an integer number of KiB. */
   hooksize = ((hooksize - 1) | 0x3ff) + 1;
-  slots_available = (hooksize - (&grub_machine_mmaphook_end
-				 - &grub_machine_mmaphook_start))
+  slots_available = (hooksize - OFFSET_IN_ASM(&grub_machine_mmaphook_end))
     / sizeof (struct grub_e820_mmap_entry);
 
   reentry = 1;

[-- Attachment #4: example_drivemap.patch --]
[-- Type: text/x-patch, Size: 2345 bytes --]

Index: commands/i386/pc/drivemap.c
===================================================================
--- commands/i386/pc/drivemap.c	(revisión: 2447)
+++ commands/i386/pc/drivemap.c	(copia de trabajo)
@@ -29,7 +29,7 @@
 
 
 /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
-static grub_uint32_t *const int13slot = UINT_TO_PTR (4 * 0x13);
+static grub_machine_farptr *const int13slot = UINT_TO_PTR (4 * 0x13);
 
 /* Remember to update enum opt_idxs accordingly.  */
 static const struct grub_arg_option options[] = {
@@ -48,7 +48,7 @@
 };
 
 /* Realmode far ptr (2 * 16b) to the previous INT13h handler.  */
-extern grub_uint32_t grub_drivemap_oldhandler;
+extern grub_machine_farptr grub_drivemap_oldhandler;
 
 /* The type "void" is used for imported assembly labels, takes no storage and
    serves just to take the address with &label.  */
@@ -295,8 +295,7 @@
   /* Save the pointer to the old handler.  */
   grub_drivemap_oldhandler = *int13slot;
   grub_dprintf ("drivemap", "Original int13 handler: %04x:%04x\n",
-		(grub_drivemap_oldhandler >> 16) & 0x0ffff,
-		grub_drivemap_oldhandler & 0x0ffff);
+		grub_drivemap_oldhandler.segment, grub_drivemap_oldhandler.offset);
 
   /* Find a rmode-segment-aligned zone in conventional memory big
      enough to hold the handler and its data.  */
@@ -335,9 +334,9 @@
   grub_dprintf ("drivemap", "\t#%d: 0x00 <- 0x00 (end)\n", i);
 
   /* Install our function as the int13h handler in the IVT.  */
-  *int13slot = ((grub_uint32_t) handler_base) << 12;	/* Segment address.  */
+  *int13slot = grub_machine_linear2real(handler_base);
   grub_dprintf ("drivemap", "New int13 handler: %04x:%04x\n",
-		(*int13slot >> 16) & 0x0ffff, *int13slot & 0x0ffff);
+		int13slot->segment, int13slot->offset);
 
   return GRUB_ERR_NONE;
 }
@@ -345,14 +344,14 @@
 static grub_err_t
 uninstall_int13_handler (void)
 {
-  if (! grub_drivemap_oldhandler)
+  if (! grub_drivemap_oldhandler.raw_bits)
     return GRUB_ERR_NONE;
 
   *int13slot = grub_drivemap_oldhandler;
   grub_mmap_free_and_unregister (drivemap_mmap);
-  grub_drivemap_oldhandler = 0;
+  grub_drivemap_oldhandler.raw_bits = 0;
   grub_dprintf ("drivemap", "Restored int13 handler: %04x:%04x\n",
-		(*int13slot >> 16) & 0x0ffff, *int13slot & 0x0ffff);
+		int13slot->segment, int13slot->offset);
 
   return GRUB_ERR_NONE;
 }

Re: [PATCH] Centralizing understanding of far pointers

[Robert Millan]

On Sun, Jul 26, 2009 at 12:32:28AM +0200, Javier Martín wrote:
> This patch modifies the machine-specific memory.h (currently, just the
> i386-specific file), adding a new type grub_machine_farptr and two
> functions to convert between such far pointers and normal C pointers.
> 
> The code performing the mapping between realmode and pmode addresses is
> simple, and thus is repeated in many source files like drivemap, vbe*,
> mmap, etc. However, as simple as it is, its ad-hoc application tends to
> be quite unwieldy, generating long code that is verges close to the tag
> of write-only code.
> 
> The i386 farptr type has been implemented as an union between the uint32
> that was used until now (.raw_bits) and a structure separating the
> uint16 segment and offset parts for easy access and debug printing.
> 
> This post has three attachments: the patch to memory.h itself, a patch
> to the i386 mmap.c and its helper asm file, to show the impact of this
> patch (I've also taken the liberty of adding an offset macro just like
> in drivemap, and make the changed code more elegant in my opinion), and
> another patch doing the same for drivemap.
> 
> NOTE: this patch depends on the PTR_TO_UINT macro added by another patch
> still on discussion, [1].

The idea seems interesting, and in general such cleanup is welcome.  I have
some comments, only on the first patch.

First of all, please don't call them far pointers.  They're an i8086 legacy
cruft, which have nothing to do with far or close really (although we seem to
have some code that makes this reference already).

What you're doing is basically the same as real2pm() function we already had.
It seems this function should move elsewhere so it can have the generic use
you intended (it can be reimplemented as well, if that makes it cleaner).

> +typedef union
> +{
> +  struct
> +  {
> +    /* Given that i386 is little-endian, this order matches the in-memory
> +       format of CPU realmode far pointers.  */
> +    grub_uint16_t offset;
> +    grub_uint16_t segment;
> +  } __attribute__ ((packed));
> +  grub_uint32_t raw_bits;

Is there a usefulness in this `raw_bits' member?  It doesn't have any
real meaning, as it doesn't correspond to an actual address.

> +} grub_machine_farptr;

I admit this is a bit confusing, but the `machine' namespace is for
things specific to a given firmware.  i8086 mode is part of the i386
architecture, so we'd put this in the `cpu' namespace instead.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Centralizing understanding of far pointers

[Javier Martín]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Robert Millan escribió:
> First of all, please don't call them far pointers.  They're an i8086 legacy
> cruft, which have nothing to do with far or close really (although we seem to
> have some code that makes this reference already).
So... how do we call them? I am an utter failure at making up sensible
names.

> 
> What you're doing is basically the same as real2pm() function we already had.
> It seems this function should move elsewhere so it can have the generic use
> you intended (it can be reimplemented as well, if that makes it cleaner).
Indeed, that's what I thought when I saw that function in some
vbe-related file. I remembered that I wrote similar code for drivemap,
and then reasoned that mmap would used too, as it modified the IVT.
Thus, we are uselessly duplicating code that is simple but at the same
time error-prone.

The idea of this patch is to provide a way to handle the abstraction of
a pointer that is somehow mangled by the architecture, but the "user"
code doesn't need to actually know how. For example, the patched
drivemap would just take whatever linear2real(ptr) gives and stores it
into the IVT slot. Of course, this is not as "magical" as an ideal world
would have it, because the code still has to know the IVT to place it
there, but I think it's still a plus.

>> +typedef union
>> +{
>> +  struct
>> +  {
>> +    /* Given that i386 is little-endian, this order matches the in-memory
>> +       format of CPU realmode far pointers.  */
>> +    grub_uint16_t offset;
>> +    grub_uint16_t segment;
>> +  } __attribute__ ((packed));
>> +  grub_uint32_t raw_bits;
> 
> Is there a usefulness in this `raw_bits' member?  It doesn't have any
> real meaning, as it doesn't correspond to an actual address.
Mainly, the possibility of checking equality against a particular bit
pattern without using another instance of the structure/union. Also,
some code might want to still use the raw bits for some reason, instead
of either the nearly-opaque handling described above or the more
detailed view provided by the inner structure.

> 
>> +} grub_machine_farptr;
> 
> I admit this is a bit confusing, but the `machine' namespace is for
> things specific to a given firmware.  i8086 mode is part of the i386
> architecture, so we'd put this in the `cpu' namespace instead.
OK then... That means the code will have to move from i386/pc/memory.h
to i386/memory.h. And what about x86_64?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJKb5qBAAoJEKSl+Fbdeo72EfMP/0JVO7mO92Pyk9l5tiGP5IQ+
PbrLJBX1HtAemwMufW5wR+OoQjyb9LGf1JR0ff7o7Xkbtnc3iN7EI8RV4WAzuLp3
9tYpYZkIwLWa86GM8Oy8QWer58vNlHYNtH+Zmrv+341umtFZERRjWk6NwpNNgFtm
uX8FoYGqHLIhkzLWvd5vM7V+8S9LJPjq2ws/F1NbS5Sd33wTz81eFz6TNmoALNuD
KeIR9Vo6f1zN3lp0M9+CL1fzX7uP3e3dWJ+KmOlkJcEJgpsRJhz76Ux9Byq0Tby9
SLXQmEfBmSNk95HpXnn2AFuiH/o8SYyWExPAEbz//Ttym4ElGGfS+tE3d8b7auga
Y2ReU8I9XlbMczIjvnh6MaLOwzGuu6mtPFvec5PHSK9sekJ2TWYCosXpUkMo58TS
vXPou5VDAkRb6sbuMx8g7fGws5vgYuEYcIZAkW3ejcJhIkMSGODEYRsT2A8ZREzx
Qs/B48Z9Tk4nVxdPrIfDg7eB4TtEpJak84uw/fV6/WclY5ipk/SGf0iyhDfuU7vp
6w+6Kbvny0dOyssMz8cN2MlL7DinAtx2Zzi0jVHBNl253Af/idPJAPw69LDFWroN
EHjLHxxnnkF4MOiR7WzH49QRzLCuGmMfgnqKEdEW1JxNFqG5+zE0L3p1Sp/pITs7
q+2CU6+NR3veOdzg3yRh
=dKOS
-----END PGP SIGNATURE-----

Re: [PATCH] Centralizing understanding of far pointers

[Vladimir 'phcoder' Serbinenko]

2009/7/29 Javier Martín <lordhabbit@gmail.com>:
> Robert Millan escribió:
>> First of all, please don't call them far pointers.  They're an i8086 legacy
>> cruft, which have nothing to do with far or close really (although we seem to
>> have some code that makes this reference already).
> So... how do we call them? I am an utter failure at making up sensible
> names.
>
grub_i386_realmode_ptr_t
>> Is there a usefulness in this `raw_bits' member?  It doesn't have any
>> real meaning, as it doesn't correspond to an actual address.
> Mainly, the possibility of checking equality against a particular bit
> pattern without using another instance of the structure/union. Also,
> some code might want to still use the raw bits for some reason, instead
> of either the nearly-opaque handling described above or the more
> detailed view provided by the inner structure.
raw_bits don't seem very useful and using them is error-prone. E.g.
two pointers may reffer to same address even having different
raw_bits. So I would prefer not to include them unless there are real
examples when it's useful

> OK then... That means the code will have to move from i386/pc/memory.h
> to i386/memory.h. And what about x86_64?
Put a memory.h in x86_64/memory.h which will include i386/memory.h



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git