Re: [PATCH 6/9] KVM, pkeys: add pkeys support for permission_fault logic

["Han, Huaitong" <huaitong.han@intel.com>]

On Mon, 2015-11-09 at 14:17 +0100, Paolo Bonzini wrote:

> > >  static inline bool permission_fault(struct kvm_vcpu *vcpu,
> > > struct kvm_mmu *mmu,
> > > -				    unsigned pte_access,
> > > unsigned pfec)
> > > +		unsigned pte_access, unsigned pte_pkeys,
> > > unsigned pfec)
> > >  {
> > > -	int cpl = kvm_x86_ops->get_cpl(vcpu);
> > > -	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> > > +	unsigned long smap, rflags;
> > > +	u32 pkru;
> > > +	int cpl, index;
> > > +	bool wf, uf, pk, pkru_ad, pkru_wd;
> > > +
> > > +	cpl = kvm_x86_ops->get_cpl(vcpu);
> > > +	rflags = kvm_x86_ops->get_rflags(vcpu);
> > > +
> > > +	pkru = read_pkru();
> > > +
> > > +	/*
> > > +	* PKRU defines 32 bits, there are 16 domains and 2
> > > attribute bits per
> > > +	* domain in pkru, pkey is index to a defined domain, so
> > > the value of
> > > +	* pkey * PKRU_ATTRS + R/W is offset of a defined domain
> > > attribute.
> > > +	*/
> > > +	pkru_ad = (pkru >> (pte_pkeys * PKRU_ATTRS + PKRU_READ))
> > > & 1;
> > > +	pkru_wd = (pkru >> (pte_pkeys * PKRU_ATTRS +
> > > PKRU_WRITE)) & 1;
> > > +
> > > +	wf = pfec & PFERR_WRITE_MASK;
> > > +	uf = pfec & PFERR_USER_MASK;
> > > +
> > > +	/*
> > > +	* PKeys 2nd and 6th conditions:
> > > +	* 2.EFER_LMA=1
> > > +	* 6.PKRU.AD=1
> > > +	*	or The access is a data write and PKRU.WD=1 and
> > > +	*		either CR0.WP=1 or it is a user mode
> > > access
> > > +	*/
> > > +	pk = is_long_mode(vcpu) && (pkru_ad ||
> > > +			(pkru_wd && wf &&
> > > (is_write_protection(vcpu) || uf)));
> 
> A little more optimized:
> 
> 	pkru_bits = (pkru >> (pte_pkeys * PKRU_ATTRS)) & 3;
> 
> 	/*
> 	 * Ignore PKRU.WD if not relevant to this access (a read,
> 	 * or a supervisor mode access if CR0.WP=0).
> 	 */
> 	if (!wf || (!uf && !is_write_protection(vcpu)))
> 		pkru_bits &= ~(1 << PKRU_WRITE);
> 
> ... and then just check pkru_bits != 0.
> 
> > > +	/*
> > > +	* PK bit right value in pfec equal to
> > > +	* PK bit current value in pfec and pk value.
> > > +	*/
> > > +	pfec &= (pk << PFERR_PK_BIT) + ~PFERR_PK_MASK;
> > 
> > PK is only applicable to guest page tables, but if you do not
> > support
> > PKRU without EPT (patch 9), none of this is necessary, is it?
> 
> Doh. :(  Sorry, this is of course needed for the emulation case.
> 
> I think you should optimize this for the common case where pkru is
> zero,
> hence pk will always be zero.  So something like
> 
> 	pkru = is_long_mode(vcpu) ? read_pkru() : 0;
> 	if (unlikely(pkru) && (pfec & PFERR_PK_MASK)) {
> 		... from above ... */
> 
> 		/* Flip PFERR_PK_MASK if pkru_bits is non-zero */
> 		pfec ^= -pkru_bits & PFERR_PK_MASK;

If pkru_bits is zero, it means dynamically conditions is not met for
protection-key violations, so pfec on PK bit should be flipped. So I
guess it should be:
	pfec ^= pkru_bits ? 0 : PFERR_PK_MASK;

> 	}
> 
> Paolo
>