On 11/27/2017 12:37 PM, Paolo Bonzini wrote:
> On 25/11/2017 14:09, Jan H. Schönherr wrote:
>> +static void vmx_set_intr_info(struct kvm_vcpu *vcpu, u32 intr)
>> +{
>> + vmcs_write32(VM_ENTRY_INTR_INFO_FIELD, intr);
>
> Would you mind introducing vmx_set_intr_info in a separate patch?
No problem.
>> + /*
>> + * Ensure that we clear the HLT state in the VMCS. We don't need to
>> + * explicitly skip the instruction because if the HLT state is set, then
>> + * the instruction is already executing and RIP has already been
>> + * advanced.
>> + */
>> + if (!kvm_hlt_in_guest(vcpu->kvm) || !(intr & INTR_INFO_VALID_MASK))
>> + return;
>
> I think this is unnecessary; IMO you can just do
>
> if (intr & INTR_INFO_VALID_MASK)
> vmcs_write32(GUEST_ACTIVITY_STATE,
> GUEST_ACTIVITY_ACTIVE);
>
> after the write to VM_ENTRY_INTR_INFO_FIELD. This is because:
>
> - if !kvm_hlt_in_guest(vcpu->kvm), the activity state is always "active"
If we're intercepting HLT, the activity state is always active and there
is no need to overwrite it in the first place. That's why it is there
(and should stay there, in my opinion).
> - a vmread is almost as expensive as a vmwrite, so the vmread+vmwrite is
> almost never cheaper especially in the case where guest activity is
> "hlt" (which is also when sensitivity to latency is highest)
So, just blindy overwrite. But only in cases, when it actually could
be "halted"... will do -- though see comment at the bottom.
>> + if (is_external_interrupt(intr) || is_nmi(intr))
>> + return;
>
> What is this for?
This avoids the vmwrite, when INTR_INFO is set to something, that is able
to get the vCPU out of "halted" in hardware. There is one more case
in the SDM (MTF VM Exit) in addition to that, but I thought, this covers
the most likely ones.
>
> Thanks,
>
> Paolo
>
>> + if (vmcs_read32(GUEST_ACTIVITY_STATE) != GUEST_ACTIVITY_HLT)
>> + return;
Will drop this "if" as result of the discussion above.
Do we care about a potential future support for other activity states
(shutdown, wait-for-sipi) at this point? In that case, the "if" may
still be needed.
Regards
Jan
>> + vmcs_write32(GUEST_ACTIVITY_STATE, GUEST_ACTIVITY_ACTIVE);
>> +}
>> +
