Re: [PATCH v2 5/6] xen: modify page table construction

[Juergen Gross <jgross@suse.com>]

On 11/02/16 13:47, Daniel Kiper wrote:
> On Thu, Feb 11, 2016 at 08:53:25AM +0100, Juergen Gross wrote:
>> Modify the page table construction to allow multiple virtual regions
>> to be mapped. This is done as preparation for removing the p2m list
>> from the initial kernel mapping in order to support huge pv domains.
>>
>> This allows a cleaner approach for mapping the relocator page by
>> using this capability.
>>
>> The interface to the assembler level of the relocator has to be changed
>> in order to be able to process multiple page table areas.
> 
> I hope that older kernels could be loaded as is and everything works as expected.

See Patch 0/6. I have tested old kernels, too.

> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  grub-core/lib/i386/xen/relocator.S   |  47 +++---
>>  grub-core/lib/x86_64/xen/relocator.S |  44 +++--
>>  grub-core/lib/xen/relocator.c        |  22 ++-
>>  grub-core/loader/i386/xen.c          | 313 +++++++++++++++++++++++------------
>>  include/grub/xen/relocator.h         |   6 +-
>>  5 files changed, 277 insertions(+), 155 deletions(-)
>>
>> diff --git a/grub-core/lib/i386/xen/relocator.S b/grub-core/lib/i386/xen/relocator.S
>> index 694a54c..c23b405 100644
>> --- a/grub-core/lib/i386/xen/relocator.S
>> +++ b/grub-core/lib/i386/xen/relocator.S
>> @@ -50,41 +50,45 @@ VARIABLE(grub_relocator_xen_remapper_map_high)
>>  	jmp *%ebx
>>
>>  LOCAL(cont):
>> -	xorl	%eax, %eax
>> -	movl	%eax, %ebp
>> +	/* mov imm32, %eax */
>> +	.byte	0xb8
>> +VARIABLE(grub_relocator_xen_paging_areas_addr)
>> +	.long	0
>> +	movl	%eax, %ebx
>>  1:
>> -
>> +	movl	0(%ebx), %ebp
>> +	movl	4(%ebx), %ecx
> 
> Oh... No, please use constants not plain numbers and
> explain in comments what is going on. Otherwise it
> is unreadable.

Hmm, yes, some comments wouldn't hurt. :-)
Regarding constants: do you really think I should introduce their
first usage in this file with my patch?

> 
>> +	testl	%ecx, %ecx
>> +	jz	3f
>> +	addl	$8, %ebx
>> +	movl	%ebx, %esp
>> +
>> +2:
>> +	movl	%ecx, %edi
>>  	/* mov imm32, %eax */
>>  	.byte	0xb8
>>  VARIABLE(grub_relocator_xen_mfn_list)
>>  	.long	0
>> -	movl	%eax, %edi
>> -	movl	%ebp, %eax
>> -	movl    0(%edi, %eax, 4), %ecx
>> -
>> -	/* mov imm32, %ebx */
>> -	.byte	0xbb
>> -VARIABLE(grub_relocator_xen_paging_start)
>> -	.long	0
>> -	shll	$12, %eax
>> -	addl	%eax, %ebx
>> +	movl    0(%eax, %ebp, 4), %ecx
>> +	movl	%ebp, %ebx
>> +	shll	$12, %ebx
> 
> Ditto.

Look at the removed line above: I just switched register usage.

> 
>>  	movl    %ecx, %edx
>>  	shll    $12,  %ecx
>>  	shrl    $20,  %edx
>>  	orl     $5, %ecx
>>  	movl    $2, %esi
>>  	movl    $__HYPERVISOR_update_va_mapping, %eax
>> -	int     $0x82
>> +	int     $0x82	/* parameters: eax, ebx, ecx, edx, esi */
> 
> Please use more verbose comments.

:-)

> 
>>
>>  	incl	%ebp
>> -	/* mov imm32, %ecx */
>> -	.byte	0xb9
>> -VARIABLE(grub_relocator_xen_paging_size)
>> -	.long	0
>> -	cmpl	%ebp, %ecx
>> +	movl	%edi, %ecx
>> +
>> +	loop	2b
>>
>> -	ja	1b
>> +	mov	%esp, %ebx
>> +	jmp	1b
>>
>> +3:
>>  	/* mov imm32, %ebx */
>>  	.byte	0xbb
>>  VARIABLE(grub_relocator_xen_mmu_op_addr)
>> @@ -102,6 +106,9 @@ VARIABLE(grub_relocator_xen_remap_continue)
>>
>>  	jmp *%eax
>>
>> +VARIABLE(grub_relocator_xen_paging_areas)
>> +	.long	0, 0, 0, 0, 0, 0, 0, 0
>> +
>>  VARIABLE(grub_relocator_xen_mmu_op)
>>  	.space 256
>>
>> diff --git a/grub-core/lib/x86_64/xen/relocator.S b/grub-core/lib/x86_64/xen/relocator.S
>> index 92e9e72..dbb90c7 100644
>> --- a/grub-core/lib/x86_64/xen/relocator.S
>> +++ b/grub-core/lib/x86_64/xen/relocator.S
> 
> Is to possible to split this patch to i386 and x86-64 stuff patches?

I don't think so. The C-part is common and assembler sources must both
match.

> 
>> @@ -50,31 +50,24 @@ VARIABLE(grub_relocator_xen_remapper_map)
>>
>>  LOCAL(cont):
>>
>> -	/* mov imm64, %rcx */
>> -	.byte 	0x48
>> -	.byte	0xb9
>> -VARIABLE(grub_relocator_xen_paging_size)
>> -	.quad	0
>> -
>> -	/* mov imm64, %rax */
>> -	.byte 	0x48
>> -	.byte	0xb8
>> -VARIABLE(grub_relocator_xen_paging_start)
>> -	.quad	0
>> -
>> -	movq	%rax, %r12
>> -
>>  	/* mov imm64, %rax */
>>  	.byte 	0x48
>>  	.byte	0xb8
>>  VARIABLE(grub_relocator_xen_mfn_list)
>>  	.quad	0
>>
>> -	movq	%rax, %rsi
>> +	movq	%rax, %rbx
>> +	leaq	EXT_C(grub_relocator_xen_paging_areas) (%rip), %r8
>> +
>>  1:
>> +	movq	0(%r8), %r12
>> +	movq	8(%r8), %rcx
> 
> Ditto.
> 
>> +	testq	%rcx, %rcx
>> +	jz	3f
>> +2:
>>  	movq	%r12, %rdi
>> -	movq    %rsi, %rbx
>> -	movq    0(%rsi), %rsi
>> +	shlq	$12, %rdi
>> +	movq    (%rbx, %r12, 8), %rsi
> 
> Ditto.
> 
>>  	shlq    $12,  %rsi
>>  	orq     $5, %rsi
>>  	movq    $2, %rdx
>> @@ -83,13 +76,15 @@ VARIABLE(grub_relocator_xen_mfn_list)
>>  	syscall
>>
>>  	movq    %r9, %rcx
>> -	addq    $8, %rbx
>> -	addq    $4096, %r12
>> -	movq    %rbx, %rsi
>> +	incq	%r12
>> +
>> +	loop 2b
>>
>> -	loop 1b
>> +	addq	$16, %r8
> 
> Ditto.
> 
>> +	jmp	1b
>>
>> -	leaq   LOCAL(mmu_op) (%rip), %rdi
>> +3:
>> +	leaq   EXT_C(grub_relocator_xen_mmu_op) (%rip), %rdi
>>  	movq   $3, %rsi
>>  	movq   $0, %rdx
>>  	movq   $0x7FF0, %r10
> 
> Ugh... Please fix this too. Probably in separate patch.

I don't mind cleaning the assembler sources by using more comments
and constants. I just don't think this should be part of this series.

> 
>> @@ -104,7 +99,10 @@ VARIABLE(grub_relocator_xen_remap_continue)
>>
>>  	jmp *%rax
>>
>> -LOCAL(mmu_op):
>> +VARIABLE(grub_relocator_xen_paging_areas)
>> +	/* array of start, size pairs, size 0 is end marker */
>> +	.quad	0, 0, 0, 0, 0, 0, 0, 0
>> +
>>  VARIABLE(grub_relocator_xen_mmu_op)
>>  	.space 256
>>
>> diff --git a/grub-core/lib/xen/relocator.c b/grub-core/lib/xen/relocator.c
>> index 8f427d3..bc29055 100644
>> --- a/grub-core/lib/xen/relocator.c
>> +++ b/grub-core/lib/xen/relocator.c
>> @@ -36,15 +36,15 @@ extern grub_uint8_t grub_relocator_xen_remap_end;
>>  extern grub_xen_reg_t grub_relocator_xen_stack;
>>  extern grub_xen_reg_t grub_relocator_xen_start_info;
>>  extern grub_xen_reg_t grub_relocator_xen_entry_point;
>> -extern grub_xen_reg_t grub_relocator_xen_paging_start;
>> -extern grub_xen_reg_t grub_relocator_xen_paging_size;
>>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt;
>>  extern grub_xen_reg_t grub_relocator_xen_remapper_virt2;
>>  extern grub_xen_reg_t grub_relocator_xen_remapper_map;
>>  extern grub_xen_reg_t grub_relocator_xen_mfn_list;
>> +extern grub_xen_reg_t grub_relocator_xen_paging_areas[2 * XEN_MAX_MAPPINGS];
>>  extern grub_xen_reg_t grub_relocator_xen_remap_continue;
>>  #ifdef __i386__
>>  extern grub_xen_reg_t grub_relocator_xen_mmu_op_addr;
>> +extern grub_xen_reg_t grub_relocator_xen_paging_areas_addr;
>>  extern grub_xen_reg_t grub_relocator_xen_remapper_map_high;
>>  #endif
>>  extern mmuext_op_t grub_relocator_xen_mmu_op[3];
>> @@ -61,6 +61,7 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
>>  {
>>    grub_err_t err;
>>    void *relst;
>> +  int i;
>>    grub_relocator_chunk_t ch, ch_tramp;
>>    grub_xen_mfn_t *mfn_list =
>>      (grub_xen_mfn_t *) grub_xen_start_page_addr->mfn_list;
>> @@ -77,8 +78,11 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
>>    grub_relocator_xen_stack = state.stack;
>>    grub_relocator_xen_start_info = state.start_info;
>>    grub_relocator_xen_entry_point = state.entry_point;
>> -  grub_relocator_xen_paging_start = state.paging_start << 12;
>> -  grub_relocator_xen_paging_size = state.paging_size;
>> +  for (i = 0; i < XEN_MAX_MAPPINGS; i++)
>> +    {
>> +      grub_relocator_xen_paging_areas[2 * i] = state.paging_start[i];
>> +      grub_relocator_xen_paging_areas[2 * i + 1] = state.paging_size[i];
> 
> Why 2 and 1? Constants?

I guess I should probably define grub_relocator_xen_paging_areas as
an array of struct { grub_xen_reg_t start; grub_xen_reg_t size; };

> 
>> +    }
>>    grub_relocator_xen_remapper_virt = remapper_virt;
>>    grub_relocator_xen_remapper_virt2 = remapper_virt;
>>    grub_relocator_xen_remap_continue = trampoline_virt;
>> @@ -88,10 +92,12 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
>>    grub_relocator_xen_remapper_map_high = (mfn_list[remapper_pfn] >> 20);
>>    grub_relocator_xen_mmu_op_addr = (char *) &grub_relocator_xen_mmu_op
>>      - (char *) &grub_relocator_xen_remap_start + remapper_virt;
>> +  grub_relocator_xen_paging_areas_addr =
>> +    (char *) &grub_relocator_xen_paging_areas
>> +    - (char *) &grub_relocator_xen_remap_start + remapper_virt;
>>  #endif
>>
>> -  grub_relocator_xen_mfn_list = state.mfn_list
>> -    + state.paging_start * sizeof (grub_addr_t);
>> +  grub_relocator_xen_mfn_list = state.mfn_list;
>>
>>    grub_memset (grub_relocator_xen_mmu_op, 0,
>>  	       sizeof (grub_relocator_xen_mmu_op));
>> @@ -100,9 +106,9 @@ grub_relocator_xen_boot (struct grub_relocator *rel,
>>  #else
>>    grub_relocator_xen_mmu_op[0].cmd = MMUEXT_PIN_L4_TABLE;
>>  #endif
>> -  grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start];
>> +  grub_relocator_xen_mmu_op[0].arg1.mfn = mfn_list[state.paging_start[0]];
>>    grub_relocator_xen_mmu_op[1].cmd = MMUEXT_NEW_BASEPTR;
>> -  grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start];
>> +  grub_relocator_xen_mmu_op[1].arg1.mfn = mfn_list[state.paging_start[0]];
>>    grub_relocator_xen_mmu_op[2].cmd = MMUEXT_UNPIN_TABLE;
>>    grub_relocator_xen_mmu_op[2].arg1.mfn =
>>      mfn_list[grub_xen_start_page_addr->pt_base >> 12];
>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
>> index 0f41048..5e10420 100644
>> --- a/grub-core/loader/i386/xen.c
>> +++ b/grub-core/loader/i386/xen.c
>> @@ -42,6 +42,29 @@
>>
>>  GRUB_MOD_LICENSE ("GPLv3+");
>>
>> +#ifdef __x86_64__
>> +#define NUMBER_OF_LEVELS 4
>> +#define INTERMEDIATE_OR 7
>> +#define VIRT_MASK 0x0000ffffffffffffULL
>> +#else
>> +#define NUMBER_OF_LEVELS 3
>> +#define INTERMEDIATE_OR 3
>> +#define VIRT_MASK 0x00000000ffffffffULL
> 
> Long expected constants... Nice!

Just to please you. :-)
They were just moved up some lines as they are used in the new
structure definitions.

Using constants was unavoidable here. ;-)

> 
>> +#endif
>> +
>> +struct grub_xen_mapping_lvl {
>> +  grub_uint64_t virt_start;
>> +  grub_uint64_t virt_end;
>> +  grub_uint64_t pfn_start;
>> +  grub_uint64_t n_pt_pages;
>> +};
>> +
>> +struct grub_xen_mapping {
>> +  grub_uint64_t *where;
>> +  struct grub_xen_mapping_lvl area;
>> +  struct grub_xen_mapping_lvl lvls[NUMBER_OF_LEVELS];
>> +};
>> +
>>  static struct grub_relocator *relocator = NULL;
>>  static grub_uint64_t max_addr;
>>  static grub_dl_t my_mod;
>> @@ -56,9 +79,10 @@ static struct grub_relocator_xen_state state;
>>  static grub_xen_mfn_t *virt_mfn_list;
>>  static struct start_info *virt_start_info;
>>  static grub_xen_mfn_t console_pfn;
>> -static grub_uint64_t *virt_pgtable;
>> -static grub_uint64_t pgtbl_start;
>>  static grub_uint64_t pgtbl_end;
>> +static struct grub_xen_mapping mappings[XEN_MAX_MAPPINGS];
>> +static int n_mappings;
>> +static struct grub_xen_mapping *map_reloc;
>>
>>  #define PAGE_SIZE 4096
>>  #define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
>> @@ -75,100 +99,162 @@ page2offset (grub_uint64_t page)
>>    return page << PAGE_SHIFT;
>>  }
>>
>> -#ifdef __x86_64__
>> -#define NUMBER_OF_LEVELS 4
>> -#define INTERMEDIATE_OR 7
>> -#else
>> -#define NUMBER_OF_LEVELS 3
>> -#define INTERMEDIATE_OR 3
>> -#endif
>> -
>> -static grub_uint64_t
>> -get_pgtable_size (grub_uint64_t total_pages, grub_uint64_t virt_base)
>> +static grub_err_t
>> +get_pgtable_size (grub_uint64_t from, grub_uint64_t to, grub_uint64_t pfn)
>>  {
>> -  if (!virt_base)
>> -    total_pages++;
>> -  grub_uint64_t ret = 0;
>> -  grub_uint64_t ll = total_pages;
>> -  int i;
>> -  for (i = 0; i < NUMBER_OF_LEVELS; i++)
>> -    {
>> -      ll = (ll + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
>> -      /* PAE wants all 4 root directories present.  */
>> -#ifdef __i386__
>> -      if (i == 1)
>> -	ll = 4;
>> -#endif
>> -      ret += ll;
>> -    }
>> -  for (i = 1; i < NUMBER_OF_LEVELS; i++)
>> -    if (virt_base >> (PAGE_SHIFT + i * LOG_POINTERS_PER_PAGE))
>> -      ret++;
>> -  return ret;
>> -}
>> +  struct grub_xen_mapping *map, *map_cmp;
>> +  grub_uint64_t mask, bits;
>> +  int i, m;
>>
>> -static void
>> -generate_page_table (grub_uint64_t *where, grub_uint64_t paging_start,
>> -		     grub_uint64_t paging_end, grub_uint64_t total_pages,
>> -		     grub_uint64_t virt_base, grub_xen_mfn_t *mfn_list)
>> -{
>> -  if (!virt_base)
>> -    paging_end++;
>> +  if (n_mappings == XEN_MAX_MAPPINGS)
>> +    return grub_error (GRUB_ERR_BUG, "too many mapped areas");
>> +
>> +  grub_dprintf ("xen", "get_pgtable_size %d from=%llx, to=%llx, pfn=%llx\n",
>> +		n_mappings, (unsigned long long) from, (unsigned long long) to,
>> +		(unsigned long long) pfn);
>>
>> -  grub_uint64_t lx[NUMBER_OF_LEVELS], lxs[NUMBER_OF_LEVELS];
>> -  grub_uint64_t nlx, nls, sz = 0;
>> -  int l;
>> +  map = mappings + n_mappings;
>> +  grub_memset (map, 0, sizeof (*map));
>>
>> -  nlx = paging_end;
>> -  nls = virt_base >> PAGE_SHIFT;
>> -  for (l = 0; l < NUMBER_OF_LEVELS; l++)
>> +  map->area.virt_start = from & VIRT_MASK;
>> +  map->area.virt_end = (to - 1) & VIRT_MASK;
>> +  map->area.n_pt_pages = 0;
>> +
>> +  for (i = NUMBER_OF_LEVELS - 1; i >= 0; i--)
>>      {
>> -      nlx = (nlx + POINTERS_PER_PAGE - 1) >> LOG_POINTERS_PER_PAGE;
>> -      /* PAE wants all 4 root directories present.  */
>> +      map->lvls[i].pfn_start = pfn + map->area.n_pt_pages;
>> +      if (i == NUMBER_OF_LEVELS - 1)
>> +	{
>> +	  if (n_mappings == 0)
>> +	    {
>> +	      map->lvls[i].virt_start = 0;
>> +	      map->lvls[i].virt_end = VIRT_MASK;
>> +	      map->lvls[i].n_pt_pages = 1;
>> +	      map->area.n_pt_pages++;
>> +	    }
>> +	  continue;
>> +	}
>> +
>> +      bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE;
>> +      mask = (1ULL << bits) - 1;
>> +      map->lvls[i].virt_start = map->area.virt_start & ~mask;
>> +      map->lvls[i].virt_end = map->area.virt_end | mask;
>>  #ifdef __i386__
>> -      if (l == 1)
>> -	nlx = 4;
>> +      /* PAE wants last root directory present. */
>> +      if (i == 1 && to <= 0xc0000000 && n_mappings == 0)
> 
> Ditto.
> 
>> +	map->lvls[i].virt_end = VIRT_MASK;
>>  #endif
>> -      lx[l] = nlx;
>> -      sz += lx[l];
>> -      lxs[l] = nls & (POINTERS_PER_PAGE - 1);
>> -      if (nls && l != 0)
>> -	sz++;
>> -      nls >>= LOG_POINTERS_PER_PAGE;
>> +      for (m = 0; m < n_mappings; m++)
>> +	{
>> +	  map_cmp = mappings + m;
>> +	  if (map_cmp->lvls[i].virt_start == map_cmp->lvls[i].virt_end)
>> +	    continue;
>> +	  if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
>> +	      map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
>> +	   {
>> +	     map->lvls[i].virt_start = 0;
>> +	     map->lvls[i].virt_end = 0;
>> +	     break;
>> +	   }
>> +	   if (map->lvls[i].virt_start >= map_cmp->lvls[i].virt_start &&
>> +	       map->lvls[i].virt_start <= map_cmp->lvls[i].virt_end)
>> +	     map->lvls[i].virt_start = map_cmp->lvls[i].virt_end + 1;
>> +	   if (map->lvls[i].virt_end >= map_cmp->lvls[i].virt_start &&
>> +	       map->lvls[i].virt_end <= map_cmp->lvls[i].virt_end)
>> +	     map->lvls[i].virt_end = map_cmp->lvls[i].virt_start - 1;
>> +	}
>> +      if (map->lvls[i].virt_start < map->lvls[i].virt_end)
>> +	map->lvls[i].n_pt_pages =
>> +	  ((map->lvls[i].virt_end - map->lvls[i].virt_start) >> bits) + 1;
>> +      map->area.n_pt_pages += map->lvls[i].n_pt_pages;
>> +      grub_dprintf ("xen", "get_pgtable_size level %d: virt %llx-%llx %d pts\n",
>> +		    i, (unsigned long long)  map->lvls[i].virt_start,
>> +		    (unsigned long long)  map->lvls[i].virt_end,
>> +		    (int) map->lvls[i].n_pt_pages);
>>      }
>>
>> -  grub_uint64_t lp;
>> -  grub_uint64_t j;
>> -  grub_uint64_t *pg = (grub_uint64_t *) where;
>> -  int pr = 0;
>> +  grub_dprintf ("xen", "get_pgtable_size return: %d page tables\n",
>> +		(int) map->area.n_pt_pages);
>> +
>> +  state.paging_start[n_mappings] = pfn;
>> +  state.paging_size[n_mappings] = map->area.n_pt_pages;
>> +
>> +  return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_uint64_t *
>> +get_pg_table_virt (int mapping, int level)
>> +{
>> +  grub_uint64_t pfn;
>> +  struct grub_xen_mapping *map;
>> +
>> +  map = mappings + mapping;
>> +  pfn = map->lvls[level].pfn_start - map->lvls[NUMBER_OF_LEVELS - 1].pfn_start;
>> +  return map->where + pfn * POINTERS_PER_PAGE;
>> +}
>>
>> -  grub_memset (pg, 0, sz * PAGE_SIZE);
>> +static grub_uint64_t
>> +get_pg_table_prot (int level, grub_uint64_t pfn)
>> +{
>> +  int m;
>> +  grub_uint64_t pfn_s, pfn_e;
>>
>> -  lp = paging_start + lx[NUMBER_OF_LEVELS - 1];
>> -  for (l = NUMBER_OF_LEVELS - 1; l >= 1; l--)
>> +  if (level > 0)
>> +    return INTERMEDIATE_OR;
>> +  for (m = 0; m < n_mappings; m++)
>>      {
>> -      if (lxs[l] || pr)
>> -	pg[0] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
>> -      if (pr)
>> -	pg += POINTERS_PER_PAGE;
>> -      for (j = 0; j < lx[l - 1]; j++)
>> -	pg[j + lxs[l]] = page2offset (mfn_list[lp++]) | INTERMEDIATE_OR;
>> -      pg += lx[l] * POINTERS_PER_PAGE;
>> -      if (lxs[l])
>> -	pr = 1;
>> +      pfn_s = mappings[m].lvls[NUMBER_OF_LEVELS - 1].pfn_start;
>> +      pfn_e = mappings[m].area.n_pt_pages + pfn_s;
>> +      if (pfn >= pfn_s && pfn < pfn_e)
>> +	return 5;
>>      }
>> +  return 7;
> 
> What 7 and 5 means?

Page table protection bits.

> I am exhausted. Please fix above stuff and I will review this patch
> again after fixes.

I'd really like to have the maintainer's opinion on usage of constants
vs. pure numbers. Up to now I have the impression that constants are
used only to abstract i386 vs. x86-64. I wouldn't mind changing that,
but I don't like modifying all my patches which are then rejected due to
that change.

Thanks for doing the review,


Juergen