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:32, Andrew Cooper wrote:
> On 14/04/2020 15:21, Jan Beulich wrote:
>> 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.
> 
> That particular choice of wording is odd, because it should be "legacy
> hardware" instead.  I'll add this to my "pestering AMD list".
> 
> There probably is a perf hit from it, as flushing every ASID includes
> flushing ASID 0 which is Xen's TLB entries.  For back-to-back vmexits,
> this probably is noticeable.
> 
>>>> 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)?
> 
> Would you accept /* TODO: investigate using TLB_CTRL_FLUSH_ASID here
> instead. */ ?

Yes, thanks.

Jan