registers: MXCSR #336

jarkkojs · 2022-01-21T08:06:03Z

Add MxCsr register derived from MxCsr type in enarx/xsave.

Provide two unit tests:

mxcsr_default(): Check that the constant matches the value read from
the CPU when the process has not yet written anything else to it.
mxcsr_write(): Check that the write operation does not corrupt values.

Closes: enarx/enarx#1000
Signed-off-by: Jarkko Sakkinen [email protected]

jarkkojs · 2022-01-21T12:04:30Z

I'll fix the reported issues!

Freax13 · 2022-01-21T19:53:45Z

It seems like there is a bug in the const-default dependency you added: https://github.com/rust-osdev/x86_64/runs/4901262388?check_suite_focus=true

jarkkojs · 2022-01-21T22:32:34Z

It seems like there is a bug in the const-default dependency you added: https://github.com/rust-osdev/x86_64/runs/4901262388?check_suite_focus=true

Hi, would it be more sustainable if I dropped that from this PR? It's not super critical as it is just integer that is sealed to be 0x1F80 (documented in Intel SDM).

jarkkojs · 2022-01-22T00:24:31Z

It seems like there is a bug in the const-default dependency you added: https://github.com/rust-osdev/x86_64/runs/4901262388?check_suite_focus=true

Hi, would it be more sustainable if I dropped that from this PR? It's not super critical as it is just integer that is sealed to be 0x1F80 (documented in Intel SDM).

I changed it as:

impl Default for MxCsr {
    /// Return the default MXCSR value at reset, as documented in Intel SDM volume 2A.
    #[inline]
    fn default() -> Self {
        MxCsr::from_bits_truncate(0x1F80)
    }
}

Obviously this means that #derive(ConstDefault) is not possible but it is not huge deal, i.e. you can implement ConstDefault whatever uses mxcsr.

jarkkojs · 2022-01-22T02:21:15Z

@npmccallum, @haraldh: This essentially the same type as in xsave except

        /// Toward negative infinity
        const ROUNDING_CONTROL_NEGATIVE = 1 << 13;
        /// Toward positive infinity
        ///
        /// Set the both control registers for "toward zero".
        const ROUNDING_CONTROL_POSITIVE = 1 << 14;

In the original type these were ROUNDING_CONTROL0 and ROUNDING_CONTROL1. I cannot remember on top of my head the exact SDM terms (might have been even the original) but these self-document the use and purpose.

Freax13

Thanks for this PR!

src/registers/mxcsr.rs

Freax13 · 2022-01-22T12:10:15Z

It seems like there is a bug in the const-default dependency you added: https://github.com/rust-osdev/x86_64/runs/4901262388?check_suite_focus=true

Hi, would it be more sustainable if I dropped that from this PR? It's not super critical as it is just integer that is sealed to be 0x1F80 (documented in Intel SDM).

Probably, yes. Perhaps we could add a DEFAULT variant in MxCsr's definition.

jarkkojs · 2022-01-25T14:59:19Z

OK, I think I resolved the existing review comments (but obviously can refine more if required).

Freax13

Still needs the proper external_asm wrappers, but other than that this looks good!

Freax13 · 2022-01-25T15:26:14Z

src/registers/mxcsr.rs

+        const ROUNDING_CONTROL_NEGATIVE = 1 << 13;
+        /// Toward positive infinity
+        ///
+        /// Set the both control registers for "toward zero".


We could make that a separate flag ROUNDING_CONTROL_ZERO that combines both ROUNDING_CONTROL_NEGATIVE and ROUNDING_CONTROL_POSITIVE.

I had that but did not stage that change for external asm part. I'll add the ROUNDING_CONTROL_ZERO.

OK I uploaded new version. My biggest struggle with QA is that I have no idea how to enforce cargo to compile "not inline_asm" flow instead of "inlin_asm" flow.

The CI job uses cargo build --no-default-features --features external_asm,instructions.

ROUNDING_CONTROL_ZERO seems to have got removed again.

src/registers/mxcsr.rs

src/asm/asm.s

npmccallum · 2022-01-25T21:47:27Z

@Freax13 Is the inline_asm feature actually needed here now that a subset of inline assembly is now stable? It seems reasonable to keep the feature for any asm uses that aren't actually stabilized yet. But for those that are already usable on stable, can't we just implement them without a feature gate?

Freax13 · 2022-01-25T21:57:04Z

Is the inline_asm feature actually needed here now that a subset of inline assembly is now stable?

Inline assembly hasn't landed on stable yet, it will be part of Rust 1.59 released at the end of next month. However once that happens, I agree that we could probably only use inline assembly in some (if not all?) cases.

jarkkojs · 2022-01-26T00:43:22Z

I sorted my problem out by writing a small script:

find -type f -print0 | xargs -0 sed -i 's/inline_asm/xxx_inline_asm/'
find -type f -print0 | xargs -0 sed -i 's/not(xxx_inline_asm)/external_asm/'
git checkout build.rs
git checkout src/lib.rs
cargo build --features external_asm
cargo test --features external_asm
git reset --hard HEAD

Now both code paths work!

jarkkojs · 2022-01-26T00:46:10Z

You can make amazingly stupid things if you are coding blind, i.e. no ways neither to compile nor run any tests on code you're working on :-) I can contribute the above script as a workaround, unless there is some smoother way to sort this out. No doubt that is ugly as hell...

jarkkojs · 2022-01-26T00:53:25Z

I proposed a solution: #337

I.e. you could then override x86_64_inline_asm even if its default value would be implicitly set based on inline_asm.

jarkkojs · 2022-01-26T04:41:55Z

I got the build commands together:

cargo build --no-default-features --features external_asm,instructions
cargo test --no-default-features --features external_asm,instructions -- --test-threads=1

With this I was able to run the tests sequentially through with external asm.

Freax13 · 2022-01-26T09:51:51Z

I got the build commands together:
cargo build --no-default-features --features external_asm,instructions
cargo test --no-default-features --features external_asm,instructions -- --test-threads=1
With this I was able to run the tests sequentially through with external asm.

This can be further simplified to just:

cargo test --no-default-features --features external_asm,instructions

cargo test also automatically builds the code before running the tests and I don't think there's reason to limit the threads for running the tests.

Freax13

A few more small changes required, we should be able to merge this soon :-).

The CI job that checks formatting is also still failing, you can run cargo fmt to fix this.

Freax13 · 2022-01-26T09:58:59Z

src/registers/mxcsr.rs

+        const ROUNDING_CONTROL_NEGATIVE = 1 << 13;
+        /// Toward positive infinity
+        ///
+        /// Set the both control registers for "toward zero".


ROUNDING_CONTROL_ZERO seems to have got removed again.

src/registers/mxcsr.rs

README.md

jarkkojs · 2022-01-26T12:45:57Z

Thanks, I'll fix these later on today.

jarkkojs · 2022-01-28T00:06:13Z

OK, let's see :-)

Freax13

One last small suggestion, then this should be good to go 🎉

src/registers/mxcsr.rs

Freax13 · 2022-01-28T07:27:47Z

One last small suggestion, then this should be good to go tada

Oh and you need to run cargo fmt.

jarkkojs · 2022-01-28T08:21:12Z

One last small suggestion, then this should be good to go tada

Oh and you need to run cargo fmt.

I'll keep this in mind, thank you.

jarkkojs · 2022-01-28T12:12:58Z

So... I guess it is now about right? I bundled everything into a single commit and:

Co-authored-by: Tom Dohrmann <[email protected]>
Signed-off-by: Jarkko Sakkinen <[email protected]>

Add MxCsr register derived from MxCsr type in enarx/xsave. Provide two unit tests: - mxcsr_default(): Check that the constant matches the value read from the CPU when the process has not yet written anything else to it. - mxcsr_write(): Check that the write operation does not corrupt values. Co-authored-by: Tom Dohrmann <[email protected]> Signed-off-by: Jarkko Sakkinen <[email protected]>

Freax13 · 2022-01-28T12:21:13Z

So... I guess it is now about right?

Yes. Thank you for your contribution!

jarkkojs mentioned this pull request Jan 22, 2022

Add the mxcsr register to the x86_64 crate enarx/enarx#1000

Closed

Freax13 requested changes Jan 22, 2022

View reviewed changes

Freax13 requested changes Jan 25, 2022

View reviewed changes

src/asm/asm.s Outdated Show resolved Hide resolved

src/asm/asm.s Outdated Show resolved Hide resolved

Freax13 requested changes Jan 26, 2022

View reviewed changes

Freax13 requested changes Jan 28, 2022

View reviewed changes

src/registers/mxcsr.rs Outdated Show resolved Hide resolved

Freax13 enabled auto-merge January 28, 2022 12:19

Freax13 approved these changes Jan 28, 2022

View reviewed changes

Freax13 merged commit e730686 into rust-osdev:master Jan 28, 2022

phil-opp added a commit that referenced this pull request Feb 3, 2022

Update changelog for #334 and #336

204e6e3

phil-opp mentioned this pull request Feb 3, 2022

Bump version to 0.14.8 #339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

registers: MXCSR #336

registers: MXCSR #336

jarkkojs commented Jan 21, 2022 •

edited

Loading

jarkkojs commented Jan 21, 2022

Freax13 commented Jan 21, 2022

jarkkojs commented Jan 21, 2022

jarkkojs commented Jan 22, 2022 •

edited

Loading

jarkkojs commented Jan 22, 2022

Freax13 left a comment

Freax13 commented Jan 22, 2022

jarkkojs commented Jan 25, 2022

Freax13 left a comment

Freax13 Jan 25, 2022

jarkkojs Jan 25, 2022 •

edited

Loading

jarkkojs Jan 25, 2022

Freax13 Jan 25, 2022

Freax13 Jan 26, 2022

npmccallum commented Jan 25, 2022

Freax13 commented Jan 25, 2022

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 26, 2022 •

edited

Loading

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 26, 2022 •

edited

Loading

Freax13 commented Jan 26, 2022

Freax13 left a comment

Freax13 Jan 26, 2022

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 28, 2022

Freax13 left a comment

Freax13 commented Jan 28, 2022

jarkkojs commented Jan 28, 2022

jarkkojs commented Jan 28, 2022 •

edited

Loading

Freax13 commented Jan 28, 2022

registers: MXCSR #336

registers: MXCSR #336

Conversation

jarkkojs commented Jan 21, 2022 • edited Loading

jarkkojs commented Jan 21, 2022

Freax13 commented Jan 21, 2022

jarkkojs commented Jan 21, 2022

jarkkojs commented Jan 22, 2022 • edited Loading

jarkkojs commented Jan 22, 2022

Freax13 left a comment

Choose a reason for hiding this comment

Freax13 commented Jan 22, 2022

jarkkojs commented Jan 25, 2022

Freax13 left a comment

Choose a reason for hiding this comment

Freax13 Jan 25, 2022

Choose a reason for hiding this comment

jarkkojs Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

jarkkojs Jan 25, 2022

Choose a reason for hiding this comment

Freax13 Jan 25, 2022

Choose a reason for hiding this comment

Freax13 Jan 26, 2022

Choose a reason for hiding this comment

npmccallum commented Jan 25, 2022

Freax13 commented Jan 25, 2022

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 26, 2022 • edited Loading

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 26, 2022 • edited Loading

Freax13 commented Jan 26, 2022

Freax13 left a comment

Choose a reason for hiding this comment

Freax13 Jan 26, 2022

Choose a reason for hiding this comment

jarkkojs commented Jan 26, 2022

jarkkojs commented Jan 28, 2022

Freax13 left a comment

Choose a reason for hiding this comment

Freax13 commented Jan 28, 2022

jarkkojs commented Jan 28, 2022

jarkkojs commented Jan 28, 2022 • edited Loading

Freax13 commented Jan 28, 2022

jarkkojs commented Jan 21, 2022 •

edited

Loading

jarkkojs commented Jan 22, 2022 •

edited

Loading

jarkkojs Jan 25, 2022 •

edited

Loading

jarkkojs commented Jan 26, 2022 •

edited

Loading

jarkkojs commented Jan 26, 2022 •

edited

Loading

jarkkojs commented Jan 28, 2022 •

edited

Loading