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));
}
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.
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
