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

Groups > linux.kernel > #1432449 > unrolled thread

[PATCH 1/5] mmu: mark spte present if the x bit is set

Started byBandan Das <bsd@redhat.com>
First post2016-06-28 06:40 +0200
Last post2016-07-05 13:40 +0200
Articles 11 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] mmu: mark spte present if the x bit is set Bandan Das <bsd@redhat.com> - 2016-06-28 06:40 +0200
    Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Paolo Bonzini <pbonzini@redhat.com> - 2016-06-28 10:50 +0200
      Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Bandan Das <bsd@redhat.com> - 2016-06-28 19:40 +0200
        Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Paolo Bonzini <pbonzini@redhat.com> - 2016-06-28 22:20 +0200
          Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Bandan Das <bsd@redhat.com> - 2016-06-28 22:40 +0200
            Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Paolo Bonzini <pbonzini@redhat.com> - 2016-06-28 22:50 +0200
              Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Bandan Das <bsd@redhat.com> - 2016-06-28 23:10 +0200
              Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-06-29 05:10 +0200
              Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Wanpeng Li <kernellwp@gmail.com> - 2016-07-05 05:10 +0200
                Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Paolo Bonzini <pbonzini@redhat.com> - 2016-07-05 13:00 +0200
                  Re: [PATCH 1/5] mmu: mark spte present if the x bit is set Wanpeng Li <kernellwp@gmail.com> - 2016-07-05 13:40 +0200

#1432449 — [PATCH 1/5] mmu: mark spte present if the x bit is set

FromBandan Das <bsd@redhat.com>
Date2016-06-28 06:40 +0200
Subject[PATCH 1/5] mmu: mark spte present if the x bit is set
Message-ID<rOSYx-1Xl-5@gated-at.bofh.it>
This is safe because is_shadow_present_pte() is called
on host controlled page table and we know the spte is
valid

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index def97b3..a50af79 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
 
 static int is_shadow_present_pte(u64 pte)
 {
-	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
+	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
+		!is_mmio_spte(pte);
 }
 
 static int is_large_pte(u64 pte)
-- 
2.5.5

[toc] | [next] | [standalone]

#1432646

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-28 10:50 +0200
Message-ID<rOWSt-4vW-9@gated-at.bofh.it>
In reply to#1432449

On 28/06/2016 06:32, Bandan Das wrote:
> This is safe because is_shadow_present_pte() is called
> on host controlled page table and we know the spte is
> valid
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index def97b3..a50af79 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
>  
>  static int is_shadow_present_pte(u64 pte)
>  {
> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
> +		!is_mmio_spte(pte);

This should really be pte & 7 when using EPT.  But this is okay as an
alternative to a new shadow_present_mask.

Paolo

>  }
>  
>  static int is_large_pte(u64 pte)
>

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

#1433174

FromBandan Das <bsd@redhat.com>
Date2016-06-28 19:40 +0200
Message-ID<rP59o-1Ba-15@gated-at.bofh.it>
In reply to#1432646
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 06:32, Bandan Das wrote:
>> This is safe because is_shadow_present_pte() is called
>> on host controlled page table and we know the spte is
>> valid
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/mmu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index def97b3..a50af79 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -304,7 +304,8 @@ static int is_nx(struct kvm_vcpu *vcpu)
>>  
>>  static int is_shadow_present_pte(u64 pte)
>>  {
>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>> +		!is_mmio_spte(pte);
>
> This should really be pte & 7 when using EPT.  But this is okay as an
> alternative to a new shadow_present_mask.

I could revive shadow_xonly_valid probably... Anyway, for now I will
add a TODO comment here.

> Paolo
>
>>  }
>>  
>>  static int is_large_pte(u64 pte)
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1433273

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-28 22:20 +0200
Message-ID<rP7Ee-3d8-37@gated-at.bofh.it>
In reply to#1433174

On 28/06/2016 19:33, Bandan Das wrote:
>>> >>  static int is_shadow_present_pte(u64 pte)
>>> >>  {
>>> >> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>> >> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>> >> +		!is_mmio_spte(pte);
>> >
>> > This should really be pte & 7 when using EPT.  But this is okay as an
>> > alternative to a new shadow_present_mask.
> I could revive shadow_xonly_valid probably... Anyway, for now I will
> add a TODO comment here.

It's okay to it like this, because the only invalid PTEs reaching this
point are those that is_mmio_spte filters away.  Hence you'll never get
-W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
pretty clever, and doesn't need a TODO at all. :)

Paolo

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

#1433285

FromBandan Das <bsd@redhat.com>
Date2016-06-28 22:40 +0200
Message-ID<rP7Xz-3k5-23@gated-at.bofh.it>
In reply to#1433273
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 19:33, Bandan Das wrote:
>>>> >>  static int is_shadow_present_pte(u64 pte)
>>>> >>  {
>>>> >> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>> >> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>> >> +		!is_mmio_spte(pte);
>>> >
>>> > This should really be pte & 7 when using EPT.  But this is okay as an
>>> > alternative to a new shadow_present_mask.
>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>> add a TODO comment here.
>
> It's okay to it like this, because the only invalid PTEs reaching this
> point are those that is_mmio_spte filters away.  Hence you'll never get
> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
> pretty clever, and doesn't need a TODO at all. :)

Thanks, understood. So, the way it is written now covers all cases for
pte & 7. Let's still add a comment - clever things are usually
confusing to many!

> Paolo

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

#1433290

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-06-28 22:50 +0200
Message-ID<rP87f-3nq-13@gated-at.bofh.it>
In reply to#1433285

On 28/06/2016 22:37, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>  static int is_shadow_present_pte(u64 pte)
>>>>>>>  {
>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>> +		!is_mmio_spte(pte);
>>>>>
>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>> alternative to a new shadow_present_mask.
>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>> add a TODO comment here.
>>
>> It's okay to it like this, because the only invalid PTEs reaching this
>> point are those that is_mmio_spte filters away.  Hence you'll never get
>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>> pretty clever, and doesn't need a TODO at all. :)
> 
> Thanks, understood. So, the way it is written now covers all cases for
> pte & 7. Let's still add a comment - clever things are usually
> confusing to many!

I think another way to write it is "(pte & 0xFFFFFFFFull) &&
!is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
update the PTEs atomically).  Guangrong, what do you prefer?

Paolo

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

#1433320

FromBandan Das <bsd@redhat.com>
Date2016-06-28 23:10 +0200
Message-ID<rP8qC-3Jw-29@gated-at.bofh.it>
In reply to#1433290
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2016 22:37, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>>  static int is_shadow_present_pte(u64 pte)
>>>>>>>>  {
>>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>>> +		!is_mmio_spte(pte);
>>>>>>
>>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>>> alternative to a new shadow_present_mask.
>>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>>> add a TODO comment here.
>>>
>>> It's okay to it like this, because the only invalid PTEs reaching this
>>> point are those that is_mmio_spte filters away.  Hence you'll never get
>>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>>> pretty clever, and doesn't need a TODO at all. :)
>> 
>> Thanks, understood. So, the way it is written now covers all cases for
>> pte & 7. Let's still add a comment - clever things are usually
>> confusing to many!
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
> update the PTEs atomically).  Guangrong, what do you prefer?

Actually, I like this one better although until now, I was not sure if it's
a safe assumption for non-ept cases. From your description, it looks like
it is.

> Paolo

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

#1433414

FromXiao Guangrong <guangrong.xiao@linux.intel.com>
Date2016-06-29 05:10 +0200
Message-ID<rPe2Z-7cs-3@gated-at.bofh.it>
In reply to#1433290

On 06/29/2016 04:49 AM, Paolo Bonzini wrote:
>
>
> On 28/06/2016 22:37, Bandan Das wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 28/06/2016 19:33, Bandan Das wrote:
>>>>>>>>   static int is_shadow_present_pte(u64 pte)
>>>>>>>>   {
>>>>>>>> -	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
>>>>>>>> +	return pte & (PT_PRESENT_MASK | shadow_x_mask) &&
>>>>>>>> +		!is_mmio_spte(pte);
>>>>>>
>>>>>> This should really be pte & 7 when using EPT.  But this is okay as an
>>>>>> alternative to a new shadow_present_mask.
>>>> I could revive shadow_xonly_valid probably... Anyway, for now I will
>>>> add a TODO comment here.
>>>
>>> It's okay to it like this, because the only invalid PTEs reaching this
>>> point are those that is_mmio_spte filters away.  Hence you'll never get
>>> -W- PTEs here, and pte & 7 is really the same as how you wrote it.  It's
>>> pretty clever, and doesn't need a TODO at all. :)
>>
>> Thanks, understood. So, the way it is written now covers all cases for
>> pte & 7. Let's still add a comment - clever things are usually
>> confusing to many!
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 1..31 (they can have non-zero bits 32..63 on 32-bit CPUs where we don't
> update the PTEs atomically).  Guangrong, what do you prefer?

I think the way you innovated is better. :)

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

#1436614

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-05 05:10 +0200
Message-ID<rRoUh-5HL-3@gated-at.bofh.it>
In reply to#1433290
2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
[...]
>
> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits

I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?

Regards,
Wanpeng Li

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

#1436920

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-05 13:00 +0200
Message-ID<rRwf8-1IO-9@gated-at.bofh.it>
In reply to#1436614

On 05/07/2016 05:06, Wanpeng Li wrote:
> 2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> [...]
>>
>> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
>> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
> 
> I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?

-W- EPT SPTEs are present but invalid.  They should never happen unless
they are MMIO SPTEs (in which case !is_mmio_spte(pte) will return true
and the function will return false).

Paolo

> Regards,
> Wanpeng Li
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

#1436938

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-05 13:40 +0200
Message-ID<rRwRQ-2bm-3@gated-at.bofh.it>
In reply to#1436920
2016-07-05 18:50 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 05/07/2016 05:06, Wanpeng Li wrote:
>> 2016-06-29 4:49 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>> [...]
>>>
>>> I think another way to write it is "(pte & 0xFFFFFFFFull) &&
>>> !is_mmio_spte(pte)", since non-present/non-MMIO SPTEs never use bits
>>
>> I misunderstand it here, this will also treat -W- EPT SPTEs as present, right?
>
> -W- EPT SPTEs are present but invalid.  They should never happen unless
> they are MMIO SPTEs (in which case !is_mmio_spte(pte) will return true
> and the function will return false).

Thanks for the explanation. :)

Regards,
Wanpeng Li

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web