arm64/vmm: Use the VNCR_EL2 memory page to store guest registers
Needs ReviewPublic
Actions

Authored by kajetan.puchalski_arm.com on Tue, Apr 21, 11:08 AM.

Details

Reviewers

andrew
manu

Group Reviewers

arm64

Summary

Wherever possible, move the storage space for guest register values from
the hypctx struct into a preallocated memory page matching the layout of
the page pointed to by VNCR_EL2.
This will streamline implementing support for nested virtualization, but
the implementation itself is not reliant on the presence of nested
virtualization architecture features.

Signed-off-by: Kajetan Puchalski <kajetan.puchalski@arm.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 72410
Build 69293: arc lint + arc unit

Event Timeline

kajetan.puchalski_arm.com created this revision.Tue, Apr 21, 11:08 AM

Herald added a reviewer: andrew. · View Herald TranscriptTue, Apr 21, 11:08 AM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: emaste, imp. · View Herald Transcript

kajetan.puchalski_arm.com requested review of this revision.Tue, Apr 21, 11:08 AM

Harbormaster completed remote builds in B72410: Diff 175974.Tue, Apr 21, 11:08 AM

kajetan.puchalski_arm.com added a parent revision: D56550: arm64/vmm: Add FEAT_NV2 definitions.Tue, Apr 21, 11:08 AM

kajetan.puchalski_arm.com added a child revision: D56552: arm64/vmm: Store non-VNCR registers in an array.

This is really quite an obnoxious change to deal with for Morello, where some of these registers are 128-bit capabilities not 64-bit integers...

sys/arm64/vmm/arm64.h
258	Isn't this using a byte offset, but the enum values are consecutive?

In D56551#1294463, @jrtc27 wrote:

This is really quite an obnoxious change to deal with for Morello, where some of these registers are 128-bit capabilities not 64-bit integers...

Which ones specifically?
Do you just mean it's harder to deal with because without this you'd just expand the sizes of registers in hypctx, whereas the VNCR layout doesn't have the same flexibility?

sys/arm64/vmm/arm64.h
258	Enum values for registers defined with VNCR(...) are just the value of VNCR_START + <offset within the VNCR page>. The offsets are not necessarily evenly spaced out, e.g.: VNCR_VSTTBR_EL2 0x30 VNCR_VTCR_EL2 0x40 VNCR_VSTCR_EL2 0x48 The idea here is that for any VNCR register you can just get its enum value, subtract VNCR_START and it'll give you the offset that you need to add to the address stored in VNCR to get the memory location of said register.

In D56551#1294476, @kajetan.puchalski_arm.com wrote:

In D56551#1294463, @jrtc27 wrote:

This is really quite an obnoxious change to deal with for Morello, where some of these registers are 128-bit capabilities not 64-bit integers...

Which ones specifically?
Do you just mean it's harder to deal with because without this you'd just expand the sizes of registers in hypctx, whereas the VNCR layout doesn't have the same flexibility?

Yes, exactly. This loses all type system flexibility. At least if you just made a struct that matched the VNCR layout we could change the types, and ok it wouldn't then match, but that wouldn't matter for our use case (and if a CHERI + Arm + nested virt system ever exists then the layout can be made to match that). Whereas now we'd have to go and meticulously tweak all the magic VNCR constants to give us a layout that works, and tweak the one-function-to-rule-them-all to be two functions, one for integers and one for capabilities, since we have multiple types.

sys/arm64/vmm/arm64.h
258	Oh, I see, VNCR_REG hides all that. That's really quite weird, having the non-VNCR ones just count up from 0, whilst the VNCR ones count up in increments of 8. I don't think two different things should be in the same enum.

Yes, exactly. This loses all type system flexibility. At least if you just made a struct that matched the VNCR layout we could change the types, and ok it wouldn't then match, but that wouldn't matter for our use case (and if a CHERI + Arm + nested virt system ever exists then the layout can be made to match that). Whereas now we'd have to go and meticulously tweak all the magic VNCR constants to give us a layout that works, and tweak the one-function-to-rule-them-all to be two functions, one for integers and one for capabilities, since we have multiple types.

I know very little about CHERI & Morello, so I'm probably missing some context here. But if I understand correctly, you'd still need to add lots of CHERI-specific handling all over the hypervisor to make it work, the assumption that system registers are all uint64_t is baked into just about every piece of the logic.
However, it seems to me that you could adapt these changes fairly easily? For instance:

Add CHERI-specific values to the big enum just before VNCR_START, e.g. CHERI_<name>
Allocate an array of whatever type you need to store them
In hypctx_sys_reg, add if (reg > CHERI_START && reg < VNCR_START) { call some macro to access said CHERI-specific registers }

It'll compile down to the same thing.

sys/arm64/vmm/arm64.h
258	It depends how you see it, the idea behind this is that the enum values are there to tell you where to find the storage space for a given register. In the case of non-VNCR registers that's the array index, in the case of VNCR registers it's the offset. But that's just an implementation detail and the user doesn't need to directly worry about it. They're in the same enum because they're all system registers. The primary benefit of doing this type of enum sharing is that all of this gets completely compiled away. When you write `hypctx_read_sys_reg(hypctx, REG)` the compiler can statically determine whether you're asking for a VNCR or non-VNCR register, this entire function call gets compiled down to just a single `mrs`. If I separated the enums and the values would overlap then there'd be a need for either two separate functions or some runtime checks.

We will need to handle accessing different sized registers to support FEAT_D128, e.g. VTTBR_EL2 will be 128 bit there.

sys/arm64/vmm/arm64.h
47	I guess we could do something like this so all the values are 64-bit offsets
212	This and `host_ctx_addr` should be a pointer type
sys/arm64/vmm/vmm_arm64.c
561–562	It's probably best to merge these into the `hypctx` allocation, e.g. by adding them to the size `el2_hypctx_size` returns. For non-VHE we need to map them into the EL2 address space below with the call to `el2_map_enter`

Merge hypctx & VNCR into one allocation, adjust pointer type

Harbormaster completed remote builds in B72435: Diff 176096.Wed, Apr 22, 12:54 PM

kajetan.puchalski_arm.com marked an inline comment as done.Wed, Apr 22, 1:08 PM

kajetan.puchalski_arm.com added inline comments.

sys/arm64/vmm/arm64.h
212	I made host_ctx_addr into a pointer, this however gets folded into sys_regs in a later commit and is just the register value. In that case I think it's better to leave it as-is?

Encode VNCR values into enum hypctx_sysreg as 64-bit offsets

Harbormaster completed remote builds in B72441: Diff 176102.Wed, Apr 22, 1:23 PM

kajetan.puchalski_arm.com marked an inline comment as done.Wed, Apr 22, 1:25 PM

kajetan.puchalski_arm.com added inline comments.

sys/arm64/vmm/arm64.h
47	Sure, that works too. It'll get compiled away anyway.

In D56551#1294533, @andrew wrote:

We will need to handle accessing different sized registers to support FEAT_D128, e.g. VTTBR_EL2 will be 128 bit there.

For VNCR registers this would just be a matter of using a different accessor, all of the registers affected by FEAT_D128 already have 16 bytes of space left for them in the VNCR page layout.
For non-VNCR registers we could just add a second adjacent field in the sysregs enum, eg REG_EL2_2 or something similar and use a special accessor to make use of them both.
The mrrs and msrr instructions load and store two 64-bit GPRs, so this would get compiled pretty nicely.