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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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?

 -George



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