linux-kernel - Re: [PATCH v4 10/12] KVM: x86/mmu: fine-grained gfn zap when guest MTRRs are honored

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZOk1n3ssdU7bXalP@google.com>
Date:   Fri, 25 Aug 2023 16:13:35 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yan Zhao <yan.y.zhao@...el.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, chao.gao@...el.com, kai.huang@...el.com,
        robert.hoo.linux@...il.com, yuan.yao@...ux.intel.com
Subject: Re: [PATCH v4 10/12] KVM: x86/mmu: fine-grained gfn zap when guest
 MTRRs are honored

On Fri, Jul 14, 2023, Yan Zhao wrote:
>  void kvm_honors_guest_mtrrs_zap_on_cd_toggle(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_mtrr_zap_gfn_range(vcpu, gpa_to_gfn(0), gpa_to_gfn(~0ULL));
> +	struct kvm_mtrr *mtrr_state = &vcpu->arch.mtrr_state;
> +	bool mtrr_enabled = mtrr_is_enabled(mtrr_state);
> +	u8 default_mtrr_type;
> +	bool cd_ipat;
> +	u8 cd_type;
> +
> +	kvm_honors_guest_mtrrs_get_cd_memtype(vcpu, &cd_type, &cd_ipat);
> +
> +	default_mtrr_type = mtrr_enabled ? mtrr_default_type(mtrr_state) :
> +			    mtrr_disabled_type(vcpu);
> +
> +	if (cd_type != default_mtrr_type || cd_ipat)
> +		return kvm_mtrr_zap_gfn_range(vcpu, gpa_to_gfn(0), gpa_to_gfn(~0ULL));

Why does this use use the MTRR version but the failure path does not?  Ah, because
trying to allocate in the failure path will likely fail to allocate memory.  With
a statically sized array, we can just special case the 0 => -1 case.  Actually,
we can do that regardless, it just doesn't need a dedicated flag if we use an
array.

Using the MTRR version on failure (array is full) means that other vCPUs can see
that everything is being zapped and go straight to waitin.

> +
> +	/*
> +	 * If mtrr is not enabled, it will go to zap all above if the default

Pronouns again.  Maybe this?

	/*
	 * The default MTRR type has already been checked above, if MTRRs are
	 * disabled there are no other MTRR types to consider.
	 */

> +	 * type does not equal to cd_type;
> +	 * Or it has no need to zap if the default type equals to cd_type.
> +	 */
> +	if (mtrr_enabled) {

To save some indentation:

	if (!mtrr_enabled)
		return;


> +		if (prepare_zaplist_fixed_mtrr_of_non_type(vcpu, default_mtrr_type))
> +			goto fail;
> +
> +		if (prepare_zaplist_var_mtrr_of_non_type(vcpu, default_mtrr_type))
> +			goto fail;
> +
> +		kvm_zap_or_wait_mtrr_zap_list(vcpu->kvm);
> +	}
> +	return;
> +fail:
> +	kvm_clear_mtrr_zap_list(vcpu->kvm);
> +	/* resort to zapping all on failure*/
> +	kvm_zap_gfn_range(vcpu->kvm, gpa_to_gfn(0), gpa_to_gfn(~0ULL));
> +	return;
>  }
> -- 
> 2.17.1
>