Re: [Xen-devel] [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Tamas,
>
>
> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>
>> The two functions monitor_traps and mem_access_send_req duplicate
>> some of the same functionality. The mem_access_send_req however leaves a
>> lot of the standard vm_event fields to be filled by other functions.
>>
>> Since mem_access events go on the monitor ring in this patch we
>> consolidate
>> all paths to use monitor_traps to place events on the ring and to fill in
>> the common parts of the requests.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> ---
>>  xen/arch/arm/p2m.c                | 69
>> +++++++++++++++++++--------------------
>>  xen/arch/x86/hvm/hvm.c            | 16 ++++++---
>>  xen/arch/x86/hvm/monitor.c        |  6 ++++
>>  xen/arch/x86/mm/p2m.c             | 24 ++------------
>>  xen/common/mem_access.c           | 11 -------
>>  xen/include/asm-x86/hvm/monitor.h |  2 ++
>>  xen/include/asm-x86/p2m.h         | 13 +++++---
>>  xen/include/xen/mem_access.h      |  7 ----
>>  8 files changed, 63 insertions(+), 85 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index d82349c..df898a3 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -5,7 +5,7 @@
>>  #include <xen/domain_page.h>
>>  #include <xen/bitops.h>
>>  #include <xen/vm_event.h>
>> -#include <xen/mem_access.h>
>> +#include <xen/monitor.h>
>>  #include <xen/iocap.h>
>>  #include <public/vm_event.h>
>>  #include <asm/flushtlb.h>
>> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void)
>>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>>  }
>>
>> +static int
>> +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec
>> npfec,
>> +                          xenmem_access_t xma)
>> +{
>> +    struct vcpu *v = current;
>> +    vm_event_request_t req = {};
>> +    bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1;
>> +
>> +    req.reason = VM_EVENT_REASON_MEM_ACCESS;
>> +
>> +    /* Send request to mem access subscriber */
>> +    req.u.mem_access.gfn = gpa >> PAGE_SHIFT;
>> +    req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
>> +    if ( npfec.gla_valid )
>> +    {
>> +        req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>> +        req.u.mem_access.gla = gla;
>> +
>> +        if ( npfec.kind == npfec_kind_with_gla )
>> +            req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>> +        else if ( npfec.kind == npfec_kind_in_gpt )
>> +            req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>> +    }
>> +    req.u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>> +    req.u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>> +    req.u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
>> +
>> +    return monitor_traps(v, sync, &req);
>> +}
>> +
>>  bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec
>> npfec)
>>  {
>>      int rc;
>>      bool_t violation;
>>      xenmem_access_t xma;
>> -    vm_event_request_t *req;
>>      struct vcpu *v = current;
>>      struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>>
>> @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t
>> gla, const struct npfec npfec)
>>          return false;
>>      }
>>
>> -    req = xzalloc(vm_event_request_t);
>> -    if ( req )
>> -    {
>> -        req->reason = VM_EVENT_REASON_MEM_ACCESS;
>> -
>> -        /* Pause the current VCPU */
>> -        if ( xma != XENMEM_access_n2rwx )
>> -            req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>> -
>> -        /* Send request to mem access subscriber */
>> -        req->u.mem_access.gfn = gpa >> PAGE_SHIFT;
>> -        req->u.mem_access.offset =  gpa & ((1 << PAGE_SHIFT) - 1);
>> -        if ( npfec.gla_valid )
>> -        {
>> -            req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>> -            req->u.mem_access.gla = gla;
>> -
>> -            if ( npfec.kind == npfec_kind_with_gla )
>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>> -            else if ( npfec.kind == npfec_kind_in_gpt )
>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>> -        }
>> -        req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R :
>> 0;
>> -        req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W :
>> 0;
>> -        req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X :
>> 0;
>> -        req->vcpu_id = v->vcpu_id;
>> -
>> -        mem_access_send_req(v->domain, req);
>> -        xfree(req);
>> -    }
>> -
>> -    /* Pause the current VCPU */
>> -    if ( xma != XENMEM_access_n2rwx )
>> -        vm_event_vcpu_pause(v);
>> +    if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 )
>> +        domain_crash(v->domain);
>
>
> This patch is doing more than it is claimed in the commit message.
>
> In general, moving the code and introducing changes within the same patch
> should really be avoided. So please split it in 2 patches.

Well, the changes are largely cosmetic so doing a whole separate patch
IMHO is an overkill. How about adjusting the commit message to
something like "sanitize code surrounding sending mem_access
vm_events" to better describe the adjustments made in this patch?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel