Re: [PATCH v2 0/7] misc fixes to PV extentions code

[Thomas Gleixner]

On Mon, 24 Jun 2019, Zhenzhong Duan wrote:

> [PATCH v2 1/7]  x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init
> [PATCH v2 2/7]  x86/jailhouse: Mark jailhouse_x2apic_available as __init
> 
> Above two patches only add __init annotation to some functions, not
> related to other patches. I didn't split the two out as following patches
> need them to avoid conflicts.

Not really. Just the XEN one conflicts. The jailhoise one is independent.

> [PATCH v2 3/7]  x86: Add nopv parameter to disable PV extensions
> [PATCH v2 4/7]  Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."
> [PATCH v2 5/7]  x86/xen: nopv parameter support for HVM guest
> 
> Above three patches add an unified nopv prameter used for most of hypervisor
> platform except XEN PV/PVH, jailhouse. Those need PV extensions to work.
> 
> I revert 'xen_nopv' as it's same effect as nopv on XEN platform, there is also
> an issue using 'xen_nopv' with PVH, we should ignore 'xen_nopv' for PVH.

Well, command line options are ABI. You cannot nilly willy remove them as
it might break existing setups.

The fact that nopv can replace xen_nopv is not a justification.

Also if there is an issue with xen_nopv and PVH then this issue needs to be
fixed first especially if that issue exists on older kernels.

> [PATCH v2 6/7]  locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case
> 
> This is a similar change as Commit e6fd28eb3522
> ("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case"), but for
> hyperv.

This looks like an independent bug fix. Bug fixes should either be posted
seperately or at least at the beginning of the series. And this one clearly
is self contained so why hiding it in the middle of a pile of other
patches?

> [PATCH v2 7/7]  Revert "x86/paravirt: Set up the virt_spin_lock_key after static keys get initialized"
> 
> This revert an old change which is unnecessory now, I think the original change is smarter.

Again, this has nothing to do with the meat of this series which deals with
the command line parameters and is completely independent.

So you give a list of patches with some explanation for them, but the cover
letter should provide the big picture. The details of the patches need to
be in the changelog.

  1) Describe the context

     The paravirtualization whatever lack whatever they lack and have the
     shortcoming A, B, C.

     For a consistent admin experience a common command line parameter set
     across all PV guest implementations is a better choice, yada, yada
     yada.

  2) Describe the changes as overview

     To achieve this introduce a new nopv parameter which is usable by
     all PV guest implementation.

     While analyzing the PV guest code several bugs were found and
     fixed. (Patches 1 - 3). They can be applied independent of the
     functional changes, but they are kept in the series as the functional
     changes depend on them.

Documentation/process/submitting-patches.rst clearly explains why it is a
bad idea to send random collections of patches especially if some patches
are independent and contain bug fixes.

These rules exist for a reason and are not subject to personal
interpretation. You want your patches to be reviewed and merged, so pretty
please make the life of those who need to do that as easy as possible.

It's not the job of reviewers and maintainers to distangle your randomly
ordered patch series.

Thanks,

	tglx