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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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. */ ?

~Andrew