Re: [PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-26 Thread Hollis Blanchard 
On Mon, Jan 25, 2010 at 3:32 AM, Liu Yu-B13201 b13...@freescale.com wrote:

 -Original Message-
 From: Alexander Graf [mailto:ag...@suse.de]
 Sent: Friday, January 22, 2010 7:33 PM
 To: Liu Yu-B13201
 Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org;
 k...@vger.kernel.org
 Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when
 inject interrupt to guest


 On 22.01.2010, at 12:27, Liu Yu-B13201 wrote:

 
 
  -Original Message-
  From: kvm-ppc-ow...@vger.kernel.org
  [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
  Sent: Friday, January 22, 2010 7:13 PM
  To: Liu Yu-B13201
  Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org;
  k...@vger.kernel.org
  Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when
  inject interrupt to guest
 
 
  On 22.01.2010, at 11:54, Liu Yu wrote:
 
  Old method prematurely sets ESR and DEAR.
  Move this part after we decide to inject interrupt,
  and make it more like hardware behave.
 
  Signed-off-by: Liu Yu yu@freescale.com
  ---
  arch/powerpc/kvm/booke.c   |   24 ++--
  arch/powerpc/kvm/emulate.c |    2 --
  2 files changed, 14 insertions(+), 12 deletions(-)
 
  @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run
  *run, struct kvm_vcpu *vcpu,
            break;
 
    case BOOKE_INTERRUPT_DATA_STORAGE:
  -         vcpu-arch.dear = vcpu-arch.fault_dear;
  -         vcpu-arch.esr = vcpu-arch.fault_esr;
            kvmppc_booke_queue_irqprio(vcpu,
  BOOKE_IRQPRIO_DATA_STORAGE);
 
  kvmppc_booke_queue_data_storage(vcpu, vcpu-arch.fault_esr,
  vcpu-arch.fault_dear);
 
            kvmppc_account_exit(vcpu, DSI_EXITS);
            r = RESUME_GUEST;
            break;
 
    case BOOKE_INTERRUPT_INST_STORAGE:
  -         vcpu-arch.esr = vcpu-arch.fault_esr;
            kvmppc_booke_queue_irqprio(vcpu,
  BOOKE_IRQPRIO_INST_STORAGE);
 
  kvmppc_booke_queue_inst_storage(vcpu, vcpu-arch.fault_esr);
 
 
  Not sure if this is redundant, as we already have fault_esr.
  Or should we ignore what hareware is and create a new esr to guest?

 On Book3S I take the SRR1 we get from the host as
 inspiration of what to pass to the guest as SRR1. I think
 we should definitely be able to inject a fault that we didn't
 get in that exact form from the exit path.

 I'm also not sure if something could clobber fault_esr if
 another interrupt takes precedence. Say a #MC.

 No as far as I know.
 And if yes, the clobber could as well happen before we copy it.
 Hollis, what do you think we should do here?

I'm torn, and in some ways it's not that important right now. However,
I think it makes sense to add something like vcpu-queued_esr as a
separate field from vcpu-fault_esr. The use case I'm thinking of is a
debugger wanting to invoke guest kernel to provide a translation for
an address not currently present in the TLB (i.e. not currently
visible to the debugger).

-Hollis
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-25 Thread Liu Yu-B13201 
 

 -Original Message-
 From: Alexander Graf [mailto:ag...@suse.de] 
 Sent: Friday, January 22, 2010 7:33 PM
 To: Liu Yu-B13201
 Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org; 
 k...@vger.kernel.org
 Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when 
 inject interrupt to guest
 
 
 On 22.01.2010, at 12:27, Liu Yu-B13201 wrote:
 
  
  
  -Original Message-
  From: kvm-ppc-ow...@vger.kernel.org 
  [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
  Sent: Friday, January 22, 2010 7:13 PM
  To: Liu Yu-B13201
  Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org; 
  k...@vger.kernel.org
  Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when 
  inject interrupt to guest
  
  
  On 22.01.2010, at 11:54, Liu Yu wrote:
  
  Old method prematurely sets ESR and DEAR.
  Move this part after we decide to inject interrupt,
  and make it more like hardware behave.
  
  Signed-off-by: Liu Yu yu@freescale.com
  ---
  arch/powerpc/kvm/booke.c   |   24 ++--
  arch/powerpc/kvm/emulate.c |2 --
  2 files changed, 14 insertions(+), 12 deletions(-)
  
  @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run 
  *run, struct kvm_vcpu *vcpu,
break;
  
case BOOKE_INTERRUPT_DATA_STORAGE:
  - vcpu-arch.dear = vcpu-arch.fault_dear;
  - vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_booke_queue_irqprio(vcpu, 
  BOOKE_IRQPRIO_DATA_STORAGE);
  
  kvmppc_booke_queue_data_storage(vcpu, vcpu-arch.fault_esr, 
  vcpu-arch.fault_dear);
  
kvmppc_account_exit(vcpu, DSI_EXITS);
r = RESUME_GUEST;
break;
  
case BOOKE_INTERRUPT_INST_STORAGE:
  - vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_booke_queue_irqprio(vcpu, 
  BOOKE_IRQPRIO_INST_STORAGE);
  
  kvmppc_booke_queue_inst_storage(vcpu, vcpu-arch.fault_esr);
  
  
  Not sure if this is redundant, as we already have fault_esr.
  Or should we ignore what hareware is and create a new esr to guest?
 
 On Book3S I take the SRR1 we get from the host as 
 inspiration of what to pass to the guest as SRR1. I think 
 we should definitely be able to inject a fault that we didn't 
 get in that exact form from the exit path.
 
 I'm also not sure if something could clobber fault_esr if 
 another interrupt takes precedence. Say a #MC.

No as far as I know.
And if yes, the clobber could as well happen before we copy it.
Hollis, what do you think we should do here?


--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-22 Thread Liu Yu 
Old method prematurely sets ESR and DEAR.
Move this part after we decide to inject interrupt,
and make it more like hardware behave.

Signed-off-by: Liu Yu yu@freescale.com
---
 arch/powerpc/kvm/booke.c   |   24 ++--
 arch/powerpc/kvm/emulate.c |2 --
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index e283e44..a8ee148 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -84,7 +84,8 @@ static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
 
 void kvmppc_core_queue_program(struct kvm_vcpu *vcpu, ulong flags)
 {
-   /* BookE does flags in ESR, so ignore those we get here */
+   if (flags == SRR1_PROGTRAP)
+   vcpu-arch.fault_esr = ESR_PTR;
kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
 }
 
@@ -115,14 +116,19 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu 
*vcpu,
 {
int allowed = 0;
ulong msr_mask;
+   bool update_esr = false, update_dear = false;
 
switch (priority) {
-   case BOOKE_IRQPRIO_PROGRAM:
case BOOKE_IRQPRIO_DTLB_MISS:
-   case BOOKE_IRQPRIO_ITLB_MISS:
-   case BOOKE_IRQPRIO_SYSCALL:
case BOOKE_IRQPRIO_DATA_STORAGE:
+   update_dear = true;
+   /* fall through */
case BOOKE_IRQPRIO_INST_STORAGE:
+   case BOOKE_IRQPRIO_PROGRAM:
+   update_esr = true;
+   /* fall through */
+   case BOOKE_IRQPRIO_ITLB_MISS:
+   case BOOKE_IRQPRIO_SYSCALL:
case BOOKE_IRQPRIO_FP_UNAVAIL:
case BOOKE_IRQPRIO_SPE_UNAVAIL:
case BOOKE_IRQPRIO_SPE_FP_DATA:
@@ -157,6 +163,10 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu 
*vcpu,
vcpu-arch.srr0 = vcpu-arch.pc;
vcpu-arch.srr1 = vcpu-arch.msr;
vcpu-arch.pc = vcpu-arch.ivpr | vcpu-arch.ivor[priority];
+   if (update_esr == true)
+   vcpu-arch.esr = vcpu-arch.fault_esr;
+   if (update_dear == true)
+   vcpu-arch.dear = vcpu-arch.fault_dear;
kvmppc_set_msr(vcpu, vcpu-arch.msr  msr_mask);
 
clear_bit(priority, vcpu-arch.pending_exceptions);
@@ -229,7 +239,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu 
*vcpu,
if (vcpu-arch.msr  MSR_PR) {
/* Program traps generated by user-level software must 
be handled
 * by the guest kernel. */
-   vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
r = RESUME_GUEST;
kvmppc_account_exit(vcpu, USR_PR_INST);
@@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
kvm_vcpu *vcpu,
break;
 
case BOOKE_INTERRUPT_DATA_STORAGE:
-   vcpu-arch.dear = vcpu-arch.fault_dear;
-   vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DATA_STORAGE);
kvmppc_account_exit(vcpu, DSI_EXITS);
r = RESUME_GUEST;
break;
 
case BOOKE_INTERRUPT_INST_STORAGE:
-   vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_INST_STORAGE);
kvmppc_account_exit(vcpu, ISI_EXITS);
r = RESUME_GUEST;
@@ -317,8 +323,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu 
*vcpu,
if (gtlb_index  0) {
/* The guest didn't have a mapping for it. */
kvmppc_booke_queue_irqprio(vcpu, 
BOOKE_IRQPRIO_DTLB_MISS);
-   vcpu-arch.dear = vcpu-arch.fault_dear;
-   vcpu-arch.esr = vcpu-arch.fault_esr;
kvmppc_mmu_dtlb_miss(vcpu);
kvmppc_account_exit(vcpu, DTLB_REAL_MISS_EXITS);
r = RESUME_GUEST;
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index b905623..4f6b9df 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -151,8 +151,6 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct 
kvm_vcpu *vcpu)
case OP_TRAP:
 #ifdef CONFIG_PPC64
case OP_TRAP_64:
-#else
-   vcpu-arch.esr |= ESR_PTR;
 #endif
kvmppc_core_queue_program(vcpu, SRR1_PROGTRAP);
advance = 0;
-- 
1.6.4

--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-22 Thread Alexander Graf 

On 22.01.2010, at 11:54, Liu Yu wrote:

 Old method prematurely sets ESR and DEAR.
 Move this part after we decide to inject interrupt,
 and make it more like hardware behave.
 
 Signed-off-by: Liu Yu yu@freescale.com
 ---
 arch/powerpc/kvm/booke.c   |   24 ++--
 arch/powerpc/kvm/emulate.c |2 --
 2 files changed, 14 insertions(+), 12 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index e283e44..a8ee148 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -84,7 +84,8 @@ static void kvmppc_booke_queue_irqprio(struct kvm_vcpu 
 *vcpu,
 
 void kvmppc_core_queue_program(struct kvm_vcpu *vcpu, ulong flags)
 {
 - /* BookE does flags in ESR, so ignore those we get here */
 + if (flags == SRR1_PROGTRAP)
 + vcpu-arch.fault_esr = ESR_PTR;

This looks odd. I was more thinking of flags being ESR in this context. So 
you'd save off flags to a field in the vcpu here and restore it later when the 
fault actually gets injected.

   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
 }
 
 @@ -115,14 +116,19 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu 
 *vcpu,
 {
   int allowed = 0;
   ulong msr_mask;
 + bool update_esr = false, update_dear = false;
 
   switch (priority) {
 - case BOOKE_IRQPRIO_PROGRAM:
   case BOOKE_IRQPRIO_DTLB_MISS:
 - case BOOKE_IRQPRIO_ITLB_MISS:
 - case BOOKE_IRQPRIO_SYSCALL:

Is this correct? Was the previous order wrong according to the spec?

   case BOOKE_IRQPRIO_DATA_STORAGE:
 + update_dear = true;
 + /* fall through */
   case BOOKE_IRQPRIO_INST_STORAGE:
 + case BOOKE_IRQPRIO_PROGRAM:
 + update_esr = true;
 + /* fall through */
 + case BOOKE_IRQPRIO_ITLB_MISS:
 + case BOOKE_IRQPRIO_SYSCALL:
   case BOOKE_IRQPRIO_FP_UNAVAIL:
   case BOOKE_IRQPRIO_SPE_UNAVAIL:
   case BOOKE_IRQPRIO_SPE_FP_DATA:
 @@ -157,6 +163,10 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu 
 *vcpu,
   vcpu-arch.srr0 = vcpu-arch.pc;
   vcpu-arch.srr1 = vcpu-arch.msr;
   vcpu-arch.pc = vcpu-arch.ivpr | vcpu-arch.ivor[priority];
 + if (update_esr == true)
 + vcpu-arch.esr = vcpu-arch.fault_esr;

I'd rather like to see new fields used for that.

 + if (update_dear == true)
 + vcpu-arch.dear = vcpu-arch.fault_dear;

same here

   kvmppc_set_msr(vcpu, vcpu-arch.msr  msr_mask);
 
   clear_bit(priority, vcpu-arch.pending_exceptions);
 @@ -229,7 +239,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
 kvm_vcpu *vcpu,
   if (vcpu-arch.msr  MSR_PR) {
   /* Program traps generated by user-level software must 
 be handled
* by the guest kernel. */
 - vcpu-arch.esr = vcpu-arch.fault_esr;
   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
   r = RESUME_GUEST;
   kvmppc_account_exit(vcpu, USR_PR_INST);
 @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
 kvm_vcpu *vcpu,
   break;
 
   case BOOKE_INTERRUPT_DATA_STORAGE:
 - vcpu-arch.dear = vcpu-arch.fault_dear;
 - vcpu-arch.esr = vcpu-arch.fault_esr;
   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DATA_STORAGE);

kvmppc_booke_queue_data_storage(vcpu, vcpu-arch.fault_esr, 
vcpu-arch.fault_dear);

   kvmppc_account_exit(vcpu, DSI_EXITS);
   r = RESUME_GUEST;
   break;
 
   case BOOKE_INTERRUPT_INST_STORAGE:
 - vcpu-arch.esr = vcpu-arch.fault_esr;
   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_INST_STORAGE);

kvmppc_booke_queue_inst_storage(vcpu, vcpu-arch.fault_esr);

   kvmppc_account_exit(vcpu, ISI_EXITS);
   r = RESUME_GUEST;
 @@ -317,8 +323,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
 kvm_vcpu *vcpu,
   if (gtlb_index  0) {
   /* The guest didn't have a mapping for it. */
   kvmppc_booke_queue_irqprio(vcpu, 
 BOOKE_IRQPRIO_DTLB_MISS);

see above

 - vcpu-arch.dear = vcpu-arch.fault_dear;
 - vcpu-arch.esr = vcpu-arch.fault_esr;
   kvmppc_mmu_dtlb_miss(vcpu);
   kvmppc_account_exit(vcpu, DTLB_REAL_MISS_EXITS);
   r = RESUME_GUEST;
 diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
 index b905623..4f6b9df 100644
 --- a/arch/powerpc/kvm/emulate.c
 +++ b/arch/powerpc/kvm/emulate.c
 @@ -151,8 +151,6 @@ int kvmppc_emulate_instruction(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   case OP_TRAP:
 #ifdef CONFIG_PPC64
   case OP_TRAP_64:
 -#else
 - vcpu-arch.esr |= ESR_PTR;
 #endif

RE: [PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-22 Thread Liu Yu-B13201 
 

 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org 
 [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
 Sent: Friday, January 22, 2010 7:13 PM
 To: Liu Yu-B13201
 Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org; 
 k...@vger.kernel.org
 Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when 
 inject interrupt to guest
 
 
 On 22.01.2010, at 11:54, Liu Yu wrote:
 
  Old method prematurely sets ESR and DEAR.
  Move this part after we decide to inject interrupt,
  and make it more like hardware behave.
  
  Signed-off-by: Liu Yu yu@freescale.com
  ---
  arch/powerpc/kvm/booke.c   |   24 ++--
  arch/powerpc/kvm/emulate.c |2 --
  2 files changed, 14 insertions(+), 12 deletions(-)
  
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
  index e283e44..a8ee148 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -84,7 +84,8 @@ static void 
 kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
  
  void kvmppc_core_queue_program(struct kvm_vcpu *vcpu, ulong flags)
  {
  -   /* BookE does flags in ESR, so ignore those we get here */
  +   if (flags == SRR1_PROGTRAP)
  +   vcpu-arch.fault_esr = ESR_PTR;
 
 This looks odd. I was more thinking of flags being ESR in 
 this context. So you'd save off flags to a field in the vcpu 
 here and restore it later when the fault actually gets injected.

See the end.

 
  kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
  }
  
  @@ -115,14 +116,19 @@ static int 
 kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
  {
  int allowed = 0;
  ulong msr_mask;
  +   bool update_esr = false, update_dear = false;
  
  switch (priority) {
  -   case BOOKE_IRQPRIO_PROGRAM:
  case BOOKE_IRQPRIO_DTLB_MISS:
  -   case BOOKE_IRQPRIO_ITLB_MISS:
  -   case BOOKE_IRQPRIO_SYSCALL:
 
 Is this correct? Was the previous order wrong according to the spec?

what order? just make switch items share the code.

 
  case BOOKE_IRQPRIO_DATA_STORAGE:
  +   update_dear = true;
  +   /* fall through */
  case BOOKE_IRQPRIO_INST_STORAGE:
  +   case BOOKE_IRQPRIO_PROGRAM:
  +   update_esr = true;
  +   /* fall through */
  +   case BOOKE_IRQPRIO_ITLB_MISS:
  +   case BOOKE_IRQPRIO_SYSCALL:
  case BOOKE_IRQPRIO_FP_UNAVAIL:
  case BOOKE_IRQPRIO_SPE_UNAVAIL:
  case BOOKE_IRQPRIO_SPE_FP_DATA:
  @@ -157,6 +163,10 @@ static int 
 kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
  vcpu-arch.srr0 = vcpu-arch.pc;
  vcpu-arch.srr1 = vcpu-arch.msr;
  vcpu-arch.pc = vcpu-arch.ivpr | 
 vcpu-arch.ivor[priority];
  +   if (update_esr == true)
  +   vcpu-arch.esr = vcpu-arch.fault_esr;
 
 I'd rather like to see new fields used for that.
  @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run 
 *run, struct kvm_vcpu *vcpu,
  break;
  
  case BOOKE_INTERRUPT_DATA_STORAGE:
  -   vcpu-arch.dear = vcpu-arch.fault_dear;
  -   vcpu-arch.esr = vcpu-arch.fault_esr;
  kvmppc_booke_queue_irqprio(vcpu, 
 BOOKE_IRQPRIO_DATA_STORAGE);
 
 kvmppc_booke_queue_data_storage(vcpu, vcpu-arch.fault_esr, 
 vcpu-arch.fault_dear);
 
  kvmppc_account_exit(vcpu, DSI_EXITS);
  r = RESUME_GUEST;
  break;
  
  case BOOKE_INTERRUPT_INST_STORAGE:
  -   vcpu-arch.esr = vcpu-arch.fault_esr;
  kvmppc_booke_queue_irqprio(vcpu, 
 BOOKE_IRQPRIO_INST_STORAGE);
 
 kvmppc_booke_queue_inst_storage(vcpu, vcpu-arch.fault_esr);
 

Not sure if this is redundant, as we already have fault_esr.
Or should we ignore what hareware is and create a new esr to guest?

 
  -   vcpu-arch.dear = vcpu-arch.fault_dear;
  -   vcpu-arch.esr = vcpu-arch.fault_esr;
  kvmppc_mmu_dtlb_miss(vcpu);
  kvmppc_account_exit(vcpu, DTLB_REAL_MISS_EXITS);
  r = RESUME_GUEST;
  diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
  index b905623..4f6b9df 100644
  --- a/arch/powerpc/kvm/emulate.c
  +++ b/arch/powerpc/kvm/emulate.c
  @@ -151,8 +151,6 @@ int kvmppc_emulate_instruction(struct 
 kvm_run *run, struct kvm_vcpu *vcpu)
  case OP_TRAP:
  #ifdef CONFIG_PPC64
  case OP_TRAP_64:
  -#else
  -   vcpu-arch.esr |= ESR_PTR;
  #endif
  kvmppc_core_queue_program(vcpu, SRR1_PROGTRAP);
 
 kvmppc_core_queue_program(vcpu, vcpu-arch.esr | ESR_PTR);
 

This breaks book3s code.
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kvmppc/booke: Set ESR and DEAR when inject interrupt to guest

2010-01-22 Thread Alexander Graf 

On 22.01.2010, at 12:27, Liu Yu-B13201 wrote:

 
 
 -Original Message-
 From: kvm-ppc-ow...@vger.kernel.org 
 [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf
 Sent: Friday, January 22, 2010 7:13 PM
 To: Liu Yu-B13201
 Cc: hol...@penguinppc.org; kvm-ppc@vger.kernel.org; 
 k...@vger.kernel.org
 Subject: Re: [PATCH] kvmppc/booke: Set ESR and DEAR when 
 inject interrupt to guest
 
 
 On 22.01.2010, at 11:54, Liu Yu wrote:
 
 Old method prematurely sets ESR and DEAR.
 Move this part after we decide to inject interrupt,
 and make it more like hardware behave.
 
 Signed-off-by: Liu Yu yu@freescale.com
 ---
 arch/powerpc/kvm/booke.c   |   24 ++--
 arch/powerpc/kvm/emulate.c |2 --
 2 files changed, 14 insertions(+), 12 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index e283e44..a8ee148 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -84,7 +84,8 @@ static void 
 kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu,
 
 void kvmppc_core_queue_program(struct kvm_vcpu *vcpu, ulong flags)
 {
 -   /* BookE does flags in ESR, so ignore those we get here */
 +   if (flags == SRR1_PROGTRAP)
 +   vcpu-arch.fault_esr = ESR_PTR;
 
 This looks odd. I was more thinking of flags being ESR in 
 this context. So you'd save off flags to a field in the vcpu 
 here and restore it later when the fault actually gets injected.
 
 See the end.
 
 
 kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_PROGRAM);
 }
 
 @@ -115,14 +116,19 @@ static int 
 kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 {
 int allowed = 0;
 ulong msr_mask;
 +   bool update_esr = false, update_dear = false;
 
 switch (priority) {
 -   case BOOKE_IRQPRIO_PROGRAM:
 case BOOKE_IRQPRIO_DTLB_MISS:
 -   case BOOKE_IRQPRIO_ITLB_MISS:
 -   case BOOKE_IRQPRIO_SYSCALL:
 
 Is this correct? Was the previous order wrong according to the spec?
 
 what order? just make switch items share the code.

Yikes. Yes. My fault.

 
 
 case BOOKE_IRQPRIO_DATA_STORAGE:
 +   update_dear = true;
 +   /* fall through */
 case BOOKE_IRQPRIO_INST_STORAGE:
 +   case BOOKE_IRQPRIO_PROGRAM:
 +   update_esr = true;
 +   /* fall through */
 +   case BOOKE_IRQPRIO_ITLB_MISS:
 +   case BOOKE_IRQPRIO_SYSCALL:
 case BOOKE_IRQPRIO_FP_UNAVAIL:
 case BOOKE_IRQPRIO_SPE_UNAVAIL:
 case BOOKE_IRQPRIO_SPE_FP_DATA:
 @@ -157,6 +163,10 @@ static int 
 kvmppc_booke_irqprio_deliver(struct kvm_vcpu *vcpu,
 vcpu-arch.srr0 = vcpu-arch.pc;
 vcpu-arch.srr1 = vcpu-arch.msr;
 vcpu-arch.pc = vcpu-arch.ivpr | 
 vcpu-arch.ivor[priority];
 +   if (update_esr == true)
 +   vcpu-arch.esr = vcpu-arch.fault_esr;
 
 I'd rather like to see new fields used for that.
 @@ -286,15 +295,12 @@ int kvmppc_handle_exit(struct kvm_run 
 *run, struct kvm_vcpu *vcpu,
 break;
 
 case BOOKE_INTERRUPT_DATA_STORAGE:
 -   vcpu-arch.dear = vcpu-arch.fault_dear;
 -   vcpu-arch.esr = vcpu-arch.fault_esr;
 kvmppc_booke_queue_irqprio(vcpu, 
 BOOKE_IRQPRIO_DATA_STORAGE);
 
 kvmppc_booke_queue_data_storage(vcpu, vcpu-arch.fault_esr, 
 vcpu-arch.fault_dear);
 
 kvmppc_account_exit(vcpu, DSI_EXITS);
 r = RESUME_GUEST;
 break;
 
 case BOOKE_INTERRUPT_INST_STORAGE:
 -   vcpu-arch.esr = vcpu-arch.fault_esr;
 kvmppc_booke_queue_irqprio(vcpu, 
 BOOKE_IRQPRIO_INST_STORAGE);
 
 kvmppc_booke_queue_inst_storage(vcpu, vcpu-arch.fault_esr);
 
 
 Not sure if this is redundant, as we already have fault_esr.
 Or should we ignore what hareware is and create a new esr to guest?

On Book3S I take the SRR1 we get from the host as inspiration of what to pass 
to the guest as SRR1. I think we should definitely be able to inject a fault 
that we didn't get in that exact form from the exit path.

I'm also not sure if something could clobber fault_esr if another interrupt 
takes precedence. Say a #MC.

 
 
 -   vcpu-arch.dear = vcpu-arch.fault_dear;
 -   vcpu-arch.esr = vcpu-arch.fault_esr;
 kvmppc_mmu_dtlb_miss(vcpu);
 kvmppc_account_exit(vcpu, DTLB_REAL_MISS_EXITS);
 r = RESUME_GUEST;
 diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
 index b905623..4f6b9df 100644
 --- a/arch/powerpc/kvm/emulate.c
 +++ b/arch/powerpc/kvm/emulate.c
 @@ -151,8 +151,6 @@ int kvmppc_emulate_instruction(struct 
 kvm_run *run, struct kvm_vcpu *vcpu)
 case OP_TRAP:
 #ifdef CONFIG_PPC64
 case OP_TRAP_64:
 -#else
 -   vcpu-arch.esr |= ESR_PTR;
 #endif
 kvmppc_core_queue_program(vcpu, SRR1_PROGTRAP);
 
 kvmppc_core_queue_program(vcpu, vcpu-arch.esr | ESR_PTR);
 
 
 This breaks book3s code.

How so? For the handlers that don't take a flags parameter, just add one, 
stub it out for Book3S and I'll fix