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

[Tim Deegan <tim@xxxxxxx>]

At 16:50 +0000 on 16 Dec (1450284615), George Dunlap wrote:
> 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.  Looking at the SDM (3A.4.7) the
> > current rules are for that bit to be set iff ((PAE && NXE) || SMEP).
> 
> Right, but I think your answer points to the fundamental problem with
> the way this code has been written.  The pfec value passed to
> gva_to_gfn() is used for two separate functions: 1) to tell
> guest_walk_tables() what kind of access is happening, so that it can
> check the appropriate bits 2) to be used as a pfec value to pass back to
> the guest when delivering a page fault.

Yes.  At the time, this made sense: the caller and the walker were
collectively assembling the PFEC, with the caller providing bits that
depended on the kind of access and the walker managing the bits that
depended on the pagetables.  We could have had a separate flags
parameter to describe (read/write/fetch X user/supervisor), but that
would by construction have been identical to the inital set of PFEC bits.

Now that we have a potential case where the hardware behaves
differently for instruction fetches but _doesn't_ flag them in the
PFEC, then we clearly need to track them separately.

> Really those should be two separate parameters, one in and one out; and
> it should be gva_to_gfn()'s job to translate the missing bits from
> guest_walk_tables() into a pfec value suitable to use when injecting a
> fault.  (Or potentially guest_walk_tables()'s job.)

Agreed.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel