* [PATCH 1/7] target/ppc: Implement the VTB for HV access
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2019-11-29 1:39 ` David Gibson
2019-11-28 13:46 ` [PATCH 2/7] target/ppc: Work [S]PURR implementation and add HV support Cédric Le Goater
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
The virtual timebase register (VTB) is a 64-bit register which
increments at the same rate as the timebase register, present on POWER8
and later processors.
The register is able to be read/written by the hypervisor and read by
the supervisor. All other accesses are illegal.
Currently the VTB is just an alias for the timebase (TB) register.
Implement the VTB so that is can be read/written independent of the TB.
Make use of the existing method for accessing timebase facilities where
by the compensation is stored and used to compute the value on reads/is
updated on writes.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
[ clg: rebased on current ppc tree ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/ppc/ppc.h | 1 +
target/ppc/cpu.h | 2 ++
target/ppc/helper.h | 2 ++
hw/ppc/ppc.c | 16 ++++++++++++++++
linux-user/ppc/cpu_loop.c | 5 +++++
target/ppc/timebase_helper.c | 10 ++++++++++
target/ppc/translate_init.inc.c | 19 +++++++++++++++----
7 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 585be6ab98c5..02481cd27c36 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -24,6 +24,7 @@ struct ppc_tb_t {
/* Time base management */
int64_t tb_offset; /* Compensation */
int64_t atb_offset; /* Compensation */
+ int64_t vtb_offset;
uint32_t tb_freq; /* TB frequency */
/* Decrementer management */
uint64_t decr_next; /* Tick for next decr interrupt */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e3e82327b723..19d6e724bb5a 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1305,6 +1305,8 @@ uint64_t cpu_ppc_load_atbl(CPUPPCState *env);
uint32_t cpu_ppc_load_atbu(CPUPPCState *env);
void cpu_ppc_store_atbl(CPUPPCState *env, uint32_t value);
void cpu_ppc_store_atbu(CPUPPCState *env, uint32_t value);
+uint64_t cpu_ppc_load_vtb(CPUPPCState *env);
+void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value);
bool ppc_decr_clear_on_delivery(CPUPPCState *env);
target_ulong cpu_ppc_load_decr(CPUPPCState *env);
void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index f843814b8aa8..a5f53bb421a7 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -649,6 +649,7 @@ DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_tbu, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_atbl, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_atbu, TCG_CALL_NO_RWG, tl, env)
+DEF_HELPER_FLAGS_1(load_vtb, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_601_rtcl, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
#if !defined(CONFIG_USER_ONLY)
@@ -669,6 +670,7 @@ DEF_HELPER_FLAGS_1(load_decr, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_2(store_decr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_1(load_hdecr, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_2(store_hdecr, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_FLAGS_2(store_vtb, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_2(store_hid0_601, void, env, tl)
DEF_HELPER_3(store_403_pbr, void, env, i32, tl)
DEF_HELPER_FLAGS_1(load_40x_pit, TCG_CALL_NO_RWG, tl, env)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 8dd982fc1e40..263922052536 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -694,6 +694,22 @@ void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value)
&tb_env->atb_offset, ((uint64_t)value << 32) | tb);
}
+uint64_t cpu_ppc_load_vtb(CPUPPCState *env)
+{
+ ppc_tb_t *tb_env = env->tb_env;
+
+ return cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ tb_env->vtb_offset);
+}
+
+void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value)
+{
+ ppc_tb_t *tb_env = env->tb_env;
+
+ cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ &tb_env->vtb_offset, value);
+}
+
static void cpu_ppc_tb_stop (CPUPPCState *env)
{
ppc_tb_t *tb_env = env->tb_env;
diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
index d5704def2902..5b27f8603e33 100644
--- a/linux-user/ppc/cpu_loop.c
+++ b/linux-user/ppc/cpu_loop.c
@@ -47,6 +47,11 @@ uint32_t cpu_ppc_load_atbu(CPUPPCState *env)
return cpu_ppc_get_tb(env) >> 32;
}
+uint64_t cpu_ppc_load_vtb(CPUPPCState *env)
+{
+ return cpu_ppc_get_tb(env);
+}
+
uint32_t cpu_ppc601_load_rtcu(CPUPPCState *env)
__attribute__ (( alias ("cpu_ppc_load_tbu") ));
diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
index 73363e08ae71..8c3c2fe67c2c 100644
--- a/target/ppc/timebase_helper.c
+++ b/target/ppc/timebase_helper.c
@@ -45,6 +45,11 @@ target_ulong helper_load_atbu(CPUPPCState *env)
return cpu_ppc_load_atbu(env);
}
+target_ulong helper_load_vtb(CPUPPCState *env)
+{
+ return cpu_ppc_load_vtb(env);
+}
+
#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
target_ulong helper_load_purr(CPUPPCState *env)
{
@@ -113,6 +118,11 @@ void helper_store_hdecr(CPUPPCState *env, target_ulong val)
cpu_ppc_store_hdecr(env, val);
}
+void helper_store_vtb(CPUPPCState *env, target_ulong val)
+{
+ cpu_ppc_store_vtb(env, val);
+}
+
target_ulong helper_load_40x_pit(CPUPPCState *env)
{
return load_40x_pit(env);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index ba726dec4d00..5a560164d4a4 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -312,6 +312,16 @@ static void spr_write_hdecr(DisasContext *ctx, int sprn, int gprn)
}
}
+static void spr_read_vtb(DisasContext *ctx, int gprn, int sprn)
+{
+ gen_helper_load_vtb(cpu_gpr[gprn], cpu_env);
+}
+
+static void spr_write_vtb(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_helper_store_vtb(cpu_env, cpu_gpr[gprn]);
+}
+
#endif
#endif
@@ -8169,10 +8179,11 @@ static void gen_spr_power8_ebb(CPUPPCState *env)
/* Virtual Time Base */
static void gen_spr_vtb(CPUPPCState *env)
{
- spr_register_kvm(env, SPR_VTB, "VTB",
- SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_tbl, SPR_NOACCESS,
- KVM_REG_PPC_VTB, 0x00000000);
+ spr_register_kvm_hv(env, SPR_VTB, "VTB",
+ SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_vtb, SPR_NOACCESS,
+ &spr_read_vtb, &spr_write_vtb,
+ KVM_REG_PPC_VTB, 0x00000000);
}
static void gen_spr_power8_fscr(CPUPPCState *env)
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/7] target/ppc: Implement the VTB for HV access
2019-11-28 13:46 ` [PATCH 1/7] target/ppc: Implement the VTB for HV access Cédric Le Goater
@ 2019-11-29 1:39 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2019-11-29 1:39 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7664 bytes --]
On Thu, Nov 28, 2019 at 02:46:54PM +0100, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> The virtual timebase register (VTB) is a 64-bit register which
> increments at the same rate as the timebase register, present on POWER8
> and later processors.
>
> The register is able to be read/written by the hypervisor and read by
> the supervisor. All other accesses are illegal.
>
> Currently the VTB is just an alias for the timebase (TB) register.
>
> Implement the VTB so that is can be read/written independent of the TB.
> Make use of the existing method for accessing timebase facilities where
> by the compensation is stored and used to compute the value on reads/is
> updated on writes.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> [ clg: rebased on current ppc tree ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Don't we need something to make the VTB migrate correctly? Or do we
not care because it's only used on pnv which isn't migratable yet?
> ---
> include/hw/ppc/ppc.h | 1 +
> target/ppc/cpu.h | 2 ++
> target/ppc/helper.h | 2 ++
> hw/ppc/ppc.c | 16 ++++++++++++++++
> linux-user/ppc/cpu_loop.c | 5 +++++
> target/ppc/timebase_helper.c | 10 ++++++++++
> target/ppc/translate_init.inc.c | 19 +++++++++++++++----
> 7 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index 585be6ab98c5..02481cd27c36 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -24,6 +24,7 @@ struct ppc_tb_t {
> /* Time base management */
> int64_t tb_offset; /* Compensation */
> int64_t atb_offset; /* Compensation */
> + int64_t vtb_offset;
> uint32_t tb_freq; /* TB frequency */
> /* Decrementer management */
> uint64_t decr_next; /* Tick for next decr interrupt */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e3e82327b723..19d6e724bb5a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1305,6 +1305,8 @@ uint64_t cpu_ppc_load_atbl(CPUPPCState *env);
> uint32_t cpu_ppc_load_atbu(CPUPPCState *env);
> void cpu_ppc_store_atbl(CPUPPCState *env, uint32_t value);
> void cpu_ppc_store_atbu(CPUPPCState *env, uint32_t value);
> +uint64_t cpu_ppc_load_vtb(CPUPPCState *env);
> +void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value);
> bool ppc_decr_clear_on_delivery(CPUPPCState *env);
> target_ulong cpu_ppc_load_decr(CPUPPCState *env);
> void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index f843814b8aa8..a5f53bb421a7 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -649,6 +649,7 @@ DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_tbu, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_atbl, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_atbu, TCG_CALL_NO_RWG, tl, env)
> +DEF_HELPER_FLAGS_1(load_vtb, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_601_rtcl, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
> #if !defined(CONFIG_USER_ONLY)
> @@ -669,6 +670,7 @@ DEF_HELPER_FLAGS_1(load_decr, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_2(store_decr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_FLAGS_1(load_hdecr, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_2(store_hdecr, TCG_CALL_NO_RWG, void, env, tl)
> +DEF_HELPER_FLAGS_2(store_vtb, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_2(store_hid0_601, void, env, tl)
> DEF_HELPER_3(store_403_pbr, void, env, i32, tl)
> DEF_HELPER_FLAGS_1(load_40x_pit, TCG_CALL_NO_RWG, tl, env)
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 8dd982fc1e40..263922052536 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -694,6 +694,22 @@ void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value)
> &tb_env->atb_offset, ((uint64_t)value << 32) | tb);
> }
>
> +uint64_t cpu_ppc_load_vtb(CPUPPCState *env)
> +{
> + ppc_tb_t *tb_env = env->tb_env;
> +
> + return cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + tb_env->vtb_offset);
> +}
> +
> +void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value)
> +{
> + ppc_tb_t *tb_env = env->tb_env;
> +
> + cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + &tb_env->vtb_offset, value);
> +}
> +
> static void cpu_ppc_tb_stop (CPUPPCState *env)
> {
> ppc_tb_t *tb_env = env->tb_env;
> diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
> index d5704def2902..5b27f8603e33 100644
> --- a/linux-user/ppc/cpu_loop.c
> +++ b/linux-user/ppc/cpu_loop.c
> @@ -47,6 +47,11 @@ uint32_t cpu_ppc_load_atbu(CPUPPCState *env)
> return cpu_ppc_get_tb(env) >> 32;
> }
>
> +uint64_t cpu_ppc_load_vtb(CPUPPCState *env)
> +{
> + return cpu_ppc_get_tb(env);
> +}
> +
> uint32_t cpu_ppc601_load_rtcu(CPUPPCState *env)
> __attribute__ (( alias ("cpu_ppc_load_tbu") ));
>
> diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
> index 73363e08ae71..8c3c2fe67c2c 100644
> --- a/target/ppc/timebase_helper.c
> +++ b/target/ppc/timebase_helper.c
> @@ -45,6 +45,11 @@ target_ulong helper_load_atbu(CPUPPCState *env)
> return cpu_ppc_load_atbu(env);
> }
>
> +target_ulong helper_load_vtb(CPUPPCState *env)
> +{
> + return cpu_ppc_load_vtb(env);
> +}
> +
> #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> target_ulong helper_load_purr(CPUPPCState *env)
> {
> @@ -113,6 +118,11 @@ void helper_store_hdecr(CPUPPCState *env, target_ulong val)
> cpu_ppc_store_hdecr(env, val);
> }
>
> +void helper_store_vtb(CPUPPCState *env, target_ulong val)
> +{
> + cpu_ppc_store_vtb(env, val);
> +}
> +
> target_ulong helper_load_40x_pit(CPUPPCState *env)
> {
> return load_40x_pit(env);
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d00..5a560164d4a4 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -312,6 +312,16 @@ static void spr_write_hdecr(DisasContext *ctx, int sprn, int gprn)
> }
> }
>
> +static void spr_read_vtb(DisasContext *ctx, int gprn, int sprn)
> +{
> + gen_helper_load_vtb(cpu_gpr[gprn], cpu_env);
> +}
> +
> +static void spr_write_vtb(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_helper_store_vtb(cpu_env, cpu_gpr[gprn]);
> +}
> +
> #endif
> #endif
>
> @@ -8169,10 +8179,11 @@ static void gen_spr_power8_ebb(CPUPPCState *env)
> /* Virtual Time Base */
> static void gen_spr_vtb(CPUPPCState *env)
> {
> - spr_register_kvm(env, SPR_VTB, "VTB",
> - SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_tbl, SPR_NOACCESS,
> - KVM_REG_PPC_VTB, 0x00000000);
> + spr_register_kvm_hv(env, SPR_VTB, "VTB",
> + SPR_NOACCESS, SPR_NOACCESS,
> + &spr_read_vtb, SPR_NOACCESS,
> + &spr_read_vtb, &spr_write_vtb,
> + KVM_REG_PPC_VTB, 0x00000000);
> }
>
> static void gen_spr_power8_fscr(CPUPPCState *env)
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/7] target/ppc: Work [S]PURR implementation and add HV support
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
2019-11-28 13:46 ` [PATCH 1/7] target/ppc: Implement the VTB for HV access Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2019-11-29 1:53 ` David Gibson
2019-11-28 13:46 ` [PATCH 3/7] target/ppc: Add SPR ASDR Cédric Le Goater
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
The Processor Utilisation of Resources Register (PURR) and Scaled
Processor Utilisation of Resources Register (SPURR) provide an estimate
of the resources used by the thread, present on POWER7 and later
processors.
Currently the [S]PURR registers simply count at the rate of the
timebase.
Preserve this behaviour but rework the implementation to store an offset
like the timebase rather than doing the calculation manually. Also allow
hypervisor write access to the register along with the currently
available read access.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
[ clg: rebased on current ppc tree ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
David,
In the initial discussion, PURR and VTB still needed to be added to
the migration stream. The patch is changing the representation indeed
but that seems OK. AFAICT, all the SPRs are migrated. I didn't quite
understand that part. See http://patchwork.ozlabs.org/patch/1094662
for more info.
Nevertheless, you added your Reviewed-by.
include/hw/ppc/ppc.h | 3 +--
target/ppc/cpu.h | 1 +
target/ppc/helper.h | 1 +
hw/ppc/ppc.c | 17 +++++++----------
target/ppc/timebase_helper.c | 5 +++++
target/ppc/translate_init.inc.c | 23 +++++++++++++++--------
6 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 02481cd27c36..27bef85ca869 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -33,8 +33,7 @@ struct ppc_tb_t {
/* Hypervisor decrementer management */
uint64_t hdecr_next; /* Tick for next hdecr interrupt */
QEMUTimer *hdecr_timer;
- uint64_t purr_load;
- uint64_t purr_start;
+ int64_t purr_offset;
void *opaque;
uint32_t flags;
};
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 19d6e724bb5a..9128dbefbdb0 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1313,6 +1313,7 @@ void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
target_ulong cpu_ppc_load_hdecr(CPUPPCState *env);
void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong value);
uint64_t cpu_ppc_load_purr(CPUPPCState *env);
+void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value);
uint32_t cpu_ppc601_load_rtcl(CPUPPCState *env);
uint32_t cpu_ppc601_load_rtcu(CPUPPCState *env);
#if !defined(CONFIG_USER_ONLY)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index a5f53bb421a7..356a14d8a639 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -655,6 +655,7 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
#if !defined(CONFIG_USER_ONLY)
#if defined(TARGET_PPC64)
DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
+DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_2(store_ptcr, void, env, tl)
#endif
DEF_HELPER_2(store_sdr1, void, env, tl)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 263922052536..353f11b91d40 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -821,12 +821,9 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
uint64_t cpu_ppc_load_purr (CPUPPCState *env)
{
ppc_tb_t *tb_env = env->tb_env;
- uint64_t diff;
- diff = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - tb_env->purr_start;
-
- return tb_env->purr_load +
- muldiv64(diff, tb_env->tb_freq, NANOSECONDS_PER_SECOND);
+ return cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ tb_env->purr_offset);
}
/* When decrementer expires,
@@ -985,12 +982,12 @@ static void cpu_ppc_hdecr_cb(void *opaque)
cpu_ppc_hdecr_excp(cpu);
}
-static void cpu_ppc_store_purr(PowerPCCPU *cpu, uint64_t value)
+void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
{
- ppc_tb_t *tb_env = cpu->env.tb_env;
+ ppc_tb_t *tb_env = env->tb_env;
- tb_env->purr_load = value;
- tb_env->purr_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ &tb_env->purr_offset, value);
}
static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
@@ -1007,7 +1004,7 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
*/
_cpu_ppc_store_decr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 32);
_cpu_ppc_store_hdecr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 32);
- cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
+ cpu_ppc_store_purr(env, 0x0000000000000000ULL);
}
static void timebase_save(PPCTimebase *tb)
diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
index 8c3c2fe67c2c..2395295b778c 100644
--- a/target/ppc/timebase_helper.c
+++ b/target/ppc/timebase_helper.c
@@ -55,6 +55,11 @@ target_ulong helper_load_purr(CPUPPCState *env)
{
return (target_ulong)cpu_ppc_load_purr(env);
}
+
+void helper_store_purr(CPUPPCState *env, target_ulong val)
+{
+ cpu_ppc_store_purr(env, val);
+}
#endif
target_ulong helper_load_601_rtcl(CPUPPCState *env)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 5a560164d4a4..6105e74e9dc6 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -287,6 +287,11 @@ static void spr_read_purr(DisasContext *ctx, int gprn, int sprn)
gen_helper_load_purr(cpu_gpr[gprn], cpu_env);
}
+static void spr_write_purr(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_helper_store_purr(cpu_env, cpu_gpr[gprn]);
+}
+
/* HDECR */
static void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn)
{
@@ -8008,14 +8013,16 @@ static void gen_spr_book3s_purr(CPUPPCState *env)
{
#if !defined(CONFIG_USER_ONLY)
/* PURR & SPURR: Hack - treat these as aliases for the TB for now */
- spr_register_kvm(env, SPR_PURR, "PURR",
- &spr_read_purr, SPR_NOACCESS,
- &spr_read_purr, SPR_NOACCESS,
- KVM_REG_PPC_PURR, 0x00000000);
- spr_register_kvm(env, SPR_SPURR, "SPURR",
- &spr_read_purr, SPR_NOACCESS,
- &spr_read_purr, SPR_NOACCESS,
- KVM_REG_PPC_SPURR, 0x00000000);
+ spr_register_kvm_hv(env, SPR_PURR, "PURR",
+ &spr_read_purr, SPR_NOACCESS,
+ &spr_read_purr, SPR_NOACCESS,
+ &spr_read_purr, &spr_write_purr,
+ KVM_REG_PPC_PURR, 0x00000000);
+ spr_register_kvm_hv(env, SPR_SPURR, "SPURR",
+ &spr_read_purr, SPR_NOACCESS,
+ &spr_read_purr, SPR_NOACCESS,
+ &spr_read_purr, &spr_write_purr,
+ KVM_REG_PPC_SPURR, 0x00000000);
#endif
}
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/7] target/ppc: Work [S]PURR implementation and add HV support
2019-11-28 13:46 ` [PATCH 2/7] target/ppc: Work [S]PURR implementation and add HV support Cédric Le Goater
@ 2019-11-29 1:53 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2019-11-29 1:53 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9424 bytes --]
On Thu, Nov 28, 2019 at 02:46:55PM +0100, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> The Processor Utilisation of Resources Register (PURR) and Scaled
> Processor Utilisation of Resources Register (SPURR) provide an estimate
> of the resources used by the thread, present on POWER7 and later
> processors.
>
> Currently the [S]PURR registers simply count at the rate of the
> timebase.
>
> Preserve this behaviour but rework the implementation to store an offset
> like the timebase rather than doing the calculation manually. Also allow
> hypervisor write access to the register along with the currently
> available read access.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> [ clg: rebased on current ppc tree ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
> David,
>
> In the initial discussion, PURR and VTB still needed to be added to
> the migration stream. The patch is changing the representation indeed
> but that seems OK. AFAICT, all the SPRs are migrated. I didn't quite
> understand that part. See http://patchwork.ozlabs.org/patch/1094662
> for more info.
Ah, right, forgot that discussion in my comment on 1/1.
So, "all SPRs are migrated" is kind of conditional. All SPRs stored
statically in env->sprs[] are migrated. But here we have what's
essentially a virtual register whose value is calculated at read time.
We don't actually update sprs[SPR_TB] and similar registers at 500MHz,
that would be impossibly expensive. So instead we need to migrate the
offset data - in some encoding or other - so that the apparent
register value on reads after the migrate will make sense.
I think we can do this without actually adding extra data to the
stream, by using the sprs[SPR_VTB,SPR_PURR] fields. But to do so will
need a little extra logic.
I think what Suraj was suggesting in that patchwork link was to read
the PURR value (via the helpers here) at pre_save() and write it to
sprs[SPR_PURR], then at post_load() take the value in sprs[SPR_PURR]
and write it back to the virtual PURR via the helpers here. That will
effectively freeze the PURR during the migration downtime, but
otherwise preserve it, and for PURR and SPURR I think that makes
sense.
VTB is a little different. Like the TB itself, it's essentially
tracking wall clock time, and so it should continue to count
(conceptually speaking) during the migration downtime.
I think the key here is that we want to maintain the offset between TB
and VTB across migration. It will require different logic, but
there's a good chance we can still save the data we need in
sprs[SPR_VTB] rather than having to add extra things to the stream.
That said, the writable versions of PURR, SPURR and VTB are only
relevant for pnv, and I don't think we currently support migration of
pnv machines anyway. So we could punt on this until later. But if we
do that, I would like to see some TODO comments in strategic places.
>
> Nevertheless, you added your Reviewed-by.
>
> include/hw/ppc/ppc.h | 3 +--
> target/ppc/cpu.h | 1 +
> target/ppc/helper.h | 1 +
> hw/ppc/ppc.c | 17 +++++++----------
> target/ppc/timebase_helper.c | 5 +++++
> target/ppc/translate_init.inc.c | 23 +++++++++++++++--------
> 6 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index 02481cd27c36..27bef85ca869 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -33,8 +33,7 @@ struct ppc_tb_t {
> /* Hypervisor decrementer management */
> uint64_t hdecr_next; /* Tick for next hdecr interrupt */
> QEMUTimer *hdecr_timer;
> - uint64_t purr_load;
> - uint64_t purr_start;
> + int64_t purr_offset;
> void *opaque;
> uint32_t flags;
> };
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 19d6e724bb5a..9128dbefbdb0 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1313,6 +1313,7 @@ void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
> target_ulong cpu_ppc_load_hdecr(CPUPPCState *env);
> void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong value);
> uint64_t cpu_ppc_load_purr(CPUPPCState *env);
> +void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value);
> uint32_t cpu_ppc601_load_rtcl(CPUPPCState *env);
> uint32_t cpu_ppc601_load_rtcu(CPUPPCState *env);
> #if !defined(CONFIG_USER_ONLY)
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index a5f53bb421a7..356a14d8a639 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -655,6 +655,7 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
> #if !defined(CONFIG_USER_ONLY)
> #if defined(TARGET_PPC64)
> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> +DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_2(store_ptcr, void, env, tl)
> #endif
> DEF_HELPER_2(store_sdr1, void, env, tl)
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 263922052536..353f11b91d40 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -821,12 +821,9 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
> uint64_t cpu_ppc_load_purr (CPUPPCState *env)
> {
> ppc_tb_t *tb_env = env->tb_env;
> - uint64_t diff;
>
> - diff = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - tb_env->purr_start;
> -
> - return tb_env->purr_load +
> - muldiv64(diff, tb_env->tb_freq, NANOSECONDS_PER_SECOND);
> + return cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + tb_env->purr_offset);
> }
>
> /* When decrementer expires,
> @@ -985,12 +982,12 @@ static void cpu_ppc_hdecr_cb(void *opaque)
> cpu_ppc_hdecr_excp(cpu);
> }
>
> -static void cpu_ppc_store_purr(PowerPCCPU *cpu, uint64_t value)
> +void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
> {
> - ppc_tb_t *tb_env = cpu->env.tb_env;
> + ppc_tb_t *tb_env = env->tb_env;
>
> - tb_env->purr_load = value;
> - tb_env->purr_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + &tb_env->purr_offset, value);
> }
>
> static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
> @@ -1007,7 +1004,7 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
> */
> _cpu_ppc_store_decr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 32);
> _cpu_ppc_store_hdecr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 32);
> - cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
> + cpu_ppc_store_purr(env, 0x0000000000000000ULL);
> }
>
> static void timebase_save(PPCTimebase *tb)
> diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
> index 8c3c2fe67c2c..2395295b778c 100644
> --- a/target/ppc/timebase_helper.c
> +++ b/target/ppc/timebase_helper.c
> @@ -55,6 +55,11 @@ target_ulong helper_load_purr(CPUPPCState *env)
> {
> return (target_ulong)cpu_ppc_load_purr(env);
> }
> +
> +void helper_store_purr(CPUPPCState *env, target_ulong val)
> +{
> + cpu_ppc_store_purr(env, val);
> +}
> #endif
>
> target_ulong helper_load_601_rtcl(CPUPPCState *env)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 5a560164d4a4..6105e74e9dc6 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -287,6 +287,11 @@ static void spr_read_purr(DisasContext *ctx, int gprn, int sprn)
> gen_helper_load_purr(cpu_gpr[gprn], cpu_env);
> }
>
> +static void spr_write_purr(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_helper_store_purr(cpu_env, cpu_gpr[gprn]);
> +}
> +
> /* HDECR */
> static void spr_read_hdecr(DisasContext *ctx, int gprn, int sprn)
> {
> @@ -8008,14 +8013,16 @@ static void gen_spr_book3s_purr(CPUPPCState *env)
> {
> #if !defined(CONFIG_USER_ONLY)
> /* PURR & SPURR: Hack - treat these as aliases for the TB for now */
> - spr_register_kvm(env, SPR_PURR, "PURR",
> - &spr_read_purr, SPR_NOACCESS,
> - &spr_read_purr, SPR_NOACCESS,
> - KVM_REG_PPC_PURR, 0x00000000);
> - spr_register_kvm(env, SPR_SPURR, "SPURR",
> - &spr_read_purr, SPR_NOACCESS,
> - &spr_read_purr, SPR_NOACCESS,
> - KVM_REG_PPC_SPURR, 0x00000000);
> + spr_register_kvm_hv(env, SPR_PURR, "PURR",
> + &spr_read_purr, SPR_NOACCESS,
> + &spr_read_purr, SPR_NOACCESS,
> + &spr_read_purr, &spr_write_purr,
> + KVM_REG_PPC_PURR, 0x00000000);
> + spr_register_kvm_hv(env, SPR_SPURR, "SPURR",
> + &spr_read_purr, SPR_NOACCESS,
> + &spr_read_purr, SPR_NOACCESS,
> + &spr_read_purr, &spr_write_purr,
> + KVM_REG_PPC_SPURR, 0x00000000);
> #endif
> }
>
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/7] target/ppc: Add SPR ASDR
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
2019-11-28 13:46 ` [PATCH 1/7] target/ppc: Implement the VTB for HV access Cédric Le Goater
2019-11-28 13:46 ` [PATCH 2/7] target/ppc: Work [S]PURR implementation and add HV support Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2019-11-28 13:46 ` [PATCH 4/7] target/ppc: Add SPR TBU40 Cédric Le Goater
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
The Access Segment Descriptor Register (ASDR) provides information about
the storage element when taking a hypervisor storage interrupt. When
performing nested radix address translation, this is normally the guest
real address. This register is present on POWER9 processors and later.
Implement the ADSR, note read and write access is limited to the
hypervisor.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/cpu.h | 1 +
target/ppc/translate_init.inc.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 9128dbefbdb0..646a94370dba 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1780,6 +1780,7 @@ typedef PowerPCCPU ArchCPU;
#define SPR_MPC_MD_DBRAM1 (0x32A)
#define SPR_RCPU_L2U_RA3 (0x32B)
#define SPR_TAR (0x32F)
+#define SPR_ASDR (0x330)
#define SPR_IC (0x350)
#define SPR_VTB (0x351)
#define SPR_MMCRC (0x353)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 6105e74e9dc6..a3cf1d8a450c 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -8290,6 +8290,12 @@ static void gen_spr_power9_mmu(CPUPPCState *env)
SPR_NOACCESS, SPR_NOACCESS,
&spr_read_generic, &spr_write_ptcr,
KVM_REG_PPC_PTCR, 0x00000000);
+ /* Address Segment Descriptor Register */
+ spr_register_hv(env, SPR_ASDR, "ASDR",
+ SPR_NOACCESS, SPR_NOACCESS,
+ SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, &spr_write_generic,
+ 0x0000000000000000);
#endif
}
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 4/7] target/ppc: Add SPR TBU40
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
` (2 preceding siblings ...)
2019-11-28 13:46 ` [PATCH 3/7] target/ppc: Add SPR ASDR Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2020-01-14 0:30 ` Philippe Mathieu-Daudé
2019-11-28 13:46 ` [PATCH 5/7] target/ppc: Add privileged message send facilities Cédric Le Goater
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
The spr TBU40 is used to set the upper 40 bits of the timebase
register, present on POWER5+ and later processors.
This register can only be written by the hypervisor, and cannot be read.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/cpu.h | 1 +
target/ppc/helper.h | 1 +
hw/ppc/ppc.c | 13 +++++++++++++
target/ppc/timebase_helper.c | 5 +++++
target/ppc/translate_init.inc.c | 19 +++++++++++++++++++
5 files changed, 39 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 646a94370dba..8ffcfa0ea162 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1312,6 +1312,7 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env);
void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
target_ulong cpu_ppc_load_hdecr(CPUPPCState *env);
void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong value);
+void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value);
uint64_t cpu_ppc_load_purr(CPUPPCState *env);
void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value);
uint32_t cpu_ppc601_load_rtcl(CPUPPCState *env);
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 356a14d8a639..cd0dfe383a2a 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -672,6 +672,7 @@ DEF_HELPER_FLAGS_2(store_decr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_1(load_hdecr, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_2(store_hdecr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_2(store_vtb, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_FLAGS_2(store_tbu40, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_2(store_hid0_601, void, env, tl)
DEF_HELPER_3(store_403_pbr, void, env, i32, tl)
DEF_HELPER_FLAGS_1(load_40x_pit, TCG_CALL_NO_RWG, tl, env)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 353f11b91d40..3666e58865b3 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -710,6 +710,19 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value)
&tb_env->vtb_offset, value);
}
+void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value)
+{
+ ppc_tb_t *tb_env = env->tb_env;
+ uint64_t tb;
+
+ tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ tb_env->tb_offset);
+ tb &= 0xFFFFFFUL;
+ tb |= (value & ~0xFFFFFFUL);
+ cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+ &tb_env->tb_offset, tb);
+}
+
static void cpu_ppc_tb_stop (CPUPPCState *env)
{
ppc_tb_t *tb_env = env->tb_env;
diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
index 2395295b778c..703bd9ed18b9 100644
--- a/target/ppc/timebase_helper.c
+++ b/target/ppc/timebase_helper.c
@@ -128,6 +128,11 @@ void helper_store_vtb(CPUPPCState *env, target_ulong val)
cpu_ppc_store_vtb(env, val);
}
+void helper_store_tbu40(CPUPPCState *env, target_ulong val)
+{
+ cpu_ppc_store_tbu40(env, val);
+}
+
target_ulong helper_load_40x_pit(CPUPPCState *env)
{
return load_40x_pit(env);
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index a3cf1d8a450c..9815df6f77c8 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -327,6 +327,11 @@ static void spr_write_vtb(DisasContext *ctx, int sprn, int gprn)
gen_helper_store_vtb(cpu_env, cpu_gpr[gprn]);
}
+static void spr_write_tbu40(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_helper_store_tbu40(cpu_env, cpu_gpr[gprn]);
+}
+
#endif
#endif
@@ -7848,6 +7853,16 @@ static void gen_spr_power5p_ear(CPUPPCState *env)
0x00000000);
}
+static void gen_spr_power5p_tb(CPUPPCState *env)
+{
+ /* TBU40 (High 40 bits of the Timebase register */
+ spr_register_hv(env, SPR_TBU40, "TBU40",
+ SPR_NOACCESS, SPR_NOACCESS,
+ SPR_NOACCESS, SPR_NOACCESS,
+ SPR_NOACCESS, &spr_write_tbu40,
+ 0x00000000);
+}
+
#if !defined(CONFIG_USER_ONLY)
static void spr_write_hmer(DisasContext *ctx, int sprn, int gprn)
{
@@ -8399,6 +8414,7 @@ static void init_proc_power5plus(CPUPPCState *env)
gen_spr_power5p_common(env);
gen_spr_power5p_lpar(env);
gen_spr_power5p_ear(env);
+ gen_spr_power5p_tb(env);
/* env variables */
env->dcache_line_size = 128;
@@ -8511,6 +8527,7 @@ static void init_proc_POWER7(CPUPPCState *env)
gen_spr_power5p_common(env);
gen_spr_power5p_lpar(env);
gen_spr_power5p_ear(env);
+ gen_spr_power5p_tb(env);
gen_spr_power6_common(env);
gen_spr_power6_dbg(env);
gen_spr_power7_book4(env);
@@ -8652,6 +8669,7 @@ static void init_proc_POWER8(CPUPPCState *env)
gen_spr_power5p_common(env);
gen_spr_power5p_lpar(env);
gen_spr_power5p_ear(env);
+ gen_spr_power5p_tb(env);
gen_spr_power6_common(env);
gen_spr_power6_dbg(env);
gen_spr_power8_tce_address_control(env);
@@ -8842,6 +8860,7 @@ static void init_proc_POWER9(CPUPPCState *env)
gen_spr_power5p_common(env);
gen_spr_power5p_lpar(env);
gen_spr_power5p_ear(env);
+ gen_spr_power5p_tb(env);
gen_spr_power6_common(env);
gen_spr_power6_dbg(env);
gen_spr_power8_tce_address_control(env);
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/7] target/ppc: Add SPR TBU40
2019-11-28 13:46 ` [PATCH 4/7] target/ppc: Add SPR TBU40 Cédric Le Goater
@ 2020-01-14 0:30 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-14 0:30 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
On 11/28/19 2:46 PM, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> The spr TBU40 is used to set the upper 40 bits of the timebase
> register, present on POWER5+ and later processors.
>
> This register can only be written by the hypervisor, and cannot be read.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> target/ppc/cpu.h | 1 +
> target/ppc/helper.h | 1 +
> hw/ppc/ppc.c | 13 +++++++++++++
> target/ppc/timebase_helper.c | 5 +++++
> target/ppc/translate_init.inc.c | 19 +++++++++++++++++++
> 5 files changed, 39 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 646a94370dba..8ffcfa0ea162 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1312,6 +1312,7 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env);
> void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value);
> target_ulong cpu_ppc_load_hdecr(CPUPPCState *env);
> void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong value);
> +void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value);
> uint64_t cpu_ppc_load_purr(CPUPPCState *env);
> void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value);
> uint32_t cpu_ppc601_load_rtcl(CPUPPCState *env);
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 356a14d8a639..cd0dfe383a2a 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -672,6 +672,7 @@ DEF_HELPER_FLAGS_2(store_decr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_FLAGS_1(load_hdecr, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_2(store_hdecr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_FLAGS_2(store_vtb, TCG_CALL_NO_RWG, void, env, tl)
> +DEF_HELPER_FLAGS_2(store_tbu40, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_2(store_hid0_601, void, env, tl)
> DEF_HELPER_3(store_403_pbr, void, env, i32, tl)
> DEF_HELPER_FLAGS_1(load_40x_pit, TCG_CALL_NO_RWG, tl, env)
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 353f11b91d40..3666e58865b3 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -710,6 +710,19 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value)
> &tb_env->vtb_offset, value);
> }
>
> +void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value)
> +{
> + ppc_tb_t *tb_env = env->tb_env;
> + uint64_t tb;
> +
> + tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + tb_env->tb_offset);
> + tb &= 0xFFFFFFUL;
> + tb |= (value & ~0xFFFFFFUL);
IMHO easier to review as:
tb = deposit64(value, 0, 40, tb);
> + cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> + &tb_env->tb_offset, tb);
> +}
> +
> static void cpu_ppc_tb_stop (CPUPPCState *env)
> {
> ppc_tb_t *tb_env = env->tb_env;
> diff --git a/target/ppc/timebase_helper.c b/target/ppc/timebase_helper.c
> index 2395295b778c..703bd9ed18b9 100644
> --- a/target/ppc/timebase_helper.c
> +++ b/target/ppc/timebase_helper.c
> @@ -128,6 +128,11 @@ void helper_store_vtb(CPUPPCState *env, target_ulong val)
> cpu_ppc_store_vtb(env, val);
> }
>
> +void helper_store_tbu40(CPUPPCState *env, target_ulong val)
> +{
> + cpu_ppc_store_tbu40(env, val);
> +}
> +
> target_ulong helper_load_40x_pit(CPUPPCState *env)
> {
> return load_40x_pit(env);
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index a3cf1d8a450c..9815df6f77c8 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -327,6 +327,11 @@ static void spr_write_vtb(DisasContext *ctx, int sprn, int gprn)
> gen_helper_store_vtb(cpu_env, cpu_gpr[gprn]);
> }
>
> +static void spr_write_tbu40(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_helper_store_tbu40(cpu_env, cpu_gpr[gprn]);
> +}
> +
> #endif
> #endif
>
> @@ -7848,6 +7853,16 @@ static void gen_spr_power5p_ear(CPUPPCState *env)
> 0x00000000);
> }
>
> +static void gen_spr_power5p_tb(CPUPPCState *env)
> +{
> + /* TBU40 (High 40 bits of the Timebase register */
> + spr_register_hv(env, SPR_TBU40, "TBU40",
> + SPR_NOACCESS, SPR_NOACCESS,
> + SPR_NOACCESS, SPR_NOACCESS,
> + SPR_NOACCESS, &spr_write_tbu40,
> + 0x00000000);
> +}
> +
> #if !defined(CONFIG_USER_ONLY)
> static void spr_write_hmer(DisasContext *ctx, int sprn, int gprn)
> {
> @@ -8399,6 +8414,7 @@ static void init_proc_power5plus(CPUPPCState *env)
> gen_spr_power5p_common(env);
> gen_spr_power5p_lpar(env);
> gen_spr_power5p_ear(env);
> + gen_spr_power5p_tb(env);
>
> /* env variables */
> env->dcache_line_size = 128;
> @@ -8511,6 +8527,7 @@ static void init_proc_POWER7(CPUPPCState *env)
> gen_spr_power5p_common(env);
> gen_spr_power5p_lpar(env);
> gen_spr_power5p_ear(env);
> + gen_spr_power5p_tb(env);
> gen_spr_power6_common(env);
> gen_spr_power6_dbg(env);
> gen_spr_power7_book4(env);
> @@ -8652,6 +8669,7 @@ static void init_proc_POWER8(CPUPPCState *env)
> gen_spr_power5p_common(env);
> gen_spr_power5p_lpar(env);
> gen_spr_power5p_ear(env);
> + gen_spr_power5p_tb(env);
> gen_spr_power6_common(env);
> gen_spr_power6_dbg(env);
> gen_spr_power8_tce_address_control(env);
> @@ -8842,6 +8860,7 @@ static void init_proc_POWER9(CPUPPCState *env)
> gen_spr_power5p_common(env);
> gen_spr_power5p_lpar(env);
> gen_spr_power5p_ear(env);
> + gen_spr_power5p_tb(env);
> gen_spr_power6_common(env);
> gen_spr_power6_dbg(env);
> gen_spr_power8_tce_address_control(env);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/7] target/ppc: Add privileged message send facilities
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
` (3 preceding siblings ...)
2019-11-28 13:46 ` [PATCH 4/7] target/ppc: Add SPR TBU40 Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2019-12-17 4:00 ` David Gibson
2019-11-28 13:46 ` [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception Cédric Le Goater
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Privileged message send facilities exist on POWER8 processors and
later and include a register and instructions which can be used to
generate, observe/modify the state of and clear privileged doorbell
exceptions as described below.
The Directed Privileged Doorbell Exception State (DPDES) register
reflects the state of pending privileged doorbell exceptions and can
also be used to modify that state. The register can be used to read
and modify the state of privileged doorbell exceptions for all threads
of a subprocessor and thus is a shared facility for that subprocessor.
The register can be read/written by the hypervisor and read by the
supervisor if enabled in the HFSCR, otherwise a hypervisor facility
unavailable exception is generated.
The privileged message send and clear instructions (msgsndp & msgclrp)
are used to generate and clear the presence of a directed privileged
doorbell exception, respectively. The msgsndp instruction can be used
to target any thread of the current subprocessor, msgclrp acts on the
thread issuing the instruction. These instructions are privileged, but
will generate a hypervisor facility unavailable exception if not
enabled in the HFSCR and executed in privileged non-hypervisor
state. The HV facility unavailable exception will be addressed in
other patch.
Add and implement this register and instructions by reading or
modifying the pending interrupt state of the cpu.
Note that TCG only supports one thread per core and so we only need to
worry about the cpu making the access.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
[clg: - introduced a book3s_msgsnd_common() helper to reduce code
duplication
- modified book3s_dbell2irq()
- moved out the support for hypervisor facility unavailable
exception
- modified commit log to add a statement on the HV FU exception
- checkpatch fixes
- compile fixes for the ppc-softmmu and linux-user targets ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/helper.h | 4 +++
target/ppc/excp_helper.c | 43 ++++++++++++++++++++++++++++-----
target/ppc/misc_helper.c | 22 +++++++++++++++++
target/ppc/translate.c | 26 ++++++++++++++++++++
target/ppc/translate_init.inc.c | 20 ++++++++++++---
5 files changed, 105 insertions(+), 10 deletions(-)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index cd0dfe383a2a..76518a1df6f0 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_2(store_ptcr, void, env, tl)
+DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
+DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_1(book3s_msgsndp, void, tl)
+DEF_HELPER_2(book3s_msgclrp, void, env, tl)
#endif
DEF_HELPER_2(store_sdr1, void, env, tl)
DEF_HELPER_2(store_pidr, void, env, tl)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 50b004d00d1e..5a247945e97f 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
}
if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
- powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
+ if (env->insns_flags & PPC_SEGMENT_64B) {
+ powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
+ } else {
+ powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
+ }
return;
}
if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
@@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
}
/* Server Processor Control */
-static int book3s_dbell2irq(target_ulong rb)
+static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
{
int msg = rb & DBELL_TYPE_MASK;
@@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
* message type is 5. All other types are reserved and the
* instruction is a no-op
*/
- return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
+ if (msg == DBELL_TYPE_DBELL_SERVER) {
+ return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL;
+ }
+
+ return -1;
}
void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
{
- int irq = book3s_dbell2irq(rb);
+ int irq = book3s_dbell2irq(rb, 1);
if (irq < 0) {
return;
@@ -1242,9 +1250,9 @@ void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
env->pending_interrupts &= ~(1 << irq);
}
-void helper_book3s_msgsnd(target_ulong rb)
+static void book3s_msgsnd_common(target_ulong rb, bool hv_dbell)
{
- int irq = book3s_dbell2irq(rb);
+ int irq = book3s_dbell2irq(rb, hv_dbell);
int pir = rb & DBELL_PROCIDTAG_MASK;
CPUState *cs;
@@ -1265,6 +1273,29 @@ void helper_book3s_msgsnd(target_ulong rb)
}
qemu_mutex_unlock_iothread();
}
+
+void helper_book3s_msgsnd(target_ulong rb)
+{
+ book3s_msgsnd_common(rb, 1);
+}
+
+#if defined(TARGET_PPC64)
+void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
+{
+ int irq = book3s_dbell2irq(rb, 0);
+
+ if (irq < 0) {
+ return;
+ }
+
+ env->pending_interrupts &= ~(1 << irq);
+}
+
+void helper_book3s_msgsndp(target_ulong rb)
+{
+ book3s_msgsnd_common(rb, 0);
+}
+#endif
#endif
void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 2318f3ab45b2..a0e7bd9c32d3 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -105,6 +105,28 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
env->spr[SPR_PCR] = value & pcc->pcr_mask;
}
+
+target_ulong helper_load_dpdes(CPUPPCState *env)
+{
+ if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
+ return 1;
+ }
+ return 0;
+}
+
+void helper_store_dpdes(CPUPPCState *env, target_ulong val)
+{
+ PowerPCCPU *cpu = env_archcpu(env);
+ CPUState *cs = CPU(cpu);
+
+ if (val) {
+ /* Only one cpu for now */
+ env->pending_interrupts |= 1 << PPC_INTERRUPT_DOORBELL;
+ cpu_interrupt(cs, CPU_INTERRUPT_HARD);
+ } else {
+ env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
+ }
+}
#endif /* defined(TARGET_PPC64) */
void helper_store_pidr(CPUPPCState *env, target_ulong val)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index f5fe5d06118a..ba759ab2bb0f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6645,6 +6645,28 @@ static void gen_msgsnd(DisasContext *ctx)
#endif /* defined(CONFIG_USER_ONLY) */
}
+#if defined(TARGET_PPC64)
+static void gen_msgclrp(DisasContext *ctx)
+{
+#if defined(CONFIG_USER_ONLY)
+ GEN_PRIV;
+#else
+ CHK_SV;
+ gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
+#endif /* defined(CONFIG_USER_ONLY) */
+}
+
+static void gen_msgsndp(DisasContext *ctx)
+{
+#if defined(CONFIG_USER_ONLY)
+ GEN_PRIV;
+#else
+ CHK_SV;
+ gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
+#endif /* defined(CONFIG_USER_ONLY) */
+}
+#endif
+
static void gen_msgsync(DisasContext *ctx)
{
#if defined(CONFIG_USER_ONLY)
@@ -7187,6 +7209,10 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
PPC2_ISA300),
GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
+GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001,
+ PPC_NONE, PPC2_ISA207S),
+GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001,
+ PPC_NONE, PPC2_ISA207S),
#endif
#undef GEN_INT_ARITH_ADD
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 9815df6f77c8..7c74a763ba66 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -464,6 +464,17 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
{
gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
}
+
+/* DPDES */
+static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
+{
+ gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
+}
+
+static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
+{
+ gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
+}
#endif
#endif
@@ -8233,10 +8244,11 @@ static void gen_spr_power8_dpdes(CPUPPCState *env)
{
#if !defined(CONFIG_USER_ONLY)
/* Directed Privileged Door-bell Exception State, used for IPI */
- spr_register(env, SPR_DPDES, "DPDES",
- SPR_NOACCESS, SPR_NOACCESS,
- &spr_read_generic, SPR_NOACCESS,
- 0x00000000);
+ spr_register_kvm_hv(env, SPR_DPDES, "DPDES",
+ SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_dpdes, SPR_NOACCESS,
+ &spr_read_dpdes, &spr_write_dpdes,
+ KVM_REG_PPC_DPDES, 0x00000000);
#endif
}
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
2019-11-28 13:46 ` [PATCH 5/7] target/ppc: Add privileged message send facilities Cédric Le Goater
@ 2019-12-17 4:00 ` David Gibson
2020-01-08 15:32 ` Cédric Le Goater
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2019-12-17 4:00 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 11030 bytes --]
On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> Privileged message send facilities exist on POWER8 processors and
> later and include a register and instructions which can be used to
> generate, observe/modify the state of and clear privileged doorbell
> exceptions as described below.
So, it's probably worth noting somewhere here that the "privileged
doorbell" instructions, being supervisor privileged than the existing
doorbell instructions which are hypervisor privileged.
> The Directed Privileged Doorbell Exception State (DPDES) register
> reflects the state of pending privileged doorbell exceptions and can
> also be used to modify that state. The register can be used to read
> and modify the state of privileged doorbell exceptions for all threads
> of a subprocessor and thus is a shared facility for that subprocessor.
> The register can be read/written by the hypervisor and read by the
> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
> unavailable exception is generated.
>
> The privileged message send and clear instructions (msgsndp & msgclrp)
> are used to generate and clear the presence of a directed privileged
> doorbell exception, respectively. The msgsndp instruction can be used
> to target any thread of the current subprocessor, msgclrp acts on the
> thread issuing the instruction. These instructions are privileged, but
> will generate a hypervisor facility unavailable exception if not
> enabled in the HFSCR and executed in privileged non-hypervisor
> state. The HV facility unavailable exception will be addressed in
> other patch.
>
> Add and implement this register and instructions by reading or
> modifying the pending interrupt state of the cpu.
>
> Note that TCG only supports one thread per core and so we only need to
> worry about the cpu making the access.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
> duplication
> - modified book3s_dbell2irq()
> - moved out the support for hypervisor facility unavailable
> exception
> - modified commit log to add a statement on the HV FU exception
> - checkpatch fixes
> - compile fixes for the ppc-softmmu and linux-user targets ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Double signoff?
> ---
> target/ppc/helper.h | 4 +++
> target/ppc/excp_helper.c | 43 ++++++++++++++++++++++++++++-----
> target/ppc/misc_helper.c | 22 +++++++++++++++++
> target/ppc/translate.c | 26 ++++++++++++++++++++
> target/ppc/translate_init.inc.c | 20 ++++++++++++---
> 5 files changed, 105 insertions(+), 10 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index cd0dfe383a2a..76518a1df6f0 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
> DEF_HELPER_2(store_ptcr, void, env, tl)
> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
> +DEF_HELPER_1(book3s_msgsndp, void, tl)
> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
> #endif
> DEF_HELPER_2(store_sdr1, void, env, tl)
> DEF_HELPER_2(store_pidr, void, env, tl)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 50b004d00d1e..5a247945e97f 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> }
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> - powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> + if (env->insns_flags & PPC_SEGMENT_64B) {
> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
> + } else {
> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> + }
> return;
> }
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
> }
>
> /* Server Processor Control */
> -static int book3s_dbell2irq(target_ulong rb)
> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
> {
> int msg = rb & DBELL_TYPE_MASK;
>
> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
> * message type is 5. All other types are reserved and the
> * instruction is a no-op
> */
> - return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
> + if (msg == DBELL_TYPE_DBELL_SERVER) {
> + return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL;
> + }
> +
> + return -1;
> }
>
> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> {
> - int irq = book3s_dbell2irq(rb);
> + int irq = book3s_dbell2irq(rb, 1);
true/false are preferred to 0/1 for bool types.
>
> if (irq < 0) {
> return;
> @@ -1242,9 +1250,9 @@ void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> env->pending_interrupts &= ~(1 << irq);
> }
>
> -void helper_book3s_msgsnd(target_ulong rb)
> +static void book3s_msgsnd_common(target_ulong rb, bool hv_dbell)
> {
> - int irq = book3s_dbell2irq(rb);
> + int irq = book3s_dbell2irq(rb, hv_dbell);
> int pir = rb & DBELL_PROCIDTAG_MASK;
> CPUState *cs;
>
> @@ -1265,6 +1273,29 @@ void helper_book3s_msgsnd(target_ulong rb)
> }
> qemu_mutex_unlock_iothread();
> }
> +
> +void helper_book3s_msgsnd(target_ulong rb)
> +{
As noted you only need to worry about a single thread here. But
shouldn't you validate or mask the target against that? I don't know
what the hardware behaviour is if you give a bad target here: does it
ignore it, or trap?
> + book3s_msgsnd_common(rb, 1);
> +}
> +
> +#if defined(TARGET_PPC64)
> +void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
> +{
> + int irq = book3s_dbell2irq(rb, 0);
> +
> + if (irq < 0) {
> + return;
> + }
> +
> + env->pending_interrupts &= ~(1 << irq);
> +}
> +
> +void helper_book3s_msgsndp(target_ulong rb)
> +{
> + book3s_msgsnd_common(rb, 0);
> +}
> +#endif
> #endif
>
> void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 2318f3ab45b2..a0e7bd9c32d3 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -105,6 +105,28 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
>
> env->spr[SPR_PCR] = value & pcc->pcr_mask;
> }
> +
> +target_ulong helper_load_dpdes(CPUPPCState *env)
> +{
> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> + return 1;
> + }
> + return 0;
> +}
> +
> +void helper_store_dpdes(CPUPPCState *env, target_ulong val)
> +{
> + PowerPCCPU *cpu = env_archcpu(env);
> + CPUState *cs = CPU(cpu);
> +
> + if (val) {
Likewise, shouldn't val be masked or validated, rather than treating
any non-zero value as setting the interrupt for this thread?
> + /* Only one cpu for now */
> + env->pending_interrupts |= 1 << PPC_INTERRUPT_DOORBELL;
> + cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> + } else {
> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> + }
> +}
> #endif /* defined(TARGET_PPC64) */
>
> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index f5fe5d06118a..ba759ab2bb0f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6645,6 +6645,28 @@ static void gen_msgsnd(DisasContext *ctx)
> #endif /* defined(CONFIG_USER_ONLY) */
> }
>
> +#if defined(TARGET_PPC64)
> +static void gen_msgclrp(DisasContext *ctx)
> +{
> +#if defined(CONFIG_USER_ONLY)
> + GEN_PRIV;
> +#else
> + CHK_SV;
> + gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> +#endif /* defined(CONFIG_USER_ONLY) */
> +}
> +
> +static void gen_msgsndp(DisasContext *ctx)
> +{
> +#if defined(CONFIG_USER_ONLY)
> + GEN_PRIV;
> +#else
> + CHK_SV;
> + gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
> +#endif /* defined(CONFIG_USER_ONLY) */
> +}
> +#endif
> +
> static void gen_msgsync(DisasContext *ctx)
> {
> #if defined(CONFIG_USER_ONLY)
> @@ -7187,6 +7209,10 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
> GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
> PPC2_ISA300),
> GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
> +GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001,
> + PPC_NONE, PPC2_ISA207S),
> +GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001,
> + PPC_NONE, PPC2_ISA207S),
> #endif
>
> #undef GEN_INT_ARITH_ADD
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 9815df6f77c8..7c74a763ba66 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -464,6 +464,17 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> {
> gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> }
> +
> +/* DPDES */
> +static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
> +{
> + gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
> +}
> +
> +static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
> +{
> + gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
> +}
> #endif
> #endif
>
> @@ -8233,10 +8244,11 @@ static void gen_spr_power8_dpdes(CPUPPCState *env)
> {
> #if !defined(CONFIG_USER_ONLY)
> /* Directed Privileged Door-bell Exception State, used for IPI */
> - spr_register(env, SPR_DPDES, "DPDES",
> - SPR_NOACCESS, SPR_NOACCESS,
> - &spr_read_generic, SPR_NOACCESS,
> - 0x00000000);
> + spr_register_kvm_hv(env, SPR_DPDES, "DPDES",
> + SPR_NOACCESS, SPR_NOACCESS,
> + &spr_read_dpdes, SPR_NOACCESS,
> + &spr_read_dpdes, &spr_write_dpdes,
> + KVM_REG_PPC_DPDES, 0x00000000);
> #endif
> }
>
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
2019-12-17 4:00 ` David Gibson
@ 2020-01-08 15:32 ` Cédric Le Goater
2020-01-09 1:45 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2020-01-08 15:32 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
On 12/17/19 5:00 AM, David Gibson wrote:
> On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
>> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>
>> Privileged message send facilities exist on POWER8 processors and
>> later and include a register and instructions which can be used to
>> generate, observe/modify the state of and clear privileged doorbell
>> exceptions as described below.
>
> So, it's probably worth noting somewhere here that the "privileged
> doorbell" instructions, being supervisor privileged than the existing
> doorbell instructions which are hypervisor privileged.
Sure. I will add a sentence on the topic.
>> The Directed Privileged Doorbell Exception State (DPDES) register
>> reflects the state of pending privileged doorbell exceptions and can
>> also be used to modify that state. The register can be used to read
>> and modify the state of privileged doorbell exceptions for all threads
>> of a subprocessor and thus is a shared facility for that subprocessor.
>> The register can be read/written by the hypervisor and read by the
>> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
>> unavailable exception is generated.
>>
>> The privileged message send and clear instructions (msgsndp & msgclrp)
>> are used to generate and clear the presence of a directed privileged
>> doorbell exception, respectively. The msgsndp instruction can be used
>> to target any thread of the current subprocessor, msgclrp acts on the
>> thread issuing the instruction. These instructions are privileged, but
>> will generate a hypervisor facility unavailable exception if not
>> enabled in the HFSCR and executed in privileged non-hypervisor
>> state. The HV facility unavailable exception will be addressed in
>> other patch.
>>
>> Add and implement this register and instructions by reading or
>> modifying the pending interrupt state of the cpu.
>>
>> Note that TCG only supports one thread per core and so we only need to
>> worry about the cpu making the access.
>>
>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
>> duplication
>> - modified book3s_dbell2irq()
>> - moved out the support for hypervisor facility unavailable
>> exception
>> - modified commit log to add a statement on the HV FU exception
>> - checkpatch fixes
>> - compile fixes for the ppc-softmmu and linux-user targets ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Double signoff?
I don't know why git-am added an extra one :/ I hope this is gone.
>> ---
>> target/ppc/helper.h | 4 +++
>> target/ppc/excp_helper.c | 43 ++++++++++++++++++++++++++++-----
>> target/ppc/misc_helper.c | 22 +++++++++++++++++
>> target/ppc/translate.c | 26 ++++++++++++++++++++
>> target/ppc/translate_init.inc.c | 20 ++++++++++++---
>> 5 files changed, 105 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index cd0dfe383a2a..76518a1df6f0 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
>> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
>> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
>> DEF_HELPER_2(store_ptcr, void, env, tl)
>> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
>> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
>> +DEF_HELPER_1(book3s_msgsndp, void, tl)
>> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
>> #endif
>> DEF_HELPER_2(store_sdr1, void, env, tl)
>> DEF_HELPER_2(store_pidr, void, env, tl)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 50b004d00d1e..5a247945e97f 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>> }
>> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
>> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
>> - powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
>> + if (env->insns_flags & PPC_SEGMENT_64B) {
>> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
>> + } else {
>> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
>> + }
>> return;
>> }
>> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
>> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
>> }
>>
>> /* Server Processor Control */
>> -static int book3s_dbell2irq(target_ulong rb)
>> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
>> {
>> int msg = rb & DBELL_TYPE_MASK;
>>
>> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
>> * message type is 5. All other types are reserved and the
>> * instruction is a no-op
>> */
>> - return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
>> + if (msg == DBELL_TYPE_DBELL_SERVER) {
>> + return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL;
>> + }
>> +
>> + return -1;
>> }
>>
>> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
>> {
>> - int irq = book3s_dbell2irq(rb);
>> + int irq = book3s_dbell2irq(rb, 1);
>
> true/false are preferred to 0/1 for bool types.
yes or a define ?
>>
>> if (irq < 0) {
>> return;
>> @@ -1242,9 +1250,9 @@ void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
>> env->pending_interrupts &= ~(1 << irq);
>> }
>>
>> -void helper_book3s_msgsnd(target_ulong rb)
>> +static void book3s_msgsnd_common(target_ulong rb, bool hv_dbell)
>> {
>> - int irq = book3s_dbell2irq(rb);
>> + int irq = book3s_dbell2irq(rb, hv_dbell);
>> int pir = rb & DBELL_PROCIDTAG_MASK;
btw, this mask is wrong for msgsndp().
>> CPUState *cs;
>>
>> @@ -1265,6 +1273,29 @@ void helper_book3s_msgsnd(target_ulong rb)
>> }
>> qemu_mutex_unlock_iothread();
>> }
>> +
>> +void helper_book3s_msgsnd(target_ulong rb)
>> +{
>
> As noted you only need to worry about a single thread here. But
> shouldn't you validate or mask the target against that?
The mask is applied in book3s_msgsnd_common() but I am going to
change that, as the TAG bit fields have different sizes.
> I don't know
> what the hardware behaviour is if you give a bad target here: does it
> ignore it, or trap?
bad targets are no-ops. Just like for msgsnd().
>> + book3s_msgsnd_common(rb, 1);
>> +}
>> +
>> +#if defined(TARGET_PPC64)
>> +void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
>> +{
>> + int irq = book3s_dbell2irq(rb, 0);
>> +
>> + if (irq < 0) {
>> + return;
>> + }
>> +
>> + env->pending_interrupts &= ~(1 << irq);
>> +}
>> +
>> +void helper_book3s_msgsndp(target_ulong rb)
>> +{
>> + book3s_msgsnd_common(rb, 0);
>> +}
>> +#endif
>> #endif
>>
>> void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index 2318f3ab45b2..a0e7bd9c32d3 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -105,6 +105,28 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
>>
>> env->spr[SPR_PCR] = value & pcc->pcr_mask;
>> }
>> +
>> +target_ulong helper_load_dpdes(CPUPPCState *env)
>> +{
>> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> +void helper_store_dpdes(CPUPPCState *env, target_ulong val)
>> +{
>> + PowerPCCPU *cpu = env_archcpu(env);
>> + CPUState *cs = CPU(cpu);
>> +
>> + if (val) {
>
> Likewise, shouldn't val be masked or validated, rather than treating
> any non-zero value as setting the interrupt for this thread?
This is a bit wrong indeed. each bit number corresponds to a HW thread
of a core. DPDES being a shared register.
The current implementation is fine for SMT=1 but we should add a
warning for DPDES values modifying bits other than bit 63.
Thanks,
C.
>
>> + /* Only one cpu for now */
>> + env->pending_interrupts |= 1 << PPC_INTERRUPT_DOORBELL;
>> + cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>> + } else {
>> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
>> + }
>> +}
>> #endif /* defined(TARGET_PPC64) */
>>
>> void helper_store_pidr(CPUPPCState *env, target_ulong val)
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index f5fe5d06118a..ba759ab2bb0f 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -6645,6 +6645,28 @@ static void gen_msgsnd(DisasContext *ctx)
>> #endif /* defined(CONFIG_USER_ONLY) */
>> }
>>
>> +#if defined(TARGET_PPC64)
>> +static void gen_msgclrp(DisasContext *ctx)
>> +{
>> +#if defined(CONFIG_USER_ONLY)
>> + GEN_PRIV;
>> +#else
>> + CHK_SV;
>> + gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>> +#endif /* defined(CONFIG_USER_ONLY) */
>> +}
>> +
>> +static void gen_msgsndp(DisasContext *ctx)
>> +{
>> +#if defined(CONFIG_USER_ONLY)
>> + GEN_PRIV;
>> +#else
>> + CHK_SV;
>> + gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
>> +#endif /* defined(CONFIG_USER_ONLY) */
>> +}
>> +#endif
>> +
>> static void gen_msgsync(DisasContext *ctx)
>> {
>> #if defined(CONFIG_USER_ONLY)
>> @@ -7187,6 +7209,10 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
>> GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
>> PPC2_ISA300),
>> GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
>> +GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001,
>> + PPC_NONE, PPC2_ISA207S),
>> +GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001,
>> + PPC_NONE, PPC2_ISA207S),
>> #endif
>>
>> #undef GEN_INT_ARITH_ADD
>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>> index 9815df6f77c8..7c74a763ba66 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -464,6 +464,17 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
>> {
>> gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
>> }
>> +
>> +/* DPDES */
>> +static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
>> +{
>> + gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
>> +}
>> +
>> +static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
>> +{
>> + gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
>> +}
>> #endif
>> #endif
>>
>> @@ -8233,10 +8244,11 @@ static void gen_spr_power8_dpdes(CPUPPCState *env)
>> {
>> #if !defined(CONFIG_USER_ONLY)
>> /* Directed Privileged Door-bell Exception State, used for IPI */
>> - spr_register(env, SPR_DPDES, "DPDES",
>> - SPR_NOACCESS, SPR_NOACCESS,
>> - &spr_read_generic, SPR_NOACCESS,
>> - 0x00000000);
>> + spr_register_kvm_hv(env, SPR_DPDES, "DPDES",
>> + SPR_NOACCESS, SPR_NOACCESS,
>> + &spr_read_dpdes, SPR_NOACCESS,
>> + &spr_read_dpdes, &spr_write_dpdes,
>> + KVM_REG_PPC_DPDES, 0x00000000);
>> #endif
>> }
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
2020-01-08 15:32 ` Cédric Le Goater
@ 2020-01-09 1:45 ` David Gibson
2020-01-09 7:13 ` Cédric Le Goater
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2020-01-09 1:45 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 12887 bytes --]
On Wed, Jan 08, 2020 at 04:32:19PM +0100, Cédric Le Goater wrote:
> On 12/17/19 5:00 AM, David Gibson wrote:
> > On Thu, Nov 28, 2019 at 02:46:58PM +0100, Cédric Le Goater wrote:
> >> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >>
> >> Privileged message send facilities exist on POWER8 processors and
> >> later and include a register and instructions which can be used to
> >> generate, observe/modify the state of and clear privileged doorbell
> >> exceptions as described below.
> >
> > So, it's probably worth noting somewhere here that the "privileged
> > doorbell" instructions, being supervisor privileged than the existing
> > doorbell instructions which are hypervisor privileged.
>
> Sure. I will add a sentence on the topic.
>
> >> The Directed Privileged Doorbell Exception State (DPDES) register
> >> reflects the state of pending privileged doorbell exceptions and can
> >> also be used to modify that state. The register can be used to read
> >> and modify the state of privileged doorbell exceptions for all threads
> >> of a subprocessor and thus is a shared facility for that subprocessor.
> >> The register can be read/written by the hypervisor and read by the
> >> supervisor if enabled in the HFSCR, otherwise a hypervisor facility
> >> unavailable exception is generated.
> >>
> >> The privileged message send and clear instructions (msgsndp & msgclrp)
> >> are used to generate and clear the presence of a directed privileged
> >> doorbell exception, respectively. The msgsndp instruction can be used
> >> to target any thread of the current subprocessor, msgclrp acts on the
> >> thread issuing the instruction. These instructions are privileged, but
> >> will generate a hypervisor facility unavailable exception if not
> >> enabled in the HFSCR and executed in privileged non-hypervisor
> >> state. The HV facility unavailable exception will be addressed in
> >> other patch.
> >>
> >> Add and implement this register and instructions by reading or
> >> modifying the pending interrupt state of the cpu.
> >>
> >> Note that TCG only supports one thread per core and so we only need to
> >> worry about the cpu making the access.
> >>
> >> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >> [clg: - introduced a book3s_msgsnd_common() helper to reduce code
> >> duplication
> >> - modified book3s_dbell2irq()
> >> - moved out the support for hypervisor facility unavailable
> >> exception
> >> - modified commit log to add a statement on the HV FU exception
> >> - checkpatch fixes
> >> - compile fixes for the ppc-softmmu and linux-user targets ]
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >
> > Double signoff?
>
> I don't know why git-am added an extra one :/ I hope this is gone.
>
> >> ---
> >> target/ppc/helper.h | 4 +++
> >> target/ppc/excp_helper.c | 43 ++++++++++++++++++++++++++++-----
> >> target/ppc/misc_helper.c | 22 +++++++++++++++++
> >> target/ppc/translate.c | 26 ++++++++++++++++++++
> >> target/ppc/translate_init.inc.c | 20 ++++++++++++---
> >> 5 files changed, 105 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> >> index cd0dfe383a2a..76518a1df6f0 100644
> >> --- a/target/ppc/helper.h
> >> +++ b/target/ppc/helper.h
> >> @@ -657,6 +657,10 @@ DEF_HELPER_FLAGS_1(load_601_rtcu, TCG_CALL_NO_RWG, tl, env)
> >> DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> >> DEF_HELPER_FLAGS_2(store_purr, TCG_CALL_NO_RWG, void, env, tl)
> >> DEF_HELPER_2(store_ptcr, void, env, tl)
> >> +DEF_HELPER_FLAGS_1(load_dpdes, TCG_CALL_NO_RWG, tl, env)
> >> +DEF_HELPER_FLAGS_2(store_dpdes, TCG_CALL_NO_RWG, void, env, tl)
> >> +DEF_HELPER_1(book3s_msgsndp, void, tl)
> >> +DEF_HELPER_2(book3s_msgclrp, void, env, tl)
> >> #endif
> >> DEF_HELPER_2(store_sdr1, void, env, tl)
> >> DEF_HELPER_2(store_pidr, void, env, tl)
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index 50b004d00d1e..5a247945e97f 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -898,7 +898,11 @@ static void ppc_hw_interrupt(CPUPPCState *env)
> >> }
> >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> >> env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> >> - powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> >> + if (env->insns_flags & PPC_SEGMENT_64B) {
> >> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_SDOOR);
> >> + } else {
> >> + powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DOORI);
> >> + }
> >> return;
> >> }
> >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDOORBELL)) {
> >> @@ -1219,7 +1223,7 @@ void helper_msgsnd(target_ulong rb)
> >> }
> >>
> >> /* Server Processor Control */
> >> -static int book3s_dbell2irq(target_ulong rb)
> >> +static int book3s_dbell2irq(target_ulong rb, bool hv_dbell)
> >> {
> >> int msg = rb & DBELL_TYPE_MASK;
> >>
> >> @@ -1228,12 +1232,16 @@ static int book3s_dbell2irq(target_ulong rb)
> >> * message type is 5. All other types are reserved and the
> >> * instruction is a no-op
> >> */
> >> - return msg == DBELL_TYPE_DBELL_SERVER ? PPC_INTERRUPT_HDOORBELL : -1;
> >> + if (msg == DBELL_TYPE_DBELL_SERVER) {
> >> + return hv_dbell ? PPC_INTERRUPT_HDOORBELL : PPC_INTERRUPT_DOORBELL;
> >> + }
> >> +
> >> + return -1;
> >> }
> >>
> >> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> >> {
> >> - int irq = book3s_dbell2irq(rb);
> >> + int irq = book3s_dbell2irq(rb, 1);
> >
> > true/false are preferred to 0/1 for bool types.
>
> yes or a define ?
Sorry, I don't understand the question. The second argument to
book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
rather than '1'.
>
> >>
> >> if (irq < 0) {
> >> return;
> >> @@ -1242,9 +1250,9 @@ void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> >> env->pending_interrupts &= ~(1 << irq);
> >> }
> >>
> >> -void helper_book3s_msgsnd(target_ulong rb)
> >> +static void book3s_msgsnd_common(target_ulong rb, bool hv_dbell)
> >> {
> >> - int irq = book3s_dbell2irq(rb);
> >> + int irq = book3s_dbell2irq(rb, hv_dbell);
> >> int pir = rb & DBELL_PROCIDTAG_MASK;
>
> btw, this mask is wrong for msgsndp().
Ok..
> >> CPUState *cs;
> >>
> >> @@ -1265,6 +1273,29 @@ void helper_book3s_msgsnd(target_ulong rb)
> >> }
> >> qemu_mutex_unlock_iothread();
> >> }
> >> +
> >> +void helper_book3s_msgsnd(target_ulong rb)
> >> +{
> >
> > As noted you only need to worry about a single thread here. But
> > shouldn't you validate or mask the target against that?
>
> The mask is applied in book3s_msgsnd_common() but I am going to
> change that, as the TAG bit fields have different sizes.
>
> > I don't know
> > what the hardware behaviour is if you give a bad target here: does it
> > ignore it, or trap?
>
> bad targets are no-ops. Just like for msgsnd().
Ok.
> >> + book3s_msgsnd_common(rb, 1);
> >> +}
> >> +
> >> +#if defined(TARGET_PPC64)
> >> +void helper_book3s_msgclrp(CPUPPCState *env, target_ulong rb)
> >> +{
> >> + int irq = book3s_dbell2irq(rb, 0);
> >> +
> >> + if (irq < 0) {
> >> + return;
> >> + }
> >> +
> >> + env->pending_interrupts &= ~(1 << irq);
> >> +}
> >> +
> >> +void helper_book3s_msgsndp(target_ulong rb)
> >> +{
> >> + book3s_msgsnd_common(rb, 0);
> >> +}
> >> +#endif
> >> #endif
> >>
> >> void ppc_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> >> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> >> index 2318f3ab45b2..a0e7bd9c32d3 100644
> >> --- a/target/ppc/misc_helper.c
> >> +++ b/target/ppc/misc_helper.c
> >> @@ -105,6 +105,28 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
> >>
> >> env->spr[SPR_PCR] = value & pcc->pcr_mask;
> >> }
> >> +
> >> +target_ulong helper_load_dpdes(CPUPPCState *env)
> >> +{
> >> + if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> >> + return 1;
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +void helper_store_dpdes(CPUPPCState *env, target_ulong val)
> >> +{
> >> + PowerPCCPU *cpu = env_archcpu(env);
> >> + CPUState *cs = CPU(cpu);
> >> +
> >> + if (val) {
> >
> > Likewise, shouldn't val be masked or validated, rather than treating
> > any non-zero value as setting the interrupt for this thread?
>
> This is a bit wrong indeed. each bit number corresponds to a HW thread
> of a core. DPDES being a shared register.
>
> The current implementation is fine for SMT=1 but we should add a
> warning for DPDES values modifying bits other than bit 63.
>
> Thanks,
>
> C.
>
> >
> >> + /* Only one cpu for now */
> >> + env->pending_interrupts |= 1 << PPC_INTERRUPT_DOORBELL;
> >> + cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> >> + } else {
> >> + env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DOORBELL);
> >> + }
> >> +}
> >> #endif /* defined(TARGET_PPC64) */
> >>
> >> void helper_store_pidr(CPUPPCState *env, target_ulong val)
> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> >> index f5fe5d06118a..ba759ab2bb0f 100644
> >> --- a/target/ppc/translate.c
> >> +++ b/target/ppc/translate.c
> >> @@ -6645,6 +6645,28 @@ static void gen_msgsnd(DisasContext *ctx)
> >> #endif /* defined(CONFIG_USER_ONLY) */
> >> }
> >>
> >> +#if defined(TARGET_PPC64)
> >> +static void gen_msgclrp(DisasContext *ctx)
> >> +{
> >> +#if defined(CONFIG_USER_ONLY)
> >> + GEN_PRIV;
> >> +#else
> >> + CHK_SV;
> >> + gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> >> +#endif /* defined(CONFIG_USER_ONLY) */
> >> +}
> >> +
> >> +static void gen_msgsndp(DisasContext *ctx)
> >> +{
> >> +#if defined(CONFIG_USER_ONLY)
> >> + GEN_PRIV;
> >> +#else
> >> + CHK_SV;
> >> + gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
> >> +#endif /* defined(CONFIG_USER_ONLY) */
> >> +}
> >> +#endif
> >> +
> >> static void gen_msgsync(DisasContext *ctx)
> >> {
> >> #if defined(CONFIG_USER_ONLY)
> >> @@ -7187,6 +7209,10 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
> >> GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
> >> PPC2_ISA300),
> >> GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
> >> +GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001,
> >> + PPC_NONE, PPC2_ISA207S),
> >> +GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001,
> >> + PPC_NONE, PPC2_ISA207S),
> >> #endif
> >>
> >> #undef GEN_INT_ARITH_ADD
> >> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> >> index 9815df6f77c8..7c74a763ba66 100644
> >> --- a/target/ppc/translate_init.inc.c
> >> +++ b/target/ppc/translate_init.inc.c
> >> @@ -464,6 +464,17 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> >> {
> >> gen_helper_store_pcr(cpu_env, cpu_gpr[gprn]);
> >> }
> >> +
> >> +/* DPDES */
> >> +static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
> >> +{
> >> + gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
> >> +}
> >> +
> >> +static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
> >> +{
> >> + gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
> >> +}
> >> #endif
> >> #endif
> >>
> >> @@ -8233,10 +8244,11 @@ static void gen_spr_power8_dpdes(CPUPPCState *env)
> >> {
> >> #if !defined(CONFIG_USER_ONLY)
> >> /* Directed Privileged Door-bell Exception State, used for IPI */
> >> - spr_register(env, SPR_DPDES, "DPDES",
> >> - SPR_NOACCESS, SPR_NOACCESS,
> >> - &spr_read_generic, SPR_NOACCESS,
> >> - 0x00000000);
> >> + spr_register_kvm_hv(env, SPR_DPDES, "DPDES",
> >> + SPR_NOACCESS, SPR_NOACCESS,
> >> + &spr_read_dpdes, SPR_NOACCESS,
> >> + &spr_read_dpdes, &spr_write_dpdes,
> >> + KVM_REG_PPC_DPDES, 0x00000000);
> >> #endif
> >> }
> >>
> >
>
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
2020-01-09 1:45 ` David Gibson
@ 2020-01-09 7:13 ` Cédric Le Goater
2020-01-14 2:11 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2020-01-09 7:13 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
>>>> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
>>>> {
>>>> - int irq = book3s_dbell2irq(rb);
>>>> + int irq = book3s_dbell2irq(rb, 1);
>>>
>>> true/false are preferred to 0/1 for bool types.
>>
>> yes or a define ?
>
> Sorry, I don't understand the question. The second argument to
> book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> rather than '1'.
Yes. I was thinking of adding defines for the 'true' and 'false' value
to clarify what the parameter is doing. But it does not seem really
useful now.
Thanks,
C.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/7] target/ppc: Add privileged message send facilities
2020-01-09 7:13 ` Cédric Le Goater
@ 2020-01-14 2:11 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2020-01-14 2:11 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 923 bytes --]
On Thu, Jan 09, 2020 at 08:13:38AM +0100, Cédric Le Goater wrote:
> >>>> void helper_book3s_msgclr(CPUPPCState *env, target_ulong rb)
> >>>> {
> >>>> - int irq = book3s_dbell2irq(rb);
> >>>> + int irq = book3s_dbell2irq(rb, 1);
> >>>
> >>> true/false are preferred to 0/1 for bool types.
> >>
> >> yes or a define ?
> >
> > Sorry, I don't understand the question. The second argument to
> > book3s_dbell2irq() is a 'bool' type, so the parameter should be 'true'
> > rather than '1'.
>
> Yes. I was thinking of adding defines for the 'true' and 'false' value
> to clarify what the parameter is doing. But it does not seem really
> useful now.
You don't need your own defines, they already exist.
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
` (4 preceding siblings ...)
2019-11-28 13:46 ` [PATCH 5/7] target/ppc: Add privileged message send facilities Cédric Le Goater
@ 2019-11-28 13:46 ` Cédric Le Goater
2019-12-19 5:12 ` David Gibson
2019-11-28 13:47 ` [PATCH 7/7] target/ppc: Enforce that the root page directory size must be at least 5 Cédric Le Goater
2019-12-10 3:51 ` [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) David Gibson
7 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:46 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
The privileged message send and clear instructions (msgsndp & msgclrp)
are privileged, but will generate a hypervisor facility unavailable
exception if not enabled in the HFSCR and executed in privileged
non-hypervisor state.
Add checks when accessing the DPDES register and when using the
msgsndp and msgclrp isntructions.
Based on previous work from Suraj Jitindar Singh.
Cc: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/cpu.h | 6 ++++++
target/ppc/helper.h | 1 +
target/ppc/excp_helper.c | 9 +++++++++
target/ppc/misc_helper.c | 24 ++++++++++++++++++++++++
target/ppc/translate.c | 4 ++++
target/ppc/translate_init.inc.c | 18 ++++++++++++++++++
6 files changed, 62 insertions(+)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 8ffcfa0ea162..52608dfe6ff4 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -397,6 +397,10 @@ typedef struct ppc_v3_pate_t {
#define PSSCR_ESL PPC_BIT(42) /* Enable State Loss */
#define PSSCR_EC PPC_BIT(43) /* Exit Criterion */
+/* HFSCR bits */
+#define HFSCR_MSGP PPC_BIT(53) /* Privileged Message Send Facilities */
+#define HFSCR_IC_MSGP 0xA
+
#define msr_sf ((env->msr >> MSR_SF) & 1)
#define msr_isf ((env->msr >> MSR_ISF) & 1)
#define msr_shv ((env->msr >> MSR_SHV) & 1)
@@ -1333,6 +1337,8 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
#endif
void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
+void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
+ int sprn, int cause);
static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
{
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 76518a1df6f0..14c9a30a45c9 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -643,6 +643,7 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
DEF_HELPER_2(load_dump_spr, void, env, i32)
DEF_HELPER_2(store_dump_spr, void, env, i32)
+DEF_HELPER_4(hfscr_facility_check, void, env, i32, i32, i32)
DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 5a247945e97f..17dad626b74e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -469,6 +469,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
case POWERPC_EXCP_FU: /* Facility unavailable exception */
#ifdef TARGET_PPC64
env->spr[SPR_FSCR] |= ((target_ulong)env->error_code << 56);
+#endif
+ break;
+ case POWERPC_EXCP_HV_FU: /* Hypervisor Facility Unavailable Exception */
+#ifdef TARGET_PPC64
+ env->spr[SPR_HFSCR] |= ((target_ulong)env->error_code << FSCR_IC_POS);
+ srr0 = SPR_HSRR0;
+ srr1 = SPR_HSRR1;
+ new_msr |= (target_ulong)MSR_HVB;
+ new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
#endif
break;
case POWERPC_EXCP_PIT: /* Programmable interval timer interrupt */
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index a0e7bd9c32d3..0cd44c6edd82 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -41,6 +41,17 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
}
#ifdef TARGET_PPC64
+static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
+ uint32_t sprn, uint32_t cause,
+ uintptr_t raddr)
+{
+ qemu_log("Facility SPR %d is unavailable (SPR HFSCR:%d)\n", sprn, bit);
+
+ env->spr[SPR_HFSCR] &= ~((target_ulong)FSCR_IC_MASK << FSCR_IC_POS);
+
+ raise_exception_err_ra(env, POWERPC_EXCP_HV_FU, cause, raddr);
+}
+
static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
uint32_t sprn, uint32_t cause,
uintptr_t raddr)
@@ -55,6 +66,17 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
}
#endif
+void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
+ uint32_t sprn, uint32_t cause)
+{
+#ifdef TARGET_PPC64
+ if ((env->msr_mask & MSR_HVB) && !msr_hv &&
+ !(env->spr[SPR_HFSCR] & (1UL << bit))) {
+ raise_hv_fu_exception(env, bit, sprn, cause, GETPC());
+ }
+#endif
+}
+
void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
uint32_t sprn, uint32_t cause)
{
@@ -108,6 +130,8 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
target_ulong helper_load_dpdes(CPUPPCState *env)
{
+ helper_hfscr_facility_check(env, HFSCR_MSGP, SPR_DPDES,
+ HFSCR_IC_MSGP);
if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
return 1;
}
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ba759ab2bb0f..e9e70ca149fd 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6652,6 +6652,8 @@ static void gen_msgclrp(DisasContext *ctx)
GEN_PRIV;
#else
CHK_SV;
+ gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
+ HFSCR_IC_MSGP);
gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
#endif /* defined(CONFIG_USER_ONLY) */
}
@@ -6662,6 +6664,8 @@ static void gen_msgsndp(DisasContext *ctx)
GEN_PRIV;
#else
CHK_SV;
+ gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
+ HFSCR_IC_MSGP);
gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
#endif /* defined(CONFIG_USER_ONLY) */
}
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 7c74a763ba66..154e01451270 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -468,11 +468,15 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
/* DPDES */
static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
{
+ gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
+ HFSCR_IC_MSGP);
gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
}
static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
{
+ gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
+ HFSCR_IC_MSGP);
gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
}
#endif
@@ -7523,6 +7527,20 @@ POWERPC_FAMILY(e600)(ObjectClass *oc, void *data)
#define POWERPC970_HID5_INIT 0x00000000
#endif
+void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
+ int sprn, int cause)
+{
+ TCGv_i32 t1 = tcg_const_i32(bit);
+ TCGv_i32 t2 = tcg_const_i32(sprn);
+ TCGv_i32 t3 = tcg_const_i32(cause);
+
+ gen_helper_hfscr_facility_check(cpu_env, t1, t2, t3);
+
+ tcg_temp_free_i32(t3);
+ tcg_temp_free_i32(t2);
+ tcg_temp_free_i32(t1);
+}
+
static void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
int bit, int sprn, int cause)
{
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception
2019-11-28 13:46 ` [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception Cédric Le Goater
@ 2019-12-19 5:12 ` David Gibson
2020-01-08 15:36 ` Cédric Le Goater
0 siblings, 1 reply; 20+ messages in thread
From: David Gibson @ 2019-12-19 5:12 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8373 bytes --]
On Thu, Nov 28, 2019 at 02:46:59PM +0100, Cédric Le Goater wrote:
> The privileged message send and clear instructions (msgsndp & msgclrp)
> are privileged, but will generate a hypervisor facility unavailable
> exception if not enabled in the HFSCR and executed in privileged
> non-hypervisor state.
>
> Add checks when accessing the DPDES register and when using the
> msgsndp and msgclrp isntructions.
>
> Based on previous work from Suraj Jitindar Singh.
>
> Cc: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> target/ppc/cpu.h | 6 ++++++
> target/ppc/helper.h | 1 +
> target/ppc/excp_helper.c | 9 +++++++++
> target/ppc/misc_helper.c | 24 ++++++++++++++++++++++++
> target/ppc/translate.c | 4 ++++
> target/ppc/translate_init.inc.c | 18 ++++++++++++++++++
> 6 files changed, 62 insertions(+)
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 8ffcfa0ea162..52608dfe6ff4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -397,6 +397,10 @@ typedef struct ppc_v3_pate_t {
> #define PSSCR_ESL PPC_BIT(42) /* Enable State Loss */
> #define PSSCR_EC PPC_BIT(43) /* Exit Criterion */
>
> +/* HFSCR bits */
> +#define HFSCR_MSGP PPC_BIT(53) /* Privileged Message Send Facilities */
> +#define HFSCR_IC_MSGP 0xA
> +
> #define msr_sf ((env->msr >> MSR_SF) & 1)
> #define msr_isf ((env->msr >> MSR_ISF) & 1)
> #define msr_shv ((env->msr >> MSR_SHV) & 1)
> @@ -1333,6 +1337,8 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> #endif
>
> void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
> + int sprn, int cause);
>
> static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
> {
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 76518a1df6f0..14c9a30a45c9 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -643,6 +643,7 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
>
> DEF_HELPER_2(load_dump_spr, void, env, i32)
> DEF_HELPER_2(store_dump_spr, void, env, i32)
> +DEF_HELPER_4(hfscr_facility_check, void, env, i32, i32, i32)
> DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
> DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
> DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 5a247945e97f..17dad626b74e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -469,6 +469,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> case POWERPC_EXCP_FU: /* Facility unavailable exception */
> #ifdef TARGET_PPC64
> env->spr[SPR_FSCR] |= ((target_ulong)env->error_code << 56);
> +#endif
> + break;
> + case POWERPC_EXCP_HV_FU: /* Hypervisor Facility Unavailable Exception */
> +#ifdef TARGET_PPC64
> + env->spr[SPR_HFSCR] |= ((target_ulong)env->error_code << FSCR_IC_POS);
> + srr0 = SPR_HSRR0;
> + srr1 = SPR_HSRR1;
> + new_msr |= (target_ulong)MSR_HVB;
> + new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> #endif
> break;
> case POWERPC_EXCP_PIT: /* Programmable interval timer interrupt */
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index a0e7bd9c32d3..0cd44c6edd82 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -41,6 +41,17 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
> }
>
> #ifdef TARGET_PPC64
> +static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
> + uint32_t sprn, uint32_t cause,
> + uintptr_t raddr)
> +{
> + qemu_log("Facility SPR %d is unavailable (SPR HFSCR:%d)\n", sprn, bit);
That looks overly verbose. Leftover debugging?
> + env->spr[SPR_HFSCR] &= ~((target_ulong)FSCR_IC_MASK << FSCR_IC_POS);
> +
> + raise_exception_err_ra(env, POWERPC_EXCP_HV_FU, cause, raddr);
> +}
> +
> static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
> uint32_t sprn, uint32_t cause,
> uintptr_t raddr)
> @@ -55,6 +66,17 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
> }
> #endif
>
> +void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
> + uint32_t sprn, uint32_t cause)
> +{
> +#ifdef TARGET_PPC64
> + if ((env->msr_mask & MSR_HVB) && !msr_hv &&
> + !(env->spr[SPR_HFSCR] & (1UL << bit))) {
> + raise_hv_fu_exception(env, bit, sprn, cause, GETPC());
> + }
> +#endif
> +}
> +
> void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
> uint32_t sprn, uint32_t cause)
> {
> @@ -108,6 +130,8 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
>
> target_ulong helper_load_dpdes(CPUPPCState *env)
> {
> + helper_hfscr_facility_check(env, HFSCR_MSGP, SPR_DPDES,
> + HFSCR_IC_MSGP);
> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> return 1;
> }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index ba759ab2bb0f..e9e70ca149fd 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6652,6 +6652,8 @@ static void gen_msgclrp(DisasContext *ctx)
> GEN_PRIV;
> #else
> CHK_SV;
> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
> + HFSCR_IC_MSGP);
> gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
Calling a helper for the facility check, then another helper for the
actual instruction is a bit yucky. I'd prefer if you either call out
for the facility check within the instruction helper, or generate the
instructions necessary for the HFSCR check
> #endif /* defined(CONFIG_USER_ONLY) */
> }
> @@ -6662,6 +6664,8 @@ static void gen_msgsndp(DisasContext *ctx)
> GEN_PRIV;
> #else
> CHK_SV;
> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
> + HFSCR_IC_MSGP);
> gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
> #endif /* defined(CONFIG_USER_ONLY) */
> }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 7c74a763ba66..154e01451270 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -468,11 +468,15 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> /* DPDES */
> static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
> {
> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
> + HFSCR_IC_MSGP);
> gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
> }
>
> static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
> {
> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
> + HFSCR_IC_MSGP);
> gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
> }
> #endif
> @@ -7523,6 +7527,20 @@ POWERPC_FAMILY(e600)(ObjectClass *oc, void *data)
> #define POWERPC970_HID5_INIT 0x00000000
> #endif
>
> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
> + int sprn, int cause)
> +{
> + TCGv_i32 t1 = tcg_const_i32(bit);
> + TCGv_i32 t2 = tcg_const_i32(sprn);
> + TCGv_i32 t3 = tcg_const_i32(cause);
> +
> + gen_helper_hfscr_facility_check(cpu_env, t1, t2, t3);
> +
> + tcg_temp_free_i32(t3);
> + tcg_temp_free_i32(t2);
> + tcg_temp_free_i32(t1);
> +}
> +
> static void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
> int bit, int sprn, int cause)
> {
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception
2019-12-19 5:12 ` David Gibson
@ 2020-01-08 15:36 ` Cédric Le Goater
2020-01-13 23:07 ` David Gibson
0 siblings, 1 reply; 20+ messages in thread
From: Cédric Le Goater @ 2020-01-08 15:36 UTC (permalink / raw)
To: David Gibson; +Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
On 12/19/19 6:12 AM, David Gibson wrote:
> On Thu, Nov 28, 2019 at 02:46:59PM +0100, Cédric Le Goater wrote:
>> The privileged message send and clear instructions (msgsndp & msgclrp)
>> are privileged, but will generate a hypervisor facility unavailable
>> exception if not enabled in the HFSCR and executed in privileged
>> non-hypervisor state.
>>
>> Add checks when accessing the DPDES register and when using the
>> msgsndp and msgclrp isntructions.
>>
>> Based on previous work from Suraj Jitindar Singh.
>>
>> Cc: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> target/ppc/cpu.h | 6 ++++++
>> target/ppc/helper.h | 1 +
>> target/ppc/excp_helper.c | 9 +++++++++
>> target/ppc/misc_helper.c | 24 ++++++++++++++++++++++++
>> target/ppc/translate.c | 4 ++++
>> target/ppc/translate_init.inc.c | 18 ++++++++++++++++++
>> 6 files changed, 62 insertions(+)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 8ffcfa0ea162..52608dfe6ff4 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -397,6 +397,10 @@ typedef struct ppc_v3_pate_t {
>> #define PSSCR_ESL PPC_BIT(42) /* Enable State Loss */
>> #define PSSCR_EC PPC_BIT(43) /* Exit Criterion */
>>
>> +/* HFSCR bits */
>> +#define HFSCR_MSGP PPC_BIT(53) /* Privileged Message Send Facilities */
>> +#define HFSCR_IC_MSGP 0xA
>> +
>> #define msr_sf ((env->msr >> MSR_SF) & 1)
>> #define msr_isf ((env->msr >> MSR_ISF) & 1)
>> #define msr_shv ((env->msr >> MSR_SHV) & 1)
>> @@ -1333,6 +1337,8 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
>> #endif
>>
>> void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
>> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
>> + int sprn, int cause);
>>
>> static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
>> {
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 76518a1df6f0..14c9a30a45c9 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -643,6 +643,7 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
>>
>> DEF_HELPER_2(load_dump_spr, void, env, i32)
>> DEF_HELPER_2(store_dump_spr, void, env, i32)
>> +DEF_HELPER_4(hfscr_facility_check, void, env, i32, i32, i32)
>> DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
>> DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
>> DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 5a247945e97f..17dad626b74e 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -469,6 +469,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>> case POWERPC_EXCP_FU: /* Facility unavailable exception */
>> #ifdef TARGET_PPC64
>> env->spr[SPR_FSCR] |= ((target_ulong)env->error_code << 56);
>> +#endif
>> + break;
>> + case POWERPC_EXCP_HV_FU: /* Hypervisor Facility Unavailable Exception */
>> +#ifdef TARGET_PPC64
>> + env->spr[SPR_HFSCR] |= ((target_ulong)env->error_code << FSCR_IC_POS);
>> + srr0 = SPR_HSRR0;
>> + srr1 = SPR_HSRR1;
>> + new_msr |= (target_ulong)MSR_HVB;
>> + new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>> #endif
>> break;
>> case POWERPC_EXCP_PIT: /* Programmable interval timer interrupt */
>> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
>> index a0e7bd9c32d3..0cd44c6edd82 100644
>> --- a/target/ppc/misc_helper.c
>> +++ b/target/ppc/misc_helper.c
>> @@ -41,6 +41,17 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
>> }
>>
>> #ifdef TARGET_PPC64
>> +static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
>> + uint32_t sprn, uint32_t cause,
>> + uintptr_t raddr)
>> +{
>> + qemu_log("Facility SPR %d is unavailable (SPR HFSCR:%d)\n", sprn, bit);
>
> That looks overly verbose. Leftover debugging?
No. It's inherited from raise_fu_exception(). Should we remove them ?
or use mask CPU_LOG_INT ?
>
>> + env->spr[SPR_HFSCR] &= ~((target_ulong)FSCR_IC_MASK << FSCR_IC_POS);
>> +
>> + raise_exception_err_ra(env, POWERPC_EXCP_HV_FU, cause, raddr);
>> +}
>> +
>> static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
>> uint32_t sprn, uint32_t cause,
>> uintptr_t raddr)
>> @@ -55,6 +66,17 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
>> }
>> #endif
>>
>> +void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
>> + uint32_t sprn, uint32_t cause)
>> +{
>> +#ifdef TARGET_PPC64
>> + if ((env->msr_mask & MSR_HVB) && !msr_hv &&
>> + !(env->spr[SPR_HFSCR] & (1UL << bit))) {
>> + raise_hv_fu_exception(env, bit, sprn, cause, GETPC());
>> + }
>> +#endif
>> +}
>> +
>> void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
>> uint32_t sprn, uint32_t cause)
>> {
>> @@ -108,6 +130,8 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
>>
>> target_ulong helper_load_dpdes(CPUPPCState *env)
>> {
>> + helper_hfscr_facility_check(env, HFSCR_MSGP, SPR_DPDES,
>> + HFSCR_IC_MSGP);
>> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
>> return 1;
>> }
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index ba759ab2bb0f..e9e70ca149fd 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -6652,6 +6652,8 @@ static void gen_msgclrp(DisasContext *ctx)
>> GEN_PRIV;
>> #else
>> CHK_SV;
>> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
>> + HFSCR_IC_MSGP);
>> gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>
> Calling a helper for the facility check, then another helper for the
> actual instruction is a bit yucky. I'd prefer if you either call out
> for the facility check within the instruction helper, or generate the
> instructions necessary for the HFSCR check
OK. I will take a look.
C.
>
>> #endif /* defined(CONFIG_USER_ONLY) */
>> }
>> @@ -6662,6 +6664,8 @@ static void gen_msgsndp(DisasContext *ctx)
>> GEN_PRIV;
>> #else
>> CHK_SV;
>> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
>> + HFSCR_IC_MSGP);
>> gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
>> #endif /* defined(CONFIG_USER_ONLY) */
>> }
>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>> index 7c74a763ba66..154e01451270 100644
>> --- a/target/ppc/translate_init.inc.c
>> +++ b/target/ppc/translate_init.inc.c
>> @@ -468,11 +468,15 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
>> /* DPDES */
>> static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
>> {
>> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
>> + HFSCR_IC_MSGP);
>> gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
>> }
>>
>> static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
>> {
>> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
>> + HFSCR_IC_MSGP);
>> gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
>> }
>> #endif
>> @@ -7523,6 +7527,20 @@ POWERPC_FAMILY(e600)(ObjectClass *oc, void *data)
>> #define POWERPC970_HID5_INIT 0x00000000
>> #endif
>>
>> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
>> + int sprn, int cause)
>> +{
>> + TCGv_i32 t1 = tcg_const_i32(bit);
>> + TCGv_i32 t2 = tcg_const_i32(sprn);
>> + TCGv_i32 t3 = tcg_const_i32(cause);
>> +
>> + gen_helper_hfscr_facility_check(cpu_env, t1, t2, t3);
>> +
>> + tcg_temp_free_i32(t3);
>> + tcg_temp_free_i32(t2);
>> + tcg_temp_free_i32(t1);
>> +}
>> +
>> static void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
>> int bit, int sprn, int cause)
>> {
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception
2020-01-08 15:36 ` Cédric Le Goater
@ 2020-01-13 23:07 ` David Gibson
0 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2020-01-13 23:07 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Greg Kurz, Suraj Jitindar Singh, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9326 bytes --]
On Wed, Jan 08, 2020 at 04:36:50PM +0100, Cédric Le Goater wrote:
> On 12/19/19 6:12 AM, David Gibson wrote:
> > On Thu, Nov 28, 2019 at 02:46:59PM +0100, Cédric Le Goater wrote:
> >> The privileged message send and clear instructions (msgsndp & msgclrp)
> >> are privileged, but will generate a hypervisor facility unavailable
> >> exception if not enabled in the HFSCR and executed in privileged
> >> non-hypervisor state.
> >>
> >> Add checks when accessing the DPDES register and when using the
> >> msgsndp and msgclrp isntructions.
> >>
> >> Based on previous work from Suraj Jitindar Singh.
> >>
> >> Cc: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> target/ppc/cpu.h | 6 ++++++
> >> target/ppc/helper.h | 1 +
> >> target/ppc/excp_helper.c | 9 +++++++++
> >> target/ppc/misc_helper.c | 24 ++++++++++++++++++++++++
> >> target/ppc/translate.c | 4 ++++
> >> target/ppc/translate_init.inc.c | 18 ++++++++++++++++++
> >> 6 files changed, 62 insertions(+)
> >>
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 8ffcfa0ea162..52608dfe6ff4 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -397,6 +397,10 @@ typedef struct ppc_v3_pate_t {
> >> #define PSSCR_ESL PPC_BIT(42) /* Enable State Loss */
> >> #define PSSCR_EC PPC_BIT(43) /* Exit Criterion */
> >>
> >> +/* HFSCR bits */
> >> +#define HFSCR_MSGP PPC_BIT(53) /* Privileged Message Send Facilities */
> >> +#define HFSCR_IC_MSGP 0xA
> >> +
> >> #define msr_sf ((env->msr >> MSR_SF) & 1)
> >> #define msr_isf ((env->msr >> MSR_ISF) & 1)
> >> #define msr_shv ((env->msr >> MSR_SHV) & 1)
> >> @@ -1333,6 +1337,8 @@ void cpu_ppc_set_vhyp(PowerPCCPU *cpu, PPCVirtualHypervisor *vhyp);
> >> #endif
> >>
> >> void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
> >> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
> >> + int sprn, int cause);
> >>
> >> static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
> >> {
> >> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> >> index 76518a1df6f0..14c9a30a45c9 100644
> >> --- a/target/ppc/helper.h
> >> +++ b/target/ppc/helper.h
> >> @@ -643,6 +643,7 @@ DEF_HELPER_3(store_dcr, void, env, tl, tl)
> >>
> >> DEF_HELPER_2(load_dump_spr, void, env, i32)
> >> DEF_HELPER_2(store_dump_spr, void, env, i32)
> >> +DEF_HELPER_4(hfscr_facility_check, void, env, i32, i32, i32)
> >> DEF_HELPER_4(fscr_facility_check, void, env, i32, i32, i32)
> >> DEF_HELPER_4(msr_facility_check, void, env, i32, i32, i32)
> >> DEF_HELPER_FLAGS_1(load_tbl, TCG_CALL_NO_RWG, tl, env)
> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> >> index 5a247945e97f..17dad626b74e 100644
> >> --- a/target/ppc/excp_helper.c
> >> +++ b/target/ppc/excp_helper.c
> >> @@ -469,6 +469,15 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> >> case POWERPC_EXCP_FU: /* Facility unavailable exception */
> >> #ifdef TARGET_PPC64
> >> env->spr[SPR_FSCR] |= ((target_ulong)env->error_code << 56);
> >> +#endif
> >> + break;
> >> + case POWERPC_EXCP_HV_FU: /* Hypervisor Facility Unavailable Exception */
> >> +#ifdef TARGET_PPC64
> >> + env->spr[SPR_HFSCR] |= ((target_ulong)env->error_code << FSCR_IC_POS);
> >> + srr0 = SPR_HSRR0;
> >> + srr1 = SPR_HSRR1;
> >> + new_msr |= (target_ulong)MSR_HVB;
> >> + new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
> >> #endif
> >> break;
> >> case POWERPC_EXCP_PIT: /* Programmable interval timer interrupt */
> >> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> >> index a0e7bd9c32d3..0cd44c6edd82 100644
> >> --- a/target/ppc/misc_helper.c
> >> +++ b/target/ppc/misc_helper.c
> >> @@ -41,6 +41,17 @@ void helper_store_dump_spr(CPUPPCState *env, uint32_t sprn)
> >> }
> >>
> >> #ifdef TARGET_PPC64
> >> +static void raise_hv_fu_exception(CPUPPCState *env, uint32_t bit,
> >> + uint32_t sprn, uint32_t cause,
> >> + uintptr_t raddr)
> >> +{
> >> + qemu_log("Facility SPR %d is unavailable (SPR HFSCR:%d)\n", sprn, bit);
> >
> > That looks overly verbose. Leftover debugging?
>
> No. It's inherited from raise_fu_exception(). Should we remove them ?
> or use mask CPU_LOG_INT ?
Ah. We sohuld definitely add a mask. CPU_LOG_INT sounds like the
right one.
> >> + env->spr[SPR_HFSCR] &= ~((target_ulong)FSCR_IC_MASK << FSCR_IC_POS);
> >> +
> >> + raise_exception_err_ra(env, POWERPC_EXCP_HV_FU, cause, raddr);
> >> +}
> >> +
> >> static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
> >> uint32_t sprn, uint32_t cause,
> >> uintptr_t raddr)
> >> @@ -55,6 +66,17 @@ static void raise_fu_exception(CPUPPCState *env, uint32_t bit,
> >> }
> >> #endif
> >>
> >> +void helper_hfscr_facility_check(CPUPPCState *env, uint32_t bit,
> >> + uint32_t sprn, uint32_t cause)
> >> +{
> >> +#ifdef TARGET_PPC64
> >> + if ((env->msr_mask & MSR_HVB) && !msr_hv &&
> >> + !(env->spr[SPR_HFSCR] & (1UL << bit))) {
> >> + raise_hv_fu_exception(env, bit, sprn, cause, GETPC());
> >> + }
> >> +#endif
> >> +}
> >> +
> >> void helper_fscr_facility_check(CPUPPCState *env, uint32_t bit,
> >> uint32_t sprn, uint32_t cause)
> >> {
> >> @@ -108,6 +130,8 @@ void helper_store_pcr(CPUPPCState *env, target_ulong value)
> >>
> >> target_ulong helper_load_dpdes(CPUPPCState *env)
> >> {
> >> + helper_hfscr_facility_check(env, HFSCR_MSGP, SPR_DPDES,
> >> + HFSCR_IC_MSGP);
> >> if (env->pending_interrupts & (1 << PPC_INTERRUPT_DOORBELL)) {
> >> return 1;
> >> }
> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> >> index ba759ab2bb0f..e9e70ca149fd 100644
> >> --- a/target/ppc/translate.c
> >> +++ b/target/ppc/translate.c
> >> @@ -6652,6 +6652,8 @@ static void gen_msgclrp(DisasContext *ctx)
> >> GEN_PRIV;
> >> #else
> >> CHK_SV;
> >> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
> >> + HFSCR_IC_MSGP);
> >> gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> >
> > Calling a helper for the facility check, then another helper for the
> > actual instruction is a bit yucky. I'd prefer if you either call out
> > for the facility check within the instruction helper, or generate the
> > instructions necessary for the HFSCR check
>
> OK. I will take a look.
>
> C.
>
> >
> >> #endif /* defined(CONFIG_USER_ONLY) */
> >> }
> >> @@ -6662,6 +6664,8 @@ static void gen_msgsndp(DisasContext *ctx)
> >> GEN_PRIV;
> >> #else
> >> CHK_SV;
> >> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, 0,
> >> + HFSCR_IC_MSGP);
> >> gen_helper_book3s_msgsndp(cpu_gpr[rB(ctx->opcode)]);
> >> #endif /* defined(CONFIG_USER_ONLY) */
> >> }
> >> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> >> index 7c74a763ba66..154e01451270 100644
> >> --- a/target/ppc/translate_init.inc.c
> >> +++ b/target/ppc/translate_init.inc.c
> >> @@ -468,11 +468,15 @@ static void spr_write_pcr(DisasContext *ctx, int sprn, int gprn)
> >> /* DPDES */
> >> static void spr_read_dpdes(DisasContext *ctx, int gprn, int sprn)
> >> {
> >> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
> >> + HFSCR_IC_MSGP);
> >> gen_helper_load_dpdes(cpu_gpr[gprn], cpu_env);
> >> }
> >>
> >> static void spr_write_dpdes(DisasContext *ctx, int sprn, int gprn)
> >> {
> >> + gen_hfscr_facility_check(ctx, SPR_HFSCR, HFSCR_MSGP, sprn,
> >> + HFSCR_IC_MSGP);
> >> gen_helper_store_dpdes(cpu_env, cpu_gpr[gprn]);
> >> }
> >> #endif
> >> @@ -7523,6 +7527,20 @@ POWERPC_FAMILY(e600)(ObjectClass *oc, void *data)
> >> #define POWERPC970_HID5_INIT 0x00000000
> >> #endif
> >>
> >> +void gen_hfscr_facility_check(DisasContext *ctx, int facility_sprn, int bit,
> >> + int sprn, int cause)
> >> +{
> >> + TCGv_i32 t1 = tcg_const_i32(bit);
> >> + TCGv_i32 t2 = tcg_const_i32(sprn);
> >> + TCGv_i32 t3 = tcg_const_i32(cause);
> >> +
> >> + gen_helper_hfscr_facility_check(cpu_env, t1, t2, t3);
> >> +
> >> + tcg_temp_free_i32(t3);
> >> + tcg_temp_free_i32(t2);
> >> + tcg_temp_free_i32(t1);
> >> +}
> >> +
> >> static void gen_fscr_facility_check(DisasContext *ctx, int facility_sprn,
> >> int bit, int sprn, int cause)
> >> {
> >
>
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 7/7] target/ppc: Enforce that the root page directory size must be at least 5
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
` (5 preceding siblings ...)
2019-11-28 13:46 ` [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception Cédric Le Goater
@ 2019-11-28 13:47 ` Cédric Le Goater
2019-12-10 3:51 ` [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) David Gibson
7 siblings, 0 replies; 20+ messages in thread
From: Cédric Le Goater @ 2019-11-28 13:47 UTC (permalink / raw)
To: David Gibson
Cc: Cédric Le Goater, qemu-ppc, Greg Kurz, Suraj Jitindar Singh,
qemu-devel
From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
According to the ISA the root page directory size of a radix tree for
either process or partition scoped translation must be >= 5.
Thus add this to the list of conditions checked when validating the
partition table entry in validate_pate();
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
[clg: - checkpatch fixes ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
target/ppc/mmu-radix64.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 066e324464db..b8ecb8fa1d51 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -212,6 +212,9 @@ static bool validate_pate(PowerPCCPU *cpu, uint64_t lpid, ppc_v3_pate_t *pate)
if (lpid == 0 && !msr_hv) {
return false;
}
+ if ((pate->dw0 & PATE1_R_PRTS) < 5) {
+ return false;
+ }
/* More checks ... */
return true;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps)
2019-11-28 13:46 [PATCH 0/7] target/ppc: Implement KVM support under TCG (final steps) Cédric Le Goater
` (6 preceding siblings ...)
2019-11-28 13:47 ` [PATCH 7/7] target/ppc: Enforce that the root page directory size must be at least 5 Cédric Le Goater
@ 2019-12-10 3:51 ` David Gibson
7 siblings, 0 replies; 20+ messages in thread
From: David Gibson @ 2019-12-10 3:51 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: qemu-ppc, Greg Kurz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]
On Thu, Nov 28, 2019 at 02:46:53PM +0100, Cédric Le Goater wrote:
> Hello,
>
> This is a resend of Suraj's series adding KVM support to the QEMU
> PowerNV machine under TCG :
>
> http://patchwork.ozlabs.org/cover/1094658/
>
> I have addressed the comments and kept for later the final patch
> adding partition scoped radix tree translation. I need to educate
> myself a bit more on the topic first.
I've applied 1..4 to ppc-for-5.0, the rest I'm still trying to look at
more closely.
>
> Thanks,
>
> C.
>
> Cédric Le Goater (1):
> target/ppc: add support for Hypervisor Facility Unavailable Exception
>
> Suraj Jitindar Singh (6):
> target/ppc: Implement the VTB for HV access
> target/ppc: Work [S]PURR implementation and add HV support
> target/ppc: Add SPR ASDR
> target/ppc: Add SPR TBU40
> target/ppc: Add privileged message send facilities
> target/ppc: Enforce that the root page directory size must be at least 5
>
> include/hw/ppc/ppc.h | 4 +-
> target/ppc/cpu.h | 11 ++++
> target/ppc/helper.h | 9 +++
> hw/ppc/ppc.c | 46 +++++++++++---
> linux-user/ppc/cpu_loop.c | 5 ++
> target/ppc/excp_helper.c | 52 ++++++++++++++--
> target/ppc/misc_helper.c | 46 ++++++++++++++
> target/ppc/mmu-radix64.c | 3 +
> target/ppc/timebase_helper.c | 20 ++++++
> target/ppc/translate.c | 30 +++++++++
> target/ppc/translate_init.inc.c | 105 +++++++++++++++++++++++++++-----
> 11 files changed, 297 insertions(+), 34 deletions(-)
>
--
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: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread