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

SSE and FPU support #410

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

SSE and FPU support #410

wants to merge 6 commits into from

Conversation

vsntk18
Copy link
Collaborator

@vsntk18 vsntk18 commented Jul 15, 2024

This PR entails following changes:

  1. Enable the usage of SSE/FPU instructions in SVSM.
  2. xstate management while switching between tasks.
  3. A couple of unit tests check the sse instruction usage and proper xstate management.

I will submit the related exception handling in another PR.

kernel/src/cpu/sse.rs Outdated Show resolved Hide resolved
kernel/src/cpu/sse.rs Outdated Show resolved Hide resolved
kernel/src/task/tasks.rs Outdated Show resolved Hide resolved
kernel/src/mm/vm/mapping/xsave_area.rs Outdated Show resolved Hide resolved
kernel/src/cpu/sse.rs Outdated Show resolved Hide resolved
Copy link
Member

@joergroedel joergroedel left a comment

Choose a reason for hiding this comment

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

This goes into the right direction, thanks for the work! I marked some desired changes inline.

kernel/src/task/schedule.rs Outdated Show resolved Hide resolved
kernel/src/cpu/sse.rs Outdated Show resolved Hide resolved
kernel/src/cpu/sse.rs Outdated Show resolved Hide resolved
kernel/src/mm/vm/mapping/xsave_area.rs Outdated Show resolved Hide resolved
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Aug 22, 2024

Addressed the review comments.

Copy link
Member

@joergroedel joergroedel 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 updating the PR! I left a couple of review comments.


// Check if at least SSE1 is supported
fn legacy_sse_supported() -> bool {
let Some(res) = cpuid_table_raw(1, 0, 0, 0) else {
Copy link
Member

Choose a reason for hiding this comment

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

This code needs to use the cpuid instruction directly to be compatible with the native and TD platform targets. I just realized there is no wrapper yet for cpuid, so please add one and use it for any FPU-related CPUID queries. This also affects other places in this PR.

Copy link
Collaborator Author

@vsntk18 vsntk18 Aug 28, 2024

Choose a reason for hiding this comment

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

How about CpuidResult::get() which is also being used in places like kernel/src/platform/native.rs?

Comment on lines 71 to 65
if extended_sse_supported() && xsave_supported() {
cr4_xsave_enable();
xcr0_set();
} else {
panic!("XSAVE is not supported by this hardware");
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if extended_sse_supported() && xsave_supported() {
cr4_xsave_enable();
xcr0_set();
} else {
panic!("XSAVE is not supported by this hardware");
}
if !extended_sse_supported() {
panic!("Extended SSE is not supported by this hardware");
}
if !xsave_supported() {
panic!("XSAVE is not supported by this hardware");
}
cr4_xsave_enable();
xcr0_set();

Make the panic messages a bit more concise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, truncated them a bit.

Comment on lines 303 to 306
if !prev.is_null() {
sse_save_context(u64::from((*prev).xsa.vaddr()));
}

Copy link
Member

Choose a reason for hiding this comment

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

This can happen in schedule().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to schedule()

Comment on lines 371 to 374
let cur = current_task();
unsafe {
sse_restore_context(u64::from(cur.xsa.vaddr()));
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved into the if let Some((current, next)) = work { branch. Otherwise the code will load an old FPU context when no context switch is happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, right! good point.

Comment on lines 530 to 532
unsafe {
sse_restore_context(xsa_addr);
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be in setup_new_task().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

Comment on lines +187 to +193
let xsa = Self::allocate_xsave_area();
let xsa_addr = u64::from(xsa.vaddr()) as usize;
let (stack, raw_bounds, rsp_offset) = Self::allocate_ktask_stack(cpu, entry, xsa_addr)?;
Copy link
Member

Choose a reason for hiding this comment

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

The same needs to happen for user tasks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added to user tasks as well.

@joergroedel joergroedel added the wait-for-update PR is waiting to be updated to address review comments label Aug 23, 2024
@joergroedel joergroedel self-assigned this Aug 23, 2024
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Aug 30, 2024

Addressed review comments.

Some cpuid functions require a non-zero value of xcr0 to be
passed.

Signed-off-by: Vasant Karasulli <[email protected]>
Check for the available SSE/FPU related features and
initialize them.

Signed-off-by: Vasant Karasulli <[email protected]>
Reserve xsave area for tasks to manage xstate.

Signed-off-by: Vasant Karasulli <[email protected]>
use xsaveopt/xrstor instructions to save and restore task context.

Signed-off-by: Vasant Karasulli <[email protected]>
These in-svsm tests check if tasks can use some sse/fpu instructions
and xstate is saved/restored properly during task switches.

Signed-off-by: Vasant Karasulli <[email protected]>
This is a temporary solution until we have smart pointer.
This also gives a chance to move xsave/xrestor functionality
outside the assembly block.

Signed-off-by: Vasant Karasulli <[email protected]>
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Oct 21, 2024

Rebase done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait-for-update PR is waiting to be updated to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants