Consider using small page to fit guest's large page mapping:
If the mapping is writable but the dirty flag is not set, we will find
the read-only direct sp and setup the mapping, then if the write #PF
occur, we will mark this mapping writable in the read-only direct sp,
now, other real read-only mapping will happily write it without #PF.
It may hurt guest's COW
Fixed by re-install the mapping when write #PF occur.
Signed-off-by: Xiao Guangrong <[email protected]>
---
arch/x86/kvm/mmu.c | 3 ++-
arch/x86/kvm/paging_tmpl.h | 18 ++++++++++++++++++
2 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 556a798..0412ba4 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -153,7 +153,8 @@ module_param(oos_shadow, bool, 0644);
#define CREATE_TRACE_POINTS
#include "mmutrace.h"
-#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_NO_DIRTY (2ULL << PT_FIRST_AVAIL_BITS_SHIFT)
#define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index e46eb8a..fdba751 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -325,6 +325,20 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
break;
}
+ if (*sptep & SPTE_NO_DIRTY) {
+ struct kvm_mmu_page *child;
+
+ WARN_ON(level != gw->level);
+ WARN_ON(!is_shadow_present_pte(*sptep));
+ if (dirty) {
+ child = page_header(*sptep &
+ PT64_BASE_ADDR_MASK);
+ mmu_page_remove_parent_pte(child, sptep);
+ __set_spte(sptep, shadow_trap_nonpresent_pte);
+ kvm_flush_remote_tlbs(vcpu->kvm);
+ }
+ }
+
if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
continue;
@@ -365,6 +379,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
}
}
+ if (level == gw->level && !dirty &&
+ access & gw->pte_access & ACC_WRITE_MASK)
+ spte |= SPTE_NO_DIRTY;
+
spte = __pa(sp->spt)
| PT_PRESENT_MASK | PT_ACCESSED_MASK
| PT_WRITABLE_MASK | PT_USER_MASK;
--
1.6.1.2
On 06/25/2010 03:06 PM, Xiao Guangrong wrote:
> Consider using small page to fit guest's large page mapping:
>
> If the mapping is writable but the dirty flag is not set, we will find
> the read-only direct sp and setup the mapping, then if the write #PF
> occur, we will mark this mapping writable in the read-only direct sp,
> now, other real read-only mapping will happily write it without #PF.
>
> It may hurt guest's COW
>
> Fixed by re-install the mapping when write #PF occur.
>
> Signed-off-by: Xiao Guangrong<[email protected]>
> ---
> arch/x86/kvm/mmu.c | 3 ++-
> arch/x86/kvm/paging_tmpl.h | 18 ++++++++++++++++++
> 2 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 556a798..0412ba4 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -153,7 +153,8 @@ module_param(oos_shadow, bool, 0644);
> #define CREATE_TRACE_POINTS
> #include "mmutrace.h"
>
> -#define SPTE_HOST_WRITEABLE (1ULL<< PT_FIRST_AVAIL_BITS_SHIFT)
> +#define SPTE_HOST_WRITEABLE (1ULL<< PT_FIRST_AVAIL_BITS_SHIFT)
> +#define SPTE_NO_DIRTY (2ULL<< PT_FIRST_AVAIL_BITS_SHIFT)
>
> #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
>
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index e46eb8a..fdba751 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -325,6 +325,20 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
> break;
> }
>
> + if (*sptep& SPTE_NO_DIRTY) {
> + struct kvm_mmu_page *child;
> +
> + WARN_ON(level != gw->level);
> + WARN_ON(!is_shadow_present_pte(*sptep));
> + if (dirty) {
> + child = page_header(*sptep&
> + PT64_BASE_ADDR_MASK);
> + mmu_page_remove_parent_pte(child, sptep);
> + __set_spte(sptep, shadow_trap_nonpresent_pte);
> + kvm_flush_remote_tlbs(vcpu->kvm);
> + }
> + }
> +
>
Instead of adding a new bit, can you encode the protection in the direct
sp's access bits? So we'll have one sp for read-only or
writeable-but-not-dirty small pages, and another sp for
writeable-and-dirty small pages.
--
error compiling committee.c: too many arguments to function
Avi Kivity wrote:
>
> Instead of adding a new bit, can you encode the protection in the direct
> sp's access bits? So we'll have one sp for read-only or
> writeable-but-not-dirty small pages, and another sp for
> writeable-and-dirty small pages.
>
It looks like it can't solve all problems, it fix the access corrupted,
but will cause D bit losed:
mapping A and mapping B both are writable-and-dirty, when mapping A write
#PF occurs, the mapping is writable, then we can't set B's D bit anymore.
Anyway, i think we should re-intall the mapping when the state is changed. :-(
On 06/28/2010 01:02 PM, Xiao Guangrong wrote:
>
> Avi Kivity wrote:
>
>
>> Instead of adding a new bit, can you encode the protection in the direct
>> sp's access bits? So we'll have one sp for read-only or
>> writeable-but-not-dirty small pages, and another sp for
>> writeable-and-dirty small pages.
>>
>>
> It looks like it can't solve all problems, it fix the access corrupted,
> but will cause D bit losed:
>
> mapping A and mapping B both are writable-and-dirty, when mapping A write
> #PF occurs, the mapping is writable, then we can't set B's D bit anymore.
>
If B is writeable-and-dirty, then it's D bit is already set, and we
don't need to do anything.
If B is writeable-and-clean, then we'll have an spte pointing to a
read-only sp, so we'll get a write fault on access and an opportunity to
set the D bit.
> Anyway, i think we should re-intall the mapping when the state is changed. :-(
>
When the gpte is changed from read-only to writeable or from clean to
dirty, we need to update the spte, yes. But that's true for other sptes
as well, not just large gptes.
--
error compiling committee.c: too many arguments to function
Avi Kivity wrote:
> On 06/28/2010 01:02 PM, Xiao Guangrong wrote:
>>
>> Avi Kivity wrote:
>>
>>
>>> Instead of adding a new bit, can you encode the protection in the direct
>>> sp's access bits? So we'll have one sp for read-only or
>>> writeable-but-not-dirty small pages, and another sp for
>>> writeable-and-dirty small pages.
>>>
>>>
>> It looks like it can't solve all problems, it fix the access corrupted,
>> but will cause D bit losed:
>>
>> mapping A and mapping B both are writable-and-dirty, when mapping A write
>> #PF occurs, the mapping is writable, then we can't set B's D bit anymore.
>>
>
> If B is writeable-and-dirty, then it's D bit is already set, and we
> don't need to do anything.
>
> If B is writeable-and-clean, then we'll have an spte pointing to a
> read-only sp, so we'll get a write fault on access and an opportunity to
> set the D bit.
>
Sorry, a typo in my reply, i mean mapping A and B both are writable-and-clean,
while A occurs write-#PF, we should change A's spte map to writable sp, if we
only update the spte in writable-and-clean sp(form readonly to writable), the B's
D bit will miss set.
>> Anyway, i think we should re-intall the mapping when the state is
>> changed. :-(
>>
>
> When the gpte is changed from read-only to writeable or from clean to
> dirty, we need to update the spte, yes. But that's true for other sptes
> as well, not just large gptes.
>
I think the indirect sp is not hurt by this bug since for the indirect sp, the access
just form its upper-level, and the D bit is only in the last level, when we change the
pte's access, is not affect its sp's access.
But for direct sp, the sp's access is form all level. and different mapping that not share
the last mapping page will have the same last sp.
On 06/29/2010 04:17 AM, Xiao Guangrong wrote:
>
>> If B is writeable-and-dirty, then it's D bit is already set, and we
>> don't need to do anything.
>>
>> If B is writeable-and-clean, then we'll have an spte pointing to a
>> read-only sp, so we'll get a write fault on access and an opportunity to
>> set the D bit.
>>
>>
> Sorry, a typo in my reply, i mean mapping A and B both are writable-and-clean,
> while A occurs write-#PF, we should change A's spte map to writable sp, if we
> only update the spte in writable-and-clean sp(form readonly to writable), the B's
> D bit will miss set.
>
Right.
We need to update something to notice this:
- FNAME(fetch)() to replace the spte
- FNAME(walk_addr)() to invalidate the spte
I think FNAME(walk_addr) is the right place, we're updating the gpte, so
we should update the spte at the same time, just like a guest write.
But that will be expensive (there could be many sptes, so we have to
call kvm_mmu_pte_write()), so perhaps FNAME(fetch) is easier.
We have now
if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
continue;
So we need to add a check, if sp->role.access doesn't match pt_access &
pte_access, we need to get a new sp with the correct access (can only
change read->write).
>>> Anyway, i think we should re-intall the mapping when the state is
>>> changed. :-(
>>>
>>>
>> When the gpte is changed from read-only to writeable or from clean to
>> dirty, we need to update the spte, yes. But that's true for other sptes
>> as well, not just large gptes.
>>
>>
> I think the indirect sp is not hurt by this bug since for the indirect sp, the access
> just form its upper-level, and the D bit is only in the last level, when we change the
> pte's access, is not affect its sp's access.
>
> But for direct sp, the sp's access is form all level. and different mapping that not share
> the last mapping page will have the same last sp.
>
Right.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
On 06/29/2010 10:06 AM, Avi Kivity wrote:
> On 06/29/2010 04:17 AM, Xiao Guangrong wrote:
>>
>>> If B is writeable-and-dirty, then it's D bit is already set, and we
>>> don't need to do anything.
>>>
>>> If B is writeable-and-clean, then we'll have an spte pointing to a
>>> read-only sp, so we'll get a write fault on access and an
>>> opportunity to
>>> set the D bit.
>>>
>> Sorry, a typo in my reply, i mean mapping A and B both are
>> writable-and-clean,
>> while A occurs write-#PF, we should change A's spte map to writable
>> sp, if we
>> only update the spte in writable-and-clean sp(form readonly to
>> writable), the B's
>> D bit will miss set.
>
> Right.
>
> We need to update something to notice this:
>
> - FNAME(fetch)() to replace the spte
> - FNAME(walk_addr)() to invalidate the spte
>
> I think FNAME(walk_addr) is the right place, we're updating the gpte,
> so we should update the spte at the same time, just like a guest
> write. But that will be expensive (there could be many sptes, so we
> have to call kvm_mmu_pte_write()), so perhaps FNAME(fetch) is easier.
>
> We have now
>
> if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
> continue;
>
> So we need to add a check, if sp->role.access doesn't match pt_access
> & pte_access, we need to get a new sp with the correct access (can
> only change read->write).
Note:
- modifying walk_addr() to call kvm_mmu_pte_write() is probably not so
bad. It's rare that a large pte walk sets the dirty bit, and it's
probably rare to share those large ptes. Still, I think the fetch()
change is better since it's more local.
- there was once talk that instead of folding pt_access and pte_access
together into the leaf sp->role.access, each sp level would have its own
access permissions. In this case we don't even have to get a new direct
sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions
(all direct sp's would be writeable and permissions would be controlled
at their parent_pte level). Of course that's a much bigger change than
this bug fix.
--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
Avi Kivity wrote:
> On 06/29/2010 04:17 AM, Xiao Guangrong wrote:
>>
>>> If B is writeable-and-dirty, then it's D bit is already set, and we
>>> don't need to do anything.
>>>
>>> If B is writeable-and-clean, then we'll have an spte pointing to a
>>> read-only sp, so we'll get a write fault on access and an opportunity to
>>> set the D bit.
>>>
>>>
>> Sorry, a typo in my reply, i mean mapping A and B both are
>> writable-and-clean,
>> while A occurs write-#PF, we should change A's spte map to writable
>> sp, if we
>> only update the spte in writable-and-clean sp(form readonly to
>> writable), the B's
>> D bit will miss set.
>>
>
> Right.
>
> We need to update something to notice this:
>
> - FNAME(fetch)() to replace the spte
> - FNAME(walk_addr)() to invalidate the spte
>
> I think FNAME(walk_addr) is the right place, we're updating the gpte, so
> we should update the spte at the same time, just like a guest write.
> But that will be expensive (there could be many sptes, so we have to
> call kvm_mmu_pte_write()), so perhaps FNAME(fetch) is easier.
>
I agree.
> We have now
>
> if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
> continue;
>
> So we need to add a check, if sp->role.access doesn't match pt_access &
> pte_access, we need to get a new sp with the correct access (can only
> change read->write).
>
Umm, we should update the spte at the gw->level, so we need get the child
sp, and compare its access at this point, just like this:
if (level == gw->level && is_shadow_present_pte(*sptep)) {
child_sp = page_header(__pa(*sptep & PT64_BASE_ADDR_MASK));
if (child_sp->access != pt_access & pte_access & (diry ? 1 : ~ACC_WRITE_MASK )) {
/* Zap sptep */
......
}
}
So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark this spte then we can see
this spte whether need updated directly? i think it more simpler ;-)
Avi Kivity wrote:
>
> Note:
>
> - modifying walk_addr() to call kvm_mmu_pte_write() is probably not so
> bad. It's rare that a large pte walk sets the dirty bit, and it's
> probably rare to share those large ptes. Still, I think the fetch()
> change is better since it's more local.
>
> - there was once talk that instead of folding pt_access and pte_access
> together into the leaf sp->role.access, each sp level would have its own
> access permissions. In this case we don't even have to get a new direct
> sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions
> (all direct sp's would be writeable and permissions would be controlled
> at their parent_pte level). Of course that's a much bigger change than
> this bug fix.
>
Yeah, i have considered this way, but it will change the shadow page's mapping
way: it control the access at the upper level, but in the current code, we allow
the upper level have the ALL_ACCESS and control the access right at the last level.
It will break many things, such as write-protected...
On 06/29/2010 10:35 AM, Xiao Guangrong wrote:
>
>> We have now
>>
>> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
>> continue;
>>
>> So we need to add a check, if sp->role.access doesn't match pt_access&
>> pte_access, we need to get a new sp with the correct access (can only
>> change read->write).
>>
>>
> Umm, we should update the spte at the gw->level, so we need get the child
> sp, and compare its access at this point, just like this:
>
> if (level == gw->level&& is_shadow_present_pte(*sptep)) {
> child_sp = page_header(__pa(*sptep& PT64_BASE_ADDR_MASK));
>
> if (child_sp->access != pt_access& pte_access& (diry ? 1 : ~ACC_WRITE_MASK )) {
> /* Zap sptep */
> ......
> }
>
> }
>
> So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark this spte then we can see
> this spte whether need updated directly? i think it more simpler ;-)
>
It's new state, and new state means more maintenance of that state and
the need to consider the state in all relevant code paths.
In terms of maintainability, changing walk_addr() is best, since it
maintains the tight invariant that PT_PAGE_DIRECTORY_LEVEL sptes are
always consistent with their sptes. Updating fetch() to allow for a
relaxed invariant (spte may be read-only while gpte is write-dirty) is
more complicated, but performs better. This is also consistent with
what we do with PT_PAGE_TABLE_LEVEL gptes/sptes and with unsync pages.
btw, how can the patch work?
>
> + if (level == gw->level&& !dirty&&
> + access& gw->pte_access& ACC_WRITE_MASK)
> + spte |= SPTE_NO_DIRTY;
> +
> spte = __pa(sp->spt)
> | PT_PRESENT_MASK | PT_ACCESSED_MASK
> | PT_WRITABLE_MASK | PT_USER_MASK;
>
spte is immediately overwritten by the following assignment.
However, the other half of the patch can be adapted:
>
> + if (*sptep& SPTE_NO_DIRTY) {
> + struct kvm_mmu_page *child;
> +
> + WARN_ON(level != gw->level);
> + WARN_ON(!is_shadow_present_pte(*sptep));
> + if (dirty) {
> + child = page_header(*sptep&
> + PT64_BASE_ADDR_MASK);
> + mmu_page_remove_parent_pte(child, sptep);
> + __set_spte(sptep, shadow_trap_nonpresent_pte);
> + kvm_flush_remote_tlbs(vcpu->kvm);
> + }
> + }
> +
> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
> continue;
>
Simply replace (*spte & SPTE_NO_DIRTY) with a condition that checks
whether sp->access is consistent with gw->pt(e)_access.
Can you write a test case for qemu-kvm.git/kvm/test that demonstrates
the problem and the fix? It will help ensure we don't regress in this area.
--
error compiling committee.c: too many arguments to function
On 06/29/2010 10:45 AM, Xiao Guangrong wrote:
>
>> - there was once talk that instead of folding pt_access and pte_access
>> together into the leaf sp->role.access, each sp level would have its own
>> access permissions. In this case we don't even have to get a new direct
>> sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions
>> (all direct sp's would be writeable and permissions would be controlled
>> at their parent_pte level). Of course that's a much bigger change than
>> this bug fix.
>>
>>
> Yeah, i have considered this way, but it will change the shadow page's mapping
> way: it control the access at the upper level, but in the current code, we allow
> the upper level have the ALL_ACCESS and control the access right at the last level.
> It will break many things, such as write-protected...
>
spte's access bits have dual purpose, both to map guest protection and
for host protection (like for shadowed pages, or ksm pages). So the
last level sptes still need to consider host write protection.
--
error compiling committee.c: too many arguments to function
Avi Kivity wrote:
> On 06/29/2010 10:35 AM, Xiao Guangrong wrote:
>>
>>> We have now
>>>
>>> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
>>> continue;
>>>
>>> So we need to add a check, if sp->role.access doesn't match pt_access&
>>> pte_access, we need to get a new sp with the correct access (can only
>>> change read->write).
>>>
>>>
>> Umm, we should update the spte at the gw->level, so we need get the child
>> sp, and compare its access at this point, just like this:
>>
>> if (level == gw->level&& is_shadow_present_pte(*sptep)) {
>> child_sp = page_header(__pa(*sptep& PT64_BASE_ADDR_MASK));
>>
>> if (child_sp->access != pt_access& pte_access& (diry ? 1 :
>> ~ACC_WRITE_MASK )) {
>> /* Zap sptep */
>> ......
>> }
>>
>> }
>>
>> So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark
>> this spte then we can see
>> this spte whether need updated directly? i think it more simpler ;-)
>>
>
> It's new state, and new state means more maintenance of that state and
> the need to consider the state in all relevant code paths.
>
> In terms of maintainability, changing walk_addr() is best, since it
> maintains the tight invariant that PT_PAGE_DIRECTORY_LEVEL sptes are
> always consistent with their sptes. Updating fetch() to allow for a
> relaxed invariant (spte may be read-only while gpte is write-dirty) is
> more complicated, but performs better. This is also consistent with
> what we do with PT_PAGE_TABLE_LEVEL gptes/sptes and with unsync pages.
>
Maybe you are right, i just think is more quickly by using SPTE_NO_DIRTY flag
to judge whether need updated. I'll modify this patch as your suggestion.
> btw, how can the patch work?
>
>>
>> + if (level == gw->level&& !dirty&&
>> + access& gw->pte_access& ACC_WRITE_MASK)
>> + spte |= SPTE_NO_DIRTY;
>> +
>> spte = __pa(sp->spt)
>> | PT_PRESENT_MASK | PT_ACCESSED_MASK
>> | PT_WRITABLE_MASK | PT_USER_MASK;
>>
>
> spte is immediately overwritten by the following assignment.
>
Ah, sorry, i miss it, spte |= SPTE_NO_DIRTY should behind of following assignment.
> However, the other half of the patch can be adapted:
>
>>
>> + if (*sptep& SPTE_NO_DIRTY) {
>> + struct kvm_mmu_page *child;
>> +
>> + WARN_ON(level != gw->level);
>> + WARN_ON(!is_shadow_present_pte(*sptep));
>> + if (dirty) {
>> + child = page_header(*sptep&
>> + PT64_BASE_ADDR_MASK);
>> + mmu_page_remove_parent_pte(child, sptep);
>> + __set_spte(sptep, shadow_trap_nonpresent_pte);
>> + kvm_flush_remote_tlbs(vcpu->kvm);
>> + }
>> + }
>> +
>> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
>> continue;
>>
>
> Simply replace (*spte & SPTE_NO_DIRTY) with a condition that checks
> whether sp->access is consistent with gw->pt(e)_access.
>
If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY flag in
the spte, when the next #PF occurs, we just need check this flag and see whether
gpte's D bit is set, if it's true, we zap this spte and map to the correct sp.
> Can you write a test case for qemu-kvm.git/kvm/test that demonstrates
> the problem and the fix? It will help ensure we don't regress in this
> area.
>
OK, but allow me do it later :-)
Avi Kivity wrote:
> On 06/29/2010 10:45 AM, Xiao Guangrong wrote:
>>
>>> - there was once talk that instead of folding pt_access and pte_access
>>> together into the leaf sp->role.access, each sp level would have its own
>>> access permissions. In this case we don't even have to get a new direct
>>> sp, only change the PT_DIRECTORY_LEVEL spte to add write permissions
>>> (all direct sp's would be writeable and permissions would be controlled
>>> at their parent_pte level). Of course that's a much bigger change than
>>> this bug fix.
>>>
>>>
>> Yeah, i have considered this way, but it will change the shadow page's
>> mapping
>> way: it control the access at the upper level, but in the current
>> code, we allow
>> the upper level have the ALL_ACCESS and control the access right at
>> the last level.
>> It will break many things, such as write-protected...
>>
>
> spte's access bits have dual purpose, both to map guest protection and
> for host protection (like for shadowed pages, or ksm pages). So the
> last level sptes still need to consider host write protection.
>
Yeah, i see your mean, thanks, :-)
On 06/29/2010 12:04 PM, Xiao Guangrong wrote:
>
>> Simply replace (*spte& SPTE_NO_DIRTY) with a condition that checks
>> whether sp->access is consistent with gw->pt(e)_access.
>>
>>
> If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY flag in
> the spte, when the next #PF occurs, we just need check this flag and see whether
> gpte's D bit is set, if it's true, we zap this spte and map to the correct sp.
>
My point is, SPTE_NO_DIRTY is equivalent to an sp->role.access check
(the access check is a bit slower, but that shouldn't matter).
>> Can you write a test case for qemu-kvm.git/kvm/test that demonstrates
>> the problem and the fix? It will help ensure we don't regress in this
>> area.
>>
>>
> OK, but allow me do it later :-)
>
>
Sure, but please do it soon.
--
error compiling committee.c: too many arguments to function
Avi Kivity wrote:
> On 06/29/2010 12:04 PM, Xiao Guangrong wrote:
>>
>>> Simply replace (*spte& SPTE_NO_DIRTY) with a condition that checks
>>> whether sp->access is consistent with gw->pt(e)_access.
>>>
>>>
>> If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY
>> flag in
>> the spte, when the next #PF occurs, we just need check this flag and
>> see whether
>> gpte's D bit is set, if it's true, we zap this spte and map to the
>> correct sp.
>>
>
> My point is, SPTE_NO_DIRTY is equivalent to an sp->role.access check
> (the access check is a bit slower, but that shouldn't matter).
>
I see.
>
>>> Can you write a test case for qemu-kvm.git/kvm/test that demonstrates
>>> the problem and the fix? It will help ensure we don't regress in this
>>> area.
>>>
>>>
>> OK, but allow me do it later :-)
>>
>>
>
> Sure, but please do it soon.
Sure, i will do it as soon as possible.