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

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

On Wed, Aug 3, 2016 at 8:41 AM, George Dunlap <george.dunlap@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
> 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

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