On Thu, 2011-02-10 at 02:37 +0000, Loïc Minier wrote: > > This aligns partitions to 4 MiB > > I've tested this successfully with: > * qemu 2G natty image, albeit no serial so couldn't check from Linux > * qemu 4G maverick image, this wouldn't boot at first, I had to replace u-boot.bin and MLO and then it booted -- not sure why, don't think this is a regression Can you test this using an image generated by trunk? I thought that would work, but maybe I'm missing something? > * 1G overo image file dd-ed to a MMC, this booted and was aligned, but CHS was incorrect from Linux (didn't appear at 63/255 anymore) Is there any chance this wrong geometry would cause problems to the old OMAP3 roms that rely on specific values for CHS? Any way we could test it? > * 1G to MMC for overo; this worked much like the image file case; fdisk output below > * 1G to MMC for beagle on beaglexm; fdisk output below > > fdisk output on overo: > root@localhost:~# fdisk -l > > Disk /dev/mmcblk0: 1967 MB, 1967128576 bytes > 4 heads, 16 sectors/track, 60032 cylinders > Units = cylinders of 64 * 512 = 32768 bytes > Sector size (logical/physical): 512 bytes / 512 bytes > I/O size (minimum/optimal): 512 bytes / 512 bytes > Disk identifier: 0x0008ed19 > > Device Boot Start End Blocks Id System > /dev/mmcblk0p1 * 257 2009 56068 c W95 FAT32 (LBA) > Partition 1 does not end on cylinder boundary. > /dev/mmcblk0p2 2049 60032 1855488 83 Linux > Partition 2 does not end on cylinder boundary. > > fdisk output on beaglexm: > > Disk /dev/mmcblk0: 1967 MB, 1967128576 bytes > 4 heads, 16 sectors/track, 60032 cylinders > Units = cylinders of 64 * 512 = 32768 bytes > Sector size (logical/physical): 512 bytes / 512 bytes > I/O size (minimum/optimal): 512 bytes / 512 bytes > Disk identifier: 0x000bf37e > > Device Boot Start End Blocks Id System > /dev/mmcblk0p1 * 257 2009 56068 c W95 FAT32 (LBA) > Partition 1 does not end on cylinder boundary. > /dev/mmcblk0p2 2049 60032 1855488 83 Linux > Partition 2 does not end on cylinder boundary. > > differences between files attachment (review-diff.txt) > === modified file 'linaro_media_create/boards.py' > --- linaro_media_create/boards.py 2011-02-01 13:16:43 +0000 > +++ linaro_media_create/boards.py 2011-02-10 02:37:40 +0000 > @@ -32,6 +32,43 @@ > > from linaro_media_create import cmd_runner > > +# Notes: > +# * geometry is currently always 255 heads and 63 sectors due to limitations of > +# older OMAP3 boot ROMs > +# * we want partitions aligned on 4 MiB as to get the best performance and > +# limit wear-leveling > +# * partitions should preferably end on cylinder boundaries, at least to please > +# sfdisk but also just to have them as big as possible > +# * this assumes that root partition follows the boot partition which follows > +# an optional bootloader partition > +# * image_size is passed on the command-line and should preferably be a power > +# of 2; it should be used as a "don't go over this size" information for a > +# real device, and a "give me a file exactly this big" requirement for an > +# image file. Having exactly a power of 2 helps with QEMU; there seem to be > +# some truncating issues otherwise. XXX to be researched > + > +# number of sectors of 512 bytes that we align the start of partitions on; we > +# align on 4 MiB > +PART_ALIGN_S = 4 * 1024 * 1024 / 512 > +# start sector of optional bootloader partition, +2s; this is just after MBR > +# and partition table; this partition needs not be aligned > +LOADER_PART_START_S = 2 > +# start sector of boot partition, +8 MiB; +4 MiB would still be in the first > +# cylinder > +BOOT_PART_START_S = 8 * 1024 * 1024 / 512 > +assert BOOT_PART_START_S / PART_ALIGN_S * PART_ALIGN_S == BOOT_PART_START_S This assert really needs a message for when it fails. BTW, isn't this the same as asserting that "BOOT_PART_START_S % PART_ALIGN_S == 0"? > +# start sector of root partition, +64 MiB; means boot partition is roughly > +# 56 MiB; XXX there's currently no way to set the relative sizes of boot and > +# root partitions Neither relative nor absolute sizes can be specified for each of the partitions, right? Do we really care about that? > +ROOT_PART_START_S = 64 * 1024 * 1024 / 512 > +assert ROOT_PART_START_S / PART_ALIGN_S * PART_ALIGN_S == ROOT_PART_START_S Same comment about the other assert above. > +BOOT_PART_START_CYL = BOOT_PART_START_S / (63 * 255) > +LOADER_PART_SIZE_S = BOOT_PART_START_CYL * (63 * 255) - LOADER_PART_START_S > +assert LOADER_PART_START_S + LOADER_PART_SIZE_S < BOOT_PART_START_S > +ROOT_PART_START_CYL = ROOT_PART_START_S / (63 * 255) > +BOOT_PART_SIZE_S = ROOT_PART_START_CYL * (63 * 255) - BOOT_PART_START_S > +assert BOOT_PART_START_S + BOOT_PART_SIZE_S < ROOT_PART_START_S The two asserts above could do with failure messages as well. > + > > class BoardConfig(object): > """The configuration used when building an image for a board.""" > @@ -60,10 +97,16 @@ > else: > partition_type = '0x0E' > > - # This will create a partition of the given type, containing 9 > - # cylinders (74027520 bytes, ~70 MiB), followed by a Linux-type > - # partition containing the rest of the free space. > - return ',9,%s,*\n,,,-' % partition_type > + # This will create a boot partition of type partition_type at offset > + # (in sectors) BOOT_PART_START_S and of size BOOT_PART_SIZE_S followed > + # by a root partition of the default type (Linux) at offset (in > + # sectors) ROOT_PART_START_S filling the remaining space > + # XXX we should honor the specified image size by specifying a > + # corresponding root partition size You mean we're no longer using the size as the upper limit for the image size or that the image size is not exactly what was requested? If the former, I think we need a bug to track it, but if it's the latter, do we really care about it? > + return '%s,%s,%s,*\n%s,,,-' % ( > + BOOT_PART_START_S, BOOT_PART_SIZE_S, partition_type, > + ROOT_PART_START_S, > + ) > > @classmethod > def _get_boot_cmd(cls, is_live, is_lowmem, consoles, rootfs_uuid): > @@ -300,7 +343,9 @@ > # the beginning of the image (size is one cylinder, so 8224768 bytes > # with the first sector for MBR). > sfdisk_cmd = super(Mx51evkConfig, cls).get_sfdisk_cmd() > - return ',1,0xDA\n%s' % sfdisk_cmd > + return '%s,%s,0xDA\n%s' % ( > + LOADER_PART_START_S, LOADER_PART_SIZE_S, sfdisk_cmd, > + ) > > @classmethod > def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir, > === modified file 'linaro_media_create/tests/test_media_create.py' > --- linaro_media_create/tests/test_media_create.py 2011-02-10 02:37:40 +0000 > +++ linaro_media_create/tests/test_media_create.py 2011-02-10 02:37:40 +0000 > @@ -332,11 +332,11 @@ > > def test_default(self): > self.assertEquals( > - ',9,0x0C,*\n,,,-', boards.BoardConfig.get_sfdisk_cmd()) > + '16384,112136,0x0C,*\n131072,,,-', boards.BoardConfig.get_sfdisk_cmd()) > > def test_mx51evk(self): > self.assertEquals( > - ',1,0xDA\n,9,0x0C,*\n,,,-', > + '2,16063,0xDA\n16384,112136,0x0C,*\n131072,,,-', > board_configs['mx51evk'].get_sfdisk_cmd()) > > > @@ -632,7 +632,7 @@ > # every time we run sfdisk it actually repartitions the device, > # erasing any partitions created previously. > self.assertEqual( > - [(',1,0xDA\n,9,0x0C,*\n,,,-', 255, 63, '', self.media.path)], > + [('2,16063,0xDA\n16384,112136,0x0C,*\n131072,,,-', 255, 63, '', self.media.path)], > sfdisk_fixture.mock.calls) > > def test_create_partitions_for_beagle(self): > @@ -647,7 +647,7 @@ > ['sync']], > popen_fixture.mock.calls) > self.assertEqual( > - [(',9,0x0C,*\n,,,-', 255, 63, '', self.media.path)], > + [('16384,112136,0x0C,*\n131072,,,-', 255, 63, '', self.media.path)], > sfdisk_fixture.mock.calls) > > def test_create_partitions_with_img_file(self): > @@ -664,7 +664,7 @@ > self.assertEqual([['sync']], popen_fixture.mock.calls) > > self.assertEqual( > - [(',9,0x0C,*\n,,,-', 255, 63, '', tempfile)], > + [('16384,112136,0x0C,*\n131072,,,-', 255, 63, '', tempfile)], > sfdisk_fixture.mock.calls) > > def test_run_sfdisk_commands(self): > @@ -674,7 +674,7 @@ > stdout=subprocess.PIPE) > proc.communicate() > stdout, stderr = run_sfdisk_commands( > - ',1,0xDA', 5, 63, '', tempfile, as_root=False, > + '2,16063,0xDA', 255, 63, '', tempfile, as_root=False, > stderr=subprocess.PIPE) > self.assertIn('Successfully wrote the new partition table', stdout) > > @@ -683,7 +683,7 @@ > self.assertRaises( > cmd_runner.SubcommandNonZeroReturnValue, > run_sfdisk_commands, > - ',1,0xDA', 5, 63, '', tempfile, as_root=False, > + ',1,0xDA', 255, 63, '', tempfile, as_root=False, Why did you change the two tests above? > stderr=subprocess.PIPE) > > > @@ -708,17 +708,13 @@ > def test_convert_size_in_gbytes_to_bytes(self): > self.assertEqual(12 * 1024**3, convert_size_to_bytes('12G')) > > - def test_convert_size_in_kbytes_to_bytes_rounds_to_256k_multiple(self): > - # See comment in convert_size_to_bytes as to why we need to do this. > - self.assertEqual( > - 3891 * (1024 * 256), convert_size_to_bytes('1000537K')) > - > def test_calculate_partition_size_and_offset(self): > - tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-') > + # boot part at +8 MiB, root part at +16 MiB > + tempfile = self._create_qemu_img_with_partitions('16384,15746,0x0C,*\n32768,,,-') > vfat_size, vfat_offset, linux_size, linux_offset = ( > calculate_partition_size_and_offset(tempfile)) > self.assertEqual( > - [129024L, 32256L, 10321920L, 161280L], > + [8061952L, 8388608L, 4194304L, 16777216L], > [vfat_size, vfat_offset, linux_size, linux_offset]) > > def test_get_boot_and_root_partitions_for_media_beagle(self): > @@ -748,11 +744,11 @@ > def _create_qemu_img_with_partitions(self, sfdisk_commands): > tempfile = self.createTempFileAsFixture() > proc = cmd_runner.run( > - ['qemu-img', 'create', '-f', 'raw', tempfile, '10M'], > + ['qemu-img', 'create', '-f', 'raw', tempfile, '20M'], > stdout=subprocess.PIPE) > proc.communicate() > stdout, stderr = run_sfdisk_commands( > - sfdisk_commands, 5, 63, '', tempfile, as_root=False, > + sfdisk_commands, 255, 63, '', tempfile, as_root=False, I'd be interested in knowing what motivated this change as well. > # Throw away stderr as sfdisk complains a lot when operating on a > # qemu image. > stderr=subprocess.PIPE) > @@ -775,7 +771,8 @@ > self.assertEqual(None, popen_fixture.mock.calls) > > def test_get_boot_and_root_loopback_devices(self): > - tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-') > + # boot part at +8 MiB, root part at +16 MiB > + tempfile = self._create_qemu_img_with_partitions('16384,15746,0x0C,*\n32768,,,-') > atexit_fixture = self.useFixture(MockSomethingFixture( > atexit, 'register', AtExitRegister())) > popen_fixture = self.useFixture(MockCmdRunnerPopenFixture()) > @@ -785,9 +782,9 @@ > get_boot_and_root_loopback_devices(tempfile) > self.assertEqual( > [['sudo', 'losetup', '-f', '--show', tempfile, '--offset', > - '32256', '--sizelimit', '129024'], > + '8388608', '--sizelimit', '8061952'], > ['sudo', 'losetup', '-f', '--show', tempfile, '--offset', > - '161280', '--sizelimit', '10321920']], > + '16777216', '--sizelimit', '4194304']], > popen_fixture.mock.calls) > > # get_boot_and_root_loopback_devices will also setup two exit handlers > @@ -807,7 +804,8 @@ > # but here we mock Popen() and thanks to that the image is not setup > # (via qemu-img) inside setup_partitions. That's why we pass an > # already setup image file. > - tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-') > + # boot part at +8 MiB, root part at +16 MiB > + tempfile = self._create_qemu_img_with_partitions('16384,15746,0x0C,*\n32768,,,-') > popen_fixture = self.useFixture(MockCmdRunnerPopenFixture()) > self.useFixture(MockSomethingFixture( > sys, 'stdout', open('/dev/null', 'w'))) > @@ -827,11 +825,11 @@ > board_configs['beagle'], Media(tempfile), '2G', 'boot', > 'root', 'ext3', True, True, True) > self.assertEqual( > - # This is the call that would create the image file. > + # This is the call that would create a 2 GiB image file. > [['qemu-img', 'create', '-f', 'raw', tempfile, '2147483648'], > # This call would partition the image file. > - ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', '-C', '261', > - tempfile], > + ['sudo', 'sfdisk', '--force', '-D', '-uS', '-H', '255', '-S', > + '63', '-C', '261', tempfile], > # Make sure changes are written to disk. > ['sync'], > ['sudo', 'mkfs.vfat', '-F', '32', bootfs_dev, '-n', 'boot'], > @@ -844,7 +842,8 @@ > # Pretend the partitions are mounted. > self.useFixture(MockSomethingFixture( > partitions, 'is_partition_mounted', lambda part: True)) > - tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-') > + # boot part at +8 MiB, root part at +16 MiB > + tempfile = self._create_qemu_img_with_partitions('16384,15746,0x0C,*\n32768,,,-') I've seen the above string hard-coded in way too many tests now so I think it'd make sense to make a constant out of it and use it here. That way you can even get rid of the comment above it. > self.useFixture(MockSomethingFixture( > partitions, '_get_device_file_for_partition_number', > lambda dev, partition: '%s%d' % (tempfile, partition))) > @@ -857,7 +856,8 @@ > True, True, True) > self.assertEqual( > [['sudo', 'parted', '-s', tempfile, 'mklabel', 'msdos'], > - ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', tempfile], > + ['sudo', 'sfdisk', '--force', '-D', '-uS', '-H', '255', '-S', > + '63', tempfile], > ['sync'], > # Since the partitions are mounted, setup_partitions will umount > # them before running mkfs. > -- Guilherme Salgado