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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 03/08/16 16:18, Tamas K Lengyel wrote:
> 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.

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

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