Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR

["Cédric Le Goater" <clg@fr.ibm.com>]

On 03/31/2016 06:55 AM, David Gibson wrote:
> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be restored when migrating a spapr VM running in
>> TCG. This can be done using the AIL bits from the LPCR register.
>>
>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>> returns the effective address offset of the interrupt handler
>> depending on the LPCR_AIL bits. The same helper can be used in the
>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>> defines.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> 
> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> since it certainly improves behaviour, although I have a couple of
> queries:
> 
>> ---
>>
>>  Changes since v1:
>>
>>  - moved helper routine under target-ppc/
>>  - moved the restore of excp_prefix under cpu_post_load()
>>
>>  hw/ppc/spapr_hcall.c        |   13 ++-----------
>>  include/hw/ppc/spapr.h      |    5 -----
>>  target-ppc/cpu.h            |    9 +++++++++
>>  target-ppc/machine.c        |   20 +++++++++++++++++++-
>>  target-ppc/translate_init.c |   14 ++++++++++++++
>>  5 files changed, 44 insertions(+), 17 deletions(-)
>>
>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>>          return H_P4;
>>      }
>>  
>> -    switch (mflags) {
>> -    case H_SET_MODE_ADDR_TRANS_NONE:
>> -        prefix = 0;
>> -        break;
>> -    case H_SET_MODE_ADDR_TRANS_0001_8000:
>> -        prefix = 0x18000;
>> -        break;
>> -    case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>> -        prefix = 0xC000000000004000ULL;
>> -        break;
>> -    default:
>> +    prefix = cpu_ppc_get_excp_prefix(mflags);
>> +    if (prefix == (target_ulong) -1ULL) {
>>          return H_UNSUPPORTED_FLAG;
>>      }
>>  
>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>>      }
>>  }
>>  
>> +
>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>> +{
>> +    int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> +    target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>> +
>> +    if (prefix == (target_ulong) -1ULL) {
>> +        return -EINVAL;
>> +    }
>> +    env->excp_prefix = prefix;
>> +    return 0;
>> +}
>> +
>>  static int cpu_post_load(void *opaque, int version_id)
>>  {
>>      PowerPCCPU *cpu = opaque;
>>      CPUPPCState *env = &cpu->env;
>>      int i;
>>      target_ulong msr;
>> +    int ret = 0;
>>  
>>      /*
>>       * We always ignore the source PVR. The user or management
>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>  
>>      hreg_compute_mem_idx(env);
>>  
>> -    return 0;
>> +    if (env->spr[SPR_LPCR] & LPCR_AIL) {
>> +        ret = cpu_post_load_excp_prefix(env);
>> +    }
> 
> Why not call this unconditionally?  If AIL == 0 it will still do the
> right thing.

yes, indeed. the test is not required.

> Aren't there also circumstances where the exception prefix can depend
> on the MSR?  Do those need to be handled somewhere?

Well, if translation is not enabled, LPCR_AIL should be 0, and so should 
be excp_prefix. Is that something we should check/enforce at restart ? 


Looking closer, we could probably get rid of excp_prefix (for spapr) and 
compute the value directly from LPCR in powerpc_excp() where we really need 
it.

C.

>> +
>> +    return ret;
>>  }
>>  
>>  static bool fpu_needed(void *opaque)
>> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
>>      }
>>  }
>>  
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
>> +{
>> +    switch (ail) {
>> +    case AIL_NONE:
>> +        return 0;
>> +    case AIL_0001_8000:
>> +        return 0x18000;
>> +    case AIL_C000_0000_0000_4000:
>> +        return 0xC000000000004000ULL;
>> +    default:
>> +        return (target_ulong) -1ULL;
>> +    }
>> +}
>> +
>>  #endif /* !defined(CONFIG_USER_ONLY) */
>>  
>>  #endif /* defined (TARGET_PPC64) */
>> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
>>  void ppc_tlb_invalidate_all (CPUPPCState *env);
>>  void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
>>  void cpu_ppc_set_papr(PowerPCCPU *cpu);
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
>>  #endif
>>  #endif
>>  
>> @@ -2277,6 +2278,14 @@ enum {
>>      HMER_XSCOM_STATUS_LSH       = (63 - 23),
>>  };
>>  
>> +/* Alternate Interrupt Location (AIL) */
>> +enum {
>> +    AIL_NONE                = 0,
>> +    AIL_RESERVED            = 1,
>> +    AIL_0001_8000           = 2,
>> +    AIL_C000_0000_0000_4000 = 3,
>> +};
>> +
>>  /*****************************************************************************/
>>  
>>  static inline target_ulong cpu_read_xer(CPUPPCState *env)
>> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
>> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
>>  #define H_SET_MODE_ENDIAN_BIG    0
>>  #define H_SET_MODE_ENDIAN_LITTLE 1
>>  
>> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
>> -#define H_SET_MODE_ADDR_TRANS_NONE                  0
>> -#define H_SET_MODE_ADDR_TRANS_0001_8000             2
>> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000   3
>> -
>>  /* VASI States */
>>  #define H_VASI_INVALID    0
>>  #define H_VASI_ENABLED    1
>>
>