DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 49AB3800A8
Subject: Re: [PATCH] KVM: MMU: speedup update_permission_bitmask
To: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc: jmattson@google.com
References: <1503590171-41030-1-git-send-email-pbonzini@redhat.com>
From: David Hildenbrand <david@redhat.com>
Organization: Red Hat GmbH
Message-ID: <e4b5df86-31ae-2f4e-0666-393753e256df@redhat.com>
Date: Tue, 29 Aug 2017 18:46:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <1503590171-41030-1-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7165
Lines: 203

On 24.08.2017 17:56, Paolo Bonzini wrote:
> update_permission_bitmask currently does a 128-iteration loop to,
> essentially, compute a constant array.  Computing the 8 bits in parallel
> reduces it to 16 iterations, and is enough to speed it up substantially
> because many boolean operations in the inner loop become constants or
> simplify noticeably.
> 
> Because update_permission_bitmask is actually the top item in the profile
> for nested vmexits, this speeds up an L2->L1 vmexit by about ten thousand
> clock cycles, or up to 30%:
> 
>                                          before     after
>    cpuid                                 35173      25954
>    vmcall                                35122      27079
>    inl_from_pmtimer                      52635      42675
>    inl_from_qemu                         53604      44599
>    inl_from_kernel                       38498      30798
>    outl_to_kernel                        34508      28816
>    wr_tsc_adjust_msr                     34185      26818
>    rd_tsc_adjust_msr                     37409      27049
>    mmio-no-eventfd:pci-mem               50563      45276
>    mmio-wildcard-eventfd:pci-mem         34495      30823
>    mmio-datamatch-eventfd:pci-mem        35612      31071
>    portio-no-eventfd:pci-io              44925      40661
>    portio-wildcard-eventfd:pci-io        29708      27269
>    portio-datamatch-eventfd:pci-io       31135      27164
> 
> (I wrote a small C program to compare the tables for all values of CR0.WP,
> CR4.SMAP and CR4.SMEP, and they match).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/mmu.c | 121 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 70 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index f47cccace1a1..2a8a6e3e2a31 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4204,66 +4204,85 @@ static inline bool boot_cpu_is_amd(void)
>  				    boot_cpu_data.x86_phys_bits, execonly);
>  }
>  
> +#define BYTE_MASK(access) \
> +	((1 & (access) ? 2 : 0) | \
> +	 (2 & (access) ? 4 : 0) | \
> +	 (3 & (access) ? 8 : 0) | \
> +	 (4 & (access) ? 16 : 0) | \
> +	 (5 & (access) ? 32 : 0) | \
> +	 (6 & (access) ? 64 : 0) | \
> +	 (7 & (access) ? 128 : 0))
> +

Hmm, I wonder if

#define BYTE_MASK(access) \
	((1 & (access) ? (1 << 1) : 0) | \
	 (2 & (access) ? (1 << 2) : 0) | \
...

would be easier to get

> +
>  static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  				      struct kvm_mmu *mmu, bool ept)
>  {
> -	unsigned bit, byte, pfec;
> -	u8 map;
> -	bool fault, x, w, u, wf, uf, ff, smapf, cr4_smap, cr4_smep, smap = 0;
> +	unsigned byte;
> +
> +	const u8 x = BYTE_MASK(ACC_EXEC_MASK);
> +	const u8 w = BYTE_MASK(ACC_WRITE_MASK);
> +	const u8 u = BYTE_MASK(ACC_USER_MASK);
> +
> +	bool cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP) != 0;
> +	bool cr4_smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP) != 0;
> +	bool cr0_wp = is_write_protection(vcpu);

all three can be turned const. (and I'd drop the empty lines in between ..)

>  
> -	cr4_smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP);
> -	cr4_smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
>  	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
> -		pfec = byte << 1;
> -		map = 0;
> -		wf = pfec & PFERR_WRITE_MASK;
> -		uf = pfec & PFERR_USER_MASK;
> -		ff = pfec & PFERR_FETCH_MASK;
> +		unsigned pfec = byte << 1;

This one can be turned const, too. And I think you could get rid of
"byte" and directly work on "pfec" ... but not sure if that is better.
Most probably not.

> +
>  		/*
> -		 * PFERR_RSVD_MASK bit is set in PFEC if the access is not
> -		 * subject to SMAP restrictions, and cleared otherwise. The
> -		 * bit is only meaningful if the SMAP bit is set in CR4.
> +		 * Each "*f" variable has a 1 bit for each UWX value
> +		 * that causes a fault with the given PFEC.
>  		 */
> -		smapf = !(pfec & PFERR_RSVD_MASK);
> -		for (bit = 0; bit < 8; ++bit) {
> -			x = bit & ACC_EXEC_MASK;
> -			w = bit & ACC_WRITE_MASK;
> -			u = bit & ACC_USER_MASK;
> -
> -			if (!ept) {
> -				/* Not really needed: !nx will cause pte.nx to fault */
> -				x |= !mmu->nx;
> -				/* Allow supervisor writes if !cr0.wp */
> -				w |= !is_write_protection(vcpu) && !uf;
> -				/* Disallow supervisor fetches of user code if cr4.smep */
> -				x &= !(cr4_smep && u && !uf);
> -
> -				/*
> -				 * SMAP:kernel-mode data accesses from user-mode
> -				 * mappings should fault. A fault is considered
> -				 * as a SMAP violation if all of the following
> -				 * conditions are ture:
> -				 *   - X86_CR4_SMAP is set in CR4
> -				 *   - A user page is accessed
> -				 *   - Page fault in kernel mode
> -				 *   - if CPL = 3 or X86_EFLAGS_AC is clear
> -				 *
> -				 *   Here, we cover the first three conditions.
> -				 *   The fourth is computed dynamically in
> -				 *   permission_fault() and is in smapf.
> -				 *
> -				 *   Also, SMAP does not affect instruction
> -				 *   fetches, add the !ff check here to make it
> -				 *   clearer.
> -				 */
> -				smap = cr4_smap && u && !uf && !ff;
> -			}
>  
> -			fault = (ff && !x) || (uf && !u) || (wf && !w) ||
> -				(smapf && smap);
> -			map |= fault << bit;
> +		/* Faults from writes to non-writable pages */
> +		u8 wf = (pfec & PFERR_WRITE_MASK) ? ~w : 0;
> +		/* Faults from user mode accesses to supervisor pages */
> +		u8 uf = (pfec & PFERR_USER_MASK) ? ~u : 0;
> +		/* Faults from fetches of non-executable pages*/
> +		u8 ff = (pfec & PFERR_FETCH_MASK) ? ~x : 0;
> +		/* Faults from kernel mode fetches of user pages */
> +		u8 smepf = 0;
> +		/* Faults from kernel mode accesses of user pages */
> +		u8 smapf = 0;
> +
> +		if (!ept) {
> +			/* Faults from kernel mode accesses to user pages */
> +			u8 kf = (pfec & PFERR_USER_MASK) ? 0 : u;
> +
> +			/* Not really needed: !nx will cause pte.nx to fault */
> +			if (!mmu->nx)
> +				ff = 0;
> +
> +			/* Allow supervisor writes if !cr0.wp */
> +			if (!cr0_wp)
> +				wf = (pfec & PFERR_USER_MASK) ? wf : 0;
> +
> +			/* Disallow supervisor fetches of user code if cr4.smep */
> +			if (cr4_smep)
> +				smepf = (pfec & PFERR_FETCH_MASK) ? kf : 0;
> +
> +			/*
> +			 * SMAP:kernel-mode data accesses from user-mode
> +			 * mappings should fault. A fault is considered
> +			 * as a SMAP violation if all of the following
> +			 * conditions are ture:
> +			 *   - X86_CR4_SMAP is set in CR4
> +			 *   - A user page is accessed
> +			 *   - The access is not a fetch
> +			 *   - Page fault in kernel mode
> +			 *   - if CPL = 3 or X86_EFLAGS_AC is clear
> +			 *
> +			 * Here, we cover the first three conditions.
> +			 * The fourth is computed dynamically in permission_fault();
> +			 * PFERR_RSVD_MASK bit will be set in PFEC if the access is
> +			 * *not* subject to SMAP restrictions.
> +			 */
> +			if (cr4_smap)
> +				smapf = (pfec & (PFERR_RSVD_MASK|PFERR_FETCH_MASK)) ? 0 : kf;
>  		}
> -		mmu->permissions[byte] = map;
> +
> +		mmu->permissions[byte] = ff | uf | wf | smepf | smapf;
>  	}
>  }
>  
> 

My brain hurts, but I am pretty confident that this is correct. :)

-- 

Thanks,

David