sbi-spec: `CounterMask` missing from `binary` module

[rmsyn]

When using the rustsbi@v0.4.0 from crates.io, the rustsbi::spec::binary::CounterMask struct is missing.

The same problem exists when using sbi-spec@v0.0.7 directly.

However, when using the release commit locally, the struct is present.

[luojia65]

I'll release sbi-spec 0.0.8 including CounterMask, then RustSBI 0.4.1 including sbi-spec 0.0.8. Before that, we are updating sbi-rt crate as well according to CounterMask changes. Thanks!

BTW, I added missing changelog for CounterMask structure in this commit: https://github.com/rustsbi/rustsbi/commit/232b3d7b3036eeeb0d7bdb353e7d44f93ca71fb3 . Next time we should remember to write a changelog when we contribute, or it's easy to forget what have the community done on next release.

[luojia65]

Current CounterMask and HartMark structure and function signatures:

pub struct HartMask { ... }

impl HartMask {
    pub const IGNORE_MASK: usize = ...;
    pub const fn from_mask_base(counter_idx_mask: usize, counter_idx_base: usize) -> Self  { ... }
    pub const fn ignore_mask(&self) -> usize  { ... }
    pub const fn into_inner(self) -> (usize, usize)  { ... }
    pub const fn has_bit(self, counter: usize) -> bool { ... }
}

pub struct CounterMask { ... }

impl CounterMask {
    pub const IGNORE_MASK: usize = ...;
    pub const fn from_mask_base(counter_idx_mask: usize, counter_idx_base: usize) -> Self  { ... }
    pub const fn ignore_mask(&self) -> usize  { ... }
    pub const fn into_inner(self) -> (usize, usize)  { ... }
    pub const fn has_bit(self, counter: usize) -> bool { ... }
}

From RustSBI submodules and ecosystem projects, the from_mask_base, into_inner and has_bit functions have practical usages (on HartMask). However, I cannot find usages outside of sbi-spec crate itself for IGNORE_MASK const and ignore_mask function. Can we consider it internal implementation and do not expose it to public?

We'd modify the functions signatures into:

pub struct HartMask { ... }

impl HartMask {
    pub const fn from_mask_base(counter_idx_mask: usize, counter_idx_base: usize) -> Self { ... }
    pub const fn into_inner(self) -> (usize, usize)  { ... }
    pub const fn has_bit(self, counter: usize) -> bool { ... }

    const IGNORE_MASK: usize = ...; // internal implementation
    const fn ignore_mask(&self) -> usize { ... } // internal implementation
}

pub struct CounterMask { ... }

impl CounterMask {
    pub const fn from_mask_base(counter_idx_mask: usize, counter_idx_base: usize) -> Self { ... }
    pub const fn into_inner(self) -> (usize, usize)  { ... }
    pub const fn has_bit(self, counter: usize) -> bool { ... }

    const IGNORE_MASK: usize = ...; // internal implementation
    const fn ignore_mask(&self) -> usize { ... } // internal implementation
}

What's your opinion? Thanks! @rmsyn

[rmsyn]

BTW, I added missing changelog for CounterMask structure in this commit: https://github.com/rustsbi/rustsbi/commit/232b3d7b3036eeeb0d7bdb353e7d44f93ca71fb3 . Next time we should remember to write a changelog when we contribute, or it's easy to forget what have the community done on next release.

Apologies, I'll do my best to remember adding a changelog entry on my next contribution. Maybe we can add it to the CI checks?

However, I cannot find usages outside of sbi-spec crate itself for IGNORE_MASK const and ignore_mask function. What's your opinion?

I think leaving these functions public is a good idea. External users may want to check the ignore mask, e.g. when debugging. It also doesn't really have any potential for misuse.

We won't really know if external users want the API until it's been out there for a while.

If you really think it should be internal-only, I have no strong objection.

[luojia65]

@rmsyn: The CI check for changelogs has been added in this pull request: https://github.com/rustsbi/rustsbi/pull/78. Thanks for your suggestion!

For IGNORE_MASK constant value and ignore_mask function, I decide to keep it as-is in the upcoming sbi-spec version 0.0.8 to allow us to watch out how the community uses this API. As our sbi-spec project is still in early stages, we can have further discussions on whether to keep it or not after a period of time.

[luojia65]

@rmsyn: sbi-spec version 0.0.8-rc.1 is released (crates.io, documents). Please check if the version 0.0.8-rc.1 meets our demands (e.g. is the functions in CounterMask correct?); if it's correct, I'll formally release the 0.0.8 version of sbi-spec.

[luojia65]

API of sbi-rt is changed to use the CounterMask structure in sbi-spec, the new version of sbi-rt will be released soon. But for rustsbi crate, it would be a breaking change for changing definition of functions in the Pmu trait, we keep it as two usize's, and the API change to CounterMask will be applied in RustSBI 0.5.0 (Ref: https://github.com/rustsbi/rustsbi/commit/62ab2e498ca66cdf75ce049c9dbc2f1862874553).

[rmsyn]

@rmsyn: sbi-spec version 0.0.8-rc.1 is released (crates.io, documents). Please check if the version 0.0.8-rc.1 meets our demands (e.g. is the functions in CounterMask correct?); if it's correct, I'll formally release the 0.0.8 version of sbi-spec.

I'll pull down the new version, and test things out. Thank you for getting a new version up so quickly :)

Edit: everything looks good to me.

But for rustsbi crate, it would be a breaking change for changing definition of functions in the Pmu trait, we keep it as two usize's, and the API change to CounterMask will be applied in RustSBI 0.5.0 (Ref: https://github.com/rustsbi/rustsbi/commit/62ab2e498ca66cdf75ce049c9dbc2f1862874553).

This makes sense to me, and we can wait for the breaking change since CounterMask can be converted back to raw usize for the Pmu interface.