[PATCH 6/7] util: make virPCIDeviceIsPCIExpress() more intelligent
Until now there has been an extra bit of code in
qemuDomainDeviceCalculatePCIConnectFlag() (one of the two callers of
virPCIDeviceIsPCIExpress()) that tries to determine if a device is
PCIe by looking at the *length* of its sysfs config file; it only does
this when libvirt is running as a non-root process.
This patch takes advantage of our newfound ability to tell the
difference between "I read a 0 from the device PCI config file" and "I
couldn't read the PCI Express Capabilities because I don't have
sufficient permission" to move the file length check down into
virPCIDeviceIsPCIExpress(), and do that check any time we fail while
reading the config file (not only when the process is non-root).
Fixes: https://bugzilla.redhat.com/1901685
Signed-off-by: Laine Stump
---
src/util/virpci.c | 42 ++
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2f99bb93bd..01a156dfcf 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -73,10 +73,18 @@ struct _virPCIDevice {
char *used_by_drvname;
char *used_by_domname;
+/* The following 5 items are only valid after virPCIDeviceInit()
+ * has been called for the virPCIDevice object. This is *not* done
+ * in most cases (because it creates extra overhead, and parts of
+ * it can fail if libvirtd is running unprivileged)
+ */
unsigned int pcie_cap_pos;
unsigned int pci_pm_cap_pos;
bool has_flr;
bool has_pm_reset;
+bool is_pcie;
+/**/
+
bool managed;
virPCIStubDriver stubDriver;
@@ -594,7 +602,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
-virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+if (virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos) < 0)
+goto error;
if (pos) {
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
@@ -619,8 +628,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
return true;
}
+ error:
VIR_DEBUG("%s %s: no FLR capability found", dev->id, dev->name);
-
return false;
}
@@ -905,7 +914,32 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev,
int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
-virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
+dev->is_pcie = false;
+if (virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos) < 0) {
+/* an unprivileged process is unable to read *all* of a
+ * device's PCI config (it can only read the first 64
+ * bytes, which isn't enough for see the Express
+ * Capabilities data). If virPCIDeviceFindCapabilityOffset
+ * returns failure (and not just a pcie_cap_pos == 0,
+ * which is *success* at determining the device is *not*
+ * PCIe) we make an educated guess based on the length of
+ * the device's config file - if it is 256 bytes, then it
+ * is definitely a legacy PCI device. If it's larger than
+ * that, then it is *probably PCIe (although it could be
+ * PCI-x, but those are extremely rare). If the config
+ * file can't be found (in which case the "length" will be
+ * -1), then we blindly assume the most likely outcome -
+ * PCIe.
+ */
+off_t configLen = virFileLength(virPCIDeviceGetConfigPath(dev), -1);
+
+if (configLen != 256)
+dev->is_pcie = true;
+
+} else {
+dev->is_pcie = (dev->pcie_cap_pos != 0);
+}
+
virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
@@ -2609,7 +2643,7 @@ virPCIDeviceIsPCIExpress(virPCIDevicePtr dev)
if (virPCIDeviceInit(dev, fd) < 0)
goto cleanup;
-ret = dev->pcie_cap_pos != 0;
+ret = dev->is_pcie;
cleanup:
virPCIDeviceConfigClose(dev, fd);
--
2.28.0
[PATCH 5/7] util: change call sequence for virPCIDeviceFindCapabilityOffset()
Previously there was no way to differentiate between this function 1)
encountering an error while reading the pci config, and 2) determining
that the device in question is a conventional PCI device, and so has
no Express Capabilities.
The difference between these two conditions is important, because an
unprivileged libvirtd will be unable to read all of the pci config (it
can only read the first 64 bytes, and will get ENOENT when it tries to
seek past that limit) even though the device is in fact a PCIe device.
This patch just changes virPCIDeviceFindCapabilityOffset() to put the
determined offset into an argument of the function (rather than
sending it back as the return value), and to return the standard "0 on
success, -1 on failure". Failure is determined by checking the value
of errno after each attemptd read of the config file (which can only
work reliably if errno is reset to 0 before each read, and after
virPCIDeviceFindCapabilityOffset() has finished examining it).
(NB: if the config file is read successfully, but no Express
Capabilities are found, then the function returns success, but the
returned offset will be 0 (which is an impossible offset for Express
Capabilities, and so easily recognizeable).
An upcoming patch will take advantage of the change made here.
Signed-off-by: Laine Stump
---
src/util/virpci.c | 42 +++---
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 9109fb4f3a..2f99bb93bd 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -329,6 +329,7 @@ virPCIDeviceRead(virPCIDevicePtr dev,
unsigned int buflen)
{
memset(buf, 0, buflen);
+errno = 0;
if (lseek(cfgfd, pos, SEEK_SET) != pos ||
saferead(cfgfd, buf, buflen) != buflen) {
@@ -483,19 +484,24 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate
predicate,
return ret;
}
-static uint8_t
+static int
virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
int cfgfd,
- unsigned int capability)
+ unsigned int capability,
+ unsigned int *offset)
{
uint16_t status;
uint8_t pos;
+*offset = 0; /* assume failure (*nothing* can be at offset 0) */
+
status = virPCIDeviceRead16(dev, cfgfd, PCI_STATUS);
-if (!(status & PCI_STATUS_CAP_LIST))
-return 0;
+if (errno != 0 || !(status & PCI_STATUS_CAP_LIST))
+goto error;
pos = virPCIDeviceRead8(dev, cfgfd, PCI_CAPABILITY_LIST);
+if (errno != 0)
+goto error;
/* Zero indicates last capability, capabilities can't
* be in the config space header and 0xff is returned
@@ -506,18 +512,31 @@ virPCIDeviceFindCapabilityOffset(virPCIDevicePtr dev,
*/
while (pos >= PCI_CONF_HEADER_LEN && pos != 0xff) {
uint8_t capid = virPCIDeviceRead8(dev, cfgfd, pos);
+if (errno != 0)
+goto error;
+
if (capid == capability) {
VIR_DEBUG("%s %s: found cap 0x%.2x at 0x%.2x",
dev->id, dev->name, capability, pos);
-return pos;
+*offset = pos;
+return 0;
}
pos = virPCIDeviceRead8(dev, cfgfd, pos + 1);
+if (errno != 0)
+goto error;
}
-VIR_DEBUG("%s %s: failed to find cap 0x%.2x", dev->id, dev->name,
capability);
+ error:
+VIR_DEBUG("%s %s: failed to find cap 0x%.2x (%s)",
+ dev->id, dev->name, capability, g_strerror(errno));
-return 0;
+/* reset errno in case the failure was due to insufficient
+ * privileges to read the entire PCI config file
+ */
+errno = 0;
+
+return -1;
}
static unsigned int
@@ -553,7 +572,7 @@ static bool
virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
{
uint32_t caps;
-uint8_t pos;
+unsigned int pos;
g_autofree char *path = NULL;
int found;
@@ -575,7 +594,8 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
* the same thing, except for conventional PCI
* devices. This is not common yet.
*/
-pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF);
+virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_AF, &pos);
+
if (pos) {
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
@@ -885,8 +905,8 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev,
int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
-dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
-dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
+virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP,
&dev->pcie_cap_pos);
+virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM,
&dev->pci_pm_cap_pos);
[PATCH 4/7] util: make read error of PCI config file more detailed
The new message is more verbose/useful, but only logged at debug level
instead of as a warning (since it could easily happen in a non-error
situation).
Signed-off-by: Laine Stump
---
src/util/virpci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 31622cddfa..9109fb4f3a 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -332,8 +332,8 @@ virPCIDeviceRead(virPCIDevicePtr dev,
if (lseek(cfgfd, pos, SEEK_SET) != pos ||
saferead(cfgfd, buf, buflen) != buflen) {
-VIR_WARN("Failed to read from '%s' : %s", dev->path,
- g_strerror(errno));
+VIR_DEBUG("Failed to read %u bytes at %u from '%s' : %s",
+ buflen, pos, dev->path, g_strerror(errno));
return -1;
}
return 0;
--
2.28.0
[PATCH 7/7] qemu: remove now-redundant check for file length when determining PCIe vs. PCI
Now that virPCIDeviceIsPCIExpress() checks the length of the file when
the process lacks sufficient privilege to read the entire PCI config
file in sysfs, we can remove the open-coding for that case from its
consumer.
Signed-off-by: Laine Stump
---
src/qemu/qemu_domain_address.c | 38 +++---
1 file changed, 3 insertions(+), 35 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b07672e2f4..f0ba318cc8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -579,7 +579,6 @@
qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr qemuCaps,
*/
static virDomainPCIConnectFlags
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
- virQEMUDriverPtr driver,
virDomainPCIConnectFlags pcieFlags,
virDomainPCIConnectFlags virtioFlags)
{
@@ -802,7 +801,6 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_HOSTDEV: {
virDomainHostdevDefPtr hostdev = dev->data.hostdev;
-bool isExpress = false;
g_autoptr(virPCIDevice) pciDev = NULL;
virPCIDeviceAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
@@ -873,37 +871,7 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
return pcieFlags;
}
-if (!driver->privileged) {
-/* unprivileged libvirtd is unable to read *all* of a
- * device's PCI config (it can only read the first 64
- * bytes, which isn't enough for the check that's done
- * in virPCIDeviceIsPCIExpress()), so instead of
- * trying and failing, we make an educated guess based
- * on the length of the device's config file - if it
- * is 256 bytes, then it is definitely a legacy PCI
- * device. If it's larger than that, then it is
- * *probably PCIe (although it could be PCI-x, but
- * those are extremely rare). If the config file can't
- * be found (in which case the "length" will be -1),
- * then we blindly assume the most likely outcome -
- * PCIe.
- */
-off_t configLen
- = virFileLength(virPCIDeviceGetConfigPath(pciDev), -1);
-
-if (configLen == 256)
-return pciFlags;
-
-return pcieFlags;
-}
-
-/* If we are running with privileges, we can examine the
- * PCI config contents with virPCIDeviceIsPCIExpress() for
- * a definitive answer.
- */
-isExpress = virPCIDeviceIsPCIExpress(pciDev);
-
-if (isExpress)
+if (virPCIDeviceIsPCIExpress(pciDev))
return pcieFlags;
return pciFlags;
@@ -1124,7 +1092,7 @@ qemuDomainFillDevicePCIConnectFlagsIter(virDomainDefPtr
def G_GNUC_UNUSED,
qemuDomainFillDevicePCIConnectFlagsIterData *data = opaque;
info->pciConnectFlags
-= qemuDomainDeviceCalculatePCIConnectFlags(dev, data->driver,
+= qemuDomainDeviceCalculatePCIConnectFlags(dev,
data->pcieFlags,
data->virtioFlags);
return 0;
@@ -1468,7 +1436,7 @@ qemuDomainFillDevicePCIConnectFlags(virDomainDefPtr def,
qemuDomainFillDevicePCIConnectFlagsIterInit(def, qemuCaps, driver,
&data);
info->pciConnectFlags
-= qemuDomainDeviceCalculatePCIConnectFlags(dev, data.driver,
+= qemuDomainDeviceCalculatePCIConnectFlags(dev,
data.pcieFlags,
data.virtioFlags);
}
--
2.28.0
[PATCH 2/7] util: simplify calling of virPCIDeviceDetectFunctionLevelReset()
This function returned an int, and that int was being checked for < 0
in its solitary caller, but within the function it would only ever
return 0 or 1. Change the function itself to return a bool, and the
caller to just directly set the flag in the virPCIDevice.
Signed-off-by: Laine Stump
---
src/util/virpci.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index b63abac8cc..403ef1ec8e 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -549,7 +549,7 @@ virPCIDeviceFindExtendedCapabilityOffset(virPCIDevicePtr
dev,
/* detects whether this device has FLR. Returns 0 if the device does
* not have FLR, 1 if it does, and -1 on error
*/
-static int
+static bool
virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd)
{
uint32_t caps;
@@ -567,7 +567,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
caps = virPCIDeviceRead32(dev, cfgfd, dev->pcie_cap_pos +
PCI_EXP_DEVCAP);
if (caps & PCI_EXP_DEVCAP_FLR) {
VIR_DEBUG("%s %s: detected PCIe FLR capability", dev->id,
dev->name);
-return 1;
+return true;
}
}
@@ -580,7 +580,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
caps = virPCIDeviceRead16(dev, cfgfd, pos + PCI_AF_CAP);
if (caps & PCI_AF_CAP_FLR) {
VIR_DEBUG("%s %s: detected PCI FLR capability", dev->id,
dev->name);
-return 1;
+return true;
}
}
@@ -596,12 +596,12 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
if (found) {
VIR_DEBUG("%s %s: buggy device didn't advertise FLR, but is a VF;
forcing flr on",
dev->id, dev->name);
-return 1;
+return true;
}
VIR_DEBUG("%s %s: no FLR capability found", dev->id, dev->name);
-return 0;
+return false;
}
/* Require the device has the PCI Power Management capability
@@ -885,15 +885,10 @@ virPCIDeviceTryPowerManagementReset(virPCIDevicePtr dev,
int cfgfd)
static int
virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
{
-int flr;
-
dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
-flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
-if (flr < 0)
-return flr;
-dev->has_flr= !!flr;
-dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
+dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
+dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
return 0;
}
--
2.28.0
[PATCH 1/7] qemu: use g_autoptr for a virPCIDevice
The one instance of a virPCIDevice in
qemuDomainDeviceCalculatePCIConnectFlags() needs to be converted to
use g_autoptr as a prerequisite for a bugfix. It's in this patch by
itself (rather than in a patch converting all virPCIDevice usages to
g_autoptr) to simplify any backport of said bugfix.
Signed-off-by: Laine Stump
---
src/qemu/qemu_domain_address.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index d872f75b38..b07672e2f4 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -803,7 +803,7 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_HOSTDEV: {
virDomainHostdevDefPtr hostdev = dev->data.hostdev;
bool isExpress = false;
-virPCIDevicePtr pciDev;
+g_autoptr(virPCIDevice) pciDev = NULL;
virPCIDeviceAddressPtr hostAddr = &hostdev->source.subsys.u.pci.addr;
if (!virHostdevIsMdevDevice(hostdev) &&
@@ -891,8 +891,6 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
off_t configLen
= virFileLength(virPCIDeviceGetConfigPath(pciDev), -1);
-virPCIDeviceFree(pciDev);
-
if (configLen == 256)
return pciFlags;
@@ -904,7 +902,6 @@
qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* a definitive answer.
*/
isExpress = virPCIDeviceIsPCIExpress(pciDev);
-virPCIDeviceFree(pciDev);
if (isExpress)
return pcieFlags;
--
2.28.0
[PATCH 3/7] util: simplify call to virPCIDeviceDetectPowerManagementReset()
This function returned an int, but would only return 0 or 1, and the
one place it was called would just use !! to convert that value to a
bool. Change the function to directly return bool instead.
Signed-off-by: Laine Stump
---
src/util/virpci.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 403ef1ec8e..31622cddfa 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -608,7 +608,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev,
int cfgfd)
* and that a D3hot->D0 transition will results in a full
* internal reset, not just a soft reset.
*/
-static unsigned int
+static bool
virPCIDeviceDetectPowerManagementReset(virPCIDevicePtr dev, int cfgfd)
{
if (dev->pci_pm_cap_pos) {
@@ -618,13 +618,13 @@ virPCIDeviceDetectPowerManagementReset(virPCIDevicePtr
dev, int cfgfd)
ctl = virPCIDeviceRead32(dev, cfgfd, dev->pci_pm_cap_pos +
PCI_PM_CTRL);
if (!(ctl & PCI_PM_CTRL_NO_SOFT_RESET)) {
VIR_DEBUG("%s %s: detected PM reset capability", dev->id,
dev->name);
-return 1;
+return true;
}
}
VIR_DEBUG("%s %s: no PM reset capability found", dev->id, dev->name);
-return 0;
+return false;
}
/* Any active devices on the same domain/bus ? */
@@ -888,7 +888,7 @@ virPCIDeviceInit(virPCIDevicePtr dev, int cfgfd)
dev->pcie_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_EXP);
dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd,
PCI_CAP_ID_PM);
dev->has_flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
-dev->has_pm_reset = !!virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
+dev->has_pm_reset = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
return 0;
}
--
2.28.0
[PATCH 0/7] fix PCI vs PCIe detection when running libvirtd as root, but unprivileged
I think there's an adequate description in patch 6 (and maybe in the bugzilla record: https://bugzilla.redhat.com/1901685). I already typed a long description once, and git send-email failed due to network problems and threw away everything I typed. I don't feel like typing it again. Laine Stump (7): qemu: use g_autoptr for a virPCIDevice util: simplify calling of virPCIDeviceDetectFunctionLevelReset() util: simplify call to virPCIDeviceDetectPowerManagementReset() util: make read error of PCI config file more detailed util: change call sequence for virPCIDeviceFindCapabilityOffset() util: make virPCIDeviceIsPCIExpress() more intelligent qemu: remove now-redundant check for file length when determining PCIe vs. PCI src/qemu/qemu_domain_address.c | 43 ++--- src/util/virpci.c | 107 - 2 files changed, 82 insertions(+), 68 deletions(-) -- 2.28.0
Re: [PATCH RFC v4 07/15] hw/riscv: PLIC update external interrupt by KVM when kvm enabled
On Thu, Dec 3, 2020 at 4:47 AM Yifei Jiang wrote:
>
> Only support supervisor external interrupt currently.
>
> Signed-off-by: Yifei Jiang
> Signed-off-by: Yipeng Yin
> ---
> hw/intc/sifive_plic.c| 31 ++-
> target/riscv/kvm.c | 19 +++
> target/riscv/kvm_riscv.h | 1 +
> 3 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index 97a1a27a9a..a419ca3a3c 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -31,6 +31,8 @@
> #include "target/riscv/cpu.h"
> #include "sysemu/sysemu.h"
> #include "migration/vmstate.h"
> +#include "sysemu/kvm.h"
> +#include "kvm_riscv.h"
>
> #define RISCV_DEBUG_PLIC 0
>
> @@ -147,15 +149,26 @@ static void sifive_plic_update(SiFivePLICState *plic)
> continue;
> }
> int level = sifive_plic_irqs_pending(plic, addrid);
> -switch (mode) {
> -case PLICMode_M:
> -riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MEIP,
> BOOL_TO_MASK(level));
> -break;
> -case PLICMode_S:
> -riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_SEIP,
> BOOL_TO_MASK(level));
> -break;
> -default:
> -break;
> +if (kvm_enabled()) {
> +if (mode == PLICMode_M) {
> +continue;
> +}
> +#ifdef CONFIG_KVM
> +kvm_riscv_set_irq(RISCV_CPU(cpu), IRQ_S_EXT, level);
> +#endif
What if kvm_enalbed() is true, but CONFIG_KVM isn't defined?
Alistair
> +} else {
> +switch (mode) {
> +case PLICMode_M:
> +riscv_cpu_update_mip(RISCV_CPU(cpu),
> + MIP_MEIP, BOOL_TO_MASK(level));
> +break;
> +case PLICMode_S:
> +riscv_cpu_update_mip(RISCV_CPU(cpu),
> + MIP_SEIP, BOOL_TO_MASK(level));
> +break;
> +default:
> +break;
> +}
> }
> }
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 6250ca0c7d..b01ff0754c 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -454,3 +454,22 @@ void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
> env->satp = 0;
> }
>
> +void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level)
> +{
> +int ret;
> +unsigned virq = level ? KVM_INTERRUPT_SET : KVM_INTERRUPT_UNSET;
> +
> +if (irq != IRQ_S_EXT) {
> +return;
> +}
> +
> +if (!kvm_enabled()) {
> +return;
> +}
> +
> +ret = kvm_vcpu_ioctl(CPU(cpu), KVM_INTERRUPT, &virq);
> +if (ret < 0) {
> +perror("Set irq failed");
> +abort();
> +}
> +}
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> index f38c82bf59..ed281bdce0 100644
> --- a/target/riscv/kvm_riscv.h
> +++ b/target/riscv/kvm_riscv.h
> @@ -20,5 +20,6 @@
> #define QEMU_KVM_RISCV_H
>
> void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
> +void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
>
> #endif
> --
> 2.19.1
>
>
Re: [PATCH RFC v4 13/15] target/riscv: Introduce dynamic time frequency for virt machine
On Thu, Dec 3, 2020 at 4:57 AM Yifei Jiang wrote:
>
> Currently, time base frequency was fixed as SIFIVE_CLINT_TIMEBASE_FREQ.
> Here introduce "time-frequency" property to set time base frequency
> dynamically
> of which default value is still SIFIVE_CLINT_TIMEBASE_FREQ. The virt machine
> uses frequency of the first cpu to create clint and fdt.
>
> Signed-off-by: Yifei Jiang
> Signed-off-by: Yipeng Yin
> ---
> hw/riscv/virt.c| 18 ++
> target/riscv/cpu.c | 3 +++
> target/riscv/cpu.h | 2 ++
> 3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 47b7018193..788a7237b6 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -178,7 +178,7 @@ static void create_pcie_irq_map(void *fdt, char *nodename,
> }
>
> static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> -uint64_t mem_size, const char *cmdline)
> +uint64_t mem_size, const char *cmdline, uint64_t timebase_frequency)
> {
> void *fdt;
> int i, cpu, socket;
> @@ -225,7 +225,7 @@ static void create_fdt(RISCVVirtState *s, const struct
> MemmapEntry *memmap,
>
> qemu_fdt_add_subnode(fdt, "/cpus");
> qemu_fdt_setprop_cell(fdt, "/cpus", "timebase-frequency",
> - SIFIVE_CLINT_TIMEBASE_FREQ);
> + timebase_frequency);
> qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> qemu_fdt_add_subnode(fdt, "/cpus/cpu-map");
> @@ -510,6 +510,7 @@ static void virt_machine_init(MachineState *machine)
> target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t fdt_load_addr;
> uint64_t kernel_entry;
> +uint64_t timebase_frequency = 0;
> DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
> int i, j, base_hartid, hart_count;
> CPUState *cs;
> @@ -553,12 +554,20 @@ static void virt_machine_init(MachineState *machine)
> hart_count, &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->soc[i]), &error_abort);
>
> +if (!timebase_frequency) {
> +timebase_frequency = RISCV_CPU(first_cpu)->env.frequency;
> +}
> +/* If vcpu's time frequency is not specified, we use default
> frequency */
> +if (!timebase_frequency) {
> +timebase_frequency = SIFIVE_CLINT_TIMEBASE_FREQ;
> +}
> +
> /* Per-socket CLINT */
> sifive_clint_create(
> memmap[VIRT_CLINT].base + i * memmap[VIRT_CLINT].size,
> memmap[VIRT_CLINT].size, base_hartid, hart_count,
> SIFIVE_SIP_BASE, SIFIVE_TIMECMP_BASE, SIFIVE_TIME_BASE,
> -SIFIVE_CLINT_TIMEBASE_FREQ, true);
> +timebase_frequency, true);
>
> /* Per-socket PLIC hart topology configuration string */
> plic_hart_config_len =
> @@ -610,7 +619,8 @@ static void virt_machine_init(MachineState *machine)
> main_mem);
>
> /* create device tree */
> -create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline);
> +create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> + timebase_frequency);
>
> /* boot rom */
> memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 439dc89ee7..66f35bcbbf 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -494,6 +494,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
>
> riscv_cpu_register_gdb_regs_for_features(cs);
>
> +env->user_frequency = env->frequency;
> +
> qemu_init_vcpu(cs);
> cpu_reset(cs);
>
> @@ -531,6 +533,7 @@ static Property riscv_cpu_properties[] = {
> DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> +DEFINE_PROP_UINT64("time-frequency", RISCVCPU, env.frequency, 0),
Why not set the default to SIFIVE_CLINT_TIMEBASE_FREQ?
Also, QEMU now has a clock API, is using that instead a better option?
Alistair
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 16d6050ead..f5b6c34176 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -243,6 +243,8 @@ struct CPURISCVState {
> uint64_t kvm_timer_time;
> uint64_t kvm_timer_compare;
> uint64_t kvm_timer_state;
> +uint64_t user_frequency;
> +uint64_t frequency;
> };
>
> OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
> --
> 2.19.1
>
>
Re: [PATCH RFC v4 09/15] target/riscv: Add host cpu type
On Thu, Dec 3, 2020 at 4:55 AM Yifei Jiang wrote:
>
> Currently, host cpu is inherited simply.
>
> Signed-off-by: Yifei Jiang
> Signed-off-by: Yipeng Yin
> ---
> target/riscv/cpu.c | 6 ++
> target/riscv/cpu.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index faee98a58c..439dc89ee7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -186,6 +186,10 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>
> #endif
>
> +static void riscv_host_cpu_init(Object *obj)
> +{
> +}
> +
> static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
> {
> ObjectClass *oc;
> @@ -641,10 +645,12 @@ static const TypeInfo riscv_cpu_type_infos[] = {
> DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31, rvxx_sifive_e_cpu_init),
> DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34, rv32_imafcu_nommu_cpu_init),
> DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34, rvxx_sifive_u_cpu_init),
> +DEFINE_CPU(TYPE_RISCV_CPU_HOST, riscv_host_cpu_init),
> #elif defined(TARGET_RISCV64)
> DEFINE_CPU(TYPE_RISCV_CPU_BASE64, riscv_base_cpu_init),
> DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51, rvxx_sifive_e_cpu_init),
> DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54, rvxx_sifive_u_cpu_init),
> +DEFINE_CPU(TYPE_RISCV_CPU_HOST, riscv_host_cpu_init),
Shouldn't this only be included if KVM is configured? Also it should
be shared between RV32 and RV64.
Alistair
> #endif
> };
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ad1c90f798..4288898019 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -43,6 +43,7 @@
> #define TYPE_RISCV_CPU_SIFIVE_E51 RISCV_CPU_TYPE_NAME("sifive-e51")
> #define TYPE_RISCV_CPU_SIFIVE_U34 RISCV_CPU_TYPE_NAME("sifive-u34")
> #define TYPE_RISCV_CPU_SIFIVE_U54 RISCV_CPU_TYPE_NAME("sifive-u54")
> +#define TYPE_RISCV_CPU_HOST RISCV_CPU_TYPE_NAME("host")
>
> #define RV32 ((target_ulong)1 << (TARGET_LONG_BITS - 2))
> #define RV64 ((target_ulong)2 << (TARGET_LONG_BITS - 2))
> --
> 2.19.1
>
>
[PATCH v3 12/14] domain_conf: move virDomainPCIControllerOpts checks to domain_validate.c
virDomainControllerDefParseXML() does a lot of checks with
virDomainPCIControllerOpts parameters that can be moved to
virDomainControllerDefValidate, sharing the logic with other use
cases that does not rely on XML parsing.
'pseries-default-phb-numa-node' parse error was changed to reflect
the error that is being thrown by qemuValidateDomainDeviceDefController()
via deviceValidateCallback, that is executed before
virDomainControllerDefValidate().
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c| 42 +---
src/conf/domain_validate.c| 48 +++
.../pseries-default-phb-numa-node.err | 2 +-
tests/qemuxml2argvtest.c | 6 ++-
4 files changed, 56 insertions(+), 42 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 453dc6cf6a..7adf2700ae 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10864,14 +10864,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
chassisNr);
return NULL;
}
-if (def->opts.pciopts.chassisNr < 1 ||
-def->opts.pciopts.chassisNr > 255) {
-virReportError(VIR_ERR_XML_ERROR,
- _("PCI controller chassisNr '%s' out of range "
- "- must be 1-255"),
- chassisNr);
-return NULL;
-}
}
if (chassis) {
if (virStrToLong_i(chassis, NULL, 0,
@@ -10881,14 +10873,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
chassis);
return NULL;
}
-if (def->opts.pciopts.chassis < 0 ||
-def->opts.pciopts.chassis > 255) {
-virReportError(VIR_ERR_XML_ERROR,
- _("PCI controller chassis '%s' out of range "
- "- must be 0-255"),
- chassis);
-return NULL;
-}
}
if (port) {
if (virStrToLong_i(port, NULL, 0,
@@ -10898,14 +10882,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
port);
return NULL;
}
-if (def->opts.pciopts.port < 0 ||
-def->opts.pciopts.port > 255) {
-virReportError(VIR_ERR_XML_ERROR,
- _("PCI controller port '%s' out of range "
- "- must be 0-255"),
- port);
-return NULL;
-}
}
if (busNr) {
if (virStrToLong_i(busNr, NULL, 0,
@@ -10915,14 +10891,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
busNr);
return NULL;
}
-if (def->opts.pciopts.busNr < 1 ||
-def->opts.pciopts.busNr > 254) {
-virReportError(VIR_ERR_XML_ERROR,
- _("PCI controller busNr '%s' out of range "
- "- must be 1-254"),
- busNr);
-return NULL;
-}
}
if (targetIndex) {
if (virStrToLong_i(targetIndex, NULL, 0,
@@ -10934,15 +10902,9 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
return NULL;
}
}
-if (numaNode >= 0) {
-if (def->idx == 0) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("The PCI controller with index=0 can't "
- "be associated with a NUMA node"));
-return NULL;
-}
+if (numaNode >= 0)
def->opts.pciopts.numaNode = numaNode;
-}
+
if (hotplug) {
int val = virTristateSwitchTypeFromString(hotplug);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 416c24f97b..f47e80ca38 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -551,6 +551,54 @@ virDomainControllerDefValidate(const
virDomainControllerDef *controller)
return -1;
}
}
+
+if (opts->chassisNr != -1) {
+if (opts->chassisNr < 1 || opts->chassisNr > 255) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("PCI controller chassisNr '%d' out of range "
+ "- must be 1-255"),
+ opts->chassisNr);
+return -1;
+}
+}
+
+if (opts->chassis != -1) {
+if (opts->chassis < 0 || opts->chassis > 255) {
+virReportError(VIR_ERR_CONF
[PATCH v3 10/14] domain_conf.c: move blkio path check to domain_validate.c
Move this check to a new virDomainDefTunablesValidate(), which
is called by virDomainDefValidateInternal().
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 14 --
src/conf/domain_validate.c | 21 +
src/conf/domain_validate.h | 1 +
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a5dcb0bbce..064d77d933 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6981,6 +6981,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefVideoValidate(def) < 0)
return -1;
+if (virDomainDefTunablesValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -20875,7 +20878,7 @@ virDomainDefTunablesParse(virDomainDefPtr def,
unsigned int flags)
{
g_autofree xmlNodePtr *nodes = NULL;
-size_t i, j;
+size_t i;
int n;
/* Extract blkio cgroup tunables */
@@ -20896,15 +20899,6 @@ virDomainDefTunablesParse(virDomainDefPtr def,
&def->blkio.devices[i]) < 0)
return -1;
def->blkio.ndevices++;
-for (j = 0; j < i; j++) {
-if (STREQ(def->blkio.devices[j].path,
- def->blkio.devices[i].path)) {
-virReportError(VIR_ERR_XML_ERROR,
- _("duplicate blkio device path '%s'"),
- def->blkio.devices[i].path);
-return -1;
-}
-}
}
VIR_FREE(nodes);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 6fca604d17..09ab908ea3 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -496,3 +496,24 @@ virDomainSmartcardDefValidate(const virDomainSmartcardDef
*smartcard,
return 0;
}
+
+
+int
+virDomainDefTunablesValidate(const virDomainDef *def)
+{
+size_t i, j;
+
+for (i = 0; i < def->blkio.ndevices; i++) {
+for (j = 0; j < i; j++) {
+if (STREQ(def->blkio.devices[j].path,
+ def->blkio.devices[i].path)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("duplicate blkio device path '%s'"),
+ def->blkio.devices[i].path);
+return -1;
+}
+}
+}
+
+return 0;
+}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index d65de50422..2bd9e71073 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -42,3 +42,4 @@ int virDomainRNGDefValidate(const virDomainRNGDef *rng,
const virDomainDef *def);
int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard,
const virDomainDef *def);
+int virDomainDefTunablesValidate(const virDomainDef *def);
--
2.26.2
[PATCH v3 14/14] domain_conf.c: move idmapEntry checks to domain_validate.c
Create a new function called virDomainDefIdMapValidate() and
use it to move these checks out of virDomainIdmapDefParseXML()
and virDomainDefParseXML().
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 19 +++
src/conf/domain_validate.c | 23 +++
src/conf/domain_validate.h | 1 +
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7a7c9ec85c..44f136f530 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6947,6 +6947,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefTunablesValidate(def) < 0)
return -1;
+if (virDomainDefIdMapValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -18463,15 +18466,6 @@ virDomainIdmapDefParseXML(xmlXPathContextPtr ctxt,
qsort(idmap, num, sizeof(idmap[0]), virDomainIdMapEntrySort);
-if (idmap[0].start != 0) {
-/* Root user of container hasn't been mapped to any user of host,
- * return error. */
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("You must map the root user of container"));
-VIR_FREE(idmap);
-return NULL;
-}
-
return idmap;
}
@@ -21961,13 +21955,6 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
-if ((def->idmap.uidmap && !def->idmap.gidmap) ||
-(!def->idmap.uidmap && def->idmap.gidmap)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("uid and gid should be mapped both"));
-goto error;
-}
-
if ((n = virXPathNodeSet("./sysinfo", ctxt, &nodes)) < 0)
goto error;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 35deb9f017..0eed1ba982 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -612,3 +612,26 @@ virDomainControllerDefValidate(const
virDomainControllerDef *controller)
return 0;
}
+
+
+int
+virDomainDefIdMapValidate(const virDomainDef *def)
+{
+if ((def->idmap.uidmap && !def->idmap.gidmap) ||
+(!def->idmap.uidmap && def->idmap.gidmap)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("uid and gid should be mapped both"));
+return -1;
+}
+
+if ((def->idmap.uidmap && def->idmap.uidmap[0].start != 0) ||
+(def->idmap.gidmap && def->idmap.gidmap[0].start != 0)) {
+/* Root user of container hasn't been mapped to any user of host,
+ * return error. */
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("You must map the root user of container"));
+return -1;
+}
+
+return 0;
+}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index e8004e358d..497a02b9b3 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -44,3 +44,4 @@ int virDomainSmartcardDefValidate(const virDomainSmartcardDef
*smartcard,
const virDomainDef *def);
int virDomainDefTunablesValidate(const virDomainDef *def);
int virDomainControllerDefValidate(const virDomainControllerDef *controller);
+int virDomainDefIdMapValidate(const virDomainDef *def);
--
2.26.2
[PATCH v3 13/14] domain_conf: move pci-root/pcie-root address check to domain_validate.c
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 6 --
src/conf/domain_validate.c | 10 ++
tests/qemuxml2argvdata/pci-root-address.err | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7adf2700ae..7a7c9ec85c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10820,12 +10820,6 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr
xmlopt,
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: {
unsigned long long bytes;
-if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("pci-root and pcie-root controllers should
not "
- "have an address"));
-return NULL;
-}
if ((rc = virParseScaledValue("./pcihole64", NULL,
ctxt, &bytes, 1024,
1024ULL * ULONG_MAX, false)) < 0)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index f47e80ca38..35deb9f017 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -525,6 +525,16 @@ virDomainControllerDefValidate(const
virDomainControllerDef *controller)
if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
const virDomainPCIControllerOpts *opts = &controller->opts.pciopts;
+if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
+controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
+if (controller->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("pci-root and pcie-root controllers "
+ "should not have an address"));
+return -1;
+}
+}
+
if (controller->idx > 255) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("PCI controller index %d too high, maximum is
255"),
diff --git a/tests/qemuxml2argvdata/pci-root-address.err
b/tests/qemuxml2argvdata/pci-root-address.err
index 53dad81985..ffe5438224 100644
--- a/tests/qemuxml2argvdata/pci-root-address.err
+++ b/tests/qemuxml2argvdata/pci-root-address.err
@@ -1 +1 @@
-XML error: pci-root and pcie-root controllers should not have an address
+unsupported configuration: pci-root and pcie-root controllers should not have
an address
--
2.26.2
[PATCH v3 11/14] domain_conf.c: move virDomainControllerDefValidate() to domain_validate.c
Next patch will add more validations to this function. Let's move
it to domain_validate.c beforehand.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 37 -
src/conf/domain_validate.c | 37 +
src/conf/domain_validate.h | 1 +
3 files changed, 38 insertions(+), 37 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 064d77d933..453dc6cf6a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6211,43 +6211,6 @@ virDomainNetDefValidate(const virDomainNetDef *net)
}
-static int
-virDomainControllerDefValidate(const virDomainControllerDef *controller)
-{
-if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
-const virDomainPCIControllerOpts *opts = &controller->opts.pciopts;
-
-if (controller->idx > 255) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("PCI controller index %d too high, maximum is
255"),
- controller->idx);
-return -1;
-}
-
-/* Only validate the target index if it's been set */
-if (opts->targetIndex != -1) {
-
-if (opts->targetIndex < 0 || opts->targetIndex > 30) {
-virReportError(VIR_ERR_XML_ERROR,
- _("PCI controller target index '%d' out of "
- "range - must be 0-30"),
- opts->targetIndex);
-return -1;
-}
-
-if ((controller->idx == 0 && opts->targetIndex != 0) ||
-(controller->idx != 0 && opts->targetIndex == 0)) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Only the PCI controller with index 0 can "
- "have target index 0, and vice versa"));
-return -1;
-}
-}
-}
-return 0;
-}
-
-
static int
virDomainHostdevDefValidate(const virDomainHostdevDef *hostdev)
{
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 09ab908ea3..416c24f97b 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -517,3 +517,40 @@ virDomainDefTunablesValidate(const virDomainDef *def)
return 0;
}
+
+
+int
+virDomainControllerDefValidate(const virDomainControllerDef *controller)
+{
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
+const virDomainPCIControllerOpts *opts = &controller->opts.pciopts;
+
+if (controller->idx > 255) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("PCI controller index %d too high, maximum is
255"),
+ controller->idx);
+return -1;
+}
+
+/* Only validate the target index if it's been set */
+if (opts->targetIndex != -1) {
+
+if (opts->targetIndex < 0 || opts->targetIndex > 30) {
+virReportError(VIR_ERR_XML_ERROR,
+ _("PCI controller target index '%d' out of "
+ "range - must be 0-30"),
+ opts->targetIndex);
+return -1;
+}
+
+if ((controller->idx == 0 && opts->targetIndex != 0) ||
+(controller->idx != 0 && opts->targetIndex == 0)) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("Only the PCI controller with index 0 can "
+ "have target index 0, and vice versa"));
+return -1;
+}
+}
+}
+return 0;
+}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index 2bd9e71073..e8004e358d 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -43,3 +43,4 @@ int virDomainRNGDefValidate(const virDomainRNGDef *rng,
int virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard,
const virDomainDef *def);
int virDomainDefTunablesValidate(const virDomainDef *def);
+int virDomainControllerDefValidate(const virDomainControllerDef *controller);
--
2.26.2
[PATCH v3 07/14] domain_validate.c: rename virSecurityDeviceLabelDefValidateXML()
The function isn't doing XML validation of any sort. Rename it to
be compatible with its actual use.
While we're at it, change the VIR_ERR_XML_ERROR error being thrown
in the function to VIR_ERR_CONFIG_UNSUPPORTED.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 8
src/conf/domain_validate.c | 18 +-
src/conf/domain_validate.h | 8
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 220bb8cdcf..6578055119 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6164,10 +6164,10 @@ virDomainChrSourceDefValidate(const
virDomainChrSourceDef *src_def,
break;
}
-if (virSecurityDeviceLabelDefValidateXML(src_def->seclabels,
- src_def->nseclabels,
- def->seclabels,
- def->nseclabels) < 0)
+if (virSecurityDeviceLabelDefValidate(src_def->seclabels,
+ src_def->nseclabels,
+ def->seclabels,
+ def->nseclabels) < 0)
return -1;
return 0;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index bc5ca3f668..e9427cffa8 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -195,10 +195,10 @@ virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus
bus,
int
-virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
- size_t nseclabels,
- virSecurityLabelDefPtr *vmSeclabels,
- size_t nvmSeclabels)
+virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels,
+ size_t nseclabels,
+ virSecurityLabelDefPtr *vmSeclabels,
+ size_t nvmSeclabels)
{
virSecurityDeviceLabelDefPtr seclabel;
size_t i;
@@ -213,7 +213,7 @@
virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
continue;
if (!vmSeclabels[j]->relabel) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("label overrides require relabeling to be "
"enabled at the domain level"));
return -1;
@@ -297,10 +297,10 @@ virDomainDiskDefValidate(const virDomainDef *def,
}
for (next = disk->src; next; next = next->backingStore) {
-if (virSecurityDeviceLabelDefValidateXML(next->seclabels,
- next->nseclabels,
- def->seclabels,
- def->nseclabels) < 0)
+if (virSecurityDeviceLabelDefValidate(next->seclabels,
+ next->nseclabels,
+ def->seclabels,
+ def->nseclabels) < 0)
return -1;
}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index fe7c752e8c..aef169a4c9 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -28,9 +28,9 @@ int virDomainDefBootValidate(const virDomainDef *def);
int virDomainDefVideoValidate(const virDomainDef *def);
int virDomainVideoDefValidate(const virDomainVideoDef *video,
const virDomainDef *def);
-int virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr
*seclabels,
- size_t nseclabels,
- virSecurityLabelDefPtr *vmSeclabels,
- size_t nvmSeclabels);
+int virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDefPtr *seclabels,
+ size_t nseclabels,
+ virSecurityLabelDefPtr *vmSeclabels,
+ size_t nvmSeclabels);
int virDomainDiskDefValidate(const virDomainDef *def,
const virDomainDiskDef *disk);
--
2.26.2
[PATCH v3 09/14] domain_conf.c: move smartcard address check to domain_validate.c
This check is not tied to XML parsing and can be moved to
virDomainSmartcardDefValidate().
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 7 ---
src/conf/domain_validate.c | 7 +++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a301ab5c74..a5dcb0bbce 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13222,13 +13222,6 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr
xmlopt,
if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
return NULL;
-if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
-def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Controllers must use the 'ccid' address type"));
-return NULL;
-}
-
return g_steal_pointer(&def);
}
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 882fbdac57..6fca604d17 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -484,6 +484,13 @@ int
virDomainSmartcardDefValidate(const virDomainSmartcardDef *smartcard,
const virDomainDef *def)
{
+if (smartcard->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+smartcard->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Controllers must use the 'ccid' address type"));
+return -1;
+}
+
if (smartcard->type == VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH)
return virDomainChrSourceDefValidate(smartcard->data.passthru, NULL,
def);
--
2.26.2
[PATCH v3 08/14] domain_conf: move all ChrSource checks to domain_validate.c
Next patch will move a validation to virDomainSmartcardDefValidate(),
but this function can't be moved alone to domain_validate.c without
making virDomainChrSourceDefValidate(), from domain_conf.c, public.
Given that the idea is to eventually move all validations to domain_validate.c
anyways, let's move all ChrSource related validations in a single punch.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 161 -
src/conf/domain_validate.c | 161 +
src/conf/domain_validate.h | 8 ++
3 files changed, 169 insertions(+), 161 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6578055119..a301ab5c74 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6058,137 +6058,6 @@ virDomainDefHasUSB(const virDomainDef *def)
}
-#define SERIAL_CHANNEL_NAME_CHARS \
-"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-."
-
-
-static int
-virDomainChrSourceDefValidate(const virDomainChrSourceDef *src_def,
- const virDomainChrDef *chr_def,
- const virDomainDef *def)
-{
-switch ((virDomainChrType) src_def->type) {
-case VIR_DOMAIN_CHR_TYPE_NULL:
-case VIR_DOMAIN_CHR_TYPE_PTY:
-case VIR_DOMAIN_CHR_TYPE_VC:
-case VIR_DOMAIN_CHR_TYPE_STDIO:
-case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
-case VIR_DOMAIN_CHR_TYPE_LAST:
-break;
-
-case VIR_DOMAIN_CHR_TYPE_FILE:
-case VIR_DOMAIN_CHR_TYPE_DEV:
-case VIR_DOMAIN_CHR_TYPE_PIPE:
-if (!src_def->data.file.path) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing source path attribute for char device"));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CHR_TYPE_NMDM:
-if (!src_def->data.nmdm.master) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing master path attribute for nmdm device"));
-return -1;
-}
-
-if (!src_def->data.nmdm.slave) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing slave path attribute for nmdm device"));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CHR_TYPE_TCP:
-if (!src_def->data.tcp.host) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing source host attribute for char device"));
-return -1;
-}
-
-if (!src_def->data.tcp.service) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing source service attribute for char
device"));
-return -1;
-}
-
-if (src_def->data.tcp.listen && src_def->data.tcp.reconnect.enabled) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("chardev reconnect is possible only for connect
mode"));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CHR_TYPE_UDP:
-if (!src_def->data.udp.connectService) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing source service attribute for char
device"));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CHR_TYPE_UNIX:
-/* The source path can be auto generated for certain specific
- * types of channels, but in most cases we should report an
- * error if the user didn't provide it */
-if (!src_def->data.nix.path &&
-!(chr_def &&
- chr_def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
- (chr_def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN ||
- chr_def->targetType ==
VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO))) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Missing source path attribute for char device"));
-return -1;
-}
-
-if (src_def->data.nix.listen && src_def->data.nix.reconnect.enabled) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("chardev reconnect is possible only for connect
mode"));
-return -1;
-}
-break;
-
-case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
-if (!src_def->data.spiceport.channel) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Missing source channel attribute for char
device"));
-return -1;
-}
-if (strspn(src_def->data.spiceport.channel,
- SERIAL_CHANNEL_NAME_CHARS) <
strlen(src_def->data.spiceport.channel)) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
- _("Invalid character in source channel for char
device"));
-return -1;
-}
-break;
-}
-
-if (virSecurityDeviceLabelDefValidate(src_def->seclabels
[PATCH v3 04/14] domain_conf.c: move QXL attributes check to virDomainVideoDefValidate()
These checks are not related to XML parsing and can be moved to the
validate callback. Errors were changed from VIR_ERR_XML_ERROR to
VIR_ERR_CONFIG_UNSUPPORTED.
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 15 ---
src/conf/domain_validate.c | 20
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d80bc8495e..db0ca975fe 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16132,11 +16132,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
if (ram) {
-if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("ram attribute only supported for type of qxl"));
-return NULL;
-}
if (virStrToLong_uip(ram, NULL, 10, &def->ram) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("cannot parse video ram '%s'"), ram);
@@ -16153,11 +16148,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
if (vram64) {
-if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("vram64 attribute only supported for type of
qxl"));
-return NULL;
-}
if (virStrToLong_uip(vram64, NULL, 10, &def->vram64) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("cannot parse video vram64 '%s'"), vram64);
@@ -16166,11 +16156,6 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
if (vgamem) {
-if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("vgamem attribute only supported for type of
qxl"));
-return NULL;
-}
if (virStrToLong_uip(vgamem, NULL, 10, &def->vgamem) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("cannot parse video vgamem '%s'"), vgamem);
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index c81fd296b9..234eb72f11 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -128,5 +128,25 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
return -1;
}
+if (video->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
+if (video->ram != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("ram attribute only supported for video type
qxl"));
+return -1;
+}
+
+if (video->vram64 != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("vram64 attribute only supported for video type
qxl"));
+return -1;
+}
+
+if (video->vgamem != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("vgamem attribute only supported for video type
qxl"));
+return -1;
+}
+}
+
return 0;
}
--
2.26.2
[PATCH v3 06/14] domain_conf: move vendor, product and tray checks to domain_validate.c
The 'tray' check isn't a XML parse specific code and can be pushed
to the validate callback, in virDomainDiskDefValidate().
'vendor' and 'product' string sizes are already checked by the
domaincommon.rng schema, but can be of use in the validate callback
since not all scenarios will go through the XML parsing.
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 22 --
src/conf/domain_validate.c | 25 +
2 files changed, 25 insertions(+), 22 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5aeb75ce59..220bb8cdcf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10222,9 +10222,6 @@ virDomainDiskDefParsePrivateData(xmlXPathContextPtr
ctxt,
}
-#define VENDOR_LEN 8
-#define PRODUCT_LEN 16
-
static virDomainDiskDefPtr
virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
xmlNodePtr node,
@@ -10399,12 +10396,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
if (!(vendor = virXMLNodeContentString(cur)))
return NULL;
-if (strlen(vendor) > VENDOR_LEN) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk vendor is more than 8 characters"));
-return NULL;
-}
-
if (!virStringIsPrintable(vendor)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("disk vendor is not printable string"));
@@ -10415,12 +10406,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
if (!(product = virXMLNodeContentString(cur)))
return NULL;
-if (strlen(product) > PRODUCT_LEN) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("disk product is more than 16 characters"));
-return NULL;
-}
-
if (!virStringIsPrintable(product)) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("disk product is not printable string"));
@@ -10551,13 +10536,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
_("unknown disk tray status '%s'"), tray);
return NULL;
}
-
-if (def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
-def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("tray is only valid for cdrom and floppy"));
-return NULL;
-}
}
if (removable) {
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index da36bef31a..bc5ca3f668 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -225,6 +225,9 @@
virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
}
+#define VENDOR_LEN 8
+#define PRODUCT_LEN 16
+
int
virDomainDiskDefValidate(const virDomainDef *def,
const virDomainDiskDef *disk)
@@ -301,5 +304,27 @@ virDomainDiskDefValidate(const virDomainDef *def,
return -1;
}
+if (disk->tray_status &&
+disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
+disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("tray is only valid for cdrom and floppy"));
+return -1;
+}
+
+if (disk->vendor && strlen(disk->vendor) > VENDOR_LEN) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk vendor is more than %d characters"),
+ VENDOR_LEN);
+return -1;
+}
+
+if (disk->product && strlen(disk->product) > PRODUCT_LEN) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk product is more than %d characters"),
+ PRODUCT_LEN);
+return -1;
+}
+
return 0;
}
--
2.26.2
[PATCH v3 03/14] domain_conf.c: move virDomainVideoDefValidate() to domain_validate.c
We'll add more video validations into the function in the next
patch. Let's move it beforehand to domain_validate.c.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 57 --
src/conf/domain_validate.c | 57 ++
src/conf/domain_validate.h | 2 ++
3 files changed, 59 insertions(+), 57 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a8bd54a368..d80bc8495e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6612,63 +6612,6 @@ virDomainHostdevDefValidate(const virDomainHostdevDef
*hostdev)
}
-static int
-virDomainVideoDefValidate(const virDomainVideoDef *video,
- const virDomainDef *def)
-{
-size_t i;
-
-if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing video model and cannot determine default"));
-return -1;
-}
-
-/* it doesn't make sense to pair video device type 'none' with any other
- * types, there can be only a single video device in such case
- */
-for (i = 0; i < def->nvideos; i++) {
-if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
-def->nvideos > 1) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("a 'none' video type must be the only video
device "
- "defined for the domain"));
-return -1;
-}
-}
-
-switch (video->backend) {
-case VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER:
-if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("'vhostuser' driver is only supported with
'virtio' device"));
-return -1;
-}
-break;
-case VIR_DOMAIN_VIDEO_BACKEND_TYPE_DEFAULT:
-case VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU:
-if (video->accel && video->accel->rendernode) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("unsupported rendernode accel attribute without
'vhostuser'"));
-return -1;
-}
-break;
-case VIR_DOMAIN_VIDEO_BACKEND_TYPE_LAST:
-default:
-virReportEnumRangeError(virDomainInputType, video->backend);
-return -1;
-}
-
-if (video->res && (video->res->x == 0 || video->res->y == 0)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("video resolution values must be greater than 0"));
-return -1;
-}
-
-return 0;
-}
-
-
static int
virDomainMemoryDefValidate(const virDomainMemoryDef *mem,
const virDomainDef *def)
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index eb2ef6c7fb..c81fd296b9 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -73,3 +73,60 @@ virDomainDefVideoValidate(const virDomainDef *def)
return 0;
}
+
+
+int
+virDomainVideoDefValidate(const virDomainVideoDef *video,
+ const virDomainDef *def)
+{
+size_t i;
+
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing video model and cannot determine default"));
+return -1;
+}
+
+/* it doesn't make sense to pair video device type 'none' with any other
+ * types, there can be only a single video device in such case
+ */
+for (i = 0; i < def->nvideos; i++) {
+if (def->videos[i]->type == VIR_DOMAIN_VIDEO_TYPE_NONE &&
+def->nvideos > 1) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("a 'none' video type must be the only video
device "
+ "defined for the domain"));
+return -1;
+}
+}
+
+switch (video->backend) {
+case VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER:
+if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("'vhostuser' driver is only supported with
'virtio' device"));
+return -1;
+}
+break;
+case VIR_DOMAIN_VIDEO_BACKEND_TYPE_DEFAULT:
+case VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU:
+if (video->accel && video->accel->rendernode) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupported rendernode accel attribute without
'vhostuser'"));
+return -1;
+}
+break;
+case VIR_DOMAIN_VIDEO_BACKEND_TYPE_LAST:
+default:
+virReportEnumRangeError(virDomainInputType, video->backend);
+return -1;
+}
+
+if (video->res && (video->res->x == 0 || video->res->y == 0)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("video reso
[PATCH v3 05/14] domain_conf: move virDomainDiskDefValidate() to domain_validate.c
Next patch will add more validations to the function. Let's move
it beforehand to domain_validate.c.
virSecurityDeviceLabelDefValidateXML() is still used inside
domain_conf.c, so make it public for now until its current
caller (virDomainChrSourceDefValidate()) is also moved to
domain_validate.c.
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c | 152
src/conf/domain_validate.c | 153 +
src/conf/domain_validate.h | 6 ++
3 files changed, 159 insertions(+), 152 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index db0ca975fe..5aeb75ce59 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6043,158 +6043,6 @@ virDomainDefPostParse(virDomainDefPtr def,
}
-/**
- * virDomainDiskAddressDiskBusCompatibility:
- * @bus: disk bus type
- * @addressType: disk address type
- *
- * Check if the specified disk address type @addressType is compatible
- * with the specified disk bus type @bus. This function checks
- * compatibility with the bus types SATA, SCSI, FDC, and IDE only,
- * because only these are handled in common code.
- *
- * Returns true if compatible or can't be decided in common code,
- * false if known to be not compatible.
- */
-static bool
-virDomainDiskAddressDiskBusCompatibility(virDomainDiskBus bus,
- virDomainDeviceAddressType
addressType)
-{
-if (addressType == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
-return true;
-
-switch (bus) {
-case VIR_DOMAIN_DISK_BUS_IDE:
-case VIR_DOMAIN_DISK_BUS_FDC:
-case VIR_DOMAIN_DISK_BUS_SCSI:
-case VIR_DOMAIN_DISK_BUS_SATA:
-return addressType == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
-case VIR_DOMAIN_DISK_BUS_VIRTIO:
-case VIR_DOMAIN_DISK_BUS_XEN:
-case VIR_DOMAIN_DISK_BUS_USB:
-case VIR_DOMAIN_DISK_BUS_UML:
-case VIR_DOMAIN_DISK_BUS_SD:
-case VIR_DOMAIN_DISK_BUS_LAST:
-return true;
-}
-
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unexpected bus type '%d'"),
- bus);
-return true;
-}
-
-
-static int
-virSecurityDeviceLabelDefValidateXML(virSecurityDeviceLabelDefPtr *seclabels,
- size_t nseclabels,
- virSecurityLabelDefPtr *vmSeclabels,
- size_t nvmSeclabels)
-{
-virSecurityDeviceLabelDefPtr seclabel;
-size_t i;
-size_t j;
-
-for (i = 0; i < nseclabels; i++) {
-seclabel = seclabels[i];
-
-/* find the security label that it's being overridden */
-for (j = 0; j < nvmSeclabels; j++) {
-if (STRNEQ_NULLABLE(vmSeclabels[j]->model, seclabel->model))
-continue;
-
-if (!vmSeclabels[j]->relabel) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
- _("label overrides require relabeling to be "
- "enabled at the domain level"));
-return -1;
-}
-}
-}
-
-return 0;
-}
-
-
-static int
-virDomainDiskDefValidate(const virDomainDef *def,
- const virDomainDiskDef *disk)
-{
-virStorageSourcePtr next;
-
-/* Validate LUN configuration */
-if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
-/* volumes haven't been translated at this point, so accept them */
-if (!(disk->src->type == VIR_STORAGE_TYPE_BLOCK ||
- disk->src->type == VIR_STORAGE_TYPE_VOLUME ||
- (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
- disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI))) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk '%s' improperly configured for a "
- "device='lun'"), disk->dst);
-return -1;
-}
-}
-
-if (disk->src->pr &&
-disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _(" allowed only for lun devices"));
-return -1;
-}
-
-/* Reject disks with a bus type that is not compatible with the
- * given address type. The function considers only buses that are
- * handled in common code. For other bus types it's not possible
- * to decide compatibility in common code.
- */
-if (!virDomainDiskAddressDiskBusCompatibility(disk->bus, disk->info.type))
{
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Invalid address type '%s' for the disk '%s' with the
bus type '%s'"),
- virDomainDeviceAddressTypeToString(disk->info.type),
- disk->dst,
- virDomainDiskBusTypeToString(disk->bus));
-return -1;
-}
-
-if (disk->queues && disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
-virReportErro
[PATCH v3 01/14] domain_conf: move boot timeouts check to domain_validate.c
This patch creates a new function, virDomainDefBootValidate(), to host
the validation of boot menu timeout and rebootTimeout outside of parse
time. The checks in virDomainDefParseBootXML() were changed to throw
VIR_ERR_XML_ERROR in case of parse error of those values.
In an attempt to alleviate the amount of code being stacked inside
domain_conf.c, let's put this new function in a new domain_validate.c
file that will be used to place these validations.
Suggested-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
po/POTFILES.in | 1 +
src/conf/domain_conf.c | 20 +++
src/conf/domain_validate.c | 51 ++
src/conf/domain_validate.h | 27
src/conf/meson.build | 1 +
tests/qemuxml2argvtest.c | 3 ++-
6 files changed, 92 insertions(+), 11 deletions(-)
create mode 100644 src/conf/domain_validate.c
create mode 100644 src/conf/domain_validate.h
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 9782b2beb4..14636d4b93 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -26,6 +26,7 @@
@SRCDIR@src/conf/domain_capabilities.c
@SRCDIR@src/conf/domain_conf.c
@SRCDIR@src/conf/domain_event.c
+@SRCDIR@src/conf/domain_validate.c
@SRCDIR@src/conf/interface_conf.c
@SRCDIR@src/conf/netdev_bandwidth_conf.c
@SRCDIR@src/conf/netdev_vlan_conf.c
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66ee658e7b..4a45c76cf1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -33,6 +33,7 @@
#include "datatypes.h"
#include "domain_addr.h"
#include "domain_conf.h"
+#include "domain_validate.h"
#include "snapshot_conf.h"
#include "viralloc.h"
#include "virxml.h"
@@ -7344,6 +7345,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefCputuneValidate(def) < 0)
return -1;
+if (virDomainDefBootValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -18867,11 +18871,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
tmp = virXMLPropString(node, "timeout");
if (tmp && def->os.bootmenu == VIR_TRISTATE_BOOL_YES) {
-if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0 ||
-def->os.bm_timeout > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for boot menu timeout, "
- "must be in range [0,65535]"));
+if (virStrToLong_uip(tmp, NULL, 0, &def->os.bm_timeout) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for boot menu timeout"));
return -1;
}
def->os.bm_timeout_set = true;
@@ -18892,11 +18894,9 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
if (tmp) {
/* that was really just for the check if it is there */
-if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0 ||
-def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("invalid value for rebootTimeout, "
- "must be in range [-1,65535]"));
+if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for rebootTimeout"));
return -1;
}
def->os.bios.rt_set = true;
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
new file mode 100644
index 00..e4d947c553
--- /dev/null
+++ b/src/conf/domain_validate.c
@@ -0,0 +1,51 @@
+/*
+ * domain_validate.c: domain general validation functions
+ *
+ * Copyright IBM Corp, 2020
+ *
+ * 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
+ *
[PATCH v3 02/14] domain_conf.c: move primary video check to validate callback
This check isn't exclusive to XML parsing. Let's move it to
virDomainDefVideoValidate() in domain_validate.c
We don't have a failure test for this scenario, so a new test called
'video-multiple-primaries' was added to test this failure case.
Reviewed-by: Michal Privoznik
Signed-off-by: Daniel Henrique Barboza
---
src/conf/domain_conf.c| 10 +++---
src/conf/domain_validate.c| 24 ++
src/conf/domain_validate.h| 1 +
.../video-multiple-primaries.err | 1 +
.../video-multiple-primaries.xml | 32 +++
tests/qemuxml2argvtest.c | 5 +++
6 files changed, 67 insertions(+), 6 deletions(-)
create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err
create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 4a45c76cf1..a8bd54a368 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7348,6 +7348,9 @@ virDomainDefValidateInternal(const virDomainDef *def,
if (virDomainDefBootValidate(def) < 0)
return -1;
+if (virDomainDefVideoValidate(def) < 0)
+return -1;
+
if (virDomainNumaDefValidate(def->numa) < 0)
return -1;
@@ -22141,14 +22144,9 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
if (video->primary) {
-if (def->nvideos != 0 && def->videos[0]->primary) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Only one primary video device is
supported"));
-goto error;
-}
-
insertAt = 0;
}
+
if (VIR_INSERT_ELEMENT_INPLACE(def->videos,
insertAt,
def->nvideos,
diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index e4d947c553..eb2ef6c7fb 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -49,3 +49,27 @@ virDomainDefBootValidate(const virDomainDef *def)
return 0;
}
+
+
+int
+virDomainDefVideoValidate(const virDomainDef *def)
+{
+size_t i;
+
+if (def->nvideos == 0)
+return 0;
+
+/* Any video marked as primary will be put in index 0 by the
+ * parser. Ensure that we have only one primary set by the user. */
+if (def->videos[0]->primary) {
+for (i = 1; i < def->nvideos; i++) {
+if (def->videos[i]->primary) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Only one primary video device is
supported"));
+return -1;
+}
+}
+}
+
+return 0;
+}
diff --git a/src/conf/domain_validate.h b/src/conf/domain_validate.h
index 2b558668a9..df4f35c1dd 100644
--- a/src/conf/domain_validate.h
+++ b/src/conf/domain_validate.h
@@ -25,3 +25,4 @@
#include "domain_conf.h"
int virDomainDefBootValidate(const virDomainDef *def);
+int virDomainDefVideoValidate(const virDomainDef *def);
diff --git a/tests/qemuxml2argvdata/video-multiple-primaries.err
b/tests/qemuxml2argvdata/video-multiple-primaries.err
new file mode 100644
index 00..f81b7275e4
--- /dev/null
+++ b/tests/qemuxml2argvdata/video-multiple-primaries.err
@@ -0,0 +1 @@
+unsupported configuration: Only one primary video device is supported
diff --git a/tests/qemuxml2argvdata/video-multiple-primaries.xml
b/tests/qemuxml2argvdata/video-multiple-primaries.xml
new file mode 100644
index 00..977227c5ff
--- /dev/null
+++ b/tests/qemuxml2argvdata/video-multiple-primaries.xml
@@ -0,0 +1,32 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 1048576
+ 1048576
+ 1
+
+hvm
+
+
+
+ destroy
+ restart
+ destroy
+
+/usr/bin/qemu-system-i386
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 202fc83ccf..0e7d8d5ba3 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2379,6 +2379,11 @@ mymain(void)
DO_TEST_CAPS_ARCH_LATEST("default-video-type-riscv64", "riscv64");
DO_TEST_CAPS_ARCH_LATEST("default-video-type-s390x", "s390x");
+DO_TEST_PARSE_ERROR("video-multiple-primaries",
+QEMU_CAPS_DEVICE_QXL,
+QEMU_CAPS_DEVICE_VGA,
+QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
+
DO_TEST("virtio-rng-default",
QEMU_CAPS_DEVICE_VIRTIO_RNG,
QEMU_CAPS_OBJECT_RNG_RANDOM);
--
2.26.2
[PATCH v3 00/14] move checks from parse to validate callbacks
This is the v3 of https://www.redhat.com/archives/libvir-list/2020-December/msg00419.html Michal mentioned in his v2 review that we should move these validations out of domain_conf.c to a new file. Let's create a new file called domain_validate.c and start move the validations done in v2 into it, instead of pushing the already reviewed v2 series just to have more stuff to be moved later on. A follow up series will push more validations from domain_conf.c to domain_validate.c. Michal's R-bs were kept in all patches but patch 01. Daniel Henrique Barboza (14): domain_conf: move boot timeouts check to domain_validate.c domain_conf.c: move primary video check to validate callback domain_conf.c: move virDomainVideoDefValidate() to domain_validate.c domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf: move virDomainDiskDefValidate() to domain_validate.c domain_conf: move vendor, product and tray checks to domain_validate.c domain_validate.c: rename virSecurityDeviceLabelDefValidateXML() domain_conf: move all ChrSource checks to domain_validate.c domain_conf.c: move smartcard address check to domain_validate.c domain_conf.c: move blkio path check to domain_validate.c domain_conf.c: move virDomainControllerDefValidate() to domain_validate.c domain_conf: move virDomainPCIControllerOpts checks to domain_validate.c domain_conf: move pci-root/pcie-root address check to domain_validate.c domain_conf.c: move idmapEntry checks to domain_validate.c po/POTFILES.in| 1 + src/conf/domain_conf.c| 562 +-- src/conf/domain_validate.c| 637 ++ src/conf/domain_validate.h| 47 ++ src/conf/meson.build | 1 + tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 + tests/qemuxml2argvtest.c | 14 +- 10 files changed, 756 insertions(+), 543 deletions(-) create mode 100644 src/conf/domain_validate.c create mode 100644 src/conf/domain_validate.h create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml -- 2.26.2
Re: [PATCH RFC v4 06/15] target/riscv: Support start kernel directly by KVM
On Thu, Dec 3, 2020 at 4:58 AM Yifei Jiang wrote:
>
> Get kernel and fdt start address in virt.c, and pass them to KVM
> when cpu reset. In addition, add kvm_riscv.h to place riscv specific
> interface.
This doesn't seem right. Why do we need to do this? Other
architectures don't seem to do this.
Writing to the CPU from the board like this looks fishy and probably
breaks some QOM rules.
Alistair
>
> Signed-off-by: Yifei Jiang
> Signed-off-by: Yipeng Yin
> ---
> hw/riscv/virt.c | 8
> target/riscv/cpu.c | 4
> target/riscv/cpu.h | 3 +++
> target/riscv/kvm.c | 15 +++
> target/riscv/kvm_riscv.h | 24
> 5 files changed, 54 insertions(+)
> create mode 100644 target/riscv/kvm_riscv.h
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 25cea7aa67..47b7018193 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -42,6 +42,7 @@
> #include "sysemu/sysemu.h"
> #include "hw/pci/pci.h"
> #include "hw/pci-host/gpex.h"
> +#include "sysemu/kvm.h"
>
> #if defined(TARGET_RISCV32)
> # define BIOS_FILENAME "opensbi-riscv32-generic-fw_dynamic.bin"
> @@ -511,6 +512,7 @@ static void virt_machine_init(MachineState *machine)
> uint64_t kernel_entry;
> DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
> int i, j, base_hartid, hart_count;
> +CPUState *cs;
>
> /* Check socket count limit */
> if (VIRT_SOCKETS_MAX < riscv_socket_count(machine)) {
> @@ -660,6 +662,12 @@ static void virt_machine_init(MachineState *machine)
>virt_memmap[VIRT_MROM].size, kernel_entry,
>fdt_load_addr, s->fdt);
>
> +for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> +RISCVCPU *riscv_cpu = RISCV_CPU(cs);
> +riscv_cpu->env.kernel_addr = kernel_entry;
> +riscv_cpu->env.fdt_addr = fdt_load_addr;
> +}
> +
> /* SiFive Test MMIO device */
> sifive_test_create(memmap[VIRT_TEST].base);
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 6a0264fc6b..faee98a58c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -29,6 +29,7 @@
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "fpu/softfloat-helpers.h"
> +#include "kvm_riscv.h"
>
> /* RISC-V CPU definitions */
>
> @@ -330,6 +331,9 @@ static void riscv_cpu_reset(DeviceState *dev)
> cs->exception_index = EXCP_NONE;
> env->load_res = -1;
> set_default_nan_mode(1, &env->fp_status);
> +#ifdef CONFIG_KVM
> +kvm_riscv_reset_vcpu(cpu);
> +#endif
> }
>
> static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index c0a326c843..ad1c90f798 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -233,6 +233,9 @@ struct CPURISCVState {
>
> /* Fields from here on are preserved across CPU reset. */
> QEMUTimer *timer; /* Internal timer */
> +
> +hwaddr kernel_addr;
> +hwaddr fdt_addr;
> };
>
> OBJECT_DECLARE_TYPE(RISCVCPU, RISCVCPUClass,
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 8b206ce99c..6250ca0c7d 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -37,6 +37,7 @@
> #include "hw/irq.h"
> #include "qemu/log.h"
> #include "hw/loader.h"
> +#include "kvm_riscv.h"
>
> static __u64 kvm_riscv_reg_id(__u64 type, __u64 idx)
> {
> @@ -439,3 +440,17 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run
> *run)
> {
> return 0;
> }
> +
> +void kvm_riscv_reset_vcpu(RISCVCPU *cpu)
> +{
> +CPURISCVState *env = &cpu->env;
> +
> +if (!kvm_enabled()) {
> +return;
> +}
> +env->pc = cpu->env.kernel_addr;
> +env->gpr[10] = kvm_arch_vcpu_id(CPU(cpu)); /* a0 */
> +env->gpr[11] = cpu->env.fdt_addr; /* a1 */
> +env->satp = 0;
> +}
> +
> diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> new file mode 100644
> index 00..f38c82bf59
> --- /dev/null
> +++ b/target/riscv/kvm_riscv.h
> @@ -0,0 +1,24 @@
> +/*
> + * QEMU KVM support -- RISC-V specific functions.
> + *
> + * Copyright (c) 2020 Huawei Technologies Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> with
> + * this program. If not, see
Re: [PATCH RFC v4 03/15] target/riscv: Implement function kvm_arch_init_vcpu
On Thu, Dec 3, 2020 at 4:55 AM Yifei Jiang wrote:
>
> Get isa info from kvm while kvm init.
>
> Signed-off-by: Yifei Jiang
> Signed-off-by: Yipeng Yin
> ---
> target/riscv/kvm.c | 27 ++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 8c386d9acf..86660ba81b 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -38,6 +38,18 @@
> #include "qemu/log.h"
> #include "hw/loader.h"
>
> +static __u64 kvm_riscv_reg_id(__u64 type, __u64 idx)
> +{
> +__u64 id = KVM_REG_RISCV | type | idx;
> +
> +#if defined(TARGET_RISCV32)
> +id |= KVM_REG_SIZE_U32;
> +#elif defined(TARGET_RISCV64)
> +id |= KVM_REG_SIZE_U64;
> +#endif
There is a series on list (I'll send a v2 out later today) that starts
to remove these #ifdef for the RISC-V XLEN. Next time you rebase it
would be great if you can use that and hopefully remove this.
Alistair
> +return id;
> +}
> +
> const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> KVM_CAP_LAST_INFO
> };
> @@ -79,7 +91,20 @@ void kvm_arch_init_irq_routing(KVMState *s)
>
> int kvm_arch_init_vcpu(CPUState *cs)
> {
> -return 0;
> +int ret = 0;
> +target_ulong isa;
> +RISCVCPU *cpu = RISCV_CPU(cs);
> +CPURISCVState *env = &cpu->env;
> +__u64 id;
> +
> +id = kvm_riscv_reg_id(KVM_REG_RISCV_CONFIG,
> KVM_REG_RISCV_CONFIG_REG(isa));
> +ret = kvm_get_one_reg(cs, id, &isa);
> +if (ret) {
> +return ret;
> +}
> +env->misa = isa;
> +
> +return ret;
> }
>
> int kvm_arch_msi_data_to_gsi(uint32_t data)
> --
> 2.19.1
>
>
Re: [PATCH v2 0/9] move checks from parse to validate callbacks
On 12/8/20 4:58 PM, Michal Privoznik wrote: On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote: On 12/8/20 7:22 AM, Michal Privoznik wrote: But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size? Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file. What do you think? Yes, qemu_validate.c example was what I had on mind. I don't have preference whether to move everything at once or by smaller pieces (how small?). While the latter might look like easier to review, not really since git diff has now --color-moved. That way a reviewer can verify that a commit is just moving code. In fact, I'm respining this series already creating domain_validate.c right from the start. I find this way more sensible than adding code to domain_conf.c just to move to a new file afterwards. I'll post a v3 shortly. I'll keep your R-bs in the patches you already reviewed in v2 (the changes aren't worth reviewing it again). About how we're going to move the validations, I'll do what I did with qemu_validate.c. For example, move all static functions that is called by virDomainDefValidateInternal() in a single punch, then move virDomainDefValidateInternal(). Same thing with other relevant validation functions. This can be done in a follow-up series after this one. DHB Michal
Re: [PATCH v2 0/9] move checks from parse to validate callbacks
On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote: On 12/8/20 7:22 AM, Michal Privoznik wrote: But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size? Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file. What do you think? Yes, qemu_validate.c example was what I had on mind. I don't have preference whether to move everything at once or by smaller pieces (how small?). While the latter might look like easier to review, not really since git diff has now --color-moved. That way a reviewer can verify that a commit is just moving code. Michal
Re: [PATCH v2 0/9] move checks from parse to validate callbacks
On 12/8/20 7:22 AM, Michal Privoznik wrote: On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote: Hi, This is the respin of [1] after the reviews from Michal. Although we're pushing code to validate callbacks instead of post parse functions, the idea and motivation is still in line with [2]. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html [2] https://gitlab.com/libvirt/libvirt/-/issues/7 Daniel Henrique Barboza (9): domain_conf.c: move boot related timeouts check to validate callback domain_conf.c: move primary video check to validate callback domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf.c: move vendor, product and tray checks to validate callback domain_conf.c: move smartcard address check to validate callback domain_conf.c: move blkio path check to validate callback domain_conf.c: move virDomainPCIControllerOpts checks to validate callback domain_conf.c: move pci-root/pcie-root address check to validateCB domain_conf.c: move idmapEntry checks to validate callback src/conf/domain_conf.c | 353 +++--- tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 ++ tests/qemuxml2argvtest.c | 14 +- 6 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml Reviewed-by: Michal Privoznik Thanks for the review! But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size? Makes sense. I can create a 'domain_validate.c' and start moving stuff there, like I did previously with QEMU driver validations and qemu_validate.c. First order of business would be to move the validations I already played with in this series to this new file. What do you think? DHB Michal
Re: [libvirt PATCH 0/3] gitlab: refresh containers
On Tue, Dec 01, 2020 at 03:26:46PM +, Daniel P. Berrangé wrote: > This syncs with latest state of libvirt-ci.git If you push this, it'll fix the pipeline failure [1] because the centos registry is older than docker, so I'll have to adjust my workaround that I'll propose for libvirt-ci. Unless we also want to switch back to docker registry for centos and use distro-hosted registry for fedora only, I don't care either way, but it'll affect what I'll propose in libvirt-ci. IMHO let's go with this series, and if registry.centos.org uploads the same broken image, I'll add another commit in libvirt-ci as a workaround, but I'd like to avoid making manual changes to something we generated in an automated fashion. [1] https://gitlab.com/libvirt/libvirt/-/jobs/896489520 Reviewed-by: Erik Skultety
[PATCH 2/2] qemuDomainCheckpointLoad: Remove stale comment
We decided to not do metadata-less checkpoints and checking whether the metadata is consistent is done once the data is actually needed. Remove the comment. Signed-off-by: Peter Krempa --- src/qemu/qemu_driver.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0e5d7423dc..58c376fbe5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -533,13 +533,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm, vm->def->name); virDomainCheckpointSetCurrent(vm->checkpoints, current); -/* Note that it is not practical to automatically construct - * checkpoints based solely on qcow2 bitmaps, since qemu does not - * track parent relations which we find important in our metadata. - * Perhaps we could double-check that our just-loaded checkpoint - * metadata is consistent with existing qcow2 bitmaps, but a user - * that changes things behind our backs deserves what happens. */ - virResetLastError(); ret = 0; -- 2.29.2
[PATCH 1/2] qemuDomainCheckpointLoad: Don't align disks when restoring config from disk
The alignment step is not really necessary once we've done it already
since we fully populate the definition. In case of checkpoints it was a
relic necessary for populating the 'idx' to match checkpoint disk to
definition disk, but that was already removed.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7093fc619b..0e5d7423dc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -512,18 +512,11 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
continue;
}
-def = virDomainCheckpointDefParseString(xmlStr,
-qemu_driver->xmlopt,
-priv->qemuCaps,
-flags);
-if (!def || virDomainCheckpointAlignDisks(def) < 0) {
-/* Nothing we can do here, skip this one */
-virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to parse checkpoint XML from file '%s'"),
- fullpath);
-virObjectUnref(def);
+if (!(def = virDomainCheckpointDefParseString(xmlStr,
+ qemu_driver->xmlopt,
+ priv->qemuCaps,
+ flags)))
continue;
-}
chk = virDomainCheckpointAssignDef(vm->checkpoints, def);
if (chk == NULL)
--
2.29.2
[PATCH 0/2] qemuDomainCheckpointLoad: Two fixes
Peter Krempa (2): qemuDomainCheckpointLoad: Don't align disks when restoring config from disk qemuDomainCheckpointLoad: Remove stale comment src/qemu/qemu_driver.c | 22 -- 1 file changed, 4 insertions(+), 18 deletions(-) -- 2.29.2
Re: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On 12/8/20 4:20 PM, Peter Krempa wrote: On Tue, Dec 08, 2020 at 16:09:07 +0100, Michal Privoznik wrote: On 12/8/20 3:01 PM, Peter Krempa wrote: The validation callback always fetched a fresh copy of 'qemuCaps' to use for validation which is wrong in cases when the VM is already running, such as device hotplug. The newly-fetched qemuCaps may contain flags which weren't originally in use when starting the VM e.g. on a libvirtd upgrade. Since the post-parse/validation machinery has a per-run 'parseOpaque' field filled with qemuCaps of the actual process we can reuse the caps in cases when we get them. The code still fetches a fresh copy if parseOpaque doesn't have a per-run copy to preserve existing functionality. Signed-off-by: Peter Krempa --- src/qemu/qemu_validate.c | 30 -- 1 file changed, 20 insertions(+), 10 deletions(-) So this will use the old qemuCaps even though a feature might already be available in qemu but libvirt didn't detect it. For instance the virtio-pmem I'm implementing - guests started today won't have the qemuCap for virtio-pmem because it's not implemented yet. But when they update libvirt they still won't get the feature because we're using old qemuCaps for validation and thus new memory model will be denied because of lacking capability. Oh, this is actually what we do and it's required! Old libvirt might have configured the device in a way which is not compatible with the new one and thus attempting to talk to it using the new interface would just break. We currently store the old state of 'qemuCaps' and everything uses it in the state when the VM was started. Well except for the validator, which was broken and this patchset is fixing it. You will never get new features with an old VM. Okay then. Michal
Re: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On Tue, Dec 08, 2020 at 16:09:07 +0100, Michal Privoznik wrote: > On 12/8/20 3:01 PM, Peter Krempa wrote: > > The validation callback always fetched a fresh copy of 'qemuCaps' to use > > for validation which is wrong in cases when the VM is already running, > > such as device hotplug. The newly-fetched qemuCaps may contain flags > > which weren't originally in use when starting the VM e.g. on a libvirtd > > upgrade. > > > > Since the post-parse/validation machinery has a per-run 'parseOpaque' > > field filled with qemuCaps of the actual process we can reuse the caps > > in cases when we get them. > > > > The code still fetches a fresh copy if parseOpaque doesn't have a > > per-run copy to preserve existing functionality. > > > > Signed-off-by: Peter Krempa > > --- > > src/qemu/qemu_validate.c | 30 -- > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > So this will use the old qemuCaps even though a feature might already be > available in qemu but libvirt didn't detect it. For instance the virtio-pmem > I'm implementing - guests started today won't have the qemuCap for > virtio-pmem because it's not implemented yet. But when they update libvirt > they still won't get the feature because we're using old qemuCaps for > validation and thus new memory model will be denied because of lacking > capability. Oh, this is actually what we do and it's required! Old libvirt might have configured the device in a way which is not compatible with the new one and thus attempting to talk to it using the new interface would just break. We currently store the old state of 'qemuCaps' and everything uses it in the state when the VM was started. Well except for the validator, which was broken and this patchset is fixing it. You will never get new features with an old VM.
Re: [PATCH 4/4] qemu: validate: Prefer existing qemuCaps
On 12/8/20 3:01 PM, Peter Krempa wrote: The validation callback always fetched a fresh copy of 'qemuCaps' to use for validation which is wrong in cases when the VM is already running, such as device hotplug. The newly-fetched qemuCaps may contain flags which weren't originally in use when starting the VM e.g. on a libvirtd upgrade. Since the post-parse/validation machinery has a per-run 'parseOpaque' field filled with qemuCaps of the actual process we can reuse the caps in cases when we get them. The code still fetches a fresh copy if parseOpaque doesn't have a per-run copy to preserve existing functionality. Signed-off-by: Peter Krempa --- src/qemu/qemu_validate.c | 30 -- 1 file changed, 20 insertions(+), 10 deletions(-) So this will use the old qemuCaps even though a feature might already be available in qemu but libvirt didn't detect it. For instance the virtio-pmem I'm implementing - guests started today won't have the qemuCap for virtio-pmem because it's not implemented yet. But when they update libvirt they still won't get the feature because we're using old qemuCaps for validation and thus new memory model will be denied because of lacking capability. I can see the value in when we have to chose between old and new style of doing things, but I worry it will kill new features. Michal
Re: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
On 12/8/20 4:19 AM, Daniel P. Berrangé wrote:
On Mon, Dec 07, 2020 at 05:54:51PM -0500, Laine Stump wrote:
On 11/22/20 10:28 PM, Shi Lei wrote:
When netlink is supported, use netlink to create veth device pair
rather than 'ip link' command.
Signed-off-by: Shi Lei
---
src/util/virnetdevveth.c | 85 ++--
1 file changed, 56 insertions(+), 29 deletions(-)
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index b3eee1af..b4074371 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -27,6 +27,7 @@
#include "virfile.h"
#include "virstring.h"
#include "virnetdev.h"
+#include "virnetlink.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -75,6 +76,55 @@ static int virNetDevVethGetFreeNum(int startDev)
return -1;
}
+#if defined(WITH_LIBNL)
+static int
+virNetDevVethCreateInternal(const char *veth1, const char *veth2, int *status)
+{
+virNetlinkNewLinkData data = { .veth_peer = veth2 };
+
+return virNetlinkNewLink(veth1, "veth", &data, status);
+}
The only thing that makes me uncomfortable in this patch is that the two
versions of virNetDevVethCreateInternal() each return something different
for "status". In this first case, it is returning 0 on success, and -errno
on failure...
Just change this to return -1, as we very rarely want to return -errno
in libvirt, as we have formal error reporting.
I guess I should give a bit more detail here - the "-errno" returned by
virNetlinkNewLink() isn't the value of errno in the libvirt process at
the time it learns there was an error - it is the value of errno
returned in the netlink response message, ie what errno was set to in
the kernel context that is handling and responding to netlink messages.
errno in the libvirt process will not be set to this value.
And since the libvirt functions that are decoding the response messages
don't themselves log any errors, they aren't reporting that exact error...
...
Hmmm. this points out that we really need to be logging the exact error
in virNetDevVethCreateInternal(), since once we return from that
function we no longer have full info on the reason for the failure. This
would have been a problem if we had any inclination to retry the
operation (with a new device name, which the current code must do), but
once the other series from Shi is applied (the one that changes veth
name generation to mimic what is done for tap and macvtap), we won't
need to worry about retrying, and so it will be acceptable to
immediately log the error.
[...]
+#else
+static int
+virNetDevVethCreateInternal(const char *veth1, const char *veth2, int *status)
+{
+g_autoptr(virCommand) cmd = virCommandNew("ip");
+virCommandAddArgList(cmd, "link", "add", veth1, "type", "veth",
+ "peer", "name", veth2, NULL);
+
+return virCommandRun(cmd, status);
+}
But in this case it is returning the exit code of the "ip link add" command.
Also in this case if status != NULL then virCommandRun() will only log
an error if it failed to run the "ip" command; if ip runs but fails to
create the device and returns an error exit code, virCommandRun() will
silently return the exit code. In this case, we can just call
"virCommandRun(cmd, NULL)" - when status is NULL, virCommandRun() will
log an error if the exec fails, and also if the exec is successful but
the command returns an error.
And since I'm already here typing - when I reviewed the other series
(the ones that change the device name generation) I did arrive at the
opinion that we should apply those patches first, and these patches on
top of that - that way you won't have to worry about the possibility of
needing to retry virNetDevVethCreateInternal with a new name, and it
will be okay for its implementations to immediately log errors.
Re: [PATCH 0/4] qemu: Don't use updated qemuCaps in validation code
On a Tuesday in 2020, Peter Krempa wrote: See patch 4/4 for the justification. CI run: https://gitlab.com/pipo.sk/libvirt/-/pipelines/226815271 since the series is modifying stuff I don't compile usually. Peter Krempa (4): virDomainDefValidate: Add per-run 'opaque' data qemu: validate: Don't check that qemuCaps is non-NULL qemuValidateDomainDeviceDefFS: Fix block indentation qemu: validate: Prefer existing qemuCaps src/bhyve/bhyve_domain.c | 3 +- src/conf/domain_conf.c | 21 +++--- src/conf/domain_conf.h | 9 ++- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_validate.c | 138 --- src/qemu/qemu_validate.h | 7 +- src/vz/vz_driver.c | 6 +- 7 files changed, 103 insertions(+), 83 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH 0/4] qemu: Don't use updated qemuCaps in validation code
See patch 4/4 for the justification. CI run: https://gitlab.com/pipo.sk/libvirt/-/pipelines/226815271 since the series is modifying stuff I don't compile usually. Peter Krempa (4): virDomainDefValidate: Add per-run 'opaque' data qemu: validate: Don't check that qemuCaps is non-NULL qemuValidateDomainDeviceDefFS: Fix block indentation qemu: validate: Prefer existing qemuCaps src/bhyve/bhyve_domain.c | 3 +- src/conf/domain_conf.c | 21 +++--- src/conf/domain_conf.h | 9 ++- src/qemu/qemu_process.c | 2 +- src/qemu/qemu_validate.c | 138 --- src/qemu/qemu_validate.h | 7 +- src/vz/vz_driver.c | 6 +- 7 files changed, 103 insertions(+), 83 deletions(-) -- 2.28.0
[PATCH 4/4] qemu: validate: Prefer existing qemuCaps
The validation callback always fetched a fresh copy of 'qemuCaps' to use
for validation which is wrong in cases when the VM is already running,
such as device hotplug. The newly-fetched qemuCaps may contain flags
which weren't originally in use when starting the VM e.g. on a libvirtd
upgrade.
Since the post-parse/validation machinery has a per-run 'parseOpaque'
field filled with qemuCaps of the actual process we can reuse the caps
in cases when we get them.
The code still fetches a fresh copy if parseOpaque doesn't have a
per-run copy to preserve existing functionality.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_validate.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index cb0f830cf1..bf8127a575 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1067,16 +1067,21 @@ qemuValidateDomainDefPanic(const virDomainDef *def,
int
qemuValidateDomainDef(const virDomainDef *def,
void *opaque,
- void *parseOpaque G_GNUC_UNUSED)
+ void *parseOpaque)
{
virQEMUDriverPtr driver = opaque;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-g_autoptr(virQEMUCaps) qemuCaps = NULL;
+g_autoptr(virQEMUCaps) qemuCapsLocal = NULL;
+virQEMUCapsPtr qemuCaps = parseOpaque;
size_t i;
-if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
-def->emulator)))
-return -1;
+if (!qemuCaps) {
+if (!(qemuCapsLocal = virQEMUCapsCacheLookup(driver->qemuCapsCache,
+ def->emulator)))
+return -1;
+
+qemuCaps = qemuCapsLocal;
+}
if (def->os.type != VIR_DOMAIN_OSTYPE_HVM) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -4666,15 +4671,20 @@ int
qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev,
const virDomainDef *def,
void *opaque,
-void *parseOpaque G_GNUC_UNUSED)
+void *parseOpaque)
{
int ret = 0;
virQEMUDriverPtr driver = opaque;
-g_autoptr(virQEMUCaps) qemuCaps = NULL;
+g_autoptr(virQEMUCaps) qemuCapsLocal = NULL;
+virQEMUCapsPtr qemuCaps = parseOpaque;
-if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
-def->emulator)))
-return -1;
+if (!qemuCaps) {
+if (!(qemuCapsLocal = virQEMUCapsCacheLookup(driver->qemuCapsCache,
+ def->emulator)))
+return -1;
+
+qemuCaps = qemuCapsLocal;
+}
if ((ret = qemuValidateDomainDeviceDefAddress(dev, qemuCaps)) < 0)
return ret;
--
2.28.0
[PATCH 3/4] qemuValidateDomainDeviceDefFS: Fix block indentation
Signed-off-by: Peter Krempa
---
src/qemu/qemu_validate.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 81a9c9d45b..cb0f830cf1 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3997,8 +3997,7 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
return -1;
}
if (fs->multidevs != VIR_DOMAIN_FS_MODEL_DEFAULT &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_MULTIDEVS))
-{
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_FSDEV_MULTIDEVS)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("multidevs is not supported with this QEMU binary"));
return -1;
--
2.28.0
[PATCH 1/4] virDomainDefValidate: Add per-run 'opaque' data
virDomainDefPostParse infrastructure has apart from the global opaque
data also per-run data, but this was not duplicated into the validation
callbacks.
This is important when drivers want to use correct run-state for the
validation.
Signed-off-by: Peter Krempa
---
src/bhyve/bhyve_domain.c | 3 ++-
src/conf/domain_conf.c | 21 +
src/conf/domain_conf.h | 9 ++---
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_validate.c | 6 --
src/qemu/qemu_validate.h | 7 +--
src/vz/vz_driver.c | 6 --
7 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index 6935609b96..f0e553113f 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -199,7 +199,8 @@ virBhyveDriverCreateXMLConf(bhyveConnPtr driver)
static int
bhyveDomainDeviceDefValidate(const virDomainDeviceDef *dev,
const virDomainDef *def G_GNUC_UNUSED,
- void *opaque G_GNUC_UNUSED)
+ void *opaque G_GNUC_UNUSED,
+ void *parseOpaque G_GNUC_UNUSED)
{
if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER &&
dev->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_ISA &&
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 66ee658e7b..fa19563a35 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6974,14 +6974,15 @@ static int
virDomainDeviceDefValidate(const virDomainDeviceDef *dev,
const virDomainDef *def,
unsigned int parseFlags,
- virDomainXMLOptionPtr xmlopt)
+ virDomainXMLOptionPtr xmlopt,
+ void *parseOpaque)
{
/* validate configuration only in certain places */
if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)
return 0;
if (xmlopt->config.deviceValidateCallback &&
-xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv))
+xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv,
parseOpaque))
return -1;
if (virDomainDeviceDefValidateInternal(dev, def) < 0)
@@ -6999,7 +7000,8 @@ virDomainDefValidateDeviceIterator(virDomainDefPtr def,
{
struct virDomainDefPostParseDeviceIteratorData *data = opaque;
return virDomainDeviceDefValidate(dev, def,
- data->parseFlags, data->xmlopt);
+ data->parseFlags, data->xmlopt,
+ data->parseOpaque);
}
@@ -7357,6 +7359,7 @@ virDomainDefValidateInternal(const virDomainDef *def,
* @caps: driver capabilities object
* @parseFlags: virDomainDefParseFlags
* @xmlopt: XML parser option object
+ * @parseOpaque: hypervisor driver specific data for this validation run
*
* This validation function is designed to take checks of globally invalid
* configurations that the parser needs to accept so that VMs don't vanish upon
@@ -7369,11 +7372,13 @@ virDomainDefValidateInternal(const virDomainDef *def,
int
virDomainDefValidate(virDomainDefPtr def,
unsigned int parseFlags,
- virDomainXMLOptionPtr xmlopt)
+ virDomainXMLOptionPtr xmlopt,
+ void *parseOpaque)
{
struct virDomainDefPostParseDeviceIteratorData data = {
.xmlopt = xmlopt,
.parseFlags = parseFlags,
+.parseOpaque = parseOpaque,
};
/* validate configuration only in certain places */
@@ -7382,7 +7387,7 @@ virDomainDefValidate(virDomainDefPtr def,
/* call the domain config callback */
if (xmlopt->config.domainValidateCallback &&
-xmlopt->config.domainValidateCallback(def, xmlopt->config.priv) < 0)
+xmlopt->config.domainValidateCallback(def, xmlopt->config.priv,
parseOpaque) < 0)
return -1;
/* iterate the devices */
@@ -17220,7 +17225,7 @@ virDomainDeviceDefParse(const char *xmlStr,
return NULL;
/* validate the configuration */
-if (virDomainDeviceDefValidate(dev, def, flags, xmlopt) < 0)
+if (virDomainDeviceDefValidate(dev, def, flags, xmlopt, parseOpaque) < 0)
return NULL;
return g_steal_pointer(&dev);
@@ -22617,7 +22622,7 @@ virDomainObjParseXML(xmlDocPtr xml,
goto error;
/* validate configuration */
-if (virDomainDefValidate(obj->def, flags, xmlopt) < 0)
+if (virDomainDefValidate(obj->def, flags, xmlopt, parseOpaque) < 0)
goto error;
return obj;
@@ -22701,7 +22706,7 @@ virDomainDefParseNode(xmlDocPtr xml,
return NULL;
/* validate configuration */
-if (virDomainDefValidate(def, flags, xmlopt) < 0)
+if (virDomainDefValidate(def, flags, xmlopt, parseOpaque) < 0)
return NULL;
return g_steal_pointer(&def);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cdf08de60a..7
[PATCH 2/4] qemu: validate: Don't check that qemuCaps is non-NULL
The validation callbacks always fetch latest qemuCaps so it won't ever
be NULL. Remove the tautological conditions.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_validate.c | 103 +++
1 file changed, 49 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 62d7243e21..81a9c9d45b 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1099,8 +1099,7 @@ qemuValidateDomainDef(const virDomainDef *def,
return -1;
}
-if (qemuCaps &&
-!virQEMUCapsIsMachineSupported(qemuCaps, def->virtType,
def->os.machine)) {
+if (!virQEMUCapsIsMachineSupported(qemuCaps, def->virtType,
def->os.machine)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Emulator '%s' does not support machine type '%s'"),
def->emulator, def->os.machine);
@@ -2648,37 +2647,35 @@ qemuValidateDomainDeviceDefDiskFrontend(const
virDomainDiskDef *disk,
return -1;
}
-if (qemuCaps) {
-if (disk->serial &&
-disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
-disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("scsi-block 'lun' devices do not support the "
- "serial property"));
-return -1;
-}
+if (disk->serial &&
+disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
+disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("scsi-block 'lun' devices do not support the "
+ "serial property"));
+return -1;
+}
-if (disk->discard &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("discard is not supported by this QEMU binary"));
-return -1;
-}
+if (disk->discard &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DISCARD)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("discard is not supported by this QEMU binary"));
+return -1;
+}
+
+if (disk->detect_zeroes &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("detect_zeroes is not supported by this QEMU
binary"));
+return -1;
+}
-if (disk->detect_zeroes &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_DETECT_ZEROES)) {
+if (disk->iomode == VIR_DOMAIN_DISK_IO_URING) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_AIO_IO_URING)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("detect_zeroes is not supported by this QEMU
binary"));
+ _("io uring is not supported by this QEMU binary"));
return -1;
}
-
-if (disk->iomode == VIR_DOMAIN_DISK_IO_URING) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_AIO_IO_URING)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("io uring is not supported by this QEMU
binary"));
-return -1;
-}
-}
}
if (disk->serial &&
@@ -2753,33 +2750,31 @@ qemuValidateDomainDeviceDefDiskBlkdeviotune(const
virDomainDiskDef *disk,
return -1;
}
-if (qemuCaps) {
-/* block I/O throttling 1.7 */
-if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("there are some block I/O throttling parameters "
- "that are not supported with this QEMU binary"));
-return -1;
-}
+/* block I/O throttling 1.7 */
+if (virDomainBlockIoTuneInfoHasMax(&disk->blkdeviotune) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("there are some block I/O throttling parameters "
+ "that are not supported with this QEMU binary"));
+return -1;
+}
-/* block I/O group 2.4 */
-if (disk->blkdeviotune.group_name &&
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("the block I/O throttling group parameter is "
- "not supported with this QEMU binary"));
-return -1;
-}
+/* block I/O group 2.4 */
+if (disk->blkdeviotune.group_name &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_GROUP)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%
Re: [PATCH 0/2] virsh: Fix set-user-sshkeys command
On a Tuesday in 2020, Michal Privoznik wrote: The first patch is a bug fix, the second is an improvement. Michal PrÃvoznÃk (2): virsh: Fix logical error in cmdSetUserSSHKeys() virsh: cmdSetUserSSHKeys: Error early if the file doesn't contain any keys tools/virsh-domain.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) Reviewed-by: Ján Tomko Jano
[PATCH 2/2] virsh: cmdSetUserSSHKeys: Error early if the file doesn't contain any keys
When removing SSH keys via set-user-sshkeys virsh command, then
files to remove are read from passed file. But when
experimenting, I've passed /dev/null as the file which resulted
in API checks which caught that @keys argument of
virDomainAuthorizedSSHKeysSet() can't be NULL. This is because if
the file is empty then its content is an empty string and thus
the buffer the file was read in to is not NULL.
Long story short, error is reported correctly, but it's not
necessary to go through public API to catch it.
Signed-off-by: Michal Privoznik
---
tools/virsh-domain.c | 4
1 file changed, 4 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 6266c7acd2..befa8d2448 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14398,6 +14398,10 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
goto cleanup;
nkeys = virStringListLength((const char **) keys);
+if (nkeys == 0) {
+vshError(ctl, _("File %s contains no keys"), from);
+goto cleanup;
+}
}
if (virDomainAuthorizedSSHKeysSet(dom, user,
--
2.26.2
[PATCH 1/2] virsh: Fix logical error in cmdSetUserSSHKeys()
In v6.10.0-rc1~104 I've added a virsh command that exposes
virDomainAuthorizedSSHKeysSet() API under "set-user-sshkeys"
command. The command accepts mutually exclusive "--reset" and
"--remove" options (among others). While the former controls the
VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND flag, the latter
controls the VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE flag.
These flags are also mutually exclusive. But the code that sets
them has a logical error which may result in both flags being
set. In fact, this results in user being not able to set just the
remove flag.
Fixes: 87d12effbea8b414c250b6fefd93154c62a99370
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1904674
Signed-off-by: Michal Privoznik
---
tools/virsh-domain.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1fb4189b4b..6266c7acd2 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14375,17 +14375,18 @@ cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
goto cleanup;
-if (!vshCommandOptBool(cmd, "reset")) {
-flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
-
-if (!from) {
-vshError(ctl, _("Option --file is required"));
-goto cleanup;
-}
-}
-
-if (vshCommandOptBool(cmd, "remove"))
+if (vshCommandOptBool(cmd, "remove")) {
flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
+} else {
+if (!vshCommandOptBool(cmd, "reset")) {
+flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
+
+if (!from) {
+vshError(ctl, _("Option --file is required"));
+goto cleanup;
+}
+}
+}
if (from) {
if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) {
--
2.26.2
[PATCH 0/2] virsh: Fix set-user-sshkeys command
The first patch is a bug fix, the second is an improvement. Michal Prívozník (2): virsh: Fix logical error in cmdSetUserSSHKeys() virsh: cmdSetUserSSHKeys: Error early if the file doesn't contain any keys tools/virsh-domain.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) -- 2.26.2
Re: [PATCH 0/8] qemu: Improve nodename lookup and format index for backup disks
On a Monday in 2020, Peter Krempa wrote: Peter Krempa (8): qemuDomainDiskLookupByNodename: Simplify node name lookup qemuDomainGetStorageSourceByDevstr: Use virDomainDiskByTarget qemuDomainGetStorageSourceByDevstr: Avoid logged errors backup: Move file format check from parser to qemu driver virDomainBackupDiskDefParseXML: Use virDomainStorageSourceParseBase qemuDomainDiskLookupByNodename: Lookup also backup 'store' nodenames qemuDomainGetStorageSourceByDevstr: Lookup also backup 'store' nodenames conf: backup: Format index of 'store' docs/formatbackup.rst | 4 + src/conf/backup_conf.c| 46 --- src/qemu/qemu_backup.c| 10 ++- src/qemu/qemu_domain.c| 78 --- src/qemu/qemu_domain.h| 4 +- src/qemu/qemu_driver.c| 2 +- src/qemu/qemu_process.c | 7 +- .../qemustatusxml2xmldata/backup-pull-in.xml | 2 +- 8 files changed, 89 insertions(+), 64 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: Ping: [PATCH] Qemu: migration: Not bind RAM info with active migration status
On Tue, Dec 08, 2020 at 09:27:39 +, Daniel P. Berrangé wrote: > On Tue, Dec 08, 2020 at 10:06:25AM +0800, zhukeqian wrote: > > > > On 2020/12/7 18:38, Daniel P. Berrangé wrote: > > > On Mon, Dec 07, 2020 at 09:55:53AM +0800, zhukeqian wrote: > > >> Hi Daniel, > > >> > > >> On 2020/12/4 22:42, Daniel Henrique Barboza wrote: > > >>> > > >>> > > >>> On 12/4/20 5:12 AM, zhukeqian wrote: > > Hi folks, > > > > Kindly ping. I found that this patch is not applied. > > Has reviewed by Daniel Henrique Barboza and Daniel P. Berrangé. > > >>> > > >>> > > >>> It has my ACK, but looking into the messages I see that Daniel was > > >>> inquiring about this being a bug fix or an enhancement (he didn't > > >>> provide an ACK). Not sure if he wants some changes in the commit > > >>> message or if he has any other reservations about it. > > >>> > > >> I see, thanks. I will ask for his thoughts. > > > > > > Yes, it wasn't clear what this actually changed from libvirt's POV. > > > > > > What API call or usage scenario is currently broken, that this fixes ? > > > > > > > Hi Daniel, > > > > The purpose is to remove this failure check for QEMU v2.12. > > In QEMU commit 65ace0604551, it decoupled the RAM status from the active > > migration status. > > > > The usage scenario is querying migration status at destination side, which > > may contain > > active migration status, but without RAM status, so we will see that > > libvirt report error here. > > I'm confused, because AFAIK, libvirt does not need to run > query-migrate on the destination, so there shouldn't be anything > that needs fixing. Moreover, you can't even request migration statistics on the destination manually because libvirt blocks that: # virsh domjobinfo nest error: Operation not supported: migration statistics are available only on the source host Jirka
Re: [PATCH v2 0/9] move checks from parse to validate callbacks
On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote: Hi, This is the respin of [1] after the reviews from Michal. Although we're pushing code to validate callbacks instead of post parse functions, the idea and motivation is still in line with [2]. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html [2] https://gitlab.com/libvirt/libvirt/-/issues/7 Daniel Henrique Barboza (9): domain_conf.c: move boot related timeouts check to validate callback domain_conf.c: move primary video check to validate callback domain_conf.c: move QXL attributes check to virDomainVideoDefValidate() domain_conf.c: move vendor, product and tray checks to validate callback domain_conf.c: move smartcard address check to validate callback domain_conf.c: move blkio path check to validate callback domain_conf.c: move virDomainPCIControllerOpts checks to validate callback domain_conf.c: move pci-root/pcie-root address check to validateCB domain_conf.c: move idmapEntry checks to validate callback src/conf/domain_conf.c| 353 +++--- tests/qemuxml2argvdata/pci-root-address.err | 2 +- .../pseries-default-phb-numa-node.err | 2 +- .../video-multiple-primaries.err | 1 + .../video-multiple-primaries.xml | 32 ++ tests/qemuxml2argvtest.c | 14 +- 6 files changed, 268 insertions(+), 136 deletions(-) create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml Reviewed-by: Michal Privoznik But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size? Michal
Re: [PATCH] qemuxml2argvtest: Add 'nvme' disks into the 'disk-slices' case
On 12/7/20 6:07 PM, Peter Krempa wrote: Test slices on top of nvme-backed disks. Note that the changes in seemingly irrelevant parts of the output are due to re-naming the nodenames. Signed-off-by: Peter Krempa --- .../disk-slices.x86_64-latest.args| 63 --- tests/qemuxml2argvdata/disk-slices.xml| 25 .../disk-slices.x86_64-latest.xml | 25 3 files changed, 92 insertions(+), 21 deletions(-) Reviewed-by: Michal Privoznik Michal
Re: [PATCH] qemuBlockJobInfoTranslate: Take job type from qemuBlockJobDataPtr
On 12/7/20 5:57 PM, Peter Krempa wrote: Commit f5e8715a8b4 added logic which adds some fake job info when qemu didn't return anything but in such case the job type would not be set. Since we already have the proper job type recorded in qemuBlockJobDataPtr which the caller fetched, we can use this it and also remove the lookup from the disk which was necessary prior to the conversion to qemuBlockJobDataPtr. Signed-off-by: Peter Krempa --- I've wanted to fix it before pushing what would become commit f5e8715a8b4 but Michal accidentally pushed it with one of his patches. src/qemu/qemu_driver.c | 11 --- 1 file changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Michal Privoznik Michal
[libvirt PATCH] util: add missing FSF copyright statement
We previous added code for passing FDs which was explicitly derived from gnulib's passfd code: commit 17460825f3c78e1635f2beb0165c5a19e3b09f7d Author: Daniel P. Berrangé Date: Fri Jan 17 11:57:17 2020 + src: implement APIs for passing FDs over UNIX sockets This is a simplified variant of gnulib's passfd module without the portability code that we do not require. while the license was unchanged, we mistakenly failed to copy the FSF copyright header which is required by the license terms. Reported-by: Bruno Haible Signed-off-by: Daniel P. Berrangé --- Pushed as a trivial fix src/util/virsocket.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virsocket.c b/src/util/virsocket.c index 9aa29f1eb6..c8435a1087 100644 --- a/src/util/virsocket.c +++ b/src/util/virsocket.c @@ -1,5 +1,6 @@ /* * Copyright (C) 2020 Red Hat, Inc. + * Copyright (C) 2011-2020 Free Software Foundation, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public -- 2.28.0
Re: Ping: [PATCH] Qemu: migration: Not bind RAM info with active migration status
On Tue, Dec 08, 2020 at 10:06:25AM +0800, zhukeqian wrote: > > On 2020/12/7 18:38, Daniel P. Berrangé wrote: > > On Mon, Dec 07, 2020 at 09:55:53AM +0800, zhukeqian wrote: > >> Hi Daniel, > >> > >> On 2020/12/4 22:42, Daniel Henrique Barboza wrote: > >>> > >>> > >>> On 12/4/20 5:12 AM, zhukeqian wrote: > Hi folks, > > Kindly ping. I found that this patch is not applied. > Has reviewed by Daniel Henrique Barboza and Daniel P. Berrangé. > >>> > >>> > >>> It has my ACK, but looking into the messages I see that Daniel was > >>> inquiring about this being a bug fix or an enhancement (he didn't > >>> provide an ACK). Not sure if he wants some changes in the commit > >>> message or if he has any other reservations about it. > >>> > >> I see, thanks. I will ask for his thoughts. > > > > Yes, it wasn't clear what this actually changed from libvirt's POV. > > > > What API call or usage scenario is currently broken, that this fixes ? > > > > Hi Daniel, > > The purpose is to remove this failure check for QEMU v2.12. > In QEMU commit 65ace0604551, it decoupled the RAM status from the active > migration status. > > The usage scenario is querying migration status at destination side, which > may contain > active migration status, but without RAM status, so we will see that libvirt > report error here. I'm confused, because AFAIK, libvirt does not need to run query-migrate on the destination, so there shouldn't be anything that needs fixing. So can you explain what sceanario with libvirt you are seeing an error in, and exactly how this fixes it. 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: [PATCHv2 2/2] util:veth: Create veth device pair by netlink
On Mon, Dec 07, 2020 at 05:54:51PM -0500, Laine Stump wrote:
> On 11/22/20 10:28 PM, Shi Lei wrote:
> > When netlink is supported, use netlink to create veth device pair
> > rather than 'ip link' command.
> >
> > Signed-off-by: Shi Lei
> > ---
> > src/util/virnetdevveth.c | 85 ++--
> > 1 file changed, 56 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
> > index b3eee1af..b4074371 100644
> > --- a/src/util/virnetdevveth.c
> > +++ b/src/util/virnetdevveth.c
> > @@ -27,6 +27,7 @@
> > #include "virfile.h"
> > #include "virstring.h"
> > #include "virnetdev.h"
> > +#include "virnetlink.h"
> > #define VIR_FROM_THIS VIR_FROM_NONE
> > @@ -75,6 +76,55 @@ static int virNetDevVethGetFreeNum(int startDev)
> > return -1;
> > }
> > +#if defined(WITH_LIBNL)
> > +static int
> > +virNetDevVethCreateInternal(const char *veth1, const char *veth2, int
> > *status)
> > +{
> > +virNetlinkNewLinkData data = { .veth_peer = veth2 };
> > +
> > +return virNetlinkNewLink(veth1, "veth", &data, status);
> > +}
>
> The only thing that makes me uncomfortable in this patch is that the two
> versions of virNetDevVethCreateInternal() each return something different
> for "status". In this first case, it is returning 0 on success, and -errno
> on failure...
Just change this to return -1, as we very rarely want to return -errno
in libvirt, as we have formal error reporting.
>
>
> > [...]
>
> > +#else
> > +static int
> > +virNetDevVethCreateInternal(const char *veth1, const char *veth2, int
> > *status)
> > +{
> > +g_autoptr(virCommand) cmd = virCommandNew("ip");
> > +virCommandAddArgList(cmd, "link", "add", veth1, "type", "veth",
> > + "peer", "name", veth2, NULL);
> > +
> > +return virCommandRun(cmd, status);
> > +}
>
>
> But in this case it is returning the exit code of the "ip link add" command.
>
>
> It turns out that the value of status is only checked for 0 / not-0, so in
> practice it doesn't matter, but it could lead to confusion in the future.
>
>
> If we want these patches to be applied before your "netdevveth: Simplify
> virNetDevVethCreate by using virNetDevGenerateName" patch (which I'll get to
> as soon as I send this mail), then we do still need a way to differentiate
> between "The requested device already exists" and "Permanent Failure", and
> in that case I would suggest that "status" be replaced with a variable
> called "retry" which would be set to true if retrying with a different
> device name might lead to success (it would be set based on an
> interpretation of status made by each of the vir*Internal() functions)
>
>
> However, if we decide to apply the *other* patchset first, then we will
> never retry anyway (because we've already checked if the device exists
> before we try to create it, and there is therefore no loop) and so we could
> just eliminate the final argument completely, and keep each vir*Internal()
> function's status internal to itself. (As a matter of fact, status in the
> virCommandRun() version could simply be replaced with NULL in the arglist to
> virCommandRun(), and not defined at all; status in the case of
> virNetlinkNewLink() would still need to be defined and passed into the
> function, but its value would just be ignored).
>
>
> I think it may be cleaner to do the latter, and it looks like your other
> series was written to be applied without this series anyway; let me look at
> those patches and I'll reply a 2nd time to this based on the results of
> reviewing the other series...
>
>
> BTW, I appreciate your patience - I had looked at these patches nearly two
> weeks ago (soon after you sent them), but then a holiday got in the way, and
> I forgot to post my reply after I returned to work :-/
>
>
> > [...]
> > -
> > -if (virCommandRun(cmd, &status) < 0)
> > +if (virNetDevVethCreateInternal(*veth1 ? *veth1 : veth1auto,
> > +*veth2 ? *veth2 : veth2auto,
> > +&status) < 0)
> > goto cleanup;
> > if (status == 0) {
>
>
> This spot here ^ is the only place that status is examined, and I actually
> think it's not going to work as you'd like in the case of netlink - status
> will always be 0 if virNetDevVethCreateInternal() returns 0, and when the
> return is < 0, status may or may not be set to -errno of the attempted
> operation - if status is 0, that means there was a serious error before even
> trying to create the device (e.g. OOM), and if it is non-0, then it might be
> because a device with the desired name already exists, or it might be
> because we don't have enough privilege to create a new device.
>
>
> Anyway, let me look at the other patchset...
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-
Re: [PATCH for-6.0] accel: Wire accel to /machine
On Tue, Dec 08, 2020 at 11:13:07 +0300, Roman Bolshakov wrote: > On Mon, Dec 07, 2020 at 06:50:07PM +0100, Peter Krempa wrote: > > On Mon, Dec 07, 2020 at 12:38:49 -0500, Eduardo Habkost wrote: > > > On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote: > > > > There's no generic way to query current accel and its properties via QOM > > > > because there's no link between an accel and current machine. > > > > > > > > The change adds the link, i.e. if HVF is enabled the following will be > > > > available in QOM: > > > > > > > > (qemu) qom-get /machine/accel type > > > > "hvf-accel" > > > > > > > > Suggested-by: Markus Armbruster > > > > Suggested-by: Paolo Bonzini > > > > Signed-off-by: Roman Bolshakov > > > > --- > > > > > > > > Hi, > > > > > > > > this is a follow up patch that deprecates earlier series [1]. > > > > > > > > > > Is there a reference to the reasoning for dropping the earlier > > > approach? Your previous approach seems preferable. > > > > The gist of the discussion before was that deprecating old commands in > > the same release cycle as introducing the replacement might be > > problematic if libvirt wants to adapt ASAP and that the new command > > should be elevated to a intermediate tier of stability, where ACK from > > libvirt is needed to change it during the same release cycle > > incompatibly. > > > > That was meant generally for any command, and was started because we had > > a similar issue recently. > > > > My intention definitely was not to change the patch itself, but more a > > process change so that we can keep cooperating on new stuff rapidly, but > > without actually breaking what we do. > > > > Thanks Peter, > > I'll drop deprecation patch in v2 of query-accel QMP command. Actually In the discussion my stance is that you can deprecate the old command itself, but starting from that point any semantic changes to query-accel must be consulted with libvirt as if query-accel was already released. We do want to develop the use of the new command as soon as possible, because that's beneficial to both sides and can actually show a design problem in the replacement command, so having us replace it before qemu releases it is good. On the other hand once libvirt takes the replacement, we must be involved in any changes to the command due to de-synced release shchedules so that there isn't a libvirt which e.g. didnt work. I specifically don't want to derail any of this collaboration, I just want to enhance it by all parties knowingly agreeing to the "gentleman's agreement" since complicating the process would actually be detremental. The thing is that the command may be changed if we know about it and e.g. didn't yet commit the replacement. We just need to communicate.
Re: [PATCH for-6.0] accel: Wire accel to /machine
On Mon, Dec 07, 2020 at 06:50:07PM +0100, Peter Krempa wrote: > On Mon, Dec 07, 2020 at 12:38:49 -0500, Eduardo Habkost wrote: > > On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote: > > > There's no generic way to query current accel and its properties via QOM > > > because there's no link between an accel and current machine. > > > > > > The change adds the link, i.e. if HVF is enabled the following will be > > > available in QOM: > > > > > > (qemu) qom-get /machine/accel type > > > "hvf-accel" > > > > > > Suggested-by: Markus Armbruster > > > Suggested-by: Paolo Bonzini > > > Signed-off-by: Roman Bolshakov > > > --- > > > > > > Hi, > > > > > > this is a follow up patch that deprecates earlier series [1]. > > > > > > > Is there a reference to the reasoning for dropping the earlier > > approach? Your previous approach seems preferable. > > The gist of the discussion before was that deprecating old commands in > the same release cycle as introducing the replacement might be > problematic if libvirt wants to adapt ASAP and that the new command > should be elevated to a intermediate tier of stability, where ACK from > libvirt is needed to change it during the same release cycle > incompatibly. > > That was meant generally for any command, and was started because we had > a similar issue recently. > > My intention definitely was not to change the patch itself, but more a > process change so that we can keep cooperating on new stuff rapidly, but > without actually breaking what we do. > Thanks Peter, I'll drop deprecation patch in v2 of query-accel QMP command. Ragards, Roman
Re: [PATCH for-6.0] accel: Wire accel to /machine
On Mon, Dec 07, 2020 at 12:38:49PM -0500, Eduardo Habkost wrote: > On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote: > > There's no generic way to query current accel and its properties via QOM > > because there's no link between an accel and current machine. > > > > The change adds the link, i.e. if HVF is enabled the following will be > > available in QOM: > > > > (qemu) qom-get /machine/accel type > > "hvf-accel" > > > > Suggested-by: Markus Armbruster > > Suggested-by: Paolo Bonzini > > Signed-off-by: Roman Bolshakov > > --- > > > > Hi, > > > > this is a follow up patch that deprecates earlier series [1]. > > > > Is there a reference to the reasoning for dropping the earlier > approach? Your previous approach seems preferable. > Okay I wasn't sure about that :) It's clear now from your and Daniel's response that a documented QMP method is preferable for public API. > but we need a commit message that doesn't make people think the > `qom-get` command above will always work. > How about the one below: accel: Wire accel to /machine An accel is a property of a machine but it's not reflected in QOM. An ownership between machine and accel is also not expressed. The change adds the link, i.e. if HVF is enabled the following will be available in QOM: (qemu) qom-get /machine/accel type "hvf-accel" Note that management applications shouldn't rely on the type as it may change in future at QEMU's discretion. --- Regards, Roman
