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

numdomains: make proofs work with any number of domains #410

Merged
merged 7 commits into from
Dec 22, 2021
Merged

Conversation

Xaphiosis
Copy link
Member

Test with seL4/seL4#713

This is still limited to 256 domains because domain type is word 8 and quite hardcoded in the abstract specs/proofs. Please see commit messages for detailed observations on how this was accomplished, and caveats.

The default regression test will check with numDomains=16, and I've been running tests with numDomains=1 which was the most difficult to deal with due to "clever" simplification of edge cases spat out via ArrayGuards.

@lsf37
Copy link
Member

lsf37 commented Dec 21, 2021

Not sure what ails the link checker today, but it has been failing on all repos this morning, so is unrelated to this PR. Some "503 Service unavailable" thing at docker.

The theory linter fail is anticipated, the interactive commands are for demonstrating the features of the command.

@lsf37
Copy link
Member

lsf37 commented Dec 21, 2021

Not sure what ails the link checker today, but it has been failing on all repos this morning, so is unrelated to this PR. Some "503 Service unavailable" thing at docker.

There is now an actual link check failure, which is unrelated. Fix is in #412

@Xaphiosis
Copy link
Member Author

Found a hiccup when numDomains=2 and simplification of 0 < 1, force-pushed the change. Running tests with numDomains=17 as well to make sure I haven't missed anything.

@Xaphiosis
Copy link
Member Author

In case this looks PR looks quiet, @lsf37 reviewed this twice already

lib/Value_Type.thy Outdated Show resolved Hide resolved
Comment on lines 113 to 118
(* Remove the demo definitions: *)
hide_const num_domains
hide_fact num_domains_def

hide_const num_prio
hide_fact num_prio_def
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this completely removes the definitions. From memory, it makes them very hard to access but they can still turn up unexpectedly in find_theorems and find_consts.

It's probably unlikely to cause any actual issues, but my preference is to put these examples in a separate file that isn't imported.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had suspected something like this. Thoughts, @lsf37 ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, @corlewis is right, we should put this in a separate theory. I had hoped to be able to pull the definitions into the experiment block, but while the definitions do work there, the code generator seems to refuse to generate anything for them inside the local context, which means that eval won't work.

In more detail: eval complains that there are not code equations for num_domains in the example if you move the definition into the local context, but if you try and explicit export_code it will already complain that there is no constant num_domains. This does make some sense, because if there are assumptions or fixed variables in the context the generated code will not have the right semantics.

All this means this is a limitation of the value_type command in general: it can't use terms on the rhs that are only defined in a local context. If we ever need that, we'll need to use something else to compute the numeral.

I'll add a comment to that effect and move the example into a separate theory.

Comment on lines +69 to +73
(* unless we hit dec_domain_time we know ?dtnot0 holds on the state, so clean up the
postcondition once we hit thread_set_time_slice *)
hoare_post_imp[where Q="\<lambda>_. ?dtnot0" and R="\<lambda>_ s. domain_time s = 0 \<longrightarrow> X s"
and a="thread_set_time_slice t ts" for X t ts]
hoare_drop_imp[where f="ethread_get t f" for t f])
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice way of handling this (and a very useful comment)!

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a whitespace change, reindent or moving things around, don't think I did anything this round on it, though I did possibly write the original.

@@ -1502,7 +1502,6 @@ locale valid_initial_state_noenabled = invariant_over_ADT_if + (* FIXME: arch_sp
assumes scheduler_action_s0_internal: "scheduler_action s0_internal = resume_cur_thread"
assumes ct_running_or_ct_idle_s0_internal: "ct_running s0_internal \<or> ct_idle s0_internal"
assumes domain_time_s0_internal: "domain_time s0_internal > 0"
assumes num_domains_sanity: "num_domains > 1"
Copy link
Member

Choose a reason for hiding this comment

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

Wow, great job removing this! I thought that there being multiple domains was a pretty important condition for infoflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is if you want an interesting state, you'll see when you get to InfoflowC changes to ExampleValidStateH

Comment on lines +1590 to +1591
lemma invs_no_cicd_ksCurDomain_maxDomain':
"invs_no_cicd' s \<Longrightarrow> ksCurDomain s \<le> maxDomain"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: move this to Invariants_H and make it elim! like the similar rules (same for other architectures)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate which similar rules you are thinking of? I'm not seeing any invs_no_cicd' derivations in Invariants_H. Happy to put it there, but would like more clarity of the idea. invs_no_cicd'_queues also lives in Schedule_R and probably was the same low level of inspiration.

Copy link
Member

Choose a reason for hiding this comment

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

I was considering it as similar to rules like invs_valid_objs'. There's no big problem with leaving it as is, especially if invs_no_cicd'_queues is already there as well, but everything else being equal I do like having this sort of lemma early in the dependencies.

proof/refine/ARM/Schedule_R.thy Outdated Show resolved Hide resolved
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.

This all looks great to me. It's really nice to see this progress towards more automatically supporting different configurations!

With regards to the crefine work, I mainly looked at the ARM changes along with skimming the RISCV64 files. I assume that the others are similar.

Comment on lines 509 to 512
lemma num_tcb_queues_calculation: "num_tcb_queues = numDomains * numPriorities"
unfolding num_tcb_queues_def by eval
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the point of this lemma is. Is it confirming that the constant created by the value_type is correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite. Let's assume 16 domains and 256 prios. Then the value_type generates a definition that says "num_tcb_queues = 4096" and a type abbreviation "num_tcb_queues" that's also 4096, with a loss of info for how that calculation took place. So for example, in the proof of cready_queues_index_to_C_in_range' I don't unfold numDomains_def or numPriorities_def even though you would expect me to normally. The outcome is still a lemma that says something like _ < 4096 but we don't have to unfold the reasons for why it's 4096. It is a bit circular, like trying to not step on one's own toes, and probably not the high point of the abstraction. Does this help at all?

Copy link
Member

Choose a reason for hiding this comment

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

Right, that makes sense. The bit I was missing is that how the calculation occurred was useful in other lemmas. With that in mind, I can't think of a nicer way to handle it than what you've done.

apply (clarsimp simp: valid_tcb_state'_def invs_valid_queues' invs_valid_objs'
invs_queues invs_sch_act_wf' ct_in_state'_def pred_tcb_at'
rf_sr_ksCurThread word_sle_def word_sless_def sysargs_rel_to_n
mask_eq_iff_w2p mask_eq_iff_w2p word_size "StrictC'_thread_state_defs"
maxDomain_def numDomains_def)
)
Copy link
Member

Choose a reason for hiding this comment

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

tiny nitpick: remove newline before the closing parenthesis

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

Comment on lines 38 to 39
(* 256 looks like numPriorities *)
apply (auto simp: cready_queues_index_to_C_def numPriorities_def
seL4_MaxPrio_def word_le_nat_alt dest: arg_cong[where f="\<lambda>x. x mod 256"])
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment intended to still be here? I guess the fact that you didn't need to replace the number means that it isn't representing numPriorities?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean numDomains? I was worried the 256 had to do with 2 ^ LENGTH(8) where 8 is the size in bits of the domain type, but there also happen to be 256 numPriorities. I still suspect it's the latter, but probably safe even if it's the former. I'll drop the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I was reading the comment the other way around, more as a FIXME. I thought you meant that 256 might be numPriorities, and that if it was you would want to replace it.

proof/crefine/ARM/IpcCancel_C.thy Show resolved Hide resolved
apply (clarsimp simp: word_size prioToL1Index_def wordRadix_def mask_def
invertL1Index_def l2BitmapSize_def'
le_maxDomain_eq_less_numDomains word_le_nat_alt)
apply (case_tac "da = d" ; clarsimp simp: num_domains_index_updates)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename the da first?

Copy link
Member Author

@Xaphiosis Xaphiosis Dec 22, 2021

Choose a reason for hiding this comment

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

I can take a look. This is copied/moved, so I don't know where da is in the order for renaming. Still, good suggestion.

  • investigate da rename

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this one, it was indeed unnecessary. For the other ones it makes little difference to rename, it's just another d so all we could rename it to is d'.

unfolding Kernel_C.maxDom_def maxDomain_def Kernel_Config.numDomains_def
by clarsimp

lemma num_domains_calculation: "num_domains = numDomains"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lemma num_domains_calculation: "num_domains = numDomains"
lemma num_domains_calculation:
"num_domains = numDomains"

Copy link
Member Author

Choose a reason for hiding this comment

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

done

lib/Value_Type.thy Outdated Show resolved Hide resolved
lsf37 and others added 7 commits December 22, 2021 16:20
The `value_type` top-level command allows evaluating a term down to a
natural number, and using that number to define an enumerated type, as
well as (optionally) a constant definition.

Co-authored-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
Signed-off-by: Gerwin Klein <gerwin.klein@proofcraft.systems>
Introduce Kernel_Config theory for storage of non-architecture-specific
seL4 configuration variables that are shared by the abstract and design
specs.

Remove `num_domains`, in lieu of `numDomains` that is now defined only
in `Kernel_Config.thy`. The definition is hidden and must be referred to
as Kernel_Config.numDomains_def when avoiding unfolding is not possible.

Include required properties of `numDomains` as lemmas.

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
Make proofs work with any number of domains that fits in the domain type
(at this time an 8-bit word).

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
The proofs work without knowing the number of domains, including with
only a single domain.

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
Proofs now don't care about numDomains, except for a small interface in
Invariants_H. The interface is currently by convention only, and has no
enforcement capabilities.

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
For CRefine, this process is much more complex than for Refine and up,
as the C code both has its own definitions `maxDom` and `numDomains`,
but they are not defined in terms of each other, only numbers.
Similarly, array size types and their corresponding ArrayGuard bounds
checks refer to specific numbers, making a fullproof abstraction impossible.

A reasonably constrained interface to numDomains/maxDomain/maxDom in
Wellformed_C provides a sufficient abstraction to allow the proofs to be
independent of the number of domains (constrained to <= 256). Using the
value_type command allows more abstraction techniques, such as linking
the size of the scheduler queues back to numDomains*numPriorities,
without stating what the numbers are. Finally, for getting past the
ArrayGuard bounds checks, we do leak some information in the form of
`explicit` lemmas. These are the least safe, but short of augmenting the
C parser to re-wrap array sizes into equivalent constants/types, they
constitute a limited risk. Nonetheless, `explicit` lemmas should be used
as sparingly as possible.

Refinement to C proceeds by pretending we don't know the number of
domains, and whenever a control flow decision is made based on
`numDomains > 1`, we follow both branches, as we did for Refine. We also
attempt to avoid clever rewrites such as `(x < 1) = (x = 0)` which mess
up bounds checks into a domain-size array when `numDomains = 1`.

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
The InfoFlow proof itself does not care about the number of domains, and
that assumption was removed in another commit.
The specific example in the information flow refinement requires two
domains (one "high" and one "low") to be of any interest. Since it
cannot be instantiated with only one domain, the example theorems in
Example_Valid_StateH now assume that `1 <= maxDomain`.

Signed-off-by: Rafal Kolanski <rafal.kolanski@proofcraft.systems>
@Xaphiosis
Copy link
Member Author

All @corlewis issues should be addressed now. Squashed and rebased, ready to merge once tests pass to make sure I didn't mess up anything by mistake.

@Xaphiosis Xaphiosis merged commit 73911da into master Dec 22, 2021
@Xaphiosis Xaphiosis deleted the numdomains branch December 22, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants