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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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

I don't know anyone else who does that (more than perhaps
compile-testing), and I don't think anyone expects you to.

Obviously *in general* you should try to avoid breaking things in the
middle of a series -- not primarily because it may be only partially
applied, but because it makes bisecting other issues more difficult.
But we generally rely on code review to catch things like that.  And
that's also why we have the push gate.

If the choice is between subtly broken patches that get checked in
because they were too complicated for reviewers to catch the errors, and
occasionally broken bisections because reviewers didn't notice a bug
introduced in one patch and fixed in a later patch, I'd much rather have
the latter.

Or to say the same thing a different way: I would much rather have a
clean series where each patch wasn't tested (but the final patch was),
than to have a difficult-to-review patch.

 -George

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