Re: [PATCH] silo: Add 64-bit support

[Sam Ravnborg <sam@ravnborg.org>]

Hi John.

On Thu, Nov 24, 2016 at 11:59:32PM +0100, John Paul Adrian Glaubitz wrote:
> This patch adds the necessary changes to compile silo
> on sparc64. It adds the required stack bias for stack
> operations and makes sure that all variables are properly
> aligned and have the proper size, both on 32- and 64-bit
> targets. These changes have been verified to work and
> have been used in Debian to ship silo as a 64-bit package.
> 
> Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
>  Rules.make          |  5 ++---
>  common/console.c    |  6 +++---
>  common/divdi3.S     | 11 ++++++++++-
>  common/jmp.S        | 10 ++++++++--
>  common/prom.c       |  4 ++--
>  common/tree.c       |  8 ++++----
>  common/udivdi3.S    | 10 +++++++++-
>  first-isofs/crt0.S  |  8 +++++++-
>  first-isofs/isofs.c |  6 +++---
>  second/crt0.S       | 21 +++++++++++++++++++--
>  second/disk.c       |  8 ++++----
>  second/file.c       |  2 +-
>  second/muldi3.S     | 10 +++++++++-
>  silo/silo.c         | 12 ++++++------
>  14 files changed, 87 insertions(+), 34 deletions(-)
> 
> diff --git a/Rules.make b/Rules.make
> index 0f176db..e46af48 100644
> --- a/Rules.make
> +++ b/Rules.make
> @@ -2,10 +2,9 @@ VERSION=1.4.14
>  IMGVERSION=0.99
>  SHELL=/bin/bash
>  RM=rm -f
> -# We want to force 32-bit builds
> -CC=gcc -m32
> +CC=gcc
>  HOSTCC=gcc
> -LD=ld -m elf32_sparc
> +LD=ld
>  AS=as
>  STRIP=strip
>  NM=nm
> diff --git a/common/console.c b/common/console.c
> index 44d7efb..270caca 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -27,7 +27,7 @@ prom_nbgetchar(void)
>  			i = inc;
>  		break;
>  	case PROM_P1275:
> -		if (p1275_cmd ("read", 3, prom_stdin, &inc, 1) = 1)
> +		if (p1275_cmd ("read", 3, (unsigned int) prom_stdin, &inc, 1) = 1)
This is unrelated to the changelog.

The prototype:
int p1275_cmd (char *service, unsigned fmt, ...)

The above fixes a warning, where the right fix would be to let p1275_cmd()
take a signed paramter.

Likewise for all other cases that cast when calling p1275_cmd().

> +#define _SV save %sp, -STACK_BIAS-0x40, %sp

Spaces arounf "-" please.


>  #define _RV restore
>  #define FLUSH_ALL_WINDOWS \
>  	_SV; _SV; _SV; _SV; _SV; _SV; _SV; \
> @@ -46,7 +52,7 @@ __longjmp:
>  	FLUSH_ALL_WINDOWS
>  	ld [%o0], %o7		/* Return PC.  */
>  	ld [%o0 + 4], %fp	/* Saved SP.  */
> -	sub %fp, 64, %sp	/* Allocate a register save area.  */
> +	sub %fp, 64+STACK_BIAS, %sp	/* Allocate a register save area.  */
Spaces around "+"

>  	tst %o1
>  	be,a 1f
>  	mov 1, %o1
> diff --git a/common/prom.c b/common/prom.c
> index 939bbb9..7585e10 100644
> --- a/common/prom.c
> +++ b/common/prom.c
> @@ -196,7 +196,7 @@ int prom_map(int mode, unsigned long long size,
>  			    P1275_ARG_64B(3) | P1275_ARG_64B(4) |
>  			    P1275_ARG_64B(6) | 7,
>  			    "map",
> -			    prom_get_mmu_ihandle(),
> +			    (unsigned int) prom_get_mmu_ihandle(),
>  			    mode,
>  			    size,
>  			    vaddr,
> @@ -211,7 +211,7 @@ void prom_unmap(unsigned long long size, unsigned long long vaddr)
>  	p1275_cmd("call-method",
>  		  P1275_ARG_64B(2) | P1275_ARG_64B(3) | 4,
>  		  "unmap",
> -		  prom_get_mmu_ihandle(),
> +		  (unsigned int) prom_get_mmu_ihandle(),
>  		  size,
>  		  vaddr);
>  }

In both cases where the static prom_get_mmu_ihandle() is used
the return value has a cast int => unsigned int
If the unsigned variable is required then fix the function.


> diff --git a/common/tree.c b/common/tree.c
> index 56da8b8..65a7743 100644
> --- a/common/tree.c
> +++ b/common/tree.c
> @@ -22,7 +22,7 @@ int prom_getchild(int node)
>  	if (prom_vers != PROM_P1275)
>  		cnode = prom_nodeops->no_child(node);
>  	else
> -		cnode = p1275_cmd ("child", 1, node);
> +		cnode = p1275_cmd ("child", 1, (unsigned int) node);
Cannot see why this is required?

>  		
>  	if (cnode = 0 || cnode = -1)
>  		return 0;
> @@ -43,7 +43,7 @@ int prom_getsibling(int node)
>  	if (prom_vers != PROM_P1275)
>  		sibnode = prom_nodeops->no_nextnode(node);
>  	else
> -		sibnode = p1275_cmd ("peer", 1, node);
> +		sibnode = p1275_cmd ("peer", 1, (unsigned int) node);
Cannot see why this is required?

>  		
>  	if (sibnode = 0 || sibnode = -1)
>  		return 0;
> @@ -64,7 +64,7 @@ int prom_getproplen(int node, char *prop)
>  		if (prom_vers != PROM_P1275)
>  			ret = prom_nodeops->no_proplen(node, prop);
>  		else
> -			ret = p1275_cmd ("getproplen", 2, node, prop);
> +			ret = p1275_cmd ("getproplen", 2, (unsigned int) node, prop)
Likewise...


>  	}
>  	return ret;
>  }
> @@ -85,7 +85,7 @@ int prom_getproperty(int node, char *prop, char *buffer, int bufsize)
>  		if (prom_vers != PROM_P1275)
>  			ret = prom_nodeops->no_getprop(node, prop, buffer);
>  		else
> -			ret = p1275_cmd ("getprop", 4, node, prop, buffer, bufsize);
> +			ret = p1275_cmd ("getprop", 4, (unsigned int) node, prop, buffer, bufsize);
Likewise..
Same ges for reaming uses of cast to unisged int in p1275_cmd() calls.
Maybe I have missed something obvious?

> --- a/second/disk.c
> +++ b/second/disk.c
> @@ -79,7 +79,7 @@ int silo_disk_open(char *device)
>  
>          	fd = p1275_cmd ("open", 1, device);
>          	if ((unsigned)(fd + 1) > 1) {
> -		    node = p1275_cmd ("instance-to-package", 1, fd);
> +		    node = p1275_cmd ("instance-to-package", 1, (unsigned)fd);
New pattern. Now a cast to (unsigned) not (unsigend int) as before.
Goes for the rest of the file.

> diff --git a/second/file.c b/second/file.c
> index c7c1ed2..379af2f 100644
> --- a/second/file.c
> +++ b/second/file.c
> @@ -193,7 +193,7 @@ int dump_block (blk_t * blocknr, int blockcnt)
>                  }
>                  last_blockcnt = -1;
>              }
> -            if ((char *)filebuffer + (block_cnt + ((*blocknr) ? (blockcnt - last_blockcnt - 1) : 0)) * bs > filelimit) {
> +            if ((unsigned int)filebuffer + (block_cnt + ((*blocknr) ? (blockcnt - last_blockcnt - 1) : 0)) * bs > filelimit) {

That if () needs a rewrite to somethign readable - which is not
the fault of this patch.

> +++ b/second/muldi3.S
> @@ -17,11 +17,19 @@ along with GNU CC; see the file COPYING.  If not, write to
>  the Free Software Foundation, 59 Temple Place - Suite 330,
>  Boston, MA 02111-1307, USA.  */
>  
> +#if __WORDSIZE = 32
> +# define STACK_BIAS 0
> +#else
> +# define STACK_BIAS 2047
> +#endif

The above have been repeated a few times now.
Stuff it in a header so we have on place to define this.


> diff --git a/silo/silo.c b/silo/silo.c
> index 9728af2..20b7a0c 100644
> --- a/silo/silo.c
> +++ b/silo/silo.c
> @@ -107,14 +107,14 @@ static int allow_confchk_fail = 0;
>  
>  /* This is just so that we don't have to fight with incompatible ufs_fs.h headers */
>  #define SILO_UFS_MAGIC 0x00011954
> -struct silo_ufs_super_block {
> +struct __attribute__((packed)) silo_ufs_super_block {
>  	unsigned char xxx1[36];
>  	unsigned int fs_fsize;
>  	unsigned char xxx2[1332];
>  	unsigned int fs_magic;
>  };
>                                  
> -struct sun_disklabel {
> +struct __attribute__((packed)) sun_disklabel {
>      unsigned char info[128];	/* Informative text string */
>      unsigned char spare[292];	/* Boot information etc. */
>      unsigned short rspeed;	/* Disk rotational speed */
> @@ -127,9 +127,9 @@ struct sun_disklabel {
>      unsigned short ntrks;	/* Tracks per cylinder */
>      unsigned short nsect;	/* Sectors per track */
>      unsigned char spare3[4];	/* Even more magic... */
> -    struct sun_partition {
> -	unsigned long start_cylinder;
> -	unsigned long num_sectors;
> +    struct __attribute__((packed)) sun_partition {
> +	unsigned int start_cylinder;
> +	unsigned int num_sectors;
>      } partitions[8];
>      unsigned short magic;	/* Magic number */
>      unsigned short csum;	/* Label xor'd checksum */

The kernel variant uses __be16/__be32 - we should maybe do the same here.
And the kernel do not need any packer attribute?!?

> @@ -205,7 +205,7 @@ int check_fs (int fd)
>  {
>      struct silo_ufs_super_block ufs;
>      struct ext2_super_block sb;	/* Super Block Info */
> -    struct romfs_super_block {
> +    struct __attribute__((packed)) romfs_super_block {
>  	__u32 word0;
>  	__u32 word1;
>  	__u32 size;
See how the kernel define this wwithout packed attribute. Maybe better?

	Sam