Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1719743 > unrolled thread

linux-next: manual merge of the kvm tree with the tip tree

Started byStephen Rothwell <sfr@canb.auug.org.au>
First post2017-08-25 06:40 +0200
Last post2017-08-26 09:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  linux-next: manual merge of the kvm tree with the tip tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-08-25 06:40 +0200
    Re: linux-next: manual merge of the kvm tree with the tip tree Paolo Bonzini <pbonzini@redhat.com> - 2017-08-25 08:40 +0200
      Re: linux-next: manual merge of the kvm tree with the tip tree Paolo Bonzini <pbonzini@redhat.com> - 2017-08-25 22:10 +0200
        Re: linux-next: manual merge of the kvm tree with the tip tree Paolo Bonzini <pbonzini@redhat.com> - 2017-08-25 22:50 +0200
          Re: linux-next: manual merge of the kvm tree with the tip tree Ingo Molnar <mingo@kernel.org> - 2017-08-26 09:30 +0200

#1719743 — linux-next: manual merge of the kvm tree with the tip tree

FromStephen Rothwell <sfr@canb.auug.org.au>
Date2017-08-25 06:40 +0200
Subjectlinux-next: manual merge of the kvm tree with the tip tree
Message-ID<uiezw-7NM-17@gated-at.bofh.it>
Hi all,

Today's linux-next merge of the kvm tree got a conflict in:

  arch/x86/kvm/mmu.h

between commit:

  d0ec49d4de90 ("kvm/x86/svm: Support Secure Memory Encryption within KVM")

from the tip tree and commit:

  d1cd3ce90044 ("KVM: MMU: check guest CR3 reserved bits based on its physical address width.")

from the kvm tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc arch/x86/kvm/mmu.h
index 3cc725590ab9,e2999f57bfc4..000000000000
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@@ -48,7 -49,10 +49,10 @@@
  
  static inline u64 rsvd_bits(int s, int e)
  {
+ 	if (e < s)
+ 		return 0;
+ 
 -	return ((1ULL << (e - s + 1)) - 1) << s;
 +	return __sme_clr(((1ULL << (e - s + 1)) - 1) << s);
  }
  
  void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask, u64 mmio_value);

[toc] | [next] | [standalone]

#1719773

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-25 08:40 +0200
Message-ID<uigrD-xp-9@gated-at.bofh.it>
In reply to#1719743
On 25/08/2017 06:39, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the kvm tree got a conflict in:
> 
>   arch/x86/kvm/mmu.h
> 
> between commit:
> 
>   d0ec49d4de90 ("kvm/x86/svm: Support Secure Memory Encryption within KVM")
> 
> from the tip tree and commit:
> 
>   d1cd3ce90044 ("KVM: MMU: check guest CR3 reserved bits based on its physical address width.")
> 
> from the kvm tree.
> 
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging.  You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
> 

Thomas L., Ingo,

this is completely wrong:

> 
>  static inline u64 rsvd_bits(int s, int e)
>  {
> -	return ((1ULL << (e - s + 1)) - 1) << s;
> +	return __sme_clr(((1ULL << (e - s + 1)) - 1) << s);
>  }
>  

First, rsvd_bits is just a simple function to return some 1 bits.  Applying
a mask based on properties of the host MMU is incorrect.

Second, the masks computed by __reset_rsvds_bits_mask also apply to 
guest page tables, where the C bit is reserved since we don't emulate
SME.

Something like this:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2dafd36368cc..e0597d703d72 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4142,16 +4142,24 @@ void
 reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, struct kvm_mmu *context)
 {
 	bool uses_nx = context->nx || context->base_role.smep_andnot_wp;
+	struct rsvd_bits_validate *shadow_zero_check;
+	int i;
 
 	/*
 	 * Passing "true" to the last argument is okay; it adds a check
 	 * on bit 8 of the SPTEs which KVM doesn't use anyway.
 	 */
-	__reset_rsvds_bits_mask(vcpu, &context->shadow_zero_check,
+        shadow_zero_check = &context->shadow_zero_check;
+	__reset_rsvds_bits_mask(vcpu, shadow_zero_check,
 				boot_cpu_data.x86_phys_bits,
 				context->shadow_root_level, uses_nx,
 				guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES),
 				is_pse(vcpu), true);
+
+	for (i = context->shadow_root_level; --i >= 0; ) {
+		shadow_zero_check->rsvd_bits_mask[i][0] &= ~shadow_me_mask;
+		shadow_zero_check->rsvd_bits_mask[i][1] &= ~shadow_me_mask;
+	}
 }
 EXPORT_SYMBOL_GPL(reset_shadow_zero_bits_mask);
 

Can you please fix it up?   Please Cc me at paolo.bonzini@gmail.com too 
because I'll be on vacation next week.

(And thanks Stephen for the heads-up!)

Paolo

[toc] | [prev] | [next] | [standalone]

#1720388

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-25 22:10 +0200
Message-ID<uit5w-7Z-21@gated-at.bofh.it>
In reply to#1719773
On 25/08/2017 18:53, Brijesh Singh wrote:
>>
> 
> Thanks for the tip, I have expanded the patch to cover tdp cases and
> have verified
> that it works fine with SME enabled KVM. If you are okay with this then
> I can
> send patch.
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index ccb70b8..7a8edc0 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4109,16 +4109,30 @@ void
>  reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, struct kvm_mmu
> *context)
>  {
>         bool uses_nx = context->nx || context->base_role.smep_andnot_wp;
> +       struct rsvd_bits_validate *shadow_zero_check;
> +       int i;
>  
>         /*
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index ccb70b8..7a8edc0 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4109,16 +4109,30 @@ void
>  reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu, struct kvm_mmu
> *context)
>  {
>         bool uses_nx = context->nx || context->base_role.smep_andnot_wp;
> +       struct rsvd_bits_validate *shadow_zero_check;
> +       int i;
>  
>         /*
>          * Passing "true" to the last argument is okay; it adds a check
>          * on bit 8 of the SPTEs which KVM doesn't use anyway.
>          */
> -       __reset_rsvds_bits_mask(vcpu, &context->shadow_zero_check,
> +       shadow_zero_check = &context->shadow_zero_check;
> +       __reset_rsvds_bits_mask(vcpu, shadow_zero_check,
>                                 boot_cpu_data.x86_phys_bits,
>                                 context->shadow_root_level, uses_nx,
>                                 guest_cpuid_has_gbpages(vcpu),
> is_pse(vcpu),
>                                 true);
> +
> +       if (!shadow_me_mask)
> +               return;
> +
> +       for (i = context->shadow_root_level; --i >= 0;) {
> +               shadow_zero_check->rsvd_bits_mask[i][0] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][1] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][2] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][3] &= ~shadow_me_mask;

Neither my version nor yours is correct. :)  The right one has [0][i]
and [1][i] (I inverted the indices by mistake).

With that change, you can include my

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

> +       }
> +
>  }
>  EXPORT_SYMBOL_GPL(reset_shadow_zero_bits_mask);
>  
> @@ -4136,8 +4150,13 @@ static void
>  reset_tdp_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
>                                 struct kvm_mmu *context)
>  {
> +       struct rsvd_bits_validate *shadow_zero_check;
> +       int i;
> +
> +       shadow_zero_check = &context->shadow_zero_check;
> +
>         if (boot_cpu_is_amd())
> -               __reset_rsvds_bits_mask(vcpu, &context->shadow_zero_check,
> +               __reset_rsvds_bits_mask(vcpu, shadow_zero_check,
>                                         boot_cpu_data.x86_phys_bits,
>                                         context->shadow_root_level, false,
>                                         boot_cpu_has(X86_FEATURE_GBPAGES),

Please use shadow_zero_check here too:

                __reset_rsvds_bits_mask_ept(&context->shadow_zero_check,

Thanks,

Paolo

> @@ -4147,6 +4166,15 @@ reset_tdp_shadow_zero_bits_mask(struct kvm_vcpu
> *vcpu,
>                                             boot_cpu_data.x86_phys_bits,
>                                             false);
>  
> +       if (!shadow_me_mask)
> +               return;
> +
> +       for (i = context->shadow_root_level; --i >= 0;) {
> +               shadow_zero_check->rsvd_bits_mask[i][0] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][1] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][2] &= ~shadow_me_mask;
> +               shadow_zero_check->rsvd_bits_mask[i][3] &= ~shadow_me_mask;
> +       }
>  }
>  
>  /*
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 3cc7255..d7d248a 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -48,7 +48,7 @@
>  
>  static inline u64 rsvd_bits(int s, int e)
>  {
> -       return __sme_clr(((1ULL << (e - s + 1)) - 1) << s);
> +       return ((1ULL << (e - s + 1)) - 1) << s;
>  }
>  
>  void kvm_mmu_set_mmio_spte_mask(u64 mmio_mask, u64 mmio_value);
> 
>

[toc] | [prev] | [next] | [standalone]

#1720404

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-25 22:50 +0200
Message-ID<uitId-kA-3@gated-at.bofh.it>
In reply to#1720388
On 25/08/2017 22:41, Brijesh Singh wrote:
>>>>
> 
>> Neither my version nor yours is correct. :)  The right one has [0][i]
>> and [1][i] (I inverted the indices by mistake).
>>
>> With that change, you can include my
>>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>
> 
> Ingo,
> 
> I assuming that this patch should be sent through the tip since SME support
> came from tip. I will be submitting the patch very soon.

Yes, that is correct.  I cannot apply it directly to the KVM tree.

Paolo

[toc] | [prev] | [next] | [standalone]

#1720561

FromIngo Molnar <mingo@kernel.org>
Date2017-08-26 09:30 +0200
Message-ID<uiDHz-6Hs-21@gated-at.bofh.it>
In reply to#1720404
* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 25/08/2017 22:41, Brijesh Singh wrote:
> >>>>
> > 
> >> Neither my version nor yours is correct. :)  The right one has [0][i]
> >> and [1][i] (I inverted the indices by mistake).
> >>
> >> With that change, you can include my
> >>
> >> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> >>
> > 
> > Ingo,
> > 
> > I assuming that this patch should be sent through the tip since SME support
> > came from tip. I will be submitting the patch very soon.
> 
> Yes, that is correct.  I cannot apply it directly to the KVM tree.

I've merged it to tip:x86/mm where the SME bits live and will propagate it to 
linux-next ASAP, once it's gone through my local testing.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web