[Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
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(). We also update monitor_traps to now include setting the common vcpu_id field so that all other call-sites can ommit this step. 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. Signed-off-by: Tamas K Lengyel Reviewed-by: Andrew Cooper Acked-by: Razvan Cojocaru --- Cc: Stefano Stabellini Cc: Julien Grall Cc: Jan Beulich Cc: George Dunlap v3: reduce the code movement and sanitization performed to a minimum --- xen/arch/arm/p2m.c | 15 --- xen/arch/x86/hvm/hvm.c | 18 -- xen/arch/x86/hvm/monitor.c | 5 - xen/arch/x86/mm/p2m.c| 26 +- xen/common/mem_access.c | 11 --- xen/common/monitor.c | 2 ++ xen/include/asm-x86/p2m.h| 13 - xen/include/xen/mem_access.h | 7 --- 8 files changed, 31 insertions(+), 66 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 40a0b80..a3f05b4 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -5,7 +5,7 @@ #include #include #include -#include +#include #include #include #include @@ -1740,10 +1740,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) { 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); @@ -1760,16 +1756,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) 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); +if ( monitor_traps(v, (xma != XENMEM_access_n2rwx), req) < 0 ) +domain_crash(v->domain); + xfree(req); } -/* Pause the current VCPU */ -if ( xma != XENMEM_access_n2rwx ) -vm_event_vcpu_pause(v); - return false; } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index daaee1d..42f163e 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,15 @@ 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 */ +else +{ +/* + * Rights not promoted (aka. sync event), work here is done + */ rc = 1; goto out_put_gfn; } @@ -1956,7 +1960,9 @@ out: } if ( req_ptr ) { -mem_access_send_req(currd, req_ptr); +if ( monitor_traps(curr, sync, req_ptr) < 0 ) +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..0f6ef96 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -44,7 +44,6 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old vm_event_request_t req = { .reason = VM_EVENT_REASON_WRITE_CTRLREG, -.vcpu_id = curr->vcpu_id, .u.write_ctrlreg.index = index, .u.write_ctrlreg.new_value = value, .u.write_ctrlreg.old_value = old @@ -65,7 +64,6 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value) { vm_event_request_t req = { .reason = VM_EVENT_REASON_MOV_TO_MSR, -.vcpu_id = curr->vcpu_id, .u.mov_to_msr.msr
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
>>> On 03.08.16 at 20:41, 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. > > 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(). We also update monitor_traps to now > include setting the common vcpu_id field so that all other call-sites can > ommit > this step. > > 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. > > Signed-off-by: Tamas K Lengyel > Reviewed-by: Andrew Cooper > Acked-by: Razvan Cojocaru > --- > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Jan Beulich > Cc: George Dunlap > > v3: reduce the code movement and sanitization performed to a minimum Doesn't this invalidate prior reviews and acks? > @@ -1846,11 +1846,15 @@ 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 */ > +else > +{ > +/* > + * Rights not promoted (aka. sync event), work here is done > + */ Comment style. > @@ -1750,23 +1745,12 @@ 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) > + */ Again. > +return (p2ma != p2m_access_n2rwx); Pointless parentheses. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: On 03.08.16 at 20:41, 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. >> >> 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(). We also update monitor_traps to now >> include setting the common vcpu_id field so that all other call-sites can >> ommit >> this step. >> >> 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. >> >> Signed-off-by: Tamas K Lengyel >> Reviewed-by: Andrew Cooper >> Acked-by: Razvan Cojocaru >> --- >> Cc: Stefano Stabellini >> Cc: Julien Grall >> Cc: Jan Beulich >> Cc: George Dunlap >> >> v3: reduce the code movement and sanitization performed to a minimum > > Doesn't this invalidate prior reviews and acks? I don't think so, the difference is pretty much cosmetic. > >> @@ -1846,11 +1846,15 @@ 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 */ >> +else >> +{ >> +/* >> + * Rights not promoted (aka. sync event), work here is done >> + */ > > Comment style. Alright, you had an issue with the pre-existing comment style but you also have an issue with this. What exactly is the issue here? > >> @@ -1750,23 +1745,12 @@ 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) >> + */ > > Again. Again.. > >> +return (p2ma != p2m_access_n2rwx); > > Pointless parentheses. > The parentheses were already there, I'm trying no to do style adjustments in this patch. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
>>> On 04.08.16 at 08:36, wrote: > On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: > On 03.08.16 at 20:41, wrote: >>> @@ -1846,11 +1846,15 @@ 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 */ >>> +else >>> +{ >>> +/* >>> + * Rights not promoted (aka. sync event), work here is done >>> + */ >> >> Comment style. > > Alright, you had an issue with the pre-existing comment style but you > also have an issue with this. What exactly is the issue here? See ./CODING_STYLE: This is a single line comment, and what was and is missing is the full stop at the end. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On 04/08/16 08:51, Jan Beulich wrote: On 04.08.16 at 08:36, wrote: >> On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: >> On 03.08.16 at 20:41, wrote: @@ -1846,11 +1846,15 @@ 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 */ +else +{ +/* + * Rights not promoted (aka. sync event), work here is done + */ >>> >>> Comment style. >> >> Alright, you had an issue with the pre-existing comment style but you >> also have an issue with this. What exactly is the issue here? > > See ./CODING_STYLE: This is a single line comment, and what was > and is missing is the full stop at the end. Maybe we should have a larger discussion about this, but I think that the insistence on having a full stop is going too far. It has absolutely zero effect on readability or legibility; it creates literally zero additional value, and so cannot possibly be worth the time it takes you to write the sentence, or even the electricity it takes to send that sentence over the internet -- much less this coy sort of "There's an unspecified problem with this comment" resulting in a back-and-forth like this. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
>>> On 04.08.16 at 11:25, wrote: > On 04/08/16 08:51, Jan Beulich wrote: > On 04.08.16 at 08:36, wrote: >>> On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: >>> On 03.08.16 at 20:41, wrote: > @@ -1846,11 +1846,15 @@ 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 */ > +else > +{ > +/* > + * Rights not promoted (aka. sync event), work here is > done > + */ Comment style. >>> >>> Alright, you had an issue with the pre-existing comment style but you >>> also have an issue with this. What exactly is the issue here? >> >> See ./CODING_STYLE: This is a single line comment, and what was >> and is missing is the full stop at the end. > > Maybe we should have a larger discussion about this, but I think that > the insistence on having a full stop is going too far. It has > absolutely zero effect on readability or legibility; it creates > literally zero additional value, and so cannot possibly be worth the > time it takes you to write the sentence, or even the electricity it > takes to send that sentence over the internet -- much less this coy sort > of "There's an unspecified problem with this comment" resulting in a > back-and-forth like this. I certainly wouldn't heavily object to that part of the comment requirements getting dropped, but I think the full stop serves a purpose just like it does in plain written text. I would be in favor of allowing comments which aren't (and clearly aren't meant to be) full sentences, and hence shouldn't have a full stop at their end, yet I don't think that would apply in the two specific cases here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On 04/08/16 10:36, Jan Beulich wrote: On 04.08.16 at 11:25, wrote: >> On 04/08/16 08:51, Jan Beulich wrote: >> On 04.08.16 at 08:36, wrote: On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: On 03.08.16 at 20:41, wrote: >> @@ -1846,11 +1846,15 @@ 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 >> */ >> +else >> +{ >> +/* >> + * Rights not promoted (aka. sync event), work here is >> done >> + */ > > Comment style. Alright, you had an issue with the pre-existing comment style but you also have an issue with this. What exactly is the issue here? >>> >>> See ./CODING_STYLE: This is a single line comment, and what was >>> and is missing is the full stop at the end. >> >> Maybe we should have a larger discussion about this, but I think that >> the insistence on having a full stop is going too far. It has >> absolutely zero effect on readability or legibility; it creates >> literally zero additional value, and so cannot possibly be worth the >> time it takes you to write the sentence, or even the electricity it >> takes to send that sentence over the internet -- much less this coy sort >> of "There's an unspecified problem with this comment" resulting in a >> back-and-forth like this. > > I certainly wouldn't heavily object to that part of the comment > requirements getting dropped, but I think the full stop serves a > purpose just like it does in plain written text. I would be in favor > of allowing comments which aren't (and clearly aren't meant to > be) full sentences, and hence shouldn't have a full stop at their > end, yet I don't think that would apply in the two specific cases > here. I think the issue here as that to me, these kinds of comments are not "plain written text"; they're more like signs or labels. If you search Google Images for "sign" or "clothing label", you'll find that no single-sentence signs have full stops at the end. That's why to me it actually feels like having a full stop at the end of a comment like this is actively wrong -- it changes it from one type of speech into another type of speech. I wouldn't bike-shed over the issue by insisting a full stop be removed; but it would definitely irritate me to be told that I had to add one. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On Thu, Aug 4, 2016 at 1:51 AM, Jan Beulich wrote: On 04.08.16 at 08:36, wrote: >> On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: >> On 03.08.16 at 20:41, wrote: @@ -1846,11 +1846,15 @@ 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 */ +else +{ +/* + * Rights not promoted (aka. sync event), work here is done + */ >>> >>> Comment style. >> >> Alright, you had an issue with the pre-existing comment style but you >> also have an issue with this. What exactly is the issue here? > > See ./CODING_STYLE: This is a single line comment, and what was > and is missing is the full stop at the end. > Well, thanks for spotting it but I'm not going to resend resend the patch for this. If that's seriously all that's missing and you feel really strongly about it things like that have routinely been corrected when the patch is applied. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
>>> On 04.08.16 at 18:16, wrote: > On Thu, Aug 4, 2016 at 1:51 AM, Jan Beulich wrote: > On 04.08.16 at 08:36, wrote: >>> On Thu, Aug 4, 2016 at 12:29 AM, Jan Beulich wrote: >>> On 03.08.16 at 20:41, wrote: > @@ -1846,11 +1846,15 @@ 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 */ > +else > +{ > +/* > + * Rights not promoted (aka. sync event), work here is > done > + */ Comment style. >>> >>> Alright, you had an issue with the pre-existing comment style but you >>> also have an issue with this. What exactly is the issue here? >> >> See ./CODING_STYLE: This is a single line comment, and what was >> and is missing is the full stop at the end. > > Well, thanks for spotting it but I'm not going to resend resend the > patch for this. If that's seriously all that's missing and you feel > really strongly about it things like that have routinely been > corrected when the patch is applied. Sure, no problem if I end up committing it. As it stands, it's not fully acked, so I can't apply it just yet. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On Wed, Aug 3, 2016 at 12:41 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. > > 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(). We also update monitor_traps to now > include setting the common vcpu_id field so that all other call-sites can > ommit > this step. > > 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. > > Signed-off-by: Tamas K Lengyel > Reviewed-by: Andrew Cooper > Acked-by: Razvan Cojocaru > --- > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Jan Beulich > Cc: George Dunlap > > v3: reduce the code movement and sanitization performed to a minimum Patch ping. Needs an ARM ack (George has acked v2 for x86 that I forgot to update in the patch message). Tamas > --- > xen/arch/arm/p2m.c | 15 --- > xen/arch/x86/hvm/hvm.c | 18 -- > xen/arch/x86/hvm/monitor.c | 5 - > xen/arch/x86/mm/p2m.c| 26 +- > xen/common/mem_access.c | 11 --- > xen/common/monitor.c | 2 ++ > xen/include/asm-x86/p2m.h| 13 - > xen/include/xen/mem_access.h | 7 --- > 8 files changed, 31 insertions(+), 66 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 40a0b80..a3f05b4 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -5,7 +5,7 @@ > #include > #include > #include > -#include > +#include > #include > #include > #include > @@ -1740,10 +1740,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, > const struct npfec npfec) > { > 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); > @@ -1760,16 +1756,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, > const struct npfec npfec) > 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); > +if ( monitor_traps(v, (xma != XENMEM_access_n2rwx), req) < 0 ) > +domain_crash(v->domain); > + > xfree(req); > } > > -/* Pause the current VCPU */ > -if ( xma != XENMEM_access_n2rwx ) > -vm_event_vcpu_pause(v); > - > return false; > } > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index daaee1d..42f163e 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,15 @@ 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 */ > +else > +{ > +/* > + * Rights not promoted (aka. sync event), work here is done > + */ > rc = 1; > goto out_put_gfn; > } > @@ -1956,7 +1960,9 @@ out: > } > if ( req_ptr ) > { > -mem_access_send_req(currd, req_ptr); > +if ( monitor_traps(curr, sync, req_ptr) < 0 ) > +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..0f6ef96 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -44,7 +44,6 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long > value, unsigned long old > > vm_event_request_t req = { > .reason = VM_EVENT_REASON_WRI
Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request
On Wed, 3 Aug 2016, 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. > > 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(). We also update monitor_traps to now > include setting the common vcpu_id field so that all other call-sites can > ommit > this step. > > 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. > > Signed-off-by: Tamas K Lengyel > Reviewed-by: Andrew Cooper > Acked-by: Razvan Cojocaru The ARM bits: Acked-by: Stefano Stabellini > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Jan Beulich > Cc: George Dunlap > > v3: reduce the code movement and sanitization performed to a minimum > --- > xen/arch/arm/p2m.c | 15 --- > xen/arch/x86/hvm/hvm.c | 18 -- > xen/arch/x86/hvm/monitor.c | 5 - > xen/arch/x86/mm/p2m.c| 26 +- > xen/common/mem_access.c | 11 --- > xen/common/monitor.c | 2 ++ > xen/include/asm-x86/p2m.h| 13 - > xen/include/xen/mem_access.h | 7 --- > 8 files changed, 31 insertions(+), 66 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 40a0b80..a3f05b4 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -5,7 +5,7 @@ > #include > #include > #include > -#include > +#include > #include > #include > #include > @@ -1740,10 +1740,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, > const struct npfec npfec) > { > 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); > @@ -1760,16 +1756,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, > const struct npfec npfec) > 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); > +if ( monitor_traps(v, (xma != XENMEM_access_n2rwx), req) < 0 ) > +domain_crash(v->domain); > + > xfree(req); > } > > -/* Pause the current VCPU */ > -if ( xma != XENMEM_access_n2rwx ) > -vm_event_vcpu_pause(v); > - > return false; > } > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index daaee1d..42f163e 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,15 @@ 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 */ > +else > +{ > +/* > + * Rights not promoted (aka. sync event), work here is done > + */ > rc = 1; > goto out_put_gfn; > } > @@ -1956,7 +1960,9 @@ out: > } > if ( req_ptr ) > { > -mem_access_send_req(currd, req_ptr); > +if ( monitor_traps(curr, sync, req_ptr) < 0 ) > +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..0f6ef96 100644 > --- a/xen/arch/x86/hvm/monitor.c > +++ b/xen/arch/x86/hvm/monitor.c > @@ -44,7 +44,6 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long > value, unsigned long old > > vm_event_request_t req = { > .reason = VM_EVENT_REASON_WRITE_CTRLREG, > -.vcpu_id = curr->vcpu_id, >