Re: [kvm-unit-tests PATCH v4 4/8] x86: Improve set_mmu_range() to implement npt

From: Sean Christopherson <seanjc@google.com>
To: "Shukla, Manali" <mashukla@amd.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org
Subject: Re: [kvm-unit-tests PATCH v4 4/8] x86: Improve set_mmu_range() to implement npt
Date: Fri, 24 Jun 2022 00:46:06 +0000	[thread overview]
Message-ID: <YrUJTt1wfMPotyvW@google.com> (raw)
In-Reply-To: <aed29a6c-5e59-d924-f3ed-a3cef91aac79@amd.com>

On Wed, Jun 22, 2022, Shukla, Manali wrote:
> 
> On 6/16/2022 5:34 AM, Sean Christopherson wrote:
> > void __setup_mmu_range(pgd_t *cr3, phys_addr_t start, size_t len, bool nested_mmu)
> > {
> >         u64 orig_opt_mask = pte_opt_mask;
> > 	u64 max = (u64)len + (u64)start;
> > 	u64 phys = start;
> > 
> > 	/* comment goes here. */
> > 	pte_opt_mask |= PT_USER_MASK;
> > 
> >         if (use_hugepages) {
> >                 while (phys + LARGE_PAGE_SIZE <= max) {
> >                         install_large_page(cr3, phys, (void *)(ulong)phys);
> > 		        phys += LARGE_PAGE_SIZE;
> > 	        }
> > 	}
> > 	install_pages(cr3, phys, max - phys, (void *)(ulong)phys);
> > 
> > 	pte_opt_mask = orig_opt_mask;
> > }
> 
> Hi Sean,
> 
> Thank you so much for reviewing my changes.
> 
> RSVD bit test case will start failing with above implementation as we will be
> setting PT_USER_MASK bit for all host PTEs (in order to toggle CR4.SMEP)
> which will defeat one of the purpose of this patch. 

/facepalm

> Right now, pte_opt_mask value which is set from setup_vm(), is overwritten in
> setup_mmu_range() for all the conditions.  How about setting PT_USER_MASK
> only for nested mmu in setup_mmu_range()?  It will retain the same value of
> pte_opt_mask which is set from setup_vm() in all the other cases.

Ya, that should work.

> #define IS_NESTED_MMU 1ULL
> #define USE_HUGEPAGES 2ULL

Use BIT().  And not the ULL single the param is an unsigned long, not a u64.

> void __setup_mmu_range(pgd_t *cr3, phys_addr_t start, size_t len, unsigned long mmu_flags) {

Brace goes on its own line.

>         u64 orig_opt_mask = pte_opt_mask;
>         u64 max = (u64)len + (u64)start;
>         u64 phys = start;
> 
>         /* Allocate 4k pages only for nested page table, PT_USER_MASK needs to

	/*
	 * Multi-line comments look like this.
	 * Line 2.
	 */

>          * be enabled for nested page.
>          */
>         if (mmu_flags & IS_NESTED_MMU)
>                 pte_opt_mask |= PT_USER_MASK;
> 
>         if (mmu_flags & USE_HUGEPAGES) {
>                 while (phys + LARGE_PAGE_SIZE <= max) {
>                         install_large_page(cr3, phys, (void *)(ulong)phys);
>                         phys += LARGE_PAGE_SIZE;
>                 }
>         }
>         install_pages(cr3, phys, max - phys, (void *)(ulong)phys);
> 
>         pte_opt_mask = orig_opt_mask;
> }
> 
> static inline void setup_mmu_range(pgd_t *cr3, phys_addr_t start, size_t len) {
>         __setup_mmu_range(cr3, start, len, USE_HUGEPAGES);
> }
> 
> Thank you,
> Manali