[libvirt] [PATCH RESEND 4/4] NEWS: document new acpi pci hotplug config option
Added the following new libvirt conf option to the release note to indicate their availability with the next release: Signed-off-by: Ani Sinha --- NEWS.rst | 21 + 1 file changed, 21 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 65fb112d11..85e796f274 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -8,6 +8,27 @@ the changes introduced by each of them. For a more fine-grained view, use the `git log`_. +v8.2.0 (unreleased) +=== + +* **Security** + +* **Removed features** + +* **New features** + + * qemu: Add a new global feature for controlling ACPI PCI hotplug on bridges + +A new config option under +sub-element have been added to allow users enable/disable ACPI based PCI +hotplug for cold plugged bridges (that is, bridges that were present in the +domain definition when the guest was first started).This setting is only +applicable for x86 guests using qemu driver. + +* **Improvements** + +* **Bug fixes** + v8.1.0 (unreleased) === -- 2.25.1
[libvirt] [PATCH RESEND 3/4] qemu: command: add support for acpi-bridge-hotplug feature
This change adds backend qemu command line support for new libvirt
global feature 'acpi-bridge-hotplug'. This option can be used as
following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to
qemu for x86 guests:
(pc): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
This change also adds the required qemuxml2argv unit tests in order to
test correct qemu arguments. Unit tests have also been added to test
qemu capability validation checks as well as checks for using this
option with the right architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 19 ++
...-hotplug-bridge-disable.aarch64-latest.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 35 +
...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 38 +++
tests/qemuxml2argvtest.c | 7
6 files changed, 101 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c836799888..206a9794cb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6280,6 +6280,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG];
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6325,6 +6326,24 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
+const char *pm_object = NULL;
+
+if (!qemuDomainIsQ35(def))
+pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
+
+if (pm_object != NULL) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
new file mode 100644
index 00..90527dfefd
--- /dev/null
+++
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}'
\
+-machine pc-i440fx-2.5,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 1024 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
+-boot strict=on \
+-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-device
'{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
+-sandbox
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git
[libvirt] [PATCH RESEND 2/4] conf: introduce support for acpi-bridge-hotplug feature
This change introduces a new libvirt sub-element under
that can be used to configure all pci related features.
Currently the only sub-sub element supported by this sub-element is
'acpi-bridge-hotplug' as shown below:
The above option is only available for the QEMU driver, for x86 guests
only. It is a global option, affecting all PCI bridge controllers on
the guest.
The 'acpi-bridge-hotplug' option enables or disables ACPI hotplug
support for cold-plugged pci bridges. Examples of bridges include the
PCI-PCI bridge (pci-bridge controller) for pc (i440fx) machinetypes,
or PCIe-PCI bridges and pcie-root-port controllers for q35
machinetypes.
For pc machinetypes in x86, this option has been available in QEMU
since version 2.1. Please see the following changes in qemu repo:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
bridge hotplug is disabled")
For q35 machinetypes, this was introduced in QEMU 6.1 with the
following changes in qemu repo:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
(b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based
machines (as opposed to native hotplug) are outlined in (b). There are
use cases where users would still want to use native
hotplug. Therefore, this config option enables users to choose either
ACPI based hotplug or native hotplug for bridges (for example for pcie
root port controller in q35 machines).
Qemu capability validation checks have also been added along with
related unit tests to exercise the new conf option.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 32 +++
docs/schemas/domaincommon.rng | 15
src/conf/domain_conf.c| 89 ++-
src/conf/domain_conf.h| 9 ++
src/qemu/qemu_validate.c | 42 +
.../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
.../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36
.../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36
.../q35-acpi-hotplug-bridge-disable.xml | 53 +++
.../q35-acpi-hotplug-bridge-enable.xml| 53 +++
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
15 files changed, 385 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9202cd3107..864dfd8bd8 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1879,6 +1879,9 @@ Hypervisors may allow certain CPU / machine features to
be toggled on/off.
+
+
+
@@ -1997,6 +2000,35 @@ are:
passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode -
optional string sync_pt or share_pt :since:`6.3.0`
=== ==
=== ==
+``pci``
+ Various PCI bus related features of the hypervisor.
+
+
=
=== ==
+ Feature Description
Value Since
+
=
=== ==
+ acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI
bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`8.2.0`
+
=
=== ==
+
+ Note:
+ pc machine types (i440fx) have ACPI hotplug enabled
[libvirt] [PATCH RESEND 1/4] qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
qemu added support for i440fx specific global boolean flag
PIIX4_PM.acpi-pci-hotplug-with-bridge-support
around version 2.1. This flag is enabled by default. When disabled, it
turns off acpi pci hotplug for cold plugged pci bridges in i440fx
machine types.
Very recently, in qemu version 6.1, the same global option was also
added for q35 machine types as well.
ICH9-LPC.acpi-pci-hotplug-with-bridge-support
This option turns on or off acpi based hotplug for cold plugged pcie
bridges like pcie root ports. This flag is also enabled by
default. Please refer to the following qemu changes:
c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
This patch adds the corresponding qemu capabilities in libvirt. For
i440fx, the capability is detected as
QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is
detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
Please note that the test specific qemu capabilities .replies files
has already been updated as a part of regular refreshing them when a
new qemu version is released. Hence, no updates to those files are
required.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
5 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 529e9ceaf5..08d5d733ce 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -665,6 +665,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"virtio-mem-pci.prealloc", /*
QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC */
"calc-dirty-rate", /* QEMU_CAPS_CALC_DIRTY_RATE */
"dirtyrate-param.mode", /* QEMU_CAPS_DIRTYRATE_MODE */
+
+ /* 425 */
+ "ich9.acpi-hotplug-bridge", /*
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */
);
@@ -1551,6 +1554,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsVirtioGpu[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = {
{ "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL },
+{ "acpi-pci-hotplug-with-bridge-support",
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] =
{
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f6188b42de..51dc668913 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -641,6 +641,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_CALC_DIRTY_RATE, /* accepts calc-dirty-rate */
QEMU_CAPS_DIRTYRATE_MODE , /* calc-dirty-rate accepts mode parameter */
+/* 425 */
+QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35
ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index ba1aecc37e..51e1e07d2f 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -239,6 +239,7 @@
+
6001000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index d77907af55..7b665c82e8 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
+
6002000
0
43100244
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index ae800abcc4..692e2f14da 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6002050
0
43100243
--
2.25.1
[libvirt] [PATCH RESEND 0/4] re-introduce
I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of hot-plugging switches with PCIe Native hotplug since ACPI PCI Hot-plug only works with cold-plugged bridges. If the user wants to explicitely use this feature, they can disable ACPI PCI Hot-plug with: --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug instead of PCIe Native. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-5-imamm...@redhat.com> Reviewed-by: Ani Sinha Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin The patch description says it all. Instead of masking out the HPC bit in pcie slots, we keep them turned on. Instead, we do not advertize native hotplug capability for PCIE using _OSC control method. See section 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for these slots so now the guest OS can select ACPI hotplug instead. The second change is introduction of a property with which we keep the existing behavior for pc-q35-6.1 machines. This means HPC bit is masked and ACPI hotplug is enabled by default for pcie root ports. The QEMU commit is: (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb Author: Julia Suvorova Date: Fri Nov 12 06:08:54 2021 -0500 hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be turned on, while the switch to ACPI Hot-plug will be done in the DSDT table. Introducing 'x-keep-native-hpc' property disables the HPC bit only in 6.1 and as a result keeps the forced 'reserve-io' on pcie-root-ports in 6.1 too. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-3-imamm...@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Lastly, as a related side note, because from QEMU 6.2 onwards, we do not mask out HPC bit in PCIE, the work done by this patch is no longer needed: (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820 Author: Marcel Apfelbaum Date: Mon Aug 2 12:00:57 2021 +0300 hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Q35 has now ACPI hotplug enabled by default for PCI(e) devices. As opposed to native PCIe hotplug, guests like Fedora 34 will not assign IO range to pcie-root-ports not supporting native hotplug, resulting into a regression. Reproduce by: qemu-bin -M q35 -device pcie-root-port,id=p1
Re: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
On Mon, Feb 28, 2022 at 11:24:07AM -0500, Laine Stump wrote: > On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: > > Now that the virNWFilterBinding APIs are using the nwfilter > > update lock directly, there is no need for the virt drivers > > to do it themselves. > > I'm always nervous when the ordering of locks is changed, and that's what is > happening with the combination of this patch and the previous patch. Before > it was always the NWFilterLock that was acquired first, and then the domain > object is retrieved (which involves getting the domain-list lock while > getting a ref to the domain object). > > But since holding of the domain-list lock is localized to that short period > (and certainly never across a call to the NWFilter binding API) I'm fairly > certain this (along with the previous patch) is safe from creating any new > deadlocks. Note the reason for that ordering was that in the past the nwfilter driver would have ability to call back into the virt driver to ask the virt driver to reload all nwfilters for VMs. With this callback we would acquire the nwfilter update lock, and then iterate over each VM in turn. The nwfilter driver no longer has this ability. It is completely self contained when re-loadeding nwfilters. The only calls are from the virt driver into the nwfilter driver. Thus there can never be any scenario where a nwfilter driver lock is held when the virt driver tries to acquire a domain lock. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 5/5] nwfilter: make some gentech driver methods static
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: The virNWFilterTechDriverForName & virNWFilterUpdateInstantiateFilter methods are only used within the same source file, so don't need to be exported. Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump (again, I didn't even look to the source to verify; the fact that it still compiles proves that you're right. Hooray for machines doing (part of) our work for us!)
Re: [libvirt PATCH 4/5] nwfilter: remove decl of virNWFilterCreateVarHashmap
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: This method doesn't exist since commit d1a7c08eb145d8b9d9c4a268f43b1590049a Author: Daniel P. Berrangé Date: Thu Apr 26 12:26:51 2018 +0100 nwfilter: convert the gentech driver code to use virNWFilterBindingDefPtr Signed-off-by: Daniel P. Berrangé The proof is in the successful compile without it :-) Reviewed-by: Laine Stump
Re: [libvirt PATCH 3/5] qemu,lxc: remove use to nwfilter update lock
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: Now that the virNWFilterBinding APIs are using the nwfilter update lock directly, there is no need for the virt drivers to do it themselves. I'm always nervous when the ordering of locks is changed, and that's what is happening with the combination of this patch and the previous patch. Before it was always the NWFilterLock that was acquired first, and then the domain object is retrieved (which involves getting the domain-list lock while getting a ref to the domain object). But since holding of the domain-list lock is localized to that short period (and certainly never across a call to the NWFilter binding API) I'm fairly certain this (along with the previous patch) is safe from creating any new deadlocks. Signed-off-by: Daniel P. Berrangé Reviewed-by: Laine Stump --- src/lxc/lxc_driver.c | 6 -- src/qemu/qemu_driver.c| 18 -- src/qemu/qemu_migration.c | 3 --- src/qemu/qemu_process.c | 4 4 files changed, 31 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 020ec257ae..4185a61267 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -971,8 +971,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1); -virNWFilterReadLockFilterUpdates(); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; @@ -1014,7 +1012,6 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom, cleanup: virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -1080,8 +1077,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, if (flags & VIR_DOMAIN_START_VALIDATE) parse_flags |= VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; -virNWFilterReadLockFilterUpdates(); - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -1138,7 +1133,6 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, cleanup: virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); -virNWFilterUnlockFilterUpdates(); return dom; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b74b0375a7..e4e1cd7bdf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1606,8 +1606,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, if (flags & VIR_DOMAIN_START_RESET_NVRAM) start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; -virNWFilterReadLockFilterUpdates(); - if (!(def = virDomainDefParseString(xml, driver->xmlopt, NULL, parse_flags))) goto cleanup; @@ -1661,7 +1659,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, virDomainObjEndAPI(); virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); -virNWFilterUnlockFilterUpdates(); return dom; } @@ -5776,8 +5773,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (flags & VIR_DOMAIN_SAVE_RESET_NVRAM) reset_nvram = true; -virNWFilterReadLockFilterUpdates(); - fd = qemuSaveImageOpen(driver, NULL, path, , , (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, , false, false); @@ -5849,7 +5844,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (vm && ret < 0) qemuDomainRemoveInactiveJob(driver, vm); virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -6403,8 +6397,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_START_FORCE_BOOT | VIR_DOMAIN_START_RESET_NVRAM, -1); -virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -6433,7 +6425,6 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) cleanup: virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -7815,8 +7806,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, virDomainObj *vm = NULL; int ret = -1; -virNWFilterReadLockFilterUpdates(); - if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; @@ -7839,7 +7828,6 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(); -virNWFilterUnlockFilterUpdates(); return ret; } @@ -7869,8 +7857,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); -virNWFilterReadLockFilterUpdates(); - cfg = virQEMUDriverGetConfig(driver); if (!(vm = qemuDomainObjFromDomain(dom))) @@ -7947,7 +7933,6 @@ static int
Re: [libvirt PATCH 2/5] nwfilter: hold filter update lock when creating/deleting bindings
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The nwfilter update lock is historically acquired by the virt
drivers in order to achieve serialization between nwfilter
define/undefine, and instantiation/teardown of filters.
When running in the modular daemons, however, the mutex that
the virt drivers are locking is in a completely different
process from the mutex that the nwfilter driver is locking.
Serialization is lost and thus call from the virt driver to
virNWFilterBindingCreateXML can deadlock with a concurrent
call to the virNWFilterDefineXML method.
The solution is surprisingly easy, the update lock simply
needs acquiring in the virNWFilterBindingCreateXML method
and virNWFilterBindingUndefine method instead of in the
virt drivers.
The only semantic difference here is that when a virtual
machine has multiple NICs, the instantiation and teardown
of filters is no longer serialized for the whole VM, but
rather for each NIC. This should not be a problem since
the virt drivers already need to cope with tearing down
a partially created VM where only some of the NICs are
setup.
Signed-off-by: Daniel P. Berrangé
Reviewed-by: Laine Stump
(I considered this patch together with 3/5 - see my comment there of why
I think the re-ordering of the locking is safe)
---
src/nwfilter/nwfilter_driver.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 08f138dd79..3ce8fce7f9 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -760,11 +760,14 @@ nwfilterBindingCreateXML(virConnectPtr conn,
if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter)))
goto cleanup;
+virNWFilterReadLockFilterUpdates();
if (virNWFilterInstantiateFilter(driver, def) < 0) {
+virNWFilterUnlockFilterUpdates();
virNWFilterBindingObjListRemove(driver->bindings, obj);
g_clear_pointer(, virObjectUnref);
goto cleanup;
}
+virNWFilterUnlockFilterUpdates();
virNWFilterBindingObjSave(obj, driver->bindingDir);
cleanup:
@@ -801,7 +804,9 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding)
if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0)
goto cleanup;
+virNWFilterReadLockFilterUpdates();
virNWFilterTeardownFilter(def);
+virNWFilterUnlockFilterUpdates();
virNWFilterBindingObjDelete(obj, driver->bindingDir);
virNWFilterBindingObjListRemove(driver->bindings, obj);
Re: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
On Mon, Feb 28, 2022 at 11:05:09AM -0500, Laine Stump wrote: > On 2/25/22 10:30 AM, Daniel P. Berrangé wrote: > > The virNWFilterLockIface method is only called from one place, > > the learnIPAddressThread method. > > nwfilter_gentech_driver.c:virNWFilterDoInstantiate() and > _virNWFilterTeardownFilter() also call virNWFilterLockIface. Sigh, yes. I didn't properly consider that stuff outside of learnip.c would be using its mutex. We should move the iface locking into the gentech driver to make it clear is is shared functionality. Consider this patch dropped. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 1/5] nwfilter: stop using recursive mutex for IP learning
On 2/25/22 10:30 AM, Daniel P. Berrangé wrote:
The virNWFilterLockIface method is only called from one place,
the learnIPAddressThread method.
nwfilter_gentech_driver.c:virNWFilterDoInstantiate() and
_virNWFilterTeardownFilter() also call virNWFilterLockIface.
And there is this call chain from learnIPAddressThread() that ends up at
one or the other of those:
learnIPAddressThread()
virNWFilterInstantiateFilterLate()
virNWFilterInstantiateFilterUpdate()
virNWFilterDoInstantiate() <
_virNWFilterTeardownFilter() <
But of course just prior to calling virNWFilterINstantiateFilterLate(),
learnIPAddressThread() calls:
virNWFilterUnlockIface(req->binding->portdevname);
which matches the previous virNWFilterLockIface(), so there shouldn't be
a problem for the learnIPAddressThread at least.
I'm not sure about the threads of other calls to
virNWFilterDoInstantiate/TeardownFilter though, haven't tried to
decipher them yet.
This is the entry point for
the learning thread on the interface in question. As such
there is never any recursive locking on this mutex from the
same thread. This appears to have been the case since the
code was first introduced. Thus a plain mutex is sufficient.
Signed-off-by: Daniel P. Berrangé
---
src/nwfilter/nwfilter_learnipaddr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/nwfilter/nwfilter_learnipaddr.c
b/src/nwfilter/nwfilter_learnipaddr.c
index fb552bd1e6..4840b0539f 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -151,7 +151,7 @@ virNWFilterLockIface(const char *ifname)
if (!ifaceLock) {
ifaceLock = g_new0(virNWFilterIfaceLock, 1);
-if (virMutexInitRecursive(>lock) < 0) {
+if (virMutexInit(>lock) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("mutex initialization failed"));
g_free(ifaceLock);
Re: Libvirt Rust bindings could use some work
On Mon, Feb 21, 2022 at 11:53 AM Wim de With wrote: > > > Since the intent of libvirt using LGPL was explicitly to allow non-GPL > > compatible applications to use libvirt, it is a mistake to be using > > a license that breaks this ability for Rust. > > > > In Golang we've used MIT license > > > > For Rust we should aim for whatever is most appropriate - MIT or BSD > > or Apache - I'm not familiar with which is dominant in the Rust world. > > Apache and MIT or even dual licensing of both are the most common. > The official API guidelines recommend dual licensing under Apache and > MIT. > I would prefer we didn't repeat that dumb advice in libvirt-rs. Just go with Apache-2.0 if we want a permissively licensed crate. I would suggest MPL-2.0 for libvirt-rs, though. That allows proprietary linking but maintains that each file that makes up the crate *must* remain FOSS, and is compatible with GNU licenses. It's static-link compatible copyleft, basically. -- 真実はいつも一つ！/ Always, there's only one truth!
[PATCH] libxl: remove redundant variable from libxlDomainJobObj
It makes no sense to have 'started' variable in the
libxlDomainJobObj as the same one is already in virDomainJobData,
but never used.
Signed-off-by: Kristina Hanicova
---
src/libxl/libxl_domain.c | 10 +-
src/libxl/libxl_domain.h | 1 -
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ee031267ca..205049f98a 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -118,7 +118,7 @@ libxlDomainObjBeginJob(libxlDriverPrivate *driver
G_GNUC_UNUSED,
VIR_DEBUG("Starting job: %s", libxlDomainJobTypeToString(job));
priv->job.active = job;
priv->job.owner = virThreadSelfID();
-priv->job.started = now;
+priv->job.current->started = now;
priv->job.current->jobType = VIR_DOMAIN_JOB_UNBOUNDED;
return 0;
@@ -171,18 +171,18 @@ libxlDomainJobUpdateTime(struct libxlDomainJobObj *job)
virDomainJobData *jobData = job->current;
unsigned long long now;
-if (!job->started)
+if (!jobData->started)
return 0;
if (virTimeMillisNow() < 0)
return -1;
-if (now < job->started) {
-job->started = 0;
+if (now < jobData->started) {
+jobData->started = 0;
return 0;
}
-jobData->timeElapsed = now - job->started;
+jobData->timeElapsed = now - jobData->started;
return 0;
}
diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
index 475e4a6933..157f480b93 100644
--- a/src/libxl/libxl_domain.h
+++ b/src/libxl/libxl_domain.h
@@ -46,7 +46,6 @@ struct libxlDomainJobObj {
virCond cond; /* Use to coordinate jobs */
enum libxlDomainJob active; /* Currently running job */
int owner; /* Thread which set current job */
-unsigned long long started; /* When the job started */
virDomainJobData *current;/* Statistics for the current job */
};
--
2.35.1
