Reset xcr0 on restore

[ludfjig]

XCR0 is guest-writable and was not reset by restore(), so its value could leak between guest executions. This fixes the issue by resetting XCR0 to its architectural default, 0x1. I chose a fixed baseline rather than capturing XCR0 because Hyperlight’s default CR4 has OSXSAVE = 0, so extended state is disabled by default. We can reconsider preserving XCR0 when Hyperlight supports AVX.

If useful we could capture it in snapshot as well, I'm just not sure if necessary

[syntactically]

Why add new accessor functions/etc rather than handle xcr0 in sregs along with the other crN?

[ludfjig]

Why add new accessor functions/etc rather than handle xcr0 in sregs along with the other crN?

It's a different hypercall so I thought this was cleanest (except on whp). Since I also didn't add it to snapshot state, i'd need some extra code not to serialize it as well to snapshot if it's part of CommonSpecialRegisters

[jsturtevant]

LGTM

[syntactically]

Why add new accessor functions/etc rather than handle xcr0 in sregs along with the other crN?

It's a different hypercall so I thought this was cleanest (except on whp). Since I also didn't add it to snapshot state, i'd need some extra code not to serialize it as well to snapshot if it's part of CommonSpecialRegisters

Hm, I think it's also nice to avoid adding more arch specific items to #[cfg(target_arch = ...)] items to the core traits (and would like to see how many of the existing ones we can get rid of / push towards the edges), so reusing one of the existing abstractions seems nicer. It also does feel semantically like the other CRx to me? Does it end up adding any overhead to do it in the sregs path, or is it currently used 1:1? If it is 1:1, it feels like it ought to be part of the same abstraction to me.

[ludfjig]

Why add new accessor functions/etc rather than handle xcr0 in sregs along with the other crN?

It's a different hypercall so I thought this was cleanest (except on whp). Since I also didn't add it to snapshot state, i'd need some extra code not to serialize it as well to snapshot if it's part of CommonSpecialRegisters

Hm, I think it's also nice to avoid adding more arch specific items to #[cfg(target_arch = ...)] items to the core traits (and would like to see how many of the existing ones we can get rid of / push towards the edges), so reusing one of the existing abstractions seems nicer. It also does feel semantically like the other CRx to me? Does it end up adding any overhead to do it in the sregs path, or is it currently used 1:1? If it is 1:1, it feels like it ought to be part of the same abstraction to me.

There are some places we only call set_sregs, without needing to call set_xcr0, since we rely on it being initialized to default value (creating new vm, creating vm from snapshot) which is guaranteed by the hv. Combining them would introduce an extra hypercall here that's unnecessary. In general I agree with you though about avoiding adding more target specific cfgs to the trait. How about leaving it like this for now, and in the future if we decide to allow configuring xcr0 to different value, we can consider combining set_sregs and set_xcr0?

[syntactically]

There are some places we only call set_sregs, without needing to call set_xcr0, since we rely on it being initialized to default value (creating new vm, creating vm from snapshot) which is guaranteed by the hv. Combining them would introduce an extra hypercall here that's unnecessary. In general I agree with you though about avoiding adding more target specific cfgs to the trait

When we have time to do some cleanup here we can enumerate the lifecycles of various things and find arch independent abstractions that work better then, but happy to merge this as is for now.