On Wed, 27 Aug 2025 at 02:06, Richard Henderson <[email protected]> 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
