Skip to content

Conversation

@IsaacWoods
Copy link
Member

This adds support for tags for reading the address of the RSDP, a structure used in ACPI. There are two different structs, one for ACPI Version 1.0, the other for subsequent versions. It also adds tests to the grub2 test.

@IsaacWoods IsaacWoods mentioned this pull request May 3, 2018
22 tasks
@xacrimon
Copy link
Contributor

xacrimon commented May 5, 2018

I think it looks good but i would like some insight from another maintainer aswell before merging. Could anyone of you take a look? @phil-opp and @le-jzr

@xacrimon xacrimon requested review from le-jzr, phil-opp and xacrimon and removed request for le-jzr May 5, 2018 08:03
Copy link
Member

@phil-opp phil-opp 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 to me, it just seems like there's one field missing for RsdpV2Tag.

_rsdt_address: u32,
length: u32,
xsdt_address: u64, // This is the PHYSICAL address of the XSDT
_reserved: [u8; 3],
Copy link
Member

Choose a reason for hiding this comment

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

There should be an extended_checksum field before _reserved, shouldn't it?

@phil-opp phil-opp mentioned this pull request May 5, 2018
@xacrimon
Copy link
Contributor

xacrimon commented May 5, 2018

Yeah I think there should

@IsaacWoods
Copy link
Member Author

🤦 fixed. Would you like this squashed, or is it fine as-is?

Copy link
Contributor

@xacrimon xacrimon 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 it'll be fine without a squash.

@xacrimon
Copy link
Contributor

xacrimon commented May 5, 2018

Ready to merge Isaac

@IsaacWoods
Copy link
Member Author

Awesome, but I don't have write access, so could someone who does do the actual merge?

@xacrimon
Copy link
Contributor

xacrimon commented May 5, 2018

@phil-opp Could you give Isaac write access? I'll be doing this merge.

@xacrimon xacrimon merged commit 733d1bd into rust-osdev:master May 5, 2018
@xacrimon
Copy link
Contributor

xacrimon commented May 5, 2018

Merged

@phil-opp
Copy link
Member

phil-opp commented May 5, 2018

Could you give Isaac write access? I'll be doing this merge.

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants