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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Apr 04, 2016 at 04:47:53PM +0200, Cédric Le Goater wrote:
> On 04/04/2016 06:16 AM, David Gibson wrote:
> > On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric Le Goater wrote:
> >> On 04/01/2016 04:43 AM, David Gibson wrote:
> >>> On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
> >>>> On 31/03/16 05:55, 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.
> >>>>>
> >>>>> Aren't there also circumstances where the exception prefix can depend
> >>>>> on the MSR?  Do those need to be handled somewhere?
> >>>>
> >>>> Yes indeed - this was part of my patchset last year to fix up various
> >>>> migration issues for the Mac PPC machines (see commit
> >>>> 2360b6e84f78d41fa0f76555a947148b73645259).
> >>>>
> >>>> I agree that having the env->excp_prefix logic split like this isn't a
> >>>> particularly great idea. Let's just have a single helper function as in
> >>>> the patch above and use that in both places (and in fact with recent
> >>>> changes I have a feeling env->excp_prefix is one of the few remaining
> >>>> reasons that the above change is still required, but please check this).
> >>>
> >>> Right.
> >>>
> >>> So, what I'd really prefer to see here is a single
> >>> update_excp_prefix() helper which will set env->excp_prefix based on
> >>> whatever registers are relevant (LPCR, MSR and probably the cpu
> >>> model).  That would be called on incoming migration, and after
> >>> updating any of the relevant registers (so from the mtmsr and mtlpcr
> >>> emulations).  H_SET_MODE should be handled by first updating the LPCR
> >>> value, then calling the update helper.
> >>>
> >>> Cédric, I'm ok if you provide a version of that which only handles
> >>> POWER7 and POWER8 (i.e. spapr compatible models for now).  
> >>
> >> Sure. 
> >>
> >> But first, could you please take a look at a reworked patch of Ben who 
> >> had forseen the issue ? I kept the AIL fix, added some extra for ILE
> >> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine.
> >>
> >> If you think this is too risky for 2.6, I will work on what you asked for.
> > 
> > Ah, yes this approach does seem more correct.  It looks like it leaves
> > room for further cleanups to excp_prefix later (such as completely
> > removing it in favour of calculating it from reg values at exception
> > time).  
> 
> Yes. There is still a :
> 
> 	vector |= env->excp_prefix;
> 
> which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used. 
> The code section calculating 'vector' in powerpc_excp() could be isolated 
> in its own routine I guess. We could clarify things there.

Right.

> > But for now it looks like it will address the migration issue at hand.
> > 
> > I've applied it to ppc-for-2.6.
> 
> Tell me if you want a proper resend by mail.

No, I think I have it adequately cleaned up myself.

-- 
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 --]