Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request

[Stefano Stabellini]

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, _ptr) )
> -{
> +sync = p2m_mem_access_check(gpa, gla, npfec, _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, 

Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request

[Tamas K Lengyel]

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, _ptr) )
> -{
> +sync = p2m_mem_access_check(gpa, gla, npfec, _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
> +++ 

Re: [Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request

[Jan Beulich]

>>> 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, _ptr) )
> -{
> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Tamas K Lengyel]

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, _ptr) )
 -{
 +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[George Dunlap]

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, _ptr) )
>> -{
>> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Jan Beulich]

>>> 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, _ptr) )
> -{
> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[George Dunlap]

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, _ptr) )
 -{
 +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Jan Beulich]

>>> 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, _ptr) )
>>> -{
>>> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Tamas K Lengyel]

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, _ptr) )
>> -{
>> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Jan Beulich]

>>> 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, _ptr) )
> -{
> +sync = p2m_mem_access_check(gpa, gla, npfec, _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

[Xen-devel] [PATCH v3] mem_access: sanitize code around sending vm_event request

[Tamas K Lengyel]

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, _ptr) )
-{
+sync = p2m_mem_access_check(gpa, gla, npfec, _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