Page MenuHomeFreeBSD

arm64: limit EFI excluded regions to physical memory types
ClosedPublic

Authored by mhorne on Mar 9 2023, 7:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 24, 5:21 AM
Unknown Object (File)
Fri, Jun 21, 7:53 AM
Unknown Object (File)
Wed, Jun 12, 7:59 PM
Unknown Object (File)
May 16 2024, 4:25 AM
Unknown Object (File)
May 16 2024, 4:24 AM
Unknown Object (File)
May 16 2024, 4:24 AM
Unknown Object (File)
May 10 2024, 3:32 PM
Unknown Object (File)
May 2 2024, 1:47 PM
Subscribers

Details

Summary

Consolidate add_efi_map_entry() and exclude_efi_map_entry() into a
single function, handle_efi_map_entry(), so that the exact set of entry
types handled is the same in the addition or exclusion cases. Before,
exclude_efi_map_entry() had a 'default' case that would exclude all
entry types that were not listed explicitly in the switch statement.

Logically, we do not need to exclude a range that could not possibly be
added to physmem, and we do not need to exclude bus ranges that are not
physical memory, for example EFI_MD_TYPE_IOMEM.

Since physmem's ram0 device will reserve bus memory resources for its
owned ranges, this was preventing attachment of the watchdog device on
the RPI4B. For some reason its region of memory-mapped I/O appeared in
the EFI memory map (with the aformentioned EFI_MD_TYPE_IOMEM type). This
change fixes the attachment issue, as we prevent the physmem API from
messing with this range of bus space.

PR: 270044
Reported by: karels, Mark Millard

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mhorne requested review of this revision.Mar 9 2023, 7:33 PM

I thnk this is fine. We'll need to keep an eye on the standard to see if we need to add any others in the future.

This revision is now accepted and ready to land.Mar 9 2023, 7:43 PM

This fixes the problem on my RPi 4B.

Should there be an empty default case, or should it be in the top group? And should there be a break after the physmem_exclude_region call (pre-existing, I know)?

Add explicit default case for clarity; also in add_efi_map_entry().

This revision now requires review to proceed.Mar 9 2023, 8:19 PM
sys/arm64/arm64/machdep.c
476

Probably I should tweak this to just "Other types".

I wonder if we could create a common function exclude_efi_map_entry and add_efi_map_entry both call so we don't need to have two lists that may be out of sync if we are not careful.

Handle andrew's suggestion and consolidate add_efi_map_entry() and exclude_efi_map_entry().

sys/arm64/arm64/machdep.c
476

Stuffing a bool into a void * is messy. Maybe it is worth adding a bool parameter to the callback function? Or creating a union of a void * and a bool, and passing a union?

sys/arm64/arm64/machdep.c
476

I'm OK with passing the bool this way. If you do change it you could pass NULL/not-NULL or put the bool on the stack & pass a pointer to it.

500

Is the else needed?

Pass bool on the stack; drop empty else.

This revision is now accepted and ready to land.Mar 15 2023, 2:20 PM

LGTM on form; I'll trust you and Andrew on the sets of EFI types.