On 10/12/2019 10:07, Jan Beulich wrote: > On 10.12.2019 10:59, Andrew Cooper wrote: >> On 09/12/2019 18:06, Roger Pau Monne wrote: >>> Currently cr4 is not cached before suspension, and mmu_cr4_features is >>> used in order to restore the expected cr4 value. This is correct so >>> far because the tasklet that executes the suspend/resume code is >>> running in the idle vCPU context. >>> >>> In order to make the code less fragile, explicitly save the current >>> cr4 value before suspension, so that it can be restored afterwards. >>> This ensures that the cr4 value cached in the cpu_info doesn't get out >>> of sync after resume from suspension. >>> >>> Suggested-by: Jan Beulich <jbeul...@suse.com> >>> Signed-off-by: Roger Pau Monné <roger....@citrix.com> >> Why? There is nothing fragile here. Suspend/resume is always in idle >> context and loads of other logic already depends on this. >> >> I've been slowly stripping out redundant saved state like this. > Where it's clearly redundant, this is fine. But I don't think it's > sufficiently clear here
There is a reason I made it explicitly crystal clear with c/s 87e7b4d5b > , and going back to what was there before > is imo generally less error prone than going to some fixed state. It is demonstrably more error prone, which is why I'm slowly killing it. Stashing state wastes unnecessary space, and adds an error case where state is either stashed incorrectly, or gets modified before restore, and we'll blindly use. Two examples of real bugs caused by this are c/s 0c30171cb and 4ee0ad72d Absolutely nothing remaining in suspend.c should be spilled. It can all be (re)caluclated from the same information source as the AP boot path, and the result will be strictly smaller in RAM, and more robust. > Furthermore I was hoping we could eventually do away with > mmu_cr4_features. How do you plan on doing this? ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
