Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dc2cfd5e-8212-dfc7-28cd-9e3a1d63c638@intel.com>
Date:   Thu, 14 Jul 2022 12:09:57 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH v7 000/102] KVM TDX basic feature support
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>, isaku.yamahata@intel.com
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
 <Ys9rcnyIZlUc76iG@google.com>
From:   Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <Ys9rcnyIZlUc76iG@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/14/2022 9:03 AM, Sean Christopherson wrote:
> On Mon, Jun 27, 2022, isaku.yamahata@intel.com wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> KVM TDX basic feature support
>>
>> Hello.  This is v7 the patch series vof KVM TDX support.
>> This is based on v5.19-rc1 + kvm/queue branch + TDX HOST patch series.
>> The tree can be found at https://github.com/intel/tdx/tree/kvm-upstream
>> How to run/test: It's describe at https://github.com/intel/tdx/wiki/TDX-KVM
>>
>> Major changes from v6:
>> - rebased to v5.19 base
>>
>> TODO:
>> - integrate fd-based guest memory. As the discussion is still on-going, I
>>    intentionally dropped fd-based guest memory support yet.  The integration can
>>    be found at https://github.com/intel/tdx/tree/kvm-upstream-workaround.
>> - 2M large page support. It's work-in-progress.
>> For large page support, there are several design choices. Here is the design options.
>> Any thoughts/feedback?
> 
> Apologies, I didn't read beyond the intro paragraph.  In case something like this
> comes up again, it's probably best to send a standalone email tagged RFC, I doubt
> I'm the only one that missed this embedded RFC.
> 
>> KVM MMU Large page support for TDX
>   
> ...
> 
>> * options to track private or shared
>> At each page size (4KB, 2MB, and 1GB), track private, shared, or mixed (2MB and
>> 1GB case). For 4KB each page, 1 bit per page is needed. private or shared.  For
>> large pages (2MB and 1GB), 2 bits per large page is needed. (private, shared, or
>> mixed).  When resolving KVM page fault, we don't want to check the lower-size
>> pages to check if the given GPA can be a large for performance.  On MapGPA check
>> it instead.
>>
>> Option A). enhance kvm_arch_memory_slot
>>    enum kvm_page_type {
>>         KVM_PAGE_TYPE_INVALID,
>>         KVM_PAGE_TYPE_SHARED,
>>         KVM_PAGE_TYPE_PRIVATE,
>>         KVM_PAGE_TYPE_MIXED,
>>    };
>>
>>    struct kvm_page_attr {
>>         enum kvm_page_type type;
>>    };
>>
>>   struct kvm_arch_memory_slot {
>>   +      struct kvm_page_attr *page_attr[KVM_NR_PAGE_SIZES];
>>
>> Option B). steal one more bit SPTE_MIXED_MASK in addition to SPTE_SHARED_MASK
>> If !SPTE_MIXED_MASK, it can be large page.

I don't think this is a good option, since it requires all the mappings 
exist all the time both in shared spte tree and private spte tree.

>> Option C). use SPTE_SHARED_MASK and kvm_mmu_page::mixed bitmap
>> kvm_mmu_page::mixed bitmap of 1GB, root indicates mixed for 2MB, 1GB.
>>
>>
>> * comparison
>> A).
>> + straightforward to implement
>> + SPTE_SHARED_MASK isn't needed
>> - memory overhead compared to B). or C).
>> - more memory reference on KVM page fault
>>
>> B).
>> + simpler than C) (complex than A)?)
>> + efficient on KVM page fault. (only SPTE reference)
>> + low memory overhead
>> - Waste precious SPTE bits.
>>
>> C).
>> + efficient on KVM page fault. (only SPTE reference)
>> + low memory overhead
>> - complicates MapGPA
>> - scattered data structure
> 
> Option D). track shared regions in an Xarray, update kvm_arch_memory_slot.lpage_info
> on insertion/removal to (dis)allow hugepages as needed.

UPM v7[1] introduces "struct xarray mem_attr_array" to track the 
shared/private attr of a range.

So in kvm_vm_ioctl_set_encrypted_region() it needs to

- increase the lpage_info counter when a 2m/1g range changed from 
identical to mixed, and

- decrease the counter when mixed -> identical

[1]: 
https://lore.kernel.org/all/20220706082016.2603916-12-chao.p.peng@linux.intel.com/

> 
>    + efficient on KVM page fault (no new lookups)
>    + zero memory overhead (assuming KVM has to eat the cost of the Xarray anyways)
>    + straightforward to implement
>    + can (and should) be merged as part of the UPM series
> 
> I believe xa_for_each_range() can be used to see if a given 2mb/1gb range is
> completely covered (fully shared) or not covered at all (fully private), but I'm
> not 100% certain that xa_for_each_range() works the way I think it does.