qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Cédric Le Goater" <clg@kaod.org>
To: Fabiano Rosas <farosas@linux.ibm.com>,
	Nicholas Piggin <npiggin@gmail.com>, <qemu-ppc@nongnu.org>
Cc: "Cédric Le Goater" <clg@fr.ibm.com>,
	qemu-devel@nongnu.org,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
Date: Tue, 13 Apr 2021 19:09:34 +0200	[thread overview]
Message-ID: <9fecb60b-8074-83d4-8c59-2d324d576e06@kaod.org> (raw)
In-Reply-To: <87v98q41v0.fsf@linux.ibm.com>

On 4/13/21 5:53 PM, Fabiano Rosas wrote:
> 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?

The powerpc_excp() routine is insane.

It feels that we should be duplicating the code in handlers for each 
of the POWER[7-10] CPUs and keep a default powerpc_excp() for the
others.

C.

> 
>>  
>>      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 17:13 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
2021-04-13 17:09     ` Cédric Le Goater [this message]
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=9fecb60b-8074-83d4-8c59-2d324d576e06@kaod.org \
    --to=clg@kaod.org \
    --cc=clg@fr.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=farosas@linux.ibm.com \
    --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).