Requesting feedback on the implementation of 128-bit registers.

Note that there ought to be no functional change with all of this,
because 128-bit sysregs are not hereing enabled.  Despite all of the
different FEAT_* names, it's all indivisible from FEAT_D128.

I've used Int128 throughout, which seems sensible at first glance,
but *really* complicates the definition of the registers within
ARMCPU, especially when the low part is aliased with AArch32.
The only saving grace is that there are so few 128-bit registers.

Gettng there isn't trivial either.  I wasn't happy with the idea
of replicating the orig_{read,write,access} set of hooks used by
VHE, so I've rewritten that to perform the VHE redirection at
translation time, much like how we do for NV2.

In the process of rewriting VHE, re-organize registration to
simplify memory management and reduce the number of arguments
to the inner helper functions.

At one point in development, I did not have CP_REG_AA64_128BIT_MASK,
which was going to require cpreg_to_kvm_id to also take a
ARMCPRegInfo pointer.  There aren't a whole lot of calls to that
helper, but the one in hvf required reorganization to handle.
With CP_REG_AA64_128BIT_MASK, that reorganization is not required,
but I still think it's a good cleanup so I have kept it.

Alternate implementation strategies:

  (1) Disconnect the 64-bit halves of the sysregs.
      Cons:
        - The 128-bit-ness isn't screamingly obvious.
      Pros:
        - Avoid having to rearrange the ARMCPU layout, and simpler
          integration with AArch32.
        - Avoid Int128 most places, since we wind up doing quite a
          lot of packing and unpacking the structure.
        - Because Int128 *isn't* a structure for most 64-bit builds,
          we can accidentally assign a 64-bit local to a 128-bit field.
        - Possibly easier migration changes; I havn't thought about
          that too much.

  (2) Avoid adding read128fn, raw_*128fn, and most write128fn, at
      least for now, because no defined sysregs need such handling.
      E.g. the 128-bit write of TTBR0_EL1 could call the existing
      writefn for the low half and directly store the high half.
      We do still need write128fn for the TLBIP set of insns, not
      yet implemented here, but even that could avoid Int128 by
      having 2 64-bit arguments.


Thoughts?


r~


Richard Henderson (61):
  target/arm: Introduce KVMID_AA64_SYS_REG64
  target/arm: Move compare_u64 to helper.c
  target/arm/hvf: Split out sysreg.c.inc
  target/arm/hvf: Reorder DEF_SYSREG arguments
  target/arm/hvf: Add KVMID_TO_HVF, HVF_TO_KVMID
  target/arm/hvf: Remove hvf_sreg_match.key
  target/arm/hvf: Replace hvf_sreg_match with hvf_sreg_list
  target/arm/hvf: Sort the cpreg_indexes array
  target/arm/hvf: Use raw_read, raw_write to access
  target/arm: Use raw_write in cp_reg_reset
  target/arm: Rename all ARMCPRegInfo from opaque to ri
  target/arm: Drop define_one_arm_cp_reg_with_opaque
  target/arm: Restrict the scope of CPREG_FIELD32, CPREG_FIELD64
  target/arm: Replace cpreg_field_is_64bit with cpreg_field_type
  target/arm: Add CP_REG_AA32_64BIT_{SHIFT,MASK}
  target/arm: Rename CP_REG_AA32_NS_{SHIFT,MASK}
  target/arm: Convert init_cpreg_list to g_hash_table_foreach
  target/arm: Remove cp argument to ENCODE_AA64_CP_REG
  target/arm: Reorder ENCODE_AA64_CP_REG arguments
  target/arm: Split out add_cpreg_to_hashtable_aa{32,64}
  target/arm: Improve asserts in define_one_arm_cp_reg
  target/arm: Move cp processing to define_one_arm_cp_reg
  target/arm: Move cpreg elimination to define_one_arm_cp_reg
  target/arm: Add key parameter to add_cpreg_to_hashtable
  target/arm: Split out alloc_cpreg
  target/arm: Hoist the allocation of ARMCPRegInfo
  target/arm: Remove name argument to alloc_cpreg
  target/arm: Move alias setting for wildcards
  target/arm: Move writeback of CP_ANY fields
  target/arm: Move endianness fixup for 32-bit registers
  target/arm: Rename TBFLAG_A64_NV2_MEM_E20 with *_E2H
  target/arm: Split out redirect_cpreg
  target/arm: Redirect VHE FOO_EL1 -> FOO_EL2 during translation
  target/arm: Redirect VHE FOO_EL12 to FOO_EL1 during translation
  target/arm: Rename some cpreg to their aarch64 names
  target/arm: Remove define_arm_vh_e2h_redirects_aliases
  target/arm: Implement isar tests for FEAT_SYSREG128, FEAT_SYSINSTR128
  target/arm: Define CP_REG_SIZE_U128
  target/arm: Update ARMCPRegInfo for 128-bit sysregs
  target/arm: Assert ARM_CP_128BIT only with ARM_CP_STATE_AA64
  target/arm: Split add_cpreg_to_hashtable_aa64
  target/arm: Add raw_read128, raw_write128
  target/arm: Add read_raw_cp_reg128, write_raw_cp_reg128
  target/arm: Put 128-bit sysregs into a separate list
  target/arm/kvm: Assert no 128-bit sysregs in kvm_arm_init_cpreg_list
  target/arm/hvf: Assert no 128-bit sysregs in hvf_arch_init_vcpu
  migration: Add vmstate_info_int128
  target/arm: Migrate cpreg128 registers
  target/arm: Add syn_aa64_sysreg128trap
  target/arm: Introduce helper_{get,set}_cp_reg128
  target/arm: Implement MRRS, MSRR, SYSP
  include/qemu/compiler: Introduce HOST_ENDIAN_FIELDS
  include/hw/core/cpu: Use HOST_ENDIAN_FIELDS in IcountDecr
  include/qemu/host-utils: Use HOST_ENDIAN_FIELDS in muldiv64_rounding
  target/arm: Use HOST_ENDIAN_FIELDS in CPUARMState
  target/arm: Consolidate definitions of PAR
  target/arm: Extend PAR_EL1 to 128-bit
  target/arm: Consolidate definitions of TTBR[01]
  target/arm: Split out flush_if_asid_change
  target/arm: Use flush_if_asid_change in vmsa_ttbr_write
  target/arm: Extend TTBR system registers to 128-bit

 include/hw/core/cpu.h          |    8 +-
 include/migration/vmstate.h    |    1 +
 include/qemu/compiler.h        |   11 +
 include/qemu/host-utils.h      |    6 +-
 target/arm/cpregs.h            |  134 +--
 target/arm/cpu-features.h      |   10 +
 target/arm/cpu.h               |   95 +-
 target/arm/internals.h         |    4 +
 target/arm/kvm-consts.h        |   16 +-
 target/arm/syndrome.h          |   10 +
 target/arm/tcg/helper.h        |    2 +
 target/arm/tcg/translate.h     |    2 +
 hw/intc/arm_gicv3_cpuif.c      |   10 +-
 migration/vmstate-types.c      |   30 +
 target/arm/cpu.c               |   10 +-
 target/arm/gdbstub.c           |   14 +-
 target/arm/helper.c            | 1536 +++++++++++++++++++-------------
 target/arm/hvf/hvf.c           |  243 ++---
 target/arm/kvm.c               |   13 +-
 target/arm/machine.c           |   50 ++
 target/arm/ptw.c               |   14 +-
 target/arm/tcg/cpregs-at.c     |    4 +-
 target/arm/tcg/hflags.c        |    8 +-
 target/arm/tcg/op_helper.c     |   28 +
 target/arm/tcg/translate-a64.c |  207 ++++-
 target/arm/hvf/sysreg.c.inc    |  146 +++
 target/arm/tcg/a64.decode      |   12 +-
 27 files changed, 1595 insertions(+), 1029 deletions(-)
 create mode 100644 target/arm/hvf/sysreg.c.inc

-- 
2.43.0

On 8/27/25 11:03, Richard Henderson wrote:
> Requesting feedback on the implementation of 128-bit registers.

... and of course I also failed to empty out my temp directory,
so there's 4 stale patches that got included.  At least they're
obvious in being N/M instead of NN/MM.


r~

On Wed, 27 Aug 2025 at 02:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Requesting feedback on the implementation of 128-bit registers.
>
> Note that there ought to be no functional change with all of this,
> because 128-bit sysregs are not hereing enabled.  Despite all of the
> different FEAT_* names, it's all indivisible from FEAT_D128.
>
> I've used Int128 throughout, which seems sensible at first glance,
> but *really* complicates the definition of the registers within
> ARMCPU, especially when the low part is aliased with AArch32.
> The only saving grace is that there are so few 128-bit registers.
>
> Gettng there isn't trivial either.  I wasn't happy with the idea
> of replicating the orig_{read,write,access} set of hooks used by
> VHE, so I've rewritten that to perform the VHE redirection at
> translation time, much like how we do for NV2.
>
> In the process of rewriting VHE, re-organize registration to
> simplify memory management and reduce the number of arguments
> to the inner helper functions.
>
> At one point in development, I did not have CP_REG_AA64_128BIT_MASK,
> which was going to require cpreg_to_kvm_id to also take a
> ARMCPRegInfo pointer.  There aren't a whole lot of calls to that
> helper, but the one in hvf required reorganization to handle.
> With CP_REG_AA64_128BIT_MASK, that reorganization is not required,
> but I still think it's a good cleanup so I have kept it.
>
> Alternate implementation strategies:
>
>   (1) Disconnect the 64-bit halves of the sysregs.
>       Cons:
>         - The 128-bit-ness isn't screamingly obvious.
>       Pros:
>         - Avoid having to rearrange the ARMCPU layout, and simpler
>           integration with AArch32.
>         - Avoid Int128 most places, since we wind up doing quite a
>           lot of packing and unpacking the structure.
>         - Because Int128 *isn't* a structure for most 64-bit builds,
>           we can accidentally assign a 64-bit local to a 128-bit field.
>         - Possibly easier migration changes; I havn't thought about
>           that too much.

I haven't looked at the patchset in very close detail, but
(as we discussed at KVM Forum) my inclination is to prefer
to have explicitly separate halves rather than an Int128.
If we had a real compiler type int128_t everywhere I think
my opinion would be different, but as it is we can't write
operations on the field in an "obvious" way, and we have to
have clunky int128_* functions like this one:

+        flush_if_asid_change(env, int128_getlo(raw_read128(env, ri)),
+                             int128_getlo(value),
+                             ARMMMUIdxBit_E20_2 |
+                             ARMMMUIdxBit_E20_2_PAN |
+                             ARMMMUIdxBit_E20_0);

so there doesn't seem like a great deal of benefit to
defining an Int128 field.

Is some of the early part of this patchset useful refactoring
that we could get upstream already?

thanks
-- PMM

On 9/16/25 05:14, Peter Maydell wrote:
> Is some of the early part of this patchset useful refactoring
> that we could get upstream already?

Up through patch 36 reorganizes VHE to operate more like NV2, redirecting cpregs during 
translation instead of using runtime thunks.

In addition to a being a nice cleanup on its own, I think it would simplify the EXLOCK 
changes to ELR_EL1 in the GCS patch set.


r~