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

verification for IOAPIC PR seL4/seL4#896 #753

Merged
merged 5 commits into from
Jun 12, 2024
Merged

verification for IOAPIC PR seL4/seL4#896 #753

merged 5 commits into from
Jun 12, 2024

Conversation

lsf37
Copy link
Member

@lsf37 lsf37 commented May 7, 2024

seL4/seL4#896 changes IOAPIC-related decoding slightly and requires a few new small invariants on the Haskell level.

Test with: seL4/seL4#896

@lsf37 lsf37 self-assigned this May 7, 2024
@lsf37 lsf37 added the seL4-PR requires merging a corresponding seL4 pull request label May 7, 2024
@lsf37 lsf37 marked this pull request as draft May 7, 2024 19:41
@lsf37 lsf37 force-pushed the ioapic branch 3 times, most recently from 0d9a0a2 to ad180ed Compare May 7, 2024 19:55
@lsf37 lsf37 marked this pull request as ready for review May 7, 2024 20:00
@lsf37 lsf37 requested review from corlewis and Xaphiosis May 7, 2024 20:01
Copy link
Member

@corlewis corlewis left a comment

Choose a reason for hiding this comment

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

Looks good to me (assuming that the Word_Lib failure is trivial)!

Did the new invariant in valid_arch_state' really not need any proof updates to show that it's maintained? I'm actually pretty impressed if so, even with it only depending on such specific parts of the state, I would have thought that some of the higher level functions would need at least some manual steps to show it.

proof/crefine/X64/ADT_C.thy Outdated Show resolved Hide resolved
proof/crefine/X64/Interrupt_C.thy Show resolved Hide resolved
@lsf37
Copy link
Member Author

lsf37 commented May 8, 2024

Looks good to me (assuming that the Word_Lib failure is trivial)!

It should be, I moved the lemma manually without checking dependencies, and that's what I get for my laziness :-)

Did the new invariant in valid_arch_state' really not need any proof updates to show that it's maintained? I'm actually pretty impressed if so, even with it only depending on such specific parts of the state, I would have thought that some of the higher level functions would need at least some manual steps to show it.

I was also pretty impressed. AFAICS it works, because I managed to export the state dependency via abbreviation and therefore the lifting rule for valid_arch_state' still works as before and anything that unfolds valid_arch_state' will simplify out the new parts trivially. It's nice to see those ideas actually fully working every once in a while.

Copy link
Member

@Xaphiosis Xaphiosis left a comment

Choose a reason for hiding this comment

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

Cool, another long-standing seL4 PR closed.

@lsf37 lsf37 force-pushed the ioapic branch 2 times, most recently from 5175d01 to 4cae0a9 Compare May 8, 2024 19:53
@lsf37 lsf37 requested review from corlewis and Xaphiosis May 8, 2024 19:58
@lsf37
Copy link
Member Author

lsf37 commented May 8, 2024

The Word_Lib failure has drawn in a whole slate of word lemma movement from CRefine, so it's probably worth looking at this again before I merge.

Copy link
Member

@corlewis corlewis left a comment

Choose a reason for hiding this comment

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

I'm confused by the remaining failures but assuming that it's mostly unrelated then it still looks good to me.

proof/crefine/Move_C.thy Show resolved Hide resolved
proof/crefine/Move_C.thy Show resolved Hide resolved
@lsf37
Copy link
Member Author

lsf37 commented May 9, 2024

I'm confused by the remaining failures but assuming that it's mostly unrelated then it still looks good to me.

They were because the seL4 PR was not rebased yet and didn't have some of the hyp related changes that happened in the meantime. I've pushed a rebased version of the PR and restarted the tests.

Copy link
Member

@Xaphiosis Xaphiosis left a comment

Choose a reason for hiding this comment

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

Approved v2, very nice, and I appreciate the emptying of Move_C.
Super minor nitpick:
x64 ainvs+refine: ioapic_nirqs proof updates
but
x64 crefine: ioapic_nirqs updates

Separate IOAPICs can have different max numbers of IRQs. This PR adds
the logic of seL4/seL4#896 for that to the specs.

Signed-off-by: Gerwin Klein <[email protected]>
Slighly weaken arch_irq_control_inv_valid -- the condition
`pin < ioapicIRQLines` was unused and would now require a new state
invariant on x64_ioapic_nirqs to prove.

Signed-off-by: Gerwin Klein <[email protected]>
Place constraints on the new IOPAPIC state components x64KSNumIOAPICs
and x64KSIOAPICnIRQs to support CRefine arithmetic.

Signed-off-by: Gerwin Klein <[email protected]>
@lsf37 lsf37 merged commit cb1845e into master Jun 12, 2024
14 checks passed
@lsf37 lsf37 deleted the ioapic branch June 12, 2024 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
seL4-PR requires merging a corresponding seL4 pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants