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: <d7f60ee5e2bdd72e8b1fbcabb753170167674eee.camel@intel.com>
Subject: Re: [PATCH v7 036/102] KVM: x86/mmu: Allow non-zero value for
 non-present SPTE
From:   Kai Huang <kai.huang@intel.com>
To:     Isaku Yamahata <isaku.yamahata@gmail.com>, isaku.yamahata@intel.com
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Yuan Yao <yuan.yao@linux.intel.com>
Date:   Wed, 20 Jul 2022 15:12:16 +1200
In-Reply-To: <20220714184111.GT1379820@ls.amr.corp.intel.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
         <f74b05eca8815744ce1ad672c66033101be7369c.1656366338.git.isaku.yamahata@intel.com>
         <20220714184111.GT1379820@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.3 (3.44.3-1.fc36) 
MIME-Version: 1.0
Precedence: bulk


> --- a/arch/x86/kvm/mmu/spte.c
> +++ b/arch/x86/kvm/mmu/spte.c
> @@ -36,6 +36,9 @@ u64 __read_mostly shadow_present_mask;
>  u64 __read_mostly shadow_me_value;
>  u64 __read_mostly shadow_me_mask;
>  u64 __read_mostly shadow_acc_track_mask;
> +#ifdef CONFIG_X86_64
> +u64 __read_mostly shadow_nonpresent_value;
> +#endif

Is this ever used?

> =20
>  u64 __read_mostly shadow_nonpresent_or_rsvd_mask;
>  u64 __read_mostly shadow_nonpresent_or_rsvd_lower_gfn_mask;
> @@ -360,7 +363,7 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 m=
mio_mask, u64 access_mask)
>  	 * not set any RWX bits.
>  	 */
>  	if (WARN_ON((mmio_value & mmio_mask) !=3D mmio_value) ||
> -	    WARN_ON(mmio_value && (REMOVED_SPTE & mmio_mask) =3D=3D mmio_value)=
)
> +	    WARN_ON(mmio_value && (__REMOVED_SPTE & mmio_mask) =3D=3D mmio_valu=
e))
>  		mmio_value =3D 0;

This chunk doesn't look right, or necessary.  We need mmio_mask/mmio_value =
which
causes EPT violation but with "suppress #VE" bit clear. =20

So, actually, we want to make sure SHADOW_NONPRESENT_VALUE is *NOT* in mmio=
_mask
and mmio_value.  Using (REMOVED_SPTE & mmio_mask) =3D=3D mmio_value can act=
ually
ensure SHADOW_NONPRESENT_VALUE is never set in MMIO spte, correct?  So I th=
ink
using REMOVED_SPTE is fine.

Or maybe additionally adding a explicit check is even better:

	if (WARN_ON(mmio_mask & SHADOW_NONPRESENT_VALUE))
		mmio_value =3D 0;

But this change maybe should be in another patch which deals setting up per=
-VM
mmio_mask/mmio_value anyway.  This patch, instead, focuses on allowing non-=
zero
value for non-present SPTE.