* [PATCH 0 of 2] New mem access type: None -> RWX @ 2011-11-29 21:58 Andres Lagar-Cavilla 2011-11-29 21:58 ` [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know Andres Lagar-Cavilla 2011-11-29 21:58 ` [PATCH 2 of 2] x86/mm: New mem access type to log access Andres Lagar-Cavilla 0 siblings, 2 replies; 8+ messages in thread From: Andres Lagar-Cavilla @ 2011-11-29 21:58 UTC (permalink / raw) To: xen-devel; +Cc: andres, keir.xen, tim, JBeulich, adin This patch adds a new p2m access type, n2rwx. It allows for implement a "log access" mode in the hypervisor, aking to log dirty but for all types of accesses. Faults caused by this access mode automatically promote the access rights of the ofending p2m entry, place the event in the ring, and let the vcpu keep on executing. Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> Signed-off-by: Adin Scannell <adin@scannell.ca> xen/arch/x86/hvm/hvm.c | 45 ++++++++++++++++++++++++++++++++++------ xen/arch/x86/mm/p2m.c | 8 ++++-- xen/include/asm-x86/p2m.h | 9 ++++--- xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/mm/p2m-ept.c | 1 + xen/arch/x86/mm/p2m.c | 30 +++++++++++++++++++-------- xen/include/asm-x86/p2m.h | 3 ++ xen/include/public/hvm/hvm_op.h | 3 ++ 8 files changed, 77 insertions(+), 23 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know 2011-11-29 21:58 [PATCH 0 of 2] New mem access type: None -> RWX Andres Lagar-Cavilla @ 2011-11-29 21:58 ` Andres Lagar-Cavilla 2011-12-01 16:17 ` Tim Deegan 2011-11-29 21:58 ` [PATCH 2 of 2] x86/mm: New mem access type to log access Andres Lagar-Cavilla 1 sibling, 1 reply; 8+ messages in thread From: Andres Lagar-Cavilla @ 2011-11-29 21:58 UTC (permalink / raw) To: xen-devel; +Cc: andres, keir.xen, tim, JBeulich, adin xen/arch/x86/hvm/hvm.c | 45 ++++++++++++++++++++++++++++++++++++++------- xen/arch/x86/mm/p2m.c | 8 +++++--- xen/include/asm-x86/p2m.h | 9 +++++---- 3 files changed, 48 insertions(+), 14 deletions(-) The mem event fault handler in the p2m can automatically promote the access rights of a p2m entry. In those scenarios, vcpu's are not paused and they will immediately retry the faulting instructions. This will generate a second fault if the underlying entry type requires so (paging, unsharing, pod, etc). Collapse the two faults into a single one. Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1278,9 +1278,13 @@ int hvm_hap_nested_page_fault(unsigned l if ( violation ) { - p2m_mem_access_check(gpa, gla_valid, gla, access_r, access_w, access_x); - rc = 1; - goto out_put_gfn; + if ( !p2m_mem_access_check(gpa, gla_valid, gla, access_r, + access_w, access_x) ) + { + /* Rights not promoted, vcpu paused, work here is done */ + rc = 1; + goto out_put_gfn; + } } } @@ -1288,7 +1292,8 @@ int hvm_hap_nested_page_fault(unsigned l * If this GFN is emulated MMIO or marked as read-only, pass the fault * to the mmio handler. */ - if ( (p2mt == p2m_mmio_dm) || (p2mt == p2m_ram_ro) ) + if ( (p2mt == p2m_mmio_dm) || + (access_w && (p2mt == p2m_ram_ro)) ) { if ( !handle_mmio() ) hvm_inject_exception(TRAP_gp_fault, 0, 0); @@ -1302,7 +1307,7 @@ int hvm_hap_nested_page_fault(unsigned l p2m_mem_paging_populate(v->domain, gfn); /* Mem sharing: unshare the page and try again */ - if ( p2mt == p2m_ram_shared ) + if ( access_w && (p2mt == p2m_ram_shared) ) { ASSERT(!p2m_is_nestedp2m(p2m)); mem_sharing_unshare_page(p2m->domain, gfn, 0); @@ -1319,14 +1324,15 @@ int hvm_hap_nested_page_fault(unsigned l * a large page, we do not change other pages type within that large * page. */ - paging_mark_dirty(v->domain, mfn_x(mfn)); + if ( access_w ) + paging_mark_dirty(v->domain, mfn_x(mfn)); p2m_change_type(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw); rc = 1; goto out_put_gfn; } /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */ - if ( p2mt == p2m_grant_map_ro ) + if ( access_w && (p2mt == p2m_grant_map_ro) ) { gdprintk(XENLOG_WARNING, "trying to write to read-only grant mapping\n"); @@ -1335,6 +1341,31 @@ int hvm_hap_nested_page_fault(unsigned l goto out_put_gfn; } + if ( access_x && (p2m_is_grant(p2mt)) ) + { + gdprintk(XENLOG_WARNING, + "trying to execut a grant mapping\n"); + hvm_inject_exception(TRAP_gp_fault, 0, 0); + rc = 1; + goto out_put_gfn; + } + + if ( p2m_is_grant(p2mt) ) + { + /* If we haven't caught this by now, then it's a valid access */ + rc = 1; + goto out_put_gfn; + } + + if ( p2mt == p2m_mmio_direct ) + { + if ( !(access_w && + rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))) ) { + rc = 1; + goto out_put_gfn; + } + } + rc = 0; out_put_gfn: put_gfn(p2m->domain, gfn); diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1126,7 +1126,7 @@ void p2m_mem_paging_resume(struct domain mem_event_unpause_vcpus(d, &d->mem_paging); } -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, +int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, bool_t access_r, bool_t access_w, bool_t access_x) { struct vcpu *v = current; @@ -1146,7 +1146,7 @@ void p2m_mem_access_check(unsigned long { p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rw); p2m_unlock(p2m); - return; + return 1; } p2m_unlock(p2m); @@ -1166,9 +1166,10 @@ void p2m_mem_access_check(unsigned long p2m_lock(p2m); p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); p2m_unlock(p2m); + return 1; } - return; + return 0; } memset(&req, 0, sizeof(req)); @@ -1192,6 +1193,7 @@ void p2m_mem_access_check(unsigned long (void)mem_event_put_request(d, &d->mem_access, &req); /* VCPU paused */ + return 0; } void p2m_mem_access_resume(struct domain *d) diff -r 29701f5bdd84 -r d6354df726a0 xen/include/asm-x86/p2m.h --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula #ifdef __x86_64__ /* Send mem event based on the access (gla is -1ull if not available). Handles - * the rw2rx conversion */ -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, + * the rw2rx conversion. Return value indicate if access rights have been + * promoted with no underlying vcpu pause. */ +int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, bool_t access_r, bool_t access_w, bool_t access_x); /* Resumes the running of the VCPU, restarting the last instruction */ void p2m_mem_access_resume(struct domain *d); @@ -508,10 +509,10 @@ int p2m_get_mem_access(struct domain *d, hvmmem_access_t *access); #else -static inline void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, +static inline int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, bool_t access_r, bool_t access_w, bool_t access_x) -{ } +{ return 1; } static inline int p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr, hvmmem_access_t access) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know 2011-11-29 21:58 ` [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know Andres Lagar-Cavilla @ 2011-12-01 16:17 ` Tim Deegan 2011-12-01 16:39 ` Andres Lagar-Cavilla 0 siblings, 1 reply; 8+ messages in thread From: Tim Deegan @ 2011-12-01 16:17 UTC (permalink / raw) To: Andres Lagar-Cavilla; +Cc: andres, xen-devel, keir.xen, JBeulich, adin Hi, At 16:58 -0500 on 29 Nov (1322585904), Andres Lagar-Cavilla wrote: > xen/arch/x86/hvm/hvm.c | 45 ++++++++++++++++++++++++++++++++++++++------- > xen/arch/x86/mm/p2m.c | 8 +++++--- > xen/include/asm-x86/p2m.h | 9 +++++---- > 3 files changed, 48 insertions(+), 14 deletions(-) > > > The mem event fault handler in the p2m can automatically promote the access > rights of a p2m entry. In those scenarios, vcpu's are not paused and they will > immediately retry the faulting instructions. This will generate a second fault > if the underlying entry type requires so (paging, unsharing, pod, etc). > Collapse the two faults into a single one. > > Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> > > diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1278,9 +1278,13 @@ int hvm_hap_nested_page_fault(unsigned l > > if ( violation ) > { > - p2m_mem_access_check(gpa, gla_valid, gla, access_r, access_w, access_x); > - rc = 1; > - goto out_put_gfn; > + if ( !p2m_mem_access_check(gpa, gla_valid, gla, access_r, > + access_w, access_x) ) > + { > + /* Rights not promoted, vcpu paused, work here is done */ > + rc = 1; > + goto out_put_gfn; > + } > } > } > > @@ -1288,7 +1292,8 @@ int hvm_hap_nested_page_fault(unsigned l > * If this GFN is emulated MMIO or marked as read-only, pass the fault > * to the mmio handler. > */ > - if ( (p2mt == p2m_mmio_dm) || (p2mt == p2m_ram_ro) ) > + if ( (p2mt == p2m_mmio_dm) || > + (access_w && (p2mt == p2m_ram_ro)) ) I think this is a separate change from the main intent of the patch; it would be better to have two patches, once that inserts all these 'access_w' checks and a second that does what the cset comment decribes. > { > if ( !handle_mmio() ) > hvm_inject_exception(TRAP_gp_fault, 0, 0); > @@ -1302,7 +1307,7 @@ int hvm_hap_nested_page_fault(unsigned l > p2m_mem_paging_populate(v->domain, gfn); > > /* Mem sharing: unshare the page and try again */ > - if ( p2mt == p2m_ram_shared ) > + if ( access_w && (p2mt == p2m_ram_shared) ) > { > ASSERT(!p2m_is_nestedp2m(p2m)); > mem_sharing_unshare_page(p2m->domain, gfn, 0); > @@ -1319,14 +1324,15 @@ int hvm_hap_nested_page_fault(unsigned l > * a large page, we do not change other pages type within that large > * page. > */ > - paging_mark_dirty(v->domain, mfn_x(mfn)); > + if ( access_w ) > + paging_mark_dirty(v->domain, mfn_x(mfn)); > p2m_change_type(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw); No! If we call p2m_change_type(-->ram_rw) we _must_ call mark_dirty() too. It would be OK to put both lines under the test, though. > rc = 1; > goto out_put_gfn; > } > > /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */ > - if ( p2mt == p2m_grant_map_ro ) > + if ( access_w && (p2mt == p2m_grant_map_ro) ) > { > gdprintk(XENLOG_WARNING, > "trying to write to read-only grant mapping\n"); > @@ -1335,6 +1341,31 @@ int hvm_hap_nested_page_fault(unsigned l > goto out_put_gfn; > } > > + if ( access_x && (p2m_is_grant(p2mt)) ) > + { > + gdprintk(XENLOG_WARNING, > + "trying to execut a grant mapping\n"); > + hvm_inject_exception(TRAP_gp_fault, 0, 0); > + rc = 1; > + goto out_put_gfn; > + } Again, this is a separate bugfix and should go in its own patch. > + if ( p2m_is_grant(p2mt) ) > + { > + /* If we haven't caught this by now, then it's a valid access */ > + rc = 1; > + goto out_put_gfn; > + } > + if ( p2mt == p2m_mmio_direct ) > + { > + if ( !(access_w && > + rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))) ) { > + rc = 1; > + goto out_put_gfn; > + } > + } I wonder whether, rather than trying to enumerate all the acceptable cases here, you could just remember that p2m_mem_access_check() changed something and always return 1 in that case. > + > rc = 0; > out_put_gfn: > put_gfn(p2m->domain, gfn); > diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1126,7 +1126,7 @@ void p2m_mem_paging_resume(struct domain > mem_event_unpause_vcpus(d, &d->mem_paging); > } > > -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, > +int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, > bool_t access_r, bool_t access_w, bool_t access_x) > { > struct vcpu *v = current; > @@ -1146,7 +1146,7 @@ void p2m_mem_access_check(unsigned long > { > p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rw); > p2m_unlock(p2m); > - return; > + return 1; > } > p2m_unlock(p2m); > > @@ -1166,9 +1166,10 @@ void p2m_mem_access_check(unsigned long > p2m_lock(p2m); > p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); > p2m_unlock(p2m); > + return 1; > } > > - return; > + return 0; > } > > memset(&req, 0, sizeof(req)); > @@ -1192,6 +1193,7 @@ void p2m_mem_access_check(unsigned long > > (void)mem_event_put_request(d, &d->mem_access, &req); > /* VCPU paused */ > + return 0; > } > > void p2m_mem_access_resume(struct domain *d) > diff -r 29701f5bdd84 -r d6354df726a0 xen/include/asm-x86/p2m.h > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula > > #ifdef __x86_64__ > /* Send mem event based on the access (gla is -1ull if not available). Handles > - * the rw2rx conversion */ > -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, > + * the rw2rx conversion. Return value indicate if access rights have been > + * promoted with no underlying vcpu pause. */ How does it indicate that -- i.e., what values can it return and what do they mean? (And if it only returns 0 or 1, maybe use bool_t.) Cheers, Tim. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know 2011-12-01 16:17 ` Tim Deegan @ 2011-12-01 16:39 ` Andres Lagar-Cavilla 2011-12-01 16:53 ` Tim Deegan 0 siblings, 1 reply; 8+ messages in thread From: Andres Lagar-Cavilla @ 2011-12-01 16:39 UTC (permalink / raw) To: Tim Deegan; +Cc: andres, xen-devel, keir.xen, jbeulich, adin > Hi, > > At 16:58 -0500 on 29 Nov (1322585904), Andres Lagar-Cavilla wrote: >> xen/arch/x86/hvm/hvm.c | 45 >> ++++++++++++++++++++++++++++++++++++++------- >> xen/arch/x86/mm/p2m.c | 8 +++++--- >> xen/include/asm-x86/p2m.h | 9 +++++---- >> 3 files changed, 48 insertions(+), 14 deletions(-) >> >> >> The mem event fault handler in the p2m can automatically promote the >> access >> rights of a p2m entry. In those scenarios, vcpu's are not paused and >> they will >> immediately retry the faulting instructions. This will generate a second >> fault >> if the underlying entry type requires so (paging, unsharing, pod, etc). >> Collapse the two faults into a single one. >> >> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> >> >> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -1278,9 +1278,13 @@ int hvm_hap_nested_page_fault(unsigned l >> >> if ( violation ) >> { >> - p2m_mem_access_check(gpa, gla_valid, gla, access_r, >> access_w, access_x); >> - rc = 1; >> - goto out_put_gfn; >> + if ( !p2m_mem_access_check(gpa, gla_valid, gla, access_r, >> + access_w, access_x) ) >> + { >> + /* Rights not promoted, vcpu paused, work here is done >> */ >> + rc = 1; >> + goto out_put_gfn; >> + } >> } >> } >> >> @@ -1288,7 +1292,8 @@ int hvm_hap_nested_page_fault(unsigned l >> * If this GFN is emulated MMIO or marked as read-only, pass the >> fault >> * to the mmio handler. >> */ >> - if ( (p2mt == p2m_mmio_dm) || (p2mt == p2m_ram_ro) ) >> + if ( (p2mt == p2m_mmio_dm) || >> + (access_w && (p2mt == p2m_ram_ro)) ) > > I think this is a separate change from the main intent of the patch; it > would be better to have two patches, once that inserts all these > 'access_w' checks and a second that does what the cset comment > decribes. > The new checks here and below are necessary because the fault handler assumes that the fault could not have happened due to a constrain on the access rights. So, new cases arise, such as the mmio_direct below. I can add all the additional checks in patch 1, and allow the fall through in patch 2. (more below ...) >> { >> if ( !handle_mmio() ) >> hvm_inject_exception(TRAP_gp_fault, 0, 0); >> @@ -1302,7 +1307,7 @@ int hvm_hap_nested_page_fault(unsigned l >> p2m_mem_paging_populate(v->domain, gfn); >> >> /* Mem sharing: unshare the page and try again */ >> - if ( p2mt == p2m_ram_shared ) >> + if ( access_w && (p2mt == p2m_ram_shared) ) >> { >> ASSERT(!p2m_is_nestedp2m(p2m)); >> mem_sharing_unshare_page(p2m->domain, gfn, 0); >> @@ -1319,14 +1324,15 @@ int hvm_hap_nested_page_fault(unsigned l >> * a large page, we do not change other pages type within that >> large >> * page. >> */ >> - paging_mark_dirty(v->domain, mfn_x(mfn)); >> + if ( access_w ) >> + paging_mark_dirty(v->domain, mfn_x(mfn)); >> p2m_change_type(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw); > > No! If we call p2m_change_type(-->ram_rw) we _must_ call mark_dirty() > too. It would be OK to put both lines under the test, though. > Yup, thanks. >> rc = 1; >> goto out_put_gfn; >> } >> >> /* Shouldn't happen: Maybe the guest was writing to a r/o grant >> mapping? */ >> - if ( p2mt == p2m_grant_map_ro ) >> + if ( access_w && (p2mt == p2m_grant_map_ro) ) >> { >> gdprintk(XENLOG_WARNING, >> "trying to write to read-only grant mapping\n"); >> @@ -1335,6 +1341,31 @@ int hvm_hap_nested_page_fault(unsigned l >> goto out_put_gfn; >> } >> >> + if ( access_x && (p2m_is_grant(p2mt)) ) >> + { >> + gdprintk(XENLOG_WARNING, >> + "trying to execut a grant mapping\n"); >> + hvm_inject_exception(TRAP_gp_fault, 0, 0); >> + rc = 1; >> + goto out_put_gfn; >> + } > > Again, this is a separate bugfix and should go in its own patch. > >> + if ( p2m_is_grant(p2mt) ) >> + { >> + /* If we haven't caught this by now, then it's a valid access >> */ >> + rc = 1; >> + goto out_put_gfn; >> + } >> + if ( p2mt == p2m_mmio_direct ) >> + { >> + if ( !(access_w && >> + rangeset_contains_singleton(mmio_ro_ranges, >> mfn_x(mfn))) ) { >> + rc = 1; >> + goto out_put_gfn; >> + } >> + } > > I wonder whether, rather than trying to enumerate all the acceptable > cases here, you could just remember that p2m_mem_access_check() changed > something and always return 1 in that case. > That's the behavior without this patch, isn't it? >> + >> rc = 0; >> out_put_gfn: >> put_gfn(p2m->domain, gfn); >> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -1126,7 +1126,7 @@ void p2m_mem_paging_resume(struct domain >> mem_event_unpause_vcpus(d, &d->mem_paging); >> } >> >> -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned >> long gla, >> +int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned >> long gla, >> bool_t access_r, bool_t access_w, bool_t >> access_x) >> { >> struct vcpu *v = current; >> @@ -1146,7 +1146,7 @@ void p2m_mem_access_check(unsigned long >> { >> p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, >> p2m_access_rw); >> p2m_unlock(p2m); >> - return; >> + return 1; >> } >> p2m_unlock(p2m); >> >> @@ -1166,9 +1166,10 @@ void p2m_mem_access_check(unsigned long >> p2m_lock(p2m); >> p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, >> p2m_access_rwx); >> p2m_unlock(p2m); >> + return 1; >> } >> >> - return; >> + return 0; >> } >> >> memset(&req, 0, sizeof(req)); >> @@ -1192,6 +1193,7 @@ void p2m_mem_access_check(unsigned long >> >> (void)mem_event_put_request(d, &d->mem_access, &req); >> /* VCPU paused */ >> + return 0; >> } >> >> void p2m_mem_access_resume(struct domain *d) >> diff -r 29701f5bdd84 -r d6354df726a0 xen/include/asm-x86/p2m.h >> --- a/xen/include/asm-x86/p2m.h >> +++ b/xen/include/asm-x86/p2m.h >> @@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula >> >> #ifdef __x86_64__ >> /* Send mem event based on the access (gla is -1ull if not available). >> Handles >> - * the rw2rx conversion */ >> -void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned >> long gla, >> + * the rw2rx conversion. Return value indicate if access rights have >> been >> + * promoted with no underlying vcpu pause. */ > > How does it indicate that -- i.e., what values can it return and what do > they mean? (And if it only returns 0 or 1, maybe use bool_t.) > Ok, bool_t. > Cheers, > > Tim. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know 2011-12-01 16:39 ` Andres Lagar-Cavilla @ 2011-12-01 16:53 ` Tim Deegan 0 siblings, 0 replies; 8+ messages in thread From: Tim Deegan @ 2011-12-01 16:53 UTC (permalink / raw) To: Andres Lagar-Cavilla; +Cc: andres, xen-devel, keir.xen, jbeulich, adin At 08:39 -0800 on 01 Dec (1322728771), Andres Lagar-Cavilla wrote: > > I wonder whether, rather than trying to enumerate all the acceptable > > cases here, you could just remember that p2m_mem_access_check() changed > > something and always return 1 in that case. > > > That's the behavior without this patch, isn't it? Yes, but that's not the _interesting_ change in this patch. :) The interesting thing is that you can upgrade the access rights and also pass the fault to the mmio emulator without returning and retrying in between. That is, I thin the change from if (access fixup) return 1 handle MMIO ... return 0 /* unhandled -- crash */ to if (access fixup) remember we've done something handle MMIO ... if (we did a fixup above) return 1 /* try again */ else return 0 /* crash */ does what you want without making the handler much bigger and more brittle. Tim. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2 of 2] x86/mm: New mem access type to log access 2011-11-29 21:58 [PATCH 0 of 2] New mem access type: None -> RWX Andres Lagar-Cavilla 2011-11-29 21:58 ` [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know Andres Lagar-Cavilla @ 2011-11-29 21:58 ` Andres Lagar-Cavilla 2011-12-01 16:25 ` Tim Deegan 1 sibling, 1 reply; 8+ messages in thread From: Andres Lagar-Cavilla @ 2011-11-29 21:58 UTC (permalink / raw) To: xen-devel; +Cc: andres, keir.xen, tim, JBeulich, adin xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/mm/p2m-ept.c | 1 + xen/arch/x86/mm/p2m.c | 30 +++++++++++++++++++++--------- xen/include/asm-x86/p2m.h | 3 +++ xen/include/public/hvm/hvm_op.h | 3 +++ 5 files changed, 29 insertions(+), 9 deletions(-) This patch adds a new p2m access type, n2rwx. It allows for implement a "log access" mode in the hypervisor, aking to log dirty but for all types of accesses. Faults caused by this access mode automatically promote the access rights of the ofending p2m entry, place the event in the ring, and let the vcpu keep on executing. Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org> Signed-off-by: Adin Scannell <adin@scannell.ca> diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/hvm/hvm.c --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1250,6 +1250,7 @@ int hvm_hap_nested_page_fault(unsigned l switch (p2ma) { case p2m_access_n: + case p2m_access_n2rwx: default: violation = access_r || access_w || access_x; break; diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/mm/p2m-ept.c --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -111,6 +111,7 @@ static void ept_p2m_type_to_flags(ept_en switch (access) { case p2m_access_n: + case p2m_access_n2rwx: entry->r = entry->w = entry->x = 0; break; case p2m_access_r: diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1148,6 +1148,11 @@ int p2m_mem_access_check(unsigned long g p2m_unlock(p2m); return 1; } + else if ( p2ma == p2m_access_n2rwx ) + { + ASSERT(access_w || access_r || access_x); + p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); + } p2m_unlock(p2m); /* Otherwise, check if there is a memory event listener, and send the message along */ @@ -1162,10 +1167,13 @@ int p2m_mem_access_check(unsigned long g } else { - /* A listener is not required, so clear the access restrictions */ - p2m_lock(p2m); - p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); - p2m_unlock(p2m); + if ( p2ma != p2m_access_n2rwx ) + { + /* A listener is not required, so clear the access restrictions */ + p2m_lock(p2m); + p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); + p2m_unlock(p2m); + } return 1; } @@ -1176,9 +1184,12 @@ int p2m_mem_access_check(unsigned long g req.type = MEM_EVENT_TYPE_ACCESS; req.reason = MEM_EVENT_REASON_VIOLATION; - /* Pause the current VCPU unconditionally */ - vcpu_pause_nosync(v); - req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED; + /* Pause the current VCPU */ + if ( p2ma != p2m_access_n2rwx ) + { + vcpu_pause_nosync(v); + req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED; + } /* Send request to mem event */ req.gfn = gfn; @@ -1192,8 +1203,8 @@ int p2m_mem_access_check(unsigned long g req.vcpu_id = v->vcpu_id; (void)mem_event_put_request(d, &d->mem_access, &req); - /* VCPU paused */ - return 0; + /* VCPU may be paused, return whether we promoted automatically */ + return (p2ma == p2m_access_n2rwx); } void p2m_mem_access_resume(struct domain *d) @@ -1237,6 +1248,7 @@ int p2m_set_mem_access(struct domain *d, p2m_access_wx, p2m_access_rwx, p2m_access_rx2rw, + p2m_access_n2rwx, p2m->default_access, }; diff -r d6354df726a0 -r 52d6aede6206 xen/include/asm-x86/p2m.h --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -108,6 +108,9 @@ typedef enum { p2m_access_wx = 6, p2m_access_rwx = 7, p2m_access_rx2rw = 8, /* Special: page goes from RX to RW on write */ + p2m_access_n2rwx = 9, /* Special: page goes from N to RWX on access, * + * generates an event but does not pause the + * vcpu */ /* NOTE: Assumed to be only 4 bits right now */ } p2m_access_t; diff -r d6354df726a0 -r 52d6aede6206 xen/include/public/hvm/hvm_op.h --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -174,6 +174,9 @@ typedef enum { HVMMEM_access_rwx, HVMMEM_access_rx2rw, /* Page starts off as r-x, but automatically * change to r-w on a write */ + HVMMEM_access_n2rwx, /* Log access: starts off as n, automatically + * goes to rwx, generating an event without + * pausing the vcpu */ HVMMEM_access_default /* Take the domain default */ } hvmmem_access_t; /* Notify that a region of memory is to have specific access types */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2 of 2] x86/mm: New mem access type to log access 2011-11-29 21:58 ` [PATCH 2 of 2] x86/mm: New mem access type to log access Andres Lagar-Cavilla @ 2011-12-01 16:25 ` Tim Deegan 2011-12-01 16:33 ` Andres Lagar-Cavilla 0 siblings, 1 reply; 8+ messages in thread From: Tim Deegan @ 2011-12-01 16:25 UTC (permalink / raw) To: Andres Lagar-Cavilla; +Cc: andres, xen-devel, keir.xen, JBeulich, adin At 16:58 -0500 on 29 Nov (1322585905), Andres Lagar-Cavilla wrote: > @@ -1162,10 +1167,13 @@ int p2m_mem_access_check(unsigned long g > } > else > { > - /* A listener is not required, so clear the access restrictions */ > - p2m_lock(p2m); > - p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); > - p2m_unlock(p2m); > + if ( p2ma != p2m_access_n2rwx ) > + { > + /* A listener is not required, so clear the access restrictions */ > + p2m_lock(p2m); > + p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx); > + p2m_unlock(p2m); > + } > return 1; > } This logic is getting a bit convoluted, and I'm not sure it's correct. If a page is marked n2rwx and there's no listener, it looks like this will cause it to spin forever re-taking the fault rather than pausing it waiting for the listener to attach. Cheers, Tim. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2 of 2] x86/mm: New mem access type to log access 2011-12-01 16:25 ` Tim Deegan @ 2011-12-01 16:33 ` Andres Lagar-Cavilla 0 siblings, 0 replies; 8+ messages in thread From: Andres Lagar-Cavilla @ 2011-12-01 16:33 UTC (permalink / raw) To: Tim Deegan; +Cc: andres, xen-devel, keir.xen, jbeulich, adin > At 16:58 -0500 on 29 Nov (1322585905), Andres Lagar-Cavilla wrote: >> @@ -1162,10 +1167,13 @@ int p2m_mem_access_check(unsigned long g >> } >> else >> { >> - /* A listener is not required, so clear the access >> restrictions */ >> - p2m_lock(p2m); >> - p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, >> p2m_access_rwx); >> - p2m_unlock(p2m); >> + if ( p2ma != p2m_access_n2rwx ) >> + { >> + /* A listener is not required, so clear the access >> restrictions */ >> + p2m_lock(p2m); >> + p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, >> p2m_access_rwx); >> + p2m_unlock(p2m); >> + } >> return 1; >> } > > This logic is getting a bit convoluted, and I'm not sure it's correct. > If a page is marked n2rwx and there's no listener, it looks like this > will cause it to spin forever re-taking the fault rather than pausing it > waiting for the listener to attach. The entry is set to p2m_access_rwx, so no additional faults will be generated. In the n2rwx case, the entry was promoted to rwx previously, in a p2m_lock protected section. We're avoiding a repeat here. Andres > > Cheers, > > Tim. > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-12-01 16:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-29 21:58 [PATCH 0 of 2] New mem access type: None -> RWX Andres Lagar-Cavilla 2011-11-29 21:58 ` [PATCH 1 of 2] x86/mm: When mem event automatically promotes access rights, let other subsystems know Andres Lagar-Cavilla 2011-12-01 16:17 ` Tim Deegan 2011-12-01 16:39 ` Andres Lagar-Cavilla 2011-12-01 16:53 ` Tim Deegan 2011-11-29 21:58 ` [PATCH 2 of 2] x86/mm: New mem access type to log access Andres Lagar-Cavilla 2011-12-01 16:25 ` Tim Deegan 2011-12-01 16:33 ` Andres Lagar-Cavilla
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.