qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Fabiano Rosas <farosas@linux.ibm.com>
To: Nicholas Piggin <npiggin@gmail.com>, qemu-ppc@nongnu.org
Cc: "Cédric Le Goater" <clg@fr.ibm.com>,
	qemu-devel@nongnu.org, "Nicholas Piggin" <npiggin@gmail.com>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
Date: Tue, 13 Apr 2021 12:53:55 -0300	[thread overview]
Message-ID: <87v98q41v0.fsf@linux.ibm.com> (raw)
In-Reply-To: <20210413125448.1689545-4-npiggin@gmail.com>

Nicholas Piggin <npiggin@gmail.com> writes:

> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
> and it removes support for the LPCR[AIL]=0b10 mode.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c            |   5 ++
>  target/ppc/cpu-qom.h            |   2 +
>  target/ppc/cpu.h                |   5 +-
>  target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
>  target/ppc/translate.c          |   3 +-
>  target/ppc/translate_init.c.inc |   2 +-
>  6 files changed, 107 insertions(+), 24 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 7b5cd3553c..2f65f20f72 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>          return H_UNSUPPORTED_FLAG;
>      }
>  
> +    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
> +        /* AIL 2 is also reserved in ISA v3.1 */
> +        return H_UNSUPPORTED_FLAG;
> +    }
> +
>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>  
>      return H_SUCCESS;
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 118baf8d41..06b6571bc9 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -116,6 +116,8 @@ enum powerpc_excp_t {
>      POWERPC_EXCP_POWER8,
>      /* POWER9 exception model           */
>      POWERPC_EXCP_POWER9,
> +    /* POWER10 exception model           */
> +    POWERPC_EXCP_POWER10,
>  };
>  
>  /*****************************************************************************/
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e73416da68..24e53e0ac3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t {
>  #define LPCR_PECE_U_SHIFT (63 - 19)
>  #define LPCR_PECE_U_MASK  (0x7ull << LPCR_PECE_U_SHIFT)
>  #define LPCR_HVEE         PPC_BIT(17) /* Hypervisor Virt Exit Enable */
> -#define LPCR_RMLS_SHIFT   (63 - 37)
> +#define LPCR_RMLS_SHIFT   (63 - 37)   /* RMLS (removed in ISA v3.0) */
>  #define LPCR_RMLS         (0xfull << LPCR_RMLS_SHIFT)
> +#define LPCR_HAIL         PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */
>  #define LPCR_ILE          PPC_BIT(38)
> -#define LPCR_AIL_SHIFT    (63 - 40)      /* Alternate interrupt location */
> +#define LPCR_AIL_SHIFT    (63 - 40)   /* Alternate interrupt location */
>  #define LPCR_AIL          (3ull << LPCR_AIL_SHIFT)
>  #define LPCR_UPRT         PPC_BIT(41) /* Use Process Table */
>  #define LPCR_EVIRT        PPC_BIT(42) /* Enhanced Virtualisation */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index b8881c0f85..e8bf0598b4 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -197,7 +197,8 @@ 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 = -1, ail;
> +    int srr0, srr1, asrr0, asrr1, lev = -1;
> +    int ail, hail, using_ail;
>      bool lpes0;
>  
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -239,24 +240,39 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       * On anything else, we behave as if LPES0 is 1
>       * (externals don't alter MSR:HV)
>       *
> -     * AIL is initialized here but can be cleared by
> -     * selected exceptions
> +     * ail/hail are initialized here but can be cleared by
> +     * selected exceptions, and other conditions afterwards.
>       */
>  #if defined(TARGET_PPC64)
>      if (excp_model == POWERPC_EXCP_POWER7 ||
>          excp_model == POWERPC_EXCP_POWER8 ||
> -        excp_model == POWERPC_EXCP_POWER9) {
> +        excp_model == POWERPC_EXCP_POWER9 ||
> +        excp_model == POWERPC_EXCP_POWER10) {
>          lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
>          if (excp_model != POWERPC_EXCP_POWER7) {
>              ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>          } else {
>              ail = 0;
>          }
> +        if (excp_model == POWERPC_EXCP_POWER10) {
> +            if (AIL_0001_8000) {

if (2)

> +                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
> +            }
> +
> +            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
> +                hail = AIL_C000_0000_0000_4000;
> +            } else {
> +                hail = 0;
> +            }
> +        } else {
> +            hail = ail;

I think it's better if we set hail = 0 here. Read on...

> +        }
>      } else
>  #endif /* defined(TARGET_PPC64) */
>      {
>          lpes0 = true;
>          ail = 0;
> +        hail = 0;
>      }
>  
>      /*
> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              new_msr |= (target_ulong)MSR_HVB;
>          }
>          ail = 0;
> +        hail = 0;
>  
>          /* machine check exceptions don't have ME set */
>          new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              }
>          }
>          ail = 0;
> +        hail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>              new_msr |= (target_ulong)1 << MSR_LE;
>          }
> -    } else if (excp_model == POWERPC_EXCP_POWER9) {
> +    } else if (excp_model == POWERPC_EXCP_POWER9 ||
> +               excp_model == POWERPC_EXCP_POWER10) {
>          if (new_msr & MSR_HVB) {
>              if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>                  new_msr |= (target_ulong)1 << MSR_LE;
> @@ -791,22 +810,77 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  #endif
>  
>      /*
> -     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
> +     * The ail variable is for AIL behaviour for interrupts that begin
> +     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
> +     * interrupts that begin execution with MSR[HV]=1.
> +     *
> +     * There is a large matrix of behaviours depending on the processor and
> +     * the current operating modes when the interrupt is taken, and the
> +     * interrupt type. The below tables lists behaviour for interrupts except
> +     * for SRESET, machine check, and HMI (which are all taken in real mode
> +     * with AIL 0).
> +     *
> +     * POWER8, POWER9 with LPCR[HR]=0
> +     * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> +     * +-----------+-------------+---------+-------------+-----+
> +     * | a         | 00/01/10    | x       | x           | 0   |
> +     * | a         | 11          | 0       | 1           | 0   |
> +     * | a         | 11          | 1       | 1           | a   |
> +     * | a         | 11          | 0       | 0           | a   |
> +     * +-------------------------------------------------------+
> +     *
> +     * POWER9 with LPCR[HR]=1
> +     * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> +     * +-----------+-------------+---------+-------------+-----+
> +     * | a         | 00/01/10    | x       | x           | 0   |
> +     * | a         | 11          | x       | x           | a   |
> +     * +-------------------------------------------------------+
> +     *
> +     * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
> +     * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
> +     * This is good for performance but allows the guest to influence the
> +     * AIL of the hypervisor interrupt using its MSR, and also the hypervisor
> +     * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the
> +     * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts.
> +     *
> +     * POWER10 addresses those issues with a new LPCR[HAIL] bit that is
> +     * applied to interrupt that begin execution with MSR[HV]=1 (so both
> +     * MSR[HV] 0->1 and 1->1).

I'd put take these two paragraphs in the commit message as well. Just
alter the beginning:
s/The difference with POWER9 being that/In POWER9,/

> +     *
> +     * POWER10 behaviour is
> +     * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> +     * +-----------+------------+-------------+---------+-------------+-----+
> +     * | a         | h          | 00/01/10    | 0       | 0           | 0   |
> +     * | a         | h          | 11          | 0       | 0           | a   |
> +     * | a         | h          | x           | 0       | 1           | h   |
> +     * | a         | h          | 00/01/10    | 1       | 1           | 0   |
> +     * | a         | h          | 11          | 1       | 1           | h   |
> +     * +--------------------------------------------------------------------+
>       */
> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
> -        ail = 0;
> -    }
>  
> -    /*
> -     * AIL does not work if there is a MSR[HV] 0->1 transition and the
> -     * partition is in HPT mode. For radix guests, such interrupts are
> -     * allowed to be delivered to the hypervisor in ail mode.
> -     */
> -    if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
> -        if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
> +    if (excp_model == POWERPC_EXCP_POWER8 ||
> +        excp_model == POWERPC_EXCP_POWER9) {
> +        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>              ail = 0;
> +            hail = 0;
> +        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
> +            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
> +                hail = 0;
> +            }
> +        }
> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
> +        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
> +            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
> +                ail = 0;
> +                hail = 0;
> +            }
>          }
>      }
> +    if (new_msr & MSR_HVB) {
> +        using_ail = hail;
> +    } else {
> +        using_ail = ail;
> +    }

So if at the start of the function we set:

<= P7:
ail = 0;
hail = 0;

P8,P9:
ail = LPCR_AIL;
hail = 0;

P10:
ail = LPCR_AIL, except AIL2;
hail = AIL3;

Then we could do this, which I think is more readable (comments are just
for the email):

// If there's an HV transition to 1, then HPT zeroes AIL and radix
// doesn't. For P10 we'll use HAIL anyway, so no need to check.

if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
    !(env->spr[SPR_LPCR] & LPCR_HR)) {
          ail = 0;
}

// If P10 and HV=1, use HAIL instead of AIL

if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
   ail = hail;
}

// The IR|DR check always takes precedence and zeroes AIL/HAIL, no
matter the processor version.

if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
    ail = 0;
}

(...)

vector |= ppc_excp_vector_offset(cs, ail);

I think that should work...*squints* 
What do you think?

>  
>      vector = env->excp_vectors[excp];
>      if (vector == (target_ulong)-1ULL) {
> @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          env->spr[srr1] = msr;
>  
>          /* Handle AIL */
> -        if (ail) {
> +        if (using_ail) {
>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -            vector |= ppc_excp_vector_offset(cs, ail);
> +            vector |= ppc_excp_vector_offset(cs, using_ail);
>          }
>  
>  #if defined(TARGET_PPC64)
>      } else {
>          /* scv AIL is a little different */
> -        if (ail) {
> +        if (using_ail) {
>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>          }
> -        if (ail == AIL_C000_0000_0000_4000) {
> +        if (using_ail == AIL_C000_0000_0000_4000) {
>              vector |= 0xc000000000003000ull;
>          } else {
>              vector |= 0x0000000000017000ull;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0984ce637b..e9ed001229 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #if defined(TARGET_PPC64)
>      if (env->excp_model == POWERPC_EXCP_POWER7 ||
>          env->excp_model == POWERPC_EXCP_POWER8 ||
> -        env->excp_model == POWERPC_EXCP_POWER9)  {
> +        env->excp_model == POWERPC_EXCP_POWER9 ||
> +        env->excp_model == POWERPC_EXCP_POWER10)  {
>          qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
>                       env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
>      }
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 70f9b9b150..5d427e9d38 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>      pcc->radix_page_info = &POWER10_radix_page_info;
>      pcc->lrg_decr_bits = 56;
>  #endif
> -    pcc->excp_model = POWERPC_EXCP_POWER9;
> +    pcc->excp_model = POWERPC_EXCP_POWER10;
>      pcc->bus_model = PPC_FLAGS_INPUT_POWER9;
>      pcc->bfd_mach = bfd_mach_ppc64;
>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |


  reply	other threads:[~2021-04-13 15:58 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin
2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
2021-04-13 13:48   ` Fabiano Rosas
2021-04-14  3:26     ` Nicholas Piggin
2021-04-13 14:31   ` [EXTERNAL] " Cédric Le Goater
2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
2021-04-13 14:18   ` [EXTERNAL] " Cédric Le Goater
2021-04-13 15:54   ` Fabiano Rosas
2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin
2021-04-13 15:53   ` Fabiano Rosas [this message]
2021-04-13 17:09     ` Cédric Le Goater
2021-04-14  1:56       ` Nicholas Piggin
2021-04-14  3:28     ` Nicholas Piggin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v98q41v0.fsf@linux.ibm.com \
    --to=farosas@linux.ibm.com \
    --cc=clg@fr.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=npiggin@gmail.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --subject='Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).