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

Remove IO ports from non-X64 arches, move valid_ioports into valid_arch_state on X64 #829

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

Xaphiosis
Copy link
Member

@Xaphiosis Xaphiosis commented Nov 21, 2024

Commits are non-overlapping and can be reviewed on a commit-by-commit basis. It'll probably need more squashing than what I have now, advice welcome. 🦆🦆🦆

It was always a bit silly that valid_ioports existed on architectures with no IO ports. After discussing with Corey, the idea of moving it into valid_arch_state on X64 and retiring it on other arches came up. This worked, but resulted in changes to the arch-split interface in AInvs due to valid_arch_state now potentially depending on caps.

In Refine on X64, I did the same change so that during arch-split we won't need to see IO ports mentioned on arches that don't have them. It also required a fair bit of mangling to get through the extra valid_arch_state' dependency on caps. The hope is that maybe we can get rid of valid_ioports' altogether via assert magic. That will be the next experiment.

Edit: experiment was successful, no X64 Refine+CRefine changes needed in this PR anymore

For Access+Infoflow, see commit message.

@Xaphiosis Xaphiosis requested review from lsf37 and corlewis November 21, 2024 19:05
@Xaphiosis Xaphiosis added the arch-split splitting proofs into generic and architecture dependent label Nov 21, 2024
proof/invariant-abstract/Detype_AI.thy Outdated Show resolved Hide resolved
proof/invariant-abstract/IpcCancel_AI.thy Outdated Show resolved Hide resolved
Comment on lines +370 to +371
apply (timeit \<open>auto elim!: delta_sym_refs pred_tcb_weaken_strongerE
simp: obj_at_def is_ep_def2 idle_not_queued refs_in_tcb_bound_refs
dest: idle_no_refs
split: if_split_asm\<close>)[2] (* slow: ~100s *)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the comment, but do we want to keep the timeit command in?

Copy link
Member Author

Choose a reason for hiding this comment

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

The last two times I've left the timeit in there for these very long commands, the reasoning being that when someone is doing changes in future, there's immediate feedback on whether the performance changed significantly (i.e. you get that info even if you already built all the proofs, don't have to re-trigger it and lose progress). You're right, the benefit is a bit marginal. What do you think, keep or remove?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I do like that benefit now that you've pointed it out. I reckon might as well keep it, it shouldn't be too confusing if we stumble across it in the future.

That does remind me though, I wish that the timing panel in Isabelle had some way of showing which entry corresponds to the command the cursor is currently on. If it had that then you could easily get the performance info without the timeit.

Copy link
Member Author

Choose a reason for hiding this comment

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

timeit will work even when defer proofs is on (although it will only show output at the end of the proof block), but the timing panel will only show timing for the entire proof block/lemma, so I will leave it for now

Copy link
Member

Choose a reason for hiding this comment

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

My vote is also for leaving the timeit in. Longer-term it would probably be nice to speed this proof up a bit, but that's out of scope for this PR.

proof/invariant-abstract/Ipc_AI.thy Outdated Show resolved Hide resolved
proof/refine/X64/Detype_R.thy Outdated Show resolved Hide resolved
proof/refine/X64/Ipc_R.thy Outdated Show resolved Hide resolved
proof/refine/X64/Ipc_R.thy Outdated Show resolved Hide resolved
proof/refine/X64/Retype_R.thy Outdated Show resolved Hide resolved
proof/refine/X64/Schedule_R.thy Outdated Show resolved Hide resolved
proof/access-control/CNode_AC.thy Show resolved Hide resolved
proof/infoflow/ARM/ArchRetype_IF.thy Outdated Show resolved Hide resolved
@Xaphiosis
Copy link
Member Author

I removed the X64 Refine+CRefine changes from this PR because I was able to get rid of valid_ioports' in the other PR, and then the rest magically work. This means @corlewis's comments on that are now outdated and I'll resolve them with N/A.
This is now good to go for merging.

@Xaphiosis
Copy link
Member Author

I've not updated copyrights on any of the files. Looking at the sizes of the diffs, candidates for this are Ipc_AI and ArchIpc_AI (on all arches). Thoughts?

@lsf37
Copy link
Member

lsf37 commented Dec 2, 2024

I've not updated copyrights on any of the files. Looking at the sizes of the diffs, candidates for this are Ipc_AI and ArchIpc_AI (on all arches). Thoughts?

I don't have strong feelings on this one. The change is significant enough to add it, but it's not like it's extremely important to do so.

Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

Cool. Would have feared much worse impact for this, and it turned out really quite nice.

Having valid_ioports as a part of non-arch predicates caused its leakage
into all other architectures, which do not have IO ports.
By folding valid_ioports into valid_arch_state, the latter becomes
a predicate affected by caps, causing changes in the generic interface
in order to avoid any mentions of IO ports in non-arch-specific proofs.

Signed-off-by: Rafal Kolanski <[email protected]>
…erface

Previously valid_ioports was present on all arches. With it being
migrated to valid_arch_state on X64, the interface changed to include
valid_arch_state depending on caps. Update proofs to conform to the new
interface, removing all mentions of IO ports from non-X64 arches.

Signed-off-by: Rafal Kolanski <[email protected]>
Currently this means ARM and RISCV64. Already, these two architectures
did not consider IO ports, so were not compatible with X64, and neither
was an X64 integrity proof planned. This change leans on that by
requalifying/assuming some extra lemmas which pretend that
valid_arch_state not relying on caps is a generic concept.

Signed-off-by: Rafal Kolanski <[email protected]>
@Xaphiosis Xaphiosis merged commit fbec9c7 into seL4:master Dec 4, 2024
9 of 14 checks passed
@Xaphiosis Xaphiosis deleted the hide-ioports branch December 4, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-split splitting proofs into generic and architecture dependent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants