Re: [OpenBIOS] [PATCH] Fix some unaligned memory accesses

[Stefan Reinauer]

* Aurelien Jarno  [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.
>
> */