Enable SNP for WCOW - #2507
Conversation
|
@takuro-sato could you please rebase this on Mahati's branch? There are a lot of related changes coming in from my PR, Kirtana's PR and Mahati's PR. Mahati's branch includes changes from my PR as well as Kirtana's PR. |
|
@ambarve They are indeed related, but this PR doesn't require them. So can't we just merge it based on current main? I'm also happy to wait for the existing PRs to be merged, but I think it would be easier for me to just to wait, and rebase it on main again. (Even if I rebase it on Mahati's, I would need to rebase it on main again anyway) |
| doc.VirtualMachine.ComputeTopology.Memory.Backing = &memoryBacking | ||
| doc.VirtualMachine.ComputeTopology.Memory.AllowOvercommit = opts.AllowOvercommit | ||
| // EnableHotHint is not compatible with physical. | ||
| doc.VirtualMachine.ComputeTopology.Memory.EnableHotHint = opts.AllowOvercommit |
There was a problem hiding this comment.
handleWCOWSecurityPolicy sets AllowOvercommit to false, so we don't need to explicitly set that here.
Do we not need physically backed memory for VBS mode?
There was a problem hiding this comment.
Maybe I'm missing something, but the change is only for SNP. VBS will keep having memoryBacking with this change.
There was a problem hiding this comment.
@ambarve I agree with @takuro-sato - if isolation type is SNP (ok, literally "SecureNestedPaging") then the reulting doc is identical to the one the old code would generate.
There was a problem hiding this comment.
Sorry, I meant to ask, "Do we not need physically backed memory for SNP mode?" This new code will set the memory backing type to physical only for non SNP cases. For SNP cases it will use whatever is the default (which would be a nil pointer, I think). Is that what we want?
Similarly, I think if allow overcommit is set to true, boot will fail in VBS mode too. That's why the handleWCOWSecurityPolicy method explicitly errors out if that option is set. So we don't need to handle that in a separate if case here.
There was a problem hiding this comment.
I think we should leave the existing behaviour as is (ie overcommit in VBS will fail in the HCS calls) so we can minimise our impact and risk of breaking things.
There was a problem hiding this comment.
BTW, we don't want to respect annotations that will 100% cause a failure is in case SF or some other control plane is setting them and so we have to do more work there. SF changes take a long time. This may also lead to us using the lcow annotation for the wcow security policy eventually.
There was a problem hiding this comment.
@ambarve infact, I think main now gets only ever gets the overcommit flag as a side effect of "handleWCOWSecurityPolicy" which people may find surprising. In the LCOW case what I did was leave all the existing processing in place and do the handleSecurityPolicy part after such flags (with a comment to say that this is essential) so that the security policy handler was not essential to the non confidential path.
ebe9df4 to
004a3b7
Compare
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
004a3b7 to
b53e344
Compare
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com> Co-authored-by: Amit Barve <ambarve@microsoft.com>
a94008b to
416844c
Compare
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
df89b78 to
d86b617
Compare
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
Signed-off-by: Takuro Sato <takurosato@microsoft.com>
This PR changes the code to always have the default isolation type of "SecureNestedPaging". It also updates the code so that it doesn't error out if allow over commit is set to true in annotations, instead that value is silently overridden to false. This commit updates the tests to follow the same logic. Signed-off-by: Amit Barve <ambarve@microsoft.com>
4 participants
Changes
Add new annotations WCOWIsolationType and WCOWDisableSecureBootTODOs for a future PR
doc.VirtualMachine.Versionin create_wcow.go so that UVM has the new flag enabled for VTL0 guest request. The new version should be one that supports the flag, but the exact version number is TBD yet. With the current code, bumping version will enable the new flag as default value. Or we can specify each flag we need as takuro-sato@4050c1e#diff-4c78bbfa476a3289e2b577d78fa432a0d650e16f05b4a4e5ce4332291d8b9dad