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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Aug 4, 2016 at 10:45 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Tamas,
>
> On 04/08/16 17:12, Tamas K Lengyel wrote:
>>
>> On Thu, Aug 4, 2016 at 8:10 AM, George Dunlap <george.dunlap@xxxxxxxxxx>
>> wrote:
>>>
>>> On 03/08/16 17:31, Tamas K Lengyel wrote:
>>>>
>>>> I understand and that's fine. However, for our components we may
>>>> prefer a different style of workflow on occasion as our definition of
>>>> what constitutes difficult-to-review might be different. That's why
>>>> moving it out from the common p2m codebase should help with avoiding
>>>> this type of arguments in the future and also remove the burden of
>>>> having to have maintainers of other components review it either though
>>>> it doesn't touch anything outside of what we are maintaining.
>>>
>>>
>>> Well you still need to get an ack from someone even if it only touches
>>> code that you maintain.
>>
>>
>> Yes, but we would have been done with this patch days ago as the other
>> maintainer of this component, Razvan, has already acked it days ago.
>
>
> I think this is the first time we had an issue about the workflow. All the
> other comments on separate series were valid and related to interaction with
> the hardware and the rest of the code.
>
> I think this is a good idea to move memaccess code out of the P2M code (see
> [1]), however this should not be done only because we differ about the
> workflow. Whilst I agree you know really well the interaction of memaccess
> with the userspace, I have got some concerns about your knowledge of the ARM
> architecture.
>
> Your last few contributions (SMC trapping, VM event set/get registers and
> memaccess race condition) led to prolonged discussion about how the platform
> behaves.
>

I'm not going to argue about that - I'm certainly still catching up on
many of the nuances of ARM. Having you review parts and point out
corner-cases has been very valuable and it's certainly most welcome.
However, there are times where it seems patches are being blocked on
grounds that are flimsy, like arguing for breaking up the patch into
multiple pieces or adding periods into the end of comments, when us,
the maintainers of the subsystem, have already acked it and would
rather move on to more productive things.

As for the SMC trapping, the patch I sent doesn't introduce any
regression compared to what is already present, yet you blocked it
until further notice. IMHO it would be totally fine to send the SMC
events as they are, including the failed condition SMC checks, and
later tune it to filter those out, yet it doesn't seem we will have
any of that in the short term. For the VM event register setting you
argued either we get/set all registers or none at all when from our
perspective it is not required - again, doesn't introduce any
regression anywhere and only touches code that we are the maintainers
of.

So no, I think this has been an ongoing issue over multiple patches in
the last couple months.

Tamas

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