Re: [PATCH] x86/svm: Don't use vmcb->tlb_control as if it is a boolean

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2020 16:15, Andrew Cooper wrote:
> On 14/04/2020 14:57, Jan Beulich wrote:
>> On 14.04.2020 14:14, Andrew Cooper wrote:
>>> @@ -44,19 +41,20 @@ void svm_asid_handle_vmrun(void)
>>>      struct hvm_vcpu_asid *p_asid =
>>>          nestedhvm_vcpu_in_guestmode(curr)
>>>          ? &vcpu_nestedhvm(curr).nv_n2asid : &curr->arch.hvm.n1asid;
>>> -    bool_t need_flush = hvm_asid_handle_vmenter(p_asid);
>>> +    bool need_flush = hvm_asid_handle_vmenter(p_asid);
>>>  
>>>      /* ASID 0 indicates that ASIDs are disabled. */
>>>      if ( p_asid->asid == 0 )
>>>      {
>>>          vmcb_set_guest_asid(vmcb, 1);
>>> -        vmcb->tlb_control = 1;
>>> +        vmcb->tlb_control = TLB_CTRL_FLUSH_ALL;
>> While there ought to be no difference in behavior, use of
>> TLB_CTRL_FLUSH_ASID would seem more logical to me here. Other
>> than below we're no after flushing all ASIDs in this case
>> afaict.
>>
>> Question of course is - did early CPUs treat this as boolean,
>> accepting any non-zero value to mean "flush all"?
> 
> The spec states "When the VMM sets the TLB_CONTROL field to 1, ...",
> which is fairly clear on the matter.

Well, it is a clear statement without it being clear how close to
truth it is. Consider the spec also saying "Should only be used by
legacy hypervisors" for the value of 1.

>> Preferably with such an adjustment
> 
> I'd prefer not to.  There is a good chance that your suggestion will
> suffer a vmentry failure, or not flush anything on old hardware.

Okay then. Could I talk you into adding at least a respective
comment there? Or one indicating that we should stop using the
value of 1 altogether (which of course is a bigger change)?

Jan