Re: [PATCH v3 0/5] Add support for two i386 pm options which control acpi hotplug
On Sat, Sep 25, 2021 at 8:39 PM Ani Sinha wrote: > > Ok then let me do this. I will split up this patch set and send out a > separate patch set just for acpi-hotplug-root with the conf change as > suggested by danpb. It makes sense for me too to go down the path of his > suggestion. > > Meanwhile let's figure out what we wanted to do for acpi-hotplug-bridge. That > could be posted as a separate patch set. > > Sounds good? OK, while we have split this up and are making progress with the pci-root-hotplug patchset, have we decided where to put the conf for pci-hotplug-bridge? Should we leave it as it is under ? > > On Sat, Sep 25, 2021 at 8:29 PM Laine Stump wrote: >> >> On 9/23/21 4:46 PM, Laine Stump wrote: >> > On 9/11/21 11:26 PM, Ani Sinha wrote: >> >> Hi all: >> >> >> >> This patchset introduces libvirt xml support for the following two pm >> >> conf >> >> options: >> >> >> >> >> >> >> >> >> >> >> > >> > (before I get into a more radical discussion about different options - >> > since we aren't exactly duplicating the QEMU option name anyway, what if >> > we made these names more consistent, e.g. "acpi-hotplug-bridge" and >> > "acpi-hotplug-root"?) >> > >> > I've thought quite a bit about whether to put these attributes here, or >> > somewhere else, and I'm still undecided. >> > >> > My initial reaction to this was "PM == Power Management, and power >> > management is all about suspend mode support. Hotplug isn't power >> > management." But then you look at the name of the QEMU option and PM is >> > right there in the name, and I guess it's *kind of related* (effectively >> > suspending/resuming a single device), so maybe I'm thinking too narrowly. >> > >> > So are there alternate places that might fit the purpose of these new >> > options better, rather than directly mimicking the QEMU option placement >> > (for better or worse)? A couple alternative possibilities: >> > >> > 1) >> > >> > One possibility would be to include these new flags within the existing >> > subelement of , which is already used to control >> > whether the guest exposes ACPI to the guest *at all* (via adding >> > "-no-acpi" to the QEMU commandline when is missing - NB: this >> > feature flag is currently supported only on x86 and aarch64 QEMU >> > platforms, and ignored for all other hypervisors). >> > >> > Possibly the new flags could be put in something like this: >> > >> > >> > >> > >> > >> > >> >... >> > >> > >> > But: >> > >> > * currently there are no subelements to . So this isn't "extending >> > according to an existing pattern". >> > >> > * even though the element uses presence of a subelement to >> > indicate "enabled" and absence of the subelement to indicate "disabled". >> > But in the case of these new acpi bridge options we would need to >> > explicitly have the "enabled='yes/no'" rather than just using presence >> > of the option to mean "enabled" and absence to mean "disabled" because >> > the default for "root-hotplug" up until now has been *enabled*, and the >> > default for hotplug-bridge is different depending on machinetype. We >> > need to continue working properly (and identically) with old/existing >> > XML, but if we didn't have an "enabled" attribute for these new flags, >> > there would be no way to tell the difference between "not specified" and >> > "disabled", and so no way to disable the feature for a QEMU where the >> > default was "enabled". (Why does this matter? Because I don't like the >> > inconsistency that would arise from some feature flags using absense to >> > mean "disabled" and some using it to mean "use the default".) >> > >> > * Having something in in the domain XML kind of implies that >> > the associated capability flags should be represented in the >> > section of the domain capabilities. For example, is listed under >> > in the output of virsh capabilities, separately from the flag >> > indicating presence of the -no-acpi option. I'm not sure if we would >> > need to add something there for these options if we moved them into >> > (seems a bit redundant to me to have it in both places, but >> > I'm sure there are $reasons). >> > >> > >> > 2) * >> > >> > Alternately, there is an subelement of , which is currently >> > used to add a SLIC table (some sort of software license table, which I'd >> > never heard of before) using QEMU's -acpitable commandline option. It is >> > also used somehow by the Xen driver. >> > >> > >> > >> > /path/to/slic.dat >> > >> > >> > >> >... >> > >> > >> > My problem with adding these new PCI controller acpi options to os/acpi >> > is simply that it's in the subelement, which is claimed elsewhere >> > to be intended for OS boot options, and is used for things like >> > specifying the path to a kernel / initrd to boot from. >> > >> > 3) >> > >> > A third option, suggested somewhere by Ani, would be to make a >> > completely new top-level element, called
[PATCH] conf: fix block type CDROM cannot support startupPolicy
block type CDROM also support startupPolicy in the past, so
let us fix it.
Signed-off-by: Jie Wang
---
src/conf/domain_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f7025bffe4..dd374e8ab3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30853,9 +30853,10 @@ virDomainDiskTranslateSourcePool(virDomainDiskDef *def)
}
if (def->startupPolicy != 0 &&
-virStorageSourceGetActualType(def->src) != VIR_STORAGE_TYPE_FILE) {
+(virStorageSourceGetActualType(def->src) != VIR_STORAGE_TYPE_FILE &&
+ virStorageSourceGetActualType(def->src) != VIR_STORAGE_TYPE_BLOCK)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
- _("'startupPolicy' is only valid for 'file' type
volume"));
+ _("'startupPolicy' is only valid for 'file' or 'block'
type volume"));
return -1;
}
--
2.24.0.windows.2
[PATCH v6 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
The following change in qemu added support for a global boolean flag specific
to i440fx machines that would turn off or on acpi based hotplug for pci root
bus:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the
root bus")
The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu
commandline. It is enabled by default. This patch adds the corresponding qemu
capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.
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
Reviewed-by: Laine Stump
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.1.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 db5432c9fc..71aca20c4c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
"set-action", /* QEMU_CAPS_SET_ACTION */
"virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
+
+ /* 410 */
+ "piix4-acpi-root-hotplug-en", /*
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
);
@@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsIDEDrive[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = {
{ "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
+{ "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 097f28bd40..c2d1e352bd 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
+/* 410 */
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc
PIIX4_PM.acpi-root-pci-hotplug */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index e09880e937..ffd0e66d00 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -233,6 +233,7 @@
+
5002000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index 571336c1fa..658a1e742f 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -241,6 +241,7 @@
+
600
0
43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 74b87847d0..5bb21fec47 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6001000
0
43100243
--
2.25.1
[PATCH v6 4/4] NEWS: release note the new hotplug enable/disable option on pci-root controller
A new 'target' subelement of the pci-root controller has been introduced having a 'hotplug' property. This proprty can be used to turn off or turn on hotplug capability of the devices plugged into the pci-root ports. This change release notes this feature for the next release. The new element can be used like this: This will turn off hotplug capability on the pci-root ports. To turn the capability on, we set hotplug='on' above (which is also the default). Signed-off-by: Ani Sinha --- NEWS.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index fd20e50d18..b244cbb2be 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -29,6 +29,12 @@ v7.8.0 (unreleased) active. This information can also be retrieved with the new virsh command ``nodedev-info``. + * qemu: support disabling hotplug of devices on the pci-root controller + +libvirt can now set the "hotplug" option for pci-root controller which can +be used to disable hotplug/unplug of devices from the pci root port. The +default behavior is to keep hotplug on pci-root ports enabled. + * **Improvements** * **Bug fixes** -- 2.25.1
[PATCH v6 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
This change adds qemu backend command line support for enabling or disabling
hotplug on the pci-root controller using the 'target' sub-element of the
pci-root controller as shown below:
'' is only valid for pc (x86) machines and turns on
the following command line option that is passed to qemu for x86 guests:
-global PIIX4_PM.acpi-root-pci-hotplug=
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.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 17 ++
.../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++
.../pc-i440fx-acpi-root-hotplug-disable.err | 1 +
tests/qemuxml2argvtest.c | 3 ++
4 files changed, 52 insertions(+)
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b60ee1192b..a8775bca24 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2645,6 +2645,20 @@ qemuBuildSkipController(const virDomainControllerDef
*controller,
return false;
}
+static int
+qemuBuildPMPCIRootHotplugCommandLine(virCommand *cmd,
+ const virDomainControllerDef *controller)
+{
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
+controller->idx == 0 &&
+controller->opts.pciopts.hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd, "PIIX4_PM.acpi-root-pci-hotplug=%s",
+
virTristateSwitchTypeToString(controller->opts.pciopts.hotplug));
+}
+return 0;
+}
static int
qemuBuildControllersByTypeCommandLine(virCommand *cmd,
@@ -2661,6 +2675,9 @@ qemuBuildControllersByTypeCommandLine(virCommand *cmd,
if (cont->type != type)
continue;
+if (qemuBuildPMPCIRootHotplugCommandLine(cmd, cont))
+continue;
+
if (qemuBuildSkipController(cont, def))
continue;
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
new file mode 100644
index 00..dd8ea503fc
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.args
@@ -0,0 +1,31 @@
+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 \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object
secret,id=masterKey0,format=raw,file=/tmp/lib/domain--1-i440fx/master-key.aes \
+-machine pc-i440fx-2.5,accel=tcg,usb=off,dump-guest-core=off \
+-m 1024 \
+-realtime mlock=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,path=/tmp/lib/domain--1-i440fx/monitor.sock,server=on,wait=off
\
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-boot strict=on \
+-global PIIX4_PM.acpi-root-pci-hotplug=off \
+-usb \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
new file mode 100644
index 00..b507f1f8bc
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.err
@@ -0,0 +1 @@
+unsupported configuration: setting the hotplug property on a 'pci-root' device
is not supported by this QEMU binary
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 13e387df3f..d5ddd60182 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2571,6 +2571,9 @@ mymain(void)
QEMU_CAPS_DEVICE_IOH3420,
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, QEMU_CAPS_ICH9_AHCI,
QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4);
+DO_TEST("pc-i440fx-acpi-root-hotplug-disable",
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-disable");
DO_TEST("q35-usb2",
QEMU_CAPS_DEVICE_PCI_BRIDGE,
QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
--
2.25.1
[PATCH v6 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
This change introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:
'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu/kvm accelerator only.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the
root bus")
The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses.
Related unit tests to exercise the new conf option has also been added.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 12
src/qemu/qemu_validate.c | 9 +-
.../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++
.../pc-i440fx-acpi-root-hotplug-enable.xml| 30 +++
.../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
.../pc-i440fx-acpi-root-hotplug-enable.xml| 1 +
tests/qemuxml2xmltest.c | 4 +++
7 files changed, 81 insertions(+), 6 deletions(-)
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 0f5d833521..e2a1768ba7 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3776,11 +3776,13 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).`
controller's "port" configuration value, which is visible to the virtual
machine. If set, port must be between 0 and 255.
``hotplug``
- pcie-root-port and pcie-switch-downstream-port controllers can also have a
- ``hotplug`` attribute in the subelement, which is used to
- disable hotplug/unplug of devices on a particular controller. The default
- setting of ``hotplug`` is ``on``; it should be set to ``off`` to disable
- hotplug/unplug of devices on a particular controller. :since:`Since 6.3.0`
+ pci-root (:since:`Since 7.8.0`), pcie-root-port (:since:`Since 6.3.0`) and
+ pcie-switch-downstream-port controllers (:since:`Since 6.3.0`) can
+ also have a ``hotplug`` attribute in the subelement, which is
+ used to disable hotplug/unplug of devices on a particular controller. The
+ default setting of ``hotplug`` is ``on``; it should be set to ``off`` to
+ disable hotplug/unplug of devices on a particular controller.
+
``busNr``
pci-expander-bus and pcie-expander-bus controllers can have an optional
``busNr`` attribute (1-254). This will be the bus number of the new bus; All
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 13fbfd01b2..203b2b231a 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
/* hotplug */
if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("setting the %s property on a '%s' device is
not supported by this QEMU binary"),
+ "hotplug", "pci-root");
+return -1;
+}
+break;
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
@@ -3889,7 +3897,6 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
}
break;
-case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
diff --git a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
new file mode 100644
index 00..93f2779f68
--- /dev/null
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
@@ -0,0 +1,30 @@
+
+ i440fx
+ 56f5055c-1b8d-490c-844a-ad646a1c
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+
[PATCH v6 0/4] Add support to enable/disable hotplug on pci-root controller
changelog:
v6: incorporated Jan's suggestions.
v5: incorporated suggestions from Laine for patches #2 and #3. The qemu command
line
is now added in a new function and called from
qemuBuildControllersByTypeCommandLine().
Output xmls are now symlinked to input xmls for unit tests.
v4: split the original patchset into a pci-root controller specific patch
series.
also the libvirt conf is now a sub-element of the pci-root controller as was
suggested by Dan Berrange. Please see discussion here:
https://listman.redhat.com/archives/libvir-list/2021-September/msg00839.html
v3: reorganized the patches as per Laine's suggestion. Added more
details in commit messages. Added conf description in formatdomain.rst.
Added changelog for next release.
v2: fixed bugs and added additional missing unit tests.
v1: initial implementation. Had some bugs and missed some unit tests
This patchset introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:
'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
introduced from qemu version 5.2 with the following change in qemu repository:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the
root bus")
The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses [1].
The corresponding commandline option to qemu for x86 guests is:
-global PIIX4_PM.acpi-root-pci-hotplug=
Notes:
1. The use case scenario described by Laine in
https://listman.redhat.com/archives/libvir-list/2020-February/msg00110.html
intentionally does not discuss i440fx and focusses solely on q35. I do realize
that redhat has moved on from i440fx and currently efforts for new features
are concentrated on q35 machines only. We have had some hard debates on this
on the qemu mailing list before. The fact of the matter is that i440fx is
not at 1-1 parity with q35. There are many users who are currenly using i440fx
and are simply not ready to move to q35 without sacrificing some
existing features they support today. For example
https://wiki.qemu.org/images/4/4e/Q35.pdf lists some of q35 limitations.
https://www.linux-kvm.org/images/0/06/2012-forum-Q35.pdf provides more
information on the differences. Hence we need to solve the issue Laine has
described in the above email for i440fx without adding additional bridges.
Further, in Daniel Berrange's words from :
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg03012.html
"From the upstream POV, there's been no decision / agreement to phase
out PIIX, this is purely a RHEL downstream decision & plan. If other
distros / users have a different POV, and find the feature useful, we
should accept the patch if it meets the normal QEMU patch requirements.
"
Also to be noted that I have already experimented this qemu commandline option
using libvirt passthrough feature as has been documented in
http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
This was only meant to be a short term solution until libvirt started
supporting this natively. Supporting this option through libvirt would simplify
their use case as well as add capability validations
and graceful failure scenarios in case qemu did not support the option.
Ani Sinha (4):
qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx
machines
conf: introduce option to enable/disable pci hotplug on pci-root
controller
qemu: command: add support to enable/disable hotplug on pci-root
controller
NEWS: release note the new hotplug enable/disable option on pci-root
controller
NEWS.rst | 6
docs/formatdomain.rst | 12 ---
src/qemu/qemu_capabilities.c | 4 +++
src/qemu/qemu_capabilities.h | 3 ++
src/qemu/qemu_command.c | 17 ++
src/qemu/qemu_validate.c | 9 +-
.../caps_5.2.0.x86_64.xml | 1 +
.../caps_6.0.0.x86_64.xml | 1 +
.../caps_6.1.0.x86_64.xml | 1 +
.../pc-i440fx-acpi-root-hotplug-disable.args | 31 +++
.../pc-i440fx-acpi-root-hotplug-disable.err | 1 +
.../pc-i440fx-acpi-root-hotplug-disable.xml | 30 ++
.../pc-i440fx-acpi-root-hotplug-enable.xml| 30 ++
tests/qemuxml2argvtest.c | 3 ++
.../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
.../pc-i440fx-acpi-root-hotplug-enable.xml| 1 +
RE: [libvirt][PATCH v7 0/5] Support query and use SGX
Really appreciate if anyone can give comments/opinions on this patch. Thank you in advance. > -Original Message- > From: Huang, Haibin > Sent: Wednesday, September 8, 2021 9:16 AM > To: libvir-list@redhat.com; Huang, Haibin ; Ding, > Jian-feng ; Yang, Lin A ; Lu, > Lianhao ; pbonz...@redhat.com; > pkre...@redhat.com; twied...@redhat.com; phrd...@redhat.com; > berra...@redhat.com; mpriv...@redhat.com > Subject: [libvirt][PATCH v7 0/5] Support query and use SGX > > This patch series provides support for enabling Intel's Software Guard > Extensions (SGX) feature in guest VM. > Giving the SGX support in QEMU be accepted and will be merged in two days > Intel Software Guard Extensions (Intel® SGX) is a set of instructions that > increases the security of application code and data, giving them more > protection from disclosure or modification. Developers can partition sensitive > information into enclaves, which are areas of execution in memory with more > security protection. > > The typical flow looks below at very high level: > > 1. Calls virConnectGetDomainCapabilities API to domain capabilities that > includes the following SGX information. > > > ... > > N > > > > 2. User requests to start a guest calling virCreateXML() with SGX requirement. > It should contain > > > ... > > > N > > > ... > > > Haibin Huang (2): > Support to query SGX capability > Add get domaincaps unit test > > Lin Yang (3): > conf: Introduce SGX EPC element into device memory xml > qemu: Add command-line to generate SGX EPC memory backend > Add unit tests for guest VM creation command with SGX EPC > > docs/schemas/domaincaps.rng | 19 ++- > docs/schemas/domaincommon.rng | 1 + > src/conf/domain_capabilities.c| 29 > src/conf/domain_capabilities.h| 13 ++ > src/conf/domain_conf.c| 5 + > src/conf/domain_conf.h| 1 + > src/conf/domain_validate.c| 1 + > src/libvirt_private.syms | 2 +- > src/qemu/qemu_alias.c | 6 +- > src/qemu/qemu_capabilities.c | 142 ++ > src/qemu/qemu_capabilities.h | 4 + > src/qemu/qemu_command.c | 39 - > src/qemu/qemu_domain.c| 10 +- > src/qemu/qemu_domain_address.c| 4 + > src/qemu/qemu_monitor.c | 10 ++ > src/qemu/qemu_monitor.h | 3 + > src/qemu/qemu_monitor_json.c | 89 +++ > src/qemu/qemu_monitor_json.h | 3 + > src/qemu/qemu_process.c | 2 + > src/qemu/qemu_validate.c | 8 + > src/security/security_apparmor.c | 1 + > src/security/security_dac.c | 2 + > src/security/security_selinux.c | 2 + > tests/domaincapsdata/bhyve_basic.x86_64.xml | 1 + > tests/domaincapsdata/bhyve_fbuf.x86_64.xml| 1 + > tests/domaincapsdata/bhyve_uefi.x86_64.xml| 1 + > tests/domaincapsdata/empty.xml| 1 + > tests/domaincapsdata/libxl-xenfv.xml | 1 + > tests/domaincapsdata/libxl-xenpv.xml | 1 + > .../domaincapsdata/qemu_2.11.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.11.0-tcg.x86_64.xml | 1 + > tests/domaincapsdata/qemu_2.11.0.s390x.xml| 1 + > tests/domaincapsdata/qemu_2.11.0.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.12.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.12.0-tcg.x86_64.xml | 1 + > .../qemu_2.12.0-virt.aarch64.xml | 1 + > tests/domaincapsdata/qemu_2.12.0.aarch64.xml | 1 + > tests/domaincapsdata/qemu_2.12.0.ppc64.xml| 1 + > tests/domaincapsdata/qemu_2.12.0.s390x.xml| 1 + > tests/domaincapsdata/qemu_2.12.0.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.4.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.4.0-tcg.x86_64.xml | 1 + > tests/domaincapsdata/qemu_2.4.0.x86_64.xml| 1 + > .../domaincapsdata/qemu_2.5.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.5.0-tcg.x86_64.xml | 1 + > tests/domaincapsdata/qemu_2.5.0.x86_64.xml| 1 + > .../domaincapsdata/qemu_2.6.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.6.0-tcg.x86_64.xml | 1 + > .../qemu_2.6.0-virt.aarch64.xml | 1 + > tests/domaincapsdata/qemu_2.6.0.aarch64.xml | 1 + > tests/domaincapsdata/qemu_2.6.0.ppc64.xml | 1 + > tests/domaincapsdata/qemu_2.6.0.x86_64.xml| 1 + > .../domaincapsdata/qemu_2.7.0-q35.x86_64.xml | 1 + > .../domaincapsdata/qemu_2.7.0-tcg.x86_64.xml | 1 + > tests/domaincapsdata/qemu_2.7.0.s390x.xml | 1 + > tests/domaincapsdata/qemu_2.7.0.x86_64.xml| 1 + > .../domaincapsdata/qemu_2.8.0-q35.x86_64.xml | 1 + >
[libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
Add support for deserializing the binary PCI/PCIe VPD format and storing
results in memory.
The VPD format is specified in "I.3. VPD Definitions" in PCI specs
(2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
notes, the PCI Local Bus and PCIe VPD formats are binary compatible
and PCIe 4.0 merely started incorporating what was already present in
PCI specs.
Linux kernel exposes a binary blob in the VPD format via sysfs since
v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
a parser to interpret.
A GTree is used as a data structure in order to maintain key ordering
which will be important in XML to XML tests later.
Signed-off-by: Dmitrii Shcherbakov
---
build-aux/syntax-check.mk | 4 +-
po/POTFILES.in| 1 +
src/libvirt_private.syms | 15 +
src/util/meson.build | 1 +
src/util/virpcivpd.c | 755 ++
src/util/virpcivpd.h | 117 ++
src/util/virpcivpdpriv.h | 45 +++
tests/meson.build | 1 +
tests/testutils.c | 40 ++
tests/testutils.h | 4 +
tests/virpcivpdtest.c | 705 +++
11 files changed, 1686 insertions(+), 2 deletions(-)
create mode 100644 src/util/virpcivpd.c
create mode 100644 src/util/virpcivpd.h
create mode 100644 src/util/virpcivpdpriv.h
create mode 100644 tests/virpcivpdtest.c
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index cb54c8ba36..a428831380 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -775,9 +775,9 @@ sc_prohibit_windows_special_chars_in_filename:
{ echo '$(ME): Windows special chars in filename not allowed' 1>&2;
echo exit 1; } || :
sc_prohibit_mixed_case_abbreviations:
- @prohibit='Pci|Usb|Scsi' \
+ @prohibit='Pci|Usb|Scsi|Vpd' \
in_vc_files='\.[ch]$$' \
- halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \
+ halt='Use PCI, USB, SCSI, VPD, not Pci, Usb, Scsi, Vpd' \
$(_sc_search_regexp)
# Require #include in all files that call setlocale()
diff --git a/po/POTFILES.in b/po/POTFILES.in
index c200d7452a..4be4139529 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -302,6 +302,7 @@
@SRCDIR@src/util/virnvme.c
@SRCDIR@src/util/virobject.c
@SRCDIR@src/util/virpci.c
+@SRCDIR@src/util/virpcivpd.c
@SRCDIR@src/util/virperf.c
@SRCDIR@src/util/virpidfile.c
@SRCDIR@src/util/virpolkit.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6de9d9aef1..bb9b380599 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3571,6 +3571,21 @@ virVHBAManageVport;
virVHBAPathExists;
+# util/virpcivpd.h
+
+virPCIVPDKeywordResourceForEach;
+virPCIVPDKeywordResourceNew;
+virPCIVPDParse;
+virPCIVPDParseVPDLargeResourceFields;
+virPCIVPDParseVPDLargeResourceString;
+virPCIVPDReadVPDBytes;
+virPCIVPDResourceGetFieldValueFormat;
+virPCIVPDResourceGetResourceType;
+virPCIVPDResourceIsExpectedKeyword;
+virPCIVPDResourceIsValidTextValue;
+virPCIVPDStringResourceGetValue;
+virPCIVPDStringResourceNew;
+
# util/virvsock.h
virVsockAcquireGuestCid;
virVsockSetGuestCid;
diff --git a/src/util/meson.build b/src/util/meson.build
index 05934f6841..24350a3e67 100644
--- a/src/util/meson.build
+++ b/src/util/meson.build
@@ -105,6 +105,7 @@ util_sources = [
'virutil.c',
'viruuid.c',
'virvhba.c',
+ 'virpcivpd.c',
'virvsock.c',
'virxml.c',
]
diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
new file mode 100644
index 00..849ea0570c
--- /dev/null
+++ b/src/util/virpcivpd.c
@@ -0,0 +1,755 @@
+/*
+ * virpcivpd.c: helper APIs for working with the PCI/PCIe VPD capability
+ *
+ * Copyright (C) 2021 Canonical Ltd.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ *
[libvirt PATCH v5 5/7] Add PCI VPD Capability Support
* XML serialization and deserialization of PCI VPD resources;
* PCI VPD capability flags added and used in relevant places;
* XML to XML tests for the added capability.
Signed-off-by: Dmitrii Shcherbakov
---
docs/schemas/nodedev.rng | 40 +++
include/libvirt/libvirt-nodedev.h | 1 +
src/conf/node_device_conf.c | 271 ++
src/conf/node_device_conf.h | 6 +-
src/conf/virnodedeviceobj.c | 7 +-
src/node_device/node_device_driver.c | 2 +
src/node_device/node_device_udev.c| 2 +
.../pci__42_00_0_vpd.xml | 33 +++
.../pci__42_00_0_vpd.xml | 1 +
tests/nodedevxml2xmltest.c| 1 +
tools/virsh-nodedev.c | 3 +
11 files changed, 365 insertions(+), 2 deletions(-)
create mode 100644 tests/nodedevschemadata/pci__42_00_0_vpd.xml
create mode 12 tests/nodedevxml2xmlout/pci__42_00_0_vpd.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index e089e66858..fbbee4d0c6 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -223,6 +223,10 @@
+
+
+
+
@@ -770,6 +774,42 @@
+
+
+
+vpd
+
+
+
+
+
+
+string
+
+
+
+
+
+
+
+vpd-r
+vpd-w
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/include/libvirt/libvirt-nodedev.h
b/include/libvirt/libvirt-nodedev.h
index e492634217..245365b07f 100644
--- a/include/libvirt/libvirt-nodedev.h
+++ b/include/libvirt/libvirt-nodedev.h
@@ -84,6 +84,7 @@ typedef enum {
VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_CARD = 1 << 18, /* s390 AP Card
device */
VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_QUEUE = 1 << 19, /* s390 AP
Queue */
VIR_CONNECT_LIST_NODE_DEVICES_CAP_AP_MATRIX = 1 << 20, /* s390 AP
Matrix */
+VIR_CONNECT_LIST_NODE_DEVICES_CAP_VPD = 1 << 21, /* Device with
VPD */
/* filter the devices by active state */
VIR_CONNECT_LIST_NODE_DEVICES_INACTIVE = 1 << 30, /* Inactive
devices */
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 9bbff97ffd..7d9f74f45b 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -36,6 +36,7 @@
#include "virrandom.h"
#include "virlog.h"
#include "virfcp.h"
+#include "virpcivpd.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -70,6 +71,7 @@ VIR_ENUM_IMPL(virNodeDevCap,
"ap_card",
"ap_queue",
"ap_matrix",
+ "vpd",
);
VIR_ENUM_IMPL(virNodeDevNetCap,
@@ -240,6 +242,90 @@ virNodeDeviceCapMdevTypesFormat(virBuffer *buf,
}
}
+static void
+virNodeDeviceCapVPDStringResourceFormat(virBuffer *buf, virPCIVPDResource *res)
+{
+virBufferAdjustIndent(buf, 2);
+virBufferEscapeString(buf, "%s",
virPCIVPDStringResourceGetValue((virPCIVPDStringResource *)res));
+virBufferAdjustIndent(buf, -2);
+}
+
+static gboolean
+virNodeDeviceCapVPDWriteTextField(gpointer key, gpointer val, gpointer
userData)
+{
+const gchar *k = (const gchar*)key, *v = (const gchar*)val;
+virBuffer *buf = userData;
+virPCIVPDResourceFieldValueFormat format =
VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST;
+
+format = virPCIVPDResourceGetFieldValueFormat(k);
+
+if (format == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT) {
+virBufferEscapeString(buf, "", k);
+virBufferEscapeString(buf, "%s", v);
+virBufferEscapeString(buf, "\n", k);
+}
+return false;
+}
+
+static void
+virNodeDeviceCapVPDKeywordResourceFormat(virBuffer *buf, virPCIVPDResource
*res)
+{
+virPCIVPDKeywordResource *keywordRes = NULL;
+g_autofree GHashTableIter *iter = NULL;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+keywordRes = (virPCIVPDKeywordResource*)res;
+
+virPCIVPDKeywordResourceForEach(keywordRes,
virNodeDeviceCapVPDWriteTextField, buf);
+virBufferAdjustIndent(buf, -2);
+}
+
+static void
+virNodeDeviceCapVPDResourceFormat(virBuffer *buf, virPCIVPDResource *res)
+{
+GEnumValue *resRype = NULL;
+void (*resFormatFunc)(virBuffer *buf, virPCIVPDResource *res);
+
+if (G_TYPE_CHECK_INSTANCE_TYPE(res, VIR_TYPE_PCI_VPD_STRING_RESOURCE)) {
+resFormatFunc = virNodeDeviceCapVPDStringResourceFormat;
+} else if (G_TYPE_CHECK_INSTANCE_TYPE(res,
VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE)) {
+resFormatFunc = virNodeDeviceCapVPDKeywordResourceFormat;
+} else {
+/* Unexpected resource type. This should not
[libvirt PATCH v5 7/7] Add PCI VPD Capability Documentation
Describes the format of the newly added VPD capability and gives and example for a real-world device. Signed-off-by: Dmitrii Shcherbakov --- docs/drvnodedev.html.in | 46 + docs/formatnode.html.in | 24 - 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/docs/drvnodedev.html.in b/docs/drvnodedev.html.in index 70f7e6717d..438d20f2dd 100644 --- a/docs/drvnodedev.html.in +++ b/docs/drvnodedev.html.in @@ -185,6 +185,52 @@ /capability /device +VPD capability + + A device that exposes a PCI/PCIe VPD capability will include a nested + capability vpd which presents all resources (string, read-only + and read-write keyword resources) that may be contained in a valid VPD + entry. The XML format follows the binary format described in + "I.3. VPD Definitions" in PCI Local Bus (2.2+) and the identical format + in PCIe 4.0+. At the time of writing, the support for exposing this capability + is only present on Linux-based systems (kernel version v2.6.26 is the first one + to expose VPD via sysfs which Libvirt relies on). Reading the VPD contents requires + root privileges, therefore, virsh nodedev-dumpxml must be executed + accordingly. + A description of the XML format for the vpd capability can + be found here. + + + The following example shows a VPD representation for a device that exposes the + VPD capability with a string resource and a VPD-R resource. Among other things, + the VPD of this particular device includes a unique board serial number in the + "SN" field. + + +device + !-- ... -- + driver +namemlx5_core/name + /driver + capability type=pci +!-- ... -- +capability type=vpd + resource type=stringBlueField-2 DPU 25GbE Dual-Port SFP56, 16GB on-board DDR, 1GbE OOB management, Tall Bracket/resource + resource type=vpd-r +field keyword=SNMT2113X0/field +field keyword=V3d644e35a61d0eb11e000e8cef671bf3e/field +field keyword=VAMLX:MN=MLNX:CSKU=V2:UUID=V3:PCI=V0:MODL=BF2H332A/field +field keyword=PNMBF2H332A-AEEOT/field +field keyword=V2MBF2H332A-AEEOT/field +field keyword=ECB1/field +field keyword=V0PCIeGen4 x8/field + /resource +/capability + !-- ... -- + /capability +/device + + Mediated devices (MDEVs) Mediated devices (Since 3.2.0) are software diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 3b3c3105d4..3e8c771398 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -162,7 +162,13 @@ This device is capable of creating mediated devices. The sub-elements are summarized in mdev_types capability. - + + vpd + +This device exposes a VPD PCI/PCIe capability. +The sub-elements are summarized in +vpd capability. + @@ -592,5 +598,21 @@ /device +vpd capability + + + PCI devices can expose a VPD capability which + is optional per PCI Local Bus 2.2+ and PCIe 4.0+ specifications. If + the VPD capability is present, then the parent capability + element with the vpd type will contain a list of + resource elements with contents applicable to a + particular resource type (string or keyword resources). The + resource element will either contain a string + resource value or a list of field elements in + the case of keyword resources. field elements + contain a keyword attribute corresponding to the + 2-byte VPD keyword and text as element data representing the + field value. + -- 2.30.2
[libvirt PATCH v5 6/7] News: Add PCI VPD capability support
Signed-off-by: Dmitrii Shcherbakov --- NEWS.rst | 7 +++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 4617198f82..4db4e0e3e3 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -44,6 +44,13 @@ v7.8.0 (unreleased) to invoke the PCI VPD parser to get a list of resources representing PCI VPD contents in memory. + * nodedev: Add PCI VPD capability support + +Support for serializing and deserializing PCI VPD data structures is added +following the addition of the PCI VPD parser. A new PCI device capability +called "vpd" is introduced holding string resources and keyword resources +found in PCI VPD. + * **Improvements** * **Bug fixes** -- 2.30.2
[libvirt PATCH v5 0/7] Add a PCI/PCIe device VPD Capability
Add support for deserializing the binary PCI/PCIe VPD format and exposing VPD resources as XML elements in a new nested capability of PCI/PCIe devices called 'vpd'. The series contains the following incremental changes: * The PCI VPD parser module, in-memory resource representation and tests; * VPD-related helpers added to the virpci module; * VPD capability support: XML serialization/deserialization from/into VPD resource data structures; * Documentation. The VPD format is specified in "I.3. VPD Definitions" in PCI specs (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0 notes, the PCI Local Bus and PCIe VPD formats are binary compatible and PCIe 4.0 merely started incorporating what was already present in PCI specs. Linux kernel exposes a binary blob in the VPD format via sysfs since v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires a parser to interpret. There are usage scenarios where information such as the board serial number needs to be retrieved from PCI(e) VPD. Projects like Nova can utilize this information for cases which involve virtual interface plugging on SmartNIC DPUs but there may be other scenarios and types of information useful to retrieve from VPD. The fact that the format is binary requires proper parsing instead of substring searching hence the full parser is proposed. Likewise, checksum validation requires proper parsing as well. A usage example is present here: https://review.opendev.org/c/openstack/nova/+/808199 The patch follows a prior discussion on the mailing list which has additional context about the use-case but a narrower proposal: https://listman.redhat.com/archives/libvir-list/2021-May/msg00873.html https://www.mail-archive.com/libvir-list@redhat.com/msg218165.html The new functionality is mostly contained in virpcivpd with a couple of new functions added to virpci. Additionally, the necessary XML serialization/deserialization and glue code is added to expose the VPD capability to external clients as XML. A new capability flag is added along with a new capability in order to allow for filtering of PCI devices with the VPD capability using virsh: virsh nodedev-list --cap vpd sudo virsh nodedev-dumpxml --device pci__bb_ss_f In this example having the root uid is required in order to access the vpd sysfs entry, therefore, the nodedev XML output will only contain the VPD capability if virsh is run as root. The capability is treated as dynamic due to the presence of read-write sections in the VPD format per PCI/PCIe specs (the idea being that read-write resource fields may potentially be altered by the DPU OS over time independently from the host OS). Unit tests cover the parser functionality (including many possible invalid cases), in-memory representation as well as XML serialization and deserialization. Manual functional testing was performed with 2 DPUs and several other NIC models which expose PCI(e) VPD. Testing have also been performed for devices that do not have VPD or those that expose a VPD capability but exhibit invalid behavior (I/O errors while reading a sysfs entry). Per the existing guidelines, the implementation relies heavily on glib for various purposes. https://libvirt.org/glib-adoption.html * v5 fixes a couple of minor typos and adds NEWS entries. * The v4 of the patch includes a number of fixes compared to v3: * Fixed the patch to correctly build against older glib (2.56.0); * Notably, glib commit 86c073dba9d82ef3f1bc3d3116b058b9b5c3b1eb (in 2.59.0) fixes g_autolist support for derivable Glib types. To make things work in 2.56.0 a workaround is conditionally applied; * virCreateAnonymousFile now uses a temporary file which is unlinked after creation instead of memfd because OpenSUSE 15.2 does not have support memfd; * Keyword resources now use GTree instead of GHashTable as the underlying data structure: * This allows for stable ordering which is important for XML2XML tests as they were failing with when GLib versions were different, resulting in a different ordering of elements; * The keyword resource iteration function was complex and got replaced by a simpler g_tree_foreach-based approach; * Added more testing: functions added to virpci are now assessed by creating a mocked vpd file under a mocked sysfs structure while the parser is still tested in virpcivpdtest file; * Refactoring: * Applied changes based on the indent tool operation with some post-processing; * Renamed functions which had the Glib naming style to use camel case where possible. Auto-generated declarations are an exception: gobject/gtype.h defines type_name##_init, type_name##_class_init, module_obj_name##_get_type functions which were left unchanged; * camelCase is now used for local variables and function parameters; *
[libvirt PATCH v5 4/7] News: Add PCI VPD-related helper functions to virpci
Signed-off-by: Dmitrii Shcherbakov --- NEWS.rst | 7 +++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index a4178e505c..4617198f82 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -37,6 +37,13 @@ v7.8.0 (unreleased) functionality got added for Linux only at this point (kernels above v2.6.26 have support for exposing VPD via sysfs). + * virpci: Add PCI VPD-related helper functions to virpci + +In order to utilize the PCI VPD parser, a couple of helper functions got +introduced to check for the presence of a VPD file in the sysfs tree and +to invoke the PCI VPD parser to get a list of resources representing PCI +VPD contents in memory. + * **Improvements** * **Bug fixes** -- 2.30.2
[libvirt PATCH v5 3/7] Add PCI VPD-related helper functions to virpci
Add helper functions to virpci to provide means of checking for a VPD
file presence and for VPD resource retrieval using the PCI VPD parser.
The added test assesses the basic functionality of VPD retrieval while
the full parser is tested by virpcivpdtest.
Signed-off-by: Dmitrii Shcherbakov
---
src/libvirt_private.syms | 2 ++
src/util/virpci.c| 62 ++
src/util/virpci.h| 3 ++
tests/virpcimock.c | 30 +++
tests/virpcitest.c | 64
5 files changed, 161 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index bb9b380599..ebee46ce9b 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2988,7 +2988,9 @@ virPCIDeviceGetReprobe;
virPCIDeviceGetStubDriver;
virPCIDeviceGetUnbindFromStub;
virPCIDeviceGetUsedBy;
+virPCIDeviceGetVPDResources;
virPCIDeviceHasPCIExpressLink;
+virPCIDeviceHasVPD;
virPCIDeviceIsAssignable;
virPCIDeviceIsPCIExpress;
virPCIDeviceListAdd;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index f307580a53..480d91c17f 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -37,6 +37,7 @@
#include "virkmod.h"
#include "virstring.h"
#include "viralloc.h"
+#include "virpcivpd.h"
VIR_LOG_INIT("util.pci");
@@ -2640,6 +2641,53 @@ virPCIGetVirtualFunctionInfo(const char
*vf_sysfs_device_path,
return 0;
}
+
+gboolean
+virPCIDeviceHasVPD(virPCIDevice *dev)
+{
+g_autofree char *vpdPath = NULL;
+vpdPath = virPCIFile(dev->name, "vpd");
+if (!virFileExists(vpdPath)) {
+VIR_INFO("Device VPD file does not exist %s", vpdPath);
+return false;
+} else if (!virFileIsRegular(vpdPath)) {
+VIR_WARN("VPD path does not point to a regular file %s", vpdPath);
+return false;
+}
+return true;
+}
+
+/**
+ * virPCIDeviceGetVPDResources:
+ * @dev: a PCI device to get a list of VPD resources for.
+ *
+ * Obtain resources stored in the PCI device's Vital Product Data (VPD).
+ * VPD is optional in both PCI Local Bus and PCIe specifications so there is
+ * no guarantee it will be there for a particular device.
+ *
+ * Returns: a pointer to a list of VPDResource types which needs to be freed
by the caller or
+ * NULL if getting it failed for some reason.
+ */
+GList *
+virPCIDeviceGetVPDResources(virPCIDevice *dev)
+{
+g_autofree char *vpdPath = NULL;
+int fd;
+
+vpdPath = virPCIFile(dev->name, "vpd");
+if (!virPCIDeviceHasVPD(dev)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s does not have a
VPD"),
+virPCIDeviceGetName(dev));
+return NULL;
+}
+if ((fd = open(vpdPath, O_RDONLY)) < 0) {
+virReportSystemError(-fd,
+ _("Failed to open a VPD file '%s'"), vpdPath);
+return NULL;
+}
+return virPCIVPDParse(fd);
+}
+
#else
static const char *unsupported = N_("not supported on non-linux platforms");
@@ -2713,6 +2761,20 @@ virPCIGetVirtualFunctionInfo(const char
*vf_sysfs_device_path G_GNUC_UNUSED,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
return -1;
}
+
+gboolean
+virPCIDeviceHasVPD(virPCIDevice *dev)
+{
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
+return NULL;
+}
+
+GList *
+virPCIDeviceGetVPDResources(virPCIDevice *dev)
+{
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
+return NULL;
+}
#endif /* __linux__ */
int
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 9a3db6c6d8..a89561496b 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -269,6 +269,9 @@ int virPCIGetVirtualFunctionInfo(const char
*vf_sysfs_device_path,
char **pfname,
int *vf_index);
+gboolean virPCIDeviceHasVPD(virPCIDevice *dev);
+GList * virPCIDeviceGetVPDResources(virPCIDevice *dev);
+
int virPCIDeviceUnbind(virPCIDevice *dev);
int virPCIDeviceRebind(virPCIDevice *dev);
int virPCIDeviceGetDriverPathAndName(virPCIDevice *dev,
diff --git a/tests/virpcimock.c b/tests/virpcimock.c
index d4d43aac51..e10ebce76f 100644
--- a/tests/virpcimock.c
+++ b/tests/virpcimock.c
@@ -18,6 +18,8 @@
#include
+#include "virpcivpd.h"
+
#if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__)
# define VIR_MOCK_LOOKUP_MAIN
# include "virmock.h"
@@ -123,6 +125,13 @@ struct pciDeviceAddress {
};
# define ADDR_STR_FMT "%04x:%02x:%02x.%u"
+struct pciVPD {
+/* PCI VPD contents (binary, may contain NULLs), NULL if not present. */
+const char *data;
+/* VPD length in bytes. */
+size_t vpd_len;
+};
+
struct pciDevice {
struct pciDeviceAddress addr;
int vendor;
@@ -131,6 +140,7 @@ struct pciDevice {
int iommuGroup;
const char *physfn;
struct pciDriver *driver; /* Driver attached. NULL if attached to no
driver */
+struct pciVPD vpd;
};
struct fdCallback {
@@ -537,6 +547,10 @@
[libvirt PATCH v5 2/7] News: Add a PCI VPD parser
Signed-off-by: Dmitrii Shcherbakov
---
NEWS.rst | 8
1 file changed, 8 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst
index fd20e50d18..a4178e505c 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -29,6 +29,14 @@ v7.8.0 (unreleased)
active. This information can also be retrieved with the new virsh command
``nodedev-info``.
+ * virpcivpd: Add a PCI VPD parser
+
+A parser for the standard PCI/PCIe VPD ("I.3. VPD Definitions" in PCI 2.2+
+and an equivalent definition in "6.28.1 VPD Format" PCIe 4.0) was added
+along with relevant types to represent PCI VPD in memory. This
+functionality got added for Linux only at this point (kernels above
+v2.6.26 have support for exposing VPD via sysfs).
+
* **Improvements**
* **Bug fixes**
--
2.30.2
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 14:52 hat Peter Maydell geschrieben: > On Mon, 27 Sept 2021 at 12:27, Kevin Wolf wrote: > > > > Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben: > > > On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > > > > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > > which results in some incompatibilities, mainly around list handling. > > > > Mark the non-JSON version of both as unstable syntax so that management > > > > tools switch to JSON and we can later make the change without breaking > > > > things. > > > > > > > > Signed-off-by: Kevin Wolf > > > > > > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > > > > +'' > > > > + > > > > +If you rely on a stable interface for ``-device`` and ``-object`` that > > > > doesn't > > > > +change incompatibly between QEMU versions (e.g. because you are using > > > > the QEMU > > > > +command line as a machine interface in scripts rather than > > > > interactively), use > > > > +JSON syntax for these options instead. > > > > + > > > > +There is no intention to remove support for non-JSON syntax entirely, > > > > but > > > > +future versions may change the way to spell some options. > > > > > > As it stands, this is basically saying "pretty much anybody > > > using the command line, your stuff may break in future, instead > > > use some other interface you've never heard of, which doesn't > > > appear to be documented in the manual and which none of the > > > documentation's examples use". > > > > The documentation is a valid criticism. We need to document the JSON > > interfaces properly (which will really mostly be a pointer to the > > existing QMP documentation at least for -object, but it's important to > > tell people where to look for the details). > > > > > Is there some more limited deprecation we can do rather than "the > > > entire commandline for almost all users" ? > > > > I don't think "almost all" users is true. > > > I see three groups of users > > ...all of whom "rely on a stable interface for -device and -object", > and only two of whom it's reasonable to say "use the JSON version" to. I'm not sure that the human interactive case requires unchanged syntax as strictly as the other cases do. After each distro upgrade (or even a browser upgrade within the same distro), some UI changes somewhere and I have to adapt. I don't think anyone ever makes promises like "this button is going to stay in the exact same place forever". And our policy is already that we're not making such promises for HMP either. > > 1. Using a management tool that is probably using libvirt. This is > >likely the vast majority of users. They won't notice a difference > >because libvirt abstracts it away. libvirt developers are actively > >asking us for JSON (and QAPI) based interfaces because using the same > >representation to describe configurations in QMP and on the CLI makes > >their life easier. > > Yes, absolutely we should be recommending that libvirt and > other management interfaces use the JSON. > > > 2. People starting QEMU on the command line manually. This is > >essentially the same situation as HMP: If something changes, you get > >an error message, you look up the new syntax, done. Small > >inconvenience, but that's it. This includes simple scripts that just > >start QEMU and are only used to store a long command line somewhere. > > It's a small inconvenience that we seem to be imposing on our > users on a pretty frequent basis. Moreover, each one of these > really needs to be its own deprecation, so that users actually > can have some advance notice if they need it and look up the > new syntax. We shouldn't hide them all under this umbrella > "we might break anything at any time" entry, which I think > will pretty much encourage breaking compatibility more often > because you don't have to think about "oh, we should deprecate > this and maybe print warnings about use of deprecated syntax > and then drop it later", you can just break things and point > to this "we said -device wasn't going to be stable" entry. Are you suggesting bringing back stricter compatibility rules for HMP then? The problem with the deprecation period is that you need to have a time where both options work. But when you have two different parsers, you can't just magically have the union of their accepted syntaxes. Unless you invest a lot of time and effort, you have a flag day where the syntax changes. And I think this is perfectly reasonable for interactive use. Deprecations are the wrong tool for human interfaces. You don't need to know months in advance that something will change in the UI (you'll forget it again anyway until the change happens and the information becomes relevant), but this is a case for the changelog. If you do need to know months in advance, JSON is probably better suited for your use case.
Re: [libvirt PATCH] vshCmddefCheckInternals: Fix typo
On a Monday in 2021, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake
---
tools/vsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 189c452f73..7b77acad34 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -279,7 +279,7 @@ vshCmddefCheckInternals(vshControl *ctl,
}
if (!(alias = vshCmddefSearch(cmd->alias))) {
-vshError(ctl, _("command alias '%s' is pointing to a non-existant
command '%s'"),
+vshError(ctl, _("command alias '%s' is pointing to a non-existent
command '%s'"),
cmd->name, cmd->alias);
return -1;
}
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
[libvirt PATCH] vshCmddefCheckInternals: Fix typo
Signed-off-by: Tim Wiederhake
---
tools/vsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 189c452f73..7b77acad34 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -279,7 +279,7 @@ vshCmddefCheckInternals(vshControl *ctl,
}
if (!(alias = vshCmddefSearch(cmd->alias))) {
-vshError(ctl, _("command alias '%s' is pointing to a non-existant
command '%s'"),
+vshError(ctl, _("command alias '%s' is pointing to a non-existent
command '%s'"),
cmd->name, cmd->alias);
return -1;
}
--
2.31.1
Re: [PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On Mon, 27 Sep 2021, Laine Stump wrote:
> On 9/27/21 4:05 AM, Ján Tomko wrote:
> > On a Monday in 2021, Ani Sinha wrote:
> > > This change introduces libvirt xml support to enable/disable hotplug on
> > > the
> > > pci-root controller. It adds a 'target' subelement for the pci-root
> > > controller
> > > with a 'hotplug' property. This property can be used to enable or disable
> > > hotplug for the pci-root controller. For example, in order to disable
> > > hotplug
> > > on the pci-root controller, one has to use set '' as
> > > shown below:
> > >
> > >
> > >
> > >
> > >
> > > '' option would enable hotplug for pci-root
> > > controller.
> > > This is also the default value. This option is only available for pc
> > > machine
> > > types and is applicable for qemu only/kvm accelerator onlt.This feature
> > > was
> >
> > s/onlt./only. /
> >
> > > introduced from qemu version 5.2 with the following change in qemu
> > > repository:
> > >
> > > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on
> > > the root bus")
> > >
> > > The above qemu commit describes some reasons why users might to disable
> > > hotplug
> > > on PCI root buses.
> > >
> > > Related unit tests to exercise the new conf option has also been added.
> > >
> > > Signed-off-by: Ani Sinha
> > > ---
> > > docs/formatdomain.rst | 12
> > > src/qemu/qemu_validate.c | 9 +-
> > > .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++
> > > .../pc-i440fx-acpi-root-hotplug-enable.xml | 30 +++
> > > .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
> > > .../pc-i440fx-acpi-root-hotplug-enable.xml | 1 +
> > > tests/qemuxml2xmltest.c | 4 +++
> > > 7 files changed, 81 insertions(+), 6 deletions(-)
> > > create mode 100644
> > > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > > create mode 100644
> > > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
> > > create mode 12
> > > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > > create mode 12
> > > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
> > >
> > > @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > > virDomainControllerDef *cont,
> > > /* hotplug */
> > > if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> > > switch ((virDomainControllerModelPCI) cont->model) {
> > > + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > > + if (!virQEMUCapsGet(qemuCaps,
> > > QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
> > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > + _("setting the %s property on a pci-root
> > > device is not supported by this QEMU binary"),
> > > + "hotplug");
> >
> > No need to create a new translatable string, the one used by the case
> > below can be reused:
> >
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("setting the hotplug property on a '%s'
> > device is not supported by this QEMU binary"),
>
> Wouldn't it be better if "hotplug" was also put in the arg list rather than
> inline in the function? That would avoid some overzealous translator
> attempting to translate that word. (Or maybe just use the function
> virReportControllerInvalidOption() for both of these error cases, thus
> removing both strings)
virReportControllerInvalidOption() would be confusing/incorrect. "option
foo is not valid for PCI controller ..." etc is confusing. Better to
be more explicit and say that the option is not supported by Qemu binary
that is being used (maybe an upgrade will help?).
Re: [PATCH v4 3/4] qemu: command: add support to enable/disable hotplug on pci-root controller
On 9/27/21 12:54 AM, Ani Sinha wrote:
On Sun, 26 Sep 2021, Laine Stump wrote:
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG);
+DO_TEST_PARSE_ERROR_NOCAPS("pc-i440fx-acpi-root-hotplug-enable");
Ah, now I understand the origin of pc-i440fx-acpi-root-hotplug-enable.xml in
the previous patch - you had created it to be test case for this negative
test. Actually I think the negative test should be done using the ...-disable
test case with no caps; in the case that someone has "hotplug='on'" and the
option is missing, I think we should just ignore it, since "hotplug='on'" is
what they're going to get anyway (that could be added into the validation that
was added in the previous patch). You still you add the -enable test case to
qemuxml2xmltest.c as I suggested in the previous patch.
No. Lets make hotplug=on/off explicit either way. Qemu defaults keep
changing :-)
Yes, now that there is a settable property its default can be changed,
but we do know that on all versions of QEMU that don't support setting
this property, hotplug is always enabled for pci-root. Anyway, this
isn't important - we can always relax it later if someone really wants it.
Re: [PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On 9/27/21 4:05 AM, Ján Tomko wrote:
On a Monday in 2021, Ani Sinha wrote:
This change introduces libvirt xml support to enable/disable hotplug
on the
pci-root controller. It adds a 'target' subelement for the pci-root
controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable
hotplug
on the pci-root controller, one has to use set 'hotplug='off'>' as
shown below:
'' option would enable hotplug for pci-root
controller.
This is also the default value. This option is only available for pc
machine
types and is applicable for qemu only/kvm accelerator onlt.This
feature was
s/onlt./only. /
introduced from qemu version 5.2 with the following change in qemu
repository:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug
on the root bus")
The above qemu commit describes some reasons why users might to
disable hotplug
on PCI root buses.
Related unit tests to exercise the new conf option has also been added.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 12
src/qemu/qemu_validate.c | 9 +-
.../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++
.../pc-i440fx-acpi-root-hotplug-enable.xml | 30 +++
.../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
.../pc-i440fx-acpi-root-hotplug-enable.xml | 1 +
tests/qemuxml2xmltest.c | 4 +++
7 files changed, 81 insertions(+), 6 deletions(-)
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
/* hotplug */
if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
switch ((virDomainControllerModelPCI) cont->model) {
+ case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+ if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("setting the %s property on a
pci-root device is not supported by this QEMU binary"),
+ "hotplug");
No need to create a new translatable string, the one used by the case
below can be reused:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("setting the hotplug property on a
'%s' device is not supported by this QEMU binary"),
Wouldn't it be better if "hotplug" was also put in the arg list rather
than inline in the function? That would avoid some overzealous
translator attempting to translate that word. (Or maybe just use the
function virReportControllerInvalidOption() for both of these error
cases, thus removing both strings)
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Mon, 27 Sept 2021 at 12:27, Kevin Wolf wrote: > > Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben: > > On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > which results in some incompatibilities, mainly around list handling. > > > Mark the non-JSON version of both as unstable syntax so that management > > > tools switch to JSON and we can later make the change without breaking > > > things. > > > > > > Signed-off-by: Kevin Wolf > > > > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > > > +'' > > > + > > > +If you rely on a stable interface for ``-device`` and ``-object`` that > > > doesn't > > > +change incompatibly between QEMU versions (e.g. because you are using > > > the QEMU > > > +command line as a machine interface in scripts rather than > > > interactively), use > > > +JSON syntax for these options instead. > > > + > > > +There is no intention to remove support for non-JSON syntax entirely, but > > > +future versions may change the way to spell some options. > > > > As it stands, this is basically saying "pretty much anybody > > using the command line, your stuff may break in future, instead > > use some other interface you've never heard of, which doesn't > > appear to be documented in the manual and which none of the > > documentation's examples use". > > The documentation is a valid criticism. We need to document the JSON > interfaces properly (which will really mostly be a pointer to the > existing QMP documentation at least for -object, but it's important to > tell people where to look for the details). > > > Is there some more limited deprecation we can do rather than "the > > entire commandline for almost all users" ? > > I don't think "almost all" users is true. > I see three groups of users ...all of whom "rely on a stable interface for -device and -object", and only two of whom it's reasonable to say "use the JSON version" to. > 1. Using a management tool that is probably using libvirt. This is >likely the vast majority of users. They won't notice a difference >because libvirt abstracts it away. libvirt developers are actively >asking us for JSON (and QAPI) based interfaces because using the same >representation to describe configurations in QMP and on the CLI makes >their life easier. Yes, absolutely we should be recommending that libvirt and other management interfaces use the JSON. > 2. People starting QEMU on the command line manually. This is >essentially the same situation as HMP: If something changes, you get >an error message, you look up the new syntax, done. Small >inconvenience, but that's it. This includes simple scripts that just >start QEMU and are only used to store a long command line somewhere. It's a small inconvenience that we seem to be imposing on our users on a pretty frequent basis. Moreover, each one of these really needs to be its own deprecation, so that users actually can have some advance notice if they need it and look up the new syntax. We shouldn't hide them all under this umbrella "we might break anything at any time" entry, which I think will pretty much encourage breaking compatibility more often because you don't have to think about "oh, we should deprecate this and maybe print warnings about use of deprecated syntax and then drop it later", you can just break things and point to this "we said -device wasn't going to be stable" entry. As a concrete example, the commit message for this patch vaguely mentions some issues "around list handling", which gives me no idea at all about what syntax is going to break in future or whether it is likely to affect scripts I've written. > 3. People writing more complex scripts or applications that invoke QEMU >manually instead of using libvirt. They may actually be hurt by such >changes. They should probably be using a proper machine interface, >i.e. JSON mode, so the deprecation notice is for them to change >their code. This is probably a small minority and not "almost all >users". Yeah, this group is kind of similar to group 1 (well, at one end it shades into group 1 and at the other into group 2). More generally, I think I'd rather see the deprecation of the old approach appear after some period when the JSON command line version has been available to users and adopted by major consumers like libvirt, not as a patch in the same series which is introducing the JSON syntax in the first plaec. -- PMM
Re: [PATCH 06/11] qdev: Add Error parameter to qdev_set_id()
Hi Kevin,
I proposed a very similar patch in our rfc series because we needed some
of the cleaning you do here.
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05679.html
I've added a bit of doc for the function, feel free to take it if you want.
On 9/24/21 16:09, Vladimir Sementsov-Ogievskiy wrote:
24.09.2021 12:04, Kevin Wolf wrote:
object_property_add_child() fails (with _abort) if an object with
the same name already exists. As long as QemuOpts is in use for -device
and device_add, it catches duplicate IDs before qdev_set_id() is even
called. However, for enabling non-QemuOpts code paths, we need to make
sure that the condition doesn't cause a crash, but fails gracefully.
Signed-off-by: Kevin Wolf
---
include/monitor/qdev.h | 2 +-
hw/xen/xen-legacy-backend.c | 3 ++-
softmmu/qdev-monitor.c | 16 ++--
3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 389287eb44..7961308c75 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,6 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
Error **errp);
int qdev_device_help(QemuOpts *opts);
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, char *id);
+void qdev_set_id(DeviceState *dev, char *id, Error **errp);
#endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..17aca85ddc 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice
*xen_be_get_xendev(const char *type, int dom,
xendev = g_malloc0(ops->size);
object_initialize(>qdev, ops->size, TYPE_XENBACKEND);
OBJECT(xendev)->free = g_free;
- qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type,
dev));
+ qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+ _abort);
qdev_realize(DEVICE(xendev), xen_sysbus, _fatal);
object_unref(OBJECT(xendev));
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1207e57a46..c2af906df0 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -593,26 +593,27 @@ static BusState *qbus_find(const char *path,
Error **errp)
}
/* Takes ownership of @id, will be freed when deleting the device */
-void qdev_set_id(DeviceState *dev, char *id)
+void qdev_set_id(DeviceState *dev, char *id, Error **errp)
According to recommendations in error.h, worth adding also return value
(for example true=success false=failure), so [..]
{
if (id) {
dev->id = id;
}
Unrelated but.. What's the strange logic?
Is it intended that with passed id=NULL we don't update dev->id variable
but try to do following logic with old dev->id?
dev->id is expected to be NULL. The object is created just before
calling this function so it is always the case. We could probably assert
this.
if (dev->id) {
- object_property_add_child(qdev_get_peripheral(), dev->id,
- OBJECT(dev));
+ object_property_try_add_child(qdev_get_peripheral(), dev->id,
+ OBJECT(dev), errp);
} else {
static int anon_count;
gchar *name = g_strdup_printf("device[%d]", anon_count++);
- object_property_add_child(qdev_get_peripheral_anon(), name,
- OBJECT(dev));
+ object_property_try_add_child(qdev_get_peripheral_anon(), name,
+ OBJECT(dev), errp);
g_free(name);
}
}
DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
{
+ ERRP_GUARD();
DeviceClass *dc;
const char *driver, *path;
DeviceState *dev = NULL;
@@ -691,7 +692,10 @@ DeviceState *qdev_device_add(QemuOpts *opts,
Error **errp)
}
}
- qdev_set_id(dev, g_strdup(qemu_opts_id(opts)));
+ qdev_set_id(dev, g_strdup(qemu_opts_id(opts)), errp);
+ if (*errp) {
+ goto err_del_dev;
+ }
[..] here we'll have
if (!qdev_set_id(...)) {
goto err_del_dev;
}
and no need for ERRP_GUARD.
/* set properties */
if (qemu_opt_foreach(opts, set_property, dev, errp)) {
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
On 9/24/21 11:04, Kevin Wolf wrote:
Directly call qdev_device_add_from_qdict() for QMP device_add instead of
first going through QemuOpts and converting back to QDict.
Note that this changes the behaviour of device_add, though in ways that
should be considered bug fixes:
QemuOpts ignores differences between data types, so you could
successfully pass a string "123" for an integer property, or a string
"on" for a boolean property (and vice versa). After this change, the
correct data type for the property must be used in the JSON input.
qemu_opts_from_qdict() also silently ignores any options whose value is
a QDict, QList or QNull.
To illustrate, the following QMP command was accepted before and is now
rejected for both reasons:
{ "execute": "device_add",
"arguments": { "driver": "scsi-cd",
"drive": { "completely": "invalid" },
"physical_block_size": "4096" } }
Signed-off-by: Kevin Wolf
---
softmmu/qdev-monitor.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index c09b7430eb..8622ccade6 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
qdev_print_devinfos(true);
}
-void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
+static void monitor_device_add(QDict *qdict, QObject **ret_data,
+ bool from_json, Error **errp)
{
QemuOpts *opts;
DeviceState *dev;
@@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error
**errp)
qemu_opts_del(opts);
return;
}
-dev = qdev_device_add(opts, errp);
+qemu_opts_del(opts);
+
+dev = qdev_device_add_from_qdict(qdict, from_json, errp);
Hi Kevin,
I'm wandering if deleting the opts (which remove it from the "device"
opts list) is really a no-op ?
The opts list is, eg, traversed in hw/net/virtio-net.c in the function
failover_find_primary_device_id() which may be called during the
virtio_net_set_features() (a TYPE_VIRTIO_NET method).
I do not have the knowledge to tell when this method is called. But If
this is after we create the devices. Then the list will be empty at this
point now.
It seems, there are 2 other calling sites of
"qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c
and net/vhost-vdpa.c
--
Damien
Re: [PATCH 09/11] qdev: Avoid QemuOpts in QMP device_add
Am 27.09.2021 um 13:06 hat Damien Hedde geschrieben:
> On 9/24/21 11:04, Kevin Wolf wrote:
> > Directly call qdev_device_add_from_qdict() for QMP device_add instead of
> > first going through QemuOpts and converting back to QDict.
> >
> > Note that this changes the behaviour of device_add, though in ways that
> > should be considered bug fixes:
> >
> > QemuOpts ignores differences between data types, so you could
> > successfully pass a string "123" for an integer property, or a string
> > "on" for a boolean property (and vice versa). After this change, the
> > correct data type for the property must be used in the JSON input.
> >
> > qemu_opts_from_qdict() also silently ignores any options whose value is
> > a QDict, QList or QNull.
> >
> > To illustrate, the following QMP command was accepted before and is now
> > rejected for both reasons:
> >
> > { "execute": "device_add",
> >"arguments": { "driver": "scsi-cd",
> > "drive": { "completely": "invalid" },
> > "physical_block_size": "4096" } }
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > softmmu/qdev-monitor.c | 18 +++---
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index c09b7430eb..8622ccade6 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -812,7 +812,8 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
> > qdev_print_devinfos(true);
> > }
> > -void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> > +static void monitor_device_add(QDict *qdict, QObject **ret_data,
> > + bool from_json, Error **errp)
> > {
> > QemuOpts *opts;
> > DeviceState *dev;
> > @@ -825,7 +826,9 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> > Error **errp)
> > qemu_opts_del(opts);
> > return;
> > }
> > -dev = qdev_device_add(opts, errp);
> > +qemu_opts_del(opts);
> > +
> > +dev = qdev_device_add_from_qdict(qdict, from_json, errp);
>
> Hi Kevin,
>
> I'm wandering if deleting the opts (which remove it from the "device" opts
> list) is really a no-op ?
It's not exactly a no-op. Previously, the QemuOpts would only be freed
when the device is destroying, now we delete it immediately after
creating the device. This could matter in some cases.
The one case I was aware of is that QemuOpts used to be responsible for
checking for duplicate IDs. Obviously, it can't do this job any more
when we call qemu_opts_del() right after creating the device. This is
the reason for patch 6.
> The opts list is, eg, traversed in hw/net/virtio-net.c in the function
> failover_find_primary_device_id() which may be called during the
> virtio_net_set_features() (a TYPE_VIRTIO_NET method).
> I do not have the knowledge to tell when this method is called. But If this
> is after we create the devices. Then the list will be empty at this point
> now.
>
> It seems, there are 2 other calling sites of
> "qemu_opts_foreach(qemu_find_opts("device"), [...]" in net/vhost-user.c and
> net/vhost-vdpa.c
Yes, you are right. These callers probably need to be changed. Going
through the command line options rather than looking at the actual
device objects that exist doesn't feel entirely clean anyway.
Kevin
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 11:00 hat Peter Maydell geschrieben: > On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > which results in some incompatibilities, mainly around list handling. > > Mark the non-JSON version of both as unstable syntax so that management > > tools switch to JSON and we can later make the change without breaking > > things. > > > > Signed-off-by: Kevin Wolf > > > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > > +'' > > + > > +If you rely on a stable interface for ``-device`` and ``-object`` that > > doesn't > > +change incompatibly between QEMU versions (e.g. because you are using the > > QEMU > > +command line as a machine interface in scripts rather than interactively), > > use > > +JSON syntax for these options instead. > > + > > +There is no intention to remove support for non-JSON syntax entirely, but > > +future versions may change the way to spell some options. > > As it stands, this is basically saying "pretty much anybody > using the command line, your stuff may break in future, instead > use some other interface you've never heard of, which doesn't > appear to be documented in the manual and which none of the > documentation's examples use". The documentation is a valid criticism. We need to document the JSON interfaces properly (which will really mostly be a pointer to the existing QMP documentation at least for -object, but it's important to tell people where to look for the details). > Is there some more limited deprecation we can do rather than "the > entire commandline for almost all users" ? I don't think "almost all" users is true. I see three groups of users: 1. Using a management tool that is probably using libvirt. This is likely the vast majority of users. They won't notice a difference because libvirt abstracts it away. libvirt developers are actively asking us for JSON (and QAPI) based interfaces because using the same representation to describe configurations in QMP and on the CLI makes their life easier. 2. People starting QEMU on the command line manually. This is essentially the same situation as HMP: If something changes, you get an error message, you look up the new syntax, done. Small inconvenience, but that's it. This includes simple scripts that just start QEMU and are only used to store a long command line somewhere. 3. People writing more complex scripts or applications that invoke QEMU manually instead of using libvirt. They may actually be hurt by such changes. They should probably be using a proper machine interface, i.e. JSON mode, so the deprecation notice is for them to change their code. This is probably a small minority and not "almost all users". Yes, we could in theory do a more limited deprecation. The planned change from my side is just going from QemuOpts to the keyval parser, which doesn't change anything in the vast majority of cases. But we have the separation in the monitor between QMP and HMP for a good reason: Requirements for a good machine interface are different from a good human interface. The same is true for the command line. So it seems to make a lot of sense to me to have both a machine interface (JSON based) and a human interface (non-JSON) on the command line, too, and take the same liberties for evolving the human interface as we already do in HMP - which means that it's technically an unstable interface where compatibility doesn't prevent improvements, but not that it looks completely different in every QEMU version. Kevin
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Mon, Sep 27, 2021 at 12:17:03PM +0200, Kevin Wolf wrote: > Am 27.09.2021 um 10:21 hat Daniel P. Berrangé geschrieben: > > On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote: > > > On 24/09/21 11:04, Kevin Wolf wrote: > > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > > which results in some incompatibilities, mainly around list handling. > > > > Mark the non-JSON version of both as unstable syntax so that management > > > > tools switch to JSON and we can later make the change without breaking > > > > things. > > > > > > Maybe we need a different section for unstable syntaxes, rather than > > > overloading deprecated.rst? > > > > This case feels like it hits two scenarios - we want to declare it > > unstable, which is something we should document in qemu-options.hx. > > Actually, I think a section for unstable syntaxes or generally > compatibility promises wouldn't hurt. When I checked, I couldn't find > any documentation about the support status of HMP either. > > Basically, I imagine HMP and non-JSON -device/-object would be on the > same level: We don't change things without a reason, but if we do want > to change things, compatibility isn't an argument against making the > change. > > > We want to also to warn of specific breakage when the impl changes > > which is something suitable for deprecations. > > We don't do this for HMP either for individual changes. Well HMP as a whole is considered non-stable, so we don't need to call out individual things. We've got a simple story that QMP == stable, HMP == unstable. The comparison here would be if we declared the entire QEMU CLI to be unstable, except for JSON syntax args. > Basically this deprecation notice was meant to make people aware that > we're lowering the support status from a long-term stable interface to > HMP-like. Bearing in mind our previous discussions it feels like our goal is that we're tending towards a world where we are only wanting to consider JSON based configuration to be stable, and everything else non-stable. I think that's a good long term plan, but if we're really doing that then I think we need to big picture explain it in our docs rather than mention it in passing against individual args. BTW I'm also not a fan of deprecating stuff when our documentation is still using the deprecated syntax and nothing shows the new preferred syntax. We've got alot of results for $ git grep -- ' -object' 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: [PATCH v5 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
On Mon, 27 Sep 2021, Ján Tomko wrote:
> On a Monday in 2021, Ani Sinha wrote:
> > The following change in qemu added support for a global boolean flag
> > specific
> > to i440fx machines that would turn off or on acpi based hotplug for pci root
> > bus:
> >
> > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on
> > the root bus")
> >
> > The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in
> > qemu
> > commandline. It is enabled by default. This patch adds the corresponding
> > qemu
> > capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.
> >
> > 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
> > Reviewed-by: Laine Stump
> > ---
> > src/qemu/qemu_capabilities.c | 4
> > src/qemu/qemu_capabilities.h | 3 +++
> > tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
> > tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
> > tests/qemucapabilitiesdata/caps_6.1.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 db5432c9fc..71aca20c4c 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > "s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
> > "set-action", /* QEMU_CAPS_SET_ACTION */
> > "virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
> > +
> > + /* 410 */
> > + "piix4-acpi-root-hotplug-en", /*
> > QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
>
> What does "en" stand for?
en stands for "enable"
>
> "piix4.acpi-root-hotplug" (with the dot) would give a rough idea what
> property of what device is being probed (even though neither the
> "device" name nor the property name is exact in my example)
>
OK for this one I will wait and see what Laine thinks. Its a trivial
change.
Re: [PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On Mon, 27 Sep 2021, Ján Tomko wrote:
> On a Monday in 2021, Ani Sinha wrote:
> > This change introduces libvirt xml support to enable/disable hotplug on the
> > pci-root controller. It adds a 'target' subelement for the pci-root
> > controller
> > with a 'hotplug' property. This property can be used to enable or disable
> > hotplug for the pci-root controller. For example, in order to disable
> > hotplug
> > on the pci-root controller, one has to use set '' as
> > shown below:
> >
> >
> >
> >
> >
> > '' option would enable hotplug for pci-root controller.
> > This is also the default value. This option is only available for pc machine
> > types and is applicable for qemu only/kvm accelerator onlt.This feature was
>
> s/onlt./only. /
Will fix in v6.
>
> > introduced from qemu version 5.2 with the following change in qemu
> > repository:
> >
> > 3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on
> > the root bus")
> >
> > The above qemu commit describes some reasons why users might to disable
> > hotplug
> > on PCI root buses.
> >
> > Related unit tests to exercise the new conf option has also been added.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > docs/formatdomain.rst | 12
> > src/qemu/qemu_validate.c | 9 +-
> > .../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++
> > .../pc-i440fx-acpi-root-hotplug-enable.xml| 30 +++
> > .../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
> > .../pc-i440fx-acpi-root-hotplug-enable.xml| 1 +
> > tests/qemuxml2xmltest.c | 4 +++
> > 7 files changed, 81 insertions(+), 6 deletions(-)
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > create mode 100644
> > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
> > create mode 12
> > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > create mode 12
> > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
> >
> > @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > virDomainControllerDef *cont,
> > /* hotplug */
> > if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> > switch ((virDomainControllerModelPCI) cont->model) {
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > +if (!virQEMUCapsGet(qemuCaps,
> > QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("setting the %s property on a pci-root
> > device is not supported by this QEMU binary"),
> > + "hotplug");
>
> No need to create a new translatable string, the one used by the case
> below can be reused:
>
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>_("setting the hotplug property on a '%s'
> device is not supported by this QEMU binary"),
>"pci-root");
yes very good suggestion. Will fix in v6.
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
Am 27.09.2021 um 10:21 hat Daniel P. Berrangé geschrieben: > On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote: > > On 24/09/21 11:04, Kevin Wolf wrote: > > > We want to switch both from QemuOpts to the keyval parser in the future, > > > which results in some incompatibilities, mainly around list handling. > > > Mark the non-JSON version of both as unstable syntax so that management > > > tools switch to JSON and we can later make the change without breaking > > > things. > > > > Maybe we need a different section for unstable syntaxes, rather than > > overloading deprecated.rst? > > This case feels like it hits two scenarios - we want to declare it > unstable, which is something we should document in qemu-options.hx. Actually, I think a section for unstable syntaxes or generally compatibility promises wouldn't hurt. When I checked, I couldn't find any documentation about the support status of HMP either. Basically, I imagine HMP and non-JSON -device/-object would be on the same level: We don't change things without a reason, but if we do want to change things, compatibility isn't an argument against making the change. > We want to also to warn of specific breakage when the impl changes > which is something suitable for deprecations. We don't do this for HMP either for individual changes. Basically this deprecation notice was meant to make people aware that we're lowering the support status from a long-term stable interface to HMP-like. Kevin
Entering freeze for libvirt-7.8.0
I have just tagged v7.8.0-rc1 in the repository and pushed signed tarballs and source RPMs to https://libvirt.org/sources/ Please give the release candidate some testing and in case you find a serious issue which should have a fix in the upcoming release, feel free to reply to this thread to make sure the issue is more visible. If you have not done so yet, please update NEWS.rst to document any significant change you made since the last release. Thanks, Jirka
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Fri, 24 Sept 2021 at 10:14, Kevin Wolf wrote: > > We want to switch both from QemuOpts to the keyval parser in the future, > which results in some incompatibilities, mainly around list handling. > Mark the non-JSON version of both as unstable syntax so that management > tools switch to JSON and we can later make the change without breaking > things. > > Signed-off-by: Kevin Wolf > +Stable non-JSON ``-device`` and ``-object`` syntax (since 6.2) > +'' > + > +If you rely on a stable interface for ``-device`` and ``-object`` that > doesn't > +change incompatibly between QEMU versions (e.g. because you are using the > QEMU > +command line as a machine interface in scripts rather than interactively), > use > +JSON syntax for these options instead. > + > +There is no intention to remove support for non-JSON syntax entirely, but > +future versions may change the way to spell some options. As it stands, this is basically saying "pretty much anybody using the command line, your stuff may break in future, instead use some other interface you've never heard of, which doesn't appear to be documented in the manual and which none of the documentation's examples use". Is there some more limited deprecation we can do rather than "the entire commandline for almost all users" ? thanks -- PMM
Re: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On Mon, Sep 27, 2021 at 10:15:43AM +0200, Paolo Bonzini wrote: > On 24/09/21 11:04, Kevin Wolf wrote: > > We want to switch both from QemuOpts to the keyval parser in the future, > > which results in some incompatibilities, mainly around list handling. > > Mark the non-JSON version of both as unstable syntax so that management > > tools switch to JSON and we can later make the change without breaking > > things. > > Maybe we need a different section for unstable syntaxes, rather than > overloading deprecated.rst? This case feels like it hits two scenarios - we want to declare it unstable, which is something we should document in qemu-options.hx. We want to also to warn of specific breakage when the impl changes which is something suitable for deprecations. 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: [PATCH 11/11] Deprecate stable non-JSON -device and -object
On 24/09/21 11:04, Kevin Wolf wrote: We want to switch both from QemuOpts to the keyval parser in the future, which results in some incompatibilities, mainly around list handling. Mark the non-JSON version of both as unstable syntax so that management tools switch to JSON and we can later make the change without breaking things. Maybe we need a different section for unstable syntaxes, rather than overloading deprecated.rst? Paolo
Re: [PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller
On a Monday in 2021, Ani Sinha wrote:
This change introduces libvirt xml support to enable/disable hotplug on the
pci-root controller. It adds a 'target' subelement for the pci-root controller
with a 'hotplug' property. This property can be used to enable or disable
hotplug for the pci-root controller. For example, in order to disable hotplug
on the pci-root controller, one has to use set '' as
shown below:
'' option would enable hotplug for pci-root controller.
This is also the default value. This option is only available for pc machine
types and is applicable for qemu only/kvm accelerator onlt.This feature was
s/onlt./only. /
introduced from qemu version 5.2 with the following change in qemu repository:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root
bus")
The above qemu commit describes some reasons why users might to disable hotplug
on PCI root buses.
Related unit tests to exercise the new conf option has also been added.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 12
src/qemu/qemu_validate.c | 9 +-
.../pc-i440fx-acpi-root-hotplug-disable.xml | 30 +++
.../pc-i440fx-acpi-root-hotplug-enable.xml| 30 +++
.../pc-i440fx-acpi-root-hotplug-disable.xml | 1 +
.../pc-i440fx-acpi-root-hotplug-enable.xml| 1 +
tests/qemuxml2xmltest.c | 4 +++
7 files changed, 81 insertions(+), 6 deletions(-)
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 100644 tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
@@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
/* hotplug */
if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
switch ((virDomainControllerModelPCI) cont->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("setting the %s property on a pci-root device is
not supported by this QEMU binary"),
+ "hotplug");
No need to create a new translatable string, the one used by the case
below can be reused:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("setting the hotplug property on a '%s' device is
not supported by this QEMU binary"),
"pci-root");
Jano
+return -1;
+}
+break;
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
signature.asc
Description: PGP signature
Re: [PATCH v5 1/4] qemu: capablities: detect presence of acpi-root-pci-hotplug for i440fx machines
On a Monday in 2021, Ani Sinha wrote:
The following change in qemu added support for a global boolean flag specific
to i440fx machines that would turn off or on acpi based hotplug for pci root
bus:
3d7e78aaf ("Introduce a new flag for i440fx to disable PCI hotplug on the root
bus")
The option is passed as "-global PIIX4_PM.acpi-root-pci-hotplug=on" etc in qemu
commandline. It is enabled by default. This patch adds the corresponding qemu
capabilities in libvirt as QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG.
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
Reviewed-by: Laine Stump
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.1.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 db5432c9fc..71aca20c4c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -639,6 +639,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"s390-pv-guest", /* QEMU_CAPS_S390_PV_GUEST */
"set-action", /* QEMU_CAPS_SET_ACTION */
"virtio-blk.queue-size", /* QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE */
+
+ /* 410 */
+ "piix4-acpi-root-hotplug-en", /*
QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG */
What does "en" stand for?
"piix4.acpi-root-hotplug" (with the dot) would give a rough idea what
property of what device is being probed (even though neither the
"device" name nor the property name is exact in my example)
Jano
);
@@ -1465,6 +1468,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsIDEDrive[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPiix4PM[] = {
{ "disable_s3", QEMU_CAPS_PIIX_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_PIIX_DISABLE_S4, NULL },
+{ "acpi-root-pci-hotplug", QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBRedir[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 097f28bd40..c2d1e352bd 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -620,6 +620,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_SET_ACTION, /* 'set-action' QMP command */
QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, /* virtio-blk-*.queue-size */
+/* 410 */
+QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG, /* -M pc
PIIX4_PM.acpi-root-pci-hotplug */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
index e09880e937..ffd0e66d00 100644
--- a/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml
@@ -233,6 +233,7 @@
+
5002000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
index 571336c1fa..658a1e742f 100644
--- a/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml
@@ -241,6 +241,7 @@
+
600
0
43100242
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index 74b87847d0..5bb21fec47 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6001000
0
43100243
--
2.25.1
signature.asc
Description: PGP signature
Re: [PATCH] qemu: ingore the transient domain state in fake reboot
On 9/26/21 11:06 AM, Zhenzhong Duan wrote: > When action for 'on_poweroff' is set to 'restart', 'fake reboot' > is triggered and qemu shutdown state is transient. Domain state > need not to be changed and events not sent in this case. > > Fixes:4ffc807214cb80086d57e1d3e7b60959a41d2874 > Signed-off-by: Zhenzhong Duan > --- > src/qemu/qemu_process.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Michal Privoznik and pushed. Michal
