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).
>
>>
>>> +
>>> struct vmcs {
>>> - u32 revision_id;
>>> + struct vmcs_hdr hdr;
>>> u32 abort;
>>> char data[0];
>>> };
>>> @@ -253,7 +258,7 @@ struct __packed vmcs12 {
>>> /* According to the Intel spec, a VMCS region must start with the
>>> * following two fields. Then follow implementation-specific data.
>>> */
>>> - u32 revision_id;
>>> + struct vmcs_hdr hdr;
>>> u32 abort;
>>>
>>> u32 launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */
>>> @@ -421,7 +426,7 @@ struct __packed vmcs12 {
>>> "Offset of " #field " in struct vmcs12 has changed.")
>>>
>>> static inline void vmx_check_vmcs12_offsets(void) {
>>> - CHECK_OFFSET(revision_id, 0);
>>> + CHECK_OFFSET(hdr, 0);
>>> CHECK_OFFSET(abort, 4);
>>> CHECK_OFFSET(launch_state, 8);
>>> CHECK_OFFSET(io_bitmap_a, 40);
>>> @@ -4385,7 +4390,7 @@ static struct vmcs *alloc_vmcs_cpu(int cpu)
>>> return NULL;
>>> vmcs = page_address(pages);
>>> memset(vmcs, 0, vmcs_config.size);
>>> - vmcs->revision_id = vmcs_config.revision_id; /* vmcs revision id */
>>> + vmcs->hdr.revision_id = vmcs_config.revision_id;
>>> return vmcs;
>>> }
>>>
>>> @@ -7848,7 +7853,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
>>> if (!shadow_vmcs)
>>> goto out_shadow_vmcs;
>>> /* mark vmcs as shadow */
>>> - shadow_vmcs->revision_id |= (1u << 31);
>>> + shadow_vmcs->hdr.shadow_vmcs = 1;
>>> /* init shadow vmcs */
>>> vmcs_clear(shadow_vmcs);
>>> vmx->vmcs01.shadow_vmcs = shadow_vmcs;
>>> @@ -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) {
>>> kunmap(page);
>>> kvm_release_page_clean(page);
>>> nested_vmx_failValid(vcpu,
>>>
>>
>
--
Thanks,
David / dhildenb
