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

Add ELF flags for some profile-0.8 extensions #351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

palmer-dabbelt
Copy link
Contributor

These are essentially the same as Ztso: they're new requirements on existing instructions, with no way to probe them in the ISA. This treats them essentially the same way.

v0.8 of the profile specification defines Za64rs, which is defined as
"Reservation sets are contiguous, naturally aligned, and a maximum of 64
bytes."

Signed-off-by: Palmer Dabbelt <[email protected]>
v0.8 of the profile specification defines Ziccif, which is defined as
"Main memory regions with both the cacheability and coherence PMAs must
support instruction fetch, and any instruction fetches of naturally
aligned power-of-2 sizes up to min(ILEN,XLEN) (i.e., 32 bits for RVA20)
are atomic."

Signed-off-by: Palmer Dabbelt <[email protected]>
@jrtc27
Copy link
Collaborator

jrtc27 commented Nov 2, 2022

If they have ISA names then they should be in the ISA string encoded in the .riscv.attributes? EF_RISCV_TSO only exists because its definition in the spec predates encoding the ISA string in object files (ditto RVC which is both redundant and not expressive enough by itself for all the Zc* that now exist). Given the limited number of flag bits they should really only be used for fundamental things that have significant effects on the ABI, like the floating-point ABI, otherwise we'll quickly run out of bits.

@cmuellner
Copy link
Collaborator

Za64rs is just an additional restriction for Zicbo*. Any Zicbo* compatible code should also work with and without Za64rs. Why is a dedicated flag needed?

Similar situation for Ziccif, which also only reduces the possible implementation variants.

@palmer-dabbelt
Copy link
Contributor Author

Za64rs is just an additional restriction for Zicbo*. Any Zicbo* compatible code should also work with and without Za64rs. Why is a dedicated flag needed?

Similar situation for Ziccif, which also only reduces the possible implementation variants.

That's true for Ztso as well. This issue is that software that depends on these additional restrictions won't run correctly on implementations that don't follow them.

@cmuellner
Copy link
Collaborator

Za64rs is just an additional restriction for Zicbo*. Any Zicbo* compatible code should also work with and without Za64rs. Why is a dedicated flag needed?
Similar situation for Ziccif, which also only reduces the possible implementation variants.

That's true for Ztso as well. This issue is that software that depends on these additional restrictions won't run correctly on implementations that don't follow them.

Yes, SW built for za64rs won't run (correctly) on a system that has a cache block size different than 64 bytes.
But why is this different than for any other extension?
E.g. SW built for zba won't run on a system that does not implement zba.
A compatibility check of the binary's requirements against the host's implementation could catch both cases.

@palmer-dabbelt
Copy link
Contributor Author

Za64rs is just an additional restriction for Zicbo*. Any Zicbo* compatible code should also work with and without Za64rs. Why is a dedicated flag needed?
Similar situation for Ziccif, which also only reduces the possible implementation variants.

That's true for Ztso as well. This issue is that software that depends on these additional restrictions won't run correctly on implementations that don't follow them.

Yes, SW built for za64rs won't run (correctly) on a system that has a cache block size different than 64 bytes. But why is this different than for any other extension? E.g. SW built for zba won't run on a system that does not implement zba. A compatibility check of the binary's requirements against the host's implementation could catch both cases.

At least for the standard extensions aimed at portable systems we have non-conflicting instruction encodings and can thus detect these via traps (and even emulate them if necessary). We punted on that for Zfinx and friends under the rationale they're aimed at non-binary-portable systems, but the profile stuff seems pretty squarely aimed at binary-portable systems.

These don't trap, though, they just silently produce different results. There's no good way to detect that during execution, so our only option is to just prevent them from executing in the first place.

@palmer-dabbelt
Copy link
Contributor Author

If they have ISA names then they should be in the ISA string encoded in the .riscv.attributes? EF_RISCV_TSO only exists because its definition in the spec predates encoding the ISA string in object files (ditto RVC which is both redundant and not expressive enough by itself for all the Zc* that now exist). Given the limited number of flag bits they should really only be used for fundamental things that have significant effects on the ABI, like the floating-point ABI, otherwise we'll quickly run out of bits.

That seems reasonable. I'm happy to stash these somewhere other than the ELF header bits, Mark was just pretty adamant that the profile stuff should go there at the Cauldron (though it wasn't clear that makes any sense, there's not even enough bits to do this right now). Given how rapidly these show up we're going to run out of bits pretty fast.

IMO it's best to just have a specific attribute defining these? That way it's clear and not mixed in to the ISA string parsing which gets kind of hairy. I'd want to go hook attributes into the Linux and glibc ELF loaders first, though -- it seems manageable, but the early ELF loading stuff tends to be wacky and I might be missing something.

@aswaterman
Copy link
Contributor

At least for the standard extensions aimed at portable systems we have non-conflicting instruction encodings and can thus detect these via traps

I came here to say basically this... these extensions are a little different from others, because omitting most extensions will cause noisy failure (SIGILL) vs. silently doing the wrong thing.

But I agree using the ISA string is fine, as long as we're willing to validate these extensions in the ISA string anywhere we would've been willing to validate them using ELF flags.

@cmuellner
Copy link
Collaborator

Of course, I agree on the silent different result aspect.

Still, I don't think that illegal instructions should be gracefully handled without a purpose or that we should execute binaries that are built for extensions that are not explicitly listed as implemented by the execution environment.

E.g.: A system that decides to implement zba via trap-and-emulate can do so. But then it would list zba as an implemented extension on that system.
A system that does not list zba as an implemented extension should not execute any zba instructions at all (binaries that are built for zba should not be started).
The reason is, that a buggy zba implementation can be disabled by filtering zba from the list of implemented extensions. But if we don't do a compatibility check of binaries, there is no way to disable the buggy zba implementation.

So under the assumption that we have a compatibility check that prevents binaries from running in incompatible environments, dedicated flags for za64rs and ziccif should not be needed.

@palmer-dabbelt
Copy link
Contributor Author

At least for the standard extensions aimed at portable systems we have non-conflicting instruction encodings and can thus detect these via traps

I came here to say basically this... these extensions are a little different from others, because omitting most extensions will cause noisy failure (SIGILL) vs. silently doing the wrong thing.

But I agree using the ISA string is fine, as long as we're willing to validate these extensions in the ISA string anywhere we would've been willing to validate them using ELF flags.

You mean the ISA string in the attributes? That one's not really usable to base any behavior on, there's been so many different flavors that ended up in binaries it's kind of just meaningless.

I think there was some plan for adding another one, but we end up with the same pile of complexity that ended up driving the Linux hwprobe stuff towards the bitmap-based interface. That's not really meant to be statically encoded in binaries, but maybe the right answer there is to deal with whatever fallout comes from making it suitable for that? I haven't really thought about it, so not sure what (if any) problems we'd have.

@aswaterman
Copy link
Contributor

I'm agnostic as to the mechanism. I mostly meant to say that, if the reason we were going to add ELF flags was to validate that it's safe to load the ELFs, then whatever alternative to ELF flags we use should also be validated.

@palmer-dabbelt
Copy link
Contributor Author

I'm agnostic as to the mechanism. I mostly meant to say that, if the reason we were going to add ELF flags was to validate that it's safe to load the ELFs, then whatever alternative to ELF flags we use should also be validated.

Ya, I don't think there's much of an argument there, just whether we have a viable mechanism to do that already.

@ptomsich
Copy link
Collaborator

ptomsich commented Nov 3, 2022

I am a bit worried that we'll keep adding new fields instead of unifying our compatibility checks.

We currently already define one tag that pretty much means "this binary requires all of the following features present" and (consequently) we can use a subset relationship to test for compatibility.
For the example that triggered this PR, this still works, as long as there is only a zic64b and not other values permissible: we can encode "is a universal binary (e.g., uses dynamic discovery)" by {} subset-of {zic64b}.

However, if we want to have a binary that works exactly for zic64b and zic256b (let's say exactly two ifuncs provided), but not for zic128b, we'll be in trouble.
So I would instead propose a second tag that means "this binary supports all of the following features" and uses a superset relationship for compatibility. This tag could then be used universally for multiple features that have stringified names instead of having to add new flags (and logic) each time around.
To give an example again: this will address cases where a binary supports {zic64, zic256} and will run on environment that provide either {zic64} or {zic256}, but not {zic128b}.

Admittedly, processing these will require more cycles than dedicated flag fields, but it will reduce the number of mechanisms and provide a forward-compatible way to support the next parameterizable feature.
The additional overhead should be negligible compared to process creation overheads anyway.

@kito-cheng
Copy link
Collaborator

@ptomsich 's approach sound like more scalable.

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.

6 participants