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

Do not report as Mactel for Apple Macs with the T2 chip #1090

Merged
merged 3 commits into from Dec 2, 2022
Merged

Do not report as Mactel for Apple Macs with the T2 chip #1090

merged 3 commits into from Dec 2, 2022

Conversation

sharpenedblade
Copy link
Contributor

T2 apple macs behave much more like normal EFI computers than the current use of Mactel. This PR will make T2 Apple Macs report as x86-64 EFI computers, which is probably the right behavior. This PR also adds the is_t2mac() function to arch.py for detecting T2 macs.

@StorageGhoul
Copy link

Can one of the admins verify this patch?

@sharpenedblade
Copy link
Contributor Author

@tbzatek
Copy link
Member

tbzatek commented Nov 29, 2022

Yeah, this looks good enough for T2-based Macs. Could you please add similar change for the T1 chip? I've had MacBookPro14,1 that came with regular FAT32 EFI partition, I put my Gentoo rootfs in there right away and it worked just fine.

@vojtechtrefny
Copy link
Member

Jenkins, ok to test.

@bcl
Copy link
Contributor

bcl commented Nov 29, 2022

The problem with how this is implemented is that any new T1 or T2 system will fall back to being treated like an old mactel system. It would be better if it detected the old ones and defaulted to the new behavior on newer systems.

@sharpenedblade
Copy link
Contributor Author

sharpenedblade commented Nov 29, 2022

The problem with how this is implemented is that any new T1 or T2 system will fall back to being treated like an old mactel system. It would be better if it detected the old ones and defaulted to the new behavior on newer systems.

Normally this code would be a bad approach, but Apple no longer makes T2 Intel macs, so our list does not need to be updated. Apple said it will not make new Intel macs at all.

@sharpenedblade
Copy link
Contributor Author

sharpenedblade commented Nov 29, 2022

Yeah, this looks good enough for T2-based Macs. Could you please add similar change for the T1 chip? I've had MacBookPro14,1 that came with regular FAT32 EFI partition, I put my Gentoo rootfs in there right away and it worked just fine.

I could not find a list of T1 macs, but if there is a list it is just a matter of adding the DMI ids to the set.

EDIT: After some research, I think that T1 macs should continue to use the old Mactel system.

Copy link
Member

@vojtechtrefny vojtechtrefny left a comment

Choose a reason for hiding this comment

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

Looks good and simple enough. Thank you!

blivet/arch.py Show resolved Hide resolved
elif not os.path.isfile(DMI_PRODUCT_ID):
t2_mac = False
else:
buf = open(DMI_PRODUCT_ID).read()
Copy link
Member

Choose a reason for hiding this comment

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

Leaking file descriptor.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we do exactly the same below with the DMI_CHASSIS_VENDOR. I guess I can fix both later in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I leave this as is.

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 will fix this later, we have this problem at more places. I want to give some more time to other people to review this, but other than that, this is ready to be merged. Thank you again.

@tbzatek
Copy link
Member

tbzatek commented Nov 30, 2022

EDIT: After some research, I think that T1 macs should continue to use the old Mactel system.

Fair enough, I guess the secure boot enforcement the original bugreport was about works differently than on the T2 platforms.

@sharpenedblade
Copy link
Contributor Author

EDIT: After some research, I think that T1 macs should continue to use the old Mactel system.

Fair enough, I guess the secure boot enforcement the original bugreport was about works differently than on the T2 platforms.

Its not just that, the EFI in T1 macs will only properly honor some boot icon/name stuff on HFS+ partitions.

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.

None yet

5 participants