[PATCH 0/2] Fix PC corruption when injecting a fault

[PATCH 0/2] Fix PC corruption when injecting a fault

[Marc Zyngier]

When injecting a fault as the result of a system register trap, we
change the PC to point to the fault handler. This clashes with the
code that increments the PC to skip over the emulated system register
access, leading to a situation where we skip the first instruction of
the fault handler.

The good news is that so far, we never do this, so I believe the
current code is safe. But the PMU code is soon going to exercise that
path, and I'd rather plug it sooner that later.

Thanks,

	M.

Marc Zyngier (2):
  arm: KVM: Do not update PC if the trap handler has updated it
  arm64: KVM: Do not update PC if the trap handler has updated it

 arch/arm/kvm/coproc.c     | 14 +++++++--
 arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------
 2 files changed, 48 insertions(+), 39 deletions(-)

-- 
2.1.4

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Marc Zyngier]

Assuming we trap a coprocessor access, and decide that the access
is illegal, we will inject an exception in the guest. In this
case, we shouldn't increment the PC, or the vcpu will miss the
first instruction of the handler, leading to a mildly confused
guest.

Solve this by snapshoting PC before the access is performed,
and checking if it has moved or not before incrementing it.

Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/coproc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
index f3d88dc..f4ad2f2 100644
--- a/arch/arm/kvm/coproc.c
+++ b/arch/arm/kvm/coproc.c
@@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
 		r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
 
 	if (likely(r)) {
+		unsigned long pc = *vcpu_pc(vcpu);
+
 		/* If we don't have an accessor, we should never get here! */
 		BUG_ON(!r->access);
 
 		if (likely(r->access(vcpu, params, r))) {
-			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+			/*
+			 * Skip the instruction if it was emulated
+			 * without PC having changed. This allows us
+			 * to detect a fault being injected
+			 * (incrementing the PC here would cause the
+			 * vcpu to skip the first instruction of its
+			 * fault handler).
+			 */
+			if (pc == *vcpu_pc(vcpu))
+				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			return 1;
 		}
 		/* If access function fails, it should complain. */
-- 
2.1.4

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Shannon Zhao]

On 2015/12/22 17:55, Marc Zyngier wrote:
> Assuming we trap a coprocessor access, and decide that the access
> is illegal, we will inject an exception in the guest. In this
> case, we shouldn't increment the PC, or the vcpu will miss the
> first instruction of the handler, leading to a mildly confused
> guest.
> 
> Solve this by snapshoting PC before the access is performed,
> and checking if it has moved or not before incrementing it.
> 
> Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm/kvm/coproc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index f3d88dc..f4ad2f2 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
>  		r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
>  
>  	if (likely(r)) {
> +		unsigned long pc = *vcpu_pc(vcpu);
> +
>  		/* If we don't have an accessor, we should never get here! */
>  		BUG_ON(!r->access);
>  
>  		if (likely(r->access(vcpu, params, r))) {
> -			/* Skip instruction, since it was emulated */
> -			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +			/*
> +			 * Skip the instruction if it was emulated
> +			 * without PC having changed. This allows us
> +			 * to detect a fault being injected
> +			 * (incrementing the PC here would cause the
> +			 * vcpu to skip the first instruction of its
> +			 * fault handler).
> +			 */
> +			if (pc == *vcpu_pc(vcpu))
> +				kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
>  			return 1;
>  		}
>  		/* If access function fails, it should complain. */
> 

-- 
Shannon

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Peter Maydell]

On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Assuming we trap a coprocessor access, and decide that the access
> is illegal, we will inject an exception in the guest. In this
> case, we shouldn't increment the PC, or the vcpu will miss the
> first instruction of the handler, leading to a mildly confused
> guest.
>
> Solve this by snapshoting PC before the access is performed,
> and checking if it has moved or not before incrementing it.
>
> Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kvm/coproc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> index f3d88dc..f4ad2f2 100644
> --- a/arch/arm/kvm/coproc.c
> +++ b/arch/arm/kvm/coproc.c
> @@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
>                 r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
>
>         if (likely(r)) {
> +               unsigned long pc = *vcpu_pc(vcpu);
> +
>                 /* If we don't have an accessor, we should never get here! */
>                 BUG_ON(!r->access);
>
>                 if (likely(r->access(vcpu, params, r))) {
> -                       /* Skip instruction, since it was emulated */
> -                       kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +                       /*
> +                        * Skip the instruction if it was emulated
> +                        * without PC having changed. This allows us
> +                        * to detect a fault being injected
> +                        * (incrementing the PC here would cause the
> +                        * vcpu to skip the first instruction of its
> +                        * fault handler).
> +                        */
> +                       if (pc == *vcpu_pc(vcpu))
> +                               kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));

Won't this result in our incorrectly skipping the first insn
in the fault handler if the original offending instruction
was itself the first insn in the fault handler?

thanks
-- PMM

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Christoffer Dall]

On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote:
> On 22 December 2015 at 09:55, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Assuming we trap a coprocessor access, and decide that the access
> > is illegal, we will inject an exception in the guest. In this
> > case, we shouldn't increment the PC, or the vcpu will miss the
> > first instruction of the handler, leading to a mildly confused
> > guest.
> >
> > Solve this by snapshoting PC before the access is performed,
> > and checking if it has moved or not before incrementing it.
> >
> > Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm/kvm/coproc.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> > index f3d88dc..f4ad2f2 100644
> > --- a/arch/arm/kvm/coproc.c
> > +++ b/arch/arm/kvm/coproc.c
> > @@ -447,12 +447,22 @@ static int emulate_cp15(struct kvm_vcpu *vcpu,
> >                 r = find_reg(params, cp15_regs, ARRAY_SIZE(cp15_regs));
> >
> >         if (likely(r)) {
> > +               unsigned long pc = *vcpu_pc(vcpu);
> > +
> >                 /* If we don't have an accessor, we should never get here! */
> >                 BUG_ON(!r->access);
> >
> >                 if (likely(r->access(vcpu, params, r))) {
> > -                       /* Skip instruction, since it was emulated */
> > -                       kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> > +                       /*
> > +                        * Skip the instruction if it was emulated
> > +                        * without PC having changed. This allows us
> > +                        * to detect a fault being injected
> > +                        * (incrementing the PC here would cause the
> > +                        * vcpu to skip the first instruction of its
> > +                        * fault handler).
> > +                        */
> > +                       if (pc == *vcpu_pc(vcpu))
> > +                               kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> 
> Won't this result in our incorrectly skipping the first insn
> in the fault handler if the original offending instruction
> was itself the first insn in the fault handler?
> 
Wouldn't that then loop with the exception forever?

-Christoffer

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Peter Maydell]

On 22 December 2015 at 14:39, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote:
>> Won't this result in our incorrectly skipping the first insn
>> in the fault handler if the original offending instruction
>> was itself the first insn in the fault handler?
>>
> Wouldn't that then loop with the exception forever?

Yes, but so would real hardware...

thanks
-- PMM

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Marc Zyngier]

On 22/12/15 14:50, Peter Maydell wrote:
> On 22 December 2015 at 14:39, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote:
>>> Won't this result in our incorrectly skipping the first insn
>>> in the fault handler if the original offending instruction
>>> was itself the first insn in the fault handler?
>>>
>> Wouldn't that then loop with the exception forever?
> 
> Yes, but so would real hardware...

Indeed. As it is, this patch is not doing what it should. On the other
hand, I came to the conclusion that we do not need to fix this just yet,
as long as we only let KVM inject an UNDEF, and that's what the PMU code
requires.

I'll comment on the PMU thread, but the gist of it is:
1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the
source EL (Table D1-7 of the ARMv8 ARM).
2) instead of crafting an exception that modifies the PC, fail the
sysreg access and let KVM inject an UNDEF.

I'll post another patch today to address 1), and I'll finish reviewing
the PMU thread (I have a separate patch addressing 2)).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Shannon Zhao]

On 2016/1/7 16:50, Marc Zyngier wrote:
> On 22/12/15 14:50, Peter Maydell wrote:
>> On 22 December 2015 at 14:39, Christoffer Dall
>> <christoffer.dall@linaro.org> wrote:
>>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote:
>>>> Won't this result in our incorrectly skipping the first insn
>>>> in the fault handler if the original offending instruction
>>>> was itself the first insn in the fault handler?
>>>>
>>> Wouldn't that then loop with the exception forever?
>>
>> Yes, but so would real hardware...
> 
> Indeed. As it is, this patch is not doing what it should. On the other
> hand, I came to the conclusion that we do not need to fix this just yet,
> as long as we only let KVM inject an UNDEF, and that's what the PMU code
> requires.
> 
> I'll comment on the PMU thread, but the gist of it is:
> 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the
> source EL (Table D1-7 of the ARMv8 ARM).
This looks like something we add in the PMU patch set.

+		switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) {
+		case PSR_MODE_EL0t:
+			exc_offset = EL0_EXCEPT_SYNC_OFFSET_64;
+			break;
+		case PSR_MODE_EL1t:
+			exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET;
+			break;
+		case PSR_MODE_EL1h:
+			exc_offset = EL1_EXCEPT_SYNC_OFFSET;
+			break;
+		default:
+			exc_offset = EL0_EXCEPT_SYNC_OFFSET_32;
+		}
+

> 2) instead of crafting an exception that modifies the PC, fail the
> sysreg access and let KVM inject an UNDEF.
> 
> I'll post another patch today to address 1), and I'll finish reviewing
> the PMU thread (I have a separate patch addressing 2)).
> 
Thanks!

-- 
Shannon

[PATCH 1/2] arm: KVM: Do not update PC if the trap handler has updated it

[Marc Zyngier]

On 07/01/16 08:59, Shannon Zhao wrote:
> 
> 
> On 2016/1/7 16:50, Marc Zyngier wrote:
>> On 22/12/15 14:50, Peter Maydell wrote:
>>> On 22 December 2015 at 14:39, Christoffer Dall
>>> <christoffer.dall@linaro.org> wrote:
>>>> On Tue, Dec 22, 2015 at 11:08:10AM +0000, Peter Maydell wrote:
>>>>> Won't this result in our incorrectly skipping the first insn
>>>>> in the fault handler if the original offending instruction
>>>>> was itself the first insn in the fault handler?
>>>>>
>>>> Wouldn't that then loop with the exception forever?
>>>
>>> Yes, but so would real hardware...
>>
>> Indeed. As it is, this patch is not doing what it should. On the other
>> hand, I came to the conclusion that we do not need to fix this just yet,
>> as long as we only let KVM inject an UNDEF, and that's what the PMU code
>> requires.
>>
>> I'll comment on the PMU thread, but the gist of it is:
>> 1) fix the arm64 UNDEF/PABRT/DABRT code to properly account for the the
>> source EL (Table D1-7 of the ARMv8 ARM).
> This looks like something we add in the PMU patch set.
> 
> +		switch (cpsr & (PSR_MODE_MASK | PSR_MODE32_BIT)) {
> +		case PSR_MODE_EL0t:
> +			exc_offset = EL0_EXCEPT_SYNC_OFFSET_64;
> +			break;
> +		case PSR_MODE_EL1t:
> +			exc_offset = EL1_EXCEPT_BAD_SYNC_OFFSET;
> +			break;
> +		case PSR_MODE_EL1h:
> +			exc_offset = EL1_EXCEPT_SYNC_OFFSET;
> +			break;
> +		default:
> +			exc_offset = EL0_EXCEPT_SYNC_OFFSET_32;
> +		}
> +

Indeed, plus some additional code to select the actual vector and not be
limited to a Synchronous exception (even if that's the only thing we use
today).

See the patch I've just posted for more details.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it

[Marc Zyngier]

Assuming we trap a system register, and decide that the access is
illegal, we will inject an exception in the guest. In this
case, we shouldn't increment the PC, or the vcpu will miss the
first instruction of the handler, leading to a mildly confused
guest.

Solve this by snapshoting PC before the access is performed,
and checking if it has moved or not before incrementing it.

Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d2650e8..9c87e0c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params,
 	return NULL;
 }
 
+/* Perform the sysreg access, returns 0 on success */
+static int access_sys_reg(struct kvm_vcpu *vcpu,
+			  struct sys_reg_params *params,
+			  const struct sys_reg_desc *r)
+{
+	u64 pc = *vcpu_pc(vcpu);
+
+	if (unlikely(!r))
+		return -1;
+
+	/*
+	 * Not having an accessor means that we have configured a trap
+	 * that we don't know how to handle. This certainly qualifies
+	 * as a gross bug that should be fixed right away.
+	 */
+	BUG_ON(!r->access);
+
+	if (likely(r->access(vcpu, params, r))) {
+		/*
+		 * Skip the instruction if it was emulated without PC
+		 * having changed. This allows us to detect a fault
+		 * being injected (incrementing the PC here would
+		 * cause the vcpu to skip the first instruction of its
+		 * fault handler).
+		 */
+		if (pc == *vcpu_pc(vcpu))
+			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
+		return 0;
+	}
+
+	return -1;
+}
+
 int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
 {
 	kvm_inject_undefined(vcpu);
@@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
 
 	r = find_reg(params, table, num);
 
-	if (r) {
-		/*
-		 * Not having an accessor means that we have
-		 * configured a trap that we don't know how to
-		 * handle. This certainly qualifies as a gross bug
-		 * that should be fixed right away.
-		 */
-		BUG_ON(!r->access);
-
-		if (likely(r->access(vcpu, params, r))) {
-			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-		}
-
-		/* Handled */
-		return 0;
-	}
-
-	/* Not handled */
-	return -1;
+	return access_sys_reg(vcpu, params, r);
 }
 
 static void unhandled_cp_access(struct kvm_vcpu *vcpu,
@@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 	if (!r)
 		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 
-	if (likely(r)) {
-		/*
-		 * Not having an accessor means that we have
-		 * configured a trap that we don't know how to
-		 * handle. This certainly qualifies as a gross bug
-		 * that should be fixed right away.
-		 */
-		BUG_ON(!r->access);
-
-		if (likely(r->access(vcpu, params, r))) {
-			/* Skip instruction, since it was emulated */
-			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
-			return 1;
-		}
-		/* If access function fails, it should complain. */
-	} else {
+	if (access_sys_reg(vcpu, params, r)) {
 		kvm_err("Unsupported guest sys_reg access at: %lx\n",
 			*vcpu_pc(vcpu));
 		print_sys_reg_instr(params);
+		kvm_inject_undefined(vcpu);
 	}
-	kvm_inject_undefined(vcpu);
 	return 1;
 }
 
-- 
2.1.4

[PATCH 2/2] arm64: KVM: Do not update PC if the trap handler has updated it

[Shannon Zhao]

On 2015/12/22 17:55, Marc Zyngier wrote:
> Assuming we trap a system register, and decide that the access is
> illegal, we will inject an exception in the guest. In this
> case, we shouldn't increment the PC, or the vcpu will miss the
> first instruction of the handler, leading to a mildly confused
> guest.
> 
> Solve this by snapshoting PC before the access is performed,
> and checking if it has moved or not before incrementing it.
> 
Thanks a lot. This solves the problem of guest PMU failing to inject EL1
fault to guest.

Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> Reported-by: Shannon Zhao <shannon.zhao@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 73 +++++++++++++++++++++++------------------------
>  1 file changed, 36 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d2650e8..9c87e0c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -966,6 +966,39 @@ static const struct sys_reg_desc *find_reg(const struct sys_reg_params *params,
>  	return NULL;
>  }
>  
> +/* Perform the sysreg access, returns 0 on success */
> +static int access_sys_reg(struct kvm_vcpu *vcpu,
> +			  struct sys_reg_params *params,
> +			  const struct sys_reg_desc *r)
> +{
> +	u64 pc = *vcpu_pc(vcpu);
> +
> +	if (unlikely(!r))
> +		return -1;
> +
> +	/*
> +	 * Not having an accessor means that we have configured a trap
> +	 * that we don't know how to handle. This certainly qualifies
> +	 * as a gross bug that should be fixed right away.
> +	 */
> +	BUG_ON(!r->access);
> +
> +	if (likely(r->access(vcpu, params, r))) {
> +		/*
> +		 * Skip the instruction if it was emulated without PC
> +		 * having changed. This allows us to detect a fault
> +		 * being injected (incrementing the PC here would
> +		 * cause the vcpu to skip the first instruction of its
> +		 * fault handler).
> +		 */
> +		if (pc == *vcpu_pc(vcpu))
> +			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
>  int kvm_handle_cp14_load_store(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  {
>  	kvm_inject_undefined(vcpu);
> @@ -994,26 +1027,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
>  
>  	r = find_reg(params, table, num);
>  
> -	if (r) {
> -		/*
> -		 * Not having an accessor means that we have
> -		 * configured a trap that we don't know how to
> -		 * handle. This certainly qualifies as a gross bug
> -		 * that should be fixed right away.
> -		 */
> -		BUG_ON(!r->access);
> -
> -		if (likely(r->access(vcpu, params, r))) {
> -			/* Skip instruction, since it was emulated */
> -			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> -		}
> -
> -		/* Handled */
> -		return 0;
> -	}
> -
> -	/* Not handled */
> -	return -1;
> +	return access_sys_reg(vcpu, params, r);
>  }
>  
>  static void unhandled_cp_access(struct kvm_vcpu *vcpu,
> @@ -1178,27 +1192,12 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>  	if (!r)
>  		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>  
> -	if (likely(r)) {
> -		/*
> -		 * Not having an accessor means that we have
> -		 * configured a trap that we don't know how to
> -		 * handle. This certainly qualifies as a gross bug
> -		 * that should be fixed right away.
> -		 */
> -		BUG_ON(!r->access);
> -
> -		if (likely(r->access(vcpu, params, r))) {
> -			/* Skip instruction, since it was emulated */
> -			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
> -			return 1;
> -		}
> -		/* If access function fails, it should complain. */
> -	} else {
> +	if (access_sys_reg(vcpu, params, r)) {
>  		kvm_err("Unsupported guest sys_reg access at: %lx\n",
>  			*vcpu_pc(vcpu));
>  		print_sys_reg_instr(params);
> +		kvm_inject_undefined(vcpu);
>  	}
> -	kvm_inject_undefined(vcpu);
>  	return 1;
>  }
>  
> 

-- 
Shannon