
* Run docker probe only if docker or podman are available The docker probe uses "sudo -n" which can cause an e-mail with a security warning each time when configure is run. Therefore run docker probe only if either docker or podman are available. That avoids the problematic "sudo -n" on build environments which have neither docker nor podman installed. Fixes: c4575b59155e2e00 ("configure: store container engine in config-host.mak") Signed-off-by: Stefan Weil <sw@weilnetz.de> Message-Id: <20221030083510.310584-1-sw@weilnetz.de> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Thomas Huth <thuth@redhat.com> Message-Id: <20221117172532.538149-2-alex.bennee@linaro.org> * tests/avocado/machine_aspeed.py: Reduce noise on the console for SDK tests The Aspeed SDK images are based on OpenBMC which starts a lot of services. The output noise on the console can break from time to time the test waiting for the logging prompt. Change the U-Boot bootargs variable to add "quiet" to the kernel command line and reduce the output volume. This also drops the test on the CPU id which was nice to have but not essential. Signed-off-by: Cédric Le Goater <clg@kaod.org> Message-Id: <20221104075347.370503-1-clg@kaod.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Message-Id: <20221117172532.538149-3-alex.bennee@linaro.org> * tests/docker: allow user to override check target This is useful when trying to bisect a particular failing test behind a docker run. For example: make docker-test-clang@fedora \ TARGET_LIST=arm-softmmu \ TEST_COMMAND="meson test qtest-arm/qos-test" \ J=9 V=1 Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-4-alex.bennee@linaro.org> * docs/devel: add a maintainers section to development process We don't currently have a clear place in the documentation to describe the roles and responsibilities of a maintainer. Lets create one so we can. I've moved a few small bits out of other files to try and keep everything in one place. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-5-alex.bennee@linaro.org> * docs/devel: make language a little less code centric We welcome all sorts of patches. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-6-alex.bennee@linaro.org> * docs/devel: simplify the minimal checklist The bullet points are quite long and contain process tips. Move those bits of the bullet to the relevant sections and link to them. Use a table for nicer formatting of the checklist. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-7-alex.bennee@linaro.org> * docs/devel: try and improve the language around patch review It is important that contributors take the review process seriously and we collaborate in a respectful way while avoiding personal attacks. Try and make this clear in the language. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-8-alex.bennee@linaro.org> * tests/avocado: Raise timeout for boot_linux.py:BootLinuxPPC64.test_pseries_tcg On my machine, a debug build of QEMU takes about 260 seconds to complete this test, so with the current timeout value of 180 seconds it always times out. Double the timeout value to 360 so the test definitely has enough time to complete. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Message-Id: <20221110142901.3832318-1-peter.maydell@linaro.org> Message-Id: <20221117172532.538149-9-alex.bennee@linaro.org> * tests/avocado: introduce alpine virt test for CI The boot_linux tests download and run a full cloud image boot and start a full distro. While the ability to test the full boot chain is worthwhile it is perhaps a little too heavy weight and causes issues in CI. Fix this by introducing a new alpine linux ISO boot in machine_aarch64_virt. This boots a fully loaded -cpu max with all the bells and whistles in 31s on my machine. A full debug build takes around 180s on my machine so we set a more generous timeout to cover that. We don't add a test for lesser GIC versions although there is some coverage for that already in the boot_xen.py tests. If we want to introduce more comprehensive testing we can do it with a custom kernel and initrd rather than a full distro boot. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-10-alex.bennee@linaro.org> * tests/avocado: skip aarch64 cloud TCG tests in CI We now have a much lighter weight test in machine_aarch64_virt which tests the full boot chain in less time. Rename the tests while we are at it to make it clear it is a Fedora cloud image. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221117172532.538149-11-alex.bennee@linaro.org> * gitlab: integrate coverage report This should hopefully give is nice coverage information about what our tests (or at least the subset we are running) have hit. Ideally we would want a way to trigger coverage on tests likely to be affected by the current commit. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221117172532.538149-12-alex.bennee@linaro.org> * vhost: mask VIRTIO_F_RING_RESET for vhost and vhost-user devices Commit 69e1c14aa2 ("virtio: core: vq reset feature negotation support") enabled VIRTIO_F_RING_RESET by default for all virtio devices. This feature is not currently emulated by QEMU, so for vhost and vhost-user devices we need to make sure it is supported by the offloaded device emulation (in-kernel or in another process). To do this we need to add VIRTIO_F_RING_RESET to the features bitmap passed to vhost_get_features(). This way it will be masked if the device does not support it. This issue was initially discovered with vhost-vsock and vhost-user-vsock, and then also tested with vhost-user-rng which confirmed the same issue. They fail when sending features through VHOST_SET_FEATURES ioctl or VHOST_USER_SET_FEATURES message, since VIRTIO_F_RING_RESET is negotiated by the guest (Linux >= v6.0), but not supported by the device. Fixes: 69e1c14aa2 ("virtio: core: vq reset feature negotation support") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1318 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Message-Id: <20221121101101.29400-1-sgarzare@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com> Acked-by: Jason Wang <jasowang@redhat.com> * tests: acpi: whitelist DSDT before moving PRQx to _SB scope Signed-off-by: Igor Mammedov <imammedo@redhat.com> Message-Id: <20221121153613.3972225-2-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * acpi: x86: move RPQx field back to _SB scope Commit 47a373faa6b2 (acpi: pc/q35: drop ad-hoc PCI-ISA bridge AML routines and let bus ennumeration generate AML) moved ISA bridge AML generation to respective devices and was using aml_alias() to provide PRQx fields in _SB. scope. However, it turned out that SeaBIOS was not able to process Alias opcode when parsing DSDT, resulting in lack of keyboard during boot (SeaBIOS console, grub, FreeDOS). While fix for SeaBIOS is posted https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/thread/RGPL7HESH5U5JRLEO6FP77CZVHZK5J65/ fixed SeaBIOS might not make into QEMU-7.2 in time. Hence this workaround that puts PRQx back into _SB scope and gets rid of aliases in ISA bridge description, so DSDT will be parsable by broken SeaBIOS. That brings back hardcoded references to ISA bridge PCI0.S08.P40C/PCI0.SF8.PIRQ where middle part now is auto generated based on slot it's plugged in, but it should be fine as bridge initialization also hardcodes PCI address of the bridge so it can't ever move. Once QEMU tree has fixed SeaBIOS blob, we should be able to drop this part and revert back to alias based approach Reported-by: Volker Rümelin <vr_qemu@t-online.de> Signed-off-by: Igor Mammedov <imammedo@redhat.com> Message-Id: <20221121153613.3972225-3-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * tests: acpi: x86: update expected DSDT after moving PRQx fields in _SB scope Expected DSDT changes, pc: - Field (P40C, ByteAcc, NoLock, Preserve) + Scope (\_SB) { - PRQ0, 8, - PRQ1, 8, - PRQ2, 8, - PRQ3, 8 + Field (PCI0.S08.P40C, ByteAcc, NoLock, Preserve) + { + PRQ0, 8, + PRQ1, 8, + PRQ2, 8, + PRQ3, 8 + } } - Alias (PRQ0, \_SB.PRQ0) - Alias (PRQ1, \_SB.PRQ1) - Alias (PRQ2, \_SB.PRQ2) - Alias (PRQ3, \_SB.PRQ3) q35: - Field (PIRQ, ByteAcc, NoLock, Preserve) - { - PRQA, 8, - PRQB, 8, - PRQC, 8, - PRQD, 8, - Offset (0x08), - PRQE, 8, - PRQF, 8, - PRQG, 8, - PRQH, 8 + Scope (\_SB) + { + Field (PCI0.SF8.PIRQ, ByteAcc, NoLock, Preserve) + { + PRQA, 8, + PRQB, 8, + PRQC, 8, + PRQD, 8, + Offset (0x08), + PRQE, 8, + PRQF, 8, + PRQG, 8, + PRQH, 8 + } } - Alias (PRQA, \_SB.PRQA) - Alias (PRQB, \_SB.PRQB) - Alias (PRQC, \_SB.PRQC) - Alias (PRQD, \_SB.PRQD) - Alias (PRQE, \_SB.PRQE) - Alias (PRQF, \_SB.PRQF) - Alias (PRQG, \_SB.PRQG) - Alias (PRQH, \_SB.PRQH) Signed-off-by: Igor Mammedov <imammedo@redhat.com> Message-Id: <20221121153613.3972225-4-imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * MAINTAINERS: add mst to list of biosbits maintainers Adding Michael's name to the list of bios bits maintainers so that all changes and fixes into biosbits framework can go through his tree and he is notified. Suggested-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Ani Sinha <ani@anisinha.ca> Message-Id: <20221111151138.36988-1-ani@anisinha.ca> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * tests/avocado: configure acpi-bits to use avocado timeout Instead of using a hardcoded timeout, just rely on Avocado's built-in test case timeout. This helps avoid timeout issues on machines where 60 seconds is not sufficient. Signed-off-by: John Snow <jsnow@redhat.com> Message-Id: <20221115212759.3095751-1-jsnow@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Ani Sinha <ani@anisinha.ca> * acpi/tests/avocado/bits: keep the work directory when BITS_DEBUG is set in env Debugging bits issue often involves running the QEMU command line manually outside of the avocado environment with the generated ISO. Hence, its inconvenient if the iso gets cleaned up after the test has finished. This change makes sure that the work directory is kept after the test finishes if the test is run with BITS_DEBUG=1 in the environment so that the iso is available for use with the QEMU command line. CC: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Ani Sinha <ani@anisinha.ca> Message-Id: <20221117113630.543495-1-ani@anisinha.ca> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * virtio: disable error for out of spec queue-enable Virtio 1.0 is pretty clear that features have to be negotiated before enabling VQs. Unfortunately Seabios ignored this ever since gaining 1.0 support (UEFI is ok). Comment the error out for now, and add a TODO. Fixes: 3c37f8b8d1 ("virtio: introduce virtio_queue_enable()") Cc: "Kangjie Xu" <kangjie.xu@linux.alibaba.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Message-Id: <20221121200339.362452-1-mst@redhat.com> * hw/loongarch: Add default stdout uart in fdt Add "chosen" subnode into LoongArch fdt, and set it's "stdout-path" prop to uart node. Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Reviewed-by: Song Gao <gaosong@loongson.cn> Message-Id: <20221115114923.3372414-1-yangxiaojuan@loongson.cn> Signed-off-by: Song Gao <gaosong@loongson.cn> * hw/loongarch: Fix setprop_sized method in fdt rtc node. Fix setprop_sized method in fdt rtc node. Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Song Gao <gaosong@loongson.cn> Message-Id: <20221116040300.3459818-1-yangxiaojuan@loongson.cn> Signed-off-by: Song Gao <gaosong@loongson.cn> * hw/loongarch: Replace the value of uart info with macro Using macro to replace the value of uart info such as addr, size in acpi_build method. Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Reviewed-by: Song Gao <gaosong@loongson.cn> Message-Id: <20221115115008.3372489-1-yangxiaojuan@loongson.cn> Signed-off-by: Song Gao <gaosong@loongson.cn> * target/arm: Don't do two-stage lookup if stage 2 is disabled In get_phys_addr_with_struct(), we call get_phys_addr_twostage() if the CPU supports EL2. However, we don't check here that stage 2 is actually enabled. Instead we only check that inside get_phys_addr_twostage() to skip stage 2 translation. This means that even if stage 2 is disabled we still tell the stage 1 lookup to do its page table walks via stage 2. This works by luck for normal CPU accesses, but it breaks for debug accesses, which are used by the disassembler and also by semihosting file reads and writes, because the debug case takes a different code path inside S1_ptw_translate(). This means that setups that use semihosting for file loads are broken (a regression since 7.1, introduced in recent ptw refactoring), and that sometimes disassembly in debug logs reports "unable to read memory" rather than showing the guest insns. Fix the bug by hoisting the "is stage 2 enabled?" check up to get_phys_addr_with_struct(), so that we handle S2 disabled the same way we do the "no EL2" case, with a simple single stage lookup. Reported-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 20221121212404.1450382-1-peter.maydell@linaro.org * target/arm: Use signed quantity to represent VMSAv8-64 translation level The LPA2 extension implements 52-bit virtual addressing for 4k and 16k translation granules, and for the former, this means an additional level of translation is needed. This means we start counting at -1 instead of 0 when doing a walk, and so 'level' is now a signed quantity, and should be typed as such. So turn it from uint32_t into int32_t. This avoids a level of -1 getting misinterpreted as being >= 3, and terminating a page table walk prematurely with a bogus output address. Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> * Update VERSION for v7.2.0-rc2 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> * tests/avocado: Update the URLs of the advent calendar images The qemu-advent-calendar.org server will be decommissioned soon. I've mirrored the images that we use for the QEMU CI to gitlab, so update their URLs to point to the new location. Message-Id: <20221121102436.78635-1-thuth@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com> * tests/qtest: Decrease the amount of output from the qom-test The logs in the gitlab-CI have a size constraint, and sometimes we already hit this limit. The biggest part of the log then seems to be filled by the qom-test, so we should decrease the size of the output - which can be done easily by not printing the path for each property, since the path has already been logged at the beginning of each node that we handle here. However, if we omit the path, we should make sure to not recurse into child nodes in between, so that it is clear to which node each property belongs. Thus store the children and links in a temporary list and recurse only at the end of each node, when all properties have already been printed. Message-Id: <20221121194240.149268-1-thuth@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> * tests/avocado: use new rootfs for orangepi test The old URL wasn't stable. I suspect the current URL will only be stable for a few months so maybe we need another strategy for hosting rootfs snapshots? Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Message-Id: <20221118113309.1057790-1-alex.bennee@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com> * Revert "usbredir: avoid queuing hello packet on snapshot restore" Run state is also in RUN_STATE_PRELAUNCH while "-S" is used. This reverts commit 0631d4b448454ae8a1ab091c447e3f71ab6e088a Signed-off-by: Joelle van Dyne <j@getutm.app> Reviewed-by: Ján Tomko <jtomko@redhat.com> The original commit broke the usage of usbredir with libvirt, which starts every domain with "-S". This workaround is no longer needed because the usbredir behavior has been fixed in the meantime: https://gitlab.freedesktop.org/spice/usbredir/-/merge_requests/61 Signed-off-by: Ján Tomko <jtomko@redhat.com> Message-Id: <1689cec3eadcea87255e390cb236033aca72e168.1669193161.git.jtomko@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * gtk: disable GTK Clipboard with a new meson option The GTK Clipboard implementation may cause guest hangs. Therefore implement new configure switch: --enable-gtk-clipboard, as a meson option disabled by default, which warns in the help text about the experimental nature of the feature. Regenerate the meson build options to include it. The initialization of the clipboard is gtk.c, as well as the compilation of gtk-clipboard.c are now conditional on this new option to be set. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150 Signed-off-by: Claudio Fontana <cfontana@suse.de> Acked-by: Gerd Hoffmann <kraxel@redhat.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> Message-Id: <20221121135538.14625-1-cfontana@suse.de> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * hw/usb/hcd-xhci.c: spelling: tranfer Fixes: effaf5a240e03020f4ae953e10b764622c3e87cc Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> Reviewed-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Stefan Weil <sw@weilnetz.de> Message-Id: <20221105114851.306206-1-mjt@msgid.tls.msk.ru> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * ui/gtk: prevent ui lock up when dpy_gl_update called again before current draw event occurs A warning, "qemu: warning: console: no gl-unblock within" followed by guest scanout lockup can happen if dpy_gl_update is called in a row and the second call is made before gd_draw_event scheduled by the first call is taking place. This is because draw call returns without decrementing gl_block ref count if the dmabuf was already submitted as shown below. (gd_gl_area_draw/gd_egl_draw) if (dmabuf) { if (!dmabuf->draw_submitted) { return; } else { dmabuf->draw_submitted = false; } } So it should not schedule any redundant draw event in case draw_submitted is already set in gd_egl_fluch/gd_gl_area_scanout_flush. Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20221021192315.9110-1-dongwon.kim@intel.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * hw/usb/hcd-xhci: Reset the XHCIState with device_cold_reset() Currently the hcd-xhci-pci and hcd-xhci-sysbus devices, which are mostly wrappers around the TYPE_XHCI device, which is a direct subclass of TYPE_DEVICE. Since TYPE_DEVICE devices are not on any qbus and do not get automatically reset, the wrapper devices both reset the TYPE_XHCI device in their own reset functions. However, they do this using device_legacy_reset(), which will reset the device itself but not any bus it has. Switch to device_cold_reset(), which avoids using a deprecated function and also propagates reset along any child buses. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Message-Id: <20221014145423.2102706-1-peter.maydell@linaro.org> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * hw/audio/intel-hda: don't reset codecs twice Currently the intel-hda device has a reset method which manually resets all the codecs by calling device_legacy_reset() on them. This means they get reset twice, once because child devices on a qbus get reset before the parent device's reset method is called, and then again because we're manually resetting them. Drop the manual reset call, and ensure that codecs are still reset when the guest does a reset via ICH6_GCTL_RESET by using device_cold_reset() (which resets all the devices on the qbus as well as the device itself) instead of a direct call to the reset function. This is a slight ordering change because the (only) codec reset now happens before the controller registers etc are reset, rather than once before and then once after, but the codec reset function hda_audio_reset() doesn't care. This lets us drop a use of device_legacy_reset(), which is deprecated. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221014142632.2092404-2-peter.maydell@linaro.org> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * hw/audio/intel-hda: Drop unnecessary prototype The only use of intel_hda_reset() is after its definition, so we don't need to separately declare its prototype at the top of the file; drop the unnecessary line. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221014142632.2092404-3-peter.maydell@linaro.org> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * add syx snapshot extras * it compiles! * virtiofsd: Add `sigreturn` to the seccomp whitelist The virtiofsd currently crashes on s390x. This is because of a `sigreturn` system call. See audit log below: type=SECCOMP msg=audit(1669382477.611:459): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:virtd_t:s0-s0:c0.c1023 pid=6649 comm="virtiofsd" exe="/usr/libexec/virtiofsd" sig=31 arch=80000016 syscall=119 compat=0 ip=0x3fff15f748a code=0x80000000AUID="unset" UID="root" GID="root" ARCH=s390x SYSCALL=sigreturn Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: German Maglione <gmaglione@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221125143946.27717-1-mhartmay@linux.ibm.com> * libvhost-user: Fix wrong type of argument to formatting function (reported by LGTM) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Stefan Weil <sw@weilnetz.de> Message-Id: <20220422070144.1043697-2-sw@weilnetz.de> Signed-off-by: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-2-sw@weilnetz.de> * libvhost-user: Fix format strings Signed-off-by: Stefan Weil <sw@weilnetz.de> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20220422070144.1043697-3-sw@weilnetz.de> Signed-off-by: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-3-sw@weilnetz.de> * libvhost-user: Fix two more format strings This fix is required for 32 bit hosts. The bug was detected by CI for arm-linux, but is also relevant for i386-linux. Reported-by: Stefan Hajnoczi <stefanha@gmail.com> Signed-off-by: Stefan Weil <sw@weilnetz.de> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-4-sw@weilnetz.de> * libvhost-user: Add format attribute to local function vu_panic Signed-off-by: Stefan Weil <sw@weilnetz.de> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Message-Id: <20220422070144.1043697-4-sw@weilnetz.de> Signed-off-by: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-5-sw@weilnetz.de> * MAINTAINERS: Add subprojects/libvhost-user to section "vhost" Signed-off-by: Stefan Weil <sw@weilnetz.de> [Michael agreed to act as maintainer for libvhost-user via email in https://lore.kernel.org/qemu-devel/20221123015218-mutt-send-email-mst@kernel.org/. --Stefan] Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-6-sw@weilnetz.de> * Add G_GNUC_PRINTF to function qemu_set_info_str and fix related issues With the G_GNUC_PRINTF function attribute the compiler detects two potential insecure format strings: ../../../net/stream.c:248:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security] qemu_set_info_str(&s->nc, uri); ^~~ ../../../net/stream.c:322:31: warning: format string is not a string literal (potentially insecure) [-Wformat-security] qemu_set_info_str(&s->nc, uri); ^~~ There are also two other warnings: ../../../net/socket.c:182:35: warning: zero-length gnu_printf format string [-Wformat-zero-length] 182 | qemu_set_info_str(&s->nc, ""); | ^~ ../../../net/stream.c:170:35: warning: zero-length gnu_printf format string [-Wformat-zero-length] 170 | qemu_set_info_str(&s->nc, ""); Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221126152507.283271-7-sw@weilnetz.de> * del ramfile * update seabios source from 1.16.0 to 1.16.1 git shortlog rel-1.16.0..rel-1.16.1 =================================== Gerd Hoffmann (3): malloc: use variable for ZoneHigh size malloc: use large ZoneHigh when there is enough memory virtio-blk: use larger default request size Igor Mammedov (1): acpi: parse Alias object Volker Rümelin (2): pci: refactor the pci_config_*() functions reset: force standard PCI configuration access Xiaofei Lee (1): virtio-blk: Fix incorrect type conversion in virtio_blk_op() Xuan Zhuo (2): virtio-mmio: read/write the hi 32 features for mmio virtio: finalize features before using device Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * update seabios binaries to 1.16.1 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> * fix for non i386 archs * replay: Fix declaration of replay_read_next_clock Fixes the build with gcc 13: replay/replay-time.c:34:6: error: conflicting types for \ 'replay_read_next_clock' due to enum/integer mismatch; \ have 'void(ReplayClockKind)' [-Werror=enum-int-mismatch] 34 | void replay_read_next_clock(ReplayClockKind kind) | ^~~~~~~~~~~~~~~~~~~~~~ In file included from ../qemu/replay/replay-time.c:14: replay/replay-internal.h:139:6: note: previous declaration of \ 'replay_read_next_clock' with type 'void(unsigned int)' 139 | void replay_read_next_clock(unsigned int kind); | ^~~~~~~~~~~~~~~~~~~~~~ Fixes: 8eda206e090 ("replay: recording and replaying clock ticks") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com> Reviewed-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221129010547.284051-1-richard.henderson@linaro.org> * hw/display/qxl: Have qxl_log_command Return early if no log_cmd handler Only 3 command types are logged: no need to call qxl_phys2virt() for the other types. Using different cases will help to pass different structure sizes to qxl_phys2virt() in a pair of commits. Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221128202741.4945-2-philmd@linaro.org> * hw/display/qxl: Document qxl_phys2virt() Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221128202741.4945-3-philmd@linaro.org> * hw/display/qxl: Pass requested buffer size to qxl_phys2virt() Currently qxl_phys2virt() doesn't check for buffer overrun. In order to do so in the next commit, pass the buffer size as argument. For QXLCursor in qxl_render_cursor() -> qxl_cursor() we verify the size of the chunked data ahead, checking we can access 'sizeof(QXLCursor) + chunk->data_size' bytes. Since in the SPICE_CURSOR_TYPE_MONO case the cursor is assumed to fit in one chunk, no change are required. In SPICE_CURSOR_TYPE_ALPHA the ahead read is handled in qxl_unpack_chunks(). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Acked-by: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221128202741.4945-4-philmd@linaro.org> * hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144) Have qxl_get_check_slot_offset() return false if the requested buffer size does not fit within the slot memory region. Similarly qxl_phys2virt() now returns NULL in such case, and qxl_dirty_one_surface() aborts. This avoids buffer overrun in the host pointer returned by memory_region_get_ram_ptr(). Fixes: CVE-2022-4144 (out-of-bounds read) Reported-by: Wenxu Yin (@awxylitol) Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1336 Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221128202741.4945-5-philmd@linaro.org> * hw/display/qxl: Assert memory slot fits in preallocated MemoryRegion Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221128202741.4945-6-philmd@linaro.org> * block-backend: avoid bdrv_unregister_buf() NULL pointer deref bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL leads to undefined behavior. Jonathan Cameron reported this following NULL pointer dereference when a VM with a virtio-blk device and a memory-backend-file object is terminated: 1. qemu_cleanup() closes all drives, setting blk->root to NULL 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM block notifier callback because the memory-backend-file is destroyed. 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar notifier callback and undefined behavior occurs. Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221121211923.1993171-1-stefanha@redhat.com> * target/arm: Set TCGCPUOps.restore_state_to_opc for v7m This setting got missed, breaking v7m. Fixes: 56c6c98df85c ("target/arm: Convert to tcg_ops restore_state_to_opc") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1347 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Evgeny Ermakov <evgeny.v.ermakov@gmail.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221129204146.550394-1-richard.henderson@linaro.org> * Update VERSION for v7.2.0-rc3 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> * hooks are now post mem access * tests/qtests: override "force-legacy" for gpio virtio-mmio tests The GPIO device is a VIRTIO_F_VERSION_1 devices but running with a legacy MMIO interface we miss out that feature bit causing confusion. For the GPIO test force the mmio bus to support non-legacy so we can properly test it. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1333 Message-Id: <20221130112439.2527228-2-alex.bennee@linaro.org> Acked-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * vhost: enable vrings in vhost_dev_start() for vhost-user devices Commit 02b61f38d3 ("hw/virtio: incorporate backend features in features") properly negotiates VHOST_USER_F_PROTOCOL_FEATURES with the vhost-user backend, but we forgot to enable vrings as specified in docs/interop/vhost-user.rst: If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the ring starts directly in the enabled state. If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is initialized in a disabled state and is enabled by ``VHOST_USER_SET_VRING_ENABLE`` with parameter 1. Some vhost-user front-ends already did this by calling vhost_ops.vhost_set_vring_enable() directly: - backends/cryptodev-vhost.c - hw/net/virtio-net.c - hw/virtio/vhost-user-gpio.c But most didn't do that, so we would leave the vrings disabled and some backends would not work. We observed this issue with the rust version of virtiofsd [1], which uses the event loop [2] provided by the vhost-user-backend crate where requests are not processed if vring is not enabled. Let's fix this issue by enabling the vrings in vhost_dev_start() for vhost-user front-ends that don't already do this directly. Same thing also in vhost_dev_stop() where we disable vrings. [1] https://gitlab.com/virtio-fs/virtiofsd [2] https://github.com/rust-vmm/vhost/blob/240fc2966/crates/vhost-user-backend/src/event_loop.rs#L217 Fixes: 02b61f38d3 ("hw/virtio: incorporate backend features in features") Reported-by: German Maglione <gmaglione@redhat.com> Tested-by: German Maglione <gmaglione@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com> Message-Id: <20221123131630.52020-1-sgarzare@redhat.com> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Message-Id: <20221130112439.2527228-3-alex.bennee@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * hw/virtio: add started_vu status field to vhost-user-gpio As per the fix to vhost-user-blk in f5b22d06fb (vhost: recheck dev state in the vhost_migration_log routine) we really should track the connection and starting separately. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Message-Id: <20221130112439.2527228-4-alex.bennee@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * hw/virtio: generalise CHR_EVENT_CLOSED handling ..and use for both virtio-user-blk and virtio-user-gpio. This avoids the circular close by deferring shutdown due to disconnection until a later point. virtio-user-blk already had this mechanism in place so generalise it as a vhost-user helper function and use for both blk and gpio devices. While we are at it we also fix up vhost-user-gpio to re-establish the event handler after close down so we can reconnect later. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> Message-Id: <20221130112439.2527228-5-alex.bennee@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * include/hw: VM state takes precedence in virtio_device_should_start The VM status should always preempt the device status for these checks. This ensures the device is in the correct state when we suspend the VM prior to migrations. This restores the checks to the order they where in before the refactoring moved things around. While we are at it lets improve our documentation of the various fields involved and document the two functions. Fixes: 9f6bcfd99f (hw/virtio: move vm_running check to virtio_device_started) Fixes: 259d69c00b (hw/virtio: introduce virtio_device_should_start) Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Message-Id: <20221130112439.2527228-6-alex.bennee@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> * hw/nvme: fix aio cancel in format There are several bugs in the async cancel code for the Format command. Firstly, cancelling a format operation neglects to set iocb->ret as well as clearing the iocb->aiocb after cancelling the underlying aiocb which causes the aio callback to ignore the cancellation. Trivial fix. Secondly, and worse, because the request is queued up for posting to the CQ in a bottom half, if the cancellation is due to the submission queue being deleted (which calls blk_aio_cancel), the req structure is deallocated in nvme_del_sq prior to the bottom half being schedulued. Fix this by simply removing the bottom half, there is no reason to defer it anyway. Fixes: 3bcf26d3d619 ("hw/nvme: reimplement format nvm to allow cancellation") Reported-by: Jonathan Derrick <jonathan.derrick@linux.dev> Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> * hw/nvme: fix aio cancel in flush Make sure that iocb->aiocb is NULL'ed when cancelling. Fix a potential use-after-free by removing the bottom half and enqueuing the completion directly. Fixes: 38f4ac65ac88 ("hw/nvme: reimplement flush to allow cancellation") Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> * hw/nvme: fix aio cancel in zone reset If the zone reset operation is cancelled but the block unmap operation completes normally, the callback will continue resetting the next zone since it neglects to check iocb->ret which will have been set to -ECANCELED. Make sure that this is checked and bail out if an error is present. Secondly, fix a potential use-after-free by removing the bottom half and enqueuing the completion directly. Fixes: 63d96e4ffd71 ("hw/nvme: reimplement zone reset to allow cancellation") Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> * hw/nvme: fix aio cancel in dsm When the DSM operation is cancelled asynchronously, we set iocb->ret to -ECANCELED. However, the callback function only checks the return value of the completed aio, which may have completed succesfully prior to the cancellation and thus the callback ends up continuing the dsm operation instead of bailing out. Fix this. Secondly, fix a potential use-after-free by removing the bottom half and enqueuing the completion directly. Fixes: d7d1474fd85d ("hw/nvme: reimplement dsm to allow cancellation") Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> * hw/nvme: remove copy bh scheduling Fix a potential use-after-free by removing the bottom half and enqueuing the completion directly. Fixes: 796d20681d9b ("hw/nvme: reimplement the copy command to allow aio cancellation") Reviewed-by: Keith Busch <kbusch@kernel.org> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> * target/i386: allow MMX instructions with CR4.OSFXSR=0 MMX state is saved/restored by FSAVE/FRSTOR so the instructions are not illegal opcodes even if CR4.OSFXSR=0. Make sure that validate_vex takes into account the prefix and only checks HF_OSFXSR_MASK in the presence of an SSE instruction. Fixes: 20581aadec5e ("target/i386: validate VEX prefixes via the instructions' exception classes", 2022-10-18) Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1350 Reported-by: Helge Konetzka (@hejko on gitlab.com) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> * target/i386: Always completely initialize TranslateFault In get_physical_address, the canonical address check failed to set TranslateFault.stage2, which resulted in an uninitialized read from the struct when reporting the fault in x86_cpu_tlb_fill. Adjust all error paths to use structure assignment so that the entire struct is always initialized. Reported-by: Daniel Hoffman <dhoff749@gmail.com> Fixes: 9bbcf372193a ("target/i386: Reorg GET_HPHYS") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Message-Id: <20221201074522.178498-1-richard.henderson@linaro.org> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1324 Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> * hw/loongarch/virt: Add cfi01 pflash device Add cfi01 pflash device for LoongArch virt machine Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20221130100647.398565-1-yangxiaojuan@loongson.cn> Signed-off-by: Song Gao <gaosong@loongson.cn> * Sync pc on breakpoints * tests/qtest/migration-test: Fix unlink error and memory leaks When running the migration test compiled with Clang from Fedora 37 and sanitizers enabled, there is an error complaining about unlink(): ../tests/qtest/migration-test.c:1072:12: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/unistd.h:858:48: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../tests/qtest/migration-test.c:1072:12 in (test program exited with status code 1) TAP parsing error: Too few tests run (expected 33, got 20) The data->clientcert and data->clientkey pointers can indeed be unset in some tests, so we have to check them before calling unlink() with those. While we're at it, I also noticed that the code is only freeing some but not all of the allocated strings in this function, and indeed, valgrind is also complaining about memory leaks here. So let's call g_free() on all allocated strings to avoid leaking memory here. Message-Id: <20221125083054.117504-1-thuth@redhat.com> Tested-by: Bin Meng <bmeng@tinylab.org> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> * target/s390x/tcg: Fix and improve the SACF instruction The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be used from problem space, too. Just the switching to the home address space is privileged and should still generate a privilege exception. This bug is e.g. causing programs like Java that use the "getcpu" vdso kernel function to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ). While we're at it, also check if DAT is not enabled. In that case the instruction is supposed to generate a special operation exception. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655 Message-Id: <20221201184443.136355-1-thuth@redhat.com> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com> Reviewed-by: David Hildenbrand <david@redhat.com> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com> * hw/display/next-fb: Fix comment typo Signed-off-by: Evgeny Ermakov <evgeny.v.ermakov@gmail.com> Message-Id: <20221125160849.23711-1-evgeny.v.ermakov@gmail.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com> * fix dev snapshots * working syx snaps * Revert "hw/loongarch/virt: Add cfi01 pflash device" This reverts commit 14dccc8ea6ece7ee63273144fb55e4770a05e0fd. Signed-off-by: Song Gao <gaosong@loongson.cn> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-Id: <20221205113007.683505-1-gaosong@loongson.cn> * Update VERSION for v7.2.0-rc4 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Signed-off-by: Cédric Le Goater <clg@kaod.org> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Igor Mammedov <imammedo@redhat.com> Signed-off-by: Ani Sinha <ani@anisinha.ca> Signed-off-by: John Snow <jsnow@redhat.com> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Signed-off-by: Song Gao <gaosong@loongson.cn> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Claudio Fontana <cfontana@suse.de> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Signed-off-by: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Evgeny Ermakov <evgeny.v.ermakov@gmail.com> Signed-off-by: Klaus Jensen <k.jensen@samsung.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Co-authored-by: Stefan Weil <sw@weilnetz.de> Co-authored-by: Cédric Le Goater <clg@kaod.org> Co-authored-by: Alex Bennée <alex.bennee@linaro.org> Co-authored-by: Peter Maydell <peter.maydell@linaro.org> Co-authored-by: Stefano Garzarella <sgarzare@redhat.com> Co-authored-by: Igor Mammedov <imammedo@redhat.com> Co-authored-by: Ani Sinha <ani@anisinha.ca> Co-authored-by: John Snow <jsnow@redhat.com> Co-authored-by: Michael S. Tsirkin <mst@redhat.com> Co-authored-by: Xiaojuan Yang <yangxiaojuan@loongson.cn> Co-authored-by: Stefan Hajnoczi <stefanha@redhat.com> Co-authored-by: Ard Biesheuvel <ardb@kernel.org> Co-authored-by: Thomas Huth <thuth@redhat.com> Co-authored-by: Joelle van Dyne <j@getutm.app> Co-authored-by: Claudio Fontana <cfontana@suse.de> Co-authored-by: Michael Tokarev <mjt@tls.msk.ru> Co-authored-by: Dongwon Kim <dongwon.kim@intel.com> Co-authored-by: Marc Hartmayer <mhartmay@linux.ibm.com> Co-authored-by: Stefan Weil via <qemu-devel@nongnu.org> Co-authored-by: Gerd Hoffmann <kraxel@redhat.com> Co-authored-by: Richard Henderson <richard.henderson@linaro.org> Co-authored-by: Philippe Mathieu-Daudé <philmd@linaro.org> Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Co-authored-by: Evgeny Ermakov <evgeny.v.ermakov@gmail.com> Co-authored-by: Klaus Jensen <k.jensen@samsung.com> Co-authored-by: Paolo Bonzini <pbonzini@redhat.com> Co-authored-by: Song Gao <gaosong@loongson.cn>
594 lines
27 KiB
ReStructuredText
594 lines
27 KiB
ReStructuredText
.. _submitting-a-patch:
|
||
|
||
Submitting a Patch
|
||
==================
|
||
|
||
QEMU welcomes contributions to fix bugs, add functionality or improve
|
||
the documentation. However, we get a lot of patches, and so we have
|
||
some guidelines about submitting them. If you follow these, you'll
|
||
help make our task of contribution review easier and your change is
|
||
likely to be accepted and committed faster.
|
||
|
||
This page seems very long, so if you are only trying to post a quick
|
||
one-shot fix, the bare minimum we ask is that:
|
||
|
||
.. list-table:: Minimal Checklist for Patches
|
||
:widths: 35 65
|
||
:header-rows: 1
|
||
|
||
* - Check
|
||
- Reason
|
||
* - Patches contain Signed-off-by: Real Name <author@email>
|
||
- States you are legally able to contribute the code. See :ref:`patch_emails_must_include_a_signed_off_by_line`
|
||
* - Sent as patch emails to ``qemu-devel@nongnu.org``
|
||
- The project uses an email list based workflow. See :ref:`submitting_your_patches`
|
||
* - Be prepared to respond to review comments
|
||
- Code that doesn't pass review will not get merged. See :ref:`participating_in_code_review`
|
||
|
||
You do not have to subscribe to post (list policy is to reply-to-all to
|
||
preserve CCs and keep non-subscribers in the loop on the threads they
|
||
start), although you may find it easier as a subscriber to pick up good
|
||
ideas from other posts. If you do subscribe, be prepared for a high
|
||
volume of email, often over one thousand messages in a week. The list is
|
||
moderated; first-time posts from an email address (whether or not you
|
||
subscribed) may be subject to some delay while waiting for a moderator
|
||
to allow your address.
|
||
|
||
The larger your contribution is, or if you plan on becoming a long-term
|
||
contributor, then the more important the rest of this page becomes.
|
||
Reading the table of contents below should already give you an idea of
|
||
the basic requirements. Use the table of contents as a reference, and
|
||
read the parts that you have doubts about.
|
||
|
||
.. contents:: Table of Contents
|
||
|
||
.. _writing_your_patches:
|
||
|
||
Writing your Patches
|
||
--------------------
|
||
|
||
.. _use_the_qemu_coding_style:
|
||
|
||
Use the QEMU coding style
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
You can run run *scripts/checkpatch.pl <patchfile>* before submitting to
|
||
check that you are in compliance with our coding standards. Be aware
|
||
that ``checkpatch.pl`` is not infallible, though, especially where C
|
||
preprocessor macros are involved; use some common sense too. See also:
|
||
|
||
- :ref:`coding-style`
|
||
- `Automate a checkpatch run on
|
||
commit <https://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__
|
||
|
||
.. _base_patches_against_current_git_master:
|
||
|
||
Base patches against current git master
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
There's no point submitting a patch which is based on a released version
|
||
of QEMU because development will have moved on from then and it probably
|
||
won't even apply to master. We only apply selected bugfixes to release
|
||
branches and then only as backports once the code has gone into master.
|
||
|
||
It is also okay to base patches on top of other on-going work that is
|
||
not yet part of the git master branch. To aid continuous integration
|
||
tools, such as `patchew <http://patchew.org/QEMU/>`__, you should `add a
|
||
tag <https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01288.html>`__
|
||
line ``Based-on: $MESSAGE_ID`` to your cover letter to make the series
|
||
dependency obvious.
|
||
|
||
.. _split_up_long_patches:
|
||
|
||
Split up long patches
|
||
~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Split up longer patches into a patch series of logical code changes.
|
||
Each change should compile and execute successfully. For instance, don't
|
||
add a file to the makefile in patch one and then add the file itself in
|
||
patch two. (This rule is here so that people can later use tools like
|
||
`git bisect <http://git-scm.com/docs/git-bisect>`__ without hitting
|
||
points in the commit history where QEMU doesn't work for reasons
|
||
unrelated to the bug they're chasing.) Put documentation first, not
|
||
last, so that someone reading the series can do a clean-room evaluation
|
||
of the documentation, then validate that the code matched the
|
||
documentation. A commit message that mentions "Also, ..." is often a
|
||
good candidate for splitting into multiple patches. For more thoughts on
|
||
properly splitting patches and writing good commit messages, see `this
|
||
advice from
|
||
OpenStack <https://wiki.openstack.org/wiki/GitCommitMessages>`__.
|
||
|
||
.. _make_code_motion_patches_easy_to_review:
|
||
|
||
Make code motion patches easy to review
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
If a series requires large blocks of code motion, there are tricks for
|
||
making the refactoring easier to review. Split up the series so that
|
||
semantic changes (or even function renames) are done in a separate patch
|
||
from the raw code motion. Use a one-time setup of ``git config
|
||
diff.renames true;`` ``git config diff.algorithm patience`` (refer to
|
||
`git-config <http://git-scm.com/docs/git-config>`__). The 'diff.renames'
|
||
property ensures file rename patches will be given in a more compact
|
||
representation that focuses only on the differences across the file
|
||
rename, instead of showing the entire old file as a deletion and the new
|
||
file as an insertion. Meanwhile, the 'diff.algorithm' property ensures
|
||
that extracting a non-contiguous subset of one file into a new file, but
|
||
where all extracted parts occur in the same order both before and after
|
||
the patch, will reduce churn in trying to treat unrelated ``}`` lines in
|
||
the original file as separating hunks of changes.
|
||
|
||
Ideally, a code motion patch can be reviewed by doing::
|
||
|
||
git format-patch --stdout -1 > patch;
|
||
diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch)
|
||
|
||
to focus on the few changes that weren't wholesale code motion.
|
||
|
||
.. _dont_include_irrelevant_changes:
|
||
|
||
Don't include irrelevant changes
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
In particular, don't include formatting, coding style or whitespace
|
||
changes to bits of code that would otherwise not be touched by the
|
||
patch. (It's OK to fix coding style issues in the immediate area (few
|
||
lines) of the lines you're changing.) If you think a section of code
|
||
really does need a reindent or other large-scale style fix, submit this
|
||
as a separate patch which makes no semantic changes; don't put it in the
|
||
same patch as your bug fix.
|
||
|
||
For smaller patches in less frequently changed areas of QEMU, consider
|
||
using the :ref:`trivial-patches` process.
|
||
|
||
.. _write_a_meaningful_commit_message:
|
||
|
||
Write a meaningful commit message
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Commit messages should be meaningful and should stand on their own as a
|
||
historical record of why the changes you applied were necessary or
|
||
useful.
|
||
|
||
QEMU follows the usual standard for git commit messages: the first line
|
||
(which becomes the email subject line) is "subsystem: single line
|
||
summary of change". Whether the "single line summary of change" starts
|
||
with a capital is a matter of taste, but we prefer that the summary does
|
||
not end in a dot. Look at ``git shortlog -30`` for an idea of sample
|
||
subject lines. Then there is a blank line and a more detailed
|
||
description of the patch, another blank and your Signed-off-by: line.
|
||
Please do not use lines that are longer than 76 characters in your
|
||
commit message (so that the text still shows up nicely with "git show"
|
||
in a 80-columns terminal window).
|
||
|
||
The body of the commit message is a good place to document why your
|
||
change is important. Don't include comments like "This is a suggestion
|
||
for fixing this bug" (they can go below the ``---`` line in the email so
|
||
they don't go into the final commit message). Make sure the body of the
|
||
commit message can be read in isolation even if the reader's mailer
|
||
displays the subject line some distance apart (that is, a body that
|
||
starts with "... so that" as a continuation of the subject line is
|
||
harder to follow).
|
||
|
||
If your patch fixes a commit that is already in the repository, please
|
||
add an additional line with "Fixes: <at-least-12-digits-of-SHA-commit-id>
|
||
("Fixed commit subject")" below the patch description / before your
|
||
"Signed-off-by:" line in the commit message.
|
||
|
||
If your patch fixes a bug in the gitlab bug tracker, please add a line
|
||
with "Resolves: <URL-of-the-bug>" to the commit message, too. Gitlab can
|
||
close bugs automatically once commits with the "Resolved:" keyword get
|
||
merged into the master branch of the project. And if your patch addresses
|
||
a bug in another public bug tracker, you can also use a line with
|
||
"Buglink: <URL-of-the-bug>" for reference here, too.
|
||
|
||
Example::
|
||
|
||
Fixes: 14055ce53c2d ("s390x/tcg: avoid overflows in time2tod/tod2time")
|
||
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/42
|
||
Buglink: https://bugs.launchpad.net/qemu/+bug/1804323``
|
||
|
||
Some other tags that are used in commit messages include "Message-Id:"
|
||
"Tested-by:", "Acked-by:", "Reported-by:", "Suggested-by:". See ``git
|
||
log`` for these keywords for example usage.
|
||
|
||
.. _test_your_patches:
|
||
|
||
Test your patches
|
||
~~~~~~~~~~~~~~~~~
|
||
|
||
Although QEMU uses various :ref:`ci` services that attempt to test
|
||
patches submitted to the list, it still saves everyone time if you
|
||
have already tested that your patch compiles and works. Because QEMU
|
||
is such a large project the default configuration won't create a
|
||
testing pipeline on GitLab when a branch is pushed. See the :ref:`CI
|
||
variable documentation<ci_var>` for details on how to control the
|
||
running of tests; but it is still wise to also check that your patches
|
||
work with a full build before submitting a series, especially if your
|
||
changes might have an unintended effect on other areas of the code you
|
||
don't normally experiment with. See :ref:`testing` for more details on
|
||
what tests are available.
|
||
|
||
Also, it is a wise idea to include a testsuite addition as part of
|
||
your patches - either to ensure that future changes won't regress your
|
||
new feature, or to add a test which exposes the bug that the rest of
|
||
your series fixes. Keeping separate commits for the test and the fix
|
||
allows reviewers to rebase the test to occur first to prove it catches
|
||
the problem, then again to place it last in the series so that
|
||
bisection doesn't land on a known-broken state.
|
||
|
||
.. _submitting_your_patches:
|
||
|
||
Submitting your Patches
|
||
-----------------------
|
||
|
||
The QEMU project uses a public email based workflow for reviewing and
|
||
merging patches. As a result all contributions to QEMU must be **sent
|
||
as patches** to the qemu-devel `mailing list
|
||
<https://wiki.qemu.org/Contribute/MailingLists>`__. Patch
|
||
contributions should not be posted on the bug tracker, posted on
|
||
forums, or externally hosted and linked to. (We have other mailing
|
||
lists too, but all patches must go to qemu-devel, possibly with a Cc:
|
||
to another list.) ``git send-email`` (`step-by-step setup guide
|
||
<https://git-send-email.io/>`__ and `hints and tips
|
||
<https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
|
||
works best for delivering the patch without mangling it, but
|
||
attachments can be used as a last resort on a first-time submission.
|
||
|
||
.. _if_you_cannot_send_patch_emails:
|
||
|
||
If you cannot send patch emails
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
In rare cases it may not be possible to send properly formatted patch
|
||
emails. You can use `sourcehut <https://sourcehut.org/>`__ to send your
|
||
patches to the QEMU mailing list by following these steps:
|
||
|
||
#. Register or sign in to your account
|
||
#. Add your SSH public key in `meta \|
|
||
keys <https://meta.sr.ht/keys>`__.
|
||
#. Publish your git branch using **git push git@git.sr.ht:~USERNAME/qemu
|
||
HEAD**
|
||
#. Send your patches to the QEMU mailing list using the web-based
|
||
``git-send-email`` UI at https://git.sr.ht/~USERNAME/qemu/send-email
|
||
|
||
`This video
|
||
<https://spacepub.space/videos/watch/ad258d23-0ac6-488c-83fc-2bacf578de3a>`__
|
||
shows the web-based ``git-send-email`` workflow. Documentation is
|
||
available `here
|
||
<https://man.sr.ht/git.sr.ht/#sending-patches-upstream>`__.
|
||
|
||
.. _cc_the_relevant_maintainer:
|
||
|
||
CC the relevant maintainer
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Send patches both to the mailing list and CC the maintainer(s) of the
|
||
files you are modifying. look in the MAINTAINERS file to find out who
|
||
that is. Also try using scripts/get_maintainer.pl from the repository
|
||
for learning the most common committers for the files you touched.
|
||
|
||
Example::
|
||
|
||
~/src/qemu/scripts/get_maintainer.pl -f hw/ide/core.c
|
||
|
||
In fact, you can automate this, via a one-time setup of ``git config
|
||
sendemail.cccmd 'scripts/get_maintainer.pl --nogit-fallback'`` (Refer to
|
||
`git-config <http://git-scm.com/docs/git-config>`__.)
|
||
|
||
.. _do_not_send_as_an_attachment:
|
||
|
||
Do not send as an attachment
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Send patches inline so they are easy to reply to with review comments.
|
||
Do not put patches in attachments.
|
||
|
||
.. _use_git_format_patch:
|
||
|
||
Use ``git format-patch``
|
||
~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Use the right diff format.
|
||
`git format-patch <http://git-scm.com/docs/git-format-patch>`__ will
|
||
produce patch emails in the right format (check the documentation to
|
||
find out how to drive it). You can then edit the cover letter before
|
||
using ``git send-email`` to mail the files to the mailing list. (We
|
||
recommend `git send-email <http://git-scm.com/docs/git-send-email>`__
|
||
because mail clients often mangle patches by wrapping long lines or
|
||
messing up whitespace. Some distributions do not include send-email in a
|
||
default install of git; you may need to download additional packages,
|
||
such as 'git-email' on Fedora-based systems.) Patch series need a cover
|
||
letter, with shallow threading (all patches in the series are
|
||
in-reply-to the cover letter, but not to each other); single unrelated
|
||
patches do not need a cover letter (but if you do send a cover letter,
|
||
use ``--numbered`` so the cover and the patch have distinct subject lines).
|
||
Patches are easier to find if they start a new top-level thread, rather
|
||
than being buried in-reply-to another existing thread.
|
||
|
||
.. _avoid_posting_large_binary_blob:
|
||
|
||
Avoid posting large binary blob
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
If you added binaries to the repository, consider producing the patch
|
||
emails using ``git format-patch --no-binary`` and include a link to a
|
||
git repository to fetch the original commit.
|
||
|
||
.. _patch_emails_must_include_a_signed_off_by_line:
|
||
|
||
Patch emails must include a ``Signed-off-by:`` line
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Your patches **must** include a Signed-off-by: line. This is a hard
|
||
requirement because it's how you say "I'm legally okay to contribute
|
||
this and happy for it to go into QEMU". The process is modelled after
|
||
the `Linux kernel
|
||
<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
|
||
policy.
|
||
|
||
If you wrote the patch, make sure your "From:" and "Signed-off-by:"
|
||
lines use the same spelling. It's okay if you subscribe or contribute to
|
||
the list via more than one address, but using multiple addresses in one
|
||
commit just confuses things. If someone else wrote the patch, git will
|
||
include a "From:" line in the body of the email (different from your
|
||
envelope From:) that will give credit to the correct author; but again,
|
||
that author's Signed-off-by: line is mandatory, with the same spelling.
|
||
|
||
There are various tooling options for automatically adding these tags
|
||
include using ``git commit -s`` or ``git format-patch -s``. For more
|
||
information see `SubmittingPatches 1.12
|
||
<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
|
||
|
||
.. _include_a_meaningful_cover_letter:
|
||
|
||
Include a meaningful cover letter
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
This is a requirement for any series with multiple patches (as it aids
|
||
continuous integration), but optional for an isolated patch. The cover
|
||
letter explains the overall goal of such a series, and also provides a
|
||
convenient 0/N email for others to reply to the series as a whole. A
|
||
one-time setup of ``git config format.coverletter auto`` (refer to
|
||
`git-config <http://git-scm.com/docs/git-config>`__) will generate the
|
||
cover letter as needed.
|
||
|
||
When reviewers don't know your goal at the start of their review, they
|
||
may object to early changes that don't make sense until the end of the
|
||
series, because they do not have enough context yet at that point of
|
||
their review. A series where the goal is unclear also risks a higher
|
||
number of review-fix cycles because the reviewers haven't bought into
|
||
the idea yet. If the cover letter can explain these points to the
|
||
reviewer, the process will be smoother patches will get merged faster.
|
||
Make sure your cover letter includes a diffstat of changes made over the
|
||
entire series; potential reviewers know what files they are interested
|
||
in, and they need an easy way determine if your series touches them.
|
||
|
||
.. _use_the_rfc_tag_if_needed:
|
||
|
||
Use the RFC tag if needed
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
For example, "[PATCH RFC v2]". ``git format-patch --subject-prefix=RFC``
|
||
can help.
|
||
|
||
"RFC" means "Request For Comments" and is a statement that you don't
|
||
intend for your patchset to be applied to master, but would like some
|
||
review on it anyway. Reasons for doing this include:
|
||
|
||
- the patch depends on some pending kernel changes which haven't yet
|
||
been accepted, so the QEMU patch series is blocked until that
|
||
dependency has been dealt with, but is worth reviewing anyway
|
||
- the patch set is not finished yet (perhaps it doesn't cover all use
|
||
cases or work with all targets) but you want early review of a major
|
||
API change or design structure before continuing
|
||
|
||
In general, since it's asking other people to do review work on a
|
||
patchset that the submitter themselves is saying shouldn't be applied,
|
||
it's best to:
|
||
|
||
- use it sparingly
|
||
- in the cover letter, be clear about why a patch is an RFC, what areas
|
||
of the patchset you're looking for review on, and why reviewers
|
||
should care
|
||
|
||
.. _consider_whether_your_patch_is_applicable_for_stable:
|
||
|
||
Consider whether your patch is applicable for stable
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
If your patch fixes a severe issue or a regression, it may be applicable
|
||
for stable. In that case, consider adding ``Cc: qemu-stable@nongnu.org``
|
||
to your patch to notify the stable maintainers.
|
||
|
||
For more details on how QEMU's stable process works, refer to the
|
||
:ref:`stable-process` page.
|
||
|
||
.. _participating_in_code_review:
|
||
|
||
Participating in Code Review
|
||
----------------------------
|
||
|
||
All patches submitted to the QEMU project go through a code review
|
||
process before they are accepted. This will often mean a series will
|
||
go through a number of iterations before being picked up by
|
||
:ref:`maintainers<maintainers>`. You therefore should be prepared to
|
||
read replies to your messages and be willing to act on them.
|
||
|
||
Maintainers are often willing to manually fix up first-time
|
||
contributions, since there is a learning curve involved in making an
|
||
ideal patch submission. However for the best results you should
|
||
proactively respond to suggestions with changes or justifications for
|
||
your current approach.
|
||
|
||
Some areas of code that are well maintained may review patches
|
||
quickly, lesser-loved areas of code may have a longer delay.
|
||
|
||
.. _stay_around_to_fix_problems_raised_in_code_review:
|
||
|
||
Stay around to fix problems raised in code review
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Not many patches get into QEMU straight away -- it is quite common that
|
||
developers will identify bugs, or suggest a cleaner approach, or even
|
||
just point out code style issues or commit message typos. You'll need to
|
||
respond to these, and then send a second version of your patches with
|
||
the issues fixed. This takes a little time and effort on your part, but
|
||
if you don't do it then your changes will never get into QEMU.
|
||
|
||
Remember that a maintainer is under no obligation to take your
|
||
patches. If someone has spent the time reviewing your code and
|
||
suggesting improvements and you simply re-post without either
|
||
addressing the comment directly or providing additional justification
|
||
for the change then it becomes wasted effort. You cannot demand others
|
||
merge and then fix up your code after the fact.
|
||
|
||
When replying to comments on your patches **reply to all and not just
|
||
the sender** -- keeping discussion on the mailing list means everybody
|
||
can follow it. Remember the spirit of the :ref:`code_of_conduct` and
|
||
keep discussions respectful and collaborative and avoid making
|
||
personal comments.
|
||
|
||
.. _pay_attention_to_review_comments:
|
||
|
||
Pay attention to review comments
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
Someone took their time to review your work, and it pays to respect that
|
||
effort; repeatedly submitting a series without addressing all comments
|
||
from the previous round tends to alienate reviewers and stall your
|
||
patch. Reviewers aren't always perfect, so it is okay if you want to
|
||
argue that your code was correct in the first place instead of blindly
|
||
doing everything the reviewer asked. On the other hand, if someone
|
||
pointed out a potential issue during review, then even if your code
|
||
turns out to be correct, it's probably a sign that you should improve
|
||
your commit message and/or comments in the code explaining why the code
|
||
is correct.
|
||
|
||
If you fix issues that are raised during review **resend the entire
|
||
patch series** not just the one patch that was changed. This allows
|
||
maintainers to easily apply the fixed series without having to manually
|
||
identify which patches are relevant. Send the new version as a complete
|
||
fresh email or series of emails -- don't try to make it a followup to
|
||
version 1. (This helps automatic patch email handling tools distinguish
|
||
between v1 and v2 emails.)
|
||
|
||
.. _when_resending_patches_add_a_version_tag:
|
||
|
||
When resending patches add a version tag
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
All patches beyond the first version should include a version tag -- for
|
||
example, "[PATCH v2]". This means people can easily identify whether
|
||
they're looking at the most recent version. (The first version of a
|
||
patch need not say "v1", just [PATCH] is sufficient.) For patch series,
|
||
the version applies to the whole series -- even if you only change one
|
||
patch, you resend the entire series and mark it as "v2". Don't try to
|
||
track versions of different patches in the series separately. `git
|
||
format-patch <http://git-scm.com/docs/git-format-patch>`__ and `git
|
||
send-email <http://git-scm.com/docs/git-send-email>`__ both understand
|
||
the ``-v2`` option to make this easier. Send each new revision as a new
|
||
top-level thread, rather than burying it in-reply-to an earlier
|
||
revision, as many reviewers are not looking inside deep threads for new
|
||
patches.
|
||
|
||
.. _include_version_history_in_patchset_revisions:
|
||
|
||
Include version history in patchset revisions
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
For later versions of patches, include a summary of changes from
|
||
previous versions, but not in the commit message itself. In an email
|
||
formatted as a git patch, the commit message is the part above the ``---``
|
||
line, and this will go into the git changelog when the patch is
|
||
committed. This part should be a self-contained description of what this
|
||
version of the patch does, written to make sense to anybody who comes
|
||
back to look at this commit in git in six months' time. The part below
|
||
the ``---`` line and above the patch proper (git format-patch puts the
|
||
diffstat here) is a good place to put remarks for people reading the
|
||
patch email, and this is where the "changes since previous version"
|
||
summary belongs. The `git-publish
|
||
<https://github.com/stefanha/git-publish>`__ script can help with
|
||
tracking a good summary across versions. Also, the `git-backport-diff
|
||
<https://github.com/codyprime/git-scripts>`__ script can help focus
|
||
reviewers on what changed between revisions.
|
||
|
||
.. _tips_and_tricks:
|
||
|
||
Tips and Tricks
|
||
---------------
|
||
|
||
.. _proper_use_of_reviewed_by_tags_can_aid_review:
|
||
|
||
Proper use of Reviewed-by: tags can aid review
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
When reviewing a large series, a reviewer can reply to some of the
|
||
patches with a Reviewed-by tag, stating that they are happy with that
|
||
patch in isolation (sometimes conditional on minor cleanup, like fixing
|
||
whitespace, that doesn't affect code content). You should then update
|
||
those commit messages by hand to include the Reviewed-by tag, so that in
|
||
the next revision, reviewers can spot which patches were already clean
|
||
from the previous round. Conversely, if you significantly modify a patch
|
||
that was previously reviewed, remove the reviewed-by tag out of the
|
||
commit message, as well as listing the changes from the previous
|
||
version, to make it easier to focus a reviewer's attention to your
|
||
changes.
|
||
|
||
.. _if_your_patch_seems_to_have_been_ignored:
|
||
|
||
If your patch seems to have been ignored
|
||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||
|
||
If your patchset has received no replies you should "ping" it after a
|
||
week or two, by sending an email as a reply-to-all to the patch mail,
|
||
including the word "ping" and ideally also a link to the page for the
|
||
patch on `patchew <https://patchew.org/QEMU/>`__ or
|
||
`lore.kernel.org <https://lore.kernel.org/qemu-devel/>`__. It's worth
|
||
double-checking for reasons why your patch might have been ignored
|
||
(forgot to CC the maintainer? annoyed people by failing to respond to
|
||
review comments on an earlier version?), but often for less-maintained
|
||
areas of QEMU patches do just slip through the cracks. If your ping is
|
||
also ignored, ping again after another week or so. As the submitter, you
|
||
are the person with the most motivation to get your patch applied, so
|
||
you have to be persistent.
|
||
|
||
.. _is_my_patch_in:
|
||
|
||
Is my patch in?
|
||
~~~~~~~~~~~~~~~
|
||
|
||
QEMU has some Continuous Integration machines that try to catch patch
|
||
submission problems as soon as possible. `patchew
|
||
<http://patchew.org/QEMU/>`__ includes a web interface for tracking the
|
||
status of various threads that have been posted to the list, and may
|
||
send you an automated mail if it detected a problem with your patch.
|
||
|
||
Once your patch has had enough review on list, the maintainer for that
|
||
area of code will send notification to the list that they are including
|
||
your patch in a particular staging branch. Periodically, the maintainer
|
||
then takes care of :ref:`submitting-a-pull-request`
|
||
for aggregating topic branches into mainline QEMU. Generally, you do not
|
||
need to send a pull request unless you have contributed enough patches
|
||
to become a maintainer over a particular section of code. Maintainers
|
||
may further modify your commit, by resolving simple merge conflicts or
|
||
fixing minor typos pointed out during review, but will always add a
|
||
Signed-off-by line in addition to yours, indicating that it went through
|
||
their tree. Occasionally, the maintainer's pull request may hit more
|
||
difficult merge conflicts, where you may be requested to help rebase and
|
||
resolve the problems. It may take a couple of weeks between when your
|
||
patch first had a positive review to when it finally lands in qemu.git;
|
||
release cycle freezes may extend that time even longer.
|
||
|
||
.. _return_the_favor:
|
||
|
||
Return the favor
|
||
~~~~~~~~~~~~~~~~
|
||
|
||
Peer review only works if everyone chips in a bit of review time. If
|
||
everyone submitted more patches than they reviewed, we would have a
|
||
patch backlog. A good goal is to try to review at least as many patches
|
||
from others as what you submit. Don't worry if you don't know the code
|
||
base as well as a maintainer; it's perfectly fine to admit when your
|
||
review is weak because you are unfamiliar with the code.
|