On Fri, 2021-09-03 at 15:11 +0000, Sean Christopherson wrote:
> ...
Hi Sean,
Sorry for so late reply. Multi-task, you know;-)
The discussion about this patch has passed so long time and has
diverged, actually. Let me summarize our previous discussions. Then we
can converge things and settle direction.
* Copy to/from shadow vmcs, no need to validate field existence or not.
-- I agree.
* Now that only VMCS-read/write need to validate field existence, can
use static check instead of bitmap.
* And borrow bit 0 in the field->offset table to denote conditional
fields.
Because:
Shadow control can have more chances to be cache-hit than
bitmap.
The bitmap is per-VMX, additional memory allocation is not
interesting.
Robert argued:
I still prefer to use bitmap to denote conditional fields.
If used static switch…case check rather than bitmap, the
switch…case would be very long. Till today, ~51 conditional fields.
Though very less likely, we cannot guarantee no future use of
bit 0 of field->offset table entry.
From perspective of runtime efficiency, read bitmap is better
to do static check every time.
From the perspective of cache hit chance, shadow control (or
nested_vmx_msrs) and bitmap are both in nested structure, I don't think
they have essential difference.
The bitmap is just 62 bytes long now, I think it's tolerable.:)
*• Interaction with Shadow VMCS -- for those shadowed fields, we cannot
trap its read/write, therefore cannot check its existence per vmx
configuration L0 set for L1.
This last point is the most messy one.
If we would like to solve this, you proposed as a middle ground
to disable shadow VMCS totally when user space setting conflicts with
what KVM figured out.
You also said, "This is quite the complicated mess for
something I'm guessing no one actually cares about. At what point do
we chalk this up as a virtualization hole and sweep it under the rug?"
-- I couldn't agree more.
We think to disable shadow VMCS totally is not good in any
circumstances, for the sake of nested performance, etc..
We think there are 2 ways ahead:
1) Leave it as it is nowadays, i.e. discard this patch set.
Perhaps we can add some build-check to force update that hard-coded
assignment to vmcs-enum-max when necessary.
2) Make shadow vmcs bitmap per VM. This will have to allocate 2
(or 3?) more pages (shadow read/write bitmaps) per VM. Then we can
configure the shadow vmcs bitmap per user space configuration, i.e.
don't shadow those conditional VMCS fields, force its read/write to go
through handle_vm{read,write} gate.
So, Sean, can you help converge our discussion and settle next step?
Thanks.:-)
