On Mon, Jul 06, 2026 at 03:44:29PM +0530, Chinmay Rath wrote: > > On 5/28/26 16:29, Vishal Chourasia wrote: > > From: Vishal Chourasia <[email protected]> > > > > Moving the following instructions to decodetree specification: > > sc, scv : SC-form > > rfi, rfid, rfscv, hrfid : XL-form > > > > This builds upon the previous work that moved mfmsr and mtmsr[d] > > instructions to decodetree. > > > > The changes were verified by validating that the tcg ops generated by > > those instructions remain the same, which were captured with the > > `-d in_asm,op` flag, and also by booting a pseries qemu guest. > > > > This also includes improvements from review feedback: > > - Rename helpers to uppercase (RFI/RFID/RFSCV/HRFID) to match ISA mnemonics > > - Add TRANS_FLAGS_NOT() and TRANS64_FLAGS() macro variants > > - Add REQUIRE_INSNS_FLAGS_NOT() check for flag exclusion > > - Specify lev field as uint8_t in SC instruction format > > - Remove redundant masking in SCV since lev is already 7-bit > > - Replace TARGET_PPC64 ifdefs with REQUIRE_64BIT() macro > > - Gate SC, SCV, RFI, RFID, RFSCV, and HRFID with appropriate flags > > - Replace runtime is_book3s_arch2x() check in RFI with > > TRANS_FLAGS_NOT(SEGMENT_64B) > > - Consolidate CONFIG_USER_ONLY guards into single block > > > > Signed-off-by: Vishal Chourasia <[email protected]> > > --- > > > > v2: In v1, do_rfi() path effectively only implemented the instruction > > for user-only and 64-bit builds, so on qemu-system-ppc every rfi was > > translated as invalid. This trapped OpenBIOS in exception return paths > > and caused the boot/tests to hang. In v2 restore the 32-bit system > > emulation rfi path while keeping rfid, hrfid, and rfscv 64-bit-only. > > Hi Vishal, > > Thanks for fixing this. I had some queries : > > > > > target/ppc/helper.h | 8 +- > > target/ppc/insn32.decode | 11 +++ > > target/ppc/tcg-excp_helper.c | 8 +- > > target/ppc/translate.c | 139 ++++----------------------- > > target/ppc/translate/misc-impl.c.inc | 100 +++++++++++++++++++ > > 5 files changed, 139 insertions(+), 127 deletions(-) > > <snip> > > @@ -4566,21 +4482,6 @@ GEN_HANDLER(rlwnm, 0x17, 0xFF, 0xFF, 0x00000000, > > PPC_INTEGER), > > GEN_HANDLER_E(dform39, 0x39, 0xFF, 0xFF, 0x00000000, PPC_NONE, > > PPC2_ISA205), > > /* handles stfdp, stxsd, stxssp */ > > GEN_HANDLER_E(dform3D, 0x3D, 0xFF, 0xFF, 0x00000000, PPC_NONE, > > PPC2_ISA205), > > -/* ISA v3.0 changed the extended opcode from 62 to 30 */ > > -GEN_HANDLER(rfi, 0x13, 0x12, 0x01, 0x03FF8001, PPC_FLOW), > Above PPC_FLOW flag is missing in the new code it seems. I > have pointed it out at the relevant TRANS macro as well. > > -#if defined(TARGET_PPC64) > > -GEN_HANDLER(rfid, 0x13, 0x12, 0x00, 0x03FF8001, PPC_64B), > > -#if !defined(CONFIG_USER_ONLY) > > -/* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */ > > -GEN_HANDLER_E(scv, 0x11, 0x10, 0xFF, 0x03FFF01E, PPC_NONE, PPC2_ISA300), > > -GEN_HANDLER_E(scv, 0x11, 0x00, 0xFF, 0x03FFF01E, PPC_NONE, PPC2_ISA300), > > -GEN_HANDLER_E(rfscv, 0x13, 0x12, 0x02, 0x03FF8001, PPC_NONE, PPC2_ISA300), > > -#endif > > -GEN_HANDLER(hrfid, 0x13, 0x12, 0x08, 0x03FF8001, PPC_64H), > Above PPC_64H flag is missing in the new code it seems. I have > pointed it out at the relevant TRANS macro as well. > > -#endif > > -/* Top bit of opc2 corresponds with low bit of LEV, so use two handlers */ > > -GEN_HANDLER(sc, 0x11, 0x11, 0xFF, 0x03FFF01D, PPC_FLOW), > > -GEN_HANDLER(sc, 0x11, 0x01, 0xFF, 0x03FFF01D, PPC_FLOW), > > GEN_HANDLER(mcrxr, 0x1F, 0x00, 0x10, 0x007FF801, PPC_MISC), > > GEN_HANDLER(mfspr, 0x1F, 0x13, 0x0A, 0x00000001, PPC_MISC), > > GEN_HANDLER(mftb, 0x1F, 0x13, 0x0B, 0x00000001, PPC_MFTB), <snip> > > + > > +static bool do_rfi(DisasContext *ctx, arg_RFID *a, int kind) > > +{ > > +#if defined(CONFIG_USER_ONLY) > > + gen_priv_opc(ctx); > > + return true; > > +#else > > + void (*helper)(TCGv_ptr); > > + > > + switch (kind) { > > + case RFI: > > + if (is_book3s_arch2x(ctx)) { > > + gen_invalid(ctx); > > + return true; > > + } > Since you are already using TRANS_FLAGS_NOT, is it still > necessary to have the above check ? Or, better yet, from the > other way around, since you have this check, is it necessary > to introduce and have the TRANS_FLAGS_NOT macro at all ? > I felt both are doing the same job. Let me if I am missing something. No, you are not. Latter should be the preferred approach. I will address it in the next version.
> > + REQUIRE_SV(ctx); > > + helper = gen_helper_RFI; > > + break; > > +#if defined(TARGET_PPC64) > > + case HRFID: > > + REQUIRE_HV(ctx); > > + helper = gen_helper_HRFID; > > + break; > > + case RFID: > > + REQUIRE_SV(ctx); > > + helper = gen_helper_RFID; > > + break; > > + case RFSCV: > > + REQUIRE_SV(ctx); > > + helper = gen_helper_RFSCV; > > + break; > > +#endif > > + default: > > + gen_invalid(ctx); > > + return true; > > + } > > + <snip> > > + > > +TRANS_FLAGS_NOT(SEGMENT_64B, RFI, do_rfi, RFI) > > +TRANS64(RFID, do_rfi, RFID) > > +TRANS64(HRFID, do_rfi, HRFID) > > +TRANS64_FLAGS2(ISA300, RFSCV, do_rfi, RFSCV) > Looks like we have dropped some flag checks : > RFI is missing PPC_FLOW and HRFID is missing PPC_64H > Yes, I missed adding those flags. Thank you for pointing out. And, apologies for the delay in response. I will post another version shortly. Thanks, Vishal
