On 27/06/18 18:19, Radim Krčmář wrote:
+struct vmcs_hdr {
+ u32 revision_id:31;
+ u32 shadow_vmcs:1;
+};
Just wondering if this struct is needed. (didn't check following patches)
It makes code more readable but of course I could have completed the
patch series without it.
It will just force me to have to signal the "shadow_vmcs" bit in
previous "uint32_t revision_id" field
manually (e.g. "|= (1u << 31)") which I think is less elegant.
I was rather wondering if the bitmap thingy can be also done directly in
the struct vmcs (avoiding the struct vmcs_hdr).
It's possible but I don't like it because I need to define the exact same
struct for both "struct vmcs" and "struct vmcs12".
I prefer it to be a separate struct vmcs_hdr like it is now.
We could include struct vmcs in struct vmcs12 to flatten the layout for
non-nested case, but pasting the same change in vmcs12 is arguably
nicer:
* Sharing the structure also means that if Intel steals more bits from
revision_id, we'll need to modify nVMX code even though it's not even
implemented there,
* We're not building API for the vmcs_hdr, so the wrapping structure
mainly makes the code longer.
I think that copy-pasting is preferable in this case.
I agree with the first argument.
I will change this patch to copy-paste an embedded vmcs_hdr struct both
for vmcs and vmcs12 structures
for v2 of the series once the review on all the series is completed.
@@ -8418,7 +8423,7 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu)
return kvm_skip_emulated_instruction(vcpu);
}
new_vmcs12 = kmap(page);
- if (new_vmcs12->revision_id != VMCS12_REVISION) {
+ if (new_vmcs12->hdr.revision_id != VMCS12_REVISION) {
This actually changes functionality. There should be a
|| new_vmcs12->hdr.shadow
Thanks.
I actually do what you specify on next commit on series:
"KVM: nVMX: Fail VMPTRLD when vmcs12 is shadow VMCS and nested CPU do
not support VMCS shadowing".
I agree with you that in order to not change functionality I should add
this "|| new_vmcs12->hdr.shadow".
I will add this as-well to the v2 of this patch series once review is done.
Thanks,
-Liran
