[OpenBIOS] [PATCH] Fix some unaligned memory accesses

[Stefan Reinauer]

* Aurelien Jarno <aurelien at aurel32.net> [070401 22:05]:
> Hi all,
> 
> The current SVN version currently fails to work on machines that have
> strict alignment requirements, for example the SPARC target. This is due
> to the way the struct fat_bpb in fs/grubfs/fat.h is defined and accessed
> (through FAT_CVT_U16) to avoid padding. Some 16 bits fields like 
> bytes_per_sect are thus accessed unaligned.

Why is this causing trouble? bytes_per_sect was defined as an array of
u8, which you would not need to access aligned anyways. Is gcc doing
some assumptions here?
 
> The patch below takes the same approach as for other structures in
> openbios, it declares the structure as packed, and lets GCC do the right
> things to access those unaligned field.
 
Thanks, and sorry the patch is taking so long to get in. It will be
checked in in a couple of days.

> Index: fs/grubfs/fsys_fat.c
> ===================================================================
> --- fs/grubfs/fsys_fat.c	(révision 116)
> +++ fs/grubfs/fsys_fat.c	(copie de travail)
> @@ -75,23 +75,23 @@
>    if (bpb.sects_per_clust == 0)
>      return 0;
>    
> -  FAT_SUPER->sectsize_bits = log2 (FAT_CVT_U16 (bpb.bytes_per_sect));
> +  FAT_SUPER->sectsize_bits = log2 (bpb.bytes_per_sect);
>    FAT_SUPER->clustsize_bits
>      = FAT_SUPER->sectsize_bits + log2 (bpb.sects_per_clust);
>    
>    /* Fill in info about super block */
> -  FAT_SUPER->num_sectors = FAT_CVT_U16 (bpb.short_sectors) 
> -    ? FAT_CVT_U16 (bpb.short_sectors) : bpb.long_sectors;
> +  FAT_SUPER->num_sectors = bpb.short_sectors 
> +    ? bpb.short_sectors : bpb.long_sectors;
>    
>    /* FAT offset and length */
> -  FAT_SUPER->fat_offset = FAT_CVT_U16 (bpb.reserved_sects);
> +  FAT_SUPER->fat_offset = bpb.reserved_sects;
>    FAT_SUPER->fat_length = 
>      bpb.fat_length ? bpb.fat_length : bpb.fat32_length;
>    
>    /* Rootdir offset and length for FAT12/16 */
>    FAT_SUPER->root_offset = 
>      FAT_SUPER->fat_offset + bpb.num_fats * FAT_SUPER->fat_length;
> -  FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * FAT_CVT_U16(bpb.dir_entries);
> +  FAT_SUPER->root_max = FAT_DIRENTRY_LENGTH * bpb.dir_entries;
>    
>    /* Data offset and number of clusters */
>    FAT_SUPER->data_offset = 
> @@ -105,7 +105,7 @@
>    if (!bpb.fat_length)
>      {
>        /* This is a FAT32 */
> -      if (FAT_CVT_U16(bpb.dir_entries))
> +      if (bpb.dir_entries)
>   	return 0;
>        
>        if (bpb.flags & 0x0080)
> @@ -144,8 +144,8 @@
>    
>    /* Now do some sanity checks */
>    
> -  if (FAT_CVT_U16(bpb.bytes_per_sect) != (1 << FAT_SUPER->sectsize_bits)
> -      || FAT_CVT_U16(bpb.bytes_per_sect) != SECTOR_SIZE
> +  if (bpb.bytes_per_sect != (1 << FAT_SUPER->sectsize_bits)
> +      || bpb.bytes_per_sect != SECTOR_SIZE
>        || bpb.sects_per_clust != (1 << (FAT_SUPER->clustsize_bits
>   				       - FAT_SUPER->sectsize_bits))
>        || FAT_SUPER->num_clust <= 2
> Index: fs/grubfs/fat.h
> ===================================================================
> --- fs/grubfs/fat.h	(révision 116)
> +++ fs/grubfs/fat.h	(copie de travail)
> @@ -37,12 +37,12 @@
>  	__s8	ignored[3];	/* Boot strap short or near jump */
>  	__s8	system_id[8];	/* Name - can be used to special case
>  				   partition manager volumes */
> -	__u8	bytes_per_sect[2];	/* bytes per logical sector */
> +	__u16	bytes_per_sect;	/* bytes per logical sector */
>  	__u8	sects_per_clust;/* sectors/cluster */
> -	__u8	reserved_sects[2];	/* reserved sectors */
> +	__u16	reserved_sects;	/* reserved sectors */
>  	__u8	num_fats;	/* number of FATs */
> -	__u8	dir_entries[2];	/* root directory entries */
> -	__u8	short_sectors[2];	/* number of sectors */
> +	__u16	dir_entries;	/* root directory entries */
> +	__u16	short_sectors;	/* number of sectors */
>  	__u8	media;		/* media code (unused) */
>  	__u16	fat_length;	/* sectors/FAT */
>  	__u16	secs_track;	/* sectors per track */
> @@ -53,15 +53,13 @@
>  	/* The following fields are only used by FAT32 */
>  	__u32	fat32_length;	/* sectors/FAT */
>  	__u16	flags;		/* bit 8: fat mirroring, low 4: active fat */
> -	__u8	version[2];	/* major, minor filesystem version */
> +	__u16	version;	/* major, minor filesystem version */
>  	__u32	root_cluster;	/* first cluster in root directory */
>  	__u16	info_sector;	/* filesystem info sector */
>  	__u16	backup_boot;	/* backup boot sector */
>  	__u16	reserved2[6];	/* Unused */
> -};
> +} __attribute__ ((packed));
>  
> -#define FAT_CVT_U16(bytarr) (* (__u16*)(bytarr))
> -
>  /*
>   *  Defines how to differentiate a 12-bit and 16-bit FAT.
>   */
> 
> -- 
>   .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
>  : :' :  Debian developer           | Electrical Engineer
>  `. `'   aurel32 at debian.org         | aurelien at aurel32.net
>    `-    people.debian.org/~aurel32 | www.aurel32.net
> 
> -- 
> OpenBIOS                 http://openbios.org/
> Mailinglist:  http://lists.openbios.org/mailman/listinfo
> Free your System - May the Forth be with you
> 

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866