[Qemu-devel] (no subject)

[Qemu-devel] (no subject)

[Nicholas Piggin]

Date: Thu, 20 Oct 2016 17:38:24 +1100
Subject: [PATCH 0/3] ppc: system reset interrupt fixes and new hcall IPI

Hi,

We are implementing this new unmaskable IPI hcall for crash dumping
and debugging. I had some issues with QEMU delivering system reset
interrupt to guests, which was caused by the HV bit being set. After
changing that, the interrupt was being handled okay. Should there be
a more general check to ensure the HV bit is not set in the guest?

I implemented Linux support for the new hcall in crashdump code, and
it works.

Thanks,
Nick

Nicholas Piggin (3):
  ppc: fix MSR_ME handling for system reset interrupt
  ppc: allow system reset interrupt to be delivered to guests
  ppc/spapr: implement H_SIGNAL_SYS_RESET

 hw/ppc/spapr_hcall.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h   |  8 +++++++-
 target-ppc/excp_helper.c | 10 +++++++---
 3 files changed, 56 insertions(+), 4 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] ppc: fix MSR_ME handling for system reset interrupt

[Nicholas Piggin]

Power ISA specifies ME bit handling for system reset interrupt:

    if the interrupt occurred while the thread was in power-saving
    mode, set to 1; otherwise not altered

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target-ppc/excp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 921c39d..53c4075 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -385,11 +385,11 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         srr1 = SPR_BOOKE_CSRR1;
         break;
     case POWERPC_EXCP_RESET:     /* System reset exception                   */
+        /* A power-saving exception sets ME, otherwise it is unchanged */
         if (msr_pow) {
             /* indicate that we resumed from power save mode */
             msr |= 0x10000;
-        } else {
-            new_msr &= ~((target_ulong)1 << MSR_ME);
+            new_msr |= ((target_ulong)1 << MSR_ME);
         }
 
         new_msr |= (target_ulong)MSR_HVB;
-- 
2.9.3

[Qemu-devel] [PATCH 2/3] ppc: allow system reset interrupt to be delivered to guests

[Nicholas Piggin]

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target-ppc/excp_helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 53c4075..477af10 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             /* indicate that we resumed from power save mode */
             msr |= 0x10000;
             new_msr |= ((target_ulong)1 << MSR_ME);
+            new_msr |= (target_ulong)MSR_HVB;
+        } else {
+	    /* The ISA specifies the HV bit is set when the hardware interrupt
+	     * is raised, however when hypervisors deliver the exception to
+	     * guests, it should not be set.
+	     */
         }
-
-        new_msr |= (target_ulong)MSR_HVB;
         ail = 0;
         break;
     case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
-- 
2.9.3

[Qemu-devel] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[Nicholas Piggin]

The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
reset exception on other CPUs in the same guest.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  8 +++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index c5e7e8c..5ae84f0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -880,6 +880,47 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     return ret;
 }
 
+static void do_sys_reset(CPUState *cs, void *arg)
+{
+    cpu_synchronize_state(cs);
+    ppc_cpu_do_system_reset(cs);
+}
+
+static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
+                                       sPAPRMachineState *spapr,
+                                       target_ulong opcode, target_ulong *args)
+{
+    target_long target = args[0];
+    CPUState *cs;
+
+    if (target < H_SIGNAL_SYS_RESET_ALLBUTSELF) {
+        return H_PARAMETER;
+    }
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *c = POWERPC_CPU(cs);
+
+        if (cpu->cpu_dt_id == target) {
+            run_on_cpu(cs, do_sys_reset, NULL);
+            return H_SUCCESS;
+        }
+
+        if (target == H_SIGNAL_SYS_RESET_ALLBUTSELF) {
+            if (c == cpu) {
+                continue;
+            }
+        }
+
+        run_on_cpu(cs, do_sys_reset, NULL);
+    }
+
+    if (target >= 0) {
+        return H_PARAMETER;
+    }
+
+    return H_SUCCESS;
+}
+
 /*
  * Return the offset to the requested option vector @vector in the
  * option vector table @table.
@@ -1113,6 +1154,7 @@ static void hypercall_register_types(void)
     /* hcall-splpar */
     spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
     spapr_register_hypercall(H_CEDE, h_cede);
+    spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
 
     /* processor register resource access h-calls */
     spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index aeaba3e..a28538b 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -339,7 +339,13 @@ struct sPAPRMachineState {
 #define H_XIRR_X                0x2FC
 #define H_RANDOM                0x300
 #define H_SET_MODE              0x31C
-#define MAX_HCALL_OPCODE        H_SET_MODE
+#define H_SIGNAL_SYS_RESET      0x380
+#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
+
+/* Parameters to H_SIGNAL_SYS_RESET */
+#define H_SIGNAL_SYS_RESET_ALL         -1
+#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
+
 
 /* The hcalls above are standardized in PAPR and implemented by pHyp
  * as well.
-- 
2.9.3

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[Thomas Huth]

On 20.10.2016 08:59, Nicholas Piggin wrote:
> The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> reset exception on other CPUs in the same guest.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  8 +++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
...
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index aeaba3e..a28538b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -339,7 +339,13 @@ struct sPAPRMachineState {
>  #define H_XIRR_X                0x2FC
>  #define H_RANDOM                0x300
>  #define H_SET_MODE              0x31C
> -#define MAX_HCALL_OPCODE        H_SET_MODE
> +#define H_SIGNAL_SYS_RESET      0x380
> +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> +
> +/* Parameters to H_SIGNAL_SYS_RESET */
> +#define H_SIGNAL_SYS_RESET_ALL         -1
> +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> +
>  
>  /* The hcalls above are standardized in PAPR and implemented by pHyp
>   * as well.

Is there a spec for this hypercall? I can't find it in LoPAPR v1.1 ?

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] ppc: fix MSR_ME handling for system reset interrupt

[Greg Kurz]

On Thu, 20 Oct 2016 17:59:10 +1100
Nicholas Piggin <npiggin@gmail.com> wrote:

> Power ISA specifies ME bit handling for system reset interrupt:
> 
>     if the interrupt occurred while the thread was in power-saving
>     mode, set to 1; otherwise not altered
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---

As described in "6.5 Interrupt Definitions, Figure 64" of Power ISA 3.0.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  target-ppc/excp_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 921c39d..53c4075 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -385,11 +385,11 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          srr1 = SPR_BOOKE_CSRR1;
>          break;
>      case POWERPC_EXCP_RESET:     /* System reset exception                   */
> +        /* A power-saving exception sets ME, otherwise it is unchanged */
>          if (msr_pow) {
>              /* indicate that we resumed from power save mode */
>              msr |= 0x10000;
> -        } else {
> -            new_msr &= ~((target_ulong)1 << MSR_ME);
> +            new_msr |= ((target_ulong)1 << MSR_ME);
>          }
>  
>          new_msr |= (target_ulong)MSR_HVB;

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] ppc: fix MSR_ME handling for system reset interrupt

[Cédric Le Goater]

On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> Power ISA specifies ME bit handling for system reset interrupt:
> 
>     if the interrupt occurred while the thread was in power-saving
>     mode, set to 1; otherwise not altered
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  target-ppc/excp_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 921c39d..53c4075 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -385,11 +385,11 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          srr1 = SPR_BOOKE_CSRR1;
>          break;
>      case POWERPC_EXCP_RESET:     /* System reset exception                   */
> +        /* A power-saving exception sets ME, otherwise it is unchanged */
>          if (msr_pow) {
>              /* indicate that we resumed from power save mode */
>              msr |= 0x10000;
> -        } else {
> -            new_msr &= ~((target_ulong)1 << MSR_ME);
> +            new_msr |= ((target_ulong)1 << MSR_ME);
>          }
>  
>          new_msr |= (target_ulong)MSR_HVB;
> 

Re: [Qemu-devel] [Qemu-ppc] (no subject)

[Greg Kurz]

On Thu, 20 Oct 2016 17:59:09 +1100
Nicholas Piggin <npiggin@gmail.com> wrote:

> Date: Thu, 20 Oct 2016 17:38:24 +1100
> Subject: [PATCH 0/3] ppc: system reset interrupt fixes and new hcall IPI
> 
> Hi,
> 
> We are implementing this new unmaskable IPI hcall for crash dumping
> and debugging. I had some issues with QEMU delivering system reset
> interrupt to guests, which was caused by the HV bit being set. After
> changing that, the interrupt was being handled okay. Should there be
> a more general check to ensure the HV bit is not set in the guest?
> 
> I implemented Linux support for the new hcall in crashdump code, and
> it works.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (3):
>   ppc: fix MSR_ME handling for system reset interrupt
>   ppc: allow system reset interrupt to be delivered to guests
>   ppc/spapr: implement H_SIGNAL_SYS_RESET
> 
>  hw/ppc/spapr_hcall.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h   |  8 +++++++-
>  target-ppc/excp_helper.c | 10 +++++++---
>  3 files changed, 56 insertions(+), 4 deletions(-)
> 

Weird... these patches didn't make it to qemu-devel. As a result, they
don't appear in patchwork and http://wiki.qemu.org/patches/patches.json

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] ppc: allow system reset interrupt to be delivered to guests

[Cédric Le Goater]

On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target-ppc/excp_helper.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 53c4075..477af10 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              /* indicate that we resumed from power save mode */
>              msr |= 0x10000;
>              new_msr |= ((target_ulong)1 << MSR_ME);
> +            new_msr |= (target_ulong)MSR_HVB;
> +        } else {
> +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> +	     * is raised, however when hypervisors deliver the exception to
> +	     * guests, it should not be set.
> +	     */
>          }
> -
> -        new_msr |= (target_ulong)MSR_HVB;
>          ail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> 

should not that be cleared later on in powerpc_excp() by :

    env->msr = new_msr & env->msr_mask;

? but the routine is rather long so I might be missing a branch.

C.

Re: [Qemu-devel] [Qemu-ppc] (no subject)

[Cédric Le Goater]

On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> Date: Thu, 20 Oct 2016 17:38:24 +1100
> Subject: [PATCH 0/3] ppc: system reset interrupt fixes and new hcall IPI
> 
> Hi,
> 
> We are implementing this new unmaskable IPI hcall for crash dumping
> and debugging. I had some issues with QEMU delivering system reset
> interrupt to guests, which was caused by the HV bit being set. After
> changing that, the interrupt was being handled okay. Should there be
> a more general check to ensure the HV bit is not set in the guest?

cpu_ppc_set_papr() called by cpu_init() does : 

	env->msr_mask &= ~((1ull << MSR_EP) | MSR_HVB);

and then for the reset : 

ppc_cpu_reset() 
	hreg_store_msr()
		value &= env->msr_mask;

at first sight, we should be fine ? 

C. 

> 
> I implemented Linux support for the new hcall in crashdump code, and
> it works.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (3):
>   ppc: fix MSR_ME handling for system reset interrupt
>   ppc: allow system reset interrupt to be delivered to guests
>   ppc/spapr: implement H_SIGNAL_SYS_RESET
> 
>  hw/ppc/spapr_hcall.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h   |  8 +++++++-
>  target-ppc/excp_helper.c | 10 +++++++---
>  3 files changed, 56 insertions(+), 4 deletions(-)
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[Nicholas Piggin]

On Thu, 20 Oct 2016 11:21:22 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 20.10.2016 08:59, Nicholas Piggin wrote:
> > The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> > reset exception on other CPUs in the same guest.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  8 +++++++-
> >  2 files changed, 49 insertions(+), 1 deletion(-)  
> ...
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index aeaba3e..a28538b 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -339,7 +339,13 @@ struct sPAPRMachineState {
> >  #define H_XIRR_X                0x2FC
> >  #define H_RANDOM                0x300
> >  #define H_SET_MODE              0x31C
> > -#define MAX_HCALL_OPCODE        H_SET_MODE
> > +#define H_SIGNAL_SYS_RESET      0x380
> > +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> > +
> > +/* Parameters to H_SIGNAL_SYS_RESET */
> > +#define H_SIGNAL_SYS_RESET_ALL         -1
> > +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> > +
> >  
> >  /* The hcalls above are standardized in PAPR and implemented by pHyp
> >   * as well.  
> 
> Is there a spec for this hypercall? I can't find it in LoPAPR v1.1 ?

Oh sorry, I should have said that this is going through an internal
process at moment and not in a released document yet. I'll try to get
a more satisfying answer on that one.

Thanks,
Nick

Re: [Qemu-devel] [Qemu-ppc] (no subject)

[Nicholas Piggin]

On Thu, 20 Oct 2016 13:49:25 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 20 Oct 2016 17:59:09 +1100
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> > Date: Thu, 20 Oct 2016 17:38:24 +1100
> > Subject: [PATCH 0/3] ppc: system reset interrupt fixes and new hcall IPI
> > 
> > Hi,
> > 
> > We are implementing this new unmaskable IPI hcall for crash dumping
> > and debugging. I had some issues with QEMU delivering system reset
> > interrupt to guests, which was caused by the HV bit being set. After
> > changing that, the interrupt was being handled okay. Should there be
> > a more general check to ensure the HV bit is not set in the guest?
> > 
> > I implemented Linux support for the new hcall in crashdump code, and
> > it works.
> > 
> > Thanks,
> > Nick
> > 
> > Nicholas Piggin (3):
> >   ppc: fix MSR_ME handling for system reset interrupt
> >   ppc: allow system reset interrupt to be delivered to guests
> >   ppc/spapr: implement H_SIGNAL_SYS_RESET
> > 
> >  hw/ppc/spapr_hcall.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h   |  8 +++++++-
> >  target-ppc/excp_helper.c | 10 +++++++---
> >  3 files changed, 56 insertions(+), 4 deletions(-)
> >   
> 
> Weird... these patches didn't make it to qemu-devel. As a result, they
> don't appear in patchwork and http://wiki.qemu.org/patches/patches.json

Maybe because I messed up the first mail. I'll resend the series
after feedback.

Thanks,
Nick

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] ppc: allow system reset interrupt to be delivered to guests

[Nicholas Piggin]

On Thu, 20 Oct 2016 15:08:07 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target-ppc/excp_helper.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 53c4075..477af10 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> >              /* indicate that we resumed from power save mode */
> >              msr |= 0x10000;
> >              new_msr |= ((target_ulong)1 << MSR_ME);
> > +            new_msr |= (target_ulong)MSR_HVB;
> > +        } else {
> > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > +	     * is raised, however when hypervisors deliver the exception to
> > +	     * guests, it should not be set.
> > +	     */
> >          }
> > -
> > -        new_msr |= (target_ulong)MSR_HVB;
> >          ail = 0;
> >          break;
> >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> >   
> 
> should not that be cleared later on in powerpc_excp() by :
> 
>     env->msr = new_msr & env->msr_mask;
> 
> ? but the routine is rather long so I might be missing a branch.

No you're right, so it can't leak into the guest, phew!

The problem I get is the interrupt code doing some things differently
depending on on the HV bit. For example what I noticed is the guest
losing its LE bit upon entry.

Perhaps a cleaner way is for the system reset case to set new_msr
according to the ISA, and then apply the msr_mask (or at least mask
out HV) before calculating the exception model? Any preference?

Thanks,
Nick

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[Greg Kurz]

On Thu, 20 Oct 2016 17:59:12 +1100
Nicholas Piggin <npiggin@gmail.com> wrote:

> The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> reset exception on other CPUs in the same guest.
> 

Actually on all CPUs or all-but-self CPUs or a specific CPU (including self).

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  8 +++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index c5e7e8c..5ae84f0 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -880,6 +880,47 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      return ret;
>  }
>  
> +static void do_sys_reset(CPUState *cs, void *arg)
> +{
> +    cpu_synchronize_state(cs);
> +    ppc_cpu_do_system_reset(cs);
> +}
> +

We already have the following function in hw/ppc/spapr.c, which serves the
same purpose:

static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
{
    cpu_synchronize_state(cs);
    ppc_cpu_do_system_reset(cs);
}

What about using it ?

> +static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
> +                                       sPAPRMachineState *spapr,
> +                                       target_ulong opcode, target_ulong *args)
> +{
> +    target_long target = args[0];
> +    CPUState *cs;
> +
> +    if (target < H_SIGNAL_SYS_RESET_ALLBUTSELF) {
> +        return H_PARAMETER;
> +    }
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *c = POWERPC_CPU(cs);
> +
> +        if (cpu->cpu_dt_id == target) {
> +            run_on_cpu(cs, do_sys_reset, NULL);
> +            return H_SUCCESS;
> +        }
> +
> +        if (target == H_SIGNAL_SYS_RESET_ALLBUTSELF) {
> +            if (c == cpu) {
> +                continue;
> +            }
> +        }
> +
> +        run_on_cpu(cs, do_sys_reset, NULL);
> +    }
> +
> +    if (target >= 0) {
> +        return H_PARAMETER;
> +    }
> +
> +    return H_SUCCESS;
> +}
> +
>  /*
>   * Return the offset to the requested option vector @vector in the
>   * option vector table @table.
> @@ -1113,6 +1154,7 @@ static void hypercall_register_types(void)
>      /* hcall-splpar */
>      spapr_register_hypercall(H_REGISTER_VPA, h_register_vpa);
>      spapr_register_hypercall(H_CEDE, h_cede);
> +    spapr_register_hypercall(H_SIGNAL_SYS_RESET, h_signal_sys_reset);
>  
>      /* processor register resource access h-calls */
>      spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index aeaba3e..a28538b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -339,7 +339,13 @@ struct sPAPRMachineState {
>  #define H_XIRR_X                0x2FC
>  #define H_RANDOM                0x300
>  #define H_SET_MODE              0x31C
> -#define MAX_HCALL_OPCODE        H_SET_MODE
> +#define H_SIGNAL_SYS_RESET      0x380
> +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> +
> +/* Parameters to H_SIGNAL_SYS_RESET */
> +#define H_SIGNAL_SYS_RESET_ALL         -1
> +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> +

I'd rather move these to hw/ppc/spapr_hcall.c just above h_signal_sys_reset(),
the same way it is done for h_bulk_remove().

>  
>  /* The hcalls above are standardized in PAPR and implemented by pHyp
>   * as well.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[Nicholas Piggin]

On Thu, 20 Oct 2016 18:49:22 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 20 Oct 2016 17:59:12 +1100
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> > The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> > reset exception on other CPUs in the same guest.
> >   
> 
> Actually on all CPUs or all-but-self CPUs or a specific CPU (including self).

Exactly right. I'll improve the changelog and try to add something more
official.

 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  8 +++++++-
> >  2 files changed, 49 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index c5e7e8c..5ae84f0 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -880,6 +880,47 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      return ret;
> >  }
> >  
> > +static void do_sys_reset(CPUState *cs, void *arg)
> > +{
> > +    cpu_synchronize_state(cs);
> > +    ppc_cpu_do_system_reset(cs);
> > +}
> > +  
> 
> We already have the following function in hw/ppc/spapr.c, which serves the
> same purpose:
> 
> static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
> {
>     cpu_synchronize_state(cs);
>     ppc_cpu_do_system_reset(cs);
> }
> 
> What about using it ?

I suppose. "nmi" isn't really architected, whereas system reset is.
I was considering ppc_cpu_do_nmi_on_cpu call do_sys_reset, but it
didn't seem like much of an improvement. I'll use a single function
for both if you prefer.

> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -339,7 +339,13 @@ struct sPAPRMachineState {
> >  #define H_XIRR_X                0x2FC
> >  #define H_RANDOM                0x300
> >  #define H_SET_MODE              0x31C
> > -#define MAX_HCALL_OPCODE        H_SET_MODE
> > +#define H_SIGNAL_SYS_RESET      0x380
> > +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> > +
> > +/* Parameters to H_SIGNAL_SYS_RESET */
> > +#define H_SIGNAL_SYS_RESET_ALL         -1
> > +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> > +  
> 
> I'd rather move these to hw/ppc/spapr_hcall.c just above h_signal_sys_reset(),
> the same way it is done for h_bulk_remove().

Will do.

Thanks,
Nick

Re: [Qemu-devel] [PATCH 1/3] ppc: fix MSR_ME handling for system reset interrupt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On Thu, Oct 20, 2016 at 05:59:10PM +1100, Nicholas Piggin wrote:
> Power ISA specifies ME bit handling for system reset interrupt:
> 
>     if the interrupt occurred while the thread was in power-saving
>     mode, set to 1; otherwise not altered
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

I've applied this one to ppc-for-2.8.  Looks like the remaining
patches in the series need a little more polish.

> ---
>  target-ppc/excp_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 921c39d..53c4075 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -385,11 +385,11 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          srr1 = SPR_BOOKE_CSRR1;
>          break;
>      case POWERPC_EXCP_RESET:     /* System reset exception                   */
> +        /* A power-saving exception sets ME, otherwise it is unchanged */
>          if (msr_pow) {
>              /* indicate that we resumed from power save mode */
>              msr |= 0x10000;
> -        } else {
> -            new_msr &= ~((target_ulong)1 << MSR_ME);
> +            new_msr |= ((target_ulong)1 << MSR_ME);
>          }
>  
>          new_msr |= (target_ulong)MSR_HVB;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] ppc: allow system reset interrupt to be delivered to guests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]

On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
> On Thu, 20 Oct 2016 15:08:07 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > >  target-ppc/excp_helper.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > index 53c4075..477af10 100644
> > > --- a/target-ppc/excp_helper.c
> > > +++ b/target-ppc/excp_helper.c
> > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > >              /* indicate that we resumed from power save mode */
> > >              msr |= 0x10000;
> > >              new_msr |= ((target_ulong)1 << MSR_ME);
> > > +            new_msr |= (target_ulong)MSR_HVB;
> > > +        } else {
> > > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > > +	     * is raised, however when hypervisors deliver the exception to
> > > +	     * guests, it should not be set.
> > > +	     */
> > >          }
> > > -
> > > -        new_msr |= (target_ulong)MSR_HVB;
> > >          ail = 0;
> > >          break;
> > >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> > >   
> > 
> > should not that be cleared later on in powerpc_excp() by :
> > 
> >     env->msr = new_msr & env->msr_mask;
> > 
> > ? but the routine is rather long so I might be missing a branch.
> 
> No you're right, so it can't leak into the guest, phew!
> 
> The problem I get is the interrupt code doing some things differently
> depending on on the HV bit. For example what I noticed is the guest
> losing its LE bit upon entry.
> 
> Perhaps a cleaner way is for the system reset case to set new_msr
> according to the ISA, and then apply the msr_mask (or at least mask
> out HV) before calculating the exception model? Any preference?

I think the proposed revision makes sense.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2918 bytes --]

On Fri, Oct 21, 2016 at 11:56:34AM +1100, Nicholas Piggin wrote:
> On Thu, 20 Oct 2016 18:49:22 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Thu, 20 Oct 2016 17:59:12 +1100
> > Nicholas Piggin <npiggin@gmail.com> wrote:
> > 
> > > The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> > > reset exception on other CPUs in the same guest.
> > >   
> > 
> > Actually on all CPUs or all-but-self CPUs or a specific CPU (including self).
> 
> Exactly right. I'll improve the changelog and try to add something more
> official.
> 
>  
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > >  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h |  8 +++++++-
> > >  2 files changed, 49 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index c5e7e8c..5ae84f0 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -880,6 +880,47 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > >      return ret;
> > >  }
> > >  
> > > +static void do_sys_reset(CPUState *cs, void *arg)
> > > +{
> > > +    cpu_synchronize_state(cs);
> > > +    ppc_cpu_do_system_reset(cs);
> > > +}
> > > +  
> > 
> > We already have the following function in hw/ppc/spapr.c, which serves the
> > same purpose:
> > 
> > static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
> > {
> >     cpu_synchronize_state(cs);
> >     ppc_cpu_do_system_reset(cs);
> > }
> > 
> > What about using it ?
> 
> I suppose. "nmi" isn't really architected, whereas system reset is.
> I was considering ppc_cpu_do_nmi_on_cpu call do_sys_reset, but it
> didn't seem like much of an improvement. I'll use a single function
> for both if you prefer.

I think system_reset is a better name, so it's probably best to rename
the existing function.

> 
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -339,7 +339,13 @@ struct sPAPRMachineState {
> > >  #define H_XIRR_X                0x2FC
> > >  #define H_RANDOM                0x300
> > >  #define H_SET_MODE              0x31C
> > > -#define MAX_HCALL_OPCODE        H_SET_MODE
> > > +#define H_SIGNAL_SYS_RESET      0x380
> > > +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> > > +
> > > +/* Parameters to H_SIGNAL_SYS_RESET */
> > > +#define H_SIGNAL_SYS_RESET_ALL         -1
> > > +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> > > +  
> > 
> > I'd rather move these to hw/ppc/spapr_hcall.c just above h_signal_sys_reset(),
> > the same way it is done for h_bulk_remove().
> 
> Will do.
> 
> Thanks,
> Nick
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/3] ppc: fix MSR_ME handling for system reset interrupt

[Nicholas Piggin]

On Fri, 21 Oct 2016 12:09:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Oct 20, 2016 at 05:59:10PM +1100, Nicholas Piggin wrote:
> > Power ISA specifies ME bit handling for system reset interrupt:
> > 
> >     if the interrupt occurred while the thread was in power-saving
> >     mode, set to 1; otherwise not altered
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>  
> 
> I've applied this one to ppc-for-2.8.  Looks like the remaining
> patches in the series need a little more polish.

Yes I'll respin the other two based on feedback.

Thanks,
Nick

[Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[Nicholas Piggin]

On Fri, 21 Oct 2016 12:09:54 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
> > On Thu, 20 Oct 2016 15:08:07 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:  
> > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > ---
> > > >  target-ppc/excp_helper.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > > index 53c4075..477af10 100644
> > > > --- a/target-ppc/excp_helper.c
> > > > +++ b/target-ppc/excp_helper.c
> > > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > > >              /* indicate that we resumed from power save mode */
> > > >              msr |= 0x10000;
> > > >              new_msr |= ((target_ulong)1 << MSR_ME);
> > > > +            new_msr |= (target_ulong)MSR_HVB;
> > > > +        } else {
> > > > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > > > +	     * is raised, however when hypervisors deliver the exception to
> > > > +	     * guests, it should not be set.
> > > > +	     */
> > > >          }
> > > > -
> > > > -        new_msr |= (target_ulong)MSR_HVB;
> > > >          ail = 0;
> > > >          break;
> > > >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> > > >     
> > > 
> > > should not that be cleared later on in powerpc_excp() by :
> > > 
> > >     env->msr = new_msr & env->msr_mask;
> > > 
> > > ? but the routine is rather long so I might be missing a branch.  
> > 
> > No you're right, so it can't leak into the guest, phew!
> > 
> > The problem I get is the interrupt code doing some things differently
> > depending on on the HV bit. For example what I noticed is the guest
> > losing its LE bit upon entry.
> > 
> > Perhaps a cleaner way is for the system reset case to set new_msr
> > according to the ISA, and then apply the msr_mask (or at least mask
> > out HV) before calculating the exception model? Any preference?  
> 
> I think the proposed revision makes sense.
> 

What do you think of this version? This fixes up machine check guest
delivery as well. I'm sending this ahead of the new hcall patch, because
it's a bugfix for existing code. I'll get around to the hcall again next
week.

Thanks,
Nick


ppc hypervisors have delivered system reset and machine check exception
interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
or NMI injection in QEMU).

These exceptions are architected to set the HV bit in hardware, however
when injected into a guest, the HV bit should be cleared. Current code
masks off the HV bit before setting the new MSR, however this happens after
the interrupt delivery model has calculated delivery mode for the exception.
This can result in the guest's MSR LE bit being lost.

Provide a new flag for HV exceptions to allow delivery to guests. The
exception model masks out the HV bit.

Also add another sanity check to ensure other such exceptions don't try
to set HV in guest without setting guest_hv_excp

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 53c4075..1b18433 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     target_ulong msr, new_msr, vector;
-    int srr0, srr1, asrr0, asrr1, lev, ail;
+    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;
     bool lpes0;
 
     qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
@@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
      *
      * AIL is initialized here but can be cleared by
      * selected exceptions
+     *
+     * guest_hv_excp is a provision to raise HV architected
+     * exceptions in guests by delivering them with HV bit
+     * clear. System reset and machine check use this.
      */
 #if defined(TARGET_PPC64)
     if (excp_model == POWERPC_EXCP_POWER7 ||
@@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         lpes0 = true;
         ail = 0;
     }
+    guest_hv_excp = 0;
 
     /* Hypervisor emulation assistance interrupt only exists on server
      * arch 2.05 server or later. We also don't want to generate it if
@@ -214,6 +219,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
         }
         new_msr |= (target_ulong)MSR_HVB;
+        guest_hv_excp = 1;
         ail = 0;
 
         /* machine check exceptions don't have ME set */
@@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             msr |= 0x10000;
             new_msr |= ((target_ulong)1 << MSR_ME);
         }
-
         new_msr |= (target_ulong)MSR_HVB;
+        guest_hv_excp = 1;
         ail = 0;
         break;
     case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
@@ -610,10 +616,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
 
     /* Sanity check */
     if (!(env->msr_mask & MSR_HVB) && (srr0 == SPR_HSRR0)) {
-        cpu_abort(cs, "Trying to deliver HV exception %d with "
+        cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
                   "no HV support\n", excp);
     }
 
+    /* The ISA specifies the HV bit is set when the hardware interrupt
+     * is raised, however in some cases hypervisors deliver the exception
+     * to guests. HV should be cleared in that case.
+     */
+    if (!(env->msr_mask & MSR_HVB) && (new_msr & MSR_HVB)) {
+        if (guest_hv_excp) {
+            new_msr &= ~MSR_HVB;
+        } else {
+            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
+                      "no HV support\n", excp);
+        }
+    }
+
     /* If any alternate SRR register are defined, duplicate saved values */
     if (asrr0 != -1) {
         env->spr[asrr0] = env->spr[srr0];
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[Cédric Le Goater]

On 10/21/2016 06:35 AM, Nicholas Piggin wrote:
> On Fri, 21 Oct 2016 12:09:54 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
>>> On Thu, 20 Oct 2016 15:08:07 +0200
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>   
>>>> On 10/20/2016 08:59 AM, Nicholas Piggin wrote:  
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>> ---
>>>>>  target-ppc/excp_helper.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>>>>> index 53c4075..477af10 100644
>>>>> --- a/target-ppc/excp_helper.c
>>>>> +++ b/target-ppc/excp_helper.c
>>>>> @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>>>              /* indicate that we resumed from power save mode */
>>>>>              msr |= 0x10000;
>>>>>              new_msr |= ((target_ulong)1 << MSR_ME);
>>>>> +            new_msr |= (target_ulong)MSR_HVB;
>>>>> +        } else {
>>>>> +	    /* The ISA specifies the HV bit is set when the hardware interrupt
>>>>> +	     * is raised, however when hypervisors deliver the exception to
>>>>> +	     * guests, it should not be set.
>>>>> +	     */
>>>>>          }
>>>>> -
>>>>> -        new_msr |= (target_ulong)MSR_HVB;
>>>>>          ail = 0;
>>>>>          break;
>>>>>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>>>>>     
>>>>
>>>> should not that be cleared later on in powerpc_excp() by :
>>>>
>>>>     env->msr = new_msr & env->msr_mask;
>>>>
>>>> ? but the routine is rather long so I might be missing a branch.  
>>>
>>> No you're right, so it can't leak into the guest, phew!
>>>
>>> The problem I get is the interrupt code doing some things differently
>>> depending on on the HV bit. For example what I noticed is the guest
>>> losing its LE bit upon entry.
>>>
>>> Perhaps a cleaner way is for the system reset case to set new_msr
>>> according to the ISA, and then apply the msr_mask (or at least mask
>>> out HV) before calculating the exception model? Any preference?  
>>
>> I think the proposed revision makes sense.
>>
> 
> What do you think of this version? This fixes up machine check guest
> delivery as well. I'm sending this ahead of the new hcall patch, because
> it's a bugfix for existing code. I'll get around to the hcall again next
> week.
> 
> Thanks,
> Nick
> 
> 
> ppc hypervisors have delivered system reset and machine check exception
> interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> or NMI injection in QEMU).
> 
> These exceptions are architected to set the HV bit in hardware, however
> when injected into a guest, the HV bit should be cleared. Current code
> masks off the HV bit before setting the new MSR, however this happens after
> the interrupt delivery model has calculated delivery mode for the exception.
> This can result in the guest's MSR LE bit being lost.
> 
> Provide a new flag for HV exceptions to allow delivery to guests. The
> exception model masks out the HV bit.
> 
> Also add another sanity check to ensure other such exceptions don't try
> to set HV in guest without setting guest_hv_excp
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Looks good to me and I gave it a try on the pnv platform which runs in
HV mode.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 

> ---
>  target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 53c4075..1b18433 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      target_ulong msr, new_msr, vector;
> -    int srr0, srr1, asrr0, asrr1, lev, ail;
> +    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;
>      bool lpes0;
>  
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       *
>       * AIL is initialized here but can be cleared by
>       * selected exceptions
> +     *
> +     * guest_hv_excp is a provision to raise HV architected
> +     * exceptions in guests by delivering them with HV bit
> +     * clear. System reset and machine check use this.
>       */
>  #if defined(TARGET_PPC64)
>      if (excp_model == POWERPC_EXCP_POWER7 ||
> @@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          lpes0 = true;
>          ail = 0;
>      }
> +    guest_hv_excp = 0;
>  
>      /* Hypervisor emulation assistance interrupt only exists on server
>       * arch 2.05 server or later. We also don't want to generate it if
> @@ -214,6 +219,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>          }
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>  
>          /* machine check exceptions don't have ME set */
> @@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              msr |= 0x10000;
>              new_msr |= ((target_ulong)1 << MSR_ME);
>          }
> -
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> @@ -610,10 +616,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  
>      /* Sanity check */
>      if (!(env->msr_mask & MSR_HVB) && (srr0 == SPR_HSRR0)) {
> -        cpu_abort(cs, "Trying to deliver HV exception %d with "
> +        cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>                    "no HV support\n", excp);
>      }
>  
> +    /* The ISA specifies the HV bit is set when the hardware interrupt
> +     * is raised, however in some cases hypervisors deliver the exception
> +     * to guests. HV should be cleared in that case.
> +     */
> +    if (!(env->msr_mask & MSR_HVB) && (new_msr & MSR_HVB)) {
> +        if (guest_hv_excp) {
> +            new_msr &= ~MSR_HVB;
> +        } else {
> +            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
> +                      "no HV support\n", excp);
> +        }
> +    }
> +
>      /* If any alternate SRR register are defined, duplicate saved values */
>      if (asrr0 != -1) {
>          env->spr[asrr0] = env->spr[srr0];
> 

Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7358 bytes --]

On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote:
> On Fri, 21 Oct 2016 12:09:54 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
> > > On Thu, 20 Oct 2016 15:08:07 +0200
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:  
> > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > > ---
> > > > >  target-ppc/excp_helper.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > > > index 53c4075..477af10 100644
> > > > > --- a/target-ppc/excp_helper.c
> > > > > +++ b/target-ppc/excp_helper.c
> > > > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > > > >              /* indicate that we resumed from power save mode */
> > > > >              msr |= 0x10000;
> > > > >              new_msr |= ((target_ulong)1 << MSR_ME);
> > > > > +            new_msr |= (target_ulong)MSR_HVB;
> > > > > +        } else {
> > > > > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > > > > +	     * is raised, however when hypervisors deliver the exception to
> > > > > +	     * guests, it should not be set.
> > > > > +	     */
> > > > >          }
> > > > > -
> > > > > -        new_msr |= (target_ulong)MSR_HVB;
> > > > >          ail = 0;
> > > > >          break;
> > > > >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> > > > >     
> > > > 
> > > > should not that be cleared later on in powerpc_excp() by :
> > > > 
> > > >     env->msr = new_msr & env->msr_mask;
> > > > 
> > > > ? but the routine is rather long so I might be missing a branch.  
> > > 
> > > No you're right, so it can't leak into the guest, phew!
> > > 
> > > The problem I get is the interrupt code doing some things differently
> > > depending on on the HV bit. For example what I noticed is the guest
> > > losing its LE bit upon entry.
> > > 
> > > Perhaps a cleaner way is for the system reset case to set new_msr
> > > according to the ISA, and then apply the msr_mask (or at least mask
> > > out HV) before calculating the exception model? Any preference?  
> > 
> > I think the proposed revision makes sense.
> > 
> 
> What do you think of this version? This fixes up machine check guest
> delivery as well. I'm sending this ahead of the new hcall patch, because
> it's a bugfix for existing code. I'll get around to the hcall again next
> week.
> 
> Thanks,
> Nick
> 
> 
> ppc hypervisors have delivered system reset and machine check exception
> interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> or NMI injection in QEMU).
> 
> These exceptions are architected to set the HV bit in hardware, however
> when injected into a guest, the HV bit should be cleared. Current code
> masks off the HV bit before setting the new MSR, however this happens after
> the interrupt delivery model has calculated delivery mode for the exception.
> This can result in the guest's MSR LE bit being lost.
> 
> Provide a new flag for HV exceptions to allow delivery to guests. The
> exception model masks out the HV bit.
> 
> Also add another sanity check to ensure other such exceptions don't try
> to set HV in guest without setting guest_hv_excp
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 53c4075..1b18433 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      target_ulong msr, new_msr, vector;
> -    int srr0, srr1, asrr0, asrr1, lev, ail;
> +    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;

So, to clarify my understanding of this.

The guest_hv_excp flag indicates that this is a normally-HV exception
which *could* be delivered to a guest with HV clear, *not* that we're
actually doing so in this instance.  Yes?

>      bool lpes0;
>  
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       *
>       * AIL is initialized here but can be cleared by
>       * selected exceptions
> +     *
> +     * guest_hv_excp is a provision to raise HV architected
> +     * exceptions in guests by delivering them with HV bit
> +     * clear. System reset and machine check use this.
>       */
>  #if defined(TARGET_PPC64)
>      if (excp_model == POWERPC_EXCP_POWER7 ||
> @@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          lpes0 = true;
>          ail = 0;
>      }
> +    guest_hv_excp = 0;
>  
>      /* Hypervisor emulation assistance interrupt only exists on server
>       * arch 2.05 server or later. We also don't want to generate it if
> @@ -214,6 +219,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>          }
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>  
>          /* machine check exceptions don't have ME set */
> @@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              msr |= 0x10000;
>              new_msr |= ((target_ulong)1 << MSR_ME);
>          }
> -
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> @@ -610,10 +616,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  
>      /* Sanity check */
>      if (!(env->msr_mask & MSR_HVB) && (srr0 == SPR_HSRR0)) {
> -        cpu_abort(cs, "Trying to deliver HV exception %d with "
> +        cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>                    "no HV support\n", excp);
>      }
>  
> +    /* The ISA specifies the HV bit is set when the hardware interrupt
> +     * is raised, however in some cases hypervisors deliver the exception
> +     * to guests. HV should be cleared in that case.
> +     */
> +    if (!(env->msr_mask & MSR_HVB) && (new_msr & MSR_HVB)) {
> +        if (guest_hv_excp) {
> +            new_msr &= ~MSR_HVB;
> +        } else {
> +            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
> +                      "no HV support\n", excp);
> +        }
> +    }
> +
>      /* If any alternate SRR register are defined, duplicate saved values */
>      if (asrr0 != -1) {
>          env->spr[asrr0] = env->spr[srr0];

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[Nicholas Piggin]

On Mon, 24 Oct 2016 12:16:19 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote:
> > On Fri, 21 Oct 2016 12:09:54 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:  
> > > > On Thu, 20 Oct 2016 15:08:07 +0200
> > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > >     
> > > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:    
> > > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > > > ---
> > > > > >  target-ppc/excp_helper.c | 8 ++++++--
> > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > > > > index 53c4075..477af10 100644
> > > > > > --- a/target-ppc/excp_helper.c
> > > > > > +++ b/target-ppc/excp_helper.c
> > > > > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > > > > >              /* indicate that we resumed from power save mode */
> > > > > >              msr |= 0x10000;
> > > > > >              new_msr |= ((target_ulong)1 << MSR_ME);
> > > > > > +            new_msr |= (target_ulong)MSR_HVB;
> > > > > > +        } else {
> > > > > > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > > > > > +	     * is raised, however when hypervisors deliver the exception to
> > > > > > +	     * guests, it should not be set.
> > > > > > +	     */
> > > > > >          }
> > > > > > -
> > > > > > -        new_msr |= (target_ulong)MSR_HVB;
> > > > > >          ail = 0;
> > > > > >          break;
> > > > > >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> > > > > >       
> > > > > 
> > > > > should not that be cleared later on in powerpc_excp() by :
> > > > > 
> > > > >     env->msr = new_msr & env->msr_mask;
> > > > > 
> > > > > ? but the routine is rather long so I might be missing a branch.    
> > > > 
> > > > No you're right, so it can't leak into the guest, phew!
> > > > 
> > > > The problem I get is the interrupt code doing some things differently
> > > > depending on on the HV bit. For example what I noticed is the guest
> > > > losing its LE bit upon entry.
> > > > 
> > > > Perhaps a cleaner way is for the system reset case to set new_msr
> > > > according to the ISA, and then apply the msr_mask (or at least mask
> > > > out HV) before calculating the exception model? Any preference?    
> > > 
> > > I think the proposed revision makes sense.
> > >   
> > 
> > What do you think of this version? This fixes up machine check guest
> > delivery as well. I'm sending this ahead of the new hcall patch, because
> > it's a bugfix for existing code. I'll get around to the hcall again next
> > week.
> > 
> > Thanks,
> > Nick
> > 
> > 
> > ppc hypervisors have delivered system reset and machine check exception
> > interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> > or NMI injection in QEMU).
> > 
> > These exceptions are architected to set the HV bit in hardware, however
> > when injected into a guest, the HV bit should be cleared. Current code
> > masks off the HV bit before setting the new MSR, however this happens after
> > the interrupt delivery model has calculated delivery mode for the exception.
> > This can result in the guest's MSR LE bit being lost.
> > 
> > Provide a new flag for HV exceptions to allow delivery to guests. The
> > exception model masks out the HV bit.
> > 
> > Also add another sanity check to ensure other such exceptions don't try
> > to set HV in guest without setting guest_hv_excp
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 53c4075..1b18433 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> >      CPUState *cs = CPU(cpu);
> >      CPUPPCState *env = &cpu->env;
> >      target_ulong msr, new_msr, vector;
> > -    int srr0, srr1, asrr0, asrr1, lev, ail;
> > +    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;  
> 
> So, to clarify my understanding of this.
> 
> The guest_hv_excp flag indicates that this is a normally-HV exception
> which *could* be delivered to a guest with HV clear, *not* that we're
> actually doing so in this instance.  Yes?

Correct.

Thanks,
Nick

Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

On Mon, Oct 24, 2016 at 05:56:22PM +1100, Nicholas Piggin wrote:
> On Mon, 24 Oct 2016 12:16:19 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 21, 2016 at 03:35:43PM +1100, Nicholas Piggin wrote:
> > > On Fri, 21 Oct 2016 12:09:54 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:  
> > > > > On Thu, 20 Oct 2016 15:08:07 +0200
> > > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > > >     
> > > > > > On 10/20/2016 08:59 AM, Nicholas Piggin wrote:    
> > > > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > > > > > ---
> > > > > > >  target-ppc/excp_helper.c | 8 ++++++--
> > > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > > > > > index 53c4075..477af10 100644
> > > > > > > --- a/target-ppc/excp_helper.c
> > > > > > > +++ b/target-ppc/excp_helper.c
> > > > > > > @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > > > > > >              /* indicate that we resumed from power save mode */
> > > > > > >              msr |= 0x10000;
> > > > > > >              new_msr |= ((target_ulong)1 << MSR_ME);
> > > > > > > +            new_msr |= (target_ulong)MSR_HVB;
> > > > > > > +        } else {
> > > > > > > +	    /* The ISA specifies the HV bit is set when the hardware interrupt
> > > > > > > +	     * is raised, however when hypervisors deliver the exception to
> > > > > > > +	     * guests, it should not be set.
> > > > > > > +	     */
> > > > > > >          }
> > > > > > > -
> > > > > > > -        new_msr |= (target_ulong)MSR_HVB;
> > > > > > >          ail = 0;
> > > > > > >          break;
> > > > > > >      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
> > > > > > >       
> > > > > > 
> > > > > > should not that be cleared later on in powerpc_excp() by :
> > > > > > 
> > > > > >     env->msr = new_msr & env->msr_mask;
> > > > > > 
> > > > > > ? but the routine is rather long so I might be missing a branch.    
> > > > > 
> > > > > No you're right, so it can't leak into the guest, phew!
> > > > > 
> > > > > The problem I get is the interrupt code doing some things differently
> > > > > depending on on the HV bit. For example what I noticed is the guest
> > > > > losing its LE bit upon entry.
> > > > > 
> > > > > Perhaps a cleaner way is for the system reset case to set new_msr
> > > > > according to the ISA, and then apply the msr_mask (or at least mask
> > > > > out HV) before calculating the exception model? Any preference?    
> > > > 
> > > > I think the proposed revision makes sense.
> > > >   
> > > 
> > > What do you think of this version? This fixes up machine check guest
> > > delivery as well. I'm sending this ahead of the new hcall patch, because
> > > it's a bugfix for existing code. I'll get around to the hcall again next
> > > week.
> > > 
> > > Thanks,
> > > Nick
> > > 
> > > 
> > > ppc hypervisors have delivered system reset and machine check exception
> > > interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> > > or NMI injection in QEMU).
> > > 
> > > These exceptions are architected to set the HV bit in hardware, however
> > > when injected into a guest, the HV bit should be cleared. Current code
> > > masks off the HV bit before setting the new MSR, however this happens after
> > > the interrupt delivery model has calculated delivery mode for the exception.
> > > This can result in the guest's MSR LE bit being lost.
> > > 
> > > Provide a new flag for HV exceptions to allow delivery to guests. The
> > > exception model masks out the HV bit.
> > > 
> > > Also add another sanity check to ensure other such exceptions don't try
> > > to set HV in guest without setting guest_hv_excp
> > > 
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > >  target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
> > >  1 file changed, 22 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > > index 53c4075..1b18433 100644
> > > --- a/target-ppc/excp_helper.c
> > > +++ b/target-ppc/excp_helper.c
> > > @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > >      CPUState *cs = CPU(cpu);
> > >      CPUPPCState *env = &cpu->env;
> > >      target_ulong msr, new_msr, vector;
> > > -    int srr0, srr1, asrr0, asrr1, lev, ail;
> > > +    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;  
> > 
> > So, to clarify my understanding of this.
> > 
> > The guest_hv_excp flag indicates that this is a normally-HV exception
> > which *could* be delivered to a guest with HV clear, *not* that we're
> > actually doing so in this instance.  Yes?
> 
> Correct.

Ok.  Hmm.  Could you please do a minor respin and:
  - change the flag to a bool
  - add a comment explaining this meaning
  - if you can think of a name which makes the meaning more obvious (I
    can't, quickly), change that too

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]