Re: [PATCH] Fixed ieee1275 console

[Marco Gerards <mgerards@xs4all.nl>]

"Marcin Kurek" <morgoth6@gmail.com> writes:

Hi Marcin,

> Finaly I found a few free minutes to look at ofconsole as it never
> correctly work for me. There was 3 problems on my Pegasos:

:-)

> 1) Backspace key not works with USB keyboard.

It does with PS/2?  Are you sure this fix works on apple firmware?

> 2) menu looks ugly like hell as it contains some random characters in
> place of borders.

You had some code for this, right?

> 3) Console works wrong (the cursor position was wrong after some
> commands eg. ls and it displays only a black hole when grub prints
> more than one screen of text)

getxy was the problem, right?

> I am not sure how correct my fix is for non Efika/ODW machines, but I
> tested it on both without any problems.

Great.  Although for us it is important that it works on all machines,
which makes some things a bit less trivial to fix...

> In grub_ofconsole_writeesc() I think it would be good idea to use
> single "write" command, but this is a cosmetics only I guess. Also in
> grub_ofconsole_getxy() the -1 for grub_curr_x seems to be wrong for
> me.

Yes, although I wonder why I used multiple commands at the time.
Perhaps there was a problem with this that we forgot to document :-/

> Replaced borders characters in grub_ofconsole_putchar() by '-', '|',
> etc. Maybe not perfect, but looks definitly better than before [2].
> Also fixed cursor position tracking (grub_curr_x, grub_curr_y) which
> fix both console problems [3]

Can you separate the getxy patch?  It is quite small and fixes an
annoying bug.  It should be committed soon.

> Handle 127 keycode as backspace key in grub_ofconsole_readkey() which fix [1]
>
> BTW Also attach my previous patches with [PATCH] in topic this time.

:-)

Can you please write changelog entries for your patches?  It's
important to us.

> --- Marcin 'Morgoth' Kurek ---
>
> diff -urN grub2.org/fs/affs.c grub2/fs/affs.c
> --- grub2.org/fs/affs.c	2007-08-02 20:40:36.000000000 +0200
> +++ grub2/fs/affs.c	2007-09-15 10:23:35.550133111 +0200
> @@ -25,6 +25,7 @@
>  #include <grub/dl.h>
>  #include <grub/types.h>
>  #include <grub/fshelp.h>
> +#include <grub/partition.h>
>  
>  /* The affs bootblock.  */
>  struct grub_affs_bblock
> @@ -97,6 +98,9 @@
>    struct grub_fshelp_node diropen;
>    grub_disk_t disk;
>  
> +  /* Size in sectors */
> +  grub_uint64_t len;
> +
>    /* Blocksize in sectors.  */
>    int blocksize;
>  
> @@ -170,10 +174,17 @@
>    int checksumr = 0;
>    int blocksize = 0;
>  
> +
>    data = grub_malloc (sizeof (struct grub_affs_data));
>    if (!data)
>      return 0;
>  
> +  /* total_sectors are not valid on ieee1275 */
> +  if(disk->partition)
> +    data->len = grub_partition_get_len (disk->partition);
> +  else
> +    data->len = disk->total_sectors;

Can you please fix total_sectors instead?  This patch does not fix the
problem, but it moves the problem elsewhere.

>    /* Read the bootblock.  */
>    grub_disk_read (disk, 0, 0, sizeof (struct grub_affs_bblock),
>  		  (char *) &data->bblock);
> @@ -194,12 +205,6 @@
>        goto fail;
>      }
>  
> -  /* Read the bootblock.  */
> -  grub_disk_read (disk, 0, 0, sizeof (struct grub_affs_bblock),
> -		  (char *) &data->bblock);
> -  if (grub_errno)
> -    goto fail;
> -

It was read twice!?  Silly me... :-)

>    /* No sane person uses more than 8KB for a block.  At least I hope
>       for that person because in that case this won't work.  */
>    rootblock = grub_malloc (GRUB_DISK_SECTOR_SIZE * 16);
> @@ -209,7 +214,7 @@
>    rblock = (struct grub_affs_rblock *) rootblock;
>  
>    /* Read the rootblock.  */
> -  grub_disk_read (disk, (disk->total_sectors >> 1) + blocksize, 0,
> +  grub_disk_read (disk, (data->len >> 1) + blocksize, 0,
>  		  GRUB_DISK_SECTOR_SIZE * 16, (char *) rootblock);
>    if (grub_errno)
>      goto fail;
> @@ -241,7 +246,7 @@
>    data->disk = disk;
>    data->htsize = grub_be_to_cpu32 (rblock->htsize);
>    data->diropen.data = data;
> -  data->diropen.block = (disk->total_sectors >> 1);
> +  data->diropen.block = (data->len >> 1);
>  
>    grub_free (rootblock);
>  
> @@ -522,7 +527,7 @@
>      {
>        /* The rootblock maps quite well on a file header block, it's
>  	 something we can use here.  */
> -      grub_disk_read (data->disk, disk->total_sectors >> 1,
> +      grub_disk_read (data->disk, data->len >> 1,
>  		      data->blocksize * (GRUB_DISK_SECTOR_SIZE
>  					 - GRUB_AFFS_FILE_LOCATION),
>  		      sizeof (file), (char *) &file);
>
> diff -urN grub2.org/conf/powerpc-ieee1275.mk grub2/conf/powerpc-ieee1275.mk

>  #endif /* ! GRUB_LOADER_MACHINE_HEADER */
> diff -urN grub2.org/loader/powerpc/ieee1275/ofboot.c grub2/loader/powerpc/ieee1275/ofboot.c
> --- grub2.org/loader/powerpc/ieee1275/ofboot.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/loader/powerpc/ieee1275/ofboot.c	2007-09-15 04:45:47.960133111 +0200
> @@ -0,0 +1,135 @@
> +/* ofboot.c - OF boot */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2003, 2004, 2005, 2007  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/>.
> + */
> +
> +#include <grub/loader.h>
> +#include <grub/dl.h>
> +#include <grub/mm.h>
> +#include <grub/rescue.h>
> +#include <grub/misc.h>
> +#include <grub/ieee1275/ieee1275.h>
> +#include <grub/machine/loader.h>
> +
> +static grub_dl_t my_mod;
> +
> +static char *ofboot_args;
> +
> +static grub_err_t
> +grub_ofboot_boot (void)
> +{
> +  grub_err_t err;
> +
> +  err = grub_ieee1275_interpret("go", 0);
> +
> +  return err;
> +}
> +
> +static grub_err_t
> +grub_ofboot_release_mem (void)
> +{
> +  grub_free (ofboot_args);
> +  ofboot_args = 0;
> +      
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_ofboot_unload (void)
> +{
> +  grub_err_t err;
> +
> +  err = grub_ofboot_release_mem ();
> +  grub_dl_unref (my_mod);
> +
> +  return err;
> +}
> +
> +void
> +grub_rescue_cmd_ofboot (int argc, char *argv[])
> +{
> +  int i;
> +  int size;
> +  grub_err_t err;
> +  grub_ieee1275_cell_t res;
> +  char *dest;
> +
> +  grub_dl_ref (my_mod);
> +
> +  if (argc == 0)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, "no file specified");
> +      goto out;
> +    }
> +
> +  /* Release the previously used memory.  */
> +  grub_loader_unset ();
> +
> +  size = sizeof("load") + 1;
> +  for (i = 0; i < argc; i++)
> +    size += grub_strlen (argv[i]) + 1;
> +
> +  ofboot_args = grub_malloc (size);
> +  if (! ofboot_args)
> +    goto out;
> +
> +  dest = grub_stpcpy (ofboot_args, "load");
> +  for (i = 0; i < argc; i++)
> +    {
> +      *dest++ = ' ';
> +      dest = grub_stpcpy (dest, argv[i]);
> +    }
> +
> +  err = grub_ieee1275_interpret(ofboot_args, &res);
> +  if (err || res)
> +  {
> +    grub_error (GRUB_ERR_UNKNOWN_OS, "Failed to \"load\"");
> +    goto out;
> +  }
> +
> +  err = grub_ieee1275_interpret("init-program", &res);
> +  if (err || res)
> +  {
> +    grub_error (GRUB_ERR_UNKNOWN_OS, "Failed to \"init-program\"");
> +    goto out;
> +  }
> +
> +out:
> +
> +  if (grub_errno != GRUB_ERR_NONE)
> +    {
> +      grub_ofboot_release_mem ();
> +      grub_dl_unref (my_mod);
> +    }
> +  else
> +    {
> +      grub_loader_set (grub_ofboot_boot, grub_ofboot_unload, 1);
> +    }
> +}
> +
> +\f
> +GRUB_MOD_INIT(ofboot)
> +{
> +  grub_rescue_register_command ("ofboot", grub_rescue_cmd_ofboot,
> +				"load using OF interface");
> +  my_mod = mod;
> +}
> +
> +GRUB_MOD_FINI(ofboot)
> +{
> +  grub_rescue_unregister_command ("ofboot");
> +}

It would be better to make a loader out of this.  In that case it
better integrates into GRUB 2.

> diff -urN grub2.org/loader/powerpc/ieee1275/ofboot_normal.c grub2/loader/powerpc/ieee1275/ofboot_normal.c
> --- grub2.org/loader/powerpc/ieee1275/ofboot_normal.c	1970-01-01 01:00:00.000000000 +0100
> +++ grub2/loader/powerpc/ieee1275/ofboot_normal.c	2007-09-14 22:54:03.573217034 +0200
> @@ -0,0 +1,48 @@
> +/* ofboot_normal.c - OF boot */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2004,2007  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/>.
> + */
> +
> +#include <grub/normal.h>
> +#include <grub/dl.h>
> +#include <grub/machine/loader.h>
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +static grub_err_t
> +grub_cmd_ofboot (struct grub_arg_list *state  __attribute__ ((unused)),
> +		int argc, char **args)
> +{
> +  grub_rescue_cmd_ofboot (argc, args);
> +  return GRUB_ERR_NONE;
> +}
> +
> +GRUB_MOD_INIT(ofboot_normal)
> +{
> +  (void) mod;
> +  grub_register_command ("ofboot", grub_cmd_ofboot, GRUB_COMMAND_FLAG_BOTH,
> +			 "ofboot [ARGS...]",
> +			 "Loads using OF interface", options);
> +}
> +
> +GRUB_MOD_FINI(ofboot_normal)
> +{
> +  grub_unregister_command ("ofboot");
> +}
>
> --- ../Cache/cvs/grub2/term/ieee1275/ofconsole.c	2007-07-22 11:05:11.000000000 +0200
> +++ grub2/term/ieee1275/ofconsole.c	2007-10-01 12:12:47.075370575 +0200
> @@ -63,29 +63,81 @@
>  static void
>  grub_ofconsole_writeesc (const char *str)
>  {
> -  while (*str)
> -    {
> -      char chr = *(str++);
> -      grub_ieee1275_write (stdout_ihandle, &chr, 1, 0);
> -    }
> -  
> +  int len = grub_strlen(str);

grub_strlen (str)

> +  grub_ieee1275_write (stdout_ihandle, str, len, 0);
>  }
>  
>  static void
>  grub_ofconsole_putchar (grub_uint32_t c)
>  {
> -  char chr = c;
> -  if (c == '\n')
> -    {
> +  char chr;
> +
> +  switch (c)
> +  {
> +    case GRUB_TERM_DISP_LEFT:
> +      c = '<';
> +      break;
> +    case GRUB_TERM_DISP_UP:
> +      c = '^';
> +      break;
> +    case GRUB_TERM_DISP_RIGHT:
> +      c = '>';
> +      break;
> +    case GRUB_TERM_DISP_DOWN:
> +      c = 'v';
> +      break;
> +    case GRUB_TERM_DISP_HLINE:
> +      c = '-';
> +      break;
> +    case GRUB_TERM_DISP_VLINE:
> +      c = '|';
> +      break;
> +    case GRUB_TERM_DISP_UL:
> +    case GRUB_TERM_DISP_UR:
> +    case GRUB_TERM_DISP_LL:
> +    case GRUB_TERM_DISP_LR:
> +      c = '+';
> +      break;
> +    case '\t':
> +      c = ' ';
> +      break;
> +
> +    default:
> +      /* of does not support Unicode.  */
> +      if (c > 0x7f)
> +        c = '?';
> +      break;
> +  }
> +
> +  switch(c)
> +  {
> +    case '\a':
> +      break;            
> +    case '\n':
> +      grub_putcode ('\r');
>        grub_curr_y++;
> +
> +      if(grub_curr_y > (grub_ofconsole_height - 1))
> +        grub_curr_y -= 4; /* Is this realy correct for all OF versions around ? */
> +      break;
> +    case '\r':
>        grub_curr_x = 0;
> -    }
> -  else
> -    {
> +      break;
> +    case '\b':
> +      if(grub_curr_x > 0)
> +        grub_curr_x--;
> +      break;
> +
> +    default:
> +
> +      if (grub_curr_x >= (grub_ofconsole_width - 1))
> +        grub_putcode ('\n');
> +
>        grub_curr_x++;
> -      if (grub_curr_x > grub_ofconsole_width)
> -	grub_putcode ('\n');
> -    }
> +      break;
> +  }                                                                                                                                                                        
> +
> +  chr = c;
>    grub_ieee1275_write (stdout_ihandle, &chr, 1, 0);
>  }
>  
> @@ -137,43 +189,56 @@
>  
>    grub_ieee1275_read (stdin_ihandle, &c, 1, &actual);
>  
> -  if (actual > 0 && c == '\e')
> +  if (actual > 0)
>      {
> -      grub_ieee1275_read (stdin_ihandle, &c, 1, &actual);
> -      if (actual <= 0)
> -	{
> -	  *key = '\e';
> -	  return 1;
> -	}
> +      if (c != '\e')
> +      {
> +        switch(c)
> +        {
> +          case 127:
> +            /* Backspace */
> +            c = '\b';
> +            break;
> +        }

Is this also true for the Apple firmwares?  I do not fix this for one
firmware and break it for the other...

> +      }
> +      else
> +      {
> +        grub_ieee1275_read (stdin_ihandle, &c, 1, &actual);
> +        if (actual <= 0)
> +	  {
> +	    *key = '\e';
> +	    return 1;
> +	  }
>        
> -      if (c != 91)
> -	return 0;
> +        if (c != 91)
> +	  return 0;
>        
> -      grub_ieee1275_read (stdin_ihandle, &c, 1, &actual);
> -      if (actual <= 0)
> -	return 0;
> +        grub_ieee1275_read (stdin_ihandle, &c, 1, &actual);
> +        if (actual <= 0)
> +	  return 0;
>        
> -      switch (c)
> -	{
> -	case 65:
> -	  /* Up: Ctrl-p.  */
> -	  c = 16; 
> -	  break;
> -	case 66:
> -	  /* Down: Ctrl-n.  */
> -	  c = 14;
> -	  break;
> -	case 67:
> -	  /* Right: Ctrl-f.  */
> -	  c = 6;
> -	  break;
> -	case 68:
> -	  /* Left: Ctrl-b.  */
> -	  c = 2;
> -	  break;
> -	}
> +        switch (c)
> +	  {
> +	  case 65:
> +	    /* Up: Ctrl-p.  */
> +	    c = 16; 
> +	    break;
> +	  case 66:
> +	    /* Down: Ctrl-n.  */
> +	    c = 14;
> +	    break;
> +	  case 67:
> +	    /* Right: Ctrl-f.  */
> +	    c = 6;
> +	    break;
> +	  case 68:
> +	    /* Left: Ctrl-b.  */
> +	    c = 2;
> +	    break;
> +	  }
> +      }
>      }
> -  
> +
>    *key = c;
>    return actual > 0;
>  }
> @@ -217,7 +282,7 @@
>  static grub_uint16_t
>  grub_ofconsole_getxy (void)
>  {
> -  return ((grub_curr_x - 1) << 8) | grub_curr_y;
> +  return (grub_curr_x << 8) | grub_curr_y;
>  }

Most likely getxy is broken for every firmware?

--
Marco