Re: [PATCH] docs: Update for stopping small mbr gap support
Two small suggestions inlined in the quoted part.
Regards,
ecm
On at 2020-03-05 15:38 +0100, Daniel Kiper wrote:
> On Thu, Mar 05, 2020 at 06:40:01PM +0800, Michael Chang wrote:
>> Further to the discussion about disabling btrfs zstd support for
>> i386-pc[1], this paragraph in manual about mbr gap size doesn't seem to
>> hold true any longer.
>>
>> "You must ensure that the first partition starts at least 31 KiB (63
>> sectors) from the start of the disk"
>>
>> As in many occasions we inevitablely have to provide core image size
>> that goes beyond 31 KiB, this statement becomes a true liability as
>> people would be misguided and think it is still fine to use small MBR
>> gap, that has always been a headache in distribution's upgrade path as
>> growing new feature would render the size requirement bigger but no way
>> for the user to relocate their partitions.
>
> Could you split this paragraph into a few sentences? Now it does not
> read very well...
>
>> The patch tries to correct the paragraph with a more practical size that
>> works for grub and also for modern computer systems in general.
>>
>> [1] https://lists.gnu.org/archive/html/grub-devel/2019-11/msg00025.html
>>
>> Signed-off-by: Michael Chang
>> ---
>> docs/grub.texi | 20 ++--
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/grub.texi b/docs/grub.texi
>> index 83979af38..651468268 100644
>> --- a/docs/grub.texi
>> +++ b/docs/grub.texi
>> @@ -845,12 +845,20 @@ only be used if the @file{/boot} filesystem is on the
>> same disk that the
>> BIOS boots from, so that GRUB does not have to rely on guessing BIOS drive
>> numbers.
>>
>> -The GRUB development team generally recommends embedding GRUB before the
>> -first partition, unless you have special requirements. You must ensure that
>> -the first partition starts at least 31 KiB (63 sectors) from the start of
>> -the disk; on modern disks, it is often a performance advantage to align
>> -partitions on larger boundaries anyway, so the first partition might start 1
>> -MiB from the start of the disk.
>> +The GRUB development team generally recommends embedding GRUB before the
>> first
>> +partition, unless you have special requirements. You must ensure that the
>> first
>> +partition starts at least 1 MiB from the start of the disk; on modern
>> disks, it
>
> s/; on modern disks, it/. Additionally, on modern disks it/
>
>> +is often a performance advantage to align partitions on larger boundaries
>> and 1
>> +MiB is the least common multiple of many used alignment sizes. For SSD, it
>
> s/For SSD, it/E.g. SSD, it/
>
>> +became crucial to have the partition correctly aligned to avoid excessive
>> +read-modify-write cycles and thus modern tools set to use 1 MiB as a
>> stardard
>> +practice.
s/stardard/standard
>> +
>> +In case legacy systems that cannot boot if first partition not on the
>> cylinder
>
> s/In case legacy/In case of legacy/
> s/partition not/partition is not/
>
>> +boundary, the fallback blocklist install method should remain working for
>> them
>> +if the core image growing too much someday. Here we just can't advertise
>> that
s/growing/grows/
>> +31 KiB (63 sectors) is a sensible size any longer as that would pose great
>> +constraint to include new features as time goes by.
>
> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Fix FreeDOS command booting large files (near or above 64 KiB)
While testing the 86-DOS lDebug [1] booting from GRUB2, newer versions of the debugger would fail to load when booted using GRUB's freedos command. The behaviour observed in a qemu i386 machine was that the ROM-BIOS's boot load would start anew, instead of loading the selected debugger as kernel. It came to light that there was a size limit: Kernel files that were 58880 bytes (E600h) long or shorter succeeded to boot, while files that were 64000 bytes or longer failed in the manner described. Eventually it turned out that the relocator16 stub succeeded whenever it was placed completely within the first 64 KiB of the Low Memory Area. The chunk for the relocator is allocated with a minimum address of 0x8010 and a maximum address just below 0xA [2]. That means if the kernel is, for instance, E600h bytes long, then the kernel will be allocated memory starting at 00600h (the fixed FreeDOS kernel load address) up to E600h + 00600h = 0EC00h, which leaves 1400h (5120) bytes for the relocator to stay in the first 64 KiB. If the kernel is 64000 bytes (FA00h) long, then the relocator must go to FA00h + 00600h = 1h at least which is outside the first 64 KiB. The problem is that the relocator16 initialises the DS register with a "pseudo real mode" descriptor, which is defined with a segment limit of 64 KiB and a segment base of zero. After that, the relocator addressed parts of itself (implicitly) using the DS register, with an offset from ESI, which holds the linear address of the relocator's base [3]. With the larger kernel files this would lead to accessing data beyond the 64 KiB segment limit, presumably leading to a fault and perhaps a subsequent triple-fault or such. This patch fixes the relocator to set the segment base of the descriptors to the base address of the relocator; then, the subsequent accesses to the relocator's variables are done without the ESI register as an index. This does not interfere with the relocator's or its target's normal operation; the segment limits are still loaded with 64 KiB and all the segment bases are subsequently reset by the relocator anyway. Current versions of the debugger to test are uploaded to [4]. The file ldebugnh.com (LZ4-compressed and built with -D_EXTHELP=0) at 58368 bytes loads successfully, whereas ldebug.com at 64000 bytes fails. Loading one of these files requires setting root to a FAT FS partition and using the freedos command to specify the file as kernel: set root='(hd0,msdos1)' freedos /ldebug.com boot Booting the file using the multiboot command (which uses a WIP entrypoint of the debugger) works, as it does not use GRUB's relocator16 but instead includes a loader in the kernel itself, which drops it back to 86 Mode. [1]: https://hg.ulukai.org/ecm/ldebug [2]: http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/lib/i386/relocator.c?id=495781f5ed1b48bf27f16c53940d6700c181c74c#n127 [3]: http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/lib/i386/relocator16.S?id=495781f5ed1b48bf27f16c53940d6700c181c74c#n97 [4]: https://ulukai.org/ecm/lDebug-5479a7988d21-nohelp.zip Signed-off-by: C. Masloch --- grub-core/lib/i386/relocator16.S | 26 +++--- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/grub-core/lib/i386/relocator16.S b/grub-core/lib/i386/relocator16.S index 371a2ed69..e9238119b 100644 --- a/grub-core/lib/i386/relocator16.S +++ b/grub-core/lib/i386/relocator16.S @@ -38,15 +38,21 @@ VARIABLE(grub_relocator16_start) #ifdef __APPLE__ LOCAL(cs_base_bytes12_offset) = LOCAL (cs_base_bytes12) - LOCAL (base) LOCAL(cs_base_byte3_offset) = LOCAL (cs_base_byte3) - LOCAL (base) + LOCAL(ds_base_bytes12_offset) = LOCAL (ds_base_bytes12) - LOCAL (base) + LOCAL(ds_base_byte3_offset) = LOCAL (ds_base_byte3) - LOCAL (base) movl%esi, %eax movw%ax, (LOCAL(cs_base_bytes12_offset)) (RSI, 1) + movw%ax, (LOCAL(ds_base_bytes12_offset)) (RSI, 1) shrl$16, %eax movb%al, (LOCAL (cs_base_byte3_offset)) (RSI, 1) + movb%al, (LOCAL (ds_base_byte3_offset)) (RSI, 1) #else movl%esi, %eax movw%ax, (LOCAL (cs_base_bytes12) - LOCAL (base)) (RSI, 1) + movw%ax, (LOCAL (ds_base_bytes12) - LOCAL (base)) (RSI, 1) shrl$16, %eax movb%al, (LOCAL (cs_base_byte3) - LOCAL (base)) (RSI, 1) + movb%al, (LOCAL (ds_base_byte3) - LOCAL (base)) (RSI, 1) #endif RELOAD_GDT @@ -88,15 +94,15 @@ VARIABLE(grub_relocator16_start) LOCAL(segment_offset) = LOCAL (segment) - LOCAL (base) LOCAL(idt_offset) = LOCAL(relocator16_idt) - LOCAL (base) LOCAL(cont2_offset) = LOCAL (cont2) - LOCAL(base) - movw%ax, LOCAL(segment_offset) (%esi, 1) - lidt LOCAL(idt_offset) (%esi, 1) + movw%ax, (LOCAL(segment_offset)) + lidt (LOCAL(idt_offset)) /* jump to a 16 bit segment */ ljmp$PSEUDO_REAL_CSEG, $(LOCAL(co
Re: [PATCH] chainloader: patch in BPB's sectors_per_track and num_heads
Hello,
Was this overlooked?
I think I should check whether dev->disk->data actually is a (struct
grub_biosdisk_data *) but I don't know how!
Regards,
ecm
On at 2018-02-01 15:51 +01:00, C. Masloch wrote:
>
> These fields must reflect the ROM-BIOS's geometry for CHS-based
> loaders to correctly load their next stage. Most loaders do not
> query the ROM-BIOS (Int13.08), relying on the BPB fields to hold
> the correct values already.
>
> Tested with lDebug booted in qemu via grub2's
> FreeDOS direct loading support, refer to
> https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug
> (For this test, lDebug's iniload.asm must be assembled with
> -D_QUERY_GEOMETRY=0 to leave the BPB values provided by grub.)
>
> Signed-off-by: C. Masloch <pus...@38.de>
> ---
> grub-core/loader/i386/pc/chainloader.c | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/loader/i386/pc/chainloader.c
> b/grub-core/loader/i386/pc/chainloader.c
> index 18220b7aa..ef3a322b7 100644
> --- a/grub-core/loader/i386/pc/chainloader.c
> +++ b/grub-core/loader/i386/pc/chainloader.c
> @@ -19,6 +19,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -86,9 +87,16 @@ grub_chainloader_unload (void)
> void
> grub_chainloader_patch_bpb (void *bs, grub_device_t dev, grub_uint8_t dl)
> {
> - grub_uint32_t part_start = 0;
> + grub_uint32_t part_start = 0, heads = 0, sectors = 0;
>if (dev && dev->disk)
> -part_start = grub_partition_get_start (dev->disk->partition);
> +{
> + part_start = grub_partition_get_start (dev->disk->partition);
> + if (dev->disk->data)
> +{
> + heads = ((struct grub_biosdisk_data *)(dev->disk->data))->heads;
> + sectors = ((struct grub_biosdisk_data
> *)(dev->disk->data))->sectors;
> +}
> +}
>if (grub_memcmp ((char *) &((struct grub_ntfs_bpb *) bs)->oem_name,
> "NTFS", 4) == 0)
> {
> @@ -127,12 +135,20 @@ grub_chainloader_patch_bpb (void *bs,
> grub_device_t dev, grub_uint8_t dl)
> {
> bpb->num_hidden_sectors = grub_cpu_to_le32 (part_start);
> bpb->version_specific.fat12_or_fat16.num_ph_drive = dl;
> + if (sectors)
> +bpb->sectors_per_track = grub_cpu_to_le16 (sectors);
> + if (heads)
> +bpb->num_heads = grub_cpu_to_le16 (heads);
> return;
> }
>if (bpb->version_specific.fat32.sectors_per_fat_32)
> {
> bpb->num_hidden_sectors = grub_cpu_to_le32 (part_start);
> bpb->version_specific.fat32.num_ph_drive = dl;
> + if (sectors)
> +bpb->sectors_per_track = grub_cpu_to_le16 (sectors);
> + if (heads)
> +bpb->num_heads = grub_cpu_to_le16 (heads);
> return;
> }
>break;
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
On at 2018-02-23 23:05 +01:00, Daniel Kiper wrote: > Applied! > > FYI, this is last time when I sent commit confirmation. > If I send one in the future then this will be rather > exception than rule. So, if you receive my reviewed-by > and patch is not committed in a week or two then it means > that it fallen into abyss and you have to ping me. > > Have a nice weekend, > > Daniel Thanks, I'll keep that in mind. Regards, ecm ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2] chainloader: Fix wrong break condition (must be AND not, OR)
The definition of bpb's num_total_sectors_16 and num_total_sectors_32 is that either the 16-bit field is non-zero and is used (in which case eg mkfs.fat sets the 32-bit field to zero), or it is zero and the 32-bit field is used. Therefore, a BPB is invalid only if *both* fields are zero; having one field as zero and the other as non-zero is the case to be expected. (Indeed, according to Microsoft's specification one of the fields *must* be zero, and the other non-zero.) This affects all users of grub_chainloader_patch_bpb which are in chainloader.c, freedos.c, and ntldr.c Some descriptions of the semantics of these two fields: https://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html > The old 2-byte fields "total number of sectors" and "number of > sectors per FAT" are now zero; this information is now found in > the new 4-byte fields. (Here given in the FAT32 EBPB section but the total sectors 16/32 bit fields semantic is true of FAT12 and FAT16 too.) https://wiki.osdev.org/FAT#BPB_.28BIOS_Parameter_Block.29 > 19 | 2 | The total sectors in the logical volume. If this value is 0, > it means there are more than 65535 sectors in the volume, and the actual > count is stored in "Large Sectors (bytes 32-35). > 32 | 4 | Large amount of sector on media. This field is set if there > are more than 65535 sectors in the volume. (Doesn't specify what the "large" field is set to when unused, but as mentioned mkfs.fat sets it to zero then.) https://technet.microsoft.com/en-us/library/cc976796.aspx > 0x13 | WORD | 0x | > Small Sectors . The number of sectors on the volume represented in 16 > bits (< 65,536). For volumes larger than 65,536 sectors, this field > has a value of zero and the Large Sectors field is used instead. > 0x20 | DWORD | 0x01F03E00 | > Large Sectors . If the value of the Small Sectors field is zero, this > field contains the total number of sectors in the FAT16 volume. If the > value of the Small Sectors field is not zero, the value of this field > is zero. https://staff.washington.edu/dittrich/misc/fatgen103.pdf page 10 > BPB_TotSec16 | 19 | 2 | > This field is the old 16-bit total count of sectors on the volume. > This count includes the count of all sectors in all four regions of the > volume. This field can be 0; if it is 0, then BPB_TotSec32 must be > non-zero. For FAT32 volumes, this field must be 0. For FAT12 and > FAT16 volumes, this field contains the sector count, and > BPB_TotSec32 is 0 if the total sector count “fits” (is less than > 0x1). > BPB_TotSec32 | 32 | 4 | > This field is the new 32-bit total count of sectors on the volume. > This count includes the count of all sectors in all four regions of the > volume. This field can be 0; if it is 0, then BPB_TotSec16 must be > non-zero. For FAT32 volumes, this field must be non-zero. For > FAT12/FAT16 volumes, this field contains the sector count if > BPB_TotSec16 is 0 (count is greater than or equal to 0x1). (This specifies that an unused BPB_TotSec32 field is set to zero.) Tested with lDebug booted in qemu via grub2's FreeDOS direct loading support, refer to https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug Signed-off-by: C. Masloch <pus...@38.de> --- grub-core/loader/i386/pc/chainloader.c | 2 +- include/grub/fat.h | 17 ++--- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/grub-core/loader/i386/pc/chainloader.c b/grub-core/loader/i386/pc/chainloader.c index c79c4fe0f..18220b7aa 100644 --- a/grub-core/loader/i386/pc/chainloader.c +++ b/grub-core/loader/i386/pc/chainloader.c @@ -117,7 +117,7 @@ grub_chainloader_patch_bpb (void *bs, grub_device_t dev, grub_uint8_t dl) if (bpb->num_reserved_sectors == 0) break; - if (bpb->num_total_sectors_16 == 0 || bpb->num_total_sectors_32 == 0) + if (bpb->num_total_sectors_16 == 0 && bpb->num_total_sectors_32 == 0) break; if (bpb->num_fats == 0) diff --git a/include/grub/fat.h b/include/grub/fat.h index 4a5aab793..8d7e4a1e5 100644 --- a/include/grub/fat.h +++ b/include/grub/fat.h @@ -28,20 +28,15 @@ struct grub_fat_bpb grub_uint16_t bytes_per_sector; grub_uint8_t sectors_per_cluster; grub_uint16_t num_reserved_sectors; - grub_uint8_t num_fats; - /* 0x10 */ + grub_uint8_t num_fats; /* 0x10 */ grub_uint16_t num_root_entries; grub_uint16_t num_total_sectors_16; - grub_uint8_t media; - /*0 x15 */ + grub_uint8_t media; /* 0x15 */ grub_uint16_t sectors_per_fat_16; - grub_uint16_t sectors_per_track; - /*0 x19 */ - grub_uint16_t num_heads; - /*0 x1b */ - grub_uint32_t num_hidden_sectors; - /* 0x1f */ - grub_uint32_t num_total_sectors_32; + grub_uint16_t sectors_per_track; /* 0x18 */ + grub_uint16_t num_he
Re: [PATCH] chainloader: Fix wrong break condition (must be AND not OR)
On at 2018-02-06 16:41 +01:00, Daniel Kiper wrote: >>> I am happy that you fix that issue but >>> https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system#BPB331_OFS_15h >>> shows that life is more complicated. >>> >>> Could you take that into account? >> >> MS-DOS 3.20 and 3.30 BPBs aren't supported anyway. The special case for > > Aren't supported by whom? GRUB2? Yes. If you look at include/grub/fat.h, the format is that which is specified as "DOS 3.31" BPB by the Wikipedia article (plus FAT32 EBPB). (Note that some of the offsets in the comments of struct grub_fat_bpb are wrong. I'll change these in the patch v2.) >> using a partition table entry's partition length field isn't applicable >> here. (If it were, the function simply shouldn't check these fields.) > > Please say why exactly it is not applicable here. Because we are using these fields. If it was to be made applicable, it should just not check the fields. >> And this is the first I've read of that DR-DOS extension. (And again, >> the simple solution to that one is also not to check the fields for zeros.) >> >>>> Tested with lDebug booted in qemu via grub2's >>>> FreeDOS direct loading support, refer to >>>> https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug >>> >>> Could you put your SOB here? >> >> Like this? >> >> Signed-off-by: C. Masloch <pus...@38.de> > > Yep. > >> (Should I submit a PATCH v2 for this?) > > Yes, please. I am happy to commit this patch if you provide > such a nice and detailed explanation like above but with better > formating. And please add some stuff which I asked for too. Am about to mail the patch v2. (I found out how to make Thunderbird not wrap long lines! mail.wrap_long_lines = false and mailnews.wraplength = 0.) Regards, ecm ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] chainloader: patch in BPB's sectors_per_track and num_heads
These fields must reflect the ROM-BIOS's geometry for CHS-based
loaders to correctly load their next stage. Most loaders do not
query the ROM-BIOS (Int13.08), relying on the BPB fields to hold
the correct values already.
Tested with lDebug booted in qemu via grub2's
FreeDOS direct loading support, refer to
https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug
(For this test, lDebug's iniload.asm must be assembled with
-D_QUERY_GEOMETRY=0 to leave the BPB values provided by grub.)
Signed-off-by: C. Masloch <pus...@38.de>
---
grub-core/loader/i386/pc/chainloader.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/grub-core/loader/i386/pc/chainloader.c
b/grub-core/loader/i386/pc/chainloader.c
index 18220b7aa..ef3a322b7 100644
--- a/grub-core/loader/i386/pc/chainloader.c
+++ b/grub-core/loader/i386/pc/chainloader.c
@@ -19,6 +19,7 @@
#include
#include
+#include
#include
#include
#include
@@ -86,9 +87,16 @@ grub_chainloader_unload (void)
void
grub_chainloader_patch_bpb (void *bs, grub_device_t dev, grub_uint8_t dl)
{
- grub_uint32_t part_start = 0;
+ grub_uint32_t part_start = 0, heads = 0, sectors = 0;
if (dev && dev->disk)
-part_start = grub_partition_get_start (dev->disk->partition);
+{
+ part_start = grub_partition_get_start (dev->disk->partition);
+ if (dev->disk->data)
+{
+ heads = ((struct grub_biosdisk_data *)(dev->disk->data))->heads;
+ sectors = ((struct grub_biosdisk_data
*)(dev->disk->data))->sectors;
+}
+}
if (grub_memcmp ((char *) &((struct grub_ntfs_bpb *) bs)->oem_name,
"NTFS", 4) == 0)
{
@@ -127,12 +135,20 @@ grub_chainloader_patch_bpb (void *bs,
grub_device_t dev, grub_uint8_t dl)
{
bpb->num_hidden_sectors = grub_cpu_to_le32 (part_start);
bpb->version_specific.fat12_or_fat16.num_ph_drive = dl;
+ if (sectors)
+bpb->sectors_per_track = grub_cpu_to_le16 (sectors);
+ if (heads)
+bpb->num_heads = grub_cpu_to_le16 (heads);
return;
}
if (bpb->version_specific.fat32.sectors_per_fat_32)
{
bpb->num_hidden_sectors = grub_cpu_to_le32 (part_start);
bpb->version_specific.fat32.num_ph_drive = dl;
+ if (sectors)
+bpb->sectors_per_track = grub_cpu_to_le16 (sectors);
+ if (heads)
+bpb->num_heads = grub_cpu_to_le16 (heads);
return;
}
break;
--
2.11.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] chainloader: Fix wrong break condition (must be AND not OR)
On at 2018-01-29 18:09 +01:00, Daniel Kiper wrote: > On Sun, Jan 21, 2018 at 04:02:10PM +0100, C. Masloch wrote: >> The definition of bpb's num_total_sectors_16 and num_total_sectors_32 >> is that either the 16-bit field is non-zero and is used (in which case >> eg mkfs.fat sets the 32-bit field to zero), or it is zero and the >> 32-bit field is used. Therefore, a BPB is invalid only if *both* >> fields are zero; having one field as zero and the other as non-zero is >> the case to be expected. > > Could you provide reference to the spec which says that? Here's a few descriptions pointing to that fact: https://www.win.tue.nl/~aeb/linux/fs/fat/fat-1.html > The old 2-byte fields "total number of sectors" and "number of sectors per FAT" are now zero; this information is now found in the new 4-byte fields. (Here given in the FAT32 EBPB section but the total sectors 16/32 bit fields semantic is true of FAT12 and FAT16 too.) https://wiki.osdev.org/FAT#BPB_.28BIOS_Parameter_Block.29 > 19 | 2 | The total sectors in the logical volume. If this value is 0, it means there are more than 65535 sectors in the volume, and the actual count is stored in "Large Sectors (bytes 32-35). > 32 | 4 | Large amount of sector on media. This field is set if there are more than 65535 sectors in the volume. (Doesn't specify what the "large" field is set to when unused, but as mentioned mkfs.fat sets it to zero then.) https://technet.microsoft.com/en-us/library/cc976796.aspx > 0x13 | WORD | 0x | > Small Sectors . The number of sectors on the volume represented in 16 > bits (< 65,536). For volumes larger than 65,536 sectors, this field > has a value of zero and the Large Sectors field is used instead. > 0x20 | DWORD | 0x01F03E00 | > Large Sectors . If the value of the Small Sectors field is zero, this > field contains the total number of sectors in the FAT16 volume. If the > value of the Small Sectors field is not zero, the value of this field > is zero. https://staff.washington.edu/dittrich/misc/fatgen103.pdf page 10 > BPB_TotSec16 | 19 | 2 | > This field is the old 16-bit total count of sectors on the volume. > This count includes the count of all sectors in all four regions of the > volume. This field can be 0; if it is 0, then BPB_TotSec32 must be > non-zero. For FAT32 volumes, this field must be 0. For FAT12 and > FAT16 volumes, this field contains the sector count, and > BPB_TotSec32 is 0 if the total sector count “fits” (is less than > 0x1). > BPB_TotSec32 | 32 | 4 | > This field is the new 32-bit total count of sectors on the volume. > This count includes the count of all sectors in all four regions of the > volume. This field can be 0; if it is 0, then BPB_TotSec16 must be > non-zero. For FAT32 volumes, this field must be non-zero. For > FAT12/FAT16 volumes, this field contains the sector count if > BPB_TotSec16 is 0 (count is greater than or equal to 0x1). (This specifies that an unused BPB_TotSec32 field is set to zero.) >> This affects all users of grub_chainloader_patch_bpb which are in >> chainloader.c, freedos.c, and ntldr.c > > I am happy that you fix that issue but > https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system#BPB331_OFS_15h > shows that life is more complicated. > > Could you take that into account? MS-DOS 3.20 and 3.30 BPBs aren't supported anyway. The special case for using a partition table entry's partition length field isn't applicable here. (If it were, the function simply shouldn't check these fields.) And this is the first I've read of that DR-DOS extension. (And again, the simple solution to that one is also not to check the fields for zeros.) >> Tested with lDebug booted in qemu via grub2's >> FreeDOS direct loading support, refer to >> https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug > > Could you put your SOB here? Like this? Signed-off-by: C. Masloch <pus...@38.de> (Should I submit a PATCH v2 for this?) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] chainloader: Fix wrong break condition (must be AND not OR)
The definition of bpb's num_total_sectors_16 and num_total_sectors_32 is that either the 16-bit field is non-zero and is used (in which case eg mkfs.fat sets the 32-bit field to zero), or it is zero and the 32-bit field is used. Therefore, a BPB is invalid only if *both* fields are zero; having one field as zero and the other as non-zero is the case to be expected. This affects all users of grub_chainloader_patch_bpb which are in chainloader.c, freedos.c, and ntldr.c Tested with lDebug booted in qemu via grub2's FreeDOS direct loading support, refer to https://bitbucket.org/ecm/ldosboot + https://bitbucket.org/ecm/ldebug --- grub-core/loader/i386/pc/chainloader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grub-core/loader/i386/pc/chainloader.c b/grub-core/loader/i386/pc/chainloader.c index c79c4fe0f..18220b7aa 100644 --- a/grub-core/loader/i386/pc/chainloader.c +++ b/grub-core/loader/i386/pc/chainloader.c @@ -117,7 +117,7 @@ grub_chainloader_patch_bpb (void *bs, grub_device_t dev, grub_uint8_t dl) if (bpb->num_reserved_sectors == 0) break; - if (bpb->num_total_sectors_16 == 0 || bpb->num_total_sectors_32 == 0) + if (bpb->num_total_sectors_16 == 0 && bpb->num_total_sectors_32 == 0) break; if (bpb->num_fats == 0) -- 2.11.0 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Improve FreeDOS direct loading support compatibility.
On 2012-09-09 21:49 +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
+ {
+grub_relocator_chunk_t ch;
+err = grub_relocator_alloc_chunk_addr (rel, ch,
GRUB_FREEDOS_BPB_ADDR,
+ GRUB_DISK_SECTOR_SIZE);
I don't understand this. Shouldn't it be at 0x7c00 ?
This is intended to be at 0x27a00, not 0x7c00. GRUB_FREEDOS_BPB_ADDR
should evaluate to 0x27a00. I'll try to explain the reasoning for this -
if any of the following is unclear, please feel free to ask for
clarification. Sorry that it's so verbose; I suggest you read the summary
first if you prefer.
-
The FreeDOS kernel's original boot loader (boot/boot.asm in the kernel's
repo) first relocates itself to linear address 2_7A00h (formed as
segmented destination address 1FE0h:7C00h, the segmented source address
being h:7C00h). This is an adjustment from other/earlier loaders that
left the (boot sector) loader at (linear) 0_7C00h and thus restricted the
next stage* to be loaded from disk to about 29 KiB, if it is to be loaded
into contiguous space starting at a lower address (used to be 0_0700h, now
0_0600h for current FreeDOS kernel releases). With this adjustment, the
limit is increased to about 157 KiB.
[* Earlier DOS-C (ie FreeDOS kernel) releases loaded a subsequent IPL
stage first, possibly motivated by this ca. 29 KiB limit as well. Current
FreeDOS kernel releases typically directly load the whole kernel.]
In principle, GRUB could of course write the BPB anywhere up to the top of
conventional memory (below A_h, or below an EBDA if present there)
and incidentally raise the file size limit. (Or potentially the kernel
could be loaded to eg 0_8000h with the BPB placed somewhere below that,
also raising the file size limit - however 0_0600h is clearly documented
as the canonical load address of the current protocol and the kernel might
rely on it.)
However, various programs might assume that the FreeDOS load protocol
places the BPB at exactly 2_7A00h (with ss:bp and ds:bp specifying
1FE0h:7C00h). I don't know whether the current kernel itself relies on
this. I'm aware that at least MetaBoot** does search at that particular
(linear) address, so placing the BPB/sector there is necessary (though not
sufficient) for it. (MetaBoot's only purpose is to reload a different
next-stage file, hence loading it from GRUB isn't important because GRUB
can instead directly load the desired next-stage file. I merely mentioned
MetaBoot for its assumption that this particular position is implied by
the load protocol.)
[** MetaBoot belongs to the MetaKern loader for FreeDOS, developed by
FreeDOS kernel maintainer Eric Auer. It is covered by the GPL 2 with an
additional loading exception/permission, and distributed as part of
FreeDOS.]
In addition, arguably there is little use in raising the file size limit
above what the original loader can work with. This differs from my
opinion, but might be a relevant view for GRUB to consider.
-
So, summing it all up, the resulting 2_7A00h address of the BPB is to most
closely mimic the original loader, which arrived there to partially raise
a limit caused by that loader's size constraints (from the original 29 KiB
limit to 157 KiB). That level of compatibility might be unnecessary (for
the kernel itself), but apart from reproducing the original's more recent
file size limit there is no harm in it.
Note that in GRUB's ntldr.c, the BPB is written at 0_7C00h and no
equivalent file size limit exists because the next stage (NTLDR) file is
loaded starting at a higher address, 2_h.
Do you plan to contribute more? If so I'd prefer to make you sign a
copyright assignment, otherwise this patch can go in, once it's clear
with addresses.
I could gladly sign an assignment, however I don't yet plan to contribute
more.
Regards,
Chris Masloch
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] Improve FreeDOS direct loading support compatibility.
Thanks for the help. The attached patch includes spaces around all operators, doesn't include the void cast, and doesn't mention (E)DR-DOS any longer. I recently learned that the dl register requirement is present in prior releases of the FreeDOS kernel too, still called DOS-C. These DOS-C releases already are under GPLv2+ so I hope it is okay to mention them in the comment, as the attached patch does. Regards, Chris improve-freedos-compatibility-2.diff Description: Binary data ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Improve FreeDOS direct loading support compatibility.
Hello, A bit about me first; feel free to skip this paragraph. I know 86 assembly language well and have been involved in FreeDOS kernel development. On the freedos-user list I read that GRUB 2.00 added FreeDOS direct loading support. I have reviewed GRUB's implementation of the FreeDOS load protocol. The attached patch improves compatibility to the FreeDOS kernel and its original loader as well as potential compatibility to other DOS-like kernels and loaders. In particular, some requirements of the current EDR-DOS load protocol are implemented by this as well (though possibly not all of them) without harming the loading of FreeDOS kernels. Regards, Chris Masloch improve-freedos-compatibility.diff Description: Binary data ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
