Re: [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3268 bytes --]

On 29/07/16 15:21, Tamas K Lengyel wrote:
>
> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
> >
> >
> >
> > On 28/07/16 23:54, Tamas K Lengyel wrote:
> >>
> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
> >>>
> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
> >>> This patch is doing more than it is claimed in the commit message.
> >>>
> >>> In general, moving the code and introducing changes within the
> same patch
> >>> should really be avoided. So please split it in 2 patches.
> >>
> >>
> >> Well, the changes are largely cosmetic so doing a whole separate patch
> >> IMHO is an overkill. How about adjusting the commit message to
> >> something like "sanitize code surrounding sending mem_access
> >> vm_events" to better describe the adjustments made in this patch?
> >
> >
> > I think the wiki page "Submitting Xen Project patches" [1] should
> answer to your question.
> >
> > If not, trivial patches are easy to review, merging multiple trivial
> patches in a single patch is not. Moving code and at the same time as
> changing the behavior is fairly difficult to review because it hides
> the real modifications.
> >
> > Regards,
> >
> > [1]
> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
> >
>
> The behavior didn't change at all, this whole patch is code
> sanitization. It's not worth doing a separate patch for each minor
> change. The few change on the arm side is the vm_event request
> allocation going from xzalloc to stack based and using monitor_traps
> now in a split-out function. It really should be no problem reviewing
> it. Even Andrew requested minor adjustments to be included in this
> patch. Anyway, I'm not looking to change this into a series. If it's a
> no go from your side I'm just going to cut down the ARM side
> sanitization to the bare minimum of using monitor_traps as the rest
> just does not worth the effort.
>

To offer an alternative opinion.

Looking at this patch, I personally would have a hard time justifying
breaking it down any further.  Each of the changes are closely related.

However, the commit message could be a lot clearer about which steps are
taken.  If I were writing the commit message, I would go with something
a bit more like this:

----8<----
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.

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.
----8<----

If in doubt, always spell out each of the changes, and their logical
relationships.  If nothing else, it helps people trying to review the patch.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 4894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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