xen/arch/x86/hvm/svm/asid.c | 14 ++++++-------- xen/include/asm-x86/hvm/svm/vmcb.h | 13 ++++++++++++- 2 files changed, 18 insertions(+), 9 deletions(-)
svm_asid_handle_vmrun() treats tlb_control as if it were boolean, but this has
been superseded by new additions to the SVM spec.
Introduce an enum containing all legal values, and update
svm_asid_handle_vmrun() to use appropriate constants.
While adjusting this, take the opportunity to fix up two coding style issues,
and trim the include list.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
N.B. Deliberately not updating the code to use TLB_CTRL_FLUSH_ASID. The
safety of the current ASID logic depends on flusing everything when the ASID
wraps.
---
xen/arch/x86/hvm/svm/asid.c | 14 ++++++--------
xen/include/asm-x86/hvm/svm/vmcb.h | 13 ++++++++++++-
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/asid.c b/xen/arch/x86/hvm/svm/asid.c
index e554e25213..b7a737fdc1 100644
--- a/xen/arch/x86/hvm/svm/asid.c
+++ b/xen/arch/x86/hvm/svm/asid.c
@@ -15,12 +15,9 @@
* this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/init.h>
-#include <xen/lib.h>
-#include <xen/perfc.h>
-#include <asm/hvm/svm/asid.h>
#include <asm/amd.h>
#include <asm/hvm/nestedhvm.h>
+#include <asm/hvm/svm/asid.h>
void svm_asid_init(const struct cpuinfo_x86 *c)
{
@@ -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;
return;
}
- if (vmcb_get_guest_asid(vmcb) != p_asid->asid)
+ if ( vmcb_get_guest_asid(vmcb) != p_asid->asid )
vmcb_set_guest_asid(vmcb, p_asid->asid);
- vmcb->tlb_control = need_flush;
+
+ vmcb->tlb_control = need_flush ? TLB_CTRL_FLUSH_ALL : TLB_CTRL_NO_FLUSH;
}
/*
diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h
index e5ed38369e..c2e1972feb 100644
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -302,6 +302,17 @@ enum VMEXIT_EXITCODE
VMEXIT_INVALID = -1
};
+enum
+{
+ /* Available on all SVM-capable hardware. */
+ TLB_CTRL_NO_FLUSH = 0,
+ TLB_CTRL_FLUSH_ALL = 1,
+
+ /* Available with the FlushByASID feature. */
+ TLB_CTRL_FLUSH_ASID = 3,
+ TLB_CTRL_FLUSH_ASID_NONGLOBAL = 7,
+};
+
typedef union
{
struct
@@ -419,7 +430,7 @@ struct vmcb_struct {
u64 _msrpm_base_pa; /* offset 0x48 - cleanbit 1 */
u64 _tsc_offset; /* offset 0x50 - cleanbit 0 */
u32 _guest_asid; /* offset 0x58 - cleanbit 2 */
- u8 tlb_control; /* offset 0x5C */
+ u8 tlb_control; /* offset 0x5C - TLB_CTRL_* */
u8 res07[3];
vintr_t _vintr; /* offset 0x60 - cleanbit 3 */
intstat_t int_stat; /* offset 0x68 */
--
2.11.0
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"?
Preferably with such an adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
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.
> 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.
A change like that should be in its own patch, ideally with finding some
old enough hardware to verify. I do not know if I have anything that
old to hand. (Failing real hardware, some conformation from AMD about
how the CPU behaves).
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks,
~Andrew
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
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
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
© 2016 - 2024 Red Hat, Inc.