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

Implement the multiprocessor wakeup mechanism. #225

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Hsy-Intel
Copy link
Contributor

ACPI specification r6.4 introduced the Multiprocessor Wakeup Structure to let the bootstrap processor wake up application processors with a mailbox.

This PR follows the design and definition of ACPI specification and has been verified in my project.

Copy link
Member

@IsaacWoods IsaacWoods left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've not tried to use the new mailbox interface myself, but it'd be cool to have support in the crate.

I've added some review comments around the implementation.

acpi/src/madt.rs Outdated Show resolved Hide resolved
acpi/src/madt.rs Outdated
impl Madt {
pub fn wakeup_aps(
Copy link
Member

Choose a reason for hiding this comment

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

It feels odd to me to implement high-level functionality like booting APs directly on the MADT. This should probably be implemented in the platform layer - this should extract the physical address of the mailbox during its existing MADT parsing, and then provide this method on PlatformInfo I feel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it can be a separate function in the platform layer? It doesn't seem to belong to PlatformInfo.

acpi/src/madt.rs Outdated Show resolved Hide resolved
acpi/src/madt.rs Outdated Show resolved Hide resolved
@Hsy-Intel
Copy link
Contributor Author

Hi @IsaacWoods , could you review this PR again when you have time? Thanks~

Copy link
Member

@IsaacWoods IsaacWoods left a comment

Choose a reason for hiding this comment

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

Thanks - this is organisationally good to go, just one remaining point re volatile writes to the mailbox.

acpi/src/platform/mod.rs Outdated Show resolved Hide resolved
acpi/src/platform/mod.rs Outdated Show resolved Hide resolved
@Hsy-Intel
Copy link
Contributor Author

Thanks - this is organisationally good to go, just one remaining point re volatile writes to the mailbox.

The issue has been fixed. Thanks for your comments. @IsaacWoods

Copy link
Member

@IsaacWoods IsaacWoods left a comment

Choose a reason for hiding this comment

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

Great, thanks very much!

@IsaacWoods IsaacWoods merged commit bc969d0 into rust-osdev:main Nov 11, 2024
4 checks passed
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.

2 participants