| Message ID | 1469734504-5317-1-git-send-email-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Tamas, On 28/07/2016 20:35, Tamas K Lengyel wrote: > The two functions monitor_traps and mem_access_send_req duplicate > some of the same functionality. The mem_access_send_req however leaves a > lot of the standard vm_event fields to be filled by other functions. > > Since mem_access events go on the monitor ring in this patch we consolidate > all paths to use monitor_traps to place events on the ring and to fill in > the common parts of the requests. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > xen/arch/arm/p2m.c | 69 +++++++++++++++++++-------------------- > xen/arch/x86/hvm/hvm.c | 16 ++++++--- > xen/arch/x86/hvm/monitor.c | 6 ++++ > xen/arch/x86/mm/p2m.c | 24 ++------------ > xen/common/mem_access.c | 11 ------- > xen/include/asm-x86/hvm/monitor.h | 2 ++ > xen/include/asm-x86/p2m.h | 13 +++++--- > xen/include/xen/mem_access.h | 7 ---- > 8 files changed, 63 insertions(+), 85 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d82349c..df898a3 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -5,7 +5,7 @@ > #include <xen/domain_page.h> > #include <xen/bitops.h> > #include <xen/vm_event.h> > -#include <xen/mem_access.h> > +#include <xen/monitor.h> > #include <xen/iocap.h> > #include <public/vm_event.h> > #include <asm/flushtlb.h> > @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) > smp_call_function(setup_virt_paging_one, (void *)val, 1); > } > > +static int > +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, > + xenmem_access_t xma) > +{ > + struct vcpu *v = current; > + vm_event_request_t req = {}; > + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; > + > + req.reason = VM_EVENT_REASON_MEM_ACCESS; > + > + /* Send request to mem access subscriber */ > + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; > + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > + if ( npfec.gla_valid ) > + { > + req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > + req.u.mem_access.gla = gla; > + > + if ( npfec.kind == npfec_kind_with_gla ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > + else if ( npfec.kind == npfec_kind_in_gpt ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > + } > + req.u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > + req.u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > + req.u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > + > + return monitor_traps(v, sync, &req); > +} > + > bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > { > int rc; > bool_t violation; > xenmem_access_t xma; > - vm_event_request_t *req; > struct vcpu *v = current; > struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > return false; > } > > - req = xzalloc(vm_event_request_t); > - if ( req ) > - { > - req->reason = VM_EVENT_REASON_MEM_ACCESS; > - > - /* Pause the current VCPU */ > - if ( xma != XENMEM_access_n2rwx ) > - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > - > - /* Send request to mem access subscriber */ > - req->u.mem_access.gfn = gpa >> PAGE_SHIFT; > - req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > - if ( npfec.gla_valid ) > - { > - req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > - req->u.mem_access.gla = gla; > - > - if ( npfec.kind == npfec_kind_with_gla ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > - else if ( npfec.kind == npfec_kind_in_gpt ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > - } > - req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > - req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > - req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > - req->vcpu_id = v->vcpu_id; > - > - mem_access_send_req(v->domain, req); > - xfree(req); > - } > - > - /* Pause the current VCPU */ > - if ( xma != XENMEM_access_n2rwx ) > - vm_event_vcpu_pause(v); > + if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 ) > + domain_crash(v->domain); This patch is doing more than it is claimed in the commit message. In general, moving the code and introducing changes within the same patch should really be avoided. So please split it in 2 patches. Regards,
On 28/07/2016 20:35, Tamas K Lengyel wrote: > The two functions monitor_traps and mem_access_send_req duplicate > some of the same functionality. The mem_access_send_req however leaves a > lot of the standard vm_event fields to be filled by other functions. > > Since mem_access events go on the monitor ring in this patch we consolidate > all paths to use monitor_traps to place events on the ring and to fill in > the common parts of the requests. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> Common and x86 bits Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, but a few suggestions. > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d82349c..df898a3 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) > smp_call_function(setup_virt_paging_one, (void *)val, 1); > } > > +static int > +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, > + xenmem_access_t xma) > +{ > + struct vcpu *v = current; > + vm_event_request_t req = {}; > + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; > + > + req.reason = VM_EVENT_REASON_MEM_ACCESS; > + > + /* Send request to mem access subscriber */ > + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; > + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); I see this is only code motion, but ~PAGE_MASK here instead of open-coding it. > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index daaee1d..688370d 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, > } > } > > - if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) ) > - { > + sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr); > + > + if ( !sync ) { Please keep this brace on the newline (inline with the style), even if it doesn't match the style of the else clause. > fall_through = 1; > } else { > - /* Rights not promoted, vcpu paused, work here is done */ > + /* Rights not promoted (aka. sync event), work here is done */ > rc = 1; > goto out_put_gfn; > } > @@ -1956,7 +1957,12 @@ out: > } > if ( req_ptr ) > { > - mem_access_send_req(currd, req_ptr); > + if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 ) > + { > + /* Crash the domain */ > + rc = 0; > + } It is reasonable to omit the braces here. > + > xfree(req_ptr); > } > return rc; > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index 7277c12..c7285c6 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length) > return monitor_traps(curr, 1, &req); > } > > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, vcpu *v. ~Andrew
On Thu, Jul 28, 2016 at 2:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 28/07/2016 20:35, Tamas K Lengyel wrote: >> The two functions monitor_traps and mem_access_send_req duplicate >> some of the same functionality. The mem_access_send_req however leaves a >> lot of the standard vm_event fields to be filled by other functions. >> >> Since mem_access events go on the monitor ring in this patch we consolidate >> all paths to use monitor_traps to place events on the ring and to fill in >> the common parts of the requests. >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> > > Common and x86 bits Reviewed-by: Andrew Cooper > <andrew.cooper3@citrix.com>, but a few suggestions. > >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d82349c..df898a3 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) >> smp_call_function(setup_virt_paging_one, (void *)val, 1); >> } >> >> +static int >> +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, >> + xenmem_access_t xma) >> +{ >> + struct vcpu *v = current; >> + vm_event_request_t req = {}; >> + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; >> + >> + req.reason = VM_EVENT_REASON_MEM_ACCESS; >> + >> + /* Send request to mem access subscriber */ >> + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; >> + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > > I see this is only code motion, but ~PAGE_MASK here instead of > open-coding it. Sounds good. > >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index daaee1d..688370d 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, >> } >> } >> >> - if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) ) >> - { >> + sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr); >> + >> + if ( !sync ) { > > Please keep this brace on the newline (inline with the style), even if > it doesn't match the style of the else clause. Sure. > >> fall_through = 1; >> } else { >> - /* Rights not promoted, vcpu paused, work here is done */ >> + /* Rights not promoted (aka. sync event), work here is done */ >> rc = 1; >> goto out_put_gfn; >> } >> @@ -1956,7 +1957,12 @@ out: >> } >> if ( req_ptr ) >> { >> - mem_access_send_req(currd, req_ptr); >> + if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 ) >> + { >> + /* Crash the domain */ >> + rc = 0; >> + } > > It is reasonable to omit the braces here. Yea but with the comment being in-between I just didn't like the look of it without the braces.. > >> + >> xfree(req_ptr); >> } >> return rc; >> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c >> index 7277c12..c7285c6 100644 >> --- a/xen/arch/x86/hvm/monitor.c >> +++ b/xen/arch/x86/hvm/monitor.c >> @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length) >> return monitor_traps(curr, 1, &req); >> } >> >> +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, > > vcpu *v. > > ~Andrew Thanks, Tamas
On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: > Hello Tamas, > > > On 28/07/2016 20:35, Tamas K Lengyel wrote: >> >> The two functions monitor_traps and mem_access_send_req duplicate >> some of the same functionality. The mem_access_send_req however leaves a >> lot of the standard vm_event fields to be filled by other functions. >> >> Since mem_access events go on the monitor ring in this patch we >> consolidate >> all paths to use monitor_traps to place events on the ring and to fill in >> the common parts of the requests. >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> --- >> xen/arch/arm/p2m.c | 69 >> +++++++++++++++++++-------------------- >> xen/arch/x86/hvm/hvm.c | 16 ++++++--- >> xen/arch/x86/hvm/monitor.c | 6 ++++ >> xen/arch/x86/mm/p2m.c | 24 ++------------ >> xen/common/mem_access.c | 11 ------- >> xen/include/asm-x86/hvm/monitor.h | 2 ++ >> xen/include/asm-x86/p2m.h | 13 +++++--- >> xen/include/xen/mem_access.h | 7 ---- >> 8 files changed, 63 insertions(+), 85 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d82349c..df898a3 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -5,7 +5,7 @@ >> #include <xen/domain_page.h> >> #include <xen/bitops.h> >> #include <xen/vm_event.h> >> -#include <xen/mem_access.h> >> +#include <xen/monitor.h> >> #include <xen/iocap.h> >> #include <public/vm_event.h> >> #include <asm/flushtlb.h> >> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) >> smp_call_function(setup_virt_paging_one, (void *)val, 1); >> } >> >> +static int >> +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec >> npfec, >> + xenmem_access_t xma) >> +{ >> + struct vcpu *v = current; >> + vm_event_request_t req = {}; >> + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; >> + >> + req.reason = VM_EVENT_REASON_MEM_ACCESS; >> + >> + /* Send request to mem access subscriber */ >> + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; >> + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); >> + if ( npfec.gla_valid ) >> + { >> + req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID; >> + req.u.mem_access.gla = gla; >> + >> + if ( npfec.kind == npfec_kind_with_gla ) >> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >> + else if ( npfec.kind == npfec_kind_in_gpt ) >> + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; >> + } >> + req.u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; >> + req.u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; >> + req.u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; >> + >> + return monitor_traps(v, sync, &req); >> +} >> + >> bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec >> npfec) >> { >> int rc; >> bool_t violation; >> xenmem_access_t xma; >> - vm_event_request_t *req; >> struct vcpu *v = current; >> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); >> >> @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t >> gla, const struct npfec npfec) >> return false; >> } >> >> - req = xzalloc(vm_event_request_t); >> - if ( req ) >> - { >> - req->reason = VM_EVENT_REASON_MEM_ACCESS; >> - >> - /* Pause the current VCPU */ >> - if ( xma != XENMEM_access_n2rwx ) >> - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; >> - >> - /* Send request to mem access subscriber */ >> - req->u.mem_access.gfn = gpa >> PAGE_SHIFT; >> - req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); >> - if ( npfec.gla_valid ) >> - { >> - req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; >> - req->u.mem_access.gla = gla; >> - >> - if ( npfec.kind == npfec_kind_with_gla ) >> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >> - else if ( npfec.kind == npfec_kind_in_gpt ) >> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; >> - } >> - req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : >> 0; >> - req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : >> 0; >> - req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : >> 0; >> - req->vcpu_id = v->vcpu_id; >> - >> - mem_access_send_req(v->domain, req); >> - xfree(req); >> - } >> - >> - /* Pause the current VCPU */ >> - if ( xma != XENMEM_access_n2rwx ) >> - vm_event_vcpu_pause(v); >> + if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 ) >> + domain_crash(v->domain); > > > This patch is doing more than it is claimed in the commit message. > > In general, moving the code and introducing changes within the same patch > should really be avoided. So please split it in 2 patches. Well, the changes are largely cosmetic so doing a whole separate patch IMHO is an overkill. How about adjusting the commit message to something like "sanitize code surrounding sending mem_access vm_events" to better describe the adjustments made in this patch? Tamas
On 07/28/2016 10:35 PM, Tamas K Lengyel wrote: > The two functions monitor_traps and mem_access_send_req duplicate > some of the same functionality. The mem_access_send_req however leaves a > lot of the standard vm_event fields to be filled by other functions. > > Since mem_access events go on the monitor ring in this patch we consolidate > all paths to use monitor_traps to place events on the ring and to fill in > the common parts of the requests. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > xen/arch/arm/p2m.c | 69 +++++++++++++++++++-------------------- > xen/arch/x86/hvm/hvm.c | 16 ++++++--- > xen/arch/x86/hvm/monitor.c | 6 ++++ > xen/arch/x86/mm/p2m.c | 24 ++------------ > xen/common/mem_access.c | 11 ------- > xen/include/asm-x86/hvm/monitor.h | 2 ++ > xen/include/asm-x86/p2m.h | 13 +++++--- > xen/include/xen/mem_access.h | 7 ---- > 8 files changed, 63 insertions(+), 85 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d82349c..df898a3 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -5,7 +5,7 @@ > #include <xen/domain_page.h> > #include <xen/bitops.h> > #include <xen/vm_event.h> > -#include <xen/mem_access.h> > +#include <xen/monitor.h> > #include <xen/iocap.h> > #include <public/vm_event.h> > #include <asm/flushtlb.h> > @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) > smp_call_function(setup_virt_paging_one, (void *)val, 1); > } > > +static int > +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, > + xenmem_access_t xma) > +{ > + struct vcpu *v = current; > + vm_event_request_t req = {}; > + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; > + > + req.reason = VM_EVENT_REASON_MEM_ACCESS; > + > + /* Send request to mem access subscriber */ > + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; > + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > + if ( npfec.gla_valid ) > + { > + req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > + req.u.mem_access.gla = gla; > + > + if ( npfec.kind == npfec_kind_with_gla ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > + else if ( npfec.kind == npfec_kind_in_gpt ) > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > + } > + req.u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > + req.u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > + req.u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > + > + return monitor_traps(v, sync, &req); > +} > + > bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > { > int rc; > bool_t violation; > xenmem_access_t xma; > - vm_event_request_t *req; > struct vcpu *v = current; > struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > return false; > } > > - req = xzalloc(vm_event_request_t); > - if ( req ) > - { > - req->reason = VM_EVENT_REASON_MEM_ACCESS; > - > - /* Pause the current VCPU */ > - if ( xma != XENMEM_access_n2rwx ) > - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > - > - /* Send request to mem access subscriber */ > - req->u.mem_access.gfn = gpa >> PAGE_SHIFT; > - req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > - if ( npfec.gla_valid ) > - { > - req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > - req->u.mem_access.gla = gla; > - > - if ( npfec.kind == npfec_kind_with_gla ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > - else if ( npfec.kind == npfec_kind_in_gpt ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > - } > - req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > - req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > - req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > - req->vcpu_id = v->vcpu_id; The line setting req->vcpu_id has been removed here ... > - > - mem_access_send_req(v->domain, req); > - xfree(req); > - } > - > - /* Pause the current VCPU */ > - if ( xma != XENMEM_access_n2rwx ) > - vm_event_vcpu_pause(v); > + if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 ) > + domain_crash(v->domain); > > return false; > } > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index daaee1d..688370d 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, > int rc, fall_through = 0, paged = 0; > int sharing_enomem = 0; > vm_event_request_t *req_ptr = NULL; > - bool_t ap2m_active; > + bool_t ap2m_active, sync = 0; > > /* On Nested Virtualization, walk the guest page table. > * If this succeeds, all is fine. > @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, > } > } > > - if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) ) > - { > + sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr); > + > + if ( !sync ) { > fall_through = 1; > } else { > - /* Rights not promoted, vcpu paused, work here is done */ > + /* Rights not promoted (aka. sync event), work here is done */ > rc = 1; > goto out_put_gfn; > } > @@ -1956,7 +1957,12 @@ out: > } > if ( req_ptr ) > { > - mem_access_send_req(currd, req_ptr); > + if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 ) > + { > + /* Crash the domain */ > + rc = 0; > + } > + > xfree(req_ptr); > } > return rc; > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > index 7277c12..c7285c6 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length) > return monitor_traps(curr, 1, &req); > } > > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, > + vm_event_request_t *req) > +{ > + return monitor_traps(v, sync, req); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index 812dbf6..27f9d26 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, > if ( req ) > { > *req_ptr = req; > - req->reason = VM_EVENT_REASON_MEM_ACCESS; > - > - /* Pause the current VCPU */ > - if ( p2ma != p2m_access_n2rwx ) > - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > - /* Send request to mem event */ > + req->reason = VM_EVENT_REASON_MEM_ACCESS; > req->u.mem_access.gfn = gfn; > req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > if ( npfec.gla_valid ) > @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, > req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > - req->vcpu_id = v->vcpu_id; ... and here, and as such it doesn't seem to get set anywhere else now. Am I missing an code path outside of this patch where req->vcpu_id is being correctly set so this has become unnecessary? Thanks, Razvan
On 28/07/16 23:54, Tamas K Lengyel wrote: > On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >> On 28/07/2016 20:35, Tamas K Lengyel wrote: >> This patch is doing more than it is claimed in the commit message. >> >> In general, moving the code and introducing changes within the same patch >> should really be avoided. So please split it in 2 patches. > > Well, the changes are largely cosmetic so doing a whole separate patch > IMHO is an overkill. How about adjusting the commit message to > something like "sanitize code surrounding sending mem_access > vm_events" to better describe the adjustments made in this patch? I think the wiki page "Submitting Xen Project patches" [1] should answer to your question. If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving code and at the same time as changing the behavior is fairly difficult to review because it hides the real modifications. Regards, [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: > > > > On 28/07/16 23:54, Tamas K Lengyel wrote: >> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >>> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote: >>> This patch is doing more than it is claimed in the commit message. >>> >>> In general, moving the code and introducing changes within the same patch >>> should really be avoided. So please split it in 2 patches. >> >> >> Well, the changes are largely cosmetic so doing a whole separate patch >> IMHO is an overkill. How about adjusting the commit message to >> something like "sanitize code surrounding sending mem_access >> vm_events" to better describe the adjustments made in this patch? > > > I think the wiki page "Submitting Xen Project patches" [1] should answer to your question. > > If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving code and at the same time as changing the behavior is fairly difficult to review because it hides the real modifications. > > Regards, > > [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches > The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of using monitor_traps as the rest just does not worth the effort. Tamas
On Jul 29, 2016 01:27, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote: > > On 07/28/2016 10:35 PM, Tamas K Lengyel wrote: > > The two functions monitor_traps and mem_access_send_req duplicate > > some of the same functionality. The mem_access_send_req however leaves a > > lot of the standard vm_event fields to be filled by other functions. > > > > Since mem_access events go on the monitor ring in this patch we consolidate > > all paths to use monitor_traps to place events on the ring and to fill in > > the common parts of the requests. > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > > --- > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > > Cc: George Dunlap <george.dunlap@eu.citrix.com> > > --- > > xen/arch/arm/p2m.c | 69 +++++++++++++++++++-------------------- > > xen/arch/x86/hvm/hvm.c | 16 ++++++--- > > xen/arch/x86/hvm/monitor.c | 6 ++++ > > xen/arch/x86/mm/p2m.c | 24 ++------------ > > xen/common/mem_access.c | 11 ------- > > xen/include/asm-x86/hvm/monitor.h | 2 ++ > > xen/include/asm-x86/p2m.h | 13 +++++--- > > xen/include/xen/mem_access.h | 7 ---- > > 8 files changed, 63 insertions(+), 85 deletions(-) > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > index d82349c..df898a3 100644 > > --- a/xen/arch/arm/p2m.c > > +++ b/xen/arch/arm/p2m.c > > @@ -5,7 +5,7 @@ > > #include <xen/domain_page.h> > > #include <xen/bitops.h> > > #include <xen/vm_event.h> > > -#include <xen/mem_access.h> > > +#include <xen/monitor.h> > > #include <xen/iocap.h> > > #include <public/vm_event.h> > > #include <asm/flushtlb.h> > > @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) > > smp_call_function(setup_virt_paging_one, (void *)val, 1); > > } > > > > +static int > > +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, > > + xenmem_access_t xma) > > +{ > > + struct vcpu *v = current; > > + vm_event_request_t req = {}; > > + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; > > + > > + req.reason = VM_EVENT_REASON_MEM_ACCESS; > > + > > + /* Send request to mem access subscriber */ > > + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; > > + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > > + if ( npfec.gla_valid ) > > + { > > + req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > > + req.u.mem_access.gla = gla; > > + > > + if ( npfec.kind == npfec_kind_with_gla ) > > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > > + else if ( npfec.kind == npfec_kind_in_gpt ) > > + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > > + } > > + req.u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > > + req.u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > > + req.u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > > + > > + return monitor_traps(v, sync, &req); > > +} > > + > > bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > > { > > int rc; > > bool_t violation; > > xenmem_access_t xma; > > - vm_event_request_t *req; > > struct vcpu *v = current; > > struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > > > > @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) > > return false; > > } > > > > - req = xzalloc(vm_event_request_t); > > - if ( req ) > > - { > > - req->reason = VM_EVENT_REASON_MEM_ACCESS; > > - > > - /* Pause the current VCPU */ > > - if ( xma != XENMEM_access_n2rwx ) > > - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > - > > - /* Send request to mem access subscriber */ > > - req->u.mem_access.gfn = gpa >> PAGE_SHIFT; > > - req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > > - if ( npfec.gla_valid ) > > - { > > - req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > > - req->u.mem_access.gla = gla; > > - > > - if ( npfec.kind == npfec_kind_with_gla ) > > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > > - else if ( npfec.kind == npfec_kind_in_gpt ) > > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > > - } > > - req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > > - req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > > - req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > > - req->vcpu_id = v->vcpu_id; > > The line setting req->vcpu_id has been removed here ... > > > - > > - mem_access_send_req(v->domain, req); > > - xfree(req); > > - } > > - > > - /* Pause the current VCPU */ > > - if ( xma != XENMEM_access_n2rwx ) > > - vm_event_vcpu_pause(v); > > + if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 ) > > + domain_crash(v->domain); > > > > return false; > > } > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > index daaee1d..688370d 100644 > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, > > int rc, fall_through = 0, paged = 0; > > int sharing_enomem = 0; > > vm_event_request_t *req_ptr = NULL; > > - bool_t ap2m_active; > > + bool_t ap2m_active, sync = 0; > > > > /* On Nested Virtualization, walk the guest page table. > > * If this succeeds, all is fine. > > @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, > > } > > } > > > > - if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) ) > > - { > > + sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr); > > + > > + if ( !sync ) { > > fall_through = 1; > > } else { > > - /* Rights not promoted, vcpu paused, work here is done */ > > + /* Rights not promoted (aka. sync event), work here is done */ > > rc = 1; > > goto out_put_gfn; > > } > > @@ -1956,7 +1957,12 @@ out: > > } > > if ( req_ptr ) > > { > > - mem_access_send_req(currd, req_ptr); > > + if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 ) > > + { > > + /* Crash the domain */ > > + rc = 0; > > + } > > + > > xfree(req_ptr); > > } > > return rc; > > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c > > index 7277c12..c7285c6 100644 > > --- a/xen/arch/x86/hvm/monitor.c > > +++ b/xen/arch/x86/hvm/monitor.c > > @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length) > > return monitor_traps(curr, 1, &req); > > } > > > > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, > > + vm_event_request_t *req) > > +{ > > + return monitor_traps(v, sync, req); > > +} > > + > > /* > > * Local variables: > > * mode: C > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > > index 812dbf6..27f9d26 100644 > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, > > if ( req ) > > { > > *req_ptr = req; > > - req->reason = VM_EVENT_REASON_MEM_ACCESS; > > - > > - /* Pause the current VCPU */ > > - if ( p2ma != p2m_access_n2rwx ) > > - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; > > > > - /* Send request to mem event */ > > + req->reason = VM_EVENT_REASON_MEM_ACCESS; > > req->u.mem_access.gfn = gfn; > > req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); > > if ( npfec.gla_valid ) > > @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, > > req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > > req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > > req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; > > - req->vcpu_id = v->vcpu_id; > > ... and here, and as such it doesn't seem to get set anywhere else now. > Am I missing an code path outside of this patch where req->vcpu_id is > being correctly set so this has become unnecessary? > Ah yes, I forgot to add that to monitor_traps. The idea is that common parts if all requests should be set there, including the vcpu_id so we can cut down on code duplication. Tamas
On 29/07/16 15:21, Tamas K Lengyel wrote: > > On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com > <mailto:julien.grall@arm.com>> wrote: > > > > > > > > On 28/07/16 23:54, Tamas K Lengyel wrote: > >> > >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com > <mailto:julien.grall@arm.com>> wrote: > >>> > >>> On 28/07/2016 20:35, Tamas K Lengyel wrote: > >>> This patch is doing more than it is claimed in the commit message. > >>> > >>> In general, moving the code and introducing changes within the > same patch > >>> should really be avoided. So please split it in 2 patches. > >> > >> > >> Well, the changes are largely cosmetic so doing a whole separate patch > >> IMHO is an overkill. How about adjusting the commit message to > >> something like "sanitize code surrounding sending mem_access > >> vm_events" to better describe the adjustments made in this patch? > > > > > > I think the wiki page "Submitting Xen Project patches" [1] should > answer to your question. > > > > If not, trivial patches are easy to review, merging multiple trivial > patches in a single patch is not. Moving code and at the same time as > changing the behavior is fairly difficult to review because it hides > the real modifications. > > > > Regards, > > > > [1] > http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches > > > > The behavior didn't change at all, this whole patch is code > sanitization. It's not worth doing a separate patch for each minor > change. The few change on the arm side is the vm_event request > allocation going from xzalloc to stack based and using monitor_traps > now in a split-out function. It really should be no problem reviewing > it. Even Andrew requested minor adjustments to be included in this > patch. Anyway, I'm not looking to change this into a series. If it's a > no go from your side I'm just going to cut down the ARM side > sanitization to the bare minimum of using monitor_traps as the rest > just does not worth the effort. > To offer an alternative opinion. Looking at this patch, I personally would have a hard time justifying breaking it down any further. Each of the changes are closely related. However, the commit message could be a lot clearer about which steps are taken. If I were writing the commit message, I would go with something a bit more like this: ----8<---- The two functions monitor_traps and mem_access_send_req duplicate some of the same functionality. The mem_access_send_req however leaves a lot of the standard vm_event fields to be filled by other functions. Remove mem_access_send_req() completely, making use of monitor_traps() to put requests into the monitor ring. This in turn causes some cleanup around the old callsites of mem_access_send_req(), and on ARM, the introduction of the __p2m_mem_access_send_req() helper to fill in common mem_access information. Finally, this change identifies that errors from mem_access_send_req() were never checked. As errors constitute a problem with the monitor ring, crashing the domain is the most appropriate action to take. ----8<---- If in doubt, always spell out each of the changes, and their logical relationships. If nothing else, it helps people trying to review the patch. ~Andrew
On Fri, 29 Jul 2016, Tamas K Lengyel wrote: > On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: > > > > > > > > On 28/07/16 23:54, Tamas K Lengyel wrote: > >> > >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: > >>> > >>> On 28/07/2016 20:35, Tamas K Lengyel wrote: > >>> This patch is doing more than it is claimed in the commit message. > >>> > >>> In general, moving the code and introducing changes within the same patch > >>> should really be avoided. So please split it in 2 patches. > >> > >> > >> Well, the changes are largely cosmetic so doing a whole separate patch > >> IMHO is an overkill. How about adjusting the commit message to > >> something like "sanitize code surrounding sending mem_access > >> vm_events" to better describe the adjustments made in this patch? > > > > > > I think the wiki page "Submitting Xen Project patches" [1] should answer to your question. > > > > If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving > code and at the same time as changing the behavior is fairly difficult to review because it hides the real > modifications. > > > > Regards, > > > > [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches > > > > The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch > for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to > stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even > Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a > series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of > using monitor_traps as the rest just does not worth the effort. Hi Tamas, let me premise that, like you wrote, the patch is just code sanitization, certainly not worth turning it into an argument. I think different maintainers have different styles. I for one always dislike to have code movement, renaming or code style fixes together with any other more meaningful changes. In fact when people suggest "could you please also rename this variable" or "could you please also move the function to common code", I usually reply: "I can, but it will be in another patch". So I agree with Julien that I would prefer to see two patches. In fact if I were you, I would prefer to write two separate patches because it would be less troubles for me as a developer. But clearly not everybody agrees with this style as I get requests for cosmetic changes together with other changes by many other seasoned maintainers. And that's OK, it just means that different people think differently, which is a good thing for the project as a whole. You are a valued contributor and maintainer of this project -- if you strongly feel this way, I am sure we can find a way to make this work anyway.
On Fri, Jul 29, 2016 at 10:27 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 29/07/16 15:21, Tamas K Lengyel wrote: > > On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: >> >> >> >> On 28/07/16 23:54, Tamas K Lengyel wrote: >>> >>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> >>> wrote: >>>> >>>> On 28/07/2016 20:35, Tamas K Lengyel wrote: >>>> This patch is doing more than it is claimed in the commit message. >>>> >>>> In general, moving the code and introducing changes within the same >>>> patch >>>> should really be avoided. So please split it in 2 patches. >>> >>> >>> Well, the changes are largely cosmetic so doing a whole separate patch >>> IMHO is an overkill. How about adjusting the commit message to >>> something like "sanitize code surrounding sending mem_access >>> vm_events" to better describe the adjustments made in this patch? >> >> >> I think the wiki page "Submitting Xen Project patches" [1] should answer >> to your question. >> >> If not, trivial patches are easy to review, merging multiple trivial >> patches in a single patch is not. Moving code and at the same time as >> changing the behavior is fairly difficult to review because it hides the >> real modifications. >> >> Regards, >> >> [1] >> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches >> > > The behavior didn't change at all, this whole patch is code sanitization. > It's not worth doing a separate patch for each minor change. The few change > on the arm side is the vm_event request allocation going from xzalloc to > stack based and using monitor_traps now in a split-out function. It really > should be no problem reviewing it. Even Andrew requested minor adjustments > to be included in this patch. Anyway, I'm not looking to change this into a > series. If it's a no go from your side I'm just going to cut down the ARM > side sanitization to the bare minimum of using monitor_traps as the rest > just does not worth the effort. > > > To offer an alternative opinion. > > Looking at this patch, I personally would have a hard time justifying > breaking it down any further. Each of the changes are closely related. > > However, the commit message could be a lot clearer about which steps are > taken. If I were writing the commit message, I would go with something a > bit more like this: > > ----8<---- > The two functions monitor_traps and mem_access_send_req duplicate some of > the same functionality. The mem_access_send_req however leaves a lot of the > standard vm_event fields to be filled by other functions. > > Remove mem_access_send_req() completely, making use of monitor_traps() to > put requests into the monitor ring. This in turn causes some cleanup around > the old callsites of mem_access_send_req(), and on ARM, the introduction of > the __p2m_mem_access_send_req() helper to fill in common mem_access > information. > > Finally, this change identifies that errors from mem_access_send_req() were > never checked. As errors constitute a problem with the monitor ring, > crashing the domain is the most appropriate action to take. > ----8<---- > > If in doubt, always spell out each of the changes, and their logical > relationships. If nothing else, it helps people trying to review the patch. > Thanks and I agree, adjusting the commit message is what I would think would make sense as well here (and what I offered as an alternative to breaking down the series into tiny patches). Tamas
On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Fri, 29 Jul 2016, Tamas K Lengyel wrote: >> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: >> > >> > >> > >> > On 28/07/16 23:54, Tamas K Lengyel wrote: >> >> >> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >> >>> >> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote: >> >>> This patch is doing more than it is claimed in the commit message. >> >>> >> >>> In general, moving the code and introducing changes within the same patch >> >>> should really be avoided. So please split it in 2 patches. >> >> >> >> >> >> Well, the changes are largely cosmetic so doing a whole separate patch >> >> IMHO is an overkill. How about adjusting the commit message to >> >> something like "sanitize code surrounding sending mem_access >> >> vm_events" to better describe the adjustments made in this patch? >> > >> > >> > I think the wiki page "Submitting Xen Project patches" [1] should answer to your question. >> > >> > If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving >> code and at the same time as changing the behavior is fairly difficult to review because it hides the real >> modifications. >> > >> > Regards, >> > >> > [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches >> > >> >> The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch >> for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to >> stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even >> Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a >> series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of >> using monitor_traps as the rest just does not worth the effort. > > Hi Tamas, > let me premise that, like you wrote, the patch is just code > sanitization, certainly not worth turning it into an argument. > > I think different maintainers have different styles. I for one always > dislike to have code movement, renaming or code style fixes together > with any other more meaningful changes. In fact when people suggest > "could you please also rename this variable" or "could you please also > move the function to common code", I usually reply: "I can, but it will > be in another patch". > > So I agree with Julien that I would prefer to see two patches. In fact > if I were you, I would prefer to write two separate patches because it > would be less troubles for me as a developer. But clearly not everybody > agrees with this style as I get requests for cosmetic changes together > with other changes by many other seasoned maintainers. And that's OK, it > just means that different people think differently, which is a good > thing for the project as a whole. > > You are a valued contributor and maintainer of this project -- if you > strongly feel this way, I am sure we can find a way to make this work > anyway. I would highly appreciate that as I said it's just not worth the time it takes to break this down into smaller patches. It really doubles the effort it takes to test it. It's within your and Julien's right to not accept such patches touching your code-base as Maintainer and that's fine. If that's the case though we might want to look into adjusting the layout of the code so that mem_access/monitor/vm_event components are more isolated into separate files so that we can move at a different phase and different style then the rest of the code here without getting into arguments about that stuff. Thanks, Tamas
On 29/07/2016 22:02, Tamas K Lengyel wrote: > On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini > <sstabellini@kernel.org> wrote: >> On Fri, 29 Jul 2016, Tamas K Lengyel wrote: >>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: >>>> >>>> >>>> >>>> On 28/07/16 23:54, Tamas K Lengyel wrote: >>>>> >>>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote: >>>>>> >>>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote: >>>>>> This patch is doing more than it is claimed in the commit message. >>>>>> >>>>>> In general, moving the code and introducing changes within the same patch >>>>>> should really be avoided. So please split it in 2 patches. >>>>> >>>>> >>>>> Well, the changes are largely cosmetic so doing a whole separate patch >>>>> IMHO is an overkill. How about adjusting the commit message to >>>>> something like "sanitize code surrounding sending mem_access >>>>> vm_events" to better describe the adjustments made in this patch? >>>> >>>> >>>> I think the wiki page "Submitting Xen Project patches" [1] should answer to your question. >>>> >>>> If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving >>> code and at the same time as changing the behavior is fairly difficult to review because it hides the real >>> modifications. >>>> >>>> Regards, >>>> >>>> [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches >>>> >>> >>> The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch >>> for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to >>> stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even >>> Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a >>> series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of >>> using monitor_traps as the rest just does not worth the effort. >> >> Hi Tamas, >> let me premise that, like you wrote, the patch is just code >> sanitization, certainly not worth turning it into an argument. >> >> I think different maintainers have different styles. I for one always >> dislike to have code movement, renaming or code style fixes together >> with any other more meaningful changes. In fact when people suggest >> "could you please also rename this variable" or "could you please also >> move the function to common code", I usually reply: "I can, but it will >> be in another patch". >> >> So I agree with Julien that I would prefer to see two patches. In fact >> if I were you, I would prefer to write two separate patches because it >> would be less troubles for me as a developer. But clearly not everybody >> agrees with this style as I get requests for cosmetic changes together >> with other changes by many other seasoned maintainers. And that's OK, it >> just means that different people think differently, which is a good >> thing for the project as a whole. >> >> You are a valued contributor and maintainer of this project -- if you >> strongly feel this way, I am sure we can find a way to make this work >> anyway. > > I would highly appreciate that as I said it's just not worth the time > it takes to break this down into smaller patches. It really doubles > the effort it takes to test it. I find this paragraph really offensive for reviewers. This requires more efforts for reviewers to review a such patch. My time is as valuable as yours. I hope you will reconsider what you've written. Regards, > > Thanks, > Tamas >
On Fri, Jul 29, 2016 at 3:38 PM, Julien Grall <julien.grall@arm.com> wrote: > > > On 29/07/2016 22:02, Tamas K Lengyel wrote: >> >> On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini >> <sstabellini@kernel.org> wrote: >>> >>> On Fri, 29 Jul 2016, Tamas K Lengyel wrote: >>>> >>>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> On 28/07/16 23:54, Tamas K Lengyel wrote: >>>>>> >>>>>> >>>>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote: >>>>>>> This patch is doing more than it is claimed in the commit message. >>>>>>> >>>>>>> In general, moving the code and introducing changes within the same >>>>>>> patch >>>>>>> should really be avoided. So please split it in 2 patches. >>>>>> >>>>>> >>>>>> >>>>>> Well, the changes are largely cosmetic so doing a whole separate patch >>>>>> IMHO is an overkill. How about adjusting the commit message to >>>>>> something like "sanitize code surrounding sending mem_access >>>>>> vm_events" to better describe the adjustments made in this patch? >>>>> >>>>> >>>>> >>>>> I think the wiki page "Submitting Xen Project patches" [1] should >>>>> answer to your question. >>>>> >>>>> If not, trivial patches are easy to review, merging multiple trivial >>>>> patches in a single patch is not. Moving >>>> >>>> code and at the same time as changing the behavior is fairly difficult >>>> to review because it hides the real >>>> modifications. >>>>> >>>>> >>>>> Regards, >>>>> >>>>> [1] >>>>> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches >>>>> >>>> >>>> The behavior didn't change at all, this whole patch is code >>>> sanitization. It's not worth doing a separate patch >>>> for each minor change. The few change on the arm side is the vm_event >>>> request allocation going from xzalloc to >>>> stack based and using monitor_traps now in a split-out function. It >>>> really should be no problem reviewing it. Even >>>> Andrew requested minor adjustments to be included in this patch. Anyway, >>>> I'm not looking to change this into a >>>> series. If it's a no go from your side I'm just going to cut down the >>>> ARM side sanitization to the bare minimum of >>>> using monitor_traps as the rest just does not worth the effort. >>> >>> >>> Hi Tamas, >>> let me premise that, like you wrote, the patch is just code >>> sanitization, certainly not worth turning it into an argument. >>> >>> I think different maintainers have different styles. I for one always >>> dislike to have code movement, renaming or code style fixes together >>> with any other more meaningful changes. In fact when people suggest >>> "could you please also rename this variable" or "could you please also >>> move the function to common code", I usually reply: "I can, but it will >>> be in another patch". >>> >>> So I agree with Julien that I would prefer to see two patches. In fact >>> if I were you, I would prefer to write two separate patches because it >>> would be less troubles for me as a developer. But clearly not everybody >>> agrees with this style as I get requests for cosmetic changes together >>> with other changes by many other seasoned maintainers. And that's OK, it >>> just means that different people think differently, which is a good >>> thing for the project as a whole. >>> >>> You are a valued contributor and maintainer of this project -- if you >>> strongly feel this way, I am sure we can find a way to make this work >>> anyway. >> >> >> I would highly appreciate that as I said it's just not worth the time >> it takes to break this down into smaller patches. It really doubles >> the effort it takes to test it. > > > I find this paragraph really offensive for reviewers. This requires more > efforts for reviewers to review a such patch. My time is as valuable as > yours. I hope you will reconsider what you've written. > I don't know which part you find offensive and I certainly didn't mean to offend anyone. I do value your time. I understand this may require a bit more to review but honestly, I don't think the difference from your side should be significant. From my side it is and it's not worth the extra effort to go rounds about this and do a whole series so I'll just drop the portions you are not happy about. It is your right as maintainer to reject it as this code right now lives in a generic part of the ARM code-base. However, as this patch really only touches things that belong to mem_access/monitor components maybe we should split these from the generic ARM code altogether to avoid such conflicts in the future. Tamas
Hello Tamas, On 29/07/16 23:26, Tamas K Lengyel wrote: > However, as this patch really only touches > things that belong to mem_access/monitor components maybe we should > split these from the generic ARM code altogether to avoid such > conflicts in the future. I am not against moving part of mem_access outside of the P2M. However, the ARM (32-bit and 64-bit) code needs a lot more attention to make it work in *all* the case. I was able to break memaccess quite easily on various platform (include models). For instance p2m_mem_access_check_and_get_page is using the hardware to translate a VA to an IPA. This only works if the memory where the stage-1 page tables reside is not protected. The upcoming support of altp2m will make things trickier. The interface of p2m_mem_access_check_and_get_page should also be updated to take a VCPU in parameter. Lastly getting/setting access in the the mem_access_settings radix tree should stay in the p2m code as it is tight to stage-2 page table implementation. Regards,
On Mon, Aug 1, 2016 at 4:33 AM, Julien Grall <julien.grall@arm.com> wrote: > Hello Tamas, > > On 29/07/16 23:26, Tamas K Lengyel wrote: >> >> However, as this patch really only touches >> things that belong to mem_access/monitor components maybe we should >> split these from the generic ARM code altogether to avoid such >> conflicts in the future. > > > I am not against moving part of mem_access outside of the P2M. However, the > ARM (32-bit and 64-bit) code needs a lot more attention to make it work in > *all* the case. I was able to break memaccess quite easily on various > platform (include models). Yeap, we were able to reproduce the issue on our XU as well. It fortunately only occurs in a somewhat less-used part of mem_access where the user changes the default_access and also doesn't pause the domain while setting mem_access. If either of those conditions aren't true the bug is avoided. I'm not aware of anyone actually using default_access at this point, it is a legacy option present in mem_access for a now unknown use-case. > > For instance p2m_mem_access_check_and_get_page is using the hardware to > translate a VA to an IPA. This only works if the memory where the stage-1 > page tables reside is not protected. The upcoming support of altp2m will > make things trickier. > > The interface of p2m_mem_access_check_and_get_page should also be updated to > take a VCPU in parameter. Agree. > > Lastly getting/setting access in the the mem_access_settings radix tree > should stay in the p2m code as it is tight to stage-2 page table > implementation. Keeping the radix bookkeeping in p2m should be fine from our perspective. We will need the p2m lock functions accessible though from outside p2m (as well as the p2m_{set/get}_entry functions). Thanks, Tamas
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d82349c..df898a3 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -5,7 +5,7 @@ #include <xen/domain_page.h> #include <xen/bitops.h> #include <xen/vm_event.h> -#include <xen/mem_access.h> +#include <xen/monitor.h> #include <xen/iocap.h> #include <public/vm_event.h> #include <asm/flushtlb.h> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void) smp_call_function(setup_virt_paging_one, (void *)val, 1); } +static int +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec, + xenmem_access_t xma) +{ + struct vcpu *v = current; + vm_event_request_t req = {}; + bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1; + + req.reason = VM_EVENT_REASON_MEM_ACCESS; + + /* Send request to mem access subscriber */ + req.u.mem_access.gfn = gpa >> PAGE_SHIFT; + req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); + if ( npfec.gla_valid ) + { + req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID; + req.u.mem_access.gla = gla; + + if ( npfec.kind == npfec_kind_with_gla ) + req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; + else if ( npfec.kind == npfec_kind_in_gpt ) + req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; + } + req.u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; + req.u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; + req.u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; + + return monitor_traps(v, sync, &req); +} + bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) { int rc; bool_t violation; xenmem_access_t xma; - vm_event_request_t *req; struct vcpu *v = current; struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) return false; } - req = xzalloc(vm_event_request_t); - if ( req ) - { - req->reason = VM_EVENT_REASON_MEM_ACCESS; - - /* Pause the current VCPU */ - if ( xma != XENMEM_access_n2rwx ) - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; - - /* Send request to mem access subscriber */ - req->u.mem_access.gfn = gpa >> PAGE_SHIFT; - req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); - if ( npfec.gla_valid ) - { - req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; - req->u.mem_access.gla = gla; - - if ( npfec.kind == npfec_kind_with_gla ) - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; - else if ( npfec.kind == npfec_kind_in_gpt ) - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; - } - req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; - req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; - req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; - req->vcpu_id = v->vcpu_id; - - mem_access_send_req(v->domain, req); - xfree(req); - } - - /* Pause the current VCPU */ - if ( xma != XENMEM_access_n2rwx ) - vm_event_vcpu_pause(v); + if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 ) + domain_crash(v->domain); return false; } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index daaee1d..688370d 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, int rc, fall_through = 0, paged = 0; int sharing_enomem = 0; vm_event_request_t *req_ptr = NULL; - bool_t ap2m_active; + bool_t ap2m_active, sync = 0; /* On Nested Virtualization, walk the guest page table. * If this succeeds, all is fine. @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla, } } - if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) ) - { + sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr); + + if ( !sync ) { fall_through = 1; } else { - /* Rights not promoted, vcpu paused, work here is done */ + /* Rights not promoted (aka. sync event), work here is done */ rc = 1; goto out_put_gfn; } @@ -1956,7 +1957,12 @@ out: } if ( req_ptr ) { - mem_access_send_req(currd, req_ptr); + if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 ) + { + /* Crash the domain */ + rc = 0; + } + xfree(req_ptr); } return rc; diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 7277c12..c7285c6 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length) return monitor_traps(curr, 1, &req); } +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, + vm_event_request_t *req) +{ + return monitor_traps(v, sync, req); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 812dbf6..27f9d26 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, if ( req ) { *req_ptr = req; - req->reason = VM_EVENT_REASON_MEM_ACCESS; - - /* Pause the current VCPU */ - if ( p2ma != p2m_access_n2rwx ) - req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; - /* Send request to mem event */ + req->reason = VM_EVENT_REASON_MEM_ACCESS; req->u.mem_access.gfn = gfn; req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1); if ( npfec.gla_valid ) @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; - req->vcpu_id = v->vcpu_id; - - vm_event_fill_regs(req); - - if ( altp2m_active(v->domain) ) - { - req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M; - req->altp2m_idx = vcpu_altp2m(v).p2midx; - } } - /* Pause the current VCPU */ - if ( p2ma != p2m_access_n2rwx ) - vm_event_vcpu_pause(v); - - /* VCPU may be paused, return whether we promoted automatically */ - return (p2ma == p2m_access_n2rwx); + /* Return whether vCPU pause is required (aka. sync event) */ + return (p2ma != p2m_access_n2rwx); } static inline diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index b4033f0..82f4bad 100644 --- a/xen/common/mem_access.c +++ b/xen/common/mem_access.c @@ -108,17 +108,6 @@ int mem_access_memop(unsigned long cmd, return rc; } -int mem_access_send_req(struct domain *d, vm_event_request_t *req) -{ - int rc = vm_event_claim_slot(d, &d->vm_event->monitor); - if ( rc < 0 ) - return rc; - - vm_event_put_request(d, &d->vm_event->monitor, req); - - return 0; -} - /* * Local variables: * mode: C diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index a92f3fc..52c1f47 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -41,6 +41,8 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value); int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, unsigned long trap_type, unsigned long insn_length); int hvm_monitor_cpuid(unsigned long insn_length); +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, + vm_event_request_t *req); #endif /* __ASM_X86_HVM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 194020e..f4a746f 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -660,11 +660,14 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long gfn, uint64_t buffer); /* Resume normal operation (in case a domain was paused) */ void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp); -/* Send mem event based on the access (gla is -1ull if not available). Handles - * the rw2rx conversion. Boolean return value indicates if access rights have - * been promoted with no underlying vcpu pause. If the req_ptr has been populated, - * then the caller must put the event in the ring (once having released get_gfn* - * locks -- caller must also xfree the request. */ +/* + * Setup vm_event request based on the access (gla is -1ull if not available). + * Handles the rw2rx conversion. Boolean return value indicates if event type + * is syncronous (aka. requires vCPU pause). If the req_ptr has been populated, + * then the caller should use monitor_traps to send the event on the MONITOR + * ring. Once having released get_gfn* locks caller must also xfree the + * request. + */ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, struct npfec npfec, vm_event_request_t **req_ptr); diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 272f1e4..3d054e0 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -29,7 +29,6 @@ int mem_access_memop(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg); -int mem_access_send_req(struct domain *d, vm_event_request_t *req); static inline void mem_access_resume(struct vcpu *v, vm_event_response_t *rsp) @@ -47,12 +46,6 @@ int mem_access_memop(unsigned long cmd, } static inline -int mem_access_send_req(struct domain *d, vm_event_request_t *req) -{ - return -ENOSYS; -} - -static inline void mem_access_resume(struct vcpu *vcpu, vm_event_response_t *rsp) { /* Nothing to do. */
The two functions monitor_traps and mem_access_send_req duplicate some of the same functionality. The mem_access_send_req however leaves a lot of the standard vm_event fields to be filled by other functions. Since mem_access events go on the monitor ring in this patch we consolidate all paths to use monitor_traps to place events on the ring and to fill in the common parts of the requests. Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> --- xen/arch/arm/p2m.c | 69 +++++++++++++++++++-------------------- xen/arch/x86/hvm/hvm.c | 16 ++++++--- xen/arch/x86/hvm/monitor.c | 6 ++++ xen/arch/x86/mm/p2m.c | 24 ++------------ xen/common/mem_access.c | 11 ------- xen/include/asm-x86/hvm/monitor.h | 2 ++ xen/include/asm-x86/p2m.h | 13 +++++--- xen/include/xen/mem_access.h | 7 ---- 8 files changed, 63 insertions(+), 85 deletions(-)