Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for Hybrid MBR #168

Closed
wants to merge 2 commits into from
Closed

add support for Hybrid MBR #168

wants to merge 2 commits into from

Conversation

oneingan
Copy link

@oneingan oneingan commented Mar 7, 2023

Closes #166

@@ -0,0 +1,62 @@
{ disks ? [ "/dev/sda" ], ... }:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggestion how we should name hybrid.nix in that case since it's still a bit confusing to have both.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I comment here #166 (comment) existent gpt-bios-compat.nix is enough. hybrid.nix or hybrid-tmpfs-onroot.nix looks confusing and redundant for me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, the gpt-bios-compat can't do EFI because it has no efi or boot partition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I would also prefer something that can support both EFI/BIOS. We have installer that works with both EFI and BIOS and makes it unnecessary for people to have to figure out what mode there machines was booted from.

Copy link
Author

@oneingan oneingan Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, so maybe we could reorganize as:

remove gpt-bios-compat.nix (I'm not sure which is the use case of this without EFI partition)
EDIT: Ok, use case is boot larger disks in BIOS ONLY systems.
remove hybrid-tmpfs-onroot.nix (or not, but for clarity on discussion)

then if we want to keep the hybrid keyword:

hybrid.nix -> hybrid_boot.nix

Also, being stricts, seems like the real name of hybrid boot using bios_grub flags is GPT-only protocol. As in https://wiki.archlinux.org/title/Arch_boot_process#Feature_comparison and https://repo.or.cz/syslinux.git/blob/HEAD:/doc/gpt.txt.

or refered as "BIOS/GPT boot" in wikipedia: https://en.wikipedia.org/wiki/BIOS_boot_partition

hybrid.nix -> bios-gpt-boot.nix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, maybe can we get this to a conclusion?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can call it bios-efi-boot? since it does both?

Copy link
Collaborator

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should support this, seems kinda clunky and I don't see many usecases? can't this be achieved with the existing table type and some extra flags?

{
type = "hybrid_partition";
gptPartitionNumber = 1;
mbrPartitionType = "0x0c";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are other valid mbrPartitionType options?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whatever hexcode dos table partitions support, as in https://en.wikipedia.org/wiki/Partition_type#List_of_partition_IDs.

If null then it guess partition type based on GPT one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but would it still be a hybrid mbr if the partitionType is not "0x0c"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, set to 0x0chave sense for RPi because RPi boot process look exactly for it, but other use cases could work with default value.

hybrid_partitions = [
{
type = "hybrid_partition";
gptPartitionNumber = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the index in the list enough? why do we need to count here? what would happen if I set this to 3?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to choose which GPT partition want to make available in DOS table. Maybe a sane default using index is ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is the index of the gpt partition table which is defined in content?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct.

@oneingan
Copy link
Author

oneingan commented Mar 13, 2023

Not sure if we should support this, seems kinda clunky and I don't see many usecases? can't this be achieved with the existing table type and some extra flags?

Sure not. Use cases are multiple, mine is to create a working SD Card for Rapsberry Pi. But is also used frequently to boot multiple operating systems setups.

In anycase the proposed interfaces makes very explicit the use of this feature, and also clean and flexible enough for people craft their disks partitioning without affecting layers below.

Copy link
Member

@DavHau DavHau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the structure of the hybrid_table type should be simlified.

  • remove the additional hybrid_partitions list and instead introduce a new partition type containing all fields
  • this will get rid of the gptPartitionNumber, as there is only one list of partitions left
    Modified example:
{ disks ? [ "/dev/sda" ], ... }:
{
  disk = {
    main = {
      type = "disk";
      device = builtins.elemAt disks 0;
      content = {
        type = "hybrid_table";
        efiGptPartitionFirst = false;
        partitions = [
          {
            # the `type` field is not needed and can be omitted
            type = "hybrid_partition";

            name = "TOW-BOOT-FI";
            start = "0%";
            end = "32MiB";
            fs-type = "fat32";
            content = {
              type = "filesystem";
              format = "vfat";
              mountpoint = null;
            };

            # mbr specific fields
            mbrPartitionType = "0x0c";
            bootable = true; # previously named `mbrBootableFlag`
          }
          {
            type = "partition";
            name = "ESP";
            start = "32MiB";
            end = "512MiB";
            bootable = true;
            content = {
              type = "filesystem";
              format = "vfat";
              mountpoint = "/boot";
            };
          }
          # ...
        ];
      };
    };
  };
}

internal = true;
description = "Hybrid MBR/GPT Partition table";
};
efiGptPartitionFirst = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to be configurable? What is the use case? Why not always place the GPT partition first?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GPT partition first is always recommended for GRUB bootloader, but, for instance, Raspberry Pi bootloader, looks exactly for a 0x0c partition type as first partition.

IMHO, disko may support every flow gdisk hybrid tool support. As related in "Creating a Hybrid MBR" from https://www.rodsbooks.com/gdisk/hybrid.html

@DavHau
Copy link
Member

DavHau commented Mar 22, 2023

Please add a test for the new module, like for example:
tests/boot-hybrid.nix:

{ pkgs ? (import <nixpkgs> { })
, makeDiskoTest ? (pkgs.callPackage ./lib.nix { }).makeDiskoTest
}:
makeDiskoTest {
  name = "boot-hybrid";
  disko-config = ../example/hybrid-mbr.nix;
  extraTestScript = ''
    machine.succeed("mountpoint /boot");
  '';
}

danjujan added a commit to danjujan/disko that referenced this pull request Jan 13, 2024
@Mic92
Copy link
Member

Mic92 commented Jan 13, 2024

Replaced by #508

@Mic92 Mic92 closed this Jan 13, 2024
@oneingan oneingan deleted the hybridmbr branch January 14, 2024 17:01
Lassulus pushed a commit that referenced this pull request Jan 26, 2024
Add hybrid MBR functionality to the gpt type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Real hybrid MBR/GPT partitioning scheme
4 participants