2018-06-25 19:11+0300, Liran Alon:
>
> On 25/06/18 18:37, David Hildenbrand wrote:
> > On 25.06.2018 17:32, Liran Alon wrote:
> > > On 25/06/18 15:20, David Hildenbrand wrote:
> > > > On 23.06.2018 01:35, Liran Alon wrote:
> > > > > No functionality change.
> > > > >
> > > > > Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
> > > > > Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > > > > ---
> > > > > arch/x86/kvm/vmx.c | 17 +++++++++++------
> > > > > 1 file changed, 11 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > > > index 559a12b6184d..2fe86eb91aca 100644
> > > > > --- a/arch/x86/kvm/vmx.c
> > > > > +++ b/arch/x86/kvm/vmx.c
> > > > > @@ -198,8 +198,13 @@ struct kvm_vmx {
> > > > > #define NR_AUTOLOAD_MSRS 8
> > > > > +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.
> > > > > @@ -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.
