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

Large globals sections are incorrectly set up #230

Open
davidchisnall opened this issue May 24, 2024 · 4 comments · May be fixed by #236
Open

Large globals sections are incorrectly set up #230

davidchisnall opened this issue May 24, 2024 · 4 comments · May be fixed by #236
Assignees

Comments

@davidchisnall
Copy link
Collaborator

CHERIoT-Platform/llvm-project#26 reports that very large objects are not working.

This doesn't appear to be a compiler bug. Extending the test case from that example, we get a capability with the correct bounds, but cgp for the compartment appears to be being created with a zero length and so deriving a capability from that gives an untagged capability. This is presumably therefore a loader bug.

@davidchisnall davidchisnall self-assigned this May 24, 2024
@davidchisnall
Copy link
Collaborator Author

The first issue here is that, currently, the maximum size of a compartment's globals is 64 KiB. Since most devices we're expecting will have much less than 1 MiB of RAM, this isn't really a limitation for any hardware that we expect in the short term. Changing this would increase the size of compartment headers. Most compartments have significantly less than 4 KiB of globals and a lot have 4-20 B, so I wouldn't want to require very strong alignment (storing the size as a shifted value is a simple change).

The test case is an interesting combination of things:

  • The globals section must be aligned to be representable.
  • It is just under 64 KiB, but rounding it up for representability makes it 64 KiB.
  • 64 KiB overflows the size field in the header and so the size is truncated to 0.
  • The loader happily creates a 0-byte CGP.
  • Ooops.

@sleffler
Copy link
Contributor

sleffler commented May 24, 2024 via email

@davidchisnall
Copy link
Collaborator Author

We should be able to support it, but my initial attempt missed something and does not work. I have not yet discovered why not.

davidchisnall added a commit that referenced this issue May 28, 2024
We store the size of globals as a shifted value, which extends the
maximum size of globals in a compartment from 64 KiB to 256 KiB.

This adds two new tests.  One is added to the misc test and just checks
that we correctly handle globals that are larger than the range of a
csetbounds with an immediate offset.

The second checks that much larger globals work.  This test is disabled
because it uses so much RAM that we don't have enough for the allocator
tests.

At some point, we should refactor the test suite to allow subsets of the
test suite to be built.

Add test for large objects.

Fixes #230
@davidchisnall davidchisnall linked a pull request May 28, 2024 that will close this issue
@davidchisnall
Copy link
Collaborator Author

#236 enables 256 KiB of globals per compartment. This is a fairly non-invasive change (I missed the software revoker needing a special case on Friday. Relaxing over the weekend was the right call instead of trying to fix it then).

We could potentially support larger things with a different header format, if necessary, but I don't think we're going to see chips with enough SRAM for this to be a problem in the near future.

davidchisnall added a commit that referenced this issue May 28, 2024
We store the size of globals as a shifted value, which extends the
maximum size of globals in a compartment from 64 KiB to 256 KiB.

This adds two new tests.  One is added to the misc test and just checks
that we correctly handle globals that are larger than the range of a
csetbounds with an immediate offset.

The second checks that much larger globals work.  This test is disabled
because it uses so much RAM that we don't have enough for the allocator
tests.

At some point, we should refactor the test suite to allow subsets of the
test suite to be built.

Add test for large objects.

Fixes #230
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 a pull request may close this issue.

2 participants