| Message ID | 20240519153542.2572014-1-r.stratiienko@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | part: efi: Rely on a definition to specify the primary GPT position | expand |
On 19.05.24 17:35, Roman Stratiienko wrote: > Use GPT_PRIMARY_PARTITION_TABLE_LBA as the only source for a primary GPT > location instead of hardcoding it every time. > > Sometimes, we need to shift the primary GPT location. > Having the location in the single place simplifies such shifting. In gdisk there is an expert command "j - move the main partition table". This corresponds to CONFIG_EFI_PARTITION_ENTRIES_OFF. It does not move the GPT primary header but only the partition entries. So sector 1 is still occupied. If I read you correctly, CONFIG_EFI_PARTITION_ENTRIES_OFF is not doing what you need. Instead of only moving the partition entries of the primary partition table, you want to move the complete primary GPT table. Somehow the operating system must find the primary GPT table. When writing the partition table shouldn't the offset be reflected in the protective MBR fields StartingCHS and StartingLBA? See Table 5.4 "Protective MBR Partition Record protecting the entire disk*" of the UEFI 2.10 specification. Does Linux support this? For testing remove the backup partition table! GPT_PRIMARY_PARTITION_TABLE_LBA should be made configurable in this case. Best regards Heinrich > > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> > --- > disk/part_efi.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/disk/part_efi.c b/disk/part_efi.c > index 4ce9243ef2..b55e251ea1 100644 > --- a/disk/part_efi.c > +++ b/disk/part_efi.c > @@ -387,7 +387,7 @@ int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e) > gpt_h->header_crc32 = cpu_to_le32(calc_crc32); > > /* Write the First GPT to the block right after the Legacy MBR */ > - if (blk_dwrite(desc, 1, 1, gpt_h) != 1) > + if (blk_dwrite(desc, (lbaint_t)le64_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1) > goto err; > > if (blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), > @@ -534,7 +534,7 @@ int gpt_fill_pte(struct blk_desc *desc, > > static uint32_t partition_entries_offset(struct blk_desc *desc) > { > - uint32_t offset_blks = 2; > + uint32_t offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > uint32_t __maybe_unused offset_bytes; > int __maybe_unused config_offset; > > @@ -572,8 +572,8 @@ static uint32_t partition_entries_offset(struct blk_desc *desc) > * The earliest LBA this can be at is LBA#2 (i.e. right behind > * the (protective) MBR and the GPT header. > */ > - if (offset_blks < 2) > - offset_blks = 2; > + if (offset_blks < (GPT_PRIMARY_PARTITION_TABLE_LBA + 1)) > + offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > > return offset_blks; > } > @@ -584,7 +584,7 @@ int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid, > gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT); > gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1); > gpt_h->header_size = cpu_to_le32(sizeof(gpt_header)); > - gpt_h->my_lba = cpu_to_le64(1); > + gpt_h->my_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA); > gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1); > gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34); > gpt_h->partition_entry_lba =
Hello Heinrich, Thank you for your comments. Just a few thoughts about this patch. 1. I was also sending this patch as a noninvasive, semi-cosmetic change. Relying on hardcoded values is usually not a good practice. 2. Shifting of primary GPT works in u-boot and Linux kernel with minor tweaks. See patches [1] and [2] for more context. I'm not going to upstream them. 3. Accepting this patch upstream helps me to reduce the dependency chain and potential conflicts during the rebase. BR, Roman. [1]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/bootloader/u-boot/0005-GLODROID-Shift-GPT-position-to-support-Amlogic-platf.patch [2]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/kernel/common-android14-6.1-lts/0001-GLODROID-Allow-placing-primary-GPT-to-2MiB-offset-to.patch пн, 10 черв. 2024 р. о 15:22 Heinrich Schuchardt <xypron.glpk@gmx.de> пише: > > On 19.05.24 17:35, Roman Stratiienko wrote: > > Use GPT_PRIMARY_PARTITION_TABLE_LBA as the only source for a primary GPT > > location instead of hardcoding it every time. > > > > Sometimes, we need to shift the primary GPT location. > > Having the location in the single place simplifies such shifting. > > In gdisk there is an expert command "j - move the main partition table". > This corresponds to CONFIG_EFI_PARTITION_ENTRIES_OFF. It does not move > the GPT primary header but only the partition entries. So sector 1 is > still occupied. > > If I read you correctly, CONFIG_EFI_PARTITION_ENTRIES_OFF is not doing > what you need. Instead of only moving the partition entries of the > primary partition table, you want to move the complete primary GPT table. > > Somehow the operating system must find the primary GPT table. > > When writing the partition table shouldn't the offset be reflected in > the protective MBR fields StartingCHS and StartingLBA? See Table 5.4 > "Protective MBR Partition Record protecting the entire disk*" of the > UEFI 2.10 specification. > > Does Linux support this? For testing remove the backup partition table! > > GPT_PRIMARY_PARTITION_TABLE_LBA should be made configurable in this case. > > Best regards > > Heinrich > > > > > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> > > --- > > disk/part_efi.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/disk/part_efi.c b/disk/part_efi.c > > index 4ce9243ef2..b55e251ea1 100644 > > --- a/disk/part_efi.c > > +++ b/disk/part_efi.c > > @@ -387,7 +387,7 @@ int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e) > > gpt_h->header_crc32 = cpu_to_le32(calc_crc32); > > > > /* Write the First GPT to the block right after the Legacy MBR */ > > - if (blk_dwrite(desc, 1, 1, gpt_h) != 1) > > + if (blk_dwrite(desc, (lbaint_t)le64_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1) > > goto err; > > > > if (blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), > > @@ -534,7 +534,7 @@ int gpt_fill_pte(struct blk_desc *desc, > > > > static uint32_t partition_entries_offset(struct blk_desc *desc) > > { > > - uint32_t offset_blks = 2; > > + uint32_t offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > > uint32_t __maybe_unused offset_bytes; > > int __maybe_unused config_offset; > > > > @@ -572,8 +572,8 @@ static uint32_t partition_entries_offset(struct blk_desc *desc) > > * The earliest LBA this can be at is LBA#2 (i.e. right behind > > * the (protective) MBR and the GPT header. > > */ > > - if (offset_blks < 2) > > - offset_blks = 2; > > + if (offset_blks < (GPT_PRIMARY_PARTITION_TABLE_LBA + 1)) > > + offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > > > > return offset_blks; > > } > > @@ -584,7 +584,7 @@ int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid, > > gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT); > > gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1); > > gpt_h->header_size = cpu_to_le32(sizeof(gpt_header)); > > - gpt_h->my_lba = cpu_to_le64(1); > > + gpt_h->my_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA); > > gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1); > > gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34); > > gpt_h->partition_entry_lba = >
On 7/13/24 23:16, Roman Stratiienko wrote: > Hello Heinrich, > > Thank you for your comments. > > Just a few thoughts about this patch. > > 1. I was also sending this patch as a noninvasive, semi-cosmetic > change. Relying on hardcoded values is usually not a good practice. > 2. Shifting of primary GPT works in u-boot and Linux kernel with minor > tweaks. See patches [1] and [2] for more context. I'm not going to > upstream them. > 3. Accepting this patch upstream helps me to reduce the dependency > chain and potential conflicts during the rebase. Hello Roman, could you, please, answer my questions below. Best regards Heinrich > > BR, Roman. > > [1]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/bootloader/u-boot/0005-GLODROID-Shift-GPT-position-to-support-Amlogic-platf.patch > [2]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/kernel/common-android14-6.1-lts/0001-GLODROID-Allow-placing-primary-GPT-to-2MiB-offset-to.patch > > пн, 10 черв. 2024 р. о 15:22 Heinrich Schuchardt <xypron.glpk@gmx.de> пише: >> >> On 19.05.24 17:35, Roman Stratiienko wrote: >>> Use GPT_PRIMARY_PARTITION_TABLE_LBA as the only source for a primary GPT >>> location instead of hardcoding it every time. >>> >>> Sometimes, we need to shift the primary GPT location. >>> Having the location in the single place simplifies such shifting. >> >> In gdisk there is an expert command "j - move the main partition table". >> This corresponds to CONFIG_EFI_PARTITION_ENTRIES_OFF. It does not move >> the GPT primary header but only the partition entries. So sector 1 is >> still occupied. >> >> If I read you correctly, CONFIG_EFI_PARTITION_ENTRIES_OFF is not doing >> what you need. Instead of only moving the partition entries of the >> primary partition table, you want to move the complete primary GPT table. >> >> Somehow the operating system must find the primary GPT table. >> >> When writing the partition table shouldn't the offset be reflected in >> the protective MBR fields StartingCHS and StartingLBA? See Table 5.4 >> "Protective MBR Partition Record protecting the entire disk*" of the >> UEFI 2.10 specification. >> >> Does Linux support this? For testing remove the backup partition table! >> >> GPT_PRIMARY_PARTITION_TABLE_LBA should be made configurable in this case. >> >> Best regards >> >> Heinrich >> >>> >>> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> >>> --- >>> disk/part_efi.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/disk/part_efi.c b/disk/part_efi.c >>> index 4ce9243ef2..b55e251ea1 100644 >>> --- a/disk/part_efi.c >>> +++ b/disk/part_efi.c >>> @@ -387,7 +387,7 @@ int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e) >>> gpt_h->header_crc32 = cpu_to_le32(calc_crc32); >>> >>> /* Write the First GPT to the block right after the Legacy MBR */ >>> - if (blk_dwrite(desc, 1, 1, gpt_h) != 1) >>> + if (blk_dwrite(desc, (lbaint_t)le64_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1) >>> goto err; >>> >>> if (blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), >>> @@ -534,7 +534,7 @@ int gpt_fill_pte(struct blk_desc *desc, >>> >>> static uint32_t partition_entries_offset(struct blk_desc *desc) >>> { >>> - uint32_t offset_blks = 2; >>> + uint32_t offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; >>> uint32_t __maybe_unused offset_bytes; >>> int __maybe_unused config_offset; >>> >>> @@ -572,8 +572,8 @@ static uint32_t partition_entries_offset(struct blk_desc *desc) >>> * The earliest LBA this can be at is LBA#2 (i.e. right behind >>> * the (protective) MBR and the GPT header. >>> */ >>> - if (offset_blks < 2) >>> - offset_blks = 2; >>> + if (offset_blks < (GPT_PRIMARY_PARTITION_TABLE_LBA + 1)) >>> + offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; >>> >>> return offset_blks; >>> } >>> @@ -584,7 +584,7 @@ int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid, >>> gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT); >>> gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1); >>> gpt_h->header_size = cpu_to_le32(sizeof(gpt_header)); >>> - gpt_h->my_lba = cpu_to_le64(1); >>> + gpt_h->my_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA); >>> gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1); >>> gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34); >>> gpt_h->partition_entry_lba = >>
Hello Heinrich, Sorry, I'm having a hard time answering your questions. > When writing the partition table shouldn't the offset be reflected in the protective MBR fields StartingCHS and StartingLBA? See Table 5.4 "Protective MBR Partition Record protecting the entire disk*" of the UEFI 2.10 specification. > Does Linux support this? Linux doesn't follow the protective MBR to find the GPT location if that was your question. > For testing remove the backup partition table! Testing what? The backup partition table is always restored by Linux if missing or corrupted. I tested it previously and it worked. That was the only question I could find. If your other statements intended to ask the question, please let me know. BR, Roman нд, 14 лип. 2024 р. о 09:39 Heinrich Schuchardt <xypron.glpk@gmx.de> пише: > > On 7/13/24 23:16, Roman Stratiienko wrote: > > Hello Heinrich, > > > > Thank you for your comments. > > > > Just a few thoughts about this patch. > > > > 1. I was also sending this patch as a noninvasive, semi-cosmetic > > change. Relying on hardcoded values is usually not a good practice. > > 2. Shifting of primary GPT works in u-boot and Linux kernel with minor > > tweaks. See patches [1] and [2] for more context. I'm not going to > > upstream them. > > 3. Accepting this patch upstream helps me to reduce the dependency > > chain and potential conflicts during the rebase. > > Hello Roman, > > could you, please, answer my questions below. > > Best regards > > Heinrich > > > > > BR, Roman. > > > > [1]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/bootloader/u-boot/0005-GLODROID-Shift-GPT-position-to-support-Amlogic-platf.patch > > [2]: https://github.com/GloDroidCommunity/amlogic-series/blob/8cc5ee7df1358092f77f43aa0c9661883473e5af/patches-aosp/glodroid/kernel/common-android14-6.1-lts/0001-GLODROID-Allow-placing-primary-GPT-to-2MiB-offset-to.patch > > > > пн, 10 черв. 2024 р. о 15:22 Heinrich Schuchardt <xypron.glpk@gmx.de> пише: > >> > >> On 19.05.24 17:35, Roman Stratiienko wrote: > >>> Use GPT_PRIMARY_PARTITION_TABLE_LBA as the only source for a primary GPT > >>> location instead of hardcoding it every time. > >>> > >>> Sometimes, we need to shift the primary GPT location. > >>> Having the location in the single place simplifies such shifting. > >> > >> In gdisk there is an expert command "j - move the main partition table". > >> This corresponds to CONFIG_EFI_PARTITION_ENTRIES_OFF. It does not move > >> the GPT primary header but only the partition entries. So sector 1 is > >> still occupied. > >> > >> If I read you correctly, CONFIG_EFI_PARTITION_ENTRIES_OFF is not doing > >> what you need. Instead of only moving the partition entries of the > >> primary partition table, you want to move the complete primary GPT table. > >> > >> Somehow the operating system must find the primary GPT table. > >> > >> When writing the partition table shouldn't the offset be reflected in > >> the protective MBR fields StartingCHS and StartingLBA? See Table 5.4 > >> "Protective MBR Partition Record protecting the entire disk*" of the > >> UEFI 2.10 specification. > >> > >> Does Linux support this? For testing remove the backup partition table! > >> > >> GPT_PRIMARY_PARTITION_TABLE_LBA should be made configurable in this case. > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> > >>> --- > >>> disk/part_efi.c | 10 +++++----- > >>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/disk/part_efi.c b/disk/part_efi.c > >>> index 4ce9243ef2..b55e251ea1 100644 > >>> --- a/disk/part_efi.c > >>> +++ b/disk/part_efi.c > >>> @@ -387,7 +387,7 @@ int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e) > >>> gpt_h->header_crc32 = cpu_to_le32(calc_crc32); > >>> > >>> /* Write the First GPT to the block right after the Legacy MBR */ > >>> - if (blk_dwrite(desc, 1, 1, gpt_h) != 1) > >>> + if (blk_dwrite(desc, (lbaint_t)le64_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1) > >>> goto err; > >>> > >>> if (blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), > >>> @@ -534,7 +534,7 @@ int gpt_fill_pte(struct blk_desc *desc, > >>> > >>> static uint32_t partition_entries_offset(struct blk_desc *desc) > >>> { > >>> - uint32_t offset_blks = 2; > >>> + uint32_t offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > >>> uint32_t __maybe_unused offset_bytes; > >>> int __maybe_unused config_offset; > >>> > >>> @@ -572,8 +572,8 @@ static uint32_t partition_entries_offset(struct blk_desc *desc) > >>> * The earliest LBA this can be at is LBA#2 (i.e. right behind > >>> * the (protective) MBR and the GPT header. > >>> */ > >>> - if (offset_blks < 2) > >>> - offset_blks = 2; > >>> + if (offset_blks < (GPT_PRIMARY_PARTITION_TABLE_LBA + 1)) > >>> + offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; > >>> > >>> return offset_blks; > >>> } > >>> @@ -584,7 +584,7 @@ int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid, > >>> gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT); > >>> gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1); > >>> gpt_h->header_size = cpu_to_le32(sizeof(gpt_header)); > >>> - gpt_h->my_lba = cpu_to_le64(1); > >>> + gpt_h->my_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA); > >>> gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1); > >>> gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34); > >>> gpt_h->partition_entry_lba = > >> >
diff --git a/disk/part_efi.c b/disk/part_efi.c index 4ce9243ef2..b55e251ea1 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -387,7 +387,7 @@ int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e) gpt_h->header_crc32 = cpu_to_le32(calc_crc32); /* Write the First GPT to the block right after the Legacy MBR */ - if (blk_dwrite(desc, 1, 1, gpt_h) != 1) + if (blk_dwrite(desc, (lbaint_t)le64_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1) goto err; if (blk_dwrite(desc, le64_to_cpu(gpt_h->partition_entry_lba), @@ -534,7 +534,7 @@ int gpt_fill_pte(struct blk_desc *desc, static uint32_t partition_entries_offset(struct blk_desc *desc) { - uint32_t offset_blks = 2; + uint32_t offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; uint32_t __maybe_unused offset_bytes; int __maybe_unused config_offset; @@ -572,8 +572,8 @@ static uint32_t partition_entries_offset(struct blk_desc *desc) * The earliest LBA this can be at is LBA#2 (i.e. right behind * the (protective) MBR and the GPT header. */ - if (offset_blks < 2) - offset_blks = 2; + if (offset_blks < (GPT_PRIMARY_PARTITION_TABLE_LBA + 1)) + offset_blks = GPT_PRIMARY_PARTITION_TABLE_LBA + 1; return offset_blks; } @@ -584,7 +584,7 @@ int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid, gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT); gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1); gpt_h->header_size = cpu_to_le32(sizeof(gpt_header)); - gpt_h->my_lba = cpu_to_le64(1); + gpt_h->my_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA); gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1); gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34); gpt_h->partition_entry_lba =
Use GPT_PRIMARY_PARTITION_TABLE_LBA as the only source for a primary GPT location instead of hardcoding it every time. Sometimes, we need to shift the primary GPT location. Having the location in the single place simplifies such shifting. Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> --- disk/part_efi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)