[v9,00/20] Invert Endian bit in SPARCv9 MMU TTE
mbox series

Message ID cover.1566466906.git.tony.nguyen@bt.com
Headers show
  • Invert Endian bit in SPARCv9 MMU TTE
Related show


Tony Nguyen Aug. 23, 2019, 1:19 a.m. UTC
From: Tony Nguyen <tony.nguyen@bt.com>

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

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.
>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.

Thanks all for the feedback. Learnt a lot =)

- Moved size+sign+endianness attributes from TCGMemOp into MemOp.
  In v1 TCGMemOp was re-purposed entirely into MemOp.
- Replaced MemOp MO_{8|16|32|64} with TCGMemOp MO_{UB|UW|UL|UQ} alias.
  This is to avoid warnings on comparing and coercing different enums.
- Renamed get_memop to get_tcgmemop for clarity.
- MEMOP is now SIZE_MEMOP, which is just ctzl(size).
- Split patch 3/4 so one memory_region_dispatch_{read|write} interface
  is converted per patch.
- Do not reuse TLB_RECHECK, use new TLB_FORCE_SLOW instead.
- Split patch 4/4 so adding the MemTxAddrs parameters and converting
  tlb_set_page() to tlb_set_page_with_attrs() is separate from usage.
- CC'd maintainers.

- Like v1, the entire TCGMemOp enum is now MemOp.
- MemOp target dependant attributes are conditional upon NEED_CPU_H

- Added Paolo Bonzini as include/exec/memop.h maintainer

- Improved commit messages to clarify how interface to access
  MemoryRegion will be converted from "unsigned size" to "MemOp op".
- Moved cpu_transaction_failed() MemOp conversion from patch #11 to #9
  to make review easier.

- Improved commit messages.
- Include as patch #1 an earlier posted TARGET_ALIGNED_ONLY configure patch.
- Typeless macro SIZE_MEMOP is now inline.
- size_memop now includes CONFIG_DEBUG_TCG code.
- size_memop now also encodes endianness via MO_TE.
- Reverted size_memop operand "unsigned long" back to "unsigned".
- Second pass of size_memop to replace no-op place holder with MO_{8|16|32|64}.
- Delay memory_region_dispatch_{read,write} operand conversion until no-op
  size_memop is implemented so we have proper typing at all points in between.
- Fixed bug where not all memory_region_dispatch_{read,write} callers where
  encoding endianness into the MemOp operand, see patch #20.
- Fixed bug where not all memory_region_dispatch_{read,write} callers were
  collapsing their byte swap into adjust_endianness, see patch #20 and #22.
- Split byte swap collapsing patch (v5 #11) into #21 and #22.
- Corrected non-common *-common-obj to *-obj.
- Replaced enum device_endian with MemOp to simplify endianness checks. A
  straight forward sed but touched *alot* of files. See patch #16 and #17.
- Deleted enum device_endian.
- Deleted DEVICE_HOST_ENDIAN definition.
- Generalized the description of introduced MemTxAttrs attribute byte_swap.

- Fixed bug where size_memop was implicitly encoding MO_TE. Endianness,
  {MO_TE|MO_BE|MO_LE}, is now explicitly encoded by MemoryRegion accessors.
- While a no-op, size_memop return type remains an unsigned.
- Use '= 0' short hand instead of macro logic to declare host endianness.
- With new set of constant arguments, sanity checked the compiler is still
  folding away tests in cputlb.c
- Re-declared many native endian devices as little or big endian. This is why
  v7 has +16 patches.

- Split out re-declaration of native endian devices as little or big endian.
  The clean up approach was too naive, and touched too many subsystem.
- Introduced devend_memop converter which the clean up was trying to avoid.
- Collapsed bswap in flatview_{read|write}_continue into adjust_endianness.
- Fixed pci{stg|lg}_service_call to access MemoryRegion with MO_BE not MO_LE.
- Fixed memory_region_{add|del}_eventfd to adjust_endianness with target,
  not host, endianness.
- Used git send-email... apologies to all for the HTML!

- Rebase on master and test with git am... again apologies to all, thanks for
  the patience =)

Tony Nguyen (20):
  tcg: TCGMemOp is now accelerator independent MemOp
  memory: Introduce size_memop
  target/mips: Access MemoryRegion with MemOp
  hw/s390x: Access MemoryRegion with MemOp
  hw/intc/armv7m_nic: Access MemoryRegion with MemOp
  hw/virtio: Access MemoryRegion with MemOp
  hw/vfio: Access MemoryRegion with MemOp
  exec: Access MemoryRegion with MemOp
  cputlb: Access MemoryRegion with MemOp
  memory: Access MemoryRegion with MemOp
  hw/s390x: Hard code size with MO_{8|16|32|64}
  target/mips: Hard code size with MO_{8|16|32|64}
  exec: Hard code size with MO_{8|16|32|64}
  memory: Access MemoryRegion with endianness
  cputlb: Replace size and endian operands for MemOp
  memory: Single byte swap along the I/O path
  cpu: TLB_FLAGS_MASK bit to force memory slow path
  cputlb: Byte swap memory transaction attribute
  target/sparc: Add TLB entry with attributes
  target/sparc: sun4u Invert Endian TTE bit

 MAINTAINERS                             |   1 +
 accel/tcg/cputlb.c                      | 197 ++++++++++--------------
 exec.c                                  |  10 +-
 hw/intc/armv7m_nvic.c                   |  13 +-
 hw/s390x/s390-pci-inst.c                |  11 +-
 hw/vfio/pci-quirks.c                    |   7 +-
 hw/virtio/virtio-pci.c                  |  15 +-
 include/exec/cpu-all.h                  |  10 +-
 include/exec/memattrs.h                 |   2 +
 include/exec/memop.h                    | 134 ++++++++++++++++
 include/exec/memory.h                   |  12 +-
 memory.c                                |  58 ++++---
 memory_ldst.inc.c                       |  81 ++--------
 target/alpha/translate.c                |   2 +-
 target/arm/translate-a64.c              |  48 +++---
 target/arm/translate-a64.h              |   2 +-
 target/arm/translate-sve.c              |   2 +-
 target/arm/translate.c                  |  32 ++--
 target/arm/translate.h                  |   2 +-
 target/hppa/translate.c                 |  14 +-
 target/i386/translate.c                 | 132 ++++++++--------
 target/m68k/translate.c                 |   2 +-
 target/microblaze/translate.c           |   4 +-
 target/mips/op_helper.c                 |   5 +-
 target/mips/translate.c                 |   8 +-
 target/openrisc/translate.c             |   4 +-
 target/ppc/translate.c                  |  12 +-
 target/riscv/insn_trans/trans_rva.inc.c |   8 +-
 target/riscv/insn_trans/trans_rvi.inc.c |   4 +-
 target/s390x/translate.c                |   6 +-
 target/s390x/translate_vx.inc.c         |  10 +-
 target/sparc/cpu.h                      |   2 +
 target/sparc/mmu_helper.c               |  40 +++--
 target/sparc/translate.c                |  14 +-
 target/tilegx/translate.c               |  10 +-
 target/tricore/translate.c              |   8 +-
 tcg/README                              |   2 +-
 tcg/aarch64/tcg-target.inc.c            |  26 ++--
 tcg/arm/tcg-target.inc.c                |  26 ++--
 tcg/i386/tcg-target.inc.c               |  24 +--
 tcg/mips/tcg-target.inc.c               |  16 +-
 tcg/optimize.c                          |   2 +-
 tcg/ppc/tcg-target.inc.c                |  12 +-
 tcg/riscv/tcg-target.inc.c              |  20 +--
 tcg/s390/tcg-target.inc.c               |  14 +-
 tcg/sparc/tcg-target.inc.c              |   6 +-
 tcg/tcg-op.c                            |  38 ++---
 tcg/tcg-op.h                            |  86 +++++------
 tcg/tcg.c                               |   2 +-
 tcg/tcg.h                               | 101 +-----------
 trace/mem-internal.h                    |   4 +-
 trace/mem.h                             |   4 +-
 52 files changed, 662 insertions(+), 643 deletions(-)
 create mode 100644 include/exec/memop.h



Richard Henderson Aug. 23, 2019, 2:29 a.m. UTC | #1
On 8/22/19 6:19 PM, Tony Nguyen wrote:
> v9:
> - Rebase on master and test with git am... again apologies to all, thanks for
>   the patience =)

So... after an hour only the cover letter has arrived.
I'm thinking that it didn't work.

Tony Nguyen Aug. 23, 2019, 12:25 p.m. UTC | #2
On 8/22/19 12:29 PM, Richard Henderson wrote:
> So... after an hour only the cover letter has arrived.
> I'm thinking that it didn't work.

Frustratingly, I keep getting blocked.

Perhaps new GMail and Mail.com accounts sending in bulk on behalf of bt.com
falls foul of spam filters.


I am mindful tcg-next has a subset of the series.

Again, thanks for the patience. 

Warm Regards,
Richard Henderson Aug. 23, 2019, 4:46 p.m. UTC | #3
On 8/23/19 5:25 AM, tony.nguyen@bt.com wrote:
> On 8/22/19 12:29 PM, Richard Henderson wrote:
>> So... after an hour only the cover letter has arrived.
>> I'm thinking that it didn't work.
> Frustratingly, I keep getting blocked.
> Perhaps new GMail and Mail.com accounts sending in bulk on behalf of bt.com
> falls foul of spam filters.

I have no idea how to solve this.  Perhaps a personal gmail account, bypassing

In the meantime, I assume v9 is intended to be the same as v8, except for the
wrapped lines?  How about you push your branch to a public facing git repo
somewhere and I'll do a final review and pull from there.

Richard Henderson Aug. 23, 2019, 6:51 p.m. UTC | #4
On 8/23/19 5:25 AM, tony.nguyen@bt.com wrote:
> Frustratingly, I keep getting blocked.

Whee!  They have all arrived and applied correctly.
Thanks for your persistence.