| Message ID | 1390913212-11217-1-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Jan 28, 2014 at 01:46:52PM +0100, Piotr Wilczek wrote: > This patch fixes part_efi code to avoid unaligned access exception > on some ARM platforms. > > Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > CC: Tom Rini <trini@ti.com> > CC: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > Chnages for V2: > - used put_unaligned to copy value; > - use __aligned to align local array; > > disk/part_efi.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Sorry. After re-reading the thread here again I've posted a patch to globally switch to -mno-unaligned-access for our armv7 tunes instead. Thanks for your effort however!
On 01/28/2014 01:46 PM, Piotr Wilczek wrote: > This patch fixes part_efi code to avoid unaligned access exception > on some ARM platforms. > > Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > CC: Tom Rini <trini@ti.com> > CC: Albert ARIBAUD <albert.u.boot@aribaud.net> > --- > Chnages for V2: > - used put_unaligned to copy value; > - use __aligned to align local array; > > disk/part_efi.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/disk/part_efi.c b/disk/part_efi.c > index 9c33ae7..eb2cd57 100644 > --- a/disk/part_efi.c > +++ b/disk/part_efi.c > @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) > p_mbr->signature = MSDOS_MBR_SIGNATURE; > p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT; > p_mbr->partition_record[0].start_sect = 1; > - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; > + put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects); > > /* Write MBR sector to the MMC device */ > if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { > @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, > #ifdef CONFIG_PARTITION_UUIDS > char *str_uuid; > #endif > + static efi_guid_t basic_guid __aligned(0x04) = > + PARTITION_BASIC_DATA_GUID; > > for (i = 0; i < parts; i++) { > /* partition starting lba */ > @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, > gpt_e[i].ending_lba = cpu_to_le64(offset - 1); > > /* partition type GUID */ > - memcpy(gpt_e[i].partition_type_guid.b, > - &PARTITION_BASIC_DATA_GUID, 16); > + gpt_e[i].partition_type_guid = basic_guid; > > #ifdef CONFIG_PARTITION_UUIDS > str_uuid = partitions[i].uuid; > Sorry, I sent my tested-by on a different thread: http://patchwork.ozlabs.org/patch/319649/ Tested on i.MX6 (armv7) custom board and it is working fine without the -mno-unaligned-access flag. Tested-by: Hector Palacios <hector.palacios@digi.com> Replying to Tom's question in that other thread, regarding the issue: > Can you give me some steps on how to hit this bug? I believe it's a bug > and I believe we need to fix it, I just want to investigate a few things > while we've got a trigger case right now. Thanks! GPT partition table needs to write a protective MBR to sector 0. The MBR structure has four partition entries (each occupying 16 bytes) at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure of each partition entry is defined at include/part_efi.h struct partition { u8 boot_ind; /* 0x80 - active */ u8 head; /* starting head */ u8 sector; /* starting sector */ u8 cyl; /* starting cylinder */ u8 sys_ind; /* What partition type */ u8 end_head; /* end head */ u8 end_sector; /* end sector */ u8 end_cyl; /* end cylinder */ __le32 start_sect; /* starting sector counting from 0 */ __le32 nr_sects; /* nr of sectors in partition */ } __packed; showing eight 1-byte fields and two 4-byte fields. Since the offsets for each partition entry are unaligned, the last two fields (which are 32bit) are unaligned as well. But it's not an error, it's just the specification of the MBR requires these fields to be at those exact offsets. So the usage of unaligned macros for accessing those fields is justified. [1] http://en.wikipedia.org/wiki/Master_boot_record#Sector_layout Best regards, -- Hector Palacios
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/19/2014 09:56 AM, Hector Palacios wrote: > On 01/28/2014 01:46 PM, Piotr Wilczek wrote: >> This patch fixes part_efi code to avoid unaligned access exception >> on some ARM platforms. >> >> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> CC: Tom Rini <trini@ti.com> >> CC: Albert ARIBAUD <albert.u.boot@aribaud.net> >> --- >> Chnages for V2: >> - used put_unaligned to copy value; >> - use __aligned to align local array; >> >> disk/part_efi.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/disk/part_efi.c b/disk/part_efi.c >> index 9c33ae7..eb2cd57 100644 >> --- a/disk/part_efi.c >> +++ b/disk/part_efi.c >> @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t >> *dev_desc) >> p_mbr->signature = MSDOS_MBR_SIGNATURE; >> p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT; >> p_mbr->partition_record[0].start_sect = 1; >> - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; >> + put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects); >> >> /* Write MBR sector to the MMC device */ >> if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { >> @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, >> #ifdef CONFIG_PARTITION_UUIDS >> char *str_uuid; >> #endif >> + static efi_guid_t basic_guid __aligned(0x04) = >> + PARTITION_BASIC_DATA_GUID; >> >> for (i = 0; i < parts; i++) { >> /* partition starting lba */ >> @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, >> gpt_e[i].ending_lba = cpu_to_le64(offset - 1); >> >> /* partition type GUID */ >> - memcpy(gpt_e[i].partition_type_guid.b, >> - &PARTITION_BASIC_DATA_GUID, 16); >> + gpt_e[i].partition_type_guid = basic_guid; >> >> #ifdef CONFIG_PARTITION_UUIDS >> str_uuid = partitions[i].uuid; >> > > Sorry, I sent my tested-by on a different thread: > http://patchwork.ozlabs.org/patch/319649/ > > Tested on i.MX6 (armv7) custom board and it is working fine without > the -mno-unaligned-access flag. > > Tested-by: Hector Palacios <hector.palacios@digi.com> > > Replying to Tom's question in that other thread, regarding the issue: > >> Can you give me some steps on how to hit this bug? I believe it's a bug >> and I believe we need to fix it, I just want to investigate a few things >> while we've got a trigger case right now. Thanks! > > GPT partition table needs to write a protective MBR to sector 0. > The MBR structure has four partition entries (each occupying 16 bytes) > at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure > of each partition entry is defined at include/part_efi.h > > struct partition { > u8 boot_ind; /* 0x80 - active */ > u8 head; /* starting head */ > u8 sector; /* starting sector */ > u8 cyl; /* starting cylinder */ > u8 sys_ind; /* What partition type */ > u8 end_head; /* end head */ > u8 end_sector; /* end sector */ > u8 end_cyl; /* end cylinder */ > __le32 start_sect; /* starting sector counting from 0 */ > __le32 nr_sects; /* nr of sectors in partition */ > } __packed; > > showing eight 1-byte fields and two 4-byte fields. Since the offsets for > each partition entry are unaligned, the last two fields (which are > 32bit) are unaligned as well. But it's not an error, it's just the > specification of the MBR requires these fields to be at those exact > offsets. So the usage of unaligned macros for accessing those fields is > justified. Right. I would have sworn I used the GPT commands since we've dropped - -mno-unaligned-access but I'll just go re-test locally now then, thanks. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTBMfWAAoJENk4IS6UOR1Wm/IP/1sOKxdJvLljPxOQ5myKhT0k PWFMVuN9ZomSkr9+yYt87cVtz28OV42dqVIig7i96TYbzA19AxWY6jWVlPxlmbAZ 0qIwILgogtytJPDad/4NMWXy8GyHP6f9iYIui/DRImsZaErSek+egEshf46M5oyH 0Fd6H/HcpLlJ1QydN1jQzKSppS3HadlwVKEQqC2vN2WjKv3li/m84xoSdiNty2Lj WgqEBOTfbS5dYQub72WiiaCvs2QVBAP2jRk1Lmtu/gj/L5Av0uc+J8JFV6+Un+Tt VNUKigG8PdLonex6gCOwpULX6Lft+9x0YWJXIp0ow3JFStpxbHPgMqqKkVyC7Jgp n14dHO4mG5ZdT2m0y/Tyu1sg7tllTPtW2k+GBsRa8vnOzHq11I6/wQ712S6BK4aB jjuQsxOoxbGSzMEpMrotf4knc0tuL72Lh46Lz1ksRZNdu7NGOAO+DjGuXnzKn8H4 oEB6aFEewaMV8o2XBNaMddMg8MyPtVC4qZiN2uuMMKjxkTukKUCV9xCl9YkJLAV2 oXdVtEoczjWY6made42olvTeev5R1NDD1ex63q0sZIylJQrQSjMju+1PpQQxAwAh /Kv5xn+13JtX7eaHY2oogHRLmLB5Cp2EUwg/ZVzArNTnlVdzOsr72v7BLi/fz6T2 NA5qIFUntQunN2OweEE+ =32uL -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/19/2014 10:03 AM, Tom Rini wrote: > On 02/19/2014 09:56 AM, Hector Palacios wrote: >> On 01/28/2014 01:46 PM, Piotr Wilczek wrote: >>> This patch fixes part_efi code to avoid unaligned access exception >>> on some ARM platforms. >>> >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> CC: Tom Rini <trini@ti.com> >>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net> >>> --- >>> Chnages for V2: >>> - used put_unaligned to copy value; >>> - use __aligned to align local array; >>> >>> disk/part_efi.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/disk/part_efi.c b/disk/part_efi.c >>> index 9c33ae7..eb2cd57 100644 >>> --- a/disk/part_efi.c >>> +++ b/disk/part_efi.c >>> @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t >>> *dev_desc) >>> p_mbr->signature = MSDOS_MBR_SIGNATURE; >>> p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT; >>> p_mbr->partition_record[0].start_sect = 1; >>> - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; >>> + put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects); >>> >>> /* Write MBR sector to the MMC device */ >>> if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { >>> @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, >>> #ifdef CONFIG_PARTITION_UUIDS >>> char *str_uuid; >>> #endif >>> + static efi_guid_t basic_guid __aligned(0x04) = >>> + PARTITION_BASIC_DATA_GUID; >>> >>> for (i = 0; i < parts; i++) { >>> /* partition starting lba */ >>> @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, >>> gpt_e[i].ending_lba = cpu_to_le64(offset - 1); >>> >>> /* partition type GUID */ >>> - memcpy(gpt_e[i].partition_type_guid.b, >>> - &PARTITION_BASIC_DATA_GUID, 16); >>> + gpt_e[i].partition_type_guid = basic_guid; >>> >>> #ifdef CONFIG_PARTITION_UUIDS >>> str_uuid = partitions[i].uuid; >>> > >> Sorry, I sent my tested-by on a different thread: >> http://patchwork.ozlabs.org/patch/319649/ > >> Tested on i.MX6 (armv7) custom board and it is working fine without >> the -mno-unaligned-access flag. > >> Tested-by: Hector Palacios <hector.palacios@digi.com> > >> Replying to Tom's question in that other thread, regarding the issue: > >>> Can you give me some steps on how to hit this bug? I believe it's a bug >>> and I believe we need to fix it, I just want to investigate a few things >>> while we've got a trigger case right now. Thanks! > >> GPT partition table needs to write a protective MBR to sector 0. >> The MBR structure has four partition entries (each occupying 16 bytes) >> at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure >> of each partition entry is defined at include/part_efi.h > >> struct partition { >> u8 boot_ind; /* 0x80 - active */ >> u8 head; /* starting head */ >> u8 sector; /* starting sector */ >> u8 cyl; /* starting cylinder */ >> u8 sys_ind; /* What partition type */ >> u8 end_head; /* end head */ >> u8 end_sector; /* end sector */ >> u8 end_cyl; /* end cylinder */ >> __le32 start_sect; /* starting sector counting from 0 */ >> __le32 nr_sects; /* nr of sectors in partition */ >> } __packed; > >> showing eight 1-byte fields and two 4-byte fields. Since the offsets for >> each partition entry are unaligned, the last two fields (which are >> 32bit) are unaligned as well. But it's not an error, it's just the >> specification of the MBR requires these fields to be at those exact >> offsets. So the usage of unaligned macros for accessing those fields is >> justified. > > Right. I would have sworn I used the GPT commands since we've dropped > -mno-unaligned-access but I'll just go re-test locally now then, thanks. Indeed I hadn't re-tested recently enough, thanks. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTBMxeAAoJENk4IS6UOR1WmKAP/05tEc0Ix4vrbI+WEYv4s26v Up3Uj1yMHBgHhCmdUops6C6eNjoSX2Z2TzfFx8jQ1YCTrGXO229CSi0U/oXkTWCB lbdMxQ6IyKh1v+Z4MdEJBq43XxcGMVE8g75Z8Eb/7B74kHRvsLeLGIDMg4A3z1yB b95HOCaifhSykBELsrqWXpMhJr4KMkn+GT9lNA5umLJdHT/1A2pEglbkZWAu4tAt ybDpdpXI3Ai8eVg9NuMJXEAcYEGezlg55esFv57gJfLfBPM/e0WLF4MtZYyFJmC/ 0SLe46OG19E6JzHMKrHngZbyVSC+Iwzh5Mw6vY40IyVxA6fpXZEZ119AyxLtP4m8 BiWjjRAO65KC6qzRJJYzXKoXSD8Ky+VJ3ATWzUhVSeLQRHeE2am8JsT8ENj5dngC f93pzqTmOp9LPqOB81dlIbu+R8QoruX0mOQxygNy+7tpTTijLn+UUu23z165j42b qsBzrAddgFZzUxqfyJLHIr/bvrVqBpEP6BGv2i+5eA6Q/dPHMvVLV3hTZjnxtK52 I6RVfK2R4uTX8mK+yN2HzBdqZhCJqDgxbWYUMuDIkVq7QeY5rtPBaFlOguLPqwx2 +i38rtGj5abZcqr9ASDcmrfIoe1mIAj2NPuJejNLzbwyipyqRVO0CT+GG/FwXLLG LFWWTaNc/X4FMjGctcyr =1et2 -----END PGP SIGNATURE-----
Hi Tom, > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/19/2014 10:03 AM, Tom Rini wrote: > > On 02/19/2014 09:56 AM, Hector Palacios wrote: > >> On 01/28/2014 01:46 PM, Piotr Wilczek wrote: > >>> This patch fixes part_efi code to avoid unaligned access exception > >>> on some ARM platforms. > >>> > >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >>> CC: Tom Rini <trini@ti.com> > >>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net> > >>> --- > >>> Chnages for V2: > >>> - used put_unaligned to copy value; > >>> - use __aligned to align local array; > >>> > >>> disk/part_efi.c | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/disk/part_efi.c b/disk/part_efi.c > >>> index 9c33ae7..eb2cd57 100644 > >>> --- a/disk/part_efi.c > >>> +++ b/disk/part_efi.c > >>> @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t > >>> *dev_desc) > >>> p_mbr->signature = MSDOS_MBR_SIGNATURE; > >>> p_mbr->partition_record[0].sys_ind = > >>> EFI_PMBR_OSTYPE_EFI_GPT; p_mbr->partition_record[0].start_sect = > >>> 1; > >>> - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; > >>> + put_unaligned(dev_desc->lba, > >>> &p_mbr->partition_record[0].nr_sects); > >>> > >>> /* Write MBR sector to the MMC device */ > >>> if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) > >>> { @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, > >>> gpt_entry *gpt_e, #ifdef CONFIG_PARTITION_UUIDS > >>> char *str_uuid; > >>> #endif > >>> + static efi_guid_t basic_guid __aligned(0x04) = > >>> + PARTITION_BASIC_DATA_GUID; > >>> > >>> for (i = 0; i < parts; i++) { > >>> /* partition starting lba */ > >>> @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry > >>> *gpt_e, gpt_e[i].ending_lba = cpu_to_le64(offset - 1); > >>> > >>> /* partition type GUID */ > >>> - memcpy(gpt_e[i].partition_type_guid.b, > >>> - &PARTITION_BASIC_DATA_GUID, 16); > >>> + gpt_e[i].partition_type_guid = basic_guid; > >>> > >>> #ifdef CONFIG_PARTITION_UUIDS > >>> str_uuid = partitions[i].uuid; > >>> > > > >> Sorry, I sent my tested-by on a different thread: > >> http://patchwork.ozlabs.org/patch/319649/ > > > >> Tested on i.MX6 (armv7) custom board and it is working fine without > >> the -mno-unaligned-access flag. > > > >> Tested-by: Hector Palacios <hector.palacios@digi.com> > > > >> Replying to Tom's question in that other thread, regarding the > >> issue: > > > >>> Can you give me some steps on how to hit this bug? I believe > >>> it's a bug and I believe we need to fix it, I just want to > >>> investigate a few things while we've got a trigger case right > >>> now. Thanks! > > > >> GPT partition table needs to write a protective MBR to sector 0. > >> The MBR structure has four partition entries (each occupying 16 > >> bytes) at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). > >> The structure of each partition entry is defined at > >> include/part_efi.h > > > >> struct partition { > >> u8 boot_ind; /* 0x80 - active */ > >> u8 head; /* starting head */ > >> u8 sector; /* starting sector */ > >> u8 cyl; /* starting cylinder */ > >> u8 sys_ind; /* What partition type */ > >> u8 end_head; /* end head */ > >> u8 end_sector; /* end sector */ > >> u8 end_cyl; /* end cylinder */ > >> __le32 start_sect; /* starting sector counting from 0 */ > >> __le32 nr_sects; /* nr of sectors in partition */ > >> } __packed; > > > >> showing eight 1-byte fields and two 4-byte fields. Since the > >> offsets for each partition entry are unaligned, the last two > >> fields (which are 32bit) are unaligned as well. But it's not an > >> error, it's just the specification of the MBR requires these > >> fields to be at those exact offsets. So the usage of unaligned > >> macros for accessing those fields is justified. > > > > Right. I would have sworn I used the GPT commands since we've > > dropped -mno-unaligned-access but I'll just go re-test locally now > > then, thanks. > > Indeed I hadn't re-tested recently enough, thanks. Have you managed to reproduce the problem on your setup? I've reproduced it on Trats/Trats2 with ELDK 5.4 (gcc 4.7.2 armv7a toolchain). This patch fixes the problem. > > - -- > Tom > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJTBMxeAAoJENk4IS6UOR1WmKAP/05tEc0Ix4vrbI+WEYv4s26v > Up3Uj1yMHBgHhCmdUops6C6eNjoSX2Z2TzfFx8jQ1YCTrGXO229CSi0U/oXkTWCB > lbdMxQ6IyKh1v+Z4MdEJBq43XxcGMVE8g75Z8Eb/7B74kHRvsLeLGIDMg4A3z1yB > b95HOCaifhSykBELsrqWXpMhJr4KMkn+GT9lNA5umLJdHT/1A2pEglbkZWAu4tAt > ybDpdpXI3Ai8eVg9NuMJXEAcYEGezlg55esFv57gJfLfBPM/e0WLF4MtZYyFJmC/ > 0SLe46OG19E6JzHMKrHngZbyVSC+Iwzh5Mw6vY40IyVxA6fpXZEZ119AyxLtP4m8 > BiWjjRAO65KC6qzRJJYzXKoXSD8Ky+VJ3ATWzUhVSeLQRHeE2am8JsT8ENj5dngC > f93pzqTmOp9LPqOB81dlIbu+R8QoruX0mOQxygNy+7tpTTijLn+UUu23z165j42b > qsBzrAddgFZzUxqfyJLHIr/bvrVqBpEP6BGv2i+5eA6Q/dPHMvVLV3hTZjnxtK52 > I6RVfK2R4uTX8mK+yN2HzBdqZhCJqDgxbWYUMuDIkVq7QeY5rtPBaFlOguLPqwx2 > +i38rtGj5abZcqr9ASDcmrfIoe1mIAj2NPuJejNLzbwyipyqRVO0CT+GG/FwXLLG > LFWWTaNc/X4FMjGctcyr > =1et2 > -----END PGP SIGNATURE-----
On Mon, Feb 24, 2014 at 04:56:57PM +0100, Lukasz Majewski wrote: > Hi Tom, > > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > On 02/19/2014 10:03 AM, Tom Rini wrote: > > > On 02/19/2014 09:56 AM, Hector Palacios wrote: > > >> On 01/28/2014 01:46 PM, Piotr Wilczek wrote: > > >>> This patch fixes part_efi code to avoid unaligned access exception > > >>> on some ARM platforms. > > >>> > > >>> Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com> > > >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > >>> CC: Tom Rini <trini@ti.com> > > >>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net> > > >>> --- > > >>> Chnages for V2: > > >>> - used put_unaligned to copy value; > > >>> - use __aligned to align local array; > > >>> > > >>> disk/part_efi.c | 7 ++++--- > > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > > >>> > > >>> diff --git a/disk/part_efi.c b/disk/part_efi.c > > >>> index 9c33ae7..eb2cd57 100644 > > >>> --- a/disk/part_efi.c > > >>> +++ b/disk/part_efi.c > > >>> @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t > > >>> *dev_desc) > > >>> p_mbr->signature = MSDOS_MBR_SIGNATURE; > > >>> p_mbr->partition_record[0].sys_ind = > > >>> EFI_PMBR_OSTYPE_EFI_GPT; p_mbr->partition_record[0].start_sect = > > >>> 1; > > >>> - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; > > >>> + put_unaligned(dev_desc->lba, > > >>> &p_mbr->partition_record[0].nr_sects); > > >>> > > >>> /* Write MBR sector to the MMC device */ > > >>> if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) > > >>> { @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, > > >>> gpt_entry *gpt_e, #ifdef CONFIG_PARTITION_UUIDS > > >>> char *str_uuid; > > >>> #endif > > >>> + static efi_guid_t basic_guid __aligned(0x04) = > > >>> + PARTITION_BASIC_DATA_GUID; > > >>> > > >>> for (i = 0; i < parts; i++) { > > >>> /* partition starting lba */ > > >>> @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry > > >>> *gpt_e, gpt_e[i].ending_lba = cpu_to_le64(offset - 1); > > >>> > > >>> /* partition type GUID */ > > >>> - memcpy(gpt_e[i].partition_type_guid.b, > > >>> - &PARTITION_BASIC_DATA_GUID, 16); > > >>> + gpt_e[i].partition_type_guid = basic_guid; > > >>> > > >>> #ifdef CONFIG_PARTITION_UUIDS > > >>> str_uuid = partitions[i].uuid; > > >>> > > > > > >> Sorry, I sent my tested-by on a different thread: > > >> http://patchwork.ozlabs.org/patch/319649/ > > > > > >> Tested on i.MX6 (armv7) custom board and it is working fine without > > >> the -mno-unaligned-access flag. > > > > > >> Tested-by: Hector Palacios <hector.palacios@digi.com> > > > > > >> Replying to Tom's question in that other thread, regarding the > > >> issue: > > > > > >>> Can you give me some steps on how to hit this bug? I believe > > >>> it's a bug and I believe we need to fix it, I just want to > > >>> investigate a few things while we've got a trigger case right > > >>> now. Thanks! > > > > > >> GPT partition table needs to write a protective MBR to sector 0. > > >> The MBR structure has four partition entries (each occupying 16 > > >> bytes) at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). > > >> The structure of each partition entry is defined at > > >> include/part_efi.h > > > > > >> struct partition { > > >> u8 boot_ind; /* 0x80 - active */ > > >> u8 head; /* starting head */ > > >> u8 sector; /* starting sector */ > > >> u8 cyl; /* starting cylinder */ > > >> u8 sys_ind; /* What partition type */ > > >> u8 end_head; /* end head */ > > >> u8 end_sector; /* end sector */ > > >> u8 end_cyl; /* end cylinder */ > > >> __le32 start_sect; /* starting sector counting from 0 */ > > >> __le32 nr_sects; /* nr of sectors in partition */ > > >> } __packed; > > > > > >> showing eight 1-byte fields and two 4-byte fields. Since the > > >> offsets for each partition entry are unaligned, the last two > > >> fields (which are 32bit) are unaligned as well. But it's not an > > >> error, it's just the specification of the MBR requires these > > >> fields to be at those exact offsets. So the usage of unaligned > > >> macros for accessing those fields is justified. > > > > > > Right. I would have sworn I used the GPT commands since we've > > > dropped -mno-unaligned-access but I'll just go re-test locally now > > > then, thanks. > > > > Indeed I hadn't re-tested recently enough, thanks. > > Have you managed to reproduce the problem on your setup? > > I've reproduced it on Trats/Trats2 with ELDK 5.4 (gcc 4.7.2 armv7a > toolchain). > > This patch fixes the problem. Yes, which is why I've renewed my effort to correct our behavior with respect to gcc generating unaligned access faults where we don't need them, and this shall be resolved for v2014.04-rc2.
diff --git a/disk/part_efi.c b/disk/part_efi.c index 9c33ae7..eb2cd57 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -225,7 +225,7 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc) p_mbr->signature = MSDOS_MBR_SIGNATURE; p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT; p_mbr->partition_record[0].start_sect = 1; - p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba; + put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects); /* Write MBR sector to the MMC device */ if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, #ifdef CONFIG_PARTITION_UUIDS char *str_uuid; #endif + static efi_guid_t basic_guid __aligned(0x04) = + PARTITION_BASIC_DATA_GUID; for (i = 0; i < parts; i++) { /* partition starting lba */ @@ -388,8 +390,7 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, gpt_e[i].ending_lba = cpu_to_le64(offset - 1); /* partition type GUID */ - memcpy(gpt_e[i].partition_type_guid.b, - &PARTITION_BASIC_DATA_GUID, 16); + gpt_e[i].partition_type_guid = basic_guid; #ifdef CONFIG_PARTITION_UUIDS str_uuid = partitions[i].uuid;