On Fri, Aug 23, 2024 at 4:57 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> +static u64 modify_spte_protections(u64 spte, u64 set, u64 clear)
> {
> bool is_access_track = is_access_track_spte(spte);
>
> if (is_access_track)
> spte = restore_acc_track_spte(spte);
>
> - spte &= ~shadow_nx_mask;
> - spte |= shadow_x_mask;
> + spte = (spte | set) & ~clear;
We should add a check here WARN_ON_ONCE(set & clear) because if both
have a common bit set to 1 then the result will be different between:
1. spte = (spt | set) & ~clear
2. spte = (spt | ~clear) & set
In the current form, 'clear' has more authority in the final value of spte.
>
> +u64 make_huge_spte(struct kvm *kvm, u64 small_spte, int level)
> +{
> + u64 huge_spte;
> +
> + if (KVM_BUG_ON(!is_shadow_present_pte(small_spte), kvm))
> + return SHADOW_NONPRESENT_VALUE;
> +
> + if (KVM_BUG_ON(level == PG_LEVEL_4K, kvm))
> + return SHADOW_NONPRESENT_VALUE;
> +
KVM_BUG_ON() is very aggressive. We should replace it with WARN_ON_ONCE()
