Hi Sean, Thanks for taking the time to review the series. I took note of your comments across the series, and will incorporate them into the LPC discussion. On Wed Nov 8, 2023 at 6:33 PM UTC, Sean Christopherson wrote: > On Wed, Nov 08, 2023, Sean Christopherson wrote: > > On Wed, Nov 08, 2023, Nicolas Saenz Julienne wrote: > > > This RFC series introduces the necessary infrastructure to emulate VSM > > > enabled guests. It is a snapshot of the progress we made so far, and its > > > main goal is to gather design feedback. > > > > Heh, then please provide an overview of the design, and ideally context and/or > > justification for various design decisions. It doesn't need to be a proper design > > doc, and you can certainly point at other documentation for explaining VSM/VTLs, > > but a few paragraphs and/or verbose bullet points would go a long way. > > > > The documentation in patch 33 provides an explanation of VSM itself, and a little > > insight into how userspace can utilize the KVM implementation. But the documentation > > provides no explanation of the mechanics that KVM *developers* care about, e.g. > > the use of memory attributes, how memory attributes are enforced, whether or not > > an in-kernel local APIC is required, etc. > > > > Nor does the documentation explain *why*, e.g. why store a separate set of memory > > attributes per VTL "device", which by the by is broken and unnecessary. > > After speed reading the series.. An overview of the design, why you made certain > choices, and the tradeoffs between various options is definitely needed. > > A few questions off the top of my head: > > - What is the split between userspace and KVM? How did you arrive at that split? Our original design, which we discussed in the KVM forum 2023 [1] and is public [2], implemented most of VSM in-kernel. Notably we introduced VTL awareness in struct kvm_vcpu. This turned out to be way too complex: now vcpus have multiple CPU architectural states, events, apics, mmu, etc. First of all, the code turned out to be very intrusive, for example, most apic APIs had to be reworked one way or another to accommodate the fact there are multiple apics available. Also, we were forced to introduce VSM-specific semantics in x86 emulation code. But more importantly, the biggest pain has been dealing with state changes, they may be triggered remotely through requests, and some are already fairly delicate as-is. They involve a multitude of corner cases that almost never apply for a VTL aware kvm_vcpu. Especially if you factor in features like live migration. It's been a continuous source of regressions. Memory protections were implemented by using memory slot modifications. We introduced a downstream API that allows updating memory slots concurrently with vCPUs running. I think there was a similar proposal upstream from Red Hat some time ago. The result is complex, hard to generalize and slow. So we decided to move all this complexity outside of struct kvm_vcpu and, as much as possible, out of the kernel. We figures out the basic kernel building blocks that are absolutely necessary, and let user-space deal with the rest. > - How much *needs* to be in KVM? I.e. how much can be pushed to userspace while > maintaininly good performance? As I said above, the aim of the current design is to keep it as light as possible. The biggest move we made was moving VTL switching into user-space. We don't see any indication that performance is affected in a major way. But we will know for sure once we finish the implementation and test it under real use-cases. > - Why not make VTLs a first-party concept in KVM? E.g. rather than bury info > in a VTL device and APIC ID groups, why not modify "struct kvm" to support > replicating state that needs to be tracked per-VTL? Because of how memory > attributes affect hugepages, duplicating *memslots* might actually be easier > than teaching memslots to be VTL-aware. I do agree that we need to introduce some level VTL awareness into memslots. There's the hugepages issues you pointed out. But it'll be also necessary once we look at how to implement overlay pages that are per-VTL. (A topic I didn't mention in the series as I though I had managed to solve memory protections while avoiding the need for multiple slots). What I have in mind is introducing a memory slot address space per-VTL, similar to how we do things with SMM. It's important to note that requirements for overlay pages and memory protections are very different. Overlay pages are scarce, and are setup once and never change (AFAICT), so we think stopping all vCPUs, updating slots, and resuming execution will provide good enough performance. Memory protections happen very frequently, generally with page granularity, and may be short-lived. > - Is "struct kvm_vcpu" the best representation of an execution context (if I'm > getting the terminology right)? Let's forget I ever mentioned execution contexts. I used it in the hopes of making the VTL concept a little more understandable for non-VSM aware people. It's meant to be interchangeable with VTL. But I see how it creates confusion. > E.g. if 90% of the state is guaranteed to be identical for a given > vCPU across execution contexts, then modeling that with separate > kvm_vcpu structures is very inefficient. I highly doubt it's 90%, > but it might be quite high depending on how much the TFLS restricts > the state of the vCPU, e.g. if it's 64-bit only. For the record here's the private VTL state (TLFS 15.11.1): "In general, each VTL has its own control registers, RIP register, RSP register, and MSRs: SYSENTER_CS, SYSENTER_ESP, SYSENTER_EIP, STAR, LSTAR, CSTAR, SFMASK, EFER, PAT, KERNEL_GSBASE, FS.BASE, GS.BASE, TSC_AUX HV_X64_MSR_HYPERCALL HV_X64_MSR_GUEST_OS_ID HV_X64_MSR_REFERENCE_TSC HV_X64_MSR_APIC_FREQUENCY HV_X64_MSR_EOI HV_X64_MSR_ICR HV_X64_MSR_TPR HV_X64_MSR_APIC_ASSIST_PAGE HV_X64_MSR_NPIEP_CONFIG HV_X64_MSR_SIRBP HV_X64_MSR_SCONTROL HV_X64_MSR_SVERSION HV_X64_MSR_SIEFP HV_X64_MSR_SIMP HV_X64_MSR_EOM HV_X64_MSR_SINT0 – HV_X64_MSR_SINT15 HV_X64_MSR_STIMER0_COUNT – HV_X64_MSR_STIMER3_COUNT Local APIC registers (including CR8/TPR) " The rest is shared. Note that we've observed that during normal operation, VTL switches don't happen that often. The boot process is the most affected by any performance impact VSM might introduce, which issues 100000s (mostly memory protections). Nicolas [1] https://kvm-forum.qemu.org/2023/talk/TK7YGD/ [2] Partial rebase of our original implementation: https://github.com/vianpl/linux vsm
