From: <tony.nguyen@bt.com>
To: <qemu-devel@nongnu.org>
Cc: pbonzini@redhat.com, mark.cave-ayland@ilande.co.uk,
atar4qemu@gmail.com, rth@twiddle.net
Subject: [Qemu-devel] [PATCH 0/4] Invert Endian bit in SPARCv9 MMU TTE
Date: Wed, 17 Jul 2019 05:57:59 +0000 [thread overview]
Message-ID: <a5f7f8854af046c18c6342035662a193@tpw09926dag18e.domain1.systemhost.net> (raw)
This patchset implements the IE (Invert Endian) bit in SPARCv9 MMU TTE.
It is an attempt of the instructions outlined by Richard Henderson to Mark
Cave-Ayland.
Tested with OpenBSD on sun4u. Solaris 10 is my actual goal, but unfortunately a
separate keyboard issue remains in the way.
On 01/11/17 19:15, Mark Cave-Ayland wrote:
>On 15/08/17 19:10, Richard Henderson wrote:
>
>> [CC Peter re MemTxAttrs below]
>>
>> On 08/15/2017 09:38 AM, Mark Cave-Ayland wrote:
>>> Working through an incorrect endian issue on qemu-system-sparc64, it has
>>> become apparent that at least one OS makes use of the IE (Invert Endian)
>>> bit in the SPARCv9 MMU TTE to map PCI memory space without the
>>> programmer having to manually endian-swap accesses.
>>>
>>> In other words, to quote the UltraSPARC specification: "if this bit is
>>> set, accesses to the associated page are processed with inverse
>>> endianness from what is specified by the instruction (big-for-little and
>>> little-for-big)".
A good explanation by Mark why the IE bit is required.
>>>
>>> Looking through various bits of code, I'm trying to get a feel for the
>>> best way to implement this in an efficient manner. From what I can see
>>> this could be solved using an additional MMU index, however I'm not
>>> overly familiar with the memory and softmmu subsystems.
>>
>> No, it can't be solved with an MMU index.
>>
>>> Can anyone point me in the right direction as to what would be the best
>>> way to implement this feature within QEMU?
>>
>> It's definitely tricky.
>>
>> We definitely need some TLB_FLAGS_MASK bit set so that we're forced through
>> the
>> memory slow path. There is no other way to bypass the endianness that we've
>> already encoded from the target instruction.
>>
>> Given the tlb_set_page_with_attrs interface, I would think that we need a new
>> bit in MemTxAttrs, so that the target/sparc tlb_fill (and subroutines) can
>> pass
>> along the TTE bit for the given page.
>>
>> We have an existing problem in softmmu_template.h,
>>
>> /* ??? Note that the io helpers always read data in the target
>> byte ordering. We should push the LE/BE request down into io. */
>> res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr);
>> res = TGT_BE(res);
>>
>> We do not want to add a third(!) byte swap along the i/o path. We need to
>> collapse the two that we have already before considering this one.
>>
>> This probably takes the form of:
>>
>> (1) Replacing the "int size" argument with "TCGMemOp memop" for
>> a) io_{read,write}x in accel/tcg/cputlb.c,
>> b) memory_region_dispatch_{read,write} in memory.c,
>> c) adjust_endianness in memory.c.
>> This carries size+sign+endianness down to the next level.
>>
>> (2) In memory.c, adjust_endianness,
>>
>> if (memory_region_wrong_endianness(mr)) {
>> - switch (size) {
>> + memop ^= MO_BSWAP;
>> + }
>> + if (memop & MO_BSWAP) {
>>
>> For extra credit, re-arrange memory_region_wrong_endianness
>> to something more explicit -- "wrong" isn't helpful.
>
>Finally I've had a bit of spare time to experiment with this approach,
>and from what I can see there are currently 2 issues:
>
>
>1) Using TCGMemOp in memory.c means it is no longer accelerator agnostic
>
>For the moment I've defined a separate MemOp in memory.h and provided a
>mapping function in io_{read,write}x to map from TCGMemOp to MemOp and
>then pass that into memory_region_dispatch_{read,write}.
>
>Other than not referencing TCGMemOp in the memory API, another reason
>for doing this was that I wasn't convinced that all the MO_ attributes
>were valid outside of TCG. I do, of course, strongly defer to other
>people's knowledge in this area though.
Patch 1/4 turns TCGMemOp into accelerator agnostic MemOp.
I am mindful it is a far reaching change, and like Mark, will be grateful of
any alternative suggestions =)
>
>
>2) The above changes to adjust_endianness() fail when
>memory_region_dispatch_{read,write} are called recursively
>
>Whilst booting qemu-system-sparc64 I see that
>memory_region_dispatch_{read,write} get called recursively - once via
>io_{read,write}x and then again via flatview_read_continue() in exec.c.
>
>The net effect of this is that we perform the bswap correctly at the
>tail of the recursion, but then as we travel back up the stack we hit
>memory_region_dispatch_{read,write} once again causing a second bswap
>which means the value is returned with the incorrect endian again.
>
>
>My understanding from your softmmu_template.h comment above is that the
>memory API should do the endian swapping internally allowing the removal
>of the final TGT_BE/TGT_LE applied to the result, or did I get this wrong?
>
>> (3) In tlb_set_page_with_attrs, notice attrs.byte_swap and set
>> a new TLB_FORCE_SLOW bit within TLB_FLAGS_MASK.
>>
>> (4) In io_{read,write}x, if iotlbentry->attrs.byte_swap is set,
>> then memop ^= MO_BSWAP.
Tony Nguyen.
next reply other threads:[~2019-07-17 5:58 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-17 5:57 tony.nguyen [this message]
2019-07-17 6:04 ` [Qemu-devel] [PATCH 1/4] tcg: TCGMemOp is now accelerator independent MemOp tony.nguyen
2019-07-17 14:04 ` Richard Henderson
2019-07-17 6:06 ` [Qemu-devel] [PATCH 2/4] memory: Single byte swap along the I/O path tony.nguyen
2019-07-17 10:08 ` Paolo Bonzini
2019-07-17 14:24 ` Richard Henderson
2019-07-17 6:08 ` [Qemu-devel] [PATCH 3/4] cputlb: Byte swap memory transaction attribute tony.nguyen
2019-07-17 14:29 ` Richard Henderson
2019-07-17 6:10 ` [Qemu-devel] [PATCH 4/4] target/sparc: sun4u Invert Endian TTE bit tony.nguyen
2019-07-21 19:50 ` Mark Cave-Ayland
2019-07-17 10:09 ` [Qemu-devel] [PATCH 0/4] Invert Endian bit in SPARCv9 MMU TTE Paolo Bonzini
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=a5f7f8854af046c18c6342035662a193@tpw09926dag18e.domain1.systemhost.net \
--to=tony.nguyen@bt.com \
--cc=atar4qemu@gmail.com \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).