Guard the lower 1MB of memory

[jasoncouture]

This also adds a new fuction to ignore the attempts to guard the lower 1MB if it's really required.

Closes #311

[phil-opp]

Thanks a lot for this PR!

I'm not sure if I understand the approach though. Why are the changes to LegacyMemoryRegion::kind necessary and how do they help in guarding the first MB?

I think the main issue is that the UEFI allocation used for loading the kernel might already use the first MB if we are unlucky. So to be safe, we would need to allocate that first megabyte before creating the allocation. We can use the "allocate at address" API mentioned by @tsoutsman in #311 (comment) for that.

[jasoncouture]

@phil-opp this is ready for review. I also added tests.

[phil-opp]

We already use the name start_frame in multiple places, so we should name this field differently to avoid confusion. I think min_frame would be a fitting name:

Suggested change

	start_frame: PhysFrame,
	min_frame: PhysFrame,

[phil-opp]

This seems a bit complicated. How about we just use a max call in new_starting_at and keep calling it from the new function?

Suggested change

	if frame.start_address().as_u64() < LOWER_MEMORY_END_PAGE {
	// Skip the first 1MB of frames, regardless of what was requested.
	// there are use cases that require lower conventional memory access. (Such as SMP SIPI)
	return Self::new(memory_map);
	}
	return unsafe { Self::unsafe_new_starting_at(frame, memory_map) };
	}
	/// Creates a new frame allocator based on the given legacy memory regions. Skips any frames
	/// before the given `frame`, without attempting to protect the lower 1MB of memory.
	pub unsafe fn unsafe_new_starting_at(frame: PhysFrame, memory_map: I) -> Self {
	// Skip the first 1MB of frames, regardless of what was requested.
	// there are use cases that require lower conventional memory access. (Such as SMP SIPI)
	let lower_mem_end = PhysFrame::containing_address(PhysAddr::new(LOWER_MEMORY_END_PAGE));
	let frame = core::cmp::max(frame, lower_mem_end);

[phil-opp]

With the suggested changes below, we could keep the previous implementation:

Suggested change

	// also skip the first 1MB of frames, there are use cases that require lower conventional memory access. (Such as SMP SIPI)
	let start_frame = PhysFrame::containing_address(PhysAddr::new(LOWER_MEMORY_END_PAGE));
	unsafe { Self::unsafe_new_starting_at(start_frame, memory_map) }
	let start_frame = PhysFrame::containing_address(PhysAddr::new(0x1000));
	Self::new_starting_at(start_frame, memory_map)

[phil-opp]

I would prefer to split this condition so that we can add a comment:

Suggested change

	} else if descriptor.start() >= next_free
	|| end <= self.start_frame.start_address()
	{
	} else if descriptor.start() >= next_free {
	MemoryRegionKind::Usable
	} else if end <= self.start_frame.start_address() {
	// treat regions that end before the minimum frame as usable too

[phil-opp]

I think we need to update this else branch too, in case the descriptor goes beyond 0x100000.

[phil-opp]

Reordering the variants changes their integer value, which is technically a breaking change. So we should undo this change if possible.

[phil-opp]

We should also account for regions that don't end at LOWER_MEMORY_END_PAGE. For example:

Suggested change

	if region.end <= LOWER_MEMORY_END_PAGE && region.kind == MemoryRegionKind::Usable {
	let pages = (region.end - region.start) / 4096;
	count += pages;
	}
	if region.kind == MemoryRegionKind::Usable && region.start < LOWER_MEMORY_END_PAGE {
	let end = core::cmp::min(region.end, LOWER_MEMORY_END_PAGE);
	let pages = (end - region.start) / 4096;
	count += pages;
	}

[phil-opp]

Thanks for the updates! I left some suggestions, otherwise these changes look good to me. However, I still think that we need to ensure that we don't use the first megabyte for the kernel allocation in the UEFI loader, as I noted above:

I think the main issue is that the UEFI allocation used for loading the kernel might already use the first MB if we are unlucky. So to be safe, we would need to allocate that first megabyte before creating the allocation. We can use the "allocate at address" API mentioned in #311 (comment) for that.

[phil-opp]

@jasoncouture Friendly ping :). What's the status of this?

[jasoncouture]

@jasoncouture Friendly ping :). What's the status of this?

I'll take another look weekend after next. Got some personal things to attend for the next week or so.

[Freax13]

Superseded by #446