[libvirt] [PATCH v3 0/2] Add support for qcow2 cache
changes since v2: Patch 1 - a) add new element qcow2 as drive's child, if any qcow2 specific configuration exists. b) merge the three test cases to one. c) update docs. cache size only support bytes. d) move the parameters checking to virDomainDiskDefParseValidate Patch 2 - a) use QMP instead of qemu version to identify qemu capabilities. b) change capablity names with prefix drive.qcow2. c) merge the three test cases to one. Liu Qing (2): conf, docs: Add qcow2 cache configuration support qemu: add capability checking for qcow2 cache configuration docs/formatdomain.html.in | 43 ++ docs/schemas/domaincommon.rng | 35 src/conf/domain_conf.c | 95 +- src/qemu/qemu_capabilities.c | 9 ++ src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_command.c| 33 src/util/virstoragefile.c | 3 + src/util/virstoragefile.h | 6 ++ tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 3 + .../caps_2.6.0-gicv2.aarch64.xml | 3 + .../caps_2.6.0-gicv3.aarch64.xml | 3 + tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 3 + tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 3 + tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 3 + tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 3 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 3 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 3 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 3 + .../qemuxml2argv-disk-drive-qcow2-cache.args | 28 +++ .../qemuxml2argv-disk-drive-qcow2-cache.xml| 43 ++ tests/qemuxml2argvtest.c | 4 + .../qemuxml2xmlout-disk-drive-qcow2-cache.xml | 43 ++ tests/qemuxml2xmltest.c| 1 + 25 files changed, 381 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml -- 1.8.3.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] conf, docs: Add qcow2 cache configuration support
Random write IOPS will drop dramatically if qcow2 l2 cache could not cover the whole disk. This patch gives libvirt user a chance to adjust the qcow2 cache configuration. Three new qcow2 driver parameters are added. They are l2-cache-size, refcount-cache-size and cache-clean-interval. The following are from qcow2-cache.txt. The amount of virtual disk that can be mapped by the L2 and refcount caches (in bytes) is: disk_size = l2_cache_size * cluster_size / 8 disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits The parameter "cache-clean-interval" defines an interval (in seconds). All cache entries that haven't been accessed during that interval are removed from memory. Signed-off-by: Liu Qing --- a) add new element qcow2 as drive's child, if any qcow2 specific configuration exists. b) merge the three test cases to one. c) update docs. cache size only support bytes. d) move the parameters checking to virDomainDiskDefParseValidate docs/formatdomain.html.in | 43 ++ docs/schemas/domaincommon.rng | 35 src/conf/domain_conf.c | 95 +- src/qemu/qemu_command.c| 6 ++ src/util/virstoragefile.c | 3 + src/util/virstoragefile.h | 6 ++ .../qemuxml2argv-disk-drive-qcow2-cache.xml| 43 ++ .../qemuxml2xmlout-disk-drive-qcow2-cache.xml | 43 ++ tests/qemuxml2xmltest.c| 1 + 9 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8ca7637..245d5c4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3036,6 +3036,49 @@ set. (Since 3.5.0) +The drive element may contain a qcow2 sub element, which +allows to specifying further details related to qcow2 driver type. +Since 3.8.0 + + +The optional l2_cache_size attribute controls how much +memory will be consumed by qcow2 l2 table cache in bytes. This +option is only valid when the driver type is qcow2. The default +size is 2097152. The amount of virtual disk that can be mapped by +the L2 caches (in bytes) is: + disk_size = l2_cache_size * cluster_size / 8 +Since 3.8.0 + +In general you should leave this option alone, unless you +are very certain you know what you are doing. + + +The optional refcount_cache_size attribute controls +how much memory will be consumed by qcow2 reference count table +cache in bytes. This option is only valid when the driver type is +qcow2. The default size is 262144. The amount of virtual disk that +can be mapped by the refcount caches (in bytes) is: + disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits +Since 3.8.0 + +In general you should leave this option alone, unless you +are very certain you know what you are doing. + + +The optional cache_clean_interval attribute defines +an interval (in seconds). All cache entries that haven't been +accessed during that interval are removed from memory. This option +is only valid when the driver type is qcow2. The default +value is 0, which disables this feature. If the interval is too +short, it will cause frequent cache write back and load, which +impact performance. If the interval is too long, unused cache +will still consume memory until it's been written back to disk. +Since 3.8.0 + +In general you should leave this option alone, unless you +are very certain you know what you are doing. + + backenddomain The optional backenddomain element allows specifying a diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 06c5a91..ec44e58 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1762,6 +1762,23 @@ + + + + + + + + + + + + + + + @@ -1800,6 +1817,9 @@ + + + @@ -1895,6 +1915,21 @@ + + + + + + + + + + + + + + + diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2fc1fc3..c7d4b9b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5673,6 +567
[libvirt] [PATCH 2/2] qemu: add capability checking for qcow2 cache configuration
Add qemu capabilities QEMU_CAPS_L2_CACHE_SIZE,
QEMU_CAPS_REFCOUNT_CACHE_SIZE, QEMU_CAPS_CACHE_CLEAN_INTERVAL.
Add testing for the above qemu capabilities.
Signed-off-by: Liu Qing
---
Patch 2 - a) use QMP instead of qemu version to identify qemu capabilities.
b) change capablity names with prefix drive.qcow2.
c) merge the three test cases to one.
src/qemu/qemu_capabilities.c | 9 +
src/qemu/qemu_capabilities.h | 3 ++
src/qemu/qemu_command.c| 39 ++
tests/qemucapabilitiesdata/caps_2.5.0.x86_64.xml | 3 ++
.../caps_2.6.0-gicv2.aarch64.xml | 3 ++
.../caps_2.6.0-gicv3.aarch64.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml| 3 ++
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml| 3 ++
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 3 ++
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml| 3 ++
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 3 ++
.../qemuxml2argv-disk-drive-qcow2-cache.args | 28
tests/qemuxml2argvtest.c | 4 +++
17 files changed, 113 insertions(+), 6 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.args
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e7ea6f4..e8cce38 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -439,6 +439,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"virtio-net.tx_queue_size",
"chardev-reconnect",
"virtio-gpu.max_outputs",
+ "drive.qcow2.l2-cache-size",
+ "drive.qcow2.refcount-cache-size",
+ "drive.qcow2.cache-clean-interval",
);
@@ -1811,6 +1814,12 @@ static struct virQEMUCapsStringFlags
virQEMUCapsObjectPropsIntelIOMMU[] = {
static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = {
{ "blockdev-add/arg-type/options/+gluster/debug-level",
QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
{ "blockdev-add/arg-type/+gluster/debug", QEMU_CAPS_GLUSTER_DEBUG_LEVEL},
+{ "blockdev-add/arg-type/options/+qcow2/l2-cache-size",
QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE},
+{ "blockdev-add/arg-type/options/+qcow2/refcount-cache-size",
QEMU_CAPS_DRIVE_QCOW2_REFCOUNT_CACHE_SIZE},
+{ "blockdev-add/arg-type/options/+qcow2/cache-clean-interval",
QEMU_CAPS_DRIVE_QCOW2_CACHE_CLEAN_INTERVAL},
+{ "blockdev-add/arg-type/+qcow2/l2-cache-size",
QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE},
+{ "blockdev-add/arg-type/+qcow2/refcount-cache-size",
QEMU_CAPS_DRIVE_QCOW2_REFCOUNT_CACHE_SIZE},
+{ "blockdev-add/arg-type/+qcow2/cache-clean-interval",
QEMU_CAPS_DRIVE_QCOW2_CACHE_CLEAN_INTERVAL},
};
struct virQEMUCapsObjectTypeProps {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f32687d..81fb759 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -425,6 +425,9 @@ typedef enum {
QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE, /* virtio-net-*.tx_queue_size */
QEMU_CAPS_CHARDEV_RECONNECT, /* -chardev reconnect */
QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS, /* -device
virtio-(vga|gpu-*),max-outputs= */
+QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE, /* -drive support qcow2 l2 cache size
*/
+QEMU_CAPS_DRIVE_QCOW2_REFCOUNT_CACHE_SIZE, /* -drive support qcow2
refcount cache size */
+QEMU_CAPS_DRIVE_QCOW2_CACHE_CLEAN_INTERVAL, /* -drive qcow2 cache clean
interval */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 391aaba..e2592e0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1433,12 +1433,39 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
qemuformat = "luks";
virBufferAsprintf(buf, "format=%s,", qemuformat);
}
-if (disk->src->l2_cache_size > 0)
-virBufferAsprintf(buf, "l2-cache-size=%llu,",
disk->src->l2_cache_size);
-if (disk->src->refcount_cache_size > 0)
-virBufferAsprintf(buf, "refcount-cache-size=%llu,",
disk->src->refcount_cache_size);
-if (disk->src->cache_clean_interval > 0)
-virBufferAsprintf(buf, "cache-clean-interval=%llu,",
disk->src->cache_clean_interval);
+
+if (disk->src->l2_cache_size > 0) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE)) {
+virBufferAsprintf(buf, "l2-cache-size=%llu,",
+ disk->src->l2_cache_size);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("l2-cache-size is not supported by this Q
Re: [libvirt] Yet another RFC for CAT
2017-09-04 23:57 GMT+08:00 Daniel P. Berrange : > On Mon, Sep 04, 2017 at 04:14:00PM +0200, Martin Kletzander wrote: > > * The current design (finally something libvirt-related, right?) > > > > The discussion ended with a conclusion of the following (with my best > > knowledge, there were so many discussions about so many things that I > > would spend too much time looking up all of them): > > > > - Users should not need to specify bit masks, such complexity should be > > abstracted. We'll use sizes (e.g. 4MB) > > > > - Multiple vCPUs might need to share the same allocation. > > > > - Exclusivity of allocations is to be assumed, that is only unoccupied > > cache should be used for new allocations. > > > > The last point seems trivial but it's actually very specific condition > > that, if removed, can cause several problems. If it's hard to grasp the > > last point together with the second one, you're on the right track. If > > not, then I'll try to make a point for why the last point should be > > removed in 3... 2... 1... > > > > * Design flaws > > > > 1) Users cannot specify any allocation that would share only part with > >some other allocation of the domain or the default group. > > > yep, There's no share cache ways support. I was thinking that create a cache resource group in libvirt, and user can add vms into that resource group, this is good for those who would like to have share cache resource, maybe NFV case. but for case: VM1: fff00 VM2: 00fff which have a `f` (4 cache ways) share, seems have not really meanful. at least, I don't heart that we have that case. This was mentioned by *Marcelo Tosatti *before too. > 2) It was not specified what to do with the default resource group. > >There might be several ways to approach this, with varying pros and > >cons: > > > > a) Treat it as any other group. That is any bit set for this group > >will be excluded from usable bits when creating new allocation > >for a domain. > > > > - Very predictable behaviour > > > > - You will not be able to allocate any amount of cache without > > previous setting for the default group as that will have all > > the bits set which will make all the cache unusable > > > > b) Automatically remove the appropriate amount of bits that are > >needed for new domains. > > > > - No need to do any change to the system settings in order to > > use this new feature > > > > - We would have to change system settings, which is generally > > frowned upon when done "automatically" as a side effect of > > starting a domain, especially for such scarce resource as > > cache > > > > - The change to system settings would not be entirely > > predictable > > > > c) Act like it doesn't exist and don't remove its allocations from > >consideration > > > > - Doesn't really make sense as system processes might be > > trashing the cache as any VM, moreover when all VM processes > > without allocations will be based in the default group as > > well > > > > 3) There is no way for users to know what the particular settings are > >for any running domain. > I think you are going to expose what the current CBM looks like for a given VM? That's fair enough. > > > > The first point was deemed a corner case. Fair enough on its own, but > > considering point 2 and its solutions, it is rather difficult for me to > > justify it. Also, let's say you have domain with 4 vCPUs out of which > > you know 1 might be trashing the cache, but you don't want to restrict > > it completely, but others will utilize it very nicely. Sensible > > allocations for such domain's vCPUs might be: > > > > vCPU 0: 000f > > vCPUs 1-3: > > > > as you want vCPUs 1-3 to utilize even the part of cache that might get > > trashed by vCPU 0. Or they might share some data (especially > > guest-memory-related). > > > > The case above is not possible to set up with only per-vcpu(s) scalar > > setting. And there are more as you might imagine now. For example how > > do we behave with iothreads and emulator threads? > > This is kinds of hard to implement, but possible. is 1:1 mapping of resource group to VM? if you want to have iothreads and emulator threads to have separated cache allocation, you may need to create resource group to associated with VM's vcpus and iothreads and emulator thread. but COS number is limited, does it worth to have so fine granularity control? > Ok, I see what you're getting at. I've actually forgotten what > our current design looks like though :-) > > What level of granularity were we allowing within a guest ? > All vCPUs use separate cache regions from each other, or all > vCPUs use a share cached region, but separate from other guests, > or a mix ? > > > * My suggestion: > > > > - Provide an API for querying and changing the
Re: [libvirt] [PATCH 2/2] qemu: Order PCI controllers on the command line
On 09/05/2017 09:25 AM, Andrea Bolognani wrote:
> Unlike other controllers, PCI controller can plug into each other,
s/PCI controller/PCI controllers/
> which might turn into a problem if they are not listed in the
> expected order on the QEMU command line, because the guest will
> then not be able to start with an error such as
>
> qemu-kvm: -device pci-bridge,chassis_nr=3,id=pci.3,bus=pci.2.0,addr=0x7:
> Bus 'pci.2.0' not found
>
> Add some logic to make sure PCI controller appear in the correct
s/PCI controller/PCI controllers/
> order on the QEMU command line, regardless of the order they were
> added to the guest configuration.
Before examining the code, one concern I had was whether an
*unnecessary* re-ordering would cause a guest ABI change. So I decided
to try testing it by making an XML file with the pci controller indexes
out of order. Unfortunately, when you try to do that, libvirt ends up
inserting the controllers into its internal list in sorted order, so
it's not even possible to force/fake libvirt into putting the
controllers in the wrong order - no matter what you try, the following
will happen:
1) if the indexes are out of order in the input XML, libvirt will
re-order them as it parses.
2) if any pci controller's address (the slot it's plugged into) has a
bus attribute that is higher than that controller's own index, libvirt
logs an error and fails the operation.
So under normal circumstances, I think that the situation you're talking
about can't happen.
*BUT*
The one exception to this is when libvirt auto-adds controllers in order
to fill in unused indexes (e.g. if you have a pci controller with
index='8', but none with index='7', you'll automatically get an extra
controller added with index='7') or to provide a controller at the
proper index for a device that has a PCI address pointing to a
non-existent bus (this is the bug described in the referenced BZ). The
problem is that, when libvirt auto-adds these new controllers to the
controller array, it just appends them to the end, rather than inserting
them in proper sorted order. THAT is what leads to the problem. (as a
matter of fact, the next time you edit the domain in *any way*, the
issue is resolved because the parser re-orders the list of controllers).
I haven't tried it, but it looks to me like this could all be fixed by
simply changing the function virDomainDefAddController() to call
virDomainControllerInsert() to add the new controller at the proper
(sorted) place in the controller array instead of using
VIR_APPEND_ELEMENT_COPY() (which is what it does now). That has the nice
side effect of keeping the controllers listed in the XML in sorted
order, and making sure that the XML doesn't magically change when you
edit something unrelated.
I don't doubt that the code you've written below fixes the problem at
the time the commandline is generated (haven't tried it either :-), but
I think it's better to fix it back at its source, when the controllers
are auto-added.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1479674
>
> Signed-off-by: Andrea Bolognani
> ---
> src/qemu/qemu_command.c| 72
> --
> .../qemuxml2argv-pci-autoadd-idx.args | 2 +-
> .../qemuxml2argv-pseries-many-buses-2.args | 4 +-
> 3 files changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 83430b33f..c0e60a184 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3096,6 +3096,8 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
>virQEMUCapsPtr qemuCaps)
> {
> size_t i, j;
> +size_t *pciContIndex;
> +size_t npciContIndex;
> int usbcontroller = 0;
> bool usblegacy = false;
> int contOrder[] = {
> @@ -3107,14 +3109,10 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> * machines. For newer Q35 machines it is added out of the
> * controllers loop, after the floppy drives.
> *
> - * We don't add PCI/PCIe root controller either, because it's
> - * implicit, but we do add PCI bridges and other PCI
> - * controllers, so we leave that in to check each
> - * one. Likewise, we don't do anything for the primary IDE
> + * Likewise, we don't do anything for the primary IDE
> * controller on an i440fx machine or primary SATA on q35, but
> * we do add those beyond these two exceptions.
> */
> -VIR_DOMAIN_CONTROLLER_TYPE_PCI,
> VIR_DOMAIN_CONTROLLER_TYPE_USB,
> VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
> VIR_DOMAIN_CONTROLLER_TYPE_IDE,
> @@ -3124,6 +3122,54 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> };
> int ret = -1;
>
> +/* PCI controllers require special handling because they plug into
> + * each other, which makes getting the ordering right on the QEMU
> +
[libvirt] [PATCH] storage: Check and possibly update config file after build
https://bugzilla.redhat.com/show_bug.cgi?id=1464313
As it turns out, the on-disk config file (e.g. not the stateDir file)
needs to be updated when --override is provided since it's possible and
highly probable that the def->source.format has been adjusted and could
cause a future start after perhaps a libvirtd restart to have the older
format from a define operation from the backend.
So in the 2 places where it's possible a write would be needed (create
after define and build), let's perform a check and do the save/write
operation on the configFile if it's necessary.
Signed-off-by: John Ferlan
---
src/storage/storage_driver.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7cf5943..afb0404 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -651,6 +651,22 @@ storagePoolIsPersistent(virStoragePoolPtr pool)
}
+/* After a pool build, it's possible the inactive configFile needs to
+ * be updated especially since overwriting the pool more than likely
+ * changes the source format and may change/update a few other fields. */
+static int
+storagePoolBuildCheckUpdateConfig(virStoragePoolObjPtr obj,
+ unsigned int flags)
+{
+int ret = 0;
+
+if ((flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && obj->configFile)
+ret = virStoragePoolSaveConfig(obj->configFile, obj->def);
+
+return ret;
+}
+
+
static virStoragePoolPtr
storagePoolCreateXML(virConnectPtr conn,
const char *xml,
@@ -916,6 +932,9 @@ storagePoolCreate(virStoragePoolPtr pool,
if (backend->buildPool(pool->conn, obj, build_flags) < 0)
goto cleanup;
}
+
+if (storagePoolBuildCheckUpdateConfig(obj, build_flags) < 0)
+goto cleanup;
}
VIR_INFO("Starting up storage pool '%s'", obj->def->name);
@@ -980,6 +999,10 @@ storagePoolBuild(virStoragePoolPtr pool,
if (backend->buildPool &&
backend->buildPool(pool->conn, obj, flags) < 0)
goto cleanup;
+
+if (storagePoolBuildCheckUpdateConfig(obj, flags) < 0)
+goto cleanup;
+
ret = 0;
cleanup:
--
2.9.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Be more selective when determining cdrom for taint messaging
On 09/06/2017 08:17 AM, Michal Privoznik wrote:
> On 09/05/2017 10:51 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1471225
>>
>> Commit id '99a2d6af2' was a bit too aggressive with determining whether
>> the provided path was a "physical" cd-rom in order to generate a taint
>> message due to the possibility of some guest and host trying to control
>> the tray. For cd-rom guest devices backed to some VIR_STORAGE_TYPE_FILE
>> storage, this wouldn't be a problem and as such it shouldn't be a problem
>> for guest devices using some sort of block device on the host such as
>> iSCSI, LVM, or a Disk pool would present.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/libvirt_private.syms | 1 +
>> src/qemu/qemu_domain.c | 2 +-
>> src/util/virfile.c | 48
>>
>> src/util/virfile.h | 4
>> 4 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index f30a04b..0354568 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1680,6 +1680,7 @@ virFileGetMountSubtree;
>> virFileHasSuffix;
>> virFileInData;
>> virFileIsAbsPath;
>> +virFileIsCDROM;
>> virFileIsDir;
>> virFileIsExecutable;
>> virFileIsLink;
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 9cff501..426c577 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -4807,7 +4807,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr
>> driver,
>>
>> if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
>> virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK
>> &&
>> -disk->src->path)
>> +disk->src->path && virFileIsCDROM(disk->src->path))
>> qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH,
>> logCtxt);
>>
>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index 2f28e83..4c31949 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -4166,3 +4166,51 @@ virFileReadValueString(char **value, const char
>> *format, ...)
>> VIR_FREE(str);
>> return ret;
>> }
>> +
>> +
>> +#if defined(__linux__)
>> +
>> +/* virFileIsCDROM
>> + * @path: Supplied path.
>> + *
>> + * Determine if the path is a CD-ROM path. Typically on Linux systems this
>> + * is either /dev/cdrom or /dev/sr0, so those are easy checks. Still if
>> + * someone is trying to be tricky, we can resolve the link to /dev/cdrom
>> + * and compare it to the resolved link of the supplied @path to compare
>> + * if they're the same.
>> + *
>> + * Returns true if the path is a CDROM, false otherwise.
>> + */
>> +bool
>> +virFileIsCDROM(const char *path)
>> +{
>> +bool ret = false;
>> +char *linkpath = NULL;
>> +char *cdrompath = NULL;
>> +
>> +if (STREQ(path, "/dev/cdrom") || STREQ(path, "/dev/sr0"))
>> +return true;
>
> What if I have two CDROMs? /dev/sr0 and /dev/sr1? I'm worried that name
> match is not sufficient and we need to lstat() and check if the major
> number (st.st_rdev) is 11 (0xb) (according to
> Documentation/admin-guide/devices.txt from kernel sources). And even
> that might be not enough :(
>
Ironically I had "STRPREFIX(path, "/dev/sr") originally, but at the last
moment switched to /dev/sr0. I also considered the stat option for the
major number. Could do the same STRPREFIX for /dev/cdrom too.
In a way, I was hoping to get more ideas from the community on this. Are
we always "sure" that CD-ROM's will start with /dev/sr? That was
something else I couldn't tell. And there's so much "history" out there
in google land that one could spend way too much time researching for
all the crazy ways to name a cdrom (the path stuff below was mainly a
reaction to something I read, but I forget where). I did check QEMU and
ironically sources there are included to use /dev/cdrom.
Other options I considered, but felt were a bit over the top/excessive:
1. Could defer to nodedev - it would be able to "know" which devices
are cdrom... stores that in ... could add a capability
'cdrom' that would also returning a list of cdrom devices, then compare
the resolved path against that list.
2. Could use something like 'processLU', but only for SCSI devices...
Essentially ends up searching for a 0x5 in the
"/sys/bus/scsi/devices/*/type", then again comparing resolved paths
against the list returned.
Neither of the options was palatable for the "benefit" gained of being
able to further filter and decide which which paths wouldn't get the
tainted message.
>> +
>> +if (virFileResolveLink(path, &linkpath) < 0 ||
>> +virFileResolveLink("/dev/cdrom", &cdrompath) < 0)
>> +goto cleanup;
>
> This will only work for cases where /dev/cdrom points to the device in
> @path. However, if /dev/cdrom is pointing to /dev/sr0 and I'm calling
> this function over /dev/sr1 (because I have a machine with dozens of
[libvirt] [PATCH] qemu: Provide default LUN=0 for iSCSI if not provided
https://bugzilla.redhat.com/show_bug.cgi?id=1477880
If the "/#" is missing from the provided iSCSI path, then we need
to provide the default LUN of /0; otherwise, QEMU will fail to parse
the URL causing a failure to either create the guest or hotplug
attach the storage.
Alter the command lines generated appropriately. This change does not
effect the XML input files.
One 'side effect' of this for the argv2xml is that if a "/0" is found
on the command line, then the generated XML would need to add the "/0"
to the output XML since it wouldn't be known whether it existed or not
at qemu process creation time.
Signed-off-by: John Ferlan
---
NB: Investigated the history quite a bit between QEMU and libiscsi and
it seems as though this probably never worked and of course never "tested"
in a real environment. Ironically, in qapi/block-core.json, it is claimed
that the LUN would default to 0, but the block/iscsi.c code that calls
iscsi_parse_full_url would never (AFAICT) have supported not supplying
some LUN on the command line.
src/qemu/qemu_command.c | 20
.../qemuargv2xml-disk-drive-network-iscsi.args | 2 +-
.../qemuargv2xml-disk-drive-network-iscsi.xml| 2 +-
.../qemuxml2argv-disk-drive-network-iscsi-lun.args | 2 +-
.../qemuxml2argv-disk-drive-network-iscsi.args | 2 +-
.../qemuxml2argv-hostdev-scsi-lsi-iscsi.args | 2 +-
.../qemuxml2argv-hostdev-scsi-virtio-iscsi.args | 2 +-
7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ed8cb6e..cff84de 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -830,10 +830,22 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
src->volume, src->path) < 0)
goto cleanup;
} else {
-if (virAsprintf(&uri->path, "%s%s",
-src->path[0] == '/' ? "" : "/",
-src->path) < 0)
-goto cleanup;
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
+!strchr(src->path, '/')) {
+
+/* The details of iqn is defined by RFC 3720 and 3721, but
+ * we just need to make sure there's a lun provided. If not
+ * provided, then default to zero. If ISCSI LUN is provided
+ * by /dev/disk/by-path/... , then that path will have the
+ * specific lun requested. */
+if (virAsprintf(&uri->path, "/%s/0", src->path) < 0)
+goto cleanup;
+} else {
+if (virAsprintf(&uri->path, "%s%s",
+src->path[0] == '/' ? "" : "/",
+src->path) < 0)
+goto cleanup;
+}
}
}
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args
b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args
index 62cc5c0..ec2e8dc 100644
--- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args
+++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.args
@@ -16,7 +16,7 @@ QEMU_AUDIO_DRV=none \
-no-acpi \
-boot c \
-usb \
--drive file=iscsi://example.org:6000/iqn.1992-01.com.example,format=raw,\
+-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/0,format=raw,\
if=virtio \
-drive file=iscsi://example.org:6000/iqn.1992-01.com.example/1,format=raw,\
if=virtio \
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml
b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml
index 23542fa..694412b 100644
--- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml
+++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi.xml
@@ -16,7 +16,7 @@
/usr/bin/qemu-system-i686
-
+
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
index b25f3a0..0fbfd9a 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-lun.args
@@ -21,7 +21,7 @@ server,nowait \
-boot c \
-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \
-usb \
--drive file=iscsi://example.org:3260/iqn.1992-01.com.example,format=raw,\
+-drive file=iscsi://example.org:3260/iqn.1992-01.com.example/0,format=raw,\
if=none,id=drive-scsi0-0-0-0 \
-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\
drive=drive-scsi0-0-0-0,id=scsi0-0-0-0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
index a1d93af..ed15fda 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk
Re: [libvirt] [PATCH 1/2] qemu: Clean up qemuBuildControllerDevCommandLine()
On 09/05/2017 09:25 AM, Andrea Bolognani wrote:
> Add a cleanup: label, which will be used later, and improve
> the readability of one of the checks by making it conform to
> our formatting standard and moving a comment.
>
> Signed-off-by: Andrea Bolognani
> ---
Reviewed-by: Laine Stump
(or "ACK" if you prefer. I'm still unclear who is on which side of this
debate, and why.)
> src/qemu/qemu_command.c | 19 ---
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9a27987d4..83430b33f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3122,6 +3122,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL,
> VIR_DOMAIN_CONTROLLER_TYPE_CCID,
> };
> +int ret = -1;
>
> for (j = 0; j < ARRAY_CARDINALITY(contOrder); j++) {
> for (i = 0; i < def->ncontrollers; i++) {
> @@ -3183,7 +3184,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Multiple legacy USB controllers are "
> "not supported"));
> -return -1;
> +goto cleanup;
> }
> usblegacy = true;
> continue;
> @@ -3191,7 +3192,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
>
> if (qemuBuildControllerDevStr(def, cont, qemuCaps,
>&devstr, &usbcontroller) < 0)
> -return -1;
> +goto cleanup;
>
> if (devstr) {
> virCommandAddArg(cmd, "-device");
> @@ -3201,16 +3202,20 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
> }
> }
>
> -/* We haven't added any USB controller yet, but we haven't been asked
> - * not to add one either. Add a legacy USB controller, unless we're
> - * creating a kind of guest we want to keep legacy-free */
> if (usbcontroller == 0 &&
> !qemuDomainIsQ35(def) &&
> !qemuDomainIsVirt(def) &&
> -!ARCH_IS_S390(def->os.arch))
> +!ARCH_IS_S390(def->os.arch)) {
> +/* We haven't added any USB controller yet, but we haven't been asked
> + * not to add one either. Add a legacy USB controller, unless we're
> + * creating a kind of guest we want to keep legacy-free */
> virCommandAddArg(cmd, "-usb");
> +}
>
> -return 0;
> +ret = 0;
> +
> + cleanup:
> +return ret;
> }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] travis: Install gettext
On Wed, Sep 06, 2017 at 03:13:30PM +0200, Andrea Bolognani wrote: > msgmerge(1) and friends are required to build libvirt, so the > corresponding package should be installed in the Travis worker. It is only used if you regenerate .po files. 99% of the time developers or apps bulding libvirt won't do that, avoiding any need for msgmerge. 'make dist' triggers msgmerge, but we don't run that in travis - only make & make check 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] travis: Install gettext
msgmerge(1) and friends are required to build libvirt, so the corresponding package should be installed in the Travis worker. Signed-off-by: Andrea Bolognani --- I'm honestly baffled it ever worked at all. Pushed as a CI build fix. .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 4c9f94573..f59bd19c8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -11,6 +11,7 @@ addons: - autopoint - dnsmasq-base # - dwarves + - gettext - libapparmor-dev - libaudit-dev - libavahi-client-dev -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] travis: Fix build issues
I can't believe there is no blurb here! Andrea Bolognani (2): travis: Sort build dependencies travis: Install gettext .travis.yml | 49 + 1 file changed, 25 insertions(+), 24 deletions(-) -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] travis: Sort build dependencies
Keeping the list of build dependencies sorted alphabetically makes it way easier to visually scan it for issues. Signed-off-by: Andrea Bolognani --- Pushed as a CI build fix. .travis.yml | 48 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/.travis.yml b/.travis.yml index f05ba8454..4c9f94573 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,38 +6,38 @@ compiler: cache: ccache addons: apt: +# Please keep this list sorted alphabetically packages: - - xsltproc - autopoint - - libxml2-dev - - libncurses5-dev - - libreadline-dev - - zlib1g-dev - - libgnutls-dev - - libgcrypt11-dev + - dnsmasq-base +# - dwarves + - libapparmor-dev + - libaudit-dev - libavahi-client-dev - - libsasl2-dev - - libxen-dev - - lvm2 - - libgcrypt11-dev - - libparted0-dev - - libdevmapper-dev - - uuid-dev - - libudev-dev - - libpciaccess-dev - libcap-ng-dev + - libdevmapper-dev + - libgcrypt11-dev + - libgnutls-dev + - libncurses5-dev + - libnetcf-dev - libnl-3-dev - libnl-route-3-dev - - libyajl-dev - - libpcap0.8-dev - libnuma-dev - - libnetcf-dev - - libaudit-dev -# - dwarves - - libxml2-utils - - libapparmor-dev - - dnsmasq-base + - libparted0-dev + - libpcap0.8-dev + - libpciaccess-dev - librbd-dev + - libreadline-dev + - libsasl2-dev + - libudev-dev + - libxen-dev + - libxml2-dev + - libxml2-utils + - libyajl-dev + - lvm2 + - uuid-dev + - xsltproc + - zlib1g-dev notifications: irc: -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt domain memory statistics
Hi, On 2017-09-06 14:42, Michal Privoznik wrote: > On 09/06/2017 01:09 PM, Marko Myllynen wrote: > > Sure. It should be fairly simple. Mind opening a bug for that so that we > can track the RFE? Also, if you're willing to provide patches please be > my guest. Otherwise we will assign the work internally. Ok, I filed: https://bugzilla.redhat.com/show_bug.cgi?id=1488895 FWIW, this would be for the PCP libvirt agent (PMDA): https://github.com/performancecopilot/pcp/blob/master/src/pmdas/libvirt/pmdalibvirt.python Thanks, -- Marko Myllynen -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Be more selective when determining cdrom for taint messaging
On 09/05/2017 10:51 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1471225
>
> Commit id '99a2d6af2' was a bit too aggressive with determining whether
> the provided path was a "physical" cd-rom in order to generate a taint
> message due to the possibility of some guest and host trying to control
> the tray. For cd-rom guest devices backed to some VIR_STORAGE_TYPE_FILE
> storage, this wouldn't be a problem and as such it shouldn't be a problem
> for guest devices using some sort of block device on the host such as
> iSCSI, LVM, or a Disk pool would present.
>
> Signed-off-by: John Ferlan
> ---
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_domain.c | 2 +-
> src/util/virfile.c | 48
>
> src/util/virfile.h | 4
> 4 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index f30a04b..0354568 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1680,6 +1680,7 @@ virFileGetMountSubtree;
> virFileHasSuffix;
> virFileInData;
> virFileIsAbsPath;
> +virFileIsCDROM;
> virFileIsDir;
> virFileIsExecutable;
> virFileIsLink;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9cff501..426c577 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4807,7 +4807,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr
> driver,
>
> if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
> virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
> -disk->src->path)
> +disk->src->path && virFileIsCDROM(disk->src->path))
> qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH,
> logCtxt);
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 2f28e83..4c31949 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -4166,3 +4166,51 @@ virFileReadValueString(char **value, const char
> *format, ...)
> VIR_FREE(str);
> return ret;
> }
> +
> +
> +#if defined(__linux__)
> +
> +/* virFileIsCDROM
> + * @path: Supplied path.
> + *
> + * Determine if the path is a CD-ROM path. Typically on Linux systems this
> + * is either /dev/cdrom or /dev/sr0, so those are easy checks. Still if
> + * someone is trying to be tricky, we can resolve the link to /dev/cdrom
> + * and compare it to the resolved link of the supplied @path to compare
> + * if they're the same.
> + *
> + * Returns true if the path is a CDROM, false otherwise.
> + */
> +bool
> +virFileIsCDROM(const char *path)
> +{
> +bool ret = false;
> +char *linkpath = NULL;
> +char *cdrompath = NULL;
> +
> +if (STREQ(path, "/dev/cdrom") || STREQ(path, "/dev/sr0"))
> +return true;
What if I have two CDROMs? /dev/sr0 and /dev/sr1? I'm worried that name
match is not sufficient and we need to lstat() and check if the major
number (st.st_rdev) is 11 (0xb) (according to
Documentation/admin-guide/devices.txt from kernel sources). And even
that might be not enough :(
> +
> +if (virFileResolveLink(path, &linkpath) < 0 ||
> +virFileResolveLink("/dev/cdrom", &cdrompath) < 0)
> +goto cleanup;
This will only work for cases where /dev/cdrom points to the device in
@path. However, if /dev/cdrom is pointing to /dev/sr0 and I'm calling
this function over /dev/sr1 (because I have a machine with dozens of
CDROMs) this function returns false which is obviously wrong.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] cpu: Add new EPYC CPU model
On Wed, Aug 23, 2017 at 13:49:41 -0500, Brijesh Singh wrote: > Add a new CPU model called 'EPYC' to model processors from AMD EPYC > family (which includes EPYC 76xx,75xx,74xx, 73xx and 72xx). > > The following features bits have been added/removed compare to Opteron_G5 > > Added: monitor, movbe, rdrand, mmxext, ffxsr, rdtscp, cr8legacy, osvw, >fsgsbase, bmi1, avx2, smep, bmi2, rdseed, adx, smap, clfshopt, sha >xsaveopt, xsavec, xgetbv1, arat > > Removed: xop, fma4, tbm > > The patch is depend on EPYC CPU model supported introduced in qemu [1] > > [1] https://patchwork.kernel.org/patch/9902205/ > > Cc: Tom Lendacky > Signed-off-by: Brijesh Singh > --- > src/cpu/cpu_map.xml | 74 > + > 1 file changed, 74 insertions(+) > > diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml > index 8e7ac49..522d66b 100644 > --- a/src/cpu/cpu_map.xml > +++ b/src/cpu/cpu_map.xml > @@ -251,6 +251,9 @@ > > > > + QEMU calls this feature "sha-ni". > + > + The CPU features in cpu_map.xml are sorted by CPUID bits, which means this new feature should go between avx512cd and avx512bw. > > > > @@ -1545,6 +1548,77 @@ > > > > + > + > + > + > + > + > + ... We list model's features sorted by name. ACK with the issues fixed. However, it would be nice to add some CPUID data to our test suite. Luckily enough I have such data so I'll just resend [1] a fixed version of this patch with the test data added. Jirka [1] https://www.redhat.com/archives/libvir-list/2017-September/msg00111.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/2] cpu: Add new EPYC CPU model
From: Brijesh Singh Add a new CPU model called 'EPYC' to model processors from AMD EPYC family (which includes EPYC 76xx,75xx,74xx, 73xx and 72xx). The following features bits have been added/removed compare to Opteron_G5 Added: monitor, movbe, rdrand, mmxext, ffxsr, rdtscp, cr8legacy, osvw, fsgsbase, bmi1, avx2, smep, bmi2, rdseed, adx, smap, clfshopt, sha xsaveopt, xsavec, xgetbv1, arat Removed: xop, fma4, tbm The patch is depend on EPYC CPU model supported introduced in qemu [1] [1] https://patchwork.kernel.org/patch/9902205/ Cc: Tom Lendacky Signed-off-by: Brijesh Singh Signed-off-by: Jiri Denemark --- src/cpu/cpu_map.xml| 74 ++ ...x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml | 26 +--- .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 33 +- .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml | 27 +--- 4 files changed, 79 insertions(+), 81 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 8e7ac4973d..6243fbe902 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -260,6 +260,9 @@ + + + @@ -1545,6 +1548,77 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml index fcad36e34e..18edb71bcd 100644 --- a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml +++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml @@ -1,32 +1,11 @@ - Opteron_G5 + EPYC AMD - - - - - - - - - - - - - - - - - - - - - @@ -34,7 +13,4 @@ - - - diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml index 19f6b1d4df..54afdea4d4 100644 --- a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml +++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml @@ -1,43 +1,12 @@ x86_64 - Opteron_G3 + EPYC AMD - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml index 2773147da6..32064548c7 100644 --- a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml +++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml @@ -1,32 +1,11 @@ - Opteron_G5 + EPYC AMD - - - - - - - - - - - - - - - - - - - - - - - + + -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/2] tests: Add CPUID data for AMD Ryzen 7 1800X Eight-Core Processor
Signed-off-by: Jiri Denemark
---
tests/cputest.c| 1 +
..._64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml | 9 +
...6_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml | 10 +
...x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml | 40
.../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 48 +
.../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml | 32
.../x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json | 203 +
.../x86_64-cpuid-Ryzen-7-1800X-Eight-Core.xml | 52 ++
8 files changed, 395 insertions(+)
create mode 100644
tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml
create mode 100644
tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml
create mode 100644
tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml
create mode 100644
tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml
create mode 100644
tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml
create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json
create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.xml
diff --git a/tests/cputest.c b/tests/cputest.c
index ebcade6bed..6e00e6ba4e 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -1001,6 +1001,7 @@ mymain(void)
DO_TEST_CPUID(VIR_ARCH_X86_64, "Opteron-6282", false);
DO_TEST_CPUID(VIR_ARCH_X86_64, "Pentium-P6100", false);
DO_TEST_CPUID(VIR_ARCH_X86_64, "Phenom-B95", true);
+DO_TEST_CPUID(VIR_ARCH_X86_64, "Ryzen-7-1800X-Eight-Core", true);
DO_TEST_CPUID(VIR_ARCH_X86_64, "Xeon-5110", false);
DO_TEST_CPUID(VIR_ARCH_X86_64, "Xeon-E3-1245", true);
DO_TEST_CPUID(VIR_ARCH_X86_64, "Xeon-E5-2630", true);
diff --git
a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml
new file mode 100644
index 00..9ee1e78244
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml
@@ -0,0 +1,9 @@
+
+
+
+
+
+
+
+
+
diff --git
a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml
new file mode 100644
index 00..9c85bb63a2
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml
@@ -0,0 +1,10 @@
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml
new file mode 100644
index 00..fcad36e34e
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml
@@ -0,0 +1,40 @@
+
+ Opteron_G5
+ AMD
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml
new file mode 100644
index 00..19f6b1d4df
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml
@@ -0,0 +1,48 @@
+
+ x86_64
+ Opteron_G3
+ AMD
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml
new file mode 100644
index 00..2773147da6
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml
@@ -0,0 +1,32 @@
+
+ Opteron_G5
+ AMD
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json
b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json
new file mode 100644
index 00..7bb003d786
--- /dev/null
+++ b/tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json
@@ -0,0 +1,203 @@
+{
+ "return": {
+"model": {
+ "name": "base",
+ "props": {
+"pfthreshold": false,
+"pku": false,
+"rtm": false,
+"tsc_adjust": true,
+"tsc-deadline": true,
+"xstore-en": false,
+"tsc-scale": false,
+"sse": true,
+"smap": true,
+"stepping": 1,
+"tce": false,
+"kvm_steal_time": true,
+"smep": true,
+"rdpid": false,
+"xcrypt": false,
+"sse4_2": true,
+"monitor": false,
+"sse4_1": true,
+"kvm-mmu": false,
+"flushbyasid": false,
+"kvm-steal-time": true,
+"lm": true,
+"tsc": true,
+"adx": true,
+"fxsr": true,
+"sha-ni": false,
+"tm": false,
+"pclmuldq": true,
+"xgetbv1": true,
+"xstore": fals
[libvirt] [PATCH 0/2] Add new EPYC CPU model
Brijesh Singh (1): cpu: Add new EPYC CPU model Jiri Denemark (1): tests: Add CPUID data for AMD Ryzen 7 1800X Eight-Core Processor src/cpu/cpu_map.xml| 74 tests/cputest.c| 1 + ..._64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml | 9 + ...6_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml | 10 + ...x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml | 16 ++ .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 17 ++ .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml | 11 ++ .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json | 203 + .../x86_64-cpuid-Ryzen-7-1800X-Eight-Core.xml | 52 ++ 9 files changed, 393 insertions(+) create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-disabled.xml create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-enabled.xml create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-guest.xml create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core-json.xml create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.json create mode 100644 tests/cputestdata/x86_64-cpuid-Ryzen-7-1800X-Eight-Core.xml -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Exposing mem-path in domain XML
On Wed, Sep 06, 2017 at 01:35:45PM +0200, Michal Privoznik wrote:
> On 09/05/2017 04:07 PM, Daniel P. Berrange wrote:
> > On Tue, Sep 05, 2017 at 03:59:09PM +0200, Michal Privoznik wrote:
> >> On 07/28/2017 10:59 AM, Daniel P. Berrange wrote:
> >>> On Fri, Jul 28, 2017 at 10:45:21AM +0200, Michal Privoznik wrote:
> On 07/27/2017 03:50 PM, Daniel P. Berrange wrote:
> > On Thu, Jul 27, 2017 at 02:11:25PM +0200, Michal Privoznik wrote:
> >> Dear list,
> >>
> >> there is the following bug [1] which I'm not quite sure how to grasp.
> >> So
> >> there is this application/infrastructure called Kove [2] that allows
> >> you
> >> to have memory for your application stored on a distant host in network
> >> and basically fetch needed region on pagefault. Now imagine that
> >> somebody wants to use it for backing up domain memory. However, the way
> >> that the tool works is it has some kernel module and then some userland
> >> binary that is fed with the path of the mmaped file. I don't know all
> >> the details, but the point is, in order to let users use this we need
> >> to
> >> expose the paths for mem-path for the guest memory. I know we did not
> >> want to do this in the past, but now it looks like we don't have a way
> >> around it, do we?
> >
> > We don't want to expose the concept of paths in the XML because this is
> > a linux specific way to configure hugepages / shared memory. So we hide
> > the particular path used in the internal impl of the QEMU driver, and
> > or via the qemu.conf global config file. I don't really want to change
> > that approach, particularly if the only reason is to integrate with a
> > closed source binary like Kove.
>
> Yep, I agree with that. However, if you read the discussion in the
> linked bug you'll find that they need to know what file in the
> memory_backing_dir (from qemu.conf) corresponds to which domain. The
> reported suggested using UUID based filenames, which I fear is not
> enough because one can have multiple -s configured
> for their domain. But I guess we could go with:
>
> ${memory_backing_dir}/${domName}for generic memory
> ${memory_backing_dir}/${domName}_N for Nth
> >>>
> >>> This feels like it is going to lead to hell when you add in memory
> >>> hotplug/unplug, with inevitable races.
> >>>
> BTW: IIUC they want predictable names because they need to create the
> files before spawning qemu so that they are picked by qemu instead of
> using temporary names.
> >>>
> >>> I would like to know why they even need to associate particular memory
> >>> files with particular QEMU processes. eg if they're just exposing a
> >>> new type of tmpfs filesystem from the kernel why does it matter what
> >>> each file is used for.
> >>
> >> This might get you answer:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1461214#c4
> >>
> >> So the way I understand it is that they will create the files, and
> >> provide us with paths. So luckily, we don't have to make up the paths on
> >> our own.
> >
> > IOW it is pretending to be tmpfs except it is not behaving like tmpfs.
> > This doesn't really make me any more inclined to support this closed
> > source stuff in libvirt.
>
> Yeah, that's my feeling too. So, what about the following: let's assume
> they will fix their code so that it is proper tmpfs. Libvirt can then
> behave to it just like it is already doing so for hugetlbfs. For us
> it'll be just yet another type of hugepages. I mean, for hugepages we
> already create /hupages/mount/point/libvirt/$domain per each domain so
> the separation is there (even though this is considered internal impl),
> since it would be a proper tmpfs they can see the pid of qemu which is
> trying to mmap() (and take the name or whatever unique ID they want from
> there).
Yep, we can at least make a reasonable guarantee that all files belonging
to a single QEMU process will always be within the same sub-directory.
This allows the kmod to distinguish 2 files owned by separate VMs, from 2
files owned by the same VM and do what's needed. I don't see why it would
need to care about naming conventions beyond the layout.
> I guess what I'm trying to ask is if it was proper tmpfs, we would be
> okay with it, wouldn't we?
If it is indistinguishable from tmpfs/hugetlbfs from libvirt's POV, we
should be fine - at most you would need /etc/libvirt/qemu.conf change
to explicitly point at the custom mount point if libvirt doesn't
auto-detect the right one.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] libvirt domain memory statistics
On 09/06/2017 01:09 PM, Marko Myllynen wrote: > Hi, > > At least on RHEL 7 / libvirt-3.2 for domainListGetStats we only have the > following in libvirt.py: > > # virDomainStatsTypes > VIR_DOMAIN_STATS_STATE = 1 > VIR_DOMAIN_STATS_CPU_TOTAL = 2 > VIR_DOMAIN_STATS_BALLOON = 4 > VIR_DOMAIN_STATS_VCPU = 8 > VIR_DOMAIN_STATS_INTERFACE = 16 > VIR_DOMAIN_STATS_BLOCK = 32 > VIR_DOMAIN_STATS_PERF = 64 There's nothing more in the newer releases. > > Is there a reason memory statistics is not included here? Yes, nobody implemented it. We often implement features on demand. > Would it be > possible to allow querying memory statistics over domainListGetStats as > well in one operation? Sure. It should be fairly simple. Mind opening a bug for that so that we can track the RFE? Also, if you're willing to provide patches please be my guest. Otherwise we will assign the work internally. > Currently calls from Python are > needed to fetch memory statistics for all guests which may be a bit > costly in larger environments. Yep, agreed. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Exposing mem-path in domain XML
On 09/05/2017 04:07 PM, Daniel P. Berrange wrote:
> On Tue, Sep 05, 2017 at 03:59:09PM +0200, Michal Privoznik wrote:
>> On 07/28/2017 10:59 AM, Daniel P. Berrange wrote:
>>> On Fri, Jul 28, 2017 at 10:45:21AM +0200, Michal Privoznik wrote:
On 07/27/2017 03:50 PM, Daniel P. Berrange wrote:
> On Thu, Jul 27, 2017 at 02:11:25PM +0200, Michal Privoznik wrote:
>> Dear list,
>>
>> there is the following bug [1] which I'm not quite sure how to grasp. So
>> there is this application/infrastructure called Kove [2] that allows you
>> to have memory for your application stored on a distant host in network
>> and basically fetch needed region on pagefault. Now imagine that
>> somebody wants to use it for backing up domain memory. However, the way
>> that the tool works is it has some kernel module and then some userland
>> binary that is fed with the path of the mmaped file. I don't know all
>> the details, but the point is, in order to let users use this we need to
>> expose the paths for mem-path for the guest memory. I know we did not
>> want to do this in the past, but now it looks like we don't have a way
>> around it, do we?
>
> We don't want to expose the concept of paths in the XML because this is
> a linux specific way to configure hugepages / shared memory. So we hide
> the particular path used in the internal impl of the QEMU driver, and
> or via the qemu.conf global config file. I don't really want to change
> that approach, particularly if the only reason is to integrate with a
> closed source binary like Kove.
Yep, I agree with that. However, if you read the discussion in the
linked bug you'll find that they need to know what file in the
memory_backing_dir (from qemu.conf) corresponds to which domain. The
reported suggested using UUID based filenames, which I fear is not
enough because one can have multiple -s configured
for their domain. But I guess we could go with:
${memory_backing_dir}/${domName}for generic memory
${memory_backing_dir}/${domName}_N for Nth
>>>
>>> This feels like it is going to lead to hell when you add in memory
>>> hotplug/unplug, with inevitable races.
>>>
BTW: IIUC they want predictable names because they need to create the
files before spawning qemu so that they are picked by qemu instead of
using temporary names.
>>>
>>> I would like to know why they even need to associate particular memory
>>> files with particular QEMU processes. eg if they're just exposing a
>>> new type of tmpfs filesystem from the kernel why does it matter what
>>> each file is used for.
>>
>> This might get you answer:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1461214#c4
>>
>> So the way I understand it is that they will create the files, and
>> provide us with paths. So luckily, we don't have to make up the paths on
>> our own.
>
> IOW it is pretending to be tmpfs except it is not behaving like tmpfs.
> This doesn't really make me any more inclined to support this closed
> source stuff in libvirt.
Yeah, that's my feeling too. So, what about the following: let's assume
they will fix their code so that it is proper tmpfs. Libvirt can then
behave to it just like it is already doing so for hugetlbfs. For us
it'll be just yet another type of hugepages. I mean, for hugepages we
already create /hupages/mount/point/libvirt/$domain per each domain so
the separation is there (even though this is considered internal impl),
since it would be a proper tmpfs they can see the pid of qemu which is
trying to mmap() (and take the name or whatever unique ID they want from
there).
I guess what I'm trying to ask is if it was proper tmpfs, we would be
okay with it, wouldn't we?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] libvirt domain memory statistics
Hi, At least on RHEL 7 / libvirt-3.2 for domainListGetStats we only have the following in libvirt.py: # virDomainStatsTypes VIR_DOMAIN_STATS_STATE = 1 VIR_DOMAIN_STATS_CPU_TOTAL = 2 VIR_DOMAIN_STATS_BALLOON = 4 VIR_DOMAIN_STATS_VCPU = 8 VIR_DOMAIN_STATS_INTERFACE = 16 VIR_DOMAIN_STATS_BLOCK = 32 VIR_DOMAIN_STATS_PERF = 64 Is there a reason memory statistics is not included here? Would it be possible to allow querying memory statistics over domainListGetStats as well in one operation? Currently calls from Python are needed to fetch memory statistics for all guests which may be a bit costly in larger environments. Thanks, -- Marko Myllynen -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH python] Post-release version bump to 3.8.0
Signed-off-by: Daniel P. Berrange --- Pushed as trivial setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 6e5242d..f33ff1a 100755 --- a/setup.py +++ b/setup.py @@ -334,7 +334,7 @@ class my_clean(clean): _c_modules, _py_modules = get_module_lists() setup(name = 'libvirt-python', - version = '3.7.0', + version = '3.8.0', url = 'http://www.libvirt.org', maintainer = 'Libvirt Maintainers', maintainer_email = 'libvir-list@redhat.com', -- 2.13.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Validate device on update-device
On 09/06/2017 10:45 AM, Erik Skultety wrote:
> On Tue, Sep 05, 2017 at 04:37:23PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1439991
>>
>> Whenever a device is being updated via
>> virDomainUpdateDeviceFlags() API, we parse the device XML and
>> ideally run some generic checks to validate the configuration
>> (e.g. if device defines per-device boot order but the domain has
>> os/boot element already). Well, that's the theory - due to a
>> missing check we've jumped early from that check function.
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> src/conf/domain_conf.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f7574d77b..0064a71f5 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -26026,7 +26026,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>> {
>> virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
>>
>> -if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
>> +if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
>> +action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
>> return 0;
>
> ^Which means that now the 'compatibility' check is only skipped for _DETACH
> (which, for _DETACH, has been the case since @1c13166134). Therefore I think
> that codewise, even though you're change makes sense, it would be IMHO nicer
> to
> just drop this condition all together and also drop the call to this
> validation
> helper from the *Detach helper (with the appropriate tuning of the validator's
> signature of course).
Well, I might prefer that we call validation function even for detach
since I think it's more future proof (even though it's no-op currently),
but I don't stand so strong behind it. So okay, I'll remove the argument
and the call from *Detach helpers.
>
> With that adjustment:> Reviewed-by: Erik Skultety
Thanks,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Validate device on update-device
On Tue, Sep 05, 2017 at 04:37:23PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1439991
>
> Whenever a device is being updated via
> virDomainUpdateDeviceFlags() API, we parse the device XML and
> ideally run some generic checks to validate the configuration
> (e.g. if device defines per-device boot order but the domain has
> os/boot element already). Well, that's the theory - due to a
> missing check we've jumped early from that check function.
>
> Signed-off-by: Michal Privoznik
> ---
> src/conf/domain_conf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f7574d77b..0064a71f5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -26026,7 +26026,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
> {
> virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
>
> -if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH)
> +if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
> +action != VIR_DOMAIN_DEVICE_ACTION_UPDATE)
> return 0;
^Which means that now the 'compatibility' check is only skipped for _DETACH
(which, for _DETACH, has been the case since @1c13166134). Therefore I think
that codewise, even though you're change makes sense, it would be IMHO nicer to
just drop this condition all together and also drop the call to this validation
helper from the *Detach helper (with the appropriate tuning of the validator's
signature of course).
With that adjustment:
Reviewed-by: Erik Skultety
>
> if (!virDomainDefHasUSB(def) &&
> --
> 2.13.5
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
