Re: [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Tim Deegan <tim@xen.org>, George Dunlap <george.dunlap@citrix.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	"Wu, Feng" <feng.wu@intel.com>,
	"Nakajima, Jun" <jun.nakajima@intel.com>,
	"stefano.stabellini@eu.citrix.com"
	<stefano.stabellini@eu.citrix.com>,
	"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
	"ian.jackson@eu.citrix.com" <ian.jackson@eu.citrix.com>,
	"Dong, Eddie" <eddie.dong@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	"jbeulich@suse.com" <jbeulich@suse.com>,
	"Han, Huaitong" <huaitong.han@intel.com>,
	"wei.liu2@citrix.com" <wei.liu2@citrix.com>,
	"keir@xen.org" <keir@xen.org>,
	"ian.campbell@citrix.com" <ian.campbell@citrix.com>
Subject: Re: [V3 PATCH 7/9] x86/hvm: pkeys, add pkeys support for guest_walk_tables
Date: Wed, 16 Dec 2015 16:34:59 +0000	[thread overview]
Message-ID: <567192B3.6060206@citrix.com> (raw)
In-Reply-To: <20151216162827.GA65025@deinos.phlegethon.org>

On 16/12/15 16:28, Tim Deegan wrote:
> Hi,
>
> At 15:36 +0000 on 16 Dec (1450280191), George Dunlap wrote:
>> (hvm_fetch_from_guest_virt() seems to only set PFEC_insn_fetch if nx or
>> smep are enabled in the guest.  This seems inconsistent to me with the
>> treatment of PFEC_reserved_bit: it seems like
>> hvm_fetch_from_guest_virt() should always pass in PFEC_insn_fetch,
>> particularly as guest_walk_tables() will already gate the checks based
>> on whether nx or smep is enabled in the guest.  Tim, you know of any
>> reason for this?)
> This code is following the hardware, IIRC: on real CPUs without NX
> enabled, pagefault error codes will not have that bit set even when
> caused by instruction fetches.

There is a difference between Intel and AMD.  Intel has the bit reserved
(and therefore faults), while AMD declares the bit as ignored (and
doesn't fault).

These are yet more subtle differences which are not quite accounted for
correctly in Xen (which I am frankly unlikely to even get to in cpuid v2).

~Andrew

>   Looking at the SDM (3A.4.7) the
> current rules are for that bit to be set iff ((PAE && NXE) || SMEP).
>
> It seems unfortunate to have this check duplicated between
> guest_walk_tables() and its callers, so potentially g_w_t() should
> explicitly _clear_ PFEC_insn_fetch in cases where it should not be
> returned, and then its callers can set it unconditionally for
> all instruction fetches.
>
> (There are similar rules for the new PK bit, which should not be shown
> to the guest if CR4.PKE is clear.)
>
> Cheers,
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel