On 31/05/2018 01:02, Liran Alon wrote:
>>> +static inline bool nested_cpu_has_vmwrite_any_field(struct kvm_vcpu
>> *vcpu)
>>> +{
>>> + return to_vmx(vcpu)->nested.msrs.misc_low &
>>> + MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS;
>>> +}
>>> +
>
> I don't like the fact that we use MSR_IA32_VMX_MISC_VMWRITE_SHADOW_RO_FIELDS in vmx.c
> Why not create a dedicated VMX_MISC_VMWRITE_SHADOW_RO_FIELDS like the rest of the VMX_MISC constants?
> Same for nested_cpu_has() diff.
I'm not sure why you would duplicate everything? Perhaps we could just
move the two MSR_IA32_VMX_MISC_* constants out of msr-index.h.
>>> + for (q = 0; q < ARRAY_SIZE(fields); q++) {
>>> + for (i = 0; i < max_fields[q]; i++) {
>>> + field = fields[q][i];
>>> + field_value = __vmcs_readl(field);
>>> + vmcs12_write_any(&vmx->vcpu, field, field_value);
>>> + }
>>> + /*
>>> + * Skip the VM-exit information fields if they are read-only.
>>> + */
>>> + if (!nested_cpu_has_vmwrite_any_field(&vmx->vcpu))
>>> + break;
>
> I think the way code is written is a bit confusing (even though common practice in KVM :\).
> In my opinion, it's better to have 2 loops that one interates over shadow_read_write_fields[] and the
> second iterates over shadow_read_only_fields[] when the second only happens when
> nested_cpu_has_vmwrite_any_field().
> It is clearer and that is more important than writing short code.
Yeah, I somewhat agree with this. But I think it is not too unreadable. :)
Thanks for the reviews!
Paolo
