4:54 a.m.
This series addresses the comments to my patch in
http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html
The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS()
instead document to use the virConnectGetDomainCapabilities().
The virConnectGetDomainCapabilities() is extended to check for the host limits
and the NR_CPUs are also returned. I am planning to follow up this series
with possible fixes to maxvcpus command once this merged.
---
Shivaprasad G Bhat (4):
Document to not rely on virConnectGetMaxVcpus API
Rename and move kvmGetMaxVCPUs to utils and extend it
Check for VFIO too where the Legacy passthrough is checked
Check the kvm host cpu max limits in virConnectGetDomainCapabilities()
docs/formatdomaincaps.html.in | 4 +--
src/conf/domain_capabilities.c | 10 +++++--
src/conf/domain_capabilities.h | 1 +
src/libvirt-host.c | 6 ++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_capabilities.c | 13 +++++++--
src/qemu/qemu_capabilities.h | 3 +-
src/qemu/qemu_driver.c | 57 +++-------------------------------------
src/util/virhostcpu.c | 37 ++++++++++++++++++++++++++
src/util/virhostcpu.h | 7 +++++
tests/domaincapstest.c | 3 +-
11 files changed, 78 insertions(+), 64 deletions(-)
--
Signature
4:56 a.m.
New subject: [libvirt] [PATCH 1/4] Document to not rely on virConnectGetMaxVcpus API
The API virConnectGetMaxVcpus doesnt really reflect the actual usable number
of cpus as the maximum limits can be different for kvm and/or qemu. So update
the documentation to use virConnectGetDomainCapabilities() instead.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/libvirt-host.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 24277b7..f6b0c30 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -313,7 +313,11 @@ virConnectGetSysinfo(virConnectPtr conn, unsigned int flags)
*
* Provides the maximum number of virtual CPUs supported for a guest VM of a
* specific type. The 'type' parameter here corresponds to the 'type'
- * attribute in the <domain> element of the XML.
+ * attribute in the <domain> element of the XML. The API doesn't take emulator
+ * limits into consideration. Hence the maximum reported here can be limited by
+ * the emulator max limits. It is recommended to use the
+ * virConnectGetDomainCapabilities() API specifying the emulator details to get
+ * the accurate results.
*
* Returns the maximum of virtual CPU or -1 in case of error.
*/
8:36 a.m.
New subject: [libvirt] [PATCH 1/4] Document to not rely on virConnectGetMaxVcpus API
On Wed, 2016-06-15 at 09:56 +0000, Shivaprasad G Bhat wrote:
The API virConnectGetMaxVcpus doesnt really reflect the actual usable
number
s/doesnt/doesn't/
of cpus as the maximum limits can be different for kvm and/or qemu.
So update
the documentation to use virConnectGetDomainCapabilities() instead.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/libvirt-host.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 24277b7..f6b0c30 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -313,7 +313,11 @@ virConnectGetSysinfo(virConnectPtr conn, unsigned int flags)
*
* Provides the maximum number of virtual CPUs supported for a guest VM of a
* specific type. The 'type' parameter here corresponds to the 'type'
- * attribute in the <domain> element of the XML.
+ * attribute in the <domain> element of the XML. The API doesn't take
emulator
+ * limits into consideration. Hence the maximum reported here can be limited by
+ * the emulator max limits. It is recommended to use the
+ * virConnectGetDomainCapabilities() API specifying the emulator details to get
+ * the accurate results.
I'd reword this slightly, along the lines of
This API doesn't take emulator limits into consideration,
hence the returned value is not guaranteed to be usable.
It is recommended to use virConnectGetDomainCapabilities()
and look for <vcpu max='...'> in its output instead.
*
* Returns the maximum of virtual CPU or -1 in case of error.
*/
I feel like this patch should actually be last one in the
series, after the preferred way to retrieve the information
has actually been implemented.
--
Andrea Bolognani
Software Engineer - Virtualization Team
4:56 a.m.
New subject: [libvirt] [PATCH 2/4] Rename and move kvmGetMaxVCPUs to utils and extend it
This function needs to be used at two different places and make it global
now. Also, extend it to return the NR_CPUs when needed.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 52 +---------------------------------------------
src/util/virhostcpu.c | 37 +++++++++++++++++++++++++++++++++
src/util/virhostcpu.h | 7 ++++++
4 files changed, 46 insertions(+), 51 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e939de3..569f8e8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1066,6 +1066,7 @@ virLogManagerNew;
nodeCapsInitNUMA;
nodeGetInfo;
virHostCPUGetCount;
+virHostCPUGetKVMVCPUs;
virHostCPUGetMap;
virHostCPUGetOnlineBitmap;
virHostCPUGetPresentBitmap;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a45fd55..65ef68c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -122,24 +122,6 @@ VIR_LOG_INIT("qemu.qemu_driver");
#define QEMU_SCHED_MIN_QUOTA 1000LL
#define QEMU_SCHED_MAX_QUOTA 18446744073709551LL
-#if HAVE_LINUX_KVM_H
-# include <linux/kvm.h>
-#endif
-
-/* device for kvm ioctls */
-#define KVM_DEVICE "/dev/kvm"
-
-/* add definitions missing in older linux/kvm.h */
-#ifndef KVMIO
-# define KVMIO 0xAE
-#endif
-#ifndef KVM_CHECK_EXTENSION
-# define KVM_CHECK_EXTENSION _IO(KVMIO, 0x03)
-#endif
-#ifndef KVM_CAP_NR_VCPUS
-# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */
-#endif
-
#define QEMU_NB_BLKIO_PARAM 6
#define QEMU_NB_BANDWIDTH_PARAM 7
@@ -1269,38 +1251,6 @@ static int qemuConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED)
}
-static int
-kvmGetMaxVCPUs(void)
-{
- int fd;
- int ret;
-
- if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
- virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
- return -1;
- }
-
-#ifdef KVM_CAP_MAX_VCPUS
- /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */
- if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0)
- goto cleanup;
-#endif /* KVM_CAP_MAX_VCPUS */
-
- /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of
- * vcpus). Note that on most machines this is set to 160. */
- if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0)
- goto cleanup;
-
- /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel documentation states
- * that 4 should be used as the maximum number of cpus */
- ret = 4;
-
- cleanup:
- VIR_FORCE_CLOSE(fd);
- return ret;
-}
-
-
static char *
qemuConnectGetSysinfo(virConnectPtr conn, unsigned int flags)
{
@@ -1338,7 +1288,7 @@ qemuConnectGetMaxVcpus(virConnectPtr conn ATTRIBUTE_UNUSED, const
char *type)
return 16;
if (STRCASEEQ(type, "kvm"))
- return kvmGetMaxVCPUs();
+ return virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);
if (STRCASEEQ(type, "kqemu"))
return 1;
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 00c09cd..5712eda 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -37,6 +37,7 @@
#if HAVE_LINUX_KVM_H
# include <linux/kvm.h>
#endif
+#define KVM_DEVICE "/dev/kvm"
#if defined(__FreeBSD__) || defined(__APPLE__)
# include <sys/time.h>
@@ -1297,3 +1298,39 @@ virHostCPUGetThreadsPerSubcore(virArch arch ATTRIBUTE_UNUSED)
}
#endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */
+
+#ifndef KVM_CAP_NR_VCPUS
+# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */
+#endif
+
+int
+virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag)
+{
+ int fd;
+ int ret;
+
+ if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
+ virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
+ return -1;
+ }
+
+#ifdef KVM_CAP_MAX_VCPUS
+ /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */
+ if (flag & VIR_HOSTCPU_KVM_MAXVCPUS &&
+ (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0)
+ goto cleanup;
+#endif /* KVM_CAP_MAX_VCPUS */
+
+ /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of
+ * vcpus). Note that on most machines this is set to 160. */
+ if ((flag & VIR_HOSTCPU_KVM_MAXVCPUS || flag & VIR_HOSTCPU_KVM_NR_VCPUS)
&&
+ (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0)
+ goto cleanup;
+ /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel documentation states
+ * that 4 should be used as the maximum number of cpus */
+ ret = 4;
+
+ cleanup:
+ VIR_FORCE_CLOSE(fd);
+ return ret;
+}
diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h
index e5ffc70..c1b855a 100644
--- a/src/util/virhostcpu.h
+++ b/src/util/virhostcpu.h
@@ -51,4 +51,11 @@ int virHostCPUGetInfo(virArch hostarch,
unsigned int *cores,
unsigned int *threads);
+typedef enum {
+ VIR_HOSTCPU_KVM_MAXVCPUS = (1 << 0),
+ VIR_HOSTCPU_KVM_NR_VCPUS = (1 << 1),
+} virHostCPUKVMWrapperFlags;
+
+int virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag);
+
#endif /* __VIR_HOSTCPU_H__*/
9:22 a.m.
New subject: [libvirt] [PATCH 2/4] Rename and move kvmGetMaxVCPUs to utils and extend it
On Wed, 2016-06-15 at 09:56 +0000, Shivaprasad G Bhat wrote:
This function needs to be used at two different places and make it
global
now. Also, extend it to return the NR_CPUs when needed.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 52 +---------------------------------------------
src/util/virhostcpu.c | 37 +++++++++++++++++++++++++++++++++
src/util/virhostcpu.h | 7 ++++++
4 files changed, 46 insertions(+), 51 deletions(-)
This should really be two patches: one moving the function
to its new location while marking it as a private exported
symbol, and the other one chaging its signature and
behavior.
Actually, make it three patches - see below.
-/* add definitions missing in older linux/kvm.h */
-#ifndef KVMIO
-# define KVMIO 0xAE
-#endif
-#ifndef KVM_CHECK_EXTENSION
-# define KVM_CHECK_EXTENSION _IO(KVMIO, 0x03)
-#endif
-#ifndef KVM_CAP_NR_VCPUS
-# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */
-#endif
We no longer support any distribution old enough not to have
these definitions, so you can just drop them (as a separate
patch).
+int
+virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag)
+{
+ int fd;
+ int ret;
+
+ if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) {
+ virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE);
+ return -1;
+ }
+
+#ifdef KVM_CAP_MAX_VCPUS
+ /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */
+ if (flag & VIR_HOSTCPU_KVM_MAXVCPUS &&
+ (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0)
+ goto cleanup;
+#endif /* KVM_CAP_MAX_VCPUS */
+
+ /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of
+ * vcpus). Note that on most machines this is set to 160. */
+ if ((flag & VIR_HOSTCPU_KVM_MAXVCPUS || flag & VIR_HOSTCPU_KVM_NR_VCPUS)
&&
+ (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0)
+ goto cleanup;
You dropped a blank line here, and the "goto" is not indented
properly
+ /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel
documentation states
+ * that 4 should be used as the maximum number of cpus */
+ ret = 4;
[...]
+typedef enum {
+ VIR_HOSTCPU_KVM_MAXVCPUS = (1 << 0),
+ VIR_HOSTCPU_KVM_NR_VCPUS = (1 << 1),
+} virHostCPUKVMWrapperFlags;
+
+int virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag);
The naming for the function, and especially for the enum and
its values, is IMHO kinda awkward after the change... We could
have something like
virHostCPUGetKVMMaxVCPUs()
virHostCPUGetKVMRecommendedVCPUs()
instead. The attached patch, which applies cleanly on top of
your series, shows what I have in mind :)
--
Andrea Bolognani
Software Engineer - Virtualization Team
4:57 a.m.
New subject: [libvirt] [PATCH 3/4] Check for VFIO too where the Legacy passthrough is checked
On PPC the Legacy patthrough is not supported and only
VFIO is supported. So, the checks at places to confirm if the
host is passthrough capable checks only Legacy, fix it. This
is seen at only one place now.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 65ef68c..6b316a0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
cfg = virQEMUDriverGetConfig(driver);
- if (qemuHostdevHostSupportsPassthroughLegacy())
+ if (qemuHostdevHostSupportsPassthroughLegacy() ||
+ qemuHostdevHostSupportsPassthroughVFIO())
virttype = VIR_DOMAIN_VIRT_KVM;
else
virttype = VIR_DOMAIN_VIRT_QEMU;
9:37 a.m.
New subject: [libvirt] [PATCH 3/4] Check for VFIO too where the Legacy passthrough is checked
On Wed, 2016-06-15 at 09:57 +0000, Shivaprasad G Bhat wrote:
On PPC the Legacy patthrough is not supported and only
s/patthrough/passthrough/
VFIO is supported. So, the checks at places to confirm if the
host is passthrough capable checks only Legacy, fix it. This
is seen at only one place now.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 65ef68c..6b316a0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
cfg = virQEMUDriverGetConfig(driver);
- if (qemuHostdevHostSupportsPassthroughLegacy())
+ if (qemuHostdevHostSupportsPassthroughLegacy() ||
+ qemuHostdevHostSupportsPassthroughVFIO())
virttype = VIR_DOMAIN_VIRT_KVM;
else
virttype = VIR_DOMAIN_VIRT_QEMU;
This looks like an unrelated bugfix. If that's the case,
please yank it out of the series and submit it separately.
Looks good otherwise :)
--
Andrea Bolognani
Software Engineer - Virtualization Team
8:09 a.m.
New subject: [libvirt] [PATCH 3/4] Check for VFIO too where the Legacy passthrough is checked
On Wed, 2016-06-22 at 16:37 +0200, Andrea Bolognani wrote:
On Wed, 2016-06-15 at 09:57 +0000, Shivaprasad G Bhat wrote:
> On PPC the Legacy patthrough is not supported and only
s/patthrough/passthrough/
> VFIO is supported. So, the checks at places to confirm if the
> host is passthrough capable checks only Legacy, fix it. This
> is seen at only one place now.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_driver.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 65ef68c..6b316a0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
>
> cfg = virQEMUDriverGetConfig(driver);
>
> - if (qemuHostdevHostSupportsPassthroughLegacy())
> + if (qemuHostdevHostSupportsPassthroughLegacy() ||
> + qemuHostdevHostSupportsPassthroughVFIO())
> virttype = VIR_DOMAIN_VIRT_KVM;
> else
> virttype = VIR_DOMAIN_VIRT_QEMU;
This looks like an unrelated bugfix. If that's the case,
please yank it out of the series and submit it separately.
Looks good otherwise :)
Since you've confirmed (on IRC) that this is indeed an
independed bugfix, and the patch is already good to go,
I've amended the commit messages and pushed it.
--
Andrea Bolognani / Red Hat / Virtualization
4:58 a.m.
New subject: [libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()
The qemu limit and host limit both should be considered for
the domain vcpu max limits.
Signed-off-by: Shivaprasad G Bhat <sbhat(a)linux.vnet.ibm.com>
---
docs/formatdomaincaps.html.in | 4 ++--
src/conf/domain_capabilities.c | 10 +++++++---
src/conf/domain_capabilities.h | 1 +
src/qemu/qemu_capabilities.c | 13 ++++++++++---
src/qemu/qemu_capabilities.h | 3 ++-
src/qemu/qemu_driver.c | 2 +-
tests/domaincapstest.c | 3 ++-
7 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index d5a8414..d28a5b6 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -86,14 +86,14 @@
<pre>
<domainCapabilities>
...
- <vcpu max='255'/>
+ <vcpu max='255' suggested='96'/>
...
</domainCapabilities>
</pre>
<dl>
<dt><code>vcpu</code></dt>
- <dd>The maximum number of supported virtual CPUs</dd>
+ <dd>The maximum number of supported virtual CPUs. The suggested attribute if
present, gives the recommended maximum vcpus for the KVM host.</dd>
</dl>
<h3><a name="elementsOSBIOS">BIOS
bootloader</a></h3>
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 1676f0e..452cad4 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -329,9 +329,13 @@ virDomainCapsFormatInternal(virBufferPtr buf,
virBufferAsprintf(buf, "<machine>%s</machine>\n",
caps->machine);
virBufferAsprintf(buf, "<arch>%s</arch>\n", arch_str);
- if (caps->maxvcpus)
- virBufferAsprintf(buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
-
+ if (caps->maxvcpus) {
+ if (!caps->suggestedvcpus)
+ virBufferAsprintf(buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
+ else
+ virBufferAsprintf(buf, "<vcpu max='%d'
suggested='%d'/>\n",
+ caps->maxvcpus, caps->suggestedvcpus);
+ }
virDomainCapsOSFormat(buf, &caps->os);
virBufferAddLit(buf, "<devices>\n");
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 492a9cf..f440436 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -112,6 +112,7 @@ struct _virDomainCaps {
/* Some machine specific info */
int maxvcpus;
+ int suggestedvcpus;
virDomainCapsOS os;
virDomainCapsDeviceDisk disk;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 1ef5937..df10aa8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -38,6 +38,7 @@
#include "virbitmap.h"
#include "virnodesuspend.h"
#include "virnuma.h"
+#include "virhostcpu.h"
#include "qemu_monitor.h"
#include "virstring.h"
#include "qemu_hostdev.h"
@@ -4333,16 +4334,22 @@ int
virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
virQEMUCapsPtr qemuCaps,
virFirmwarePtr *firmwares,
- size_t nfirmwares)
+ size_t nfirmwares,
+ virDomainVirtType virttype)
{
virDomainCapsOSPtr os = &domCaps->os;
virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
virDomainCapsDeviceVideoPtr video = &domCaps->video;
- int maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
+ int hostmaxvcpus = 0;
- domCaps->maxvcpus = maxvcpus;
+ domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
+ if (virttype == VIR_DOMAIN_VIRT_KVM) {
+ hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);
+ domCaps->suggestedvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_NR_VCPUS);
+ domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
+ }
if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 ||
virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps,
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f7ede4a..ffe07e5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -494,6 +494,7 @@ int virQEMUCapsInitGuestFromBinary(virCapsPtr caps,
int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
virQEMUCapsPtr qemuCaps,
virFirmwarePtr *firmwares,
- size_t nfirmwares);
+ size_t nfirmwares,
+ virDomainVirtType virttype);
#endif /* __QEMU_CAPABILITIES_H__*/
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6b316a0..21aa053 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18485,7 +18485,7 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
goto cleanup;
if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps,
- cfg->firmwares, cfg->nfirmwares) < 0)
+ cfg->firmwares, cfg->nfirmwares, virttype) <
0)
goto cleanup;
ret = virDomainCapsFormat(domCaps);
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 9fb2c97..ea438e3 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -129,7 +129,8 @@ fillQemuCaps(virDomainCapsPtr domCaps,
if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps,
cfg->firmwares,
- cfg->nfirmwares) < 0)
+ cfg->nfirmwares,
+ VIR_DOMAIN_VIRT_QEMU) < 0)
goto cleanup;
/* The function above tries to query host's KVM & VFIO capabilities by
10:15 a.m.
New subject: [libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()
On Wed, 2016-06-15 at 09:58 +0000, Shivaprasad G Bhat wrote:
<dl>
<dt><code>vcpu</code></dt>
- <dd>The maximum number of supported virtual CPUs</dd>
+ <dd>The maximum number of supported virtual CPUs. The suggested attribute if
present, gives the recommended
maximum vcpus for the KVM host.</dd>
</dl>
Rewrite as
Information about the number of virtual CPUs that can be
assigned to a guest. The <code>max</code> attribute contains
the maximum number of virtual CPUs; the
<code>suggested</code> attribute, if present, contains the
recommended number of virtual CPUs.
making sure lines are at most 80 columns long.
I would prefer to use "recommended" instead of "suggested" for
the attribute name, and for all related variables, as the
relevant comment in linux/kvm.h reads
/* returns recommended max vcpus per vm */
and "to recommend" is generally stronger than "to suggest", ie.
you should think twice before disregarding a recommendation.
- if (caps->maxvcpus)
- virBufferAsprintf(buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
-
+ if (caps->maxvcpus) {
+ if (!caps->suggestedvcpus)
+ virBufferAsprintf(buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
+ else
+ virBufferAsprintf(buf, "<vcpu max='%d'
suggested='%d'/>\n",
+ caps->maxvcpus, caps->suggestedvcpus);
+ }
You could change this to
if (caps->maxvcpus) {
virBufferAsprintf(buf, "<vcpu max='%d'", caps->maxvcpus);
if (caps->suggestedvcpus)
virBufferAsprintf(buf, " suggested='%d'",
caps->suggestedvcpus);
virBufferAddLit(buf, "/>\n");
}
but it's up to you, really.
virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
virQEMUCapsPtr qemuCaps,
virFirmwarePtr *firmwares,
- size_t nfirmwares)
+ size_t nfirmwares,
+ virDomainVirtType virttype)
{
virDomainCapsOSPtr os = &domCaps->os;
virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
virDomainCapsDeviceVideoPtr video = &domCaps->video;
- int maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
+ int hostmaxvcpus = 0;
- domCaps->maxvcpus = maxvcpus;
+ domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
+ if (virttype == VIR_DOMAIN_VIRT_KVM) {
+ hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);
You can move the definition of the 'hostmaxvcpus' variable
in here, it's not used in the outer scope anyway.
You should also perform some error checking here:
virHostCPUGetKVMVCPUs() can return a negative number, for
example if /dev/kvm can't be accessed.
If that happens, I'm not sure whether it would be better
to simply ignore the failure and just report the QEMU
limits, or if we should abort virQEMUCapsFillDomainCaps()
altogether. Probably the former.
Everything else looks good.
--
Andrea Bolognani
Software Engineer - Virtualization Team
10:27 a.m.
New subject: [libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()
On Wed, 2016-06-15 at 09:58 +0000, Shivaprasad G Bhat wrote:
- domCaps->maxvcpus = maxvcpus;
+ domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps-
machine);
+ if (virttype == VIR_DOMAIN_VIRT_KVM) {
+ hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);
+ domCaps->suggestedvcpus =
virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_NR_VCPUS);
+ domCaps->maxvcpus = MIN(domCaps->maxvcpus,
hostmaxvcpus);
+ }
Forgot to mention this yesterday: domCaps->suggestedvcpus
will have to be capped by hostmaxvcpus as well, because
otherwise you would end up with stuff like
<vcpu max='1' suggested='160'/>
for the 'isapc' or 'xenpv' machine types.
--
Andrea Bolognani / Red Hat / Virtualization
10:25 a.m.
On Wed, 2016-06-15 at 09:54 +0000, Shivaprasad G Bhat wrote:
This series addresses the comments to my patch in
http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html
The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS()
instead document to use the virConnectGetDomainCapabilities().
The virConnectGetDomainCapabilities() is extended to check for the host limits
and the NR_CPUs are also returned. I am planning to follow up this series
with possible fixes to maxvcpus command once this merged.
Sorry for being late in reviewing this series.
If you can post a v2 addressing my comments in the next
couple of days, there's still time for it to be merged
in time for 2.0.0 - the freeze is on Sunday.
Please note that it doesn't apply cleanly on top of
current master, but the conflict is easy enough to fix.
--
Andrea Bolognani
Software Engineer - Virtualization Team
9:57 a.m.
On 06/22/2016 08:55 PM, Andrea Bolognani wrote:
On Wed, 2016-06-15 at 09:54 +0000, Shivaprasad G Bhat wrote:
> This series addresses the comments to my patch in
> http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html
>
> The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS()
> instead document to use the virConnectGetDomainCapabilities().
> The virConnectGetDomainCapabilities() is extended to check for the host limits
> and the NR_CPUs are also returned. I am planning to follow up this series
> with possible fixes to maxvcpus command once this merged.
Sorry for being late in reviewing this series.
If you can post a v2 addressing my comments in the next
couple of days, there's still time for it to be merged
in time for 2.0.0 - the freeze is on Sunday.
Thanks Andrea. As discussed with you
on IRC, I am dropping my changes
for "suggested" cpus in
the next version and follow it up separately, so just the bug fix is
easier for the build.
Please note that it doesn't apply cleanly on top of
current master, but the conflict is easy enough to fix.
--
Andrea Bolognani
Software Engineer - Virtualization Team
3294
days inactive
3303
days old
12 comments
2 participants
participants (2)
-
Andrea Bolognani -
Shivaprasad G Bhat