On 06/18/2018 07:42 AM, Keno Fischer wrote: >> There's no way this is even close to viable until it has been made to >> cope with holes. > > Ok, it seems to me the first decision is whether it should be allowed > to have the compacted format (with holes) in an in-memory xstate > image. Doing so seems possible, but would require more invasive > changes to the kernel, so I'm gonna ignore it for now. > > If we want to keep the in-memory xstate layout constant after boot > time, I see four ways to do that for this feature. > > 1) Set up XCR0 in a different place, so that the kernel xsaves/xrstors > use an XCR0 that matches the layout the kernel expects.
... and do XCR0 writes before every XSAVES/XRSTORS, including in the context-switch path? > 2) Use xsaveopt when this particular thread flag is set and have the > kernel be able to deal with non-compressed xstate images, even > if xsaves is available. What about if there is supervisor state in place? > 3) What's in this patch now, but fix up the xstate after saving it. That's _maybe_ viable. But, it's going to slow things down quite a bit and has to be done with preempt disabled. > 4) Catch the fault thrown by xsaves/xrestors in this situation, update > XCR0, redo the xsaves/restores, put XCR0 back and continue > execution after the faulting instruction. I'm worried about the kernel pieces that go digging in the XSAVE data getting confused more than the hardware getting confused. > Option 1) seems easiest, but it would also require adding code > somewhere on the common path, which I assume is a no-go ;). Yeah, that would be horribly slow. You then also have to be really careful with interrupts and preempt when you're in a funky XCR0 configuration. > Option 3) seems doable entirely in the slow path for this feature. > If we xsaves with the modified XCR0, we can then fix up the memory > image to match the expected layout. Similarly, we could xrestors > in the slow path (from standard layout) and rely on the > `fpregs_state_valid` mechanism to prevent the fault. ... with one more modification. You need two buffers _anyway_ if you do this. So you would probably just XSAVE to a new buffer and then convert that back to the "real" buffer in the thread struct. > At least currently, it is my understanding that `xfeatures_mask` only has > user features, am I mistaken about that? We're slowing adding supervisor support. I think accounting for supervisor features is a requirement for any new XSAVE code.
