target/ppc: Move {v,xx}sel and {v,xx}perm to decodetree, implement xxpermx #36

mferst · 2021-09-14T19:40:59Z

No description provided.

target/ppc/insn32.decode

target/ppc/int_helper.c

lffpires · 2021-09-23T17:37:06Z

target/ppc/translate/vmx-impl.c.inc

@@ -1581,7 +1607,6 @@ static bool trans_VSEL(DisasContext *ctx, arg_VA *a)
 GEN_VAFORM_PAIRED(vmsumubm, vmsummbm, 18)
 GEN_VAFORM_PAIRED(vmsumuhm, vmsumuhs, 19)
 GEN_VAFORM_PAIRED(vmsumshm, vmsumshs, 20)
-GEN_VAFORM_PAIRED(vsel, vperm, 21)


Hmm since vsel and vperm are defined together, maybe merge this commit and the previous one?

I'm trying to keep small commits to ease the review. Would it be better if I add a comment in the previous commit telling that the helper and other vsel stuff is removed here? Or should I squash these commits anyway?

It's unfortunate that they were implemented so tied together. Due to this, I would squash the 2 commits, taking into consideration that they're simple enough. To me, it causes more confusion to keep them separate than to squash them in a single one.

target/ppc/translate/vmx-impl.c.inc

lffpires · 2021-09-23T18:16:49Z

target/ppc/translate/vsx-impl.c.inc

-    int ra = xA(ctx->opcode);
-    int rb = xB(ctx->opcode);
-    int rc = xC(ctx->opcode);
+    REQUIRE_INSNS_FLAGS2(ctx, VSX);


Is REQUIRE_VSX below enough? In other words, is it possible for ctx->vsx_enabled to be true and PPC2_VSX to not be set?

If we do need both checks, what if we move the REQUIRE_INSNS_FLAGS2(ctx, VSX) inside REQUIRE_VSX?

Is REQUIRE_VSX below enough? In other words, is it possible for ctx->vsx_enabled to be true and PPC2_VSX to not be set?

ctx->vsx is set from bit 40 of MSR. In older versions of PowerISA (v2.05 and bellow), this bit was reserved, so I think we cannot infer anything from it's value if the CPU does not have VSX.

If we do need both checks, what if we move the REQUIRE_INSNS_FLAGS2(ctx, VSX) inside REQUIRE_VSX?

I guess we could move this check to REQUIRE_VSX, but we don't need to. If an instruction already checks for newer insns flags and we know that all CPUs that have that insns flag also have VSX, checking for PPC_VSX would be kind of redundant.

Ok. I fear it might be kind of confusing for other people when to use (and when not to use) REQUIRE_INSNS_FLAGS2(ctx, VSX and REQUIRE_VSX. This same problem also happens with Altivec. What do you think?

Would it be clearer if we called them REQUIRE_{VECTOR, VSX}_ENABLED or REQUIRE_MSR_{VECTOR, VSX}, etc.? Or maybe I'm being overzealous here.

There is a PPC2_VSX207 too, that makes things even more confusing. Maybe the real problem is the number of insns flags. I'll dig some manuals and the mail list archive, but maybe we can drop PPC_VSX in favor of some PPC2_ISA* + REQUIRE_VSX.

My understanding is that PPC2_VSX is a badly-named equivalent of PPC2_207 or PPC2_310 but for v2.06. AFAIK, by looking at the PowerISA book, v2.06 introduced VSX instructions, that's why it was decided to be called PPC2_VSX. However, VSX facility could still be disabled in this version or any other version, that's why REQUIRE_VSX(ctx); is necessary.

I agree it is confusing, my choice of action would be to suggest the renaming of this flag as an RFC.

Edit: Github might be showing this comment without context. See #36 (comment) if that's the case.

IIUC, PPC2_VSX was required because VSX is optional in PowerISA v2.06, so checking for the version (i.e.: adding a PPC2_ISA206) would not be enough. With the current behavior in QEMU, we'll throw an "Illegal Instruction Interrupt" when the facility is not present and a "VSX Unavailable Interrupt" when it's present but disabled.

Oh, I see. I was wrong with what I assumed previously then. Thanks for the explanation. My understanding now is that REQUIRE_INSNS_FLAGS2(ctx, VSX); can throw an "Illegal Instruction Interrupt" if either is a version before v2.06 or if it is v2.06 but VSX is not present. Is that correct?

REQUIRE_INSNS_FLAGS2 itself only checks ctx->insns_flags2. You can check in target/ppc/cpu_init.c which CPUs have this flag (currently POWER{7,8,9,10}). For instance, e500mc is a PowerISA v2.06 implementation that lacks VSX, its insns_flags2 is initialized with PPC2_BOOKE206 but not with PPC2_VEX.

target/ppc/translate/vsx-impl.c.inc

target/ppc/insn64.decode

vcoracolombo · 2022-01-04T14:15:25Z

target/ppc/translate/vmx-impl.c.inc

+static bool trans_VPERM(DisasContext *ctx, arg_VA *a)
 {
-    TCGv_ptr ra, rb, rc, rd;
-    if (unlikely(!ctx->altivec_enabled)) {
-        gen_exception(ctx, POWERPC_EXCP_VPU);
-        return;
-    }
-    ra = gen_avr_ptr(rA(ctx->opcode));
-    rb = gen_avr_ptr(rB(ctx->opcode));
-    rc = gen_avr_ptr(rC(ctx->opcode));
-    rd = gen_avr_ptr(rD(ctx->opcode));
-    gen_helper_vpermr(cpu_env, rd, ra, rb, rc);
-    tcg_temp_free_ptr(ra);
-    tcg_temp_free_ptr(rb);
-    tcg_temp_free_ptr(rc);
-    tcg_temp_free_ptr(rd);
+    TCGv_ptr vrt, vra, vrb, vrc;
+
+    REQUIRE_INSNS_FLAGS(ctx, ALTIVEC);
+    REQUIRE_VECTOR(ctx);
+
+    vrt = gen_avr_ptr(a->vrt);
+    vra = gen_avr_ptr(a->vra);
+    vrb = gen_avr_ptr(a->vrb);
+    vrc = gen_avr_ptr(a->rc);
+
+    gen_helper_VPERM(vrt, vra, vrb, vrc);
+
+    tcg_temp_free_ptr(vrt);
+    tcg_temp_free_ptr(vra);
+    tcg_temp_free_ptr(vrb);
+    tcg_temp_free_ptr(vrc);
+
+    return true;
+}
+
+static bool trans_VPERMR(DisasContext *ctx, arg_VA *a)
+{
+    TCGv_ptr vrt, vra, vrb, vrc;
+
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    REQUIRE_VECTOR(ctx);
+
+    vrt = gen_avr_ptr(a->vrt);
+    vra = gen_avr_ptr(a->vra);
+    vrb = gen_avr_ptr(a->vrb);
+    vrc = gen_avr_ptr(a->rc);
+
+    gen_helper_VPERMR(vrt, vra, vrb, vrc);
+
+    tcg_temp_free_ptr(vrt);
+    tcg_temp_free_ptr(vra);
+    tcg_temp_free_ptr(vrb);
+    tcg_temp_free_ptr(vrc);
+
+    return true;
+}


What do you think about merging these two functions using TRANS_FLAGS2 from #55 ?

vcoracolombo · 2022-01-04T14:25:36Z

target/ppc/translate/vsx-impl.c.inc

+static bool trans_XXPERM(DisasContext *ctx, arg_XX3 *a)
+{
+    TCGv_ptr xt, xa, xb;
+
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    REQUIRE_VSX(ctx);
+
+    xt = gen_vsr_ptr(a->xt);
+    xa = gen_vsr_ptr(a->xa);
+    xb = gen_vsr_ptr(a->xb);
+
+    gen_helper_VPERM(xt, xa, xt, xb);
+
+    tcg_temp_free_ptr(xt);
+    tcg_temp_free_ptr(xa);
+    tcg_temp_free_ptr(xb);
+
+    return true;
+}
+
+static bool trans_XXPERMR(DisasContext *ctx, arg_XX3 *a)
+{
+    TCGv_ptr xt, xa, xb;
+
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    REQUIRE_VSX(ctx);
+
+    xt = gen_vsr_ptr(a->xt);
+    xa = gen_vsr_ptr(a->xa);
+    xb = gen_vsr_ptr(a->xb);
+
+    gen_helper_VPERMR(xt, xa, xt, xb);
+
+    tcg_temp_free_ptr(xt);
+    tcg_temp_free_ptr(xa);
+    tcg_temp_free_ptr(xb);
+
+    return true;
+}


These functions could also be merged. Also, in #54 I implemented a do_helper_XX3 that could work here too to simplify the code, what do you think?

vcoracolombo · 2022-01-04T14:39:21Z

target/ppc/translate/vsx-impl.c.inc

-    int ra = xA(ctx->opcode);
-    int rb = xB(ctx->opcode);
-    int rc = xC(ctx->opcode);
+    REQUIRE_INSNS_FLAGS2(ctx, VSX);


My understanding is that PPC2_VSX is a badly-named equivalent of PPC2_207 or PPC2_310 but for v2.06. AFAIK, by looking at the PowerISA book, v2.06 introduced VSX instructions, that's why it was decided to be called PPC2_VSX. However, VSX facility could still be disabled in this version or any other version, that's why REQUIRE_VSX(ctx); is necessary.

I agree it is confusing, my choice of action would be to suggest the renaming of this flag as an RFC.

Edit: Github might be showing this comment without context. See #36 (comment) if that's the case.

mferst · 2022-03-09T13:20:45Z

Merged in upstream: https://lists.gnu.org/archive/html/qemu-ppc/2022-03/msg00002.html

Include the qtest reproducer provided by Alexander Bulekov in https://gitlab.com/qemu-project/qemu/-/issues/542. Without the previous commit, we get: $ make check-qtest-i386 ... Running test tests/qtest/intel-hda-test AddressSanitizer:DEADLYSIGNAL ================================================================= ==1580408==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc3d566fe0 #0 0x63d297cf in address_space_translate_internal softmmu/physmem.c:356 #1 0x63d27260 in flatview_do_translate softmmu/physmem.c:499:15 #2 0x63d27af5 in flatview_translate softmmu/physmem.c:565:15 #3 0x63d4ce84 in flatview_write softmmu/physmem.c:2850:10 #4 0x63d4cb18 in address_space_write softmmu/physmem.c:2950:18 #5 0x63d4d387 in address_space_rw softmmu/physmem.c:2960:16 #6 0x62ae12f2 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12 #7 0x62ae104a in dma_memory_rw include/sysemu/dma.h:132:12 #8 0x62ae6157 in dma_memory_write include/sysemu/dma.h:173:12 #9 0x62ae5ec0 in stl_le_dma include/sysemu/dma.h:275:1 #10 0x62ae5ba2 in stl_le_pci_dma include/hw/pci/pci.h:871:1 #11 0x62ad59a6 in intel_hda_response hw/audio/intel-hda.c:372:12 #12 0x62ad2afb in hda_codec_response hw/audio/intel-hda.c:107:5 #13 0x62aec4e1 in hda_audio_command hw/audio/hda-codec.c:655:5 #14 0x62ae05d9 in intel_hda_send_command hw/audio/intel-hda.c:307:5 #15 0x62adff54 in intel_hda_corb_run hw/audio/intel-hda.c:342:9 #16 0x62adc13b in intel_hda_set_corb_wp hw/audio/intel-hda.c:548:5 #17 0x62ae5942 in intel_hda_reg_write hw/audio/intel-hda.c:977:9 #18 0x62ada10a in intel_hda_mmio_write hw/audio/intel-hda.c:1054:5 #19 0x63d8f383 in memory_region_write_accessor softmmu/memory.c:492:5 #20 0x63d8ecc1 in access_with_adjusted_size softmmu/memory.c:554:18 #21 0x63d8d5d6 in memory_region_dispatch_write softmmu/memory.c:1504:16 #22 0x63d5e85e in flatview_write_continue softmmu/physmem.c:2812:23 #23 0x63d4d05b in flatview_write softmmu/physmem.c:2854:12 #24 0x63d4cb18 in address_space_write softmmu/physmem.c:2950:18 #25 0x63d4d387 in address_space_rw softmmu/physmem.c:2960:16 #26 0x62ae12f2 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12 #27 0x62ae104a in dma_memory_rw include/sysemu/dma.h:132:12 #28 0x62ae6157 in dma_memory_write include/sysemu/dma.h:173:12 #29 0x62ae5ec0 in stl_le_dma include/sysemu/dma.h:275:1 #30 0x62ae5ba2 in stl_le_pci_dma include/hw/pci/pci.h:871:1 #31 0x62ad59a6 in intel_hda_response hw/audio/intel-hda.c:372:12 #32 0x62ad2afb in hda_codec_response hw/audio/intel-hda.c:107:5 #33 0x62aec4e1 in hda_audio_command hw/audio/hda-codec.c:655:5 #34 0x62ae05d9 in intel_hda_send_command hw/audio/intel-hda.c:307:5 #35 0x62adff54 in intel_hda_corb_run hw/audio/intel-hda.c:342:9 #36 0x62adc13b in intel_hda_set_corb_wp hw/audio/intel-hda.c:548:5 #37 0x62ae5942 in intel_hda_reg_write hw/audio/intel-hda.c:977:9 #38 0x62ada10a in intel_hda_mmio_write hw/audio/intel-hda.c:1054:5 #39 0x63d8f383 in memory_region_write_accessor softmmu/memory.c:492:5 #40 0x63d8ecc1 in access_with_adjusted_size softmmu/memory.c:554:18 #41 0x63d8d5d6 in memory_region_dispatch_write softmmu/memory.c:1504:16 #42 0x63d5e85e in flatview_write_continue softmmu/physmem.c:2812:23 #43 0x63d4d05b in flatview_write softmmu/physmem.c:2854:12 #44 0x63d4cb18 in address_space_write softmmu/physmem.c:2950:18 #45 0x63d4d387 in address_space_rw softmmu/physmem.c:2960:16 #46 0x62ae12f2 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12 #47 0x62ae104a in dma_memory_rw include/sysemu/dma.h:132:12 #48 0x62ae6157 in dma_memory_write include/sysemu/dma.h:173:12 ... SUMMARY: AddressSanitizer: stack-overflow softmmu/physmem.c:356 in address_space_translate_internal ==1580408==ABORTING Broken pipe Aborted (core dumped) Signed-off-by: Philippe Mathieu-Daudé <[email protected]> Acked-by: Thomas Huth <[email protected]> Message-Id: <[email protected]> Signed-off-by: Thomas Huth <[email protected]>

mferst added 6 commits September 14, 2021 15:09

target/ppc: Move vsel to decodetree

7335758

target/ppc: move vperm/vpermr to decodetree

cd4040c

target/ppc: Move xxsel to decodetree

60f567b

target/ppc: move xxperm/xxpermr to decodetree

a9a93b2

target/ppc: Move xxpermdi to decodetree

a652a49

target/ppc: Implement xxpermx instruction

6e84abe

mferst requested review from lffpires and alqotel September 14, 2021 19:40

lffpires reviewed Sep 23, 2021

View reviewed changes

mferst added 2 commits September 24, 2021 10:51

fixup! target/ppc: move vperm/vpermr to decodetree

be72a1f

fixup! target/ppc: Move vsel to decodetree

cffb5ee

mferst self-assigned this Sep 29, 2021

mferst added 2 commits October 15, 2021 10:51

fixup! target/ppc: Implement xxpermx instruction

fcaa751

fixup! target/ppc: Implement xxpermx instruction

a4a8160

vcoracolombo reviewed Jan 4, 2022

View reviewed changes

fixup! target/ppc: Move xxsel to decodetree

5af7d23

mferst closed this Mar 9, 2022

target/ppc: Move {v,xx}sel and {v,xx}perm to decodetree, implement xxpermx #36

target/ppc: Move {v,xx}sel and {v,xx}perm to decodetree, implement xxpermx #36

Uh oh!

Conversation

mferst commented Sep 14, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcoracolombo Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mferst Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcoracolombo Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mferst commented Mar 9, 2022

Uh oh!

Uh oh!

vcoracolombo Jan 4, 2022 •

edited

Loading

mferst Jan 4, 2022 •

edited

Loading

vcoracolombo Jan 4, 2022 •

edited

Loading