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 9:30 AM, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> 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?
>

Yes, I do. And when a patch touches multiple archs it adds up quite fast.

Tamas

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