| Message ID | 1470070371-10833-1-git-send-email-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/01/2016 07:52 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(), and on ARM, the introduction of the > __p2m_mem_access_send_req() helper to fill in common mem_access information. > 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 <tamas.lengyel@zentific.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> On 01.08.16 at 18:52, <tamas.lengyel@zentific.com> wrote: > --- 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 ) { Coding style. If you dropped the brace entirely, you could at once adjust ... > fall_through = 1; > } else { ... coding style here. > - /* Rights not promoted, vcpu paused, work here is done */ > + /* Rights not promoted (aka. sync event), work here is done */ Comment style. More of these elsewhere. > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, Coding style. > + vm_event_request_t *req) > +{ > + return monitor_traps(v, sync, req); > +} Overall - is this a useful wrapper? Why can't the caller(s) call monitor_traps() directly? And if you really want to keep it, it would probably better be an inline one. Jan
On Aug 2, 2016 06:45, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 01.08.16 at 18:52, <tamas.lengyel@zentific.com> wrote: > > --- 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 ) { > > Coding style. If you dropped the brace entirely, you could at once > adjust ... > > > fall_through = 1; > > } else { > > ... coding style here. > > > - /* Rights not promoted, vcpu paused, work here is done */ > > + /* Rights not promoted (aka. sync event), work here is done */ > > Comment style. More of these elsewhere. > > > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, > > Coding style. > > > + vm_event_request_t *req) > > +{ > > + return monitor_traps(v, sync, req); > > +} > > Overall - is this a useful wrapper? Why can't the caller(s) call > monitor_traps() directly? And if you really want to keep it, it would > probably better be an inline one. > The reason for this wrapper is to avoid having to include the common monitor header here. I can move it into the hvm monitor header as inline, that's no problem. Thanks, Tamas
On Tue, Aug 2, 2016 at 9:23 AM, Tamas K Lengyel <tamas.lengyel@zentific.com> wrote: > On Aug 2, 2016 06:45, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >>> On 01.08.16 at 18:52, <tamas.lengyel@zentific.com> wrote: >> > --- 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 ) { >> >> Coding style. If you dropped the brace entirely, you could at once >> adjust ... >> >> > fall_through = 1; >> > } else { >> >> ... coding style here. >> >> > - /* Rights not promoted, vcpu paused, work here is done >> > */ >> > + /* Rights not promoted (aka. sync event), work here is >> > done */ >> >> Comment style. More of these elsewhere. >> >> > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, >> >> Coding style. >> >> > + vm_event_request_t *req) >> > +{ >> > + return monitor_traps(v, sync, req); >> > +} >> >> Overall - is this a useful wrapper? Why can't the caller(s) call >> monitor_traps() directly? And if you really want to keep it, it would >> probably better be an inline one. >> > > The reason for this wrapper is to avoid having to include the common monitor > header here. I can move it into the hvm monitor header as inline, that's no > problem. > Actually, making it into inline would require that hvm/monitor.h include the common monitor.h as well, at which point having the wrapper would be useless as hvm.c would have effectively include common monitor.h too. So yea, the goal is to avoid having to include both common/monitor and hvm/monitor in hvm.c and it needs this kind of wrapper. Tamas
>>> On 02.08.16 at 18:06, <tamas.lengyel@zentific.com> wrote: > On Tue, Aug 2, 2016 at 9:23 AM, Tamas K Lengyel <tamas.lengyel@zentific.com> wrote: >> On Aug 2, 2016 06:45, "Jan Beulich" <JBeulich@suse.com> wrote: >>> >>> On 01.08.16 at 18:52, <tamas.lengyel@zentific.com> wrote: >>> > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, >>> > + vm_event_request_t *req) >>> > +{ >>> > + return monitor_traps(v, sync, req); >>> > +} >>> >>> Overall - is this a useful wrapper? Why can't the caller(s) call >>> monitor_traps() directly? And if you really want to keep it, it would >>> probably better be an inline one. >> >> The reason for this wrapper is to avoid having to include the common monitor >> header here. I can move it into the hvm monitor header as inline, that's no >> problem. > > Actually, making it into inline would require that hvm/monitor.h > include the common monitor.h as well, at which point having the > wrapper would be useless as hvm.c would have effectively include > common monitor.h too. So yea, the goal is to avoid having to include > both common/monitor and hvm/monitor in hvm.c and it needs this kind of > wrapper. But what's wrong with including a header that declares a function you want to call? Jan
On Tue, Aug 2, 2016 at 10:13 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 02.08.16 at 18:06, <tamas.lengyel@zentific.com> wrote: >> On Tue, Aug 2, 2016 at 9:23 AM, Tamas K Lengyel <tamas.lengyel@zentific.com> wrote: >>> On Aug 2, 2016 06:45, "Jan Beulich" <JBeulich@suse.com> wrote: >>>> >>> On 01.08.16 at 18:52, <tamas.lengyel@zentific.com> wrote: >>>> > +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync, >>>> > + vm_event_request_t *req) >>>> > +{ >>>> > + return monitor_traps(v, sync, req); >>>> > +} >>>> >>>> Overall - is this a useful wrapper? Why can't the caller(s) call >>>> monitor_traps() directly? And if you really want to keep it, it would >>>> probably better be an inline one. >>> >>> The reason for this wrapper is to avoid having to include the common monitor >>> header here. I can move it into the hvm monitor header as inline, that's no >>> problem. >> >> Actually, making it into inline would require that hvm/monitor.h >> include the common monitor.h as well, at which point having the >> wrapper would be useless as hvm.c would have effectively include >> common monitor.h too. So yea, the goal is to avoid having to include >> both common/monitor and hvm/monitor in hvm.c and it needs this kind of >> wrapper. > > But what's wrong with including a header that declares a function you > want to call? > Nothing really, just wanted to separate things more neatly into their appropriate locations. It might be an overkill though I admit.. Tamas
On 01/08/16 17:52, 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(), and on ARM, the introduction of the > __p2m_mem_access_send_req() helper to fill in common mem_access information. > 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 <tamas.lengyel@zentific.com> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> This appears to be v3, not v2? > 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 p2m-bits: Acked-by: George Dunlap <george.dunlap@citrix.com> But I agree with Julien -- this patch has several independent changes which makes it quite difficult to tell what's going on. I'm sure it's taken the two of us a lot more time together to figure out what is and is not happening than it would have for you to break it down into several little chunks. If you're not already familiar with it, I would recommend looking into stackgit. My modus operandi for things like this is to get things working in one big patch, then pop it off the stack and apply bits of it at a time to make a series. It's not only more considerate of your reviewers, but it's also a helpful exercise for yourself. -George
On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 01/08/16 17:52, 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(), and on ARM, the introduction of the >> __p2m_mem_access_send_req() helper to fill in common mem_access information. >> 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 <tamas.lengyel@zentific.com> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > This appears to be v3, not v2? No, it's still just v2. > >> 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 > > p2m-bits: > > Acked-by: George Dunlap <george.dunlap@citrix.com> > > But I agree with Julien -- this patch has several independent changes > which makes it quite difficult to tell what's going on. I'm sure it's > taken the two of us a lot more time together to figure out what is and > is not happening than it would have for you to break it down into > several little chunks. > > If you're not already familiar with it, I would recommend looking into > stackgit. My modus operandi for things like this is to get things > working in one big patch, then pop it off the stack and apply bits of it > at a time to make a series. > > It's not only more considerate of your reviewers, but it's also a > helpful exercise for yourself. > The extra work doesn't just come from splitting the code itself (although I don't know which bits would really make sense to split here that would worth the effort) but testing a series on various platforms. As you are in the same boat that this should be multiple patches (so it's 3v2) I have no problem just postponing the ARM sanitization beside what's absolutely required at the moment. I'm already looking at how to move the mem_accss code out of p2m on both platforms so we don't have to end up in this loop in the future. Thanks, Tamas
On 03/08/16 16:18, Tamas K Lengyel wrote: > On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>> 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 <tamas.lengyel@zentific.com> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> This appears to be v3, not v2? > > No, it's still just v2. > >> >>> 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 >> >> p2m-bits: >> >> Acked-by: George Dunlap <george.dunlap@citrix.com> >> >> But I agree with Julien -- this patch has several independent changes >> which makes it quite difficult to tell what's going on. I'm sure it's >> taken the two of us a lot more time together to figure out what is and >> is not happening than it would have for you to break it down into >> several little chunks. >> >> If you're not already familiar with it, I would recommend looking into >> stackgit. My modus operandi for things like this is to get things >> working in one big patch, then pop it off the stack and apply bits of it >> at a time to make a series. >> >> It's not only more considerate of your reviewers, but it's also a >> helpful exercise for yourself. >> > > The extra work doesn't just come from splitting the code itself > (although I don't know which bits would really make sense to split > here that would worth the effort) but testing a series on various > platforms. I don't understand this statement -- why is testing a 3-patch series more difficult than testing a one-patch series? Are you testing each individual patch? -George
On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 03/08/16 16:18, Tamas K Lengyel wrote: >> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>> 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 <tamas.lengyel@zentific.com> >>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> >>> This appears to be v3, not v2? >> >> No, it's still just v2. >> >>> >>>> 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 >>> >>> p2m-bits: >>> >>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>> >>> But I agree with Julien -- this patch has several independent changes >>> which makes it quite difficult to tell what's going on. I'm sure it's >>> taken the two of us a lot more time together to figure out what is and >>> is not happening than it would have for you to break it down into >>> several little chunks. >>> >>> If you're not already familiar with it, I would recommend looking into >>> stackgit. My modus operandi for things like this is to get things >>> working in one big patch, then pop it off the stack and apply bits of it >>> at a time to make a series. >>> >>> It's not only more considerate of your reviewers, but it's also a >>> helpful exercise for yourself. >>> >> >> The extra work doesn't just come from splitting the code itself >> (although I don't know which bits would really make sense to split >> here that would worth the effort) but testing a series on various >> platforms. > > I don't understand this statement -- why is testing a 3-patch series > more difficult than testing a one-patch series? Are you testing each > individual patch? > Yes, I do. And when a patch touches multiple archs it adds up quite fast. Tamas
On 03/08/16 16:40, Tamas K Lengyel wrote: > On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> On 03/08/16 16:18, Tamas K Lengyel wrote: >>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>> 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 <tamas.lengyel@zentific.com> >>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> >>>> This appears to be v3, not v2? >>> >>> No, it's still just v2. >>> >>>> >>>>> 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 >>>> >>>> p2m-bits: >>>> >>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>> >>>> But I agree with Julien -- this patch has several independent changes >>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>> taken the two of us a lot more time together to figure out what is and >>>> is not happening than it would have for you to break it down into >>>> several little chunks. >>>> >>>> If you're not already familiar with it, I would recommend looking into >>>> stackgit. My modus operandi for things like this is to get things >>>> working in one big patch, then pop it off the stack and apply bits of it >>>> at a time to make a series. >>>> >>>> It's not only more considerate of your reviewers, but it's also a >>>> helpful exercise for yourself. >>>> >>> >>> The extra work doesn't just come from splitting the code itself >>> (although I don't know which bits would really make sense to split >>> here that would worth the effort) but testing a series on various >>> platforms. >> >> I don't understand this statement -- why is testing a 3-patch series >> more difficult than testing a one-patch series? Are you testing each >> individual patch? >> > > Yes, I do. And when a patch touches multiple archs it adds up quite fast. Yes, I can imagine it does. :-) But the next question is, why do you feel the need to test every patch of a series individually, rather than just testing the whole series? -George
On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 03/08/16 16:40, Tamas K Lengyel wrote: >> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>> On 03/08/16 16:18, Tamas K Lengyel wrote: >>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>>> 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 <tamas.lengyel@zentific.com> >>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> >>>>> This appears to be v3, not v2? >>>> >>>> No, it's still just v2. >>>> >>>>> >>>>>> 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 >>>>> >>>>> p2m-bits: >>>>> >>>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>>> >>>>> But I agree with Julien -- this patch has several independent changes >>>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>>> taken the two of us a lot more time together to figure out what is and >>>>> is not happening than it would have for you to break it down into >>>>> several little chunks. >>>>> >>>>> If you're not already familiar with it, I would recommend looking into >>>>> stackgit. My modus operandi for things like this is to get things >>>>> working in one big patch, then pop it off the stack and apply bits of it >>>>> at a time to make a series. >>>>> >>>>> It's not only more considerate of your reviewers, but it's also a >>>>> helpful exercise for yourself. >>>>> >>>> >>>> The extra work doesn't just come from splitting the code itself >>>> (although I don't know which bits would really make sense to split >>>> here that would worth the effort) but testing a series on various >>>> platforms. >>> >>> I don't understand this statement -- why is testing a 3-patch series >>> more difficult than testing a one-patch series? Are you testing each >>> individual patch? >>> >> >> Yes, I do. And when a patch touches multiple archs it adds up quite fast. > > Yes, I can imagine it does. :-) > > But the next question is, why do you feel the need to test every patch > of a series individually, rather than just testing the whole series? > Well, you never know when your series gets split up and have only bits of it merged. So having each patch tested individually is a necessity to ensure nothing gets broken midway through. Especially since mem_access/monitor/vm_event is not tested automatically in the Xen test system. Tamas
On 03/08/16 16:58, Tamas K Lengyel wrote: > Well, you never know when your series gets split up and have only bits > of it merged. So having each patch tested individually is a necessity > to ensure nothing gets broken midway through. Especially since > mem_access/monitor/vm_event is not tested automatically in the Xen > test system. It might be worth to add test for mem_access/monitor/vm_event in Osstest. Regards,
On 03/08/16 16:58, Tamas K Lengyel wrote: > On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> On 03/08/16 16:40, Tamas K Lengyel wrote: >>> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>> On 03/08/16 16:18, Tamas K Lengyel wrote: >>>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>>>> 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 <tamas.lengyel@zentific.com> >>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>> >>>>>> This appears to be v3, not v2? >>>>> >>>>> No, it's still just v2. >>>>> >>>>>> >>>>>>> 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 >>>>>> >>>>>> p2m-bits: >>>>>> >>>>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>>>> >>>>>> But I agree with Julien -- this patch has several independent changes >>>>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>>>> taken the two of us a lot more time together to figure out what is and >>>>>> is not happening than it would have for you to break it down into >>>>>> several little chunks. >>>>>> >>>>>> If you're not already familiar with it, I would recommend looking into >>>>>> stackgit. My modus operandi for things like this is to get things >>>>>> working in one big patch, then pop it off the stack and apply bits of it >>>>>> at a time to make a series. >>>>>> >>>>>> It's not only more considerate of your reviewers, but it's also a >>>>>> helpful exercise for yourself. >>>>>> >>>>> >>>>> The extra work doesn't just come from splitting the code itself >>>>> (although I don't know which bits would really make sense to split >>>>> here that would worth the effort) but testing a series on various >>>>> platforms. >>>> >>>> I don't understand this statement -- why is testing a 3-patch series >>>> more difficult than testing a one-patch series? Are you testing each >>>> individual patch? >>>> >>> >>> Yes, I do. And when a patch touches multiple archs it adds up quite fast. >> >> Yes, I can imagine it does. :-) >> >> But the next question is, why do you feel the need to test every patch >> of a series individually, rather than just testing the whole series? >> > > Well, you never know when your series gets split up and have only bits > of it merged. So having each patch tested individually is a necessity > to ensure nothing gets broken midway through. Especially since > mem_access/monitor/vm_event is not tested automatically in the Xen > test system. I don't know anyone else who does that (more than perhaps compile-testing), and I don't think anyone expects you to. Obviously *in general* you should try to avoid breaking things in the middle of a series -- not primarily because it may be only partially applied, but because it makes bisecting other issues more difficult. But we generally rely on code review to catch things like that. And that's also why we have the push gate. If the choice is between subtly broken patches that get checked in because they were too complicated for reviewers to catch the errors, and occasionally broken bisections because reviewers didn't notice a bug introduced in one patch and fixed in a later patch, I'd much rather have the latter. Or to say the same thing a different way: I would much rather have a clean series where each patch wasn't tested (but the final patch was), than to have a difficult-to-review patch. -George
On Wed, Aug 3, 2016 at 10:10 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 03/08/16 16:58, Tamas K Lengyel wrote: >> On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>> On 03/08/16 16:40, Tamas K Lengyel wrote: >>>> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>> On 03/08/16 16:18, Tamas K Lengyel wrote: >>>>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>>>>> 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 <tamas.lengyel@zentific.com> >>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>> >>>>>>> This appears to be v3, not v2? >>>>>> >>>>>> No, it's still just v2. >>>>>> >>>>>>> >>>>>>>> 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 >>>>>>> >>>>>>> p2m-bits: >>>>>>> >>>>>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>>>>> >>>>>>> But I agree with Julien -- this patch has several independent changes >>>>>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>>>>> taken the two of us a lot more time together to figure out what is and >>>>>>> is not happening than it would have for you to break it down into >>>>>>> several little chunks. >>>>>>> >>>>>>> If you're not already familiar with it, I would recommend looking into >>>>>>> stackgit. My modus operandi for things like this is to get things >>>>>>> working in one big patch, then pop it off the stack and apply bits of it >>>>>>> at a time to make a series. >>>>>>> >>>>>>> It's not only more considerate of your reviewers, but it's also a >>>>>>> helpful exercise for yourself. >>>>>>> >>>>>> >>>>>> The extra work doesn't just come from splitting the code itself >>>>>> (although I don't know which bits would really make sense to split >>>>>> here that would worth the effort) but testing a series on various >>>>>> platforms. >>>>> >>>>> I don't understand this statement -- why is testing a 3-patch series >>>>> more difficult than testing a one-patch series? Are you testing each >>>>> individual patch? >>>>> >>>> >>>> Yes, I do. And when a patch touches multiple archs it adds up quite fast. >>> >>> Yes, I can imagine it does. :-) >>> >>> But the next question is, why do you feel the need to test every patch >>> of a series individually, rather than just testing the whole series? >>> >> >> Well, you never know when your series gets split up and have only bits >> of it merged. So having each patch tested individually is a necessity >> to ensure nothing gets broken midway through. Especially since >> mem_access/monitor/vm_event is not tested automatically in the Xen >> test system. > > I don't know anyone else who does that (more than perhaps > compile-testing), and I don't think anyone expects you to. > > Obviously *in general* you should try to avoid breaking things in the > middle of a series -- not primarily because it may be only partially > applied, but because it makes bisecting other issues more difficult. > But we generally rely on code review to catch things like that. And > that's also why we have the push gate. > > If the choice is between subtly broken patches that get checked in > because they were too complicated for reviewers to catch the errors, and > occasionally broken bisections because reviewers didn't notice a bug > introduced in one patch and fixed in a later patch, I'd much rather have > the latter. > > Or to say the same thing a different way: I would much rather have a > clean series where each patch wasn't tested (but the final patch was), > than to have a difficult-to-review patch. > I understand and that's fine. However, for our components we may prefer a different style of workflow on occasion as our definition of what constitutes difficult-to-review might be different. That's why moving it out from the common p2m codebase should help with avoiding this type of arguments in the future and also remove the burden of having to have maintainers of other components review it either though it doesn't touch anything outside of what we are maintaining. Tamas
On 03/08/16 17:31, Tamas K Lengyel wrote: > On Wed, Aug 3, 2016 at 10:10 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> On 03/08/16 16:58, Tamas K Lengyel wrote: >>> On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>> On 03/08/16 16:40, Tamas K Lengyel wrote: >>>>> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>> On 03/08/16 16:18, Tamas K Lengyel wrote: >>>>>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>>>>>> 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 <tamas.lengyel@zentific.com> >>>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>> >>>>>>>> This appears to be v3, not v2? >>>>>>> >>>>>>> No, it's still just v2. >>>>>>> >>>>>>>> >>>>>>>>> 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 >>>>>>>> >>>>>>>> p2m-bits: >>>>>>>> >>>>>>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>> >>>>>>>> But I agree with Julien -- this patch has several independent changes >>>>>>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>>>>>> taken the two of us a lot more time together to figure out what is and >>>>>>>> is not happening than it would have for you to break it down into >>>>>>>> several little chunks. >>>>>>>> >>>>>>>> If you're not already familiar with it, I would recommend looking into >>>>>>>> stackgit. My modus operandi for things like this is to get things >>>>>>>> working in one big patch, then pop it off the stack and apply bits of it >>>>>>>> at a time to make a series. >>>>>>>> >>>>>>>> It's not only more considerate of your reviewers, but it's also a >>>>>>>> helpful exercise for yourself. >>>>>>>> >>>>>>> >>>>>>> The extra work doesn't just come from splitting the code itself >>>>>>> (although I don't know which bits would really make sense to split >>>>>>> here that would worth the effort) but testing a series on various >>>>>>> platforms. >>>>>> >>>>>> I don't understand this statement -- why is testing a 3-patch series >>>>>> more difficult than testing a one-patch series? Are you testing each >>>>>> individual patch? >>>>>> >>>>> >>>>> Yes, I do. And when a patch touches multiple archs it adds up quite fast. >>>> >>>> Yes, I can imagine it does. :-) >>>> >>>> But the next question is, why do you feel the need to test every patch >>>> of a series individually, rather than just testing the whole series? >>>> >>> >>> Well, you never know when your series gets split up and have only bits >>> of it merged. So having each patch tested individually is a necessity >>> to ensure nothing gets broken midway through. Especially since >>> mem_access/monitor/vm_event is not tested automatically in the Xen >>> test system. >> >> I don't know anyone else who does that (more than perhaps >> compile-testing), and I don't think anyone expects you to. >> >> Obviously *in general* you should try to avoid breaking things in the >> middle of a series -- not primarily because it may be only partially >> applied, but because it makes bisecting other issues more difficult. >> But we generally rely on code review to catch things like that. And >> that's also why we have the push gate. >> >> If the choice is between subtly broken patches that get checked in >> because they were too complicated for reviewers to catch the errors, and >> occasionally broken bisections because reviewers didn't notice a bug >> introduced in one patch and fixed in a later patch, I'd much rather have >> the latter. >> >> Or to say the same thing a different way: I would much rather have a >> clean series where each patch wasn't tested (but the final patch was), >> than to have a difficult-to-review patch. >> > > I understand and that's fine. However, for our components we may > prefer a different style of workflow on occasion as our definition of > what constitutes difficult-to-review might be different. That's why > moving it out from the common p2m codebase should help with avoiding > this type of arguments in the future and also remove the burden of > having to have maintainers of other components review it either though > it doesn't touch anything outside of what we are maintaining. Well you still need to get an ack from someone even if it only touches code that you maintain. And in the bigger picture, breaking down your patches into easy-to-review chunks makes it more likely for *you* to catch mistakes before you send it, and also makes it more likely that someone else will catch your mistakes before you check it in. Unless you never make coding mistakes, I would think that you'd want to do everything you could to make sure the code was as bug-free as possible, particularly as you're developing a security-oriented product. -George
On Thu, Aug 4, 2016 at 8:10 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On 03/08/16 17:31, Tamas K Lengyel wrote: >> On Wed, Aug 3, 2016 at 10:10 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>> On 03/08/16 16:58, Tamas K Lengyel wrote: >>>> On Wed, Aug 3, 2016 at 9:44 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>> On 03/08/16 16:40, Tamas K Lengyel wrote: >>>>>> On Wed, Aug 3, 2016 at 9:30 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>> On 03/08/16 16:18, Tamas K Lengyel wrote: >>>>>>>> On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@citrix.com> wrote: >>>>>>>>> On 01/08/16 17:52, 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(), and on ARM, the introduction of the >>>>>>>>>> __p2m_mem_access_send_req() helper to fill in common mem_access information. >>>>>>>>>> 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 <tamas.lengyel@zentific.com> >>>>>>>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>>>>>> >>>>>>>>> This appears to be v3, not v2? >>>>>>>> >>>>>>>> No, it's still just v2. >>>>>>>> >>>>>>>>> >>>>>>>>>> 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 >>>>>>>>> >>>>>>>>> p2m-bits: >>>>>>>>> >>>>>>>>> Acked-by: George Dunlap <george.dunlap@citrix.com> >>>>>>>>> >>>>>>>>> But I agree with Julien -- this patch has several independent changes >>>>>>>>> which makes it quite difficult to tell what's going on. I'm sure it's >>>>>>>>> taken the two of us a lot more time together to figure out what is and >>>>>>>>> is not happening than it would have for you to break it down into >>>>>>>>> several little chunks. >>>>>>>>> >>>>>>>>> If you're not already familiar with it, I would recommend looking into >>>>>>>>> stackgit. My modus operandi for things like this is to get things >>>>>>>>> working in one big patch, then pop it off the stack and apply bits of it >>>>>>>>> at a time to make a series. >>>>>>>>> >>>>>>>>> It's not only more considerate of your reviewers, but it's also a >>>>>>>>> helpful exercise for yourself. >>>>>>>>> >>>>>>>> >>>>>>>> The extra work doesn't just come from splitting the code itself >>>>>>>> (although I don't know which bits would really make sense to split >>>>>>>> here that would worth the effort) but testing a series on various >>>>>>>> platforms. >>>>>>> >>>>>>> I don't understand this statement -- why is testing a 3-patch series >>>>>>> more difficult than testing a one-patch series? Are you testing each >>>>>>> individual patch? >>>>>>> >>>>>> >>>>>> Yes, I do. And when a patch touches multiple archs it adds up quite fast. >>>>> >>>>> Yes, I can imagine it does. :-) >>>>> >>>>> But the next question is, why do you feel the need to test every patch >>>>> of a series individually, rather than just testing the whole series? >>>>> >>>> >>>> Well, you never know when your series gets split up and have only bits >>>> of it merged. So having each patch tested individually is a necessity >>>> to ensure nothing gets broken midway through. Especially since >>>> mem_access/monitor/vm_event is not tested automatically in the Xen >>>> test system. >>> >>> I don't know anyone else who does that (more than perhaps >>> compile-testing), and I don't think anyone expects you to. >>> >>> Obviously *in general* you should try to avoid breaking things in the >>> middle of a series -- not primarily because it may be only partially >>> applied, but because it makes bisecting other issues more difficult. >>> But we generally rely on code review to catch things like that. And >>> that's also why we have the push gate. >>> >>> If the choice is between subtly broken patches that get checked in >>> because they were too complicated for reviewers to catch the errors, and >>> occasionally broken bisections because reviewers didn't notice a bug >>> introduced in one patch and fixed in a later patch, I'd much rather have >>> the latter. >>> >>> Or to say the same thing a different way: I would much rather have a >>> clean series where each patch wasn't tested (but the final patch was), >>> than to have a difficult-to-review patch. >>> >> >> I understand and that's fine. However, for our components we may >> prefer a different style of workflow on occasion as our definition of >> what constitutes difficult-to-review might be different. That's why >> moving it out from the common p2m codebase should help with avoiding >> this type of arguments in the future and also remove the burden of >> having to have maintainers of other components review it either though >> it doesn't touch anything outside of what we are maintaining. > > Well you still need to get an ack from someone even if it only touches > code that you maintain. Yes, but we would have been done with this patch days ago as the other maintainer of this component, Razvan, has already acked it days ago. Tamas
Hello Tamas, On 04/08/16 17:12, Tamas K Lengyel wrote: > On Thu, Aug 4, 2016 at 8:10 AM, George Dunlap <george.dunlap@citrix.com> wrote: >> On 03/08/16 17:31, Tamas K Lengyel wrote: >>> I understand and that's fine. However, for our components we may >>> prefer a different style of workflow on occasion as our definition of >>> what constitutes difficult-to-review might be different. That's why >>> moving it out from the common p2m codebase should help with avoiding >>> this type of arguments in the future and also remove the burden of >>> having to have maintainers of other components review it either though >>> it doesn't touch anything outside of what we are maintaining. >> >> Well you still need to get an ack from someone even if it only touches >> code that you maintain. > > Yes, but we would have been done with this patch days ago as the other > maintainer of this component, Razvan, has already acked it days ago. I think this is the first time we had an issue about the workflow. All the other comments on separate series were valid and related to interaction with the hardware and the rest of the code. I think this is a good idea to move memaccess code out of the P2M code (see [1]), however this should not be done only because we differ about the workflow. Whilst I agree you know really well the interaction of memaccess with the userspace, I have got some concerns about your knowledge of the ARM architecture. Your last few contributions (SMC trapping, VM event set/get registers and memaccess race condition) led to prolonged discussion about how the platform behaves. Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00037.html
On Thu, Aug 4, 2016 at 10:45 AM, Julien Grall <julien.grall@arm.com> wrote: > Hello Tamas, > > On 04/08/16 17:12, Tamas K Lengyel wrote: >> >> On Thu, Aug 4, 2016 at 8:10 AM, George Dunlap <george.dunlap@citrix.com> >> wrote: >>> >>> On 03/08/16 17:31, Tamas K Lengyel wrote: >>>> >>>> I understand and that's fine. However, for our components we may >>>> prefer a different style of workflow on occasion as our definition of >>>> what constitutes difficult-to-review might be different. That's why >>>> moving it out from the common p2m codebase should help with avoiding >>>> this type of arguments in the future and also remove the burden of >>>> having to have maintainers of other components review it either though >>>> it doesn't touch anything outside of what we are maintaining. >>> >>> >>> Well you still need to get an ack from someone even if it only touches >>> code that you maintain. >> >> >> Yes, but we would have been done with this patch days ago as the other >> maintainer of this component, Razvan, has already acked it days ago. > > > I think this is the first time we had an issue about the workflow. All the > other comments on separate series were valid and related to interaction with > the hardware and the rest of the code. > > I think this is a good idea to move memaccess code out of the P2M code (see > [1]), however this should not be done only because we differ about the > workflow. Whilst I agree you know really well the interaction of memaccess > with the userspace, I have got some concerns about your knowledge of the ARM > architecture. > > Your last few contributions (SMC trapping, VM event set/get registers and > memaccess race condition) led to prolonged discussion about how the platform > behaves. > I'm not going to argue about that - I'm certainly still catching up on many of the nuances of ARM. Having you review parts and point out corner-cases has been very valuable and it's certainly most welcome. However, there are times where it seems patches are being blocked on grounds that are flimsy, like arguing for breaking up the patch into multiple pieces or adding periods into the end of comments, when us, the maintainers of the subsystem, have already acked it and would rather move on to more productive things. As for the SMC trapping, the patch I sent doesn't introduce any regression compared to what is already present, yet you blocked it until further notice. IMHO it would be totally fine to send the SMC events as they are, including the failed condition SMC checks, and later tune it to filter those out, yet it doesn't seem we will have any of that in the short term. For the VM event register setting you argued either we get/set all registers or none at all when from our perspective it is not required - again, doesn't introduce any regression anywhere and only touches code that we are the maintainers of. So no, I think this has been an ongoing issue over multiple patches in the last couple months. Tamas
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d82349c..a378371 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,40 @@ 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, + bool_t sync) +{ + struct vcpu *v = current; + vm_event_request_t req = {}; + + 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 & ~PAGE_MASK; + 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; + bool_t violation, sync = true; xenmem_access_t xma; - vm_event_request_t *req; struct vcpu *v = current; struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); @@ -1688,6 +1716,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) case XENMEM_access_n: case XENMEM_access_n2rwx: violation = true; + sync = false; break; } @@ -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, sync) < 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..12103f2 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 = msr, .u.mov_to_msr.value = value, }; @@ -131,8 +129,6 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, return -EOPNOTSUPP; } - req.vcpu_id = curr->vcpu_id; - return monitor_traps(curr, sync, &req); } @@ -146,12 +142,17 @@ int hvm_monitor_cpuid(unsigned long insn_length) return 0; req.reason = VM_EVENT_REASON_CPUID; - req.vcpu_id = curr->vcpu_id; req.u.cpuid.insn_length = 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/common/monitor.c b/xen/common/monitor.c index c73d1d5..451f42f 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -107,6 +107,8 @@ int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req) return rc; }; + req->vcpu_id = v->vcpu_id; + if ( sync ) { req->flags |= VM_EVENT_FLAG_VCPU_PAUSED; 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. */