On Fri, Jul 7, 2017 at 9:03 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
>
> On 07/07/2017 17:44, Jim Mattson wrote:
>> On Fri, Jul 7, 2017 at 1:34 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On 07/07/2017 10:22, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 06/07/2017 21:52, Jim Mattson wrote:
>>>>> Allow VMWRITE in L1 to modify VM-exit information fields and report
>>>>> this feature in L1's IA32_VMX_MISC MSR.
>>>>>
>>>>> Note that this feature is a prerequisite for kvm in L1 to use VMCS
>>>>> shadowing, once that feature is available.
>>>>>
>>>>> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>>>>> ---
>>>>> arch/x86/kvm/vmx.c | 105 ++++++++++++++++++++---------------------------------
>>>>> 1 file changed, 39 insertions(+), 66 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> index b4cfdcfdc1c1..72f295510f76 100644
>>>>> --- a/arch/x86/kvm/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>> @@ -7467,14 +7447,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
>>>>> }
>>>>> }
>>>>>
>>>>> -
>>>>> field = kvm_register_readl(vcpu, (((vmx_instruction_info) >> 28) & 0xf));
>>>>> - if (vmcs_field_readonly(field)) {
>>>>> - nested_vmx_failValid(vcpu,
>>>>> - VMXERR_VMWRITE_READ_ONLY_VMCS_COMPONENT);
>>>>> - return kvm_skip_emulated_instruction(vcpu);
>>>>> - }
>>>>> -
>>>>> if (vmcs12_write_any(vcpu, field, field_value) < 0) {
>>>>> nested_vmx_failValid(vcpu, VMXERR_UNSUPPORTED_VMCS_COMPONENT);
>>>>> return kvm_skip_emulated_instruction(vcpu);
>>>>>
>>>>
>>>> vmcs_field_readonly is now unused. With that removed,
>>>>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>>
>>> Actually, no. The error must be kept if the host has disabled the
>>> feature with a KVM_SET_MSR ioctl for MSR_IA32_VMX_MISC.
>>
>> Supporting both settings of this feature bit is a bit more complicated.
>
> Right, you have to set up the vmwrite bitmap correctly. But actually it should
> be a couple lines of code and your patch would be much simpler, because you keep
> the RW and RO field lists separate.
>
> After your changes here, vmx_vmread_bitmap and vmx_vmwrite_bitmap have
> always the same content. Instead, it should be possible for the read
> and write bitmap to point to the same address. KVM_SET_MSR can do:
>
> if (enable_shadow_vmcs) {
> if (/* L1 has vmwrite to all fields */)
> vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmread_bitmap));
> else
> vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap));
> }
>
Actually, KVM_SET_MSR can only disable the feature.
> And in vmx_vcpu_setup,
>
> - vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmwrite_bitmap));
> + vmcs_write64(VMWRITE_BITMAP, __pa(vmx_vmread_bitmap));
>
> so that the default is to expose the feature.
Yes, but copy_shadow_to_vmcs12() also needs to walk the "read-only" fields when
they are read/write. Dealing with the two arrays of indices is awkward. Let me
return to this later. I'll fix up patch 4 to deal with the code as it
stands today.
>
> Paolo
>
>>> The upside is that patch 4 is good as is. :)
>>
>> Not quite, but close.
>>
>>> Also:
>>>
>>>>>
>>>>> + /*
>>>>> + * We can emulate "VMWRITE to any supported field," even if
>>>>> + * the hardware doesn't support it.
>>>>> + */
>>>>> + vmx->nested.nested_vmx_misc_low |=
>>>>> + MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS;
>>>>> +
>>>
>>> There is already a "vmx->nested.nested_vmx_misc_low |= " statement a
>>> couple lines above. Please generalize the comment to something like "We
>>> can always emulate these features, even if the hardware doesn't support
>>> them".
>>>
>>> Paolo
