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

[David Gibson]

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 
> >>>
> >>> 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 
> >>> [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 
> >>> ---
> >>>  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,
> >>>   _read_generic, _write_generic,
> >>>   KVM_REG_PPC_UAMOR, 0);
> >>> +spr_register_hv(env, SPR_AMOR, "AMOR",
> >>> +SPR_NOACCESS, SPR_NOACCESS,
> >>> +SPR_NOACCESS, SPR_NOACCESS,
> >>> +_read_generic, _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, 0x);
> >>>  }
> >>>  
> >>> +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,
> >>> +_read_generic, _write_generic,
> >>> +KVM_REG_PPC_DAWR, 0x);
> >>> +spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
> >>> +SPR_NOACCESS, SPR_NOACCESS,
> >>> +SPR_NOACCESS, SPR_NOACCESS,
> >>> +_read_generic, _write_generic,
> >>> +KVM_REG_PPC_DAWRX, 0x);
> >>> +}
> >>> +
> >>>  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,
> >>>   _read_generic, _write_generic,
> >>> - KVM_REG_PPC_LPCR, 0x);
> >>> + 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


signature.asc
Description: PGP signature

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

[Thomas Huth]

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 
>>>
>>> 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 
>>> [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 
>>> ---
>>>  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,
>>>   _read_generic, _write_generic,
>>>   KVM_REG_PPC_UAMOR, 0);
>>> +spr_register_hv(env, SPR_AMOR, "AMOR",
>>> +SPR_NOACCESS, SPR_NOACCESS,
>>> +SPR_NOACCESS, SPR_NOACCESS,
>>> +_read_generic, _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, 0x);
>>>  }
>>>  
>>> +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,
>>> +_read_generic, _write_generic,
>>> +KVM_REG_PPC_DAWR, 0x);
>>> +spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
>>> +SPR_NOACCESS, SPR_NOACCESS,
>>> +SPR_NOACCESS, SPR_NOACCESS,
>>> +_read_generic, _write_generic,
>>> +KVM_REG_PPC_DAWRX, 0x);
>>> +}
>>> +
>>>  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,
>>>   _read_generic, _write_generic,
>>> - KVM_REG_PPC_LPCR, 0x);
>>> + 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.

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




signature.asc
Description: OpenPGP digital signature

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

[David Gibson]

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 
> > 
> > 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 
> > [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 
> > ---
> >  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,
> >   _read_generic, _write_generic,
> >   KVM_REG_PPC_UAMOR, 0);
> > +spr_register_hv(env, SPR_AMOR, "AMOR",
> > +SPR_NOACCESS, SPR_NOACCESS,
> > +SPR_NOACCESS, SPR_NOACCESS,
> > +_read_generic, _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, 0x);
> >  }
> >  
> > +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,
> > +_read_generic, _write_generic,
> > +KVM_REG_PPC_DAWR, 0x);
> > +spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
> > +SPR_NOACCESS, SPR_NOACCESS,
> > +SPR_NOACCESS, SPR_NOACCESS,
> > +_read_generic, _write_generic,
> > +KVM_REG_PPC_DAWRX, 0x);
> > +}
> > +
> >  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,
> >   _read_generic, _write_generic,
> > - KVM_REG_PPC_LPCR, 0x);
> > + 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.

> >  }
> >  
> > +#if !defined(CONFIG_USER_ONLY)
> > +static void spr_write_hmer(DisasContext *ctx, int sprn, int gprn)
> > +{
> > +TCGv hmer = tcg_temp_new();
> > +
> > +gen_load_spr(hmer, sprn);
> > +tcg_gen_and_tl(hmer, cpu_gpr[gprn], hmer);
> > +gen_store_spr(sprn, hmer);
> > +spr_store_dump_spr(sprn);
> > +tcg_temp_free(hmer);
> > +}
> > +#endif
> > +
> >  static void gen_spr_book3s_ids(CPUPPCState *env)
> >  {
> > +/* FIXME: Will need to deal with thread vs core only SPRs */
> > +
> >  /* Processor identification */
> > -spr_register(env, SPR_PIR, "PIR",
> > +spr_register_hv(env, SPR_PIR, "PIR",
> >   SPR_NOACCESS, SPR_NOACCESS,
> > - _read_generic, _write_pir,
> > + SPR_NOACCESS, SPR_NOACCESS,
> > + _read_generic, NULL,
> > + 0x);
> 
> What does the NULL mean here? I haven't seen any other spr_register*()
> calls yet that pass NULL as parameter for a handler. Should that maybe
> rather be a SPR_NOACCESS instead?
> 
>  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


signature.asc
Description: PGP signature

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

[Thomas Huth]

On 14.03.2016 17:56, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt 
> 
> 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 
> [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 
> ---
>  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,
>   _read_generic, _write_generic,
>   KVM_REG_PPC_UAMOR, 0);
> +spr_register_hv(env, SPR_AMOR, "AMOR",
> +SPR_NOACCESS, SPR_NOACCESS,
> +SPR_NOACCESS, SPR_NOACCESS,
> +_read_generic, _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, 0x);
>  }
>  
> +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,
> +_read_generic, _write_generic,
> +KVM_REG_PPC_DAWR, 0x);
> +spr_register_kvm_hv(env, SPR_DAWRX, "DAWRX",
> +SPR_NOACCESS, SPR_NOACCESS,
> +SPR_NOACCESS, SPR_NOACCESS,
> +_read_generic, _write_generic,
> +KVM_REG_PPC_DAWRX, 0x);
> +}
> +
>  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,
>   _read_generic, _write_generic,
> - KVM_REG_PPC_LPCR, 0x);
> + 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).

>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +static void spr_write_hmer(DisasContext *ctx, int sprn, int gprn)
> +{
> +TCGv hmer = tcg_temp_new();
> +
> +gen_load_spr(hmer, sprn);
> +tcg_gen_and_tl(hmer, cpu_gpr[gprn], hmer);
> +gen_store_spr(sprn, hmer);
> +spr_store_dump_spr(sprn);
> +tcg_temp_free(hmer);
> +}
> +#endif
> +
>  static void gen_spr_book3s_ids(CPUPPCState *env)
>  {
> +/* FIXME: Will need to deal with thread vs core only SPRs */
> +
>  /* Processor identification */
> -spr_register(env, SPR_PIR, "PIR",
> +spr_register_hv(env, SPR_PIR, "PIR",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_pir,
> + SPR_NOACCESS, SPR_NOACCESS,
> + _read_generic, NULL,
> + 0x);

What does the NULL mean here? I haven't seen any other spr_register*()
calls yet that pass NULL as parameter for a handler. Should that maybe
rather be a SPR_NOACCESS instead?

 Thomas