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:44 AM, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> On 03/08/16 16:40, Tamas K Lengyel wrote:
>> 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.
>
> Yes, I can imagine it does. :-)
>
> But the next question is, why do you feel the need to test every patch
> of a series individually, rather than just testing the whole series?
>

Well, you never know when your series gets split up and have only bits
of it merged. So having each patch tested individually is a necessity
to ensure nothing gets broken midway through. Especially since
mem_access/monitor/vm_event is not tested automatically in the Xen
test system.

Tamas

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