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."