From: David Gibson <david@gibson.dropbear.id.au>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: qemu-ppc@nongnu.org, Greg Kurz <groug@kaod.org>,
Suraj Jitindar Singh <sjitindarsingh@gmail.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH 6/7] target/ppc: add support for Hypervisor Facility Unavailable Exception
Date: Tue, 14 Jan 2020 09:07:22 +1000 [thread overview]
Message-ID: <20200113230722.GH19995@umbus> (raw)
In-Reply-To: <6673b3eb-9dad-5e55-2013-d5edfde718d9@kaod.org>
[-- 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 --]
next prev parent reply other threads:[~2020-01-13 23:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
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-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
2019-11-29 1:53 ` David Gibson
2019-11-28 13:46 ` [PATCH 3/7] target/ppc: Add SPR ASDR Cédric Le Goater
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é
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
2020-01-09 1:45 ` David Gibson
2020-01-09 7:13 ` Cédric Le Goater
2020-01-14 2:11 ` David Gibson
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
2020-01-13 23:07 ` David Gibson [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200113230722.GH19995@umbus \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=groug@kaod.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=sjitindarsingh@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.