From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req Date: Fri, 29 Jul 2016 17:27:02 +0100 Message-ID: References: <1469734504-5317-1-git-send-email-tamas.lengyel@zentific.com> <4c193694-b71f-9dc0-9553-7812ffd1e47a@arm.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8573990046706522044==" Return-path: Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bTAct-0003G2-1j for xen-devel@lists.xenproject.org; Fri, 29 Jul 2016 16:27:07 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Tamas K Lengyel , Julien Grall Cc: George Dunlap , "xen-devel@lists.xenproject.org" , Stefano Stabellini , Jan Beulich , Razvan Cojocaru List-Id: xen-devel@lists.xenproject.org --===============8573990046706522044== Content-Type: multipart/alternative; boundary="------------4A99E4EE69FBD2770D505388" --------------4A99E4EE69FBD2770D505388 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit On 29/07/16 15:21, Tamas K Lengyel wrote: > > On Jul 29, 2016 02:50, "Julien Grall" > wrote: > > > > > > > > On 28/07/16 23:54, Tamas K Lengyel wrote: > >> > >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall > 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 --------------4A99E4EE69FBD2770D505388 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
On 29/07/16 15:21, Tamas K Lengyel wrote:

On Jul 29, 2016 02:50, "Julien Grall" <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> 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
--------------4A99E4EE69FBD2770D505388-- --===============8573990046706522044== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============8573990046706522044==--