| Message ID | 1467088360-10186-2-git-send-email-bsd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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. :) -- 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
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 -- 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
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 > -- 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
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 -- 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
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)
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(-)