Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Tuesday, December 09, 2014 6:11 PM
> 
> At 08:19 +0000 on 09 Dec (1418109561), Jan Beulich wrote:
> > Why do you always pick other than the simplest possible solution?
> 
> Jan, please don't make personal comments like this in code review.  It
> can only offend and demoralize contributors, and deter other people
> from joining in.
> 
> I understand that it can be frustrating, and I'm sure I have lashed
> out at people on-list in the past.  But remember that people who are
> new to the project need time to learn, and keep the comments to the
> code itself.
> 
> I can see that this series has been going for a long time, and is
> still getting hung up on coding style issues.  Might it be useful to
> have a round of internal review from some of the more xen-experienced
> engineers at Intel before Jan looks at it again?
> 

Thanks Tim. Some of my thoughts:

1. It's more efficient for new people to start from a small, well-defined task
in one area, and then spanning to adjacent areas gradually. Patience must 
be given by the community to help them grow;

2. Unfortunately this RMRR effort grows from original two patches (very VT-d
focused) to now v8 17 patches spanning many areas (including hypercall, mmu, 
domain builder, hvmloader, etc.), and thus imposing both long learning curve
and lots of frustrations being no achievement returned. Though having a complete
solution is desired, we need to help split the big task into step-by-step 
approach 
as long as:
        - the overall design is agreed
        - each step is self-contained 
        - each step won't be wasted moving forward. 
That way new people can see incremental achievements, instead of being hit 
down before final success. 

Take this RMRR for example. Maybe we can split into steps like:

        step-1: setup RMRR identify mapping in hypervisor. fail if confliction. 
no
user space changes
        step-2: expose RMRR knowledge to userspace and detect confliction in
domain builder and hvmloader
        step-3: reconcile e820/mmio to avoid confliction in a best effort
        step-4: miscellaneous enhancements, each can be ACK-ed individually:
                - handle guest CPU access to RMRR regions
                - handle conflicting RMRR regions
                - handle group RMRRs
                - re-enable USB device assignment

That way Tiejun can focus on a self-contained task at one time, and then observe
continuous acknowledgements for his work. We don't need to claim RMRR fully
ready in Xen until last patch is accepted, but that at least provides a way to 
ack
new people when working on complex features and also allow for partial usage 
on his work.

3. Maintainers sometimes didn't give definitive guidance to the new people, 
and the high-level design is not closed in the 1st place. e.g. when I thought 
this v8
version has everyone agreed on the design, there's another comment from Jan
about using XENMEM_set_memory_map might be better, while back to Aug.
when Tiejun raised that option the answer is "I'm not sure". Note I understand
as a maintainer he might not have definite answers for all opens. But w/o a
definitive guide new people may waste lots of effort on chasing a wrong option,
which is definitely frustrating. 

So I'd suggest for such non-trivial task for a new people, all maintainers in 
involved areas (xen, mmu, tools, vtd, etc) should first spend time to agree 
on the high level design. At that stage, let's skip the coding problems to save
both time. After agreed design, then we can help the new people to improve 
the coding to reach check-in criteria, which then becomes a converging process.

for this RMRR issue, let's close the design first, and then use staged approach
to get this patch series in.

4. Regarding to coding style, Intel will definitely help our new people 
internally,
and we also like all the discussion happened publicly, to also benefit from the
community, especially regarding to current scope extended out of VT-d area. 
In the meantime, if w/ above suggestions, our new people can then focus on 
incremental deliverables and then pay more attention to coding styles. 

Thanks
Kevin

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