[PATCH V2 2/6] HV: handle #AC for Splitlock Access #ac


Tao, Yuhong
 

In VMX root-mode, CPL != 3, #AC always rise for Splitlock Access.
HV report the rip of the instruction which triggers splitlock, and
temporally disable splitlock detection, allow the instruction to be
executed. A fix to the Splitlock Access is expected.

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/irq.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index ffc9150..1a02243 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -370,17 +370,24 @@ void dispatch_exception(struct intr_excp_ctx *ctx)
{
uint16_t pcpu_id = get_pcpu_id();

- /* Obtain lock to ensure exception dump doesn't get corrupted */
- spinlock_obtain(&exception_spinlock);
+ if (ctx->vector == IDT_AC) {
+ /* #AC in HV is always triggered by Splitlock Access */
+ pr_err("HV has detected Splitlock Access at CPU%d rip:0x%016llX\n"
+ "Temporally disable Splitlock Access", pcpu_id, ctx->rip);
+ disable_ac_for_splitlock();
+ } else {
+ /* Obtain lock to ensure exception dump doesn't get corrupted */
+ spinlock_obtain(&exception_spinlock);

- /* Dump exception context */
- dump_exception(ctx, pcpu_id);
+ /* Dump exception context */
+ dump_exception(ctx, pcpu_id);

- /* Release lock to let other CPUs handle exception */
- spinlock_release(&exception_spinlock);
+ /* Release lock to let other CPUs handle exception */
+ spinlock_release(&exception_spinlock);

- /* Halt the CPU */
- cpu_dead();
+ /* Halt the CPU */
+ cpu_dead();
+ }
}

void handle_nmi(__unused struct intr_excp_ctx *ctx)
--
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 2/6] HV: handle #AC for Splitlock Access

In VMX root-mode, CPL != 3, #AC always rise for Splitlock Access.
CPL != 3?

HV report the rip of the instruction which triggers splitlock, and temporally
disable splitlock detection, allow the instruction to be executed. A fix to the
If this is from user space, we may let the user space die to avoid impacting to HV.
If it is from HV, this is un-acceptable.
We should have @pre to validate this never happen.
ASSERT is fine too for debug mode to root cause easily.

Splitlock Access is expected.

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/irq.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c index
ffc9150..1a02243 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -370,17 +370,24 @@ void dispatch_exception(struct intr_excp_ctx *ctx)
{
uint16_t pcpu_id = get_pcpu_id();

- /* Obtain lock to ensure exception dump doesn't get corrupted */
- spinlock_obtain(&exception_spinlock);
+ if (ctx->vector == IDT_AC) {
+ /* #AC in HV is always triggered by Splitlock Access */
+ pr_err("HV has detected Splitlock Access at CPU%d
rip:0x%016llX\n"
+ "Temporally disable Splitlock Access", pcpu_id, ctx->rip);
+ disable_ac_for_splitlock();
+ } else {
+ /* Obtain lock to ensure exception dump doesn't get corrupted */
+ spinlock_obtain(&exception_spinlock);

- /* Dump exception context */
- dump_exception(ctx, pcpu_id);
+ /* Dump exception context */
+ dump_exception(ctx, pcpu_id);

- /* Release lock to let other CPUs handle exception */
- spinlock_release(&exception_spinlock);
+ /* Release lock to let other CPUs handle exception */
+ spinlock_release(&exception_spinlock);

- /* Halt the CPU */
- cpu_dead();
+ /* Halt the CPU */
+ cpu_dead();
+ }
}

void handle_nmi(__unused struct intr_excp_ctx *ctx)
--
2.7.4



Tao, Yuhong
 

Hi, Eddie

-----Original Message-----
From: Dong, Eddie <eddie.dong@intel.com>
Sent: Tuesday, March 24, 2020 10:05 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 2/6] HV: handle #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 2/6] HV: handle #AC for Splitlock Access

In VMX root-mode, CPL != 3, #AC always rise for Splitlock Access.
CPL != 3?
Sorry, it should be: HV always run at ring0, in which common alignment check exception is unavailable, #AC can only rise for splitlock access

HV report the rip of the instruction which triggers splitlock, and
temporally disable splitlock detection, allow the instruction to be
executed. A fix to the
If this is from user space, we may let the user space die to avoid impacting to HV.
Has discussed with Jason, no VM-Exit for #AC. If VM triggered #AC from userspace(common alignment check or splitlock), process will be terminated by SIGBUS. If VM trigger #AC from kernel, the highest severity VM can disable #AC for splitlock and continue run, other VM kernel can hang or oops. So the highest severity VM can impact to HV

If it is from HV, this is un-acceptable.
We should have @pre to validate this never happen.
ASSERT is fine too for debug mode to root cause easily.
The requirement is that HV can continue run when splitlock access happens, use ASSERT will stop HV. If do that, requirement should be changed.
If allow HV go though splitlock access, HV #AC handler disable this feature, and return to execute splitlock instruction. And we need to re-enable this feature somewhere. Use Assert will make it more simple, do not need to disable splitlock detection and re-enable it.

So, should make it clear if HV need to go on running when it has splitlock access?

Splitlock Access is expected.

Tracked-On: #4496
Signed-off-by: Tao Yuhong <yuhong.tao@intel.com>
---
hypervisor/arch/x86/irq.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hypervisor/arch/x86/irq.c b/hypervisor/arch/x86/irq.c
index
ffc9150..1a02243 100644
--- a/hypervisor/arch/x86/irq.c
+++ b/hypervisor/arch/x86/irq.c
@@ -370,17 +370,24 @@ void dispatch_exception(struct intr_excp_ctx
*ctx) {
uint16_t pcpu_id = get_pcpu_id();

- /* Obtain lock to ensure exception dump doesn't get corrupted */
- spinlock_obtain(&exception_spinlock);
+ if (ctx->vector == IDT_AC) {
+ /* #AC in HV is always triggered by Splitlock Access */
+ pr_err("HV has detected Splitlock Access at CPU%d
rip:0x%016llX\n"
+ "Temporally disable Splitlock Access", pcpu_id, ctx->rip);
+ disable_ac_for_splitlock();
+ } else {
+ /* Obtain lock to ensure exception dump doesn't get corrupted
*/
+ spinlock_obtain(&exception_spinlock);

- /* Dump exception context */
- dump_exception(ctx, pcpu_id);
+ /* Dump exception context */
+ dump_exception(ctx, pcpu_id);

- /* Release lock to let other CPUs handle exception */
- spinlock_release(&exception_spinlock);
+ /* Release lock to let other CPUs handle exception */
+ spinlock_release(&exception_spinlock);

- /* Halt the CPU */
- cpu_dead();
+ /* Halt the CPU */
+ cpu_dead();
+ }
}

void handle_nmi(__unused struct intr_excp_ctx *ctx)
--
2.7.4