[PATCH V2 1/6] HV: enumerate capability of #AC for Splitlock Access #ac


Tao, Yuhong
 

When the destination of an atomic memory operation located in 2 cache lines,
it is called a Splitlock Access. LOCK# bus signal is asserted for splitlock
access which can reduce performance. #AC for Splitlock Access is a CPU feature, it
allows rise alignment check exception #AC(0) instead of asserting LOCK#, that
is helpful to detect Splitlock Access.

Add 3 API:
has_ac_for_splitlock(): return true if CPU has this feature
enale_ac_for_splitlock(), disable_ac_for_splitlock(): enable/disable
this feature, these 2 APIs require precondition of (has_ac_for_splitlock() == true)

If the CPU support this feature, then enable it at the primary CPU early
initialization stage, get ready to detect Splitlock Access in HV

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/cpu.c | 42 +++++++++++++++++++++++++++++++++++++
hypervisor/arch/x86/init.c | 6 ++++++
hypervisor/include/arch/x86/cpu.h | 10 +++++++++
hypervisor/include/arch/x86/cpuid.h | 2 ++
hypervisor/include/arch/x86/msr.h | 2 ++
5 files changed, 62 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index 384587c..f921722 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -572,3 +572,45 @@ uint64_t msr_read_pcpu(uint32_t msr_index, uint16_t pcpu_id)

return ret;
}
+
+static bool ac_for_splitlock = false;
+
+inline bool has_ac_for_splitlock(void)
+{
+ return ac_for_splitlock;
+}
+
+void enable_ac_for_splitlock(void)
+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl |= (1U<<29U);
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void disable_ac_for_splitlock(void)
+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl &= (~(1U<<29U));
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void init_ac_for_splitlock(void)
+{
+ uint32_t eax, ebx, ecx, edx;
+ uint64_t core_cap;
+
+ /* has MSR_IA32_CORE_CAPABILITIES? */
+ cpuid(CPUID_EXTEND_FEATURE, &eax, &ebx, &ecx, &edx);
+ if ((edx & CPUID_EDX_CORE_CAP) != 0U) {
+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
+ enable_ac_for_splitlock();
+ }
+ }
+}
diff --git a/hypervisor/arch/x86/init.c b/hypervisor/arch/x86/init.c
index b991797..229dc35 100644
--- a/hypervisor/arch/x86/init.c
+++ b/hypervisor/arch/x86/init.c
@@ -79,6 +79,8 @@ void init_primary_pcpu(void)
/* Clear BSS */
(void)memset(&ld_bss_start, 0U, (size_t)(&ld_bss_end - &ld_bss_start));

+ init_ac_for_splitlock();
+
parse_hv_cmdline();

init_debug_pre();
@@ -95,6 +97,10 @@ void init_secondary_pcpu(void)
{
uint16_t pcpu_id;

+ if (has_ac_for_splitlock() == true) {
+ enable_ac_for_splitlock();
+ }
+
init_pcpu_pre(false);

pcpu_id = get_pcpu_id();
diff --git a/hypervisor/include/arch/x86/cpu.h b/hypervisor/include/arch/x86/cpu.h
index 27e9abd..617422d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -685,6 +685,16 @@ static inline void clac(void)
uint16_t get_pcpu_nums(void);
bool is_pcpu_active(uint16_t pcpu_id);
uint64_t get_active_pcpu_bitmap(void);
+
+void init_ac_for_splitlock(void);
+bool has_ac_for_splitlock(void);
+
+/*
+ * @pre has_ac_for_splitlock() == true
+ */
+void enable_ac_for_splitlock(void);
+void disable_ac_for_splitlock(void);
+
#else /* ASSEMBLER defined */

#endif /* ASSEMBLER defined */
diff --git a/hypervisor/include/arch/x86/cpuid.h b/hypervisor/include/arch/x86/cpuid.h
index 130b37b..ecdd224 100644
--- a/hypervisor/include/arch/x86/cpuid.h
+++ b/hypervisor/include/arch/x86/cpuid.h
@@ -84,6 +84,8 @@
#define CPUID_EDX_IBRS_IBPB (1U<<26U)
/* CPUID.07H:EDX.STIBP*/
#define CPUID_EDX_STIBP (1U<<27U)
+/* CPUID.07H:EDX.CORE_CAPABILITIES */
+#define CPUID_EDX_CORE_CAP (1U<<30U)
/* CPUID.80000001H:EDX.Page1GB*/
#define CPUID_EDX_PAGE1GB (1U<<26U)
/* CPUID.07H:EBX.INVPCID*/
diff --git a/hypervisor/include/arch/x86/msr.h b/hypervisor/include/arch/x86/msr.h
index 70becbe..d311f8d 100644
--- a/hypervisor/include/arch/x86/msr.h
+++ b/hypervisor/include/arch/x86/msr.h
@@ -18,6 +18,7 @@
#define MSR_IA32_TIME_STAMP_COUNTER 0x00000010U
#define MSR_IA32_PLATFORM_ID 0x00000017U
#define MSR_IA32_APIC_BASE 0x0000001BU
+#define MSR_TEST_CTL 0x00000033U
#define MSR_IA32_FEATURE_CONTROL 0x0000003AU
#define MSR_IA32_TSC_ADJUST 0x0000003BU
/* Speculation Control */
@@ -40,6 +41,7 @@
#define MSR_IA32_PMC5 0x000000C6U
#define MSR_IA32_PMC6 0x000000C7U
#define MSR_IA32_PMC7 0x000000C8U
+#define MSR_IA32_CORE_CAPABILITIES 0x000000CFU
/* Max. qualified performance clock counter */
#define MSR_IA32_MPERF 0x000000E7U
/* Actual performance clock counter */
--
2.7.4


Eddie Dong
 

-----Original Message-----
From: acrn-dev@lists.projectacrn.org <acrn-dev@lists.projectacrn.org> On
Behalf Of Tao, Yuhong
Sent: Monday, March 23, 2020 9:59 PM
To: acrn-dev@lists.projectacrn.org
Cc: Tao, Yuhong <yuhong.tao@intel.com>
Subject: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC for
Splitlock Access

When the destination of an atomic memory operation located in 2 cache lines,
it is called a Splitlock Access. LOCK# bus signal is asserted for splitlock access
which can reduce performance. #AC for Splitlock Access is a CPU feature, it
Can Reduce performance --> may lead to long latency.

allows rise alignment check exception #AC(0) instead of asserting LOCK#, that
is helpful to detect Splitlock Access.

Add 3 API:
has_ac_for_splitlock(): return true if CPU has this feature
enale_ac_for_splitlock(), disable_ac_for_splitlock(): enable/disable this
feature, these 2 APIs require precondition of (has_ac_for_splitlock() == true)

If the CPU support this feature, then enable it at the primary CPU early
initialization stage, get ready to detect Splitlock Access in HV

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/cpu.c | 42
+++++++++++++++++++++++++++++++++++++
hypervisor/arch/x86/init.c | 6 ++++++
hypervisor/include/arch/x86/cpu.h | 10 +++++++++
hypervisor/include/arch/x86/cpuid.h | 2 ++
hypervisor/include/arch/x86/msr.h | 2 ++
5 files changed, 62 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c index
384587c..f921722 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -572,3 +572,45 @@ uint64_t msr_read_pcpu(uint32_t msr_index,
uint16_t pcpu_id)

return ret;
}
+
+static bool ac_for_splitlock = false;
+
+inline bool has_ac_for_splitlock(void)
+{
+ return ac_for_splitlock;
+}
+
+void enable_ac_for_splitlock(void)
+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl |= (1U<<29U);
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void disable_ac_for_splitlock(void)
Normally we do from top to bottom.
An API should be used first (called), and then implemented. Liu Yuan's SR-IOV patch series do well.

For this time, I can merge, but not for next.

+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl &= (~(1U<<29U));
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void init_ac_for_splitlock(void)
+{
+ uint32_t eax, ebx, ecx, edx;
+ uint64_t core_cap;
+
+ /* has MSR_IA32_CORE_CAPABILITIES? */
+ cpuid(CPUID_EXTEND_FEATURE, &eax, &ebx, &ecx, &edx);
FEAT_7_0_EDX already holds this, please use FEAT_7_0_EDX.


+ if ((edx & CPUID_EDX_CORE_CAP) != 0U) {
+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
No need to introduce ac_for_splitlock. Using FEAT_7_0_EDX, and cpu_caps for #AC.

+ enable_ac_for_splitlock();
+ }
+ }
+}
I thought we can go with one API only -- merge this with enable_xxx.

diff --git a/hypervisor/arch/x86/init.c b/hypervisor/arch/x86/init.c index
b991797..229dc35 100644
--- a/hypervisor/arch/x86/init.c
+++ b/hypervisor/arch/x86/init.c
@@ -79,6 +79,8 @@ void init_primary_pcpu(void)
/* Clear BSS */
(void)memset(&ld_bss_start, 0U, (size_t)(&ld_bss_end - &ld_bss_start));

+ init_ac_for_splitlock();
+
parse_hv_cmdline();

init_debug_pre();
@@ -95,6 +97,10 @@ void init_secondary_pcpu(void) {
uint16_t pcpu_id;

+ if (has_ac_for_splitlock() == true) {
+ enable_ac_for_splitlock();
+ }
+
init_pcpu_pre(false);

pcpu_id = get_pcpu_id();
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 27e9abd..617422d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -685,6 +685,16 @@ static inline void clac(void) uint16_t
get_pcpu_nums(void); bool is_pcpu_active(uint16_t pcpu_id); uint64_t
get_active_pcpu_bitmap(void);
+
+void init_ac_for_splitlock(void);
+bool has_ac_for_splitlock(void);
+
+/*
+ * @pre has_ac_for_splitlock() == true
+ */
+void enable_ac_for_splitlock(void);
+void disable_ac_for_splitlock(void);
+
#else /* ASSEMBLER defined */

#endif /* ASSEMBLER defined */
diff --git a/hypervisor/include/arch/x86/cpuid.h
b/hypervisor/include/arch/x86/cpuid.h
index 130b37b..ecdd224 100644
--- a/hypervisor/include/arch/x86/cpuid.h
+++ b/hypervisor/include/arch/x86/cpuid.h
@@ -84,6 +84,8 @@
#define CPUID_EDX_IBRS_IBPB (1U<<26U)
/* CPUID.07H:EDX.STIBP*/
#define CPUID_EDX_STIBP (1U<<27U)
+/* CPUID.07H:EDX.CORE_CAPABILITIES */
+#define CPUID_EDX_CORE_CAP (1U<<30U)
/* CPUID.80000001H:EDX.Page1GB*/
#define CPUID_EDX_PAGE1GB (1U<<26U)
/* CPUID.07H:EBX.INVPCID*/
diff --git a/hypervisor/include/arch/x86/msr.h
b/hypervisor/include/arch/x86/msr.h
index 70becbe..d311f8d 100644
--- a/hypervisor/include/arch/x86/msr.h
+++ b/hypervisor/include/arch/x86/msr.h
@@ -18,6 +18,7 @@
#define MSR_IA32_TIME_STAMP_COUNTER 0x00000010U
#define MSR_IA32_PLATFORM_ID 0x00000017U
#define MSR_IA32_APIC_BASE 0x0000001BU
+#define MSR_TEST_CTL 0x00000033U
#define MSR_IA32_FEATURE_CONTROL 0x0000003AU
#define MSR_IA32_TSC_ADJUST 0x0000003BU
/* Speculation Control */
@@ -40,6 +41,7 @@
#define MSR_IA32_PMC5 0x000000C6U
#define MSR_IA32_PMC6 0x000000C7U
#define MSR_IA32_PMC7 0x000000C8U
+#define MSR_IA32_CORE_CAPABILITIES 0x000000CFU
/* Max. qualified performance clock counter */
#define MSR_IA32_MPERF 0x000000E7U
/* Actual performance clock counter */
--
2.7.4



Tao, Yuhong
 

Hi, Eddie

-----Original Message-----
From: Dong, Eddie <eddie.dong@intel.com>
Sent: Tuesday, March 24, 2020 9:59 AM
To: acrn-dev@lists.projectacrn.org
Cc: Tao, Yuhong <yuhong.tao@intel.com>; Dong, Eddie <eddie.dong@intel.com>
Subject: RE: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC for
Splitlock Access



-----Original Message-----
From: acrn-dev@lists.projectacrn.org <acrn-dev@lists.projectacrn.org>
On Behalf Of Tao, Yuhong
Sent: Monday, March 23, 2020 9:59 PM
To: acrn-dev@lists.projectacrn.org
Cc: Tao, Yuhong <yuhong.tao@intel.com>
Subject: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC for
Splitlock Access

When the destination of an atomic memory operation located in 2 cache
lines, it is called a Splitlock Access. LOCK# bus signal is asserted
for splitlock access which can reduce performance. #AC for Splitlock
Access is a CPU feature, it
Can Reduce performance --> may lead to long latency.
Yes, will use it, thanks
allows rise alignment check exception #AC(0) instead of asserting
LOCK#, that is helpful to detect Splitlock Access.

Add 3 API:
has_ac_for_splitlock(): return true if CPU has this feature
enale_ac_for_splitlock(), disable_ac_for_splitlock(): enable/disable
this feature, these 2 APIs require precondition of
(has_ac_for_splitlock() == true)

If the CPU support this feature, then enable it at the primary CPU
early initialization stage, get ready to detect Splitlock Access in HV

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/cpu.c | 42
+++++++++++++++++++++++++++++++++++++
hypervisor/arch/x86/init.c | 6 ++++++
hypervisor/include/arch/x86/cpu.h | 10 +++++++++
hypervisor/include/arch/x86/cpuid.h | 2 ++
hypervisor/include/arch/x86/msr.h | 2 ++
5 files changed, 62 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index
384587c..f921722 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -572,3 +572,45 @@ uint64_t msr_read_pcpu(uint32_t msr_index,
uint16_t pcpu_id)

return ret;
}
+
+static bool ac_for_splitlock = false;
+
+inline bool has_ac_for_splitlock(void) {
+ return ac_for_splitlock;
+}
+
+void enable_ac_for_splitlock(void)
+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl |= (1U<<29U);
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void disable_ac_for_splitlock(void)
Normally we do from top to bottom.
An API should be used first (called), and then implemented. Liu Yuan's SR-IOV
patch series do well.

For this time, I can merge, but not for next.
OK, thank you, but I can fix it this time.

+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl &= (~(1U<<29U));
+ msr_write(MSR_TEST_CTL, test_ctl);
+}
+
+void init_ac_for_splitlock(void)
+{
+ uint32_t eax, ebx, ecx, edx;
+ uint64_t core_cap;
+
+ /* has MSR_IA32_CORE_CAPABILITIES? */
+ cpuid(CPUID_EXTEND_FEATURE, &eax, &ebx, &ecx, &edx);
FEAT_7_0_EDX already holds this, please use FEAT_7_0_EDX.
Yes, no need to run cupid() here, FEAT_7_0_EDX is OK


+ if ((edx & CPUID_EDX_CORE_CAP) != 0U) {
+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
No need to introduce ac_for_splitlock. Using FEAT_7_0_EDX, and cpu_caps for
#AC.
Yes, will use pcpu_has_cap (FEAT_7_0_EDX & 0x30U) to get this feature, do not need ac_for_splitlock

+ enable_ac_for_splitlock();
+ }
+ }
+}
I thought we can go with one API only -- merge this with enable_xxx.
Yes, I think it can be:

void enable_ac_for_splitlock(bool ebable);

to enable this feature: enable_ac_for_splitlock(true)
to disable this feature: enable_ac_forsplitlock(false)

diff --git a/hypervisor/arch/x86/init.c b/hypervisor/arch/x86/init.c
index
b991797..229dc35 100644
--- a/hypervisor/arch/x86/init.c
+++ b/hypervisor/arch/x86/init.c
@@ -79,6 +79,8 @@ void init_primary_pcpu(void)
/* Clear BSS */
(void)memset(&ld_bss_start, 0U, (size_t)(&ld_bss_end -
&ld_bss_start));

+ init_ac_for_splitlock();
+
parse_hv_cmdline();

init_debug_pre();
@@ -95,6 +97,10 @@ void init_secondary_pcpu(void) {
uint16_t pcpu_id;

+ if (has_ac_for_splitlock() == true) {
+ enable_ac_for_splitlock();
+ }
+
init_pcpu_pre(false);

pcpu_id = get_pcpu_id();
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 27e9abd..617422d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -685,6 +685,16 @@ static inline void clac(void) uint16_t
get_pcpu_nums(void); bool is_pcpu_active(uint16_t pcpu_id); uint64_t
get_active_pcpu_bitmap(void);
+
+void init_ac_for_splitlock(void);
+bool has_ac_for_splitlock(void);
+
+/*
+ * @pre has_ac_for_splitlock() == true */ void
+enable_ac_for_splitlock(void); void disable_ac_for_splitlock(void);
+
#else /* ASSEMBLER defined */

#endif /* ASSEMBLER defined */
diff --git a/hypervisor/include/arch/x86/cpuid.h
b/hypervisor/include/arch/x86/cpuid.h
index 130b37b..ecdd224 100644
--- a/hypervisor/include/arch/x86/cpuid.h
+++ b/hypervisor/include/arch/x86/cpuid.h
@@ -84,6 +84,8 @@
#define CPUID_EDX_IBRS_IBPB (1U<<26U)
/* CPUID.07H:EDX.STIBP*/
#define CPUID_EDX_STIBP (1U<<27U)
+/* CPUID.07H:EDX.CORE_CAPABILITIES */
+#define CPUID_EDX_CORE_CAP (1U<<30U)
/* CPUID.80000001H:EDX.Page1GB*/
#define CPUID_EDX_PAGE1GB (1U<<26U)
/* CPUID.07H:EBX.INVPCID*/
diff --git a/hypervisor/include/arch/x86/msr.h
b/hypervisor/include/arch/x86/msr.h
index 70becbe..d311f8d 100644
--- a/hypervisor/include/arch/x86/msr.h
+++ b/hypervisor/include/arch/x86/msr.h
@@ -18,6 +18,7 @@
#define MSR_IA32_TIME_STAMP_COUNTER 0x00000010U
#define MSR_IA32_PLATFORM_ID 0x00000017U
#define MSR_IA32_APIC_BASE 0x0000001BU
+#define MSR_TEST_CTL 0x00000033U
#define MSR_IA32_FEATURE_CONTROL 0x0000003AU
#define MSR_IA32_TSC_ADJUST 0x0000003BU
/* Speculation Control */
@@ -40,6 +41,7 @@
#define MSR_IA32_PMC5 0x000000C6U
#define MSR_IA32_PMC6 0x000000C7U
#define MSR_IA32_PMC7 0x000000C8U
+#define MSR_IA32_CORE_CAPABILITIES 0x000000CFU
/* Max. qualified performance clock counter */
#define MSR_IA32_MPERF 0x000000E7U
/* Actual performance clock counter */
--
2.7.4



Tao, Yuhong
 

Hi, Eddie

-----Original Message-----
From: Tao, Yuhong
Sent: Tuesday, March 24, 2020 11:05 AM
To: Dong, Eddie <eddie.dong@intel.com>; acrn-dev@lists.projectacrn.org
Subject: RE: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC for
Splitlock Access

Hi, Eddie

-----Original Message-----
From: Dong, Eddie <eddie.dong@intel.com>
Sent: Tuesday, March 24, 2020 9:59 AM
To: acrn-dev@lists.projectacrn.org
Cc: Tao, Yuhong <yuhong.tao@intel.com>; Dong, Eddie
<eddie.dong@intel.com>
Subject: RE: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC
for Splitlock Access



-----Original Message-----
From: acrn-dev@lists.projectacrn.org
<acrn-dev@lists.projectacrn.org> On Behalf Of Tao, Yuhong
Sent: Monday, March 23, 2020 9:59 PM
To: acrn-dev@lists.projectacrn.org
Cc: Tao, Yuhong <yuhong.tao@intel.com>
Subject: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC
for Splitlock Access

When the destination of an atomic memory operation located in 2
cache lines, it is called a Splitlock Access. LOCK# bus signal is
asserted for splitlock access which can reduce performance. #AC for
Splitlock Access is a CPU feature, it
Can Reduce performance --> may lead to long latency.
Yes, will use it, thanks
allows rise alignment check exception #AC(0) instead of asserting
LOCK#, that is helpful to detect Splitlock Access.

Add 3 API:
has_ac_for_splitlock(): return true if CPU has this feature
enale_ac_for_splitlock(), disable_ac_for_splitlock(): enable/disable
this feature, these 2 APIs require precondition of
(has_ac_for_splitlock() == true)

If the CPU support this feature, then enable it at the primary CPU
early initialization stage, get ready to detect Splitlock Access in
HV

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/cpu.c | 42
+++++++++++++++++++++++++++++++++++++
hypervisor/arch/x86/init.c | 6 ++++++
hypervisor/include/arch/x86/cpu.h | 10 +++++++++
hypervisor/include/arch/x86/cpuid.h | 2 ++
hypervisor/include/arch/x86/msr.h | 2 ++
5 files changed, 62 insertions(+)

diff --git a/hypervisor/arch/x86/cpu.c b/hypervisor/arch/x86/cpu.c
index
384587c..f921722 100644
--- a/hypervisor/arch/x86/cpu.c
+++ b/hypervisor/arch/x86/cpu.c
@@ -572,3 +572,45 @@ uint64_t msr_read_pcpu(uint32_t msr_index,
uint16_t pcpu_id)

return ret;
}
+
+static bool ac_for_splitlock = false;
+
+inline bool has_ac_for_splitlock(void) {
+ return ac_for_splitlock;
+}
+
+void enable_ac_for_splitlock(void)
+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl |= (1U<<29U);
+ msr_write(MSR_TEST_CTL, test_ctl); }
+
+void disable_ac_for_splitlock(void)
Normally we do from top to bottom.
An API should be used first (called), and then implemented. Liu Yuan's
SR-IOV patch series do well.

For this time, I can merge, but not for next.
OK, thank you, but I can fix it this time.

+{
+ uint64_t test_ctl;
+
+ test_ctl = msr_read(MSR_TEST_CTL);
+ test_ctl &= (~(1U<<29U));
+ msr_write(MSR_TEST_CTL, test_ctl); }
+
+void init_ac_for_splitlock(void)
+{
+ uint32_t eax, ebx, ecx, edx;
+ uint64_t core_cap;
+
+ /* has MSR_IA32_CORE_CAPABILITIES? */
+ cpuid(CPUID_EXTEND_FEATURE, &eax, &ebx, &ecx, &edx);
FEAT_7_0_EDX already holds this, please use FEAT_7_0_EDX.
Yes, no need to run cupid() here, FEAT_7_0_EDX is OK


+ if ((edx & CPUID_EDX_CORE_CAP) != 0U) {
+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
No need to introduce ac_for_splitlock. Using FEAT_7_0_EDX, and
cpu_caps for #AC.
Yes, will use pcpu_has_cap (FEAT_7_0_EDX & 0x30U) to get this feature, do not
need ac_for_splitlock
Sorry, I have read the current code again, and think ac_for_splitlock has some advantage, can we keep it? The FEAT_7_0_EDX just show if MSR_IA32_CORE_CAPABILITIES is available, and still need to read MSR_IA32_CORE_CAPABILITIES to see if support #AC for splitlock. If keep this static variable , we can just detect this feature once.


+ enable_ac_for_splitlock();
+ }
+ }
+}
I thought we can go with one API only -- merge this with enable_xxx.
Yes, I think it can be:

void enable_ac_for_splitlock(bool ebable);

to enable this feature: enable_ac_for_splitlock(true) to disable this feature:
enable_ac_forsplitlock(false)

diff --git a/hypervisor/arch/x86/init.c b/hypervisor/arch/x86/init.c
index
b991797..229dc35 100644
--- a/hypervisor/arch/x86/init.c
+++ b/hypervisor/arch/x86/init.c
@@ -79,6 +79,8 @@ void init_primary_pcpu(void)
/* Clear BSS */
(void)memset(&ld_bss_start, 0U, (size_t)(&ld_bss_end -
&ld_bss_start));

+ init_ac_for_splitlock();
+
parse_hv_cmdline();

init_debug_pre();
@@ -95,6 +97,10 @@ void init_secondary_pcpu(void) {
uint16_t pcpu_id;

+ if (has_ac_for_splitlock() == true) {
+ enable_ac_for_splitlock();
+ }
+
init_pcpu_pre(false);

pcpu_id = get_pcpu_id();
diff --git a/hypervisor/include/arch/x86/cpu.h
b/hypervisor/include/arch/x86/cpu.h
index 27e9abd..617422d 100644
--- a/hypervisor/include/arch/x86/cpu.h
+++ b/hypervisor/include/arch/x86/cpu.h
@@ -685,6 +685,16 @@ static inline void clac(void) uint16_t
get_pcpu_nums(void); bool is_pcpu_active(uint16_t pcpu_id);
uint64_t get_active_pcpu_bitmap(void);
+
+void init_ac_for_splitlock(void);
+bool has_ac_for_splitlock(void);
+
+/*
+ * @pre has_ac_for_splitlock() == true */ void
+enable_ac_for_splitlock(void); void disable_ac_for_splitlock(void);
+
#else /* ASSEMBLER defined */

#endif /* ASSEMBLER defined */
diff --git a/hypervisor/include/arch/x86/cpuid.h
b/hypervisor/include/arch/x86/cpuid.h
index 130b37b..ecdd224 100644
--- a/hypervisor/include/arch/x86/cpuid.h
+++ b/hypervisor/include/arch/x86/cpuid.h
@@ -84,6 +84,8 @@
#define CPUID_EDX_IBRS_IBPB (1U<<26U)
/* CPUID.07H:EDX.STIBP*/
#define CPUID_EDX_STIBP (1U<<27U)
+/* CPUID.07H:EDX.CORE_CAPABILITIES */
+#define CPUID_EDX_CORE_CAP (1U<<30U)
/* CPUID.80000001H:EDX.Page1GB*/
#define CPUID_EDX_PAGE1GB (1U<<26U)
/* CPUID.07H:EBX.INVPCID*/
diff --git a/hypervisor/include/arch/x86/msr.h
b/hypervisor/include/arch/x86/msr.h
index 70becbe..d311f8d 100644
--- a/hypervisor/include/arch/x86/msr.h
+++ b/hypervisor/include/arch/x86/msr.h
@@ -18,6 +18,7 @@
#define MSR_IA32_TIME_STAMP_COUNTER 0x00000010U
#define MSR_IA32_PLATFORM_ID 0x00000017U
#define MSR_IA32_APIC_BASE 0x0000001BU
+#define MSR_TEST_CTL 0x00000033U
#define MSR_IA32_FEATURE_CONTROL 0x0000003AU
#define MSR_IA32_TSC_ADJUST 0x0000003BU
/* Speculation Control */
@@ -40,6 +41,7 @@
#define MSR_IA32_PMC5 0x000000C6U
#define MSR_IA32_PMC6 0x000000C7U
#define MSR_IA32_PMC7 0x000000C8U
+#define MSR_IA32_CORE_CAPABILITIES 0x000000CFU
/* Max. qualified performance clock counter */
#define MSR_IA32_MPERF 0x000000E7U
/* Actual performance clock counter */
--
2.7.4



Eddie Dong
 

+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
No need to introduce ac_for_splitlock. Using FEAT_7_0_EDX, and
cpu_caps for #AC.
Yes, will use pcpu_has_cap (FEAT_7_0_EDX & 0x30U) to get this feature,
do not need ac_for_splitlock
Sorry, I have read the current code again, and think ac_for_splitlock has some
advantage, can we keep it? The FEAT_7_0_EDX just show if
MSR_IA32_CORE_CAPABILITIES is available, and still need to read
MSR_IA32_CORE_CAPABILITIES to see if support #AC for splitlock. If keep this
static variable , we can just detect this feature once.
Use cpu_caps, not a separate variable...


Tao, Yuhong
 

Hi, Eddie

-----Original Message-----
From: Dong, Eddie <eddie.dong@intel.com>
Sent: Tuesday, March 24, 2020 5:43 PM
To: Tao, Yuhong <yuhong.tao@intel.com>; acrn-dev@lists.projectacrn.org
Cc: Dong, Eddie <eddie.dong@intel.com>
Subject: RE: [acrn-dev] [PATCH V2 1/6] HV: enumerate capability of #AC for
Splitlock Access

+ /* support #AC for splitlock access? */
+ core_cap = msr_read(MSR_IA32_CORE_CAPABILITIES);
+ if ((core_cap & (1U<<5U)) != 0U) {
+ ac_for_splitlock = true;
No need to introduce ac_for_splitlock. Using FEAT_7_0_EDX, and
cpu_caps for #AC.
Yes, will use pcpu_has_cap (FEAT_7_0_EDX & 0x30U) to get this
feature, do not need ac_for_splitlock
Sorry, I have read the current code again, and think ac_for_splitlock
has some advantage, can we keep it? The FEAT_7_0_EDX just show if
MSR_IA32_CORE_CAPABILITIES is available, and still need to read
MSR_IA32_CORE_CAPABILITIES to see if support #AC for splitlock. If
keep this static variable , we can just detect this feature once.
Use cpu_caps, not a separate variable...
Yes, you are right. Will add it into cpu_caps