RE: [RFC PATCH v3 26/34] Hexagon (target/hexagon) macros referenced in instruction semantics

[Taylor Simpson <tsimpson@quicinc.com>]

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, October 8, 2020 11:31 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: philmd@redhat.com; laurent@vivier.eu; riku.voipio@iki.fi;
> aleksandar.m.mail@gmail.com; ale@rev.ng
> Subject: Re: [RFC PATCH v3 26/34] Hexagon (target/hexagon) macros
> referenced in instruction semantics
>
> >> Ah, so I see what you're trying to do with the merge thing, which had the
> >> host-endian problems.
> >>
> >> I think the merge stuff is a mistake.  I think you can get the semantics that
> >> you want with
> >>
> >> probe_read(ld_addr, ld_len)
> >> qemu_st(st_value, st_addr)
> >> qemu_ld(ld_value, ld_addr)
> >>
> >> In this way, all exceptions are recognized before the store is complete,
> the
> >> normal memory operations handle any possible overlap.
> >
> > Following up on this...
> >
> > 1) We don't need to do the probe_read because the load has already
> happened at this point.
>
>
> What do you mean "has already happened"?
> How can it have done without doing the merging by hand.  Which this operation ordering is intended to make unnecessary.
>
> I think you're missing the point.

Sorry I wasn't clear.  We have done the load from memory as it was at the beginning of the packet.  The result of the store is in mem_log_stores in CPUHexagonState.  This code updates the bytes that were loaded with any overlapping bytes from the store that hasn't been committed yet.

>
>
> > 2) I'm not familiar with qemu_st/qemu_ld.  Are these shorthand for
> tcg_gen_qemu_st*/tcg_gen_qemu_ld*?
>
> Yes.
>
> > We can't actually do the store at this point because it would alter the
> memory before we are sure the packet will commit (i.e., there might be still
> be an exception raised by another instruction in the packet).
>
> What other instruction?  Give me a concrete example so that I can give you a
> concrete answer.

Remember, there can be 4 instructions in a packet.  This code is only dealing with two of them (a load and a store).  Here's an example where a different instruction in the packet can fault.

    67f8:       c0 40 21 1f     1f2140c0 {      v0.uh = vsat(v0.uw,v1.uw)
    67fc:       00 45 02 a1     a1024500        memb(r2+#0) = r5
    6800:       02 c0 04 91     9104c002        r2 = memb(r4+#0) }

The vsat instruction requires a vector context.  If the thread doesn't have a vector context, an exception will be raised.  The OS can provide a context and replay the packet.

> I think you may need to do more preprocessing of the entire packet so that
> you
> can answer this sort of question (is there any other possible exception) when
> generating code.
>
> > 3) How important is it to support big endian hosts?  Would it be OK to put
> something like this to declare it isn't supported for Hexagon?
> > #if defined(HOST_WORDS_BIGENDIAN)
> > #error Hexagon guest not supported on big endian host
> > #endif
>
> That would make ./configure && make fail on such a host.
> So, no, you can't do that.
>
> >
> > 4) If the above is not OK, are the macros in bswap.h the correct ones to
> use?  Is this pseudo-code correct?
> > store_val = le32_to_cpu(store_val);
> > load_val = le32_to_cpu(load_val);
> > <merge bytes>
> > /* store_val is dead so no need to convert back */
> > load_val = cpu_to_le32(load_val)
>
> There's some misuse of cpu_to_le32 vs le32_to_cpu there (I've never liked
> those
> names, but it helps to think about what form the data is already in).

So, what is the right sequence?


Thanks,
Taylor