Re: [Qemu-devel] [PATCH 03/17] ppc: Add a bunch of hypervisor SPRs to Book3s

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

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

On Tue, Mar 15, 2016 at 11:49:31AM +0100, Thomas Huth wrote:
> On 15.03.2016 10:43, David Gibson wrote:
> > 
> > On Mon, Mar 14, 2016 at 08:14:59PM +0100, Thomas Huth wrote:
> >> On 14.03.2016 17:56, Cédric Le Goater wrote:
> >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>
> >>> We don't give them a KVM reg number to most of the registers yet as no
> >>> current KVM version supports HV mode. For DAWR and DAWRX, the KVM reg
> >>> number is needed since this register can be set by the guest via the
> >>> H_SET_MODE hypercall.
> >>>
> >>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>> [clg: squashed in patch 'ppc: Add KVM numbers to some P8 SPRs' and
> >>>       changed the commit log with a proposal of Thomas Huth ]
> >>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >>> ---
> >>>  target-ppc/translate_init.c | 140 +++++++++++++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 137 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> >>> index 6a11b41206e5..43c6e524a6bc 100644
> >>> --- a/target-ppc/translate_init.c
> >>> +++ b/target-ppc/translate_init.c
> >>> @@ -1105,6 +1105,11 @@ static void gen_spr_amr (CPUPPCState *env)
> >>>                       SPR_NOACCESS, SPR_NOACCESS,
> >>>                       &spr_read_generic, &spr_write_generic,
> >>>                       KVM_REG_PPC_UAMOR, 0);
> >>> +    spr_register_hv(env, SPR_AMOR, "AMOR",
> >>> +                    SPR_NOACCESS, SPR_NOACCESS,
> >>> +                    SPR_NOACCESS, SPR_NOACCESS,
> >>> +                    &spr_read_generic, &spr_write_generic,
> >>> +                    0);
> >>>  #endif /* !CONFIG_USER_ONLY */
> >>>  }
> >>>  #endif /* TARGET_PPC64 */
> >>> @@ -7491,6 +7496,20 @@ static void gen_spr_book3s_dbg(CPUPPCState *env)
> >>>                       KVM_REG_PPC_DABRX, 0x00000000);
> >>>  }
> >>>  
> >>> +static void gen_spr_book3s_207_dbg(CPUPPCState *env)
> >>> +{
> >>> +    spr_register_kvm_hv(env, SPR_DAWR, "DAWR",
> >>> +                        SPR_NOACCESS, SPR_NOACCESS,
> >>> +                        SPR_NOACCESS, SPR_NOACCESS,
> >>> +                        &spr_read_generic, &spr_write_generic,
> >>> +                        KVM_REG_PPC_DAWR, 0x00000000);
> >>> +    spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
> >>> +                        SPR_NOACCESS, SPR_NOACCESS,
> >>> +                        SPR_NOACCESS, SPR_NOACCESS,
> >>> +                        &spr_read_generic, &spr_write_generic,
> >>> +                        KVM_REG_PPC_DAWRX, 0x00000000);
> >>> +}
> >>> +
> >>>  static void gen_spr_970_dbg(CPUPPCState *env)
> >>>  {
> >>>      /* Breakpoints */
> >>> @@ -7683,15 +7702,116 @@ static void gen_spr_power5p_lpar(CPUPPCState *env)
> >>>      spr_register_kvm(env, SPR_LPCR, "LPCR",
> >>>                       SPR_NOACCESS, SPR_NOACCESS,
> >>>                       &spr_read_generic, &spr_write_generic,
> >>> -                     KVM_REG_PPC_LPCR, 0x00000000);
> >>> +                     KVM_REG_PPC_LPCR, LPCR_LPES0 | LPCR_LPES1);
> >>
> >> Could we please postpone that hunk to a later, separate patch (after
> >> QEMU 2.6 has been released)? It looks like it could maybe cause some
> >> trouble with some emulated boards (e.g. there is some code in
> >> target-ppc/excp_helper.c for example - which is currently disabled, but
> >> I'm not sure whether there are other spots like this somewhere else).
> > 
> > I think this whole patch needs to wait until after 2.6, I'm not seeing
> > a good rationale for squeezing it into 2.6 at this stage.
> 
> Well, this patch registers DAWR and DAWRX registers with KVM - so
> without this patch, the hardware breakpoints will be lost during
> migration. I haven't tested it, but I think that when somebody uses
> hardware breakpoints in gdb in a KVM guest, and migrates it, then the
> breakpoints won't be triggered anymore after migration without this patch.

Ah.. good point.  So the question becomes, which is lower risk:
adjusting the patches to just add DAWR without the HV SPR stuff, or
just incorporating the HV SPR stuff as is.

> Cédric, maybe you could send a patch that adds at least the DAWR and
> DAWRX registers if David does not want to have the full patch for 2.6?
> 
>  Thomas
> 
> 



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