[PULL 0/4] ppc patch queue 2012-12-18

[PULL 0/4] ppc patch queue 2012-12-18

[Alexander Graf]

Hi Marcelo / Gleb,

This is my current patch queue for ppc.  Please pull.

Changes include:

  - important header fix for UAPI
  - Book3S: PR: Enable fake sc 1 instruction for KVM on top of pHyp

Alex


The following changes since commit e11ae1a102b46f76441e328a2743ae5d6e201423:
  Gleb Natapov (1):
        KVM: remove unused variable.

are available in the git repository at:

  git://github.com/agraf/linux-2.6.git kvm-ppc-next

Alexander Graf (2):
      KVM: PPC: Only WARN on invalid emulation
      KVM: PPC: Book3S: PR: Enable alternative instruction for SC 1

Bharat Bhushan (1):
      powerpc: Corrected include header path in kvm_para.h

Mihai Caraman (1):
      KVM: PPC: Fix SREGS documentation reference

 Documentation/virtual/kvm/api.txt        |    2 +-
 arch/powerpc/include/asm/kvm_ppc.h       |    1 +
 arch/powerpc/include/uapi/asm/kvm_para.h |    2 +-
 arch/powerpc/kvm/book3s_emulate.c        |   28 ++++++++++++++++++++++++++++
 arch/powerpc/kvm/book3s_pr.c             |    5 +++++
 arch/powerpc/kvm/powerpc.c               |    3 ++-
 6 files changed, 38 insertions(+), 3 deletions(-)

[PATCH 1/4] KVM: PPC: Fix SREGS documentation reference

[Alexander Graf]

From: Mihai Caraman <mihai.caraman@freescale.com>

Reflect the uapi folder change in SREGS API documentation.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
Reviewed-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Documentation/virtual/kvm/api.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a4df553..9cf591d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -345,7 +345,7 @@ struct kvm_sregs {
 	__u64 interrupt_bitmap[(KVM_NR_INTERRUPTS + 63) / 64];
 };
 
-/* ppc -- see arch/powerpc/include/asm/kvm.h */
+/* ppc -- see arch/powerpc/include/uapi/asm/kvm.h */
 
 interrupt_bitmap is a bitmap of pending external interrupts.  At most
 one bit may be set.  This interrupt has been acknowledged by the APIC
-- 
1.6.0.2

[PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Alexander Graf]

When we hit an emulation result that we didn't expect, that is an error,
but it's nothing that warrants a BUG(), because it can be guest triggered.

So instead, let's only WARN() the user that this happened.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/powerpc.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index be83fca..e2225e5 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
 		r = RESUME_HOST;
 		break;
 	default:
-		BUG();
+		WARN_ON(1);
+		r = RESUME_GUEST;
 	}
 
 	return r;
-- 
1.6.0.2

[PATCH 3/4] KVM: PPC: Book3S: PR: Enable alternative instruction for SC 1

[Alexander Graf]

When running on top of pHyp, the hypercall instruction "sc 1" goes
straight into pHyp without trapping in supervisor mode.

So if we want to support PAPR guest in this configuration we need to
add a second way of accessing PAPR hypercalls, preferably with the
exact same semantics except for the instruction.

So let's overlay an officially reserved instruction and emulate PAPR
hypercalls whenever we hit that one.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/kvm_ppc.h |    1 +
 arch/powerpc/kvm/book3s_emulate.c  |   28 ++++++++++++++++++++++++++++
 arch/powerpc/kvm/book3s_pr.c       |    5 +++++
 3 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 572aa75..5f5f69a 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -44,6 +44,7 @@ enum emulation_result {
 	EMULATE_DO_DCR,       /* kvm_run filled with DCR request */
 	EMULATE_FAIL,         /* can't emulate this instruction */
 	EMULATE_AGAIN,        /* something went wrong. go again */
+	EMULATE_DO_PAPR,      /* kvm_run filled with PAPR request */
 };
 
 extern int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu);
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index d31a716..c88161b 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -34,6 +34,8 @@
 #define OP_31_XOP_MTSRIN	242
 #define OP_31_XOP_TLBIEL	274
 #define OP_31_XOP_TLBIE		306
+/* Opcode is officially reserved, reuse it as sc 1 when sc 1 doesn't trap */
+#define OP_31_XOP_FAKE_SC1	308
 #define OP_31_XOP_SLBMTE	402
 #define OP_31_XOP_SLBIE		434
 #define OP_31_XOP_SLBIA		498
@@ -170,6 +172,32 @@ int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			vcpu->arch.mmu.tlbie(vcpu, addr, large);
 			break;
 		}
+#ifdef CONFIG_KVM_BOOK3S_64_PR
+		case OP_31_XOP_FAKE_SC1:
+		{
+			/* SC 1 papr hypercalls */
+			ulong cmd = kvmppc_get_gpr(vcpu, 3);
+			int i;
+
+		        if ((vcpu->arch.shared->msr & MSR_PR) ||
+			    !vcpu->arch.papr_enabled) {
+				emulated = EMULATE_FAIL;
+				break;
+			}
+
+			if (kvmppc_h_pr(vcpu, cmd) == EMULATE_DONE)
+				break;
+
+			run->papr_hcall.nr = cmd;
+			for (i = 0; i < 9; ++i) {
+				ulong gpr = kvmppc_get_gpr(vcpu, 4 + i);
+				run->papr_hcall.args[i] = gpr;
+			}
+
+			emulated = EMULATE_DO_PAPR;
+			break;
+		}
+#endif
 		case OP_31_XOP_EIOIO:
 			break;
 		case OP_31_XOP_SLBMTE:
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 28d38ad..73ed11c 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -760,6 +760,11 @@ program_interrupt:
 			run->exit_reason = KVM_EXIT_MMIO;
 			r = RESUME_HOST_NV;
 			break;
+		case EMULATE_DO_PAPR:
+			run->exit_reason = KVM_EXIT_PAPR_HCALL;
+			vcpu->arch.hcall_needed = 1;
+			r = RESUME_HOST_NV;
+			break;
 		default:
 			BUG();
 		}
-- 
1.6.0.2

[PATCH 4/4] powerpc: Corrected include header path in kvm_para.h

[Alexander Graf]

From: Bharat Bhushan <r65777@freescale.com>

The include/uapi/asm/kvm_para.h includes
<include/uapi/asm/epapr_hcalls.h> but the correct reference
should be <include/asm/epapr_hcalls.h> as this is the place
where make install_header installs the header files for
userspace.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
CC: stable@vger.kernel.org
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/uapi/asm/kvm_para.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/uapi/asm/kvm_para.h b/arch/powerpc/include/uapi/asm/kvm_para.h
index ed0e025..e3af328 100644
--- a/arch/powerpc/include/uapi/asm/kvm_para.h
+++ b/arch/powerpc/include/uapi/asm/kvm_para.h
@@ -78,7 +78,7 @@ struct kvm_vcpu_arch_shared {
 
 #define KVM_HCALL_TOKEN(num)     _EV_HCALL_TOKEN(EV_KVM_VENDOR_ID, num)
 
-#include <uapi/asm/epapr_hcalls.h>
+#include <asm/epapr_hcalls.h>
 
 #define KVM_FEATURE_MAGIC_PAGE	1
 
-- 
1.6.0.2

Re: [PULL 0/4] ppc patch queue 2012-12-18

[Gleb Natapov]

On Tue, Dec 18, 2012 at 01:38:39PM +0100, Alexander Graf wrote:
> Hi Marcelo / Gleb,
> 
> This is my current patch queue for ppc.  Please pull.
> 
Is this for 3.8 or 3.9?

> Changes include:
> 
>   - important header fix for UAPI
>   - Book3S: PR: Enable fake sc 1 instruction for KVM on top of pHyp
> 
> Alex
> 
> 
> The following changes since commit e11ae1a102b46f76441e328a2743ae5d6e201423:
>   Gleb Natapov (1):
>         KVM: remove unused variable.
> 
> are available in the git repository at:
> 
>   git://github.com/agraf/linux-2.6.git kvm-ppc-next
> 
> Alexander Graf (2):
>       KVM: PPC: Only WARN on invalid emulation
>       KVM: PPC: Book3S: PR: Enable alternative instruction for SC 1
> 
> Bharat Bhushan (1):
>       powerpc: Corrected include header path in kvm_para.h
> 
> Mihai Caraman (1):
>       KVM: PPC: Fix SREGS documentation reference
> 
>  Documentation/virtual/kvm/api.txt        |    2 +-
>  arch/powerpc/include/asm/kvm_ppc.h       |    1 +
>  arch/powerpc/include/uapi/asm/kvm_para.h |    2 +-
>  arch/powerpc/kvm/book3s_emulate.c        |   28 ++++++++++++++++++++++++++++
>  arch/powerpc/kvm/book3s_pr.c             |    5 +++++
>  arch/powerpc/kvm/powerpc.c               |    3 ++-
>  6 files changed, 38 insertions(+), 3 deletions(-)

--
			Gleb.

Re: [PULL 0/4] ppc patch queue 2012-12-18

[Alexander Graf]

On 12/18/2012 04:20 PM, Gleb Natapov wrote:
> On Tue, Dec 18, 2012 at 01:38:39PM +0100, Alexander Graf wrote:
>> Hi Marcelo / Gleb,
>>
>> This is my current patch queue for ppc.  Please pull.
>>
> Is this for 3.8 or 3.9?

This is for 3.9, except for the UAPI one. That one should go into 3.8 
and stable.


Alex

>
>> Changes include:
>>
>>    - important header fix for UAPI
>>    - Book3S: PR: Enable fake sc 1 instruction for KVM on top of pHyp
>>
>> Alex
>>
>>
>> The following changes since commit e11ae1a102b46f76441e328a2743ae5d6e201423:
>>    Gleb Natapov (1):
>>          KVM: remove unused variable.
>>
>> are available in the git repository at:
>>
>>    git://github.com/agraf/linux-2.6.git kvm-ppc-next
>>
>> Alexander Graf (2):
>>        KVM: PPC: Only WARN on invalid emulation
>>        KVM: PPC: Book3S: PR: Enable alternative instruction for SC 1
>>
>> Bharat Bhushan (1):
>>        powerpc: Corrected include header path in kvm_para.h
>>
>> Mihai Caraman (1):
>>        KVM: PPC: Fix SREGS documentation reference
>>
>>   Documentation/virtual/kvm/api.txt        |    2 +-
>>   arch/powerpc/include/asm/kvm_ppc.h       |    1 +
>>   arch/powerpc/include/uapi/asm/kvm_para.h |    2 +-
>>   arch/powerpc/kvm/book3s_emulate.c        |   28 ++++++++++++++++++++++++++++
>>   arch/powerpc/kvm/book3s_pr.c             |    5 +++++
>>   arch/powerpc/kvm/powerpc.c               |    3 ++-
>>   6 files changed, 38 insertions(+), 3 deletions(-)
> --
> 			Gleb.

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Scott Wood]

On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
> When we hit an emulation result that we didn't expect, that is an  
> error,
> but it's nothing that warrants a BUG(), because it can be guest  
> triggered.
> 
> So instead, let's only WARN() the user that this happened.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/powerpc.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index be83fca..e2225e5 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run,  
> struct kvm_vcpu *vcpu)
>  		r = RESUME_HOST;
>  		break;
>  	default:
> -		BUG();
> +		WARN_ON(1);
> +		r = RESUME_GUEST;

Do you have a specific way of a guest triggering this in mind, or is it  
just being cautious?  The guest probably shouldn't be allowed to spam  
the kernel log with WARNs either.  Is a traceback even useful here?

-Scott

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Alexander Graf]

On 18.12.2012, at 23:54, Scott Wood wrote:

> On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
>> When we hit an emulation result that we didn't expect, that is an error,
>> but it's nothing that warrants a BUG(), because it can be guest triggered.
>> So instead, let's only WARN() the user that this happened.
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/powerpc.c |    3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index be83fca..e2225e5 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
>> 		r = RESUME_HOST;
>> 		break;
>> 	default:
>> -		BUG();
>> +		WARN_ON(1);
>> +		r = RESUME_GUEST;
> 
> Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?

For debugging, yes. But maybe we would be better off with a trace point. Anyway, a WARN is better than a BUG either way for now.

I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.


Alex

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Scott Wood]

On 12/18/2012 05:01:19 PM, Alexander Graf wrote:
> 
> On 18.12.2012, at 23:54, Scott Wood wrote:
> 
> > On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
> >> When we hit an emulation result that we didn't expect, that is an  
> error,
> >> but it's nothing that warrants a BUG(), because it can be guest  
> triggered.
> >> So instead, let's only WARN() the user that this happened.
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> arch/powerpc/kvm/powerpc.c |    3 ++-
> >> 1 files changed, 2 insertions(+), 1 deletions(-)
> >> diff --git a/arch/powerpc/kvm/powerpc.c  
> b/arch/powerpc/kvm/powerpc.c
> >> index be83fca..e2225e5 100644
> >> --- a/arch/powerpc/kvm/powerpc.c
> >> +++ b/arch/powerpc/kvm/powerpc.c
> >> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run,  
> struct kvm_vcpu *vcpu)
> >> 		r = RESUME_HOST;
> >> 		break;
> >> 	default:
> >> -		BUG();
> >> +		WARN_ON(1);
> >> +		r = RESUME_GUEST;
> >
> > Do you have a specific way of a guest triggering this in mind, or  
> is it just being cautious?  The guest probably shouldn't be allowed  
> to spam the kernel log with WARNs either.  Is a traceback even useful  
> here?
> 
> For debugging, yes.

I figured the interesting bits would be in the stack frames you've just  
returned from.

> But maybe we would be better off with a trace point.

Or a ratelimited error message, and maybe we should return to host  
instead of guest?

> Anyway, a WARN is better than a BUG either way for now.

Yes...

> I was able to provoke this by live patching an instruction without  
> flushing the icache, so that the last_inst instruction fetch gets a  
> different instruction from the instruction that resulted in the trap  
> we're currently in.

Which EMULATE code did you get in that case?

-Scott

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Alexander Graf]

On 19.12.2012, at 00:05, Scott Wood wrote:

> On 12/18/2012 05:01:19 PM, Alexander Graf wrote:
>> On 18.12.2012, at 23:54, Scott Wood wrote:
>> > On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
>> >> When we hit an emulation result that we didn't expect, that is an error,
>> >> but it's nothing that warrants a BUG(), because it can be guest triggered.
>> >> So instead, let's only WARN() the user that this happened.
>> >> Signed-off-by: Alexander Graf <agraf@suse.de>
>> >> ---
>> >> arch/powerpc/kvm/powerpc.c |    3 ++-
>> >> 1 files changed, 2 insertions(+), 1 deletions(-)
>> >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> >> index be83fca..e2225e5 100644
>> >> --- a/arch/powerpc/kvm/powerpc.c
>> >> +++ b/arch/powerpc/kvm/powerpc.c
>> >> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
>> >> 		r = RESUME_HOST;
>> >> 		break;
>> >> 	default:
>> >> -		BUG();
>> >> +		WARN_ON(1);
>> >> +		r = RESUME_GUEST;
>> >
>> > Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?
>> For debugging, yes.
> 
> I figured the interesting bits would be in the stack frames you've just returned from.
> 
>> But maybe we would be better off with a trace point.
> 
> Or a ratelimited error message, and maybe we should return to host instead of guest?
> 
>> Anyway, a WARN is better than a BUG either way for now.
> 
> Yes...
> 
>> I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.
> 
> Which EMULATE code did you get in that case?

I was patching SC into the PAPR_SC illegal instruction. So I got the EMULATE_PAPR code.

Maybe we should simply always handle all EMULATE codes everywhere. Then the whole problem goes away automatically :).


Alex

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Gleb Natapov]

On Wed, Dec 19, 2012 at 12:01:19AM +0100, Alexander Graf wrote:
> 
> On 18.12.2012, at 23:54, Scott Wood wrote:
> 
> > On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
> >> When we hit an emulation result that we didn't expect, that is an error,
> >> but it's nothing that warrants a BUG(), because it can be guest triggered.
> >> So instead, let's only WARN() the user that this happened.
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> arch/powerpc/kvm/powerpc.c |    3 ++-
> >> 1 files changed, 2 insertions(+), 1 deletions(-)
> >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >> index be83fca..e2225e5 100644
> >> --- a/arch/powerpc/kvm/powerpc.c
> >> +++ b/arch/powerpc/kvm/powerpc.c
> >> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
> >> 		r = RESUME_HOST;
> >> 		break;
> >> 	default:
> >> -		BUG();
> >> +		WARN_ON(1);
> >> +		r = RESUME_GUEST;
> > 
> > Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?
> 
> For debugging, yes. But maybe we would be better off with a trace point. Anyway, a WARN is better than a BUG either way for now.
> 
> I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.
> 
If guest can trigger this it better be WARN_ON_ONCE(). Otherwise, as
Scott said, guest will be able to spam host kernel log.

--
			Gleb.

Re: [PULL 0/4] ppc patch queue 2012-12-18

[Gleb Natapov]

On Tue, Dec 18, 2012 at 05:35:57PM +0100, Alexander Graf wrote:
> On 12/18/2012 04:20 PM, Gleb Natapov wrote:
> >On Tue, Dec 18, 2012 at 01:38:39PM +0100, Alexander Graf wrote:
> >>Hi Marcelo / Gleb,
> >>
> >>This is my current patch queue for ppc.  Please pull.
> >>
> >Is this for 3.8 or 3.9?
> 
> This is for 3.9, except for the UAPI one. That one should go into
> 3.8 and stable.
> 
It is good to have this information with pull request. Also patches for
3.8 go to master branch while patches for 3.9 go to next branch, so you
need to prepare separate pull requests for two of those.

> 
> Alex
> 
> >
> >>Changes include:
> >>
> >>   - important header fix for UAPI
> >>   - Book3S: PR: Enable fake sc 1 instruction for KVM on top of pHyp
> >>
> >>Alex
> >>
> >>
> >>The following changes since commit e11ae1a102b46f76441e328a2743ae5d6e201423:
> >>   Gleb Natapov (1):
> >>         KVM: remove unused variable.
> >>
> >>are available in the git repository at:
> >>
> >>   git://github.com/agraf/linux-2.6.git kvm-ppc-next
> >>
> >>Alexander Graf (2):
> >>       KVM: PPC: Only WARN on invalid emulation
> >>       KVM: PPC: Book3S: PR: Enable alternative instruction for SC 1
> >>
> >>Bharat Bhushan (1):
> >>       powerpc: Corrected include header path in kvm_para.h
> >>
> >>Mihai Caraman (1):
> >>       KVM: PPC: Fix SREGS documentation reference
> >>
> >>  Documentation/virtual/kvm/api.txt        |    2 +-
> >>  arch/powerpc/include/asm/kvm_ppc.h       |    1 +
> >>  arch/powerpc/include/uapi/asm/kvm_para.h |    2 +-
> >>  arch/powerpc/kvm/book3s_emulate.c        |   28 ++++++++++++++++++++++++++++
> >>  arch/powerpc/kvm/book3s_pr.c             |    5 +++++
> >>  arch/powerpc/kvm/powerpc.c               |    3 ++-
> >>  6 files changed, 38 insertions(+), 3 deletions(-)
> >--
> >			Gleb.

--
			Gleb.

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Alexander Graf]

On 19.12.2012, at 10:37, Gleb Natapov wrote:

> On Wed, Dec 19, 2012 at 12:01:19AM +0100, Alexander Graf wrote:
>> 
>> On 18.12.2012, at 23:54, Scott Wood wrote:
>> 
>>> On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
>>>> When we hit an emulation result that we didn't expect, that is an error,
>>>> but it's nothing that warrants a BUG(), because it can be guest triggered.
>>>> So instead, let's only WARN() the user that this happened.
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> arch/powerpc/kvm/powerpc.c |    3 ++-
>>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>>> index be83fca..e2225e5 100644
>>>> --- a/arch/powerpc/kvm/powerpc.c
>>>> +++ b/arch/powerpc/kvm/powerpc.c
>>>> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
>>>> 		r = RESUME_HOST;
>>>> 		break;
>>>> 	default:
>>>> -		BUG();
>>>> +		WARN_ON(1);
>>>> +		r = RESUME_GUEST;
>>> 
>>> Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?
>> 
>> For debugging, yes. But maybe we would be better off with a trace point. Anyway, a WARN is better than a BUG either way for now.
>> 
>> I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.
>> 
> If guest can trigger this it better be WARN_ON_ONCE(). Otherwise, as
> Scott said, guest will be able to spam host kernel log.

I really think eventually we want a trace point and no WARN at all or all possible EMULATE targets handled, because a guest can legitimately not flush its icache and thus confuse our logic.

Just consider this patch as a quick fix to make sure we enable people (me) to unload the module still after they hit this case ;). Real fix coming soon.


Alex

Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Gleb Natapov]

On Wed, Dec 19, 2012 at 10:59:28AM +0100, Alexander Graf wrote:
> 
> On 19.12.2012, at 10:37, Gleb Natapov wrote:
> 
> > On Wed, Dec 19, 2012 at 12:01:19AM +0100, Alexander Graf wrote:
> >> 
> >> On 18.12.2012, at 23:54, Scott Wood wrote:
> >> 
> >>> On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
> >>>> When we hit an emulation result that we didn't expect, that is an error,
> >>>> but it's nothing that warrants a BUG(), because it can be guest triggered.
> >>>> So instead, let's only WARN() the user that this happened.
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>> arch/powerpc/kvm/powerpc.c |    3 ++-
> >>>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >>>> index be83fca..e2225e5 100644
> >>>> --- a/arch/powerpc/kvm/powerpc.c
> >>>> +++ b/arch/powerpc/kvm/powerpc.c
> >>>> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
> >>>> 		r = RESUME_HOST;
> >>>> 		break;
> >>>> 	default:
> >>>> -		BUG();
> >>>> +		WARN_ON(1);
> >>>> +		r = RESUME_GUEST;
> >>> 
> >>> Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?
> >> 
> >> For debugging, yes. But maybe we would be better off with a trace point. Anyway, a WARN is better than a BUG either way for now.
> >> 
> >> I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.
> >> 
> > If guest can trigger this it better be WARN_ON_ONCE(). Otherwise, as
> > Scott said, guest will be able to spam host kernel log.
> 
> I really think eventually we want a trace point and no WARN at all or all possible EMULATE targets handled, because a guest can legitimately not flush its icache and thus confuse our logic.
> 
> Just consider this patch as a quick fix to make sure we enable people (me) to unload the module still after they hit this case ;). Real fix coming soon.
> 
OK.

--
			Gleb.