Messages in this thread | | | Date | Fri, 5 May 2023 18:49:57 +0200 | Subject | Re: [PATCH v1 2/9] KVM: x86/mmu: Add support for prewrite page tracking | From | Mickaël Salaün <> |
| |
On 05/05/2023 18:28, Sean Christopherson wrote: > On Fri, May 05, 2023, Micka�l Sala�n wrote: >> diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h >> index eb186bc57f6a..a7fb4ff888e6 100644 >> --- a/arch/x86/include/asm/kvm_page_track.h >> +++ b/arch/x86/include/asm/kvm_page_track.h >> @@ -3,6 +3,7 @@ >> #define _ASM_X86_KVM_PAGE_TRACK_H >> >> enum kvm_page_track_mode { >> + KVM_PAGE_TRACK_PREWRITE, > > Heh, just when I decide to finally kill off support for multiple modes[1] :-) > > My assessment from that changelog still holds true for this case: > > Drop "support" for multiple page-track modes, as there is no evidence > that array-based and refcounted metadata is the optimal solution for > other modes, nor is there any evidence that other use cases, e.g. for > access-tracking, will be a good fit for the page-track machinery in > general. > > E.g. one potential use case of access-tracking would be to prevent guest > access to poisoned memory (from the guest's perspective). In that case, > the number of poisoned pages is likely to be a very small percentage of > the guest memory, and there is no need to reference count the number of > access-tracking users, i.e. expanding gfn_track[] for a new mode would be > grossly inefficient. And for poisoned memory, host userspace would also > likely want to trap accesses, e.g. to inject #MC into the guest, and that > isn't currently supported by the page-track framework. > > A better alternative for that poisoned page use case is likely a > variation of the proposed per-gfn attributes overlay (linked), which > would allow efficiently tracking the sparse set of poisoned pages, and by > default would exit to userspace on access. > > Of particular relevance: > > - Using the page-track machinery is inefficient because the guest is likely > going to write-protect a minority of its memory. And this > > select KVM_EXTERNAL_WRITE_TRACKING if KVM > > is particularly nasty because simply enabling HEKI in the Kconfig will cause > KVM to allocate rmaps and gfn tracking. > > - There's no need to reference count the protection, i.e. 15 of the 16 bits of > gfn_track are dead weight. > > - As proposed, adding a second "mode" would double the cost of gfn tracking. > > - Tying the protections to the memslots will create an impossible-to-maintain > ABI because the protections will be lost if the owning memslot is deleted and > recreated. > > - The page-track framework provides incomplete protection and will lead to an > ongoing game of whack-a-mole, e.g. this patch catches the obvious cases by > adding calls to kvm_page_track_prewrite(), but misses things like kvm_vcpu_map(). > > - The scaling and maintenance issues will only get worse if/when someone tries > to support dropping read and/or execute permissions, e.g. for execute-only. > > - The code is x86-only, and is likely to stay that way for the foreseeable > future. > > The proposed alternative is to piggyback the memory attributes implementation[2] > that is being added (if all goes according to plan) for confidential VMs. This > use case (dropping permissions) came up not too long ago[3], which is why I have > a ready-made answer). > > I have no doubt that we'll need to solve performance and scaling issues with the > memory attributes implementation, e.g. to utilize xarray multi-range support > instead of storing information on a per-4KiB-page basis, but AFAICT, the core > idea is sound. And a very big positive from a maintenance perspective is that > any optimizations, fixes, etc. for one use case (CoCo vs. hardening) should also > benefit the other use case. > > [1] https://lore.kernel.org/all/20230311002258.852397-22-seanjc@google.com > [2] https://lore.kernel.org/all/Y2WB48kD0J4VGynX@google.com > [3] https://lore.kernel.org/all/Y1a1i9vbJ%2FpVmV9r@google.com
I agree, I used this mechanism because it was easier at first to rely on a previous work, but while I was working on the MBEC support, I realized that it's not the optimal way to do it.
I was thinking about using a new special EPT bit similar to EPT_SPTE_HOST_WRITABLE, but it may not be portable though. What do you think?
| |