lkml.org 
[lkml]   [2022]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH v5 037/104] KVM: x86/mmu: Allow non-zero init value for shadow PTE
From
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

\
 
 \ /
  Last update: 2022-04-06 07:17    [W:0.179 / U:0.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site