Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <9dfc44d6-6b20-e864-8d4f-09ab7d489b97@redhat.com>
Date:   Tue, 5 Apr 2022 16:13:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value
 for shadow PTE
Content-Language: en-US
To:     Kai Huang <kai.huang@intel.com>, isaku.yamahata@intel.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     isaku.yamahata@gmail.com, Jim Mattson <jmattson@google.com>,
        erdemaktas@google.com, Connor Kuehl <ckuehl@redhat.com>,
        Sean Christopherson <seanjc@google.com>
References: <cover.1646422845.git.isaku.yamahata@intel.com>
 <b74b3660f9d16deafe83f2670539a8287bef988f.1646422845.git.isaku.yamahata@intel.com>
 <968de4765e63d8255ae1b3ac7062ffdca64706e4.camel@intel.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <968de4765e63d8255ae1b3ac7062ffdca64706e4.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 4/1/22 07:13, Kai Huang wrote:
>> @@ -617,9 +617,9 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep)
>>   	int level = sptep_to_sp(sptep)->role.level;
>>   
>>   	if (!spte_has_volatile_bits(old_spte))
>> -		__update_clear_spte_fast(sptep, 0ull);
>> +		__update_clear_spte_fast(sptep, shadow_init_value);
>>   	else
>> -		old_spte = __update_clear_spte_slow(sptep, 0ull);
>> +		old_spte = __update_clear_spte_slow(sptep, shadow_init_value);

(FWIW this one should also assume that the init_value is zero, and WARN 
if not).

> I guess it's better to have some comment here.  Allow non-zero init value for
> shadow PTE doesn't necessarily mean the initial value should be used when one
> PTE is zapped.  I think mmu_spte_clear_track_bits() is only called for mapping
> of normal (shared) memory but not MMIO? Then perhaps it's better to have a
> comment to explain we want "suppress #VE" set to get a real EPT violation for
> normal memory access from guest?
> 
>>   
>>   	if (!is_shadow_present_pte(old_spte))
>>   		return old_spte;
>> @@ -651,7 +651,7 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep)
>>    */
>>   static void mmu_spte_clear_no_track(u64 *sptep)
>>   {
>> -	__update_clear_spte_fast(sptep, 0ull);
>> +	__update_clear_spte_fast(sptep, shadow_init_value);
>>   }
> Similar here.  Seems mmu_spte_clear_no_track() is used to zap non-leaf PTE which
> doesn't require state tracking, so theoretically it can be set to 0.  But this
> seems is also called to zap MMIO PTE so looks need to set to shadow_init_value.
> Anyway looks deserve a comment?
> 
> Btw, Above two changes to mmu_spte_clear_track_bits() and
> mmu_spte_clear_track_bits() seems a little bit out-of-scope of what this patch
> claims to do.  Allow non-zero init value for shadow PTE doesn't necessarily mean
> the initial value should be used when one PTE is zapped. Maybe we can further
> improve the patch title and commit message a little bit.  Such as: Allow non-
> zero value for empty (or invalid?) PTE? Non-present seems doesn't fit here.

I think shadow_init_value is not named well.  Let's rename it to 
shadow_nonpresent_value, and the commit to "Allow non-zero value for 
non-present SPTE".  That explains why it's used for zapping.

Paolo