When doing the snapshot using the script below:
=
!#/bin/bash
virsh blockjob $instance_name vdX --abort
virsh undefine $instance_name
qemu-img create -f qcow2 -o backing_file=$diskfile,size=$size $path/$uuid.dlta
virsh blockcopy --domain $instance_name vdX $path/$uuid.dlta --wait
--reuse-external --verbose
virsh blockjob $instance_name vdX --abort
qemu-img convert -f qcow2 -O qcow2 $path/$uuid.dlta $path/$uuid.qcow2
echo "start uploading at $(date)"
..
=
Sometimes you can encounter the following warning and error logs:
+
2016-08-26 07:51:05.685+: 8916: warning :
qemuDomainObjBeginJobInternal:1571 :
Cannot start job (query, none) for domain instance-1b3a; current job is
(modify,
none) owned by (8914 remoteDispatchDomainBlockJobAbort, 0 ) for (30s, 0s)
2016-08-26 07:51:05.685+: 8916: error : qemuDomainObjBeginJobInternal:1583 :
Timed out during operation: cannot acquire state change lock (held by
remoteDispatchDomainBlockJobAbort)
-
Mostly it will be okay later. But sometimes the state change lock maybe hold by
remoteDispatchDomainBlockJobAbort forever, then the instance couldn't be
connected
through the VNC or the network(ping,ssh...), expect after rebooting the
instance.
>From the code, we find that after sending the --abort command, there will be
>two
steps by condition waiting the two reply signals:
+
The first condition wait is:
In qemuMonitorBlockJobCancel() --> qemuMonitoJSONBlockJobCancel() -->
qemuMonitorJSONCommand() --> qemuMonitorJSONCommandWithFd() -->
qemuMonitorSend() --> virCondWait(&mon->notify, &mon->parent.lock).
The second condition wait is:
In virDomainCondWait(vm) --> virCondWait(&vm->cond, &vm->parent.lock).
-
The two corresponding replies are:
+
The "return" reply is:
QEMU_MONITOR_RECV_REPLY: mon=0x7ff3fc001020 reply={"return": [], "id":
"libvirt-338"}
With the condition wakeup signal:
virCondBroadcast(&mon->notify)
The "event" reply is:
QEMU_MONITOR_RECV_EVENT: mon=0x7fe08401a3f0 event={"timestamp": {"seconds":
1472524518,
"microseconds": 360135}, "event": "BLOCK_JOB_CANCELLED", "data": {"device":
"drive-virtio-disk0", "len": 42949672960, "offset": 10485760, "speed": 0,
"type": "mirror"}}
With the condition wakeup signal:
virDomainObjBroadcast(vm) --> virCondBroadcast(&vm->cond)
-
But sometimes the qemu daemon will reply like:
+
The "event" reply is:
QEMU_MONITOR_RECV_EVENT: mon=0x7fe08401a3f0 event={"timestamp": {"seconds":
1472524518,
"microseconds": 360135}, "event": "BLOCK_JOB_CANCELLED", "data": {"device":
"drive-virtio-disk0", "len": 42949672960, "offset": 10485760, "speed": 0,
"type": "mirror"}}
With the condition wakeup signal:
virDomainObjBroadcast(vm) --> virCondBroadcast(&vm->cond)
The "return" reply is:
QEMU_MONITOR_RECV_REPLY: mon=0x7ff3fc001020 reply={"return": [], "id":
"libvirt-338"}
With the condition wakeup signal:
virCondBroadcast(&mon->notify)
-
In this case, when in the first condition wait, the received "event" reply &
signal will be
lost and still waiting for the "return" reply & signal. So the when in the
second condition
wait step, it will wait forever by holding the lock.
This patch fix this bug.
Signed-off-by: Xiubo Li
Signed-off-by: Zhuoyu Zhang
Signed-off-by: Wei Tang
Signed-off-by: Yaowei Bai
Signed-off-by: Qide Chen
---
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 8 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index c49f31c..2624c6d 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -61,6 +61,8 @@
(JOB_MASK(QEMU_JOB_DESTROY) | \
JOB_MASK(QEMU_JOB_ASYNC))
+# define BLOCK_JOB_ABORT_TIMEOUT 5000
+
/* Only 1 job is allowed at any time
* A job includes *all* monitor commands, even those just querying
* information, not merely actions */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f9a3b15..6ebaf6b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15929,6 +15929,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
virDomainObjPtr vm;
+unsigned long long now;
int ret = -1;
virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
@@ -16018,7 +16019,12 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
qemuBlockJobUpdate(driver, vm, disk);
while (diskPriv->blockjob) {
-if (virDomainObjWait(vm) < 0) {
+
On Wed, Aug 31, 2016 at 05:10:35PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_command.c| 8 +++
.../qemuxml2argv-net-virtio-rxqueuesize.args | 25 ++
tests/qemuxml2argvtest.c | 2 ++
3 files changed, 35 insertions(+)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 982c33c..fce779b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3553,6 +3553,14 @@ qemuBuildNicDevStr(virDomainDefPtr def,
virBufferAsprintf(&buf, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2);
}
}
+if (usingVirtio && net->driver.virtio.rx_queue_size) {
I kinda hate that we blindly go on if it's not virtio device, so that
users might thing they increased the ring size but actually we just skip
using that tunable at all. But since we do that with, literally, all
the other tunables...
ACK series (when it is pushed in QEMU, which it looks like it's queued
for after the release)
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Aug 31, 2016 at 05:10:33PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1366989
QEMU added another virtio-net tunable [1]. It basically allows
users to set the size of RX virtio ring. But because virtio-net
uses two separate ring buffers to pass data from/to guest they
named it explicitly rx_queue_size. We should expose it in our XML
too.
1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html
Signed-off-by: Michal Privoznik
---
docs/formatdomain.html.in | 16 -
docs/schemas/domaincommon.rng | 5 +++
src/conf/domain_conf.c | 16 +
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 7
...ml2argv-net-virtio-rxqueuesize-invalid-size.xml | 29 +++
.../qemuxml2argv-net-virtio-rxqueuesize.xml| 29 +++
tests/qemuxml2argvtest.c | 1 +
.../qemuxml2xmlout-net-virtio-rxqueuesize.xml | 41 ++
tests/qemuxml2xmltest.c| 1 +
10 files changed, 145 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize-invalid-size.xml
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
create mode 100644
tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxqueuesize.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8c15a73..be19bb9 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4650,7 +4650,7 @@ qemu-kvm -net nic,model=? /dev/null
-
+
@@ -4766,6 +4766,20 @@ qemu-kvm -net nic,model=? /dev/null
virtio-net since 1.0.6 (QEMU and KVM only)
vhost-user since 1.2.17 (QEMU and KVM only)
+ rx_queue_size
+
+The optional rx_queue_size attribute controls
+the size of virtio ring for each queue as described above.
+The default value is hypervisor dependent and may change
+across its releases. Moreover, some hypervisors may pose
+some restrictions on actual value. For instance, latest
I wonder how long this "latest" will be true. Either say "latest as of
-mm-dd" or QEMU version or just leave the example out.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 61f6dbb..05a1227 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9517,6 +9519,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
if (q > 1)
def->driver.virtio.queues = q;
}
+if (rx_queue_size) {
+unsigned int q;
+if (virStrToLong_uip(rx_queue_size, NULL, 10, &q) < 0) {
+virReportError(VIR_ERR_XML_DETAIL,
+ _("'rx_queue_size' attribute must be positive
number: %s"),
+ rx_queue_size);
+goto error;
+}
+if (q > 1)
+def->driver.virtio.rx_queue_size = q;
Pointless condition, you can assign it unconditionally.
+}
if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) {
if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -9697,6 +9710,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(ioeventfd);
VIR_FREE(event_idx);
VIR_FREE(queues);
+VIR_FREE(rx_queue_size);
VIR_FREE(str);
VIR_FREE(filter);
VIR_FREE(type);
@@ -20890,6 +20904,8 @@ virDomainVirtioNetDriverFormat(char **outstr,
}
if (def->driver.virtio.queues)
virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues);
+if (def->driver.virtio.rx_queue_size)
+virBufferAsprintf(&buf, "rx_queue_size='%u' ",
def->driver.virtio.rx_queue_size);
Long line.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c1fb771..29ade0e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2475,6 +2475,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef
*dev,
"not supported by QEMU"));
goto cleanup;
}
+
+if (STREQ_NULLABLE(net->model, "virtio") &&
+net->driver.virtio.rx_queue_size &
(net->driver.virtio.rx_queue_size - 1)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("rx_queue_size has to be a po
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, August 31, 2016 11:49 PM
>
> > >
> > > IGD doesn't have such peer-to-peer resource setup requirement. So
> > > it's sufficient to create/destroy a mdev instance in a single action on
> > > IGD. However I'd expect we still keep the "start/stop" interface (
> > > maybe not exposed as sysfs node, instead being a VFIO API), as
> > > required to support future live migration usage. We've made prototype
> > > working for KVMGT today.
>
> Great!
>
btw here is a link to KVMGT live migration demo:
https://www.youtube.com/watch?v=y2SkU5JODIY
Thanks
Kevin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, August 31, 2016 11:49 PM
>
> On Wed, 31 Aug 2016 15:04:13 +0800
> Jike Song wrote:
>
> > On 08/31/2016 02:12 PM, Tian, Kevin wrote:
> > >> From: Alex Williamson [mailto:alex.william...@redhat.com]
> > >> Sent: Wednesday, August 31, 2016 12:17 AM
> > >>
> > >> Hi folks,
> > >>
> > >> At KVM Forum we had a BoF session primarily around the mediated device
> > >> sysfs interface. I'd like to share what I think we agreed on and the
> > >> "problem areas" that still need some work so we can get the thoughts
> > >> and ideas from those who weren't able to attend.
> > >>
> > >> DanPB expressed some concern about the mdev_supported_types sysfs
> > >> interface, which exposes a flat csv file with fields like "type",
> > >> "number of instance", "vendor string", and then a bunch of type
> > >> specific fields like "framebuffer size", "resolution", "frame rate
> > >> limit", etc. This is not entirely machine parsing friendly and sort of
> > >> abuses the sysfs concept of one value per file. Example output taken
> > >> from Neo's libvirt RFC:
> > >>
> > >> cat /sys/bus/pci/devices/:86:00.0/mdev_supported_types
> > >> # vgpu_type_id, vgpu_type, max_instance, num_heads, frl_config,
> > >> framebuffer,
> > >> max_resolution
> > >> 11 ,"GRID M60-0B", 16, 2, 45, 512M,2560x1600
> > >> 12 ,"GRID M60-0Q", 16, 2, 60, 512M,2560x1600
> > >> 13 ,"GRID M60-1B", 8, 2, 45,1024M,2560x1600
> > >> 14 ,"GRID M60-1Q", 8, 2, 60,1024M,2560x1600
> > >> 15 ,"GRID M60-2B", 4, 2, 45,2048M,2560x1600
> > >> 16 ,"GRID M60-2Q", 4, 4, 60,2048M,2560x1600
> > >> 17 ,"GRID M60-4Q", 2, 4, 60,4096M,3840x2160
> > >> 18 ,"GRID M60-8Q", 1, 4, 60,8192M,3840x2160
> > >>
> > >> The create/destroy then looks like this:
> > >>
> > >> echo "$mdev_UUID:vendor_specific_argument_list" >
> > >> /sys/bus/pci/devices/.../mdev_create
> > >>
> > >> echo "$mdev_UUID:vendor_specific_argument_list" >
> > >> /sys/bus/pci/devices/.../mdev_destroy
> > >>
> > >> "vendor_specific_argument_list" is nebulous.
> > >>
> > >> So the idea to fix this is to explode this into a directory structure,
> > >> something like:
> > >>
> > >> ├── mdev_destroy
> > >> └── mdev_supported_types
> > >> ├── 11
> > >> │ ├── create
> > >> │ ├── description
> > >> │ └── max_instances
> > >> ├── 12
> > >> │ ├── create
> > >> │ ├── description
> > >> │ └── max_instances
> > >> └── 13
> > >> ├── create
> > >> ├── description
> > >> └── max_instances
> > >>
> > >> Note that I'm only exposing the minimal attributes here for simplicity,
> > >> the other attributes would be included in separate files and we would
> > >> require vendors to create standard attributes for common device classes.
> > >
> > > I like this idea. All standard attributes are reflected into this
> > > hierarchy.
> > > In the meantime, can we still allow optional vendor string in create
> > > interface? libvirt doesn't need to know the meaning, but allows upper
> > > layer to do some vendor specific tweak if necessary.
> > >
> >
> > Not sure whether this can done within MDEV framework (attrs provided by
> > vendor driver of course), or must be within the vendor driver.
>
> The purpose of the sub-directories is that libvirt doesn't need to pass
> arbitrary, vendor strings to the create function, the attributes of the
> mdev device created are defined by the attributes in the sysfs
> directory where the create is done. The user only provides a uuid for
> the device. Arbitrary vendor parameters are a barrier, libvirt may not
> need to know the meaning, but would need to know when to apply them,
> which is just as bad. Ultimately we want libvirt to be able to
> interact with sysfs without having an vendor specific knowledge.
Understand. Today Intel doesn't have such vendor specific parameter
requirement when creating a mdev instance (assuming type definition
is enough to cover our existing parameters).
Just think about future extensibility. Say if a new parameter (say
a QoS parameter like weight or cap) must be statically set before
created mdev instance starts to work, due to device limitation, such
parameter needs to be exposed as a new attribute under the specific
mdev instance, e.g.:
/sys/bus/pci/devices//mdev/weight
Then libvirt needs to make sure it's set before open() the instance.
If such flow is acceptable, it should remove necessity of vendor specific
parameter at the create, because any such requirement should be
converted into sysfs node, if applicable to all vendors, then libvirt
can do asynchronous configurations before starting the instance.
>
> > >>
> > >> For vGPUs like NVIDIA where we don't support multiple t
On 08/11/2016 09:26 AM, Michal Privoznik wrote:
> Now that we have APIs for relabel memdevs on hotplug, fill in the
> missing implementation in qemu hotplug code.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_hotplug.c | 14 ++
> 1 file changed, 14 insertions(+)
>
Note: Patches 6-9 have an implicit ACK - they seem to be fairly
standard. Although what about apparmour?
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 6ba0b8e..afabbda 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1861,6 +1861,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> int id;
> int ret = -1;
> int rv;
> +bool restoreLabel = false;
>
> qemuDomainMemoryDeviceAlignSize(vm->def, mem);
>
> @@ -1893,6 +1894,11 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> goto removedef;
> }
>
> +if (virSecurityManagerSetMemoryLabel(driver->securityManager,
> + vm->def, mem) < 0)
> +goto cleanup;
> +restoreLabel = true;
> +
> qemuDomainObjEnterMonitor(driver, vm);
> rv = qemuMonitorAddObject(priv->mon, backendType, objalias, props);
> props = NULL; /* qemuMonitorAddObject consumes */
> @@ -1945,6 +1951,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> mem = NULL;
> goto audit;
> }
> +if (mem && restoreLabel &&
Coverity notes that checking for mem here is unnecessary. It dereffed
at the top and there is no way to get to the exit_monitor label after
the mem = NULL.
> +virSecurityManagerRestoreMemoryLabel(driver->securityManager,
> + vm->def, mem) < 0)
> +VIR_WARN("Unable to restore security label on memdev");
In any case, if this does stay within this label, I think it should move
to inside the 'orig_err' code...
The question becomes, if the qemuDomainObjExitMonitor fails, should the
Restore be called as well. Part of me says yes, but then it's noted in
the failure to ExitMonitor that we cannot touch mem, so we're SOL.
John
>
> removedef:
> if ((id = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
> @@ -3141,6 +3151,10 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver,
> if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0)
> virDomainMemoryRemove(vm->def, idx);
>
> +if (virSecurityManagerRestoreMemoryLabel(driver->securityManager,
> + vm->def, mem) < 0)
> +VIR_WARN("Unable to restore security label on memdev");
> +
> virDomainMemoryDefFree(mem);
>
> /* fix the balloon size */
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/11/2016 09:26 AM, Michal Privoznik wrote:
> Now that NVDIMM has found its way into libvirt, users might want
> to fine tune some settings for each module separately. One such
> setting is 'share=on|off' for the memory-backend-file object.
> This setting - just like its name suggest already - enables
> sharing the nvdimm module with other applications. Under the hood
> it controls whether qemu mmaps() the file as MAP_PRIVATE or
> MAP_SHARED.
>
> Yet again, we have such config knob in domain XML, but it's just
> an attribute to numa . This does not give fine enough
> tuning on per-memdevice basis so we need to have the attribute
> for each device too.
>
> Signed-off-by: Michal Privoznik
> ---
> docs/formatdomain.html.in | 15 ++-
> docs/schemas/domaincommon.rng | 19 ++---
> src/conf/domain_conf.c | 15 ++-
> src/conf/domain_conf.h | 2 +
> src/libvirt_private.syms | 2 +
> ...emuxml2argv-memory-hotplug-nvdimm-memAccess.xml | 49
> ++
> 6 files changed, 93 insertions(+), 9 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm-memAccess.xml
>
After reading this patch and the next one, I'm not sure I understand the
need. For a 'dimm' it already takes a value from whatever numa has for
the node. Adding this would seemingly allow someone to overwrite that
value. So what would happen if the numa node was private and the *dimm
node shared? The setting and usage does not restrict to nvdimm only.
The rest of this is my thoughts upon first read... I'm still hung up on
whether dimm and nvdimm should share nmems, but still figured I'd look
at more patches to provide more thoughts.
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 657df8f..06fe50d 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1356,7 +1356,7 @@
>Since 1.2.9 the optional attribute
>memAccess can control whether the memory is to be
>mapped as "shared" or "private". This is valid only for
> - hugepages-backed memory.
> + hugepages-backed memory and nvdimm modules.
>
>
>
> @@ -6724,7 +6724,7 @@ qemu-kvm -net nic,model=? /dev/null
>
>...
>
> -
> +
'dimm'?? Shouldn't this be on the 'nvdimm'?
The way I read the commit msg it's for nvdimm only. It would seem this
would allow the dimm
I would think there'd be concerns over nefarious uses of this hole in
the guest...
>
> 524287
> 0
> @@ -6762,6 +6762,17 @@ qemu-kvm -net nic,model=? /dev/null
>
>
>
> + memAccess
> +
> +
> + Then there is optional attribute memAccess
> + (Since 2.2.0) that allows
2.3.0 at the earliest.
> + uses to fine tune mapping of the memory on per module
> + basis. Values are the same as for numa :
> + shared and private.
> +
> +
> +
Placement. This is an attribute of but currently "tied to"
nvdimm. Making this a at the same level of gives me the
wrong impression. I think the text of the message belongs in the
previous paragraph. E.g. "... a Non-Volatile DIMM module. For NVDIMM
modules, an optional attribute memAccess can be provided.
This allows users to fine tune mapping of the memory on a per module
basis. Values are the same as..."
BTW: It took me a few searches to find the shared/private NUMA
discussion in the "#elementsCPU"... Makes me wonder if we should create
another label "#numaTopology" that allows us to link directly to that
discussion from right here.
>source
>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index e6ad452..3e9d342 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4467,6 +4467,15 @@
>
>
>
> +
> +
> +
> +shared
> +private
> +
> +
> +
> +
>
>
>
> @@ -4486,12 +4495,7 @@
>
>
>
> -
> -
> -shared
> -private
> -
> -
> +
>
>
>
> @@ -4725,6 +4729,9 @@
>nvdimm
>
>
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index cb5a053..84f76dd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13245,6 +13245,15 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode,
> }
> VIR_FREE(tmp);
>
> +tmp = virXMLPropString(memdevNode, "memAccess");
> +if (tmp &&
> +(def->memAccess = virNumaMemAccessTypeFromString(tmp)) <= 0) {
> +virReportError(VI
On 08/11/2016 09:26 AM, Michal Privoznik wrote:
> NVDIMM is new type of memory introduced in qemu. The idea is that
s/in qemu/into QEMU 2.6/
> we have a DIMM module that keeps the data persistent across
s/DIMM/Non Volatile memory/
> domain reboots.
Perhaps a word of two about what is the usefulness of such a beast. I
think (from a former project) one usage is to store parameters for
firmware such as OVMF
Add extra line between paragraphs...
> At the domain XML level, we already have some representation of
> 'dimm' modules. Long story short, we have element that
> lives under . Now, the element even has @model
> attribute which we can use to introduce new memory type:
>
>
>
> /tmp/nvdimm
>
>
> 523264
> 0
>
>
>
> So far, this is just a XML parser/formatter extension. QEMU
> driver implementation is in the next commit.
>
> For more info on NVDIMM visit the following web page:
>
> http://pmem.io/
>
One could also google it ;-)... In any case, the actual details are
found the Documents link from that page, subject to move over time of
course.
> Signed-off-by: Michal Privoznik
> ---
> docs/formatdomain.html.in | 26 --
> docs/schemas/domaincommon.rng | 32 ---
> src/conf/domain_conf.c | 97
> --
> src/conf/domain_conf.h | 2 +
> src/qemu/qemu_command.c| 6 ++
> src/qemu/qemu_domain.c | 1 +
> .../qemuxml2argv-memory-hotplug-nvdimm.xml | 49 +++
> 7 files changed, 171 insertions(+), 42 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-nvdimm.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index bfbb0f2..657df8f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6740,6 +6740,15 @@ qemu-kvm -net nic,model=? /dev/null
> 1
>
>
> +
> +
> +/tmp/nvdimm
> +
> +
> +524287
> +1
> +
> +
>
>...
>
> @@ -6747,17 +6756,19 @@ qemu-kvm -net nic,model=? /dev/null
>model
>
>
> - Currently only the dimm model is supported in order to
> - add a virtual DIMM module to the guest.
> + Select dimm to add a virtual DIMM module to the guest.
Ugh... The 'dimm' Since tag is in the "Memory devices" section... Which
just make the following awkward. I think you should grab that 1.2.14
from above and place it here.
Rather than Select go with "Use" or "Provide"
> + Alternatively, nvdimm model adds a Non-Volatile DIMM
Use the same format - e.g.
"Use/Provide nvdimm to add a Non-Volatile DIMM module."
> + module. Since 2.2.0
Well we won't hit 2.2.0
>
>
>
>source
>
>
> - The optional source element allows to fine tune the source of the
> - memory used for the given memory device. If the element is not
> - provided defaults configured via numatune are used.
> + For model dimm this element is optional and allows to
> + fine tune the source of the memory used for the given memory
> device.
> + If the element is not provided defaults configured via
> + numatune are used.
>
>
>pagesize can optionally be used to override the
> default
> @@ -6770,6 +6781,11 @@ qemu-kvm -net nic,model=? /dev/null
>nodemask can optionally be used to override the
> default
>set of NUMA nodes where the memory would be allocated.
>
Since pagesize and nodemask are for DIMM only, so they probably need to
be converted to some sort of list or in some way indented to ensure the
visual cue is there that they are meant for dimm. Perhaps the end of
the dimm paragraph needs a "If source is provided, then at
least one of the following values would be provided:".
I think the only way to get the right formatting look is :
pagesize
...
nodemask
...
> +
> + For model nvdimm this element is mandatory and has a
> + single child element path which value represents a
> path
s/which value/that/ ?
> + in host that back the nvdimm module in the guest.
s/in host that back/in the host that backs/
Should there be any mention that this also requires "" to be set?
So something that isn't quite clear... For 'dimm', these are 'extra'
memory, while for 'nvdimm 'it seems to be one hunk that's really not
extra memory - rather it's memory that can be used for a specific
purpose. What's not clear to me - is the existing "physical" memory in
the guest (e.g. th
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Wednesday, August 31, 2016 12:17 AM
>
> Hi folks,
>
> At KVM Forum we had a BoF session primarily around the mediated device
> sysfs interface. I'd like to share what I think we agreed on and the
> "problem areas" that still need some work so we can get the thoughts
> and ideas from those who weren't able to attend.
>
> DanPB expressed some concern about the mdev_supported_types sysfs
> interface, which exposes a flat csv file with fields like "type",
> "number of instance", "vendor string", and then a bunch of type
> specific fields like "framebuffer size", "resolution", "frame rate
> limit", etc. This is not entirely machine parsing friendly and sort of
> abuses the sysfs concept of one value per file. Example output taken
> from Neo's libvirt RFC:
>
> cat /sys/bus/pci/devices/:86:00.0/mdev_supported_types
> # vgpu_type_id, vgpu_type, max_instance, num_heads, frl_config, framebuffer,
> max_resolution
> 11 ,"GRID M60-0B", 16, 2, 45, 512M,2560x1600
> 12 ,"GRID M60-0Q", 16, 2, 60, 512M,2560x1600
> 13 ,"GRID M60-1B", 8, 2, 45,1024M,2560x1600
> 14 ,"GRID M60-1Q", 8, 2, 60,1024M,2560x1600
> 15 ,"GRID M60-2B", 4, 2, 45,2048M,2560x1600
> 16 ,"GRID M60-2Q", 4, 4, 60,2048M,2560x1600
> 17 ,"GRID M60-4Q", 2, 4, 60,4096M,3840x2160
> 18 ,"GRID M60-8Q", 1, 4, 60,8192M,3840x2160
>
> The create/destroy then looks like this:
>
> echo "$mdev_UUID:vendor_specific_argument_list" >
> /sys/bus/pci/devices/.../mdev_create
>
> echo "$mdev_UUID:vendor_specific_argument_list" >
> /sys/bus/pci/devices/.../mdev_destroy
>
> "vendor_specific_argument_list" is nebulous.
>
> So the idea to fix this is to explode this into a directory structure,
> something like:
>
> ├── mdev_destroy
> └── mdev_supported_types
> ├── 11
> │ ├── create
> │ ├── description
> │ └── max_instances
> ├── 12
> │ ├── create
> │ ├── description
> │ └── max_instances
> └── 13
> ├── create
> ├── description
> └── max_instances
>
> Note that I'm only exposing the minimal attributes here for simplicity,
> the other attributes would be included in separate files and we would
> require vendors to create standard attributes for common device classes.
I like this idea. All standard attributes are reflected into this hierarchy.
In the meantime, can we still allow optional vendor string in create
interface? libvirt doesn't need to know the meaning, but allows upper
layer to do some vendor specific tweak if necessary.
>
> For vGPUs like NVIDIA where we don't support multiple types
> concurrently, this directory structure would update as mdev devices are
> created, removing no longer available types. I carried forward
or keep the type with max_instances cleared to ZERO.
> max_instances here, but perhaps we really want to copy SR-IOV and
> report a max and current allocation. Creation and deletion is
right, cur/max_instances look reasonable.
> simplified as we can simply "echo $UUID > create" per type. I don't
> understand why destroy had a parameter list, so here I imagine we can
> simply do the same... in fact, I'd actually rather see a "remove" sysfs
> entry under each mdev device, so we remove it at the device rather than
> in some central location (any objections?).
OK to me.
>
> We discussed how this might look with Intel devices which do allow
> mixed vGPU types concurrently. We believe, but need confirmation, that
> the vendor driver could still make a finite set of supported types,
> perhaps with additional module options to the vendor driver to enable
> more "exotic" types. So for instance if IGD vGPUs are based on
> power-of-2 portions of the framebuffer size, then the vendor driver
> could list types with 32MB, 64MB, 128MB, etc in useful and popular
> sizes. As vGPUs are allocated, the larger sizes may become unavailable.
Yes, Intel can do such type of definition. One thing I'm not sure is
about impact cross listed types, i.e. when creating a new instance
under a given type, max_instances under other types would be
dynamically decremented based on available resource. Would it be
a problem for libvirt or upper level stack, since a natural interpretation
of max_instances should be a static number?
An alternative is to make max_instances configurable, so libvirt has
chance to define a pool of available instances with different types
before creating any instance. For example, initially IGD driver may
report max_instances only for a minimal sharing granularity:
128MB:
max_instances (8)
256MB:
max_instances (0)
512MB:
max_instances (0)
Then libvirt can configure more types as:
On 08/30/2016 11:59 PM, Laine Stump wrote:
> On 08/01/2016 11:36 PM, Jim Fehlig wrote:
>> libvirt uses the new_id PCI sysfs interface to bind a PCI stub driver
>> to a PCI device. The new_id interface is known to be buggy and racey,
>> hence a more deterministic interface was introduced in the 3.12 kernel:
>> driver_override. For more details see
>>
>> https://www.redhat.com/archives/libvir-list/2016-June/msg02124.html
>
> That message details the change to the kernel that caused the regression for
> Xen, but not the driver_override interface. Maybe you could add a link to the
> kernel commit that adds driver_override:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pci-driver.c?h=v3.12&id=782a985d7af26db39e86070d28f987cad21313c0
Yep, good point. Nice to have a description of the interface and examples of its
use. I'll add it to the commit messages.
>
>
>
> Everything else looks good, and passes my tests for vfio device assignment
> (including when the host driver has been blacklisted).
>
> ACK. (Sorry I forgot about this earlier in the month :-/)
Thanks! I'll wait until 2.2.0 is out before pushing this.
Regards,
Jim
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/30/2016 03:04 PM, Daniel Veillard wrote:
It's tagged in git, with signed tarball and rpms at the usual place:
ftp://libvirt.org/libvirt/
but it looks broken to me, if I use virt manager and try to start a guest
it fails to render the grapical display of the guest. Need to kill virt-manager
as it blocks and become irresponsive, that's not good. virsh CLI still works
fine
but there is a new issue on the integration of console. I doubt it was
introduced
since RC1, 2.1.0 was fine.
I would be tempted to delay the final release until this is analyzed and
possibly
fixed (BTW I'm on standard Fedora 24 x86-64) assuming others can reproduce the
issue.
I didn't see this here. I'm using F24 + virt-preview, so these are my
package versions:
virt-manager-1.4.0-3.fc24
qemu-kvm-2.7.0-0.2.rc3
It could be related to the difference in these packages, or possibly
something about your guest - anything unusual or reportable about that?
Is it all guests or just a few?
Have you tried attaching gdb to the libvirtd process to see if one of
the threads is blocked?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 31 Aug 2016 15:04:13 +0800
Jike Song wrote:
> On 08/31/2016 02:12 PM, Tian, Kevin wrote:
> >> From: Alex Williamson [mailto:alex.william...@redhat.com]
> >> Sent: Wednesday, August 31, 2016 12:17 AM
> >>
> >> Hi folks,
> >>
> >> At KVM Forum we had a BoF session primarily around the mediated device
> >> sysfs interface. I'd like to share what I think we agreed on and the
> >> "problem areas" that still need some work so we can get the thoughts
> >> and ideas from those who weren't able to attend.
> >>
> >> DanPB expressed some concern about the mdev_supported_types sysfs
> >> interface, which exposes a flat csv file with fields like "type",
> >> "number of instance", "vendor string", and then a bunch of type
> >> specific fields like "framebuffer size", "resolution", "frame rate
> >> limit", etc. This is not entirely machine parsing friendly and sort of
> >> abuses the sysfs concept of one value per file. Example output taken
> >> from Neo's libvirt RFC:
> >>
> >> cat /sys/bus/pci/devices/:86:00.0/mdev_supported_types
> >> # vgpu_type_id, vgpu_type, max_instance, num_heads, frl_config,
> >> framebuffer,
> >> max_resolution
> >> 11 ,"GRID M60-0B", 16, 2, 45, 512M,2560x1600
> >> 12 ,"GRID M60-0Q", 16, 2, 60, 512M,2560x1600
> >> 13 ,"GRID M60-1B", 8, 2, 45,1024M,2560x1600
> >> 14 ,"GRID M60-1Q", 8, 2, 60,1024M,2560x1600
> >> 15 ,"GRID M60-2B", 4, 2, 45,2048M,2560x1600
> >> 16 ,"GRID M60-2Q", 4, 4, 60,2048M,2560x1600
> >> 17 ,"GRID M60-4Q", 2, 4, 60,4096M,3840x2160
> >> 18 ,"GRID M60-8Q", 1, 4, 60,8192M,3840x2160
> >>
> >> The create/destroy then looks like this:
> >>
> >> echo "$mdev_UUID:vendor_specific_argument_list" >
> >>/sys/bus/pci/devices/.../mdev_create
> >>
> >> echo "$mdev_UUID:vendor_specific_argument_list" >
> >>/sys/bus/pci/devices/.../mdev_destroy
> >>
> >> "vendor_specific_argument_list" is nebulous.
> >>
> >> So the idea to fix this is to explode this into a directory structure,
> >> something like:
> >>
> >> ├── mdev_destroy
> >> └── mdev_supported_types
> >> ├── 11
> >> │ ├── create
> >> │ ├── description
> >> │ └── max_instances
> >> ├── 12
> >> │ ├── create
> >> │ ├── description
> >> │ └── max_instances
> >> └── 13
> >> ├── create
> >> ├── description
> >> └── max_instances
> >>
> >> Note that I'm only exposing the minimal attributes here for simplicity,
> >> the other attributes would be included in separate files and we would
> >> require vendors to create standard attributes for common device classes.
> >
> > I like this idea. All standard attributes are reflected into this hierarchy.
> > In the meantime, can we still allow optional vendor string in create
> > interface? libvirt doesn't need to know the meaning, but allows upper
> > layer to do some vendor specific tweak if necessary.
> >
>
> Not sure whether this can done within MDEV framework (attrs provided by
> vendor driver of course), or must be within the vendor driver.
The purpose of the sub-directories is that libvirt doesn't need to pass
arbitrary, vendor strings to the create function, the attributes of the
mdev device created are defined by the attributes in the sysfs
directory where the create is done. The user only provides a uuid for
the device. Arbitrary vendor parameters are a barrier, libvirt may not
need to know the meaning, but would need to know when to apply them,
which is just as bad. Ultimately we want libvirt to be able to
interact with sysfs without having an vendor specific knowledge.
> >>
> >> For vGPUs like NVIDIA where we don't support multiple types
> >> concurrently, this directory structure would update as mdev devices are
> >> created, removing no longer available types. I carried forward
> >
> > or keep the type with max_instances cleared to ZERO.
> >
>
> +1 :)
Possible yes, but why would the vendor driver report types that the
user cannot create? It just seems like superfluous information (well,
except for the use I discover below).
> >> max_instances here, but perhaps we really want to copy SR-IOV and
> >> report a max and current allocation. Creation and deletion is
> >
> > right, cur/max_instances look reasonable.
> >
> >> simplified as we can simply "echo $UUID > create" per type. I don't
> >> understand why destroy had a parameter list, so here I imagine we can
> >> simply do the same... in fact, I'd actually rather see a "remove" sysfs
> >> entry under each mdev device, so we remove it at the device rather than
> >> in some central location (any objections?).
> >
> > OK to me.
>
> IIUC, "destroy" has a parameter list is only because the previous
> $VM_UUID + instnace implementation. It should be safe to move the "destroy"
> fil
On 08/31/2016 10:24 AM, Michal Privoznik wrote:
> On 31.08.2016 14:48, John Ferlan wrote:
[...]
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null
>>>
>>>
>>>
>>> - >> event_idx='off' queues='5'>
>>> + >> event_idx='off' queues='5' rxqueuesize='128'>
>>> >> ufo='off' mrg_rxbuf='off'/>
>>>
>>>
>>> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null
>>> virtio-net since 1.0.6 (QEMU and KVM
>>> only)
>>> vhost-user since 1.2.17 (QEMU and KVM
>>> only)
>>>
>>> + rxqueuesize
>>> +
>>> +The optional rxqueuesize attribute controls
>>> +the size of virtio ring for each queue as described above.
>>
>> Need a (I assume something similar to queues with
>> the QEMU and KVM only condition)
>>
>> Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf,
>> so adding the "_" at least mimics qemu's argument.
>
> That's what I wanted to prevent. Copying name from qemu. But I've
> struggled with the naming too (as can be seen - look how far I got :D).
> I don't have a strong opinion on either of those, so whatever we feel
> like I'll stick with that.
>
I guess I find it easier to be able to search qemu and libvirt code for
the same strings "if possible". I think we've already nixed allowing "-"
(dash), so those don't match up. I don't have a strong preference either.
>>
>> Furthermore, the bz has quite a bit of discussion regarding an
>> "appropriate value", so while I don't think it's something we want to
>> provide (that is suggested values), perhaps we could create a pointer or
>> at least a few hints. At the very least a this value needs to be a power
>> of 2 value... If not provided, the QEMU default of 256 is used. A
>> larger size gives you what advantage. In general some guidance on
>> setting or where to look could be helpful.
>
> Well, as you point out later in the review too, qemu might change these
> limitation anytime and we would advice untrue. But if I'm careful with
> wording we might be safe.
>
As you point out below - "This should be used by expert users" or as the
disk device "ioeventfd" and "event_idx" descriptions indicate in bold
letters" "In general you should leave this option alone, unless you are
very certain you know what you are doing."
>>
>>> +
>>>
>>>
>>>Offloading options for the host and guest can be configured using
[...]
>> e.g. extra space (although I do see the line is at 80 chars... could
>> change the internal name to rxqsz ;-)).
>>
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("rxqueuesize has to be a power of two"));
>>
>> ^^ hence why I think it should be documented.
>>
>> The bz also indicates there's a maximum of 1024. Should we check for
>> that? Although if the maximum increases we'd have to follow suit.
>> Naturally there isn't a way to get that maximum. If something larger
>> than 1024 is passed, then qemu will fail. Ugh, the hazards of adding
>> these 1-off things that have limits and rules, but we're not given all
>> the details necessary.
>
> Nope. Moreover, this feature is going to be used by expert users who
> know exactly what they are doing. So I'd say we're okay with just trying
> to start qemu and see if it fails or not. IOW too much work on our side
> not worth it.
>
>>
>>> +goto cleanup;
>>> +}
>>> }
>>>
>>> ret = 0;
>>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
>>> b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
>>> new file mode 100644
>>> index 000..e23d3e3
>>> --- /dev/null
>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
>>> @@ -0,0 +1,29 @@
>>> +
>>> + QEMUGuest1
>>> + c7a5fdbd-edaf-9455-926a-d65c16db1809
>>> + 219100
>>> + 219100
>>> + 1
>>> +
>>> +hvm
>>> +
>>> +
>>> +
>>> + destroy
>>> + restart
>>> + destroy
>>> +
>>> +/usr/bin/qemu
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>> +
>>>
>>
>> Shouldn't qemuxml2xmltest.c be updated to add this new test?
>
> Good catch.
>
As I'm going through your NVDIMM series - it has the same problem - so I
hunted down what I did for luks-disks... See commit id '9bbf0d7e' - it
adds a file link for the tests/qemuxml2xmloutdata/ to the
../qemuxml2argvdata/... I had just copied other examples.
And I also modified qemuxml2xmltest
>>
>> Should there be a "FAIL" test with an invalid va
On 31.08.2016 14:48, John Ferlan wrote:
>
>
> On 08/19/2016 07:54 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1366989
>>
>> Qemu grew yet another virtio-net tunable [1]. It basically allows
>
> s/Qemu grew yet/QEMU added/
>
>> users to set the size of RX virtio ring. But because virtio-net
>> uses two separate ring buffers to pass data from/to guest they
>> named it explicitly rx_queue_size. We should expose it in our XML
>> too.
>>
>> 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html
>
> Has this been merged yet? I see Jason Wang has it queue for net-next
> (for 2.8), but I don't see the code in the mainline qemu I just updated to.
No, I had to apply the patch locally and work over patched qemu. QEMU
devels are currently in freeze for 2.7 release (not sure what their
release date is planned on though).
>
>>
>> Signed-off-by: Michal Privoznik
>> ---
>> docs/formatdomain.html.in | 7 +-
>> docs/schemas/domaincommon.rng | 5
>> src/conf/domain_conf.c | 16
>> src/conf/domain_conf.h | 1 +
>> src/qemu/qemu_domain.c | 7 ++
>> .../qemuxml2argv-net-virtio-rxqueuesize.xml| 29
>> ++
>> 6 files changed, 64 insertions(+), 1 deletion(-)
>> create mode 100644
>> tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index bfbb0f2..6642dc8 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null
>>
>>
>>
>> - > event_idx='off' queues='5'>
>> + > event_idx='off' queues='5' rxqueuesize='128'>
>> > ufo='off' mrg_rxbuf='off'/>
>>
>>
>> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null
>> virtio-net since 1.0.6 (QEMU and KVM
>> only)
>> vhost-user since 1.2.17 (QEMU and KVM
>> only)
>>
>> + rxqueuesize
>> +
>> +The optional rxqueuesize attribute controls
>> +the size of virtio ring for each queue as described above.
>
> Need a (I assume something similar to queues with
> the QEMU and KVM only condition)
>
> Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf,
> so adding the "_" at least mimics qemu's argument.
That's what I wanted to prevent. Copying name from qemu. But I've
struggled with the naming too (as can be seen - look how far I got :D).
I don't have a strong opinion on either of those, so whatever we feel
like I'll stick with that.
>
> Furthermore, the bz has quite a bit of discussion regarding an
> "appropriate value", so while I don't think it's something we want to
> provide (that is suggested values), perhaps we could create a pointer or
> at least a few hints. At the very least a this value needs to be a power
> of 2 value... If not provided, the QEMU default of 256 is used. A
> larger size gives you what advantage. In general some guidance on
> setting or where to look could be helpful.
Well, as you point out later in the review too, qemu might change these
limitation anytime and we would advice untrue. But if I'm careful with
wording we might be safe.
>
>> +
>>
>>
>>Offloading options for the host and guest can be configured using
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 052f28c..4b89a86 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -2529,6 +2529,11 @@
>>
>>
>>
>> +
>> +
>> +
>> +
>> +
>>
>>
>> iothread
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 14d4f7d..08cde19 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> char *ioeventfd = NULL;
>> char *event_idx = NULL;
>> char *queues = NULL;
>> +char *rxqueuesize = NULL;
>> char *str = NULL;
>> char *filter = NULL;
>> char *internal = NULL;
>> @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>> ioeventfd = virXMLPropString(cur, "ioeventfd");
>> event_idx = virXMLPropString(cur, "event_idx");
>> queues = virXMLPropString(cur, "queues");
>> +
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
> Just like in the previous commit, teach qemu driver to detect
> whether qemu supports this configuration knob or not.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_capabilities.c | 2 ++
> src/qemu/qemu_capabilities.h | 1 +
> 2 files changed, 3 insertions(+)
>
This will need a tweak since it conflicts with "query-hotpluggable-cpus"
Looks like we'll need to add some tests/qemucapabilitiesdata/* files for
2.8 (caps_2.8.0.x86_64.{xml|replies})
Conditional ACK...
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/19/2016 07:54 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1366989
>
> Qemu grew yet another virtio-net tunable [1]. It basically allows
s/Qemu grew yet/QEMU added/
> users to set the size of RX virtio ring. But because virtio-net
> uses two separate ring buffers to pass data from/to guest they
> named it explicitly rx_queue_size. We should expose it in our XML
> too.
>
> 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html
Has this been merged yet? I see Jason Wang has it queue for net-next
(for 2.8), but I don't see the code in the mainline qemu I just updated to.
>
> Signed-off-by: Michal Privoznik
> ---
> docs/formatdomain.html.in | 7 +-
> docs/schemas/domaincommon.rng | 5
> src/conf/domain_conf.c | 16
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_domain.c | 7 ++
> .../qemuxml2argv-net-virtio-rxqueuesize.xml| 29
> ++
> 6 files changed, 64 insertions(+), 1 deletion(-)
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index bfbb0f2..6642dc8 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null
>
>
>
> - event_idx='off' queues='5'>
> + event_idx='off' queues='5' rxqueuesize='128'>
> ufo='off' mrg_rxbuf='off'/>
>
>
> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null
> virtio-net since 1.0.6 (QEMU and KVM only)
> vhost-user since 1.2.17 (QEMU and KVM
> only)
>
> + rxqueuesize
> +
> +The optional rxqueuesize attribute controls
> +the size of virtio ring for each queue as described above.
Need a (I assume something similar to queues with
the QEMU and KVM only condition)
Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf,
so adding the "_" at least mimics qemu's argument.
Furthermore, the bz has quite a bit of discussion regarding an
"appropriate value", so while I don't think it's something we want to
provide (that is suggested values), perhaps we could create a pointer or
at least a few hints. At the very least a this value needs to be a power
of 2 value... If not provided, the QEMU default of 256 is used. A
larger size gives you what advantage. In general some guidance on
setting or where to look could be helpful.
> +
>
>
>Offloading options for the host and guest can be configured using
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 052f28c..4b89a86 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2529,6 +2529,11 @@
>
>
>
> +
> +
> +
> +
> +
>
>
> iothread
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 14d4f7d..08cde19 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> char *ioeventfd = NULL;
> char *event_idx = NULL;
> char *queues = NULL;
> +char *rxqueuesize = NULL;
> char *str = NULL;
> char *filter = NULL;
> char *internal = NULL;
> @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> ioeventfd = virXMLPropString(cur, "ioeventfd");
> event_idx = virXMLPropString(cur, "event_idx");
> queues = virXMLPropString(cur, "queues");
> +rxqueuesize = virXMLPropString(cur, "rxqueuesize");
> } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) {
> if (filter) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -9466,6 +9468,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> if (q > 1)
> def->driver.virtio.queues = q;
> }
> +if (rxqueuesize) {
> +unsigned int q;
> +if (virStrToLong_uip(rxqueuesize, NULL, 10, &q) < 0) {
> +virReportError(VIR_ERR_XML_DETAIL,
> + _("'rxqueuesize' attribute must be positive
> number: %s"),
> + rxqueuesize);
> +goto error;
> +
On Mon, Aug 29, 2016 at 16:42:04 +0530, Madhu Pavan wrote:
>
>
> On 08/27/2016 02:21 AM, Jiri Denemark wrote:
> > On Fri, Aug 26, 2016 at 21:41:31 +0200, Michal Privoznik wrote:
> >> On 26.08.2016 11:25, Kothapally Madhu Pavan wrote:
> >>> Unlike postcopy migration there is no --live flag check for
> >>> postcopy-after-precopy.
> >>>
> >>> Signed-off-by: Kothapally Madhu Pavan
> >>> ---
> >>> tools/virsh-domain.c |6 ++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >> ACKed and pushed.
> > This doesn't make any sense. First, post-copy migration is enabled with
> > --postcopy option to migrate command and --postcopy-after-precopy is
> > just an additional flag for post-copy migration. So if virsh was to
> > report such an error, it should check for --postcopy option. But such
> > check doesn't belong to libvirt at all, the appropriate libvirt driver
> > is supposed to check for the flags and report invalid combinations.
> I have proposed this patch as the qemu driver doesn't have
> postcopy-after-precopy
> flag and this bug can be fixed by minimal changes in libvirt. If we have
> to check for
> invalid combinations in appropriate libvirt drivers, we need to create a
> flag for
> postcopy-after-precopy migration. I will be happy to send another patch
> if this is what
> needed.
Heh, you're right indeed. I think I really shouldn't try reviewing stuff
during a conference. So the place is correct, but I still think it
should be done in a different way. As I said --postcopy-after-precopy is
just an additional flag for --postcopy and thus we should check that
--postcopy is present rather than checking for --live and the error
message should reflect that (e.g., "--postcopy-after-precopy can only be
used with --postcopy").
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Aug 31, 2016 at 12:55:57 +0200, Michal Privoznik wrote:
> When we wanted to break huge and unmaintainable virsh into
> smaller files first thing we did was to just move funcs into
> virsh-.c files and then #include them from virsh. Having it done
> this way we also needed to have them listed under EXTRA_DIST.
> However, things got changed since then and now all the virsh-*.c
> files are proper source files. Therefore they are listed under
> virsh_SOURCES too. But for some reason we forgot to remove them
> from EXTRA_DIST.
>
> Signed-off-by: Michal Privoznik
> ---
> tools/Makefile.am | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index a01c58d..e7e42c3 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> @@ -64,13 +64,6 @@ EXTRA_DIST = \
> libvirt-guests.sysconf \
> virt-login-shell.conf \
> virsh-edit.c\
> - virsh-domain.c \
> - virsh-domain-monitor.c \
> - virsh-host.c virsh-interface.c \
> - virsh-network.c virsh-nodedev.c \
> - virsh-nwfilter.c virsh-pool.c \
> - virsh-secret.c virsh-snapshot.c \
> - virsh-volume.c \
> $(PODFILES) \
> $(MANINFILES) \
> $(NULL)
ACK
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/25/2016 05:04 AM, Nikolay Shirokovskiy wrote:
>
>
> On 30.07.2016 16:04, John Ferlan wrote:
>> s/$subj/qemu: Filter cur_balloon ABI check for certain transactions
>>
>> On 06/09/2016 10:32 AM, Nikolay Shirokovskiy wrote:
>>> cur_balloon value can change in between preparing external config and
>>> using it in operations like save and migrate. As a resutl operation will
>>> fail for ABI inconsistency. cur_balloon changes can not be predicted
>>> generally and thus operations will fail from time to time.
>>>
>>> Skip checking cur_balloon if domain lock can not be hold between
>>> preparing external config outside of libvirt and checking it against active
>>> config. Instead update cur_balloon value in external config from active
>>> config.
>>> This way it is protected from forges and is keeped up to date too.
>>>
>>
>> s/$commit/
>>
>> Since the domain lock is not held during preparation of an external XML
>> config, it is possible that the value can change resulting in unexpected
>> failures during ABI consistency checking for some save and migrate
>> operations.
>>
>> This patch adds a new flag to skip the checking of the cur_balloon value
>> and then sets the destination value to the source value to ensure
>> subsequent checks without the skip flag will succeed.
>>
>>> Signed-off-by: Nikolay Shirokovskiy
>>> ---
>>> src/conf/domain_conf.c| 14 +++---
>>> src/conf/domain_conf.h| 9 +
>>> src/libvirt_private.syms | 1 +
>>> src/qemu/qemu_domain.c| 29 -
>>> src/qemu/qemu_domain.h| 6 +++---
>>> src/qemu/qemu_driver.c| 5 +++--
>>> src/qemu/qemu_migration.c | 4 ++--
>>> 7 files changed, 49 insertions(+), 19 deletions(-)
>>>
>>
>> This change seems reasonable to me and it doesn't seem to negate the
>> primary focus of commit id 'f2fc1eee' (which added the checks).
>>
>> I do have a few suggestions and can make those locally before pushing
>> after the current release is cut.
>>
>>
>
> Thanx! I argee to all the suggestions, especially to keeping the name of the
> function
> that checks ABI. After all its name reflects 99% of its functionality and it
> is
> idiomatic for 'check' functions to have side effects in this project.
>
OK - so I'll make the adjustments including going back to the original
name qemuDomainDefCheckABIStability as a bool and will push after the
release is out.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/31/2016 10:18 AM, Martin Kletzander wrote:
> On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote:
>>
>> On 08/30/2016 11:20 PM, Joao Martins wrote:
>>> Hey!
>>>
>>> On 08/30/2016 11:00 AM, Bob Liu wrote:
Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in
libxl
driver, using libxl_set_memory_target in xen libxl.
With "virsh attach-device" command we can then hotplug memory to a domain:
128
0
$ virsh attach-device domain_name this.xml --live
>
> Actually, attaching a memory device should do something else than
> calling libxl_set_memory_target(). It should add a guest-visible memory
> device into the DIMM slot (especially when it is model='dimm'). I'm no
> libxl expert, but the function you are using is just setting the memory
> IMHO and it is the same as if you would remove the check for:
> newmem > virDomainDefGetMemoryTotal
> in libxlDomainSetMemoryFlags()
As a guest visible DIMM slot, doesn't appear to be supported at least as far as
goes
my reading of libxl on staging. In which case you probably suggest dropping this
patch as it's not meant to be doing this? I misconcepted AttachDevice with
VIR_DOMAIN_DEVICE_MEMORY (thinking this could another way of increasing the
balloon
other than libxlDomainSetMemory*), despite seeing now the XML/docs being obvious
about its sole purpose.
Joao
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
When we wanted to break huge and unmaintainable virsh into
smaller files first thing we did was to just move funcs into
virsh-.c files and then #include them from virsh. Having it done
this way we also needed to have them listed under EXTRA_DIST.
However, things got changed since then and now all the virsh-*.c
files are proper source files. Therefore they are listed under
virsh_SOURCES too. But for some reason we forgot to remove them
from EXTRA_DIST.
Signed-off-by: Michal Privoznik
---
tools/Makefile.am | 7 ---
1 file changed, 7 deletions(-)
diff --git a/tools/Makefile.am b/tools/Makefile.am
index a01c58d..e7e42c3 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -64,13 +64,6 @@ EXTRA_DIST = \
libvirt-guests.sysconf \
virt-login-shell.conf \
virsh-edit.c\
- virsh-domain.c \
- virsh-domain-monitor.c \
- virsh-host.c virsh-interface.c \
- virsh-network.c virsh-nodedev.c \
- virsh-nwfilter.c virsh-pool.c \
- virsh-secret.c virsh-snapshot.c \
- virsh-volume.c \
$(PODFILES) \
$(MANINFILES) \
$(NULL)
--
2.8.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Aug 31, 2016 at 03:56:02PM +0800, Bob Liu wrote:
On 08/30/2016 11:20 PM, Joao Martins wrote:
Hey!
On 08/30/2016 11:00 AM, Bob Liu wrote:
Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl
driver, using libxl_set_memory_target in xen libxl.
With "virsh attach-device" command we can then hotplug memory to a domain:
128
0
$ virsh attach-device domain_name this.xml --live
Actually, attaching a memory device should do something else than
calling libxl_set_memory_target(). It should add a guest-visible memory
device into the DIMM slot (especially when it is model='dimm'). I'm no
libxl expert, but the function you are using is just setting the memory
IMHO and it is the same as if you would remove the check for:
newmem > virDomainDefGetMemoryTotal
in libxlDomainSetMemoryFlags()
Signed-off-by: Bob Liu
See few very minor comments below, but overall LGTM.
---
src/libxl/libxl_domain.c | 1 +
src/libxl/libxl_driver.c | 29 +
2 files changed, 30 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index f529a2e..3924ba0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
.macPrefix = { 0x00, 0x16, 0x3e },
.devicesPostParseCallback = libxlDomainDeviceDefPostParse,
.domainPostParseCallback = libxlDomainDefPostParse,
+.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
};
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a34eb02..541ea3b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr
driver,
#endif
static int
+libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
+virDomainObjPtr vm,
+virDomainMemoryDefPtr mem)
+{
+int res = 0;
I think you don't need to initialize the variable.
+libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+
+if (mem->targetNode != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupport non-zero target node for memory device"));
Probably changing the message to "non-zero target node not supported" instead?
The
messages sounds like you are removing support for it. But english is not my
mothertongue so may be it's just my wrong reading :)
Should we fail with other error as this patch, or VIR_WARN the user and ignore
Agree to use VIR_WARN(), will be updated.
I think definitely fail. Otherwise you succeed with something that the
user clearly did not want.
targetNode. AFAIK libxl doesn't yet support ballooning for a specific node.
What do
others think?
And it's said here as well, you are changing ballooning, which memory
hot-plug should not do.
+return -1;
+}
+
+res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1);
Should we unlock virDomainObj while ballooning, as similarly done in
libxlDomainSetMemory* ?
Yes, that's necessary.
+if (res < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to attach %lluKB memory for domain %d"),
+ mem->size, vm->def->id);
+return -1;
+}
+return 0;
+}
+
+static int
libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
@@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
dev->data.hostdev = NULL;
break;
+case VIR_DOMAIN_DEVICE_MEMORY:
+ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
+dev->data.memory = NULL;
This should probably be:
ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
if (!ret)
dev->data.memory = NULL;
Right?
This code was from qemu:
7408 /* note that qemuDomainAttachMemory always consumes
dev->data.memory
7409 * and dispatches DeviceAdded event on success */
7410 ret = qemuDomainAttachMemory(driver, vm,
7411 dev->data.memory);
7412 dev->data.memory = NULL;
But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the
reminding.
qemu uses it because it calls virDomainMemoryInsert() which steals the
pointer data. You are leaking the memory here. Yes, you can free the
data in libxlDomainAttachMemory(), but in case it is not needed (like in
qemu), it is just creating ugly code. Also looking at qemu's
implementation, there are bunch of safety checks and domain definition
updates which are definitely not done in this patch.
+break;
+
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("device type '%s' cannot be attached"),
--
Regards,
-Bob
--
libvir-list m
On 08/31/2016 08:56 AM, Bob Liu wrote:
>
> On 08/30/2016 11:20 PM, Joao Martins wrote:
>> Hey!
>>
>> On 08/30/2016 11:00 AM, Bob Liu wrote:
>>> Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl
>>> driver, using libxl_set_memory_target in xen libxl.
>>>
>>> With "virsh attach-device" command we can then hotplug memory to a domain:
>>>
>>>
>>> 128
>>> 0
>>>
>>>
>>>
>>> $ virsh attach-device domain_name this.xml --live
>>>
>>> Signed-off-by: Bob Liu
>> See few very minor comments below, but overall LGTM.
>>
>>> ---
>>> src/libxl/libxl_domain.c | 1 +
>>> src/libxl/libxl_driver.c | 29 +
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>> index f529a2e..3924ba0 100644
>>> --- a/src/libxl/libxl_domain.c
>>> +++ b/src/libxl/libxl_domain.c
>>> @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
>>> .macPrefix = { 0x00, 0x16, 0x3e },
>>> .devicesPostParseCallback = libxlDomainDeviceDefPostParse,
>>> .domainPostParseCallback = libxlDomainDefPostParse,
>>> +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
>>> };
>>>
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index a34eb02..541ea3b 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr
>>> driver,
>>> #endif
>>>
>>> static int
>>> +libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
>>> +virDomainObjPtr vm,
>>> +virDomainMemoryDefPtr mem)
>>> +{
>>> +int res = 0;
>> I think you don't need to initialize the variable.
>>
>>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>>> +
>>> +if (mem->targetNode != 0) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("unsupport non-zero target node for memory
>>> device"));
>> Probably changing the message to "non-zero target node not supported"
>> instead? The
>> messages sounds like you are removing support for it. But english is not my
>> mothertongue so may be it's just my wrong reading :)
>>
>> Should we fail with other error as this patch, or VIR_WARN the user and
>> ignore
>
> Agree to use VIR_WARN(), will be updated.
Ah sorry, I forgot to add a "?" in this sentence, and I am not sure what would
the
correct behavior here. I think the current is correct, and I assume user
wouldn't try
to memory balloon to a node other than 0 as it can't create a guest with
multiple
nodes either. So probably it's reasonable to left it as is, unless someone
raises a
flag to loose the restriction?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/30/2016 11:20 PM, Joao Martins wrote:
> Hey!
>
> On 08/30/2016 11:00 AM, Bob Liu wrote:
>> Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl
>> driver, using libxl_set_memory_target in xen libxl.
>>
>> With "virsh attach-device" command we can then hotplug memory to a domain:
>>
>>
>> 128
>> 0
>>
>>
>>
>> $ virsh attach-device domain_name this.xml --live
>>
>> Signed-off-by: Bob Liu
> See few very minor comments below, but overall LGTM.
>
>> ---
>> src/libxl/libxl_domain.c | 1 +
>> src/libxl/libxl_driver.c | 29 +
>> 2 files changed, 30 insertions(+)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index f529a2e..3924ba0 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
>> .macPrefix = { 0x00, 0x16, 0x3e },
>> .devicesPostParseCallback = libxlDomainDeviceDefPostParse,
>> .domainPostParseCallback = libxlDomainDefPostParse,
>> +.features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
>> };
>>
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index a34eb02..541ea3b 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr
>> driver,
>> #endif
>>
>> static int
>> +libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
>> +virDomainObjPtr vm,
>> +virDomainMemoryDefPtr mem)
>> +{
>> +int res = 0;
> I think you don't need to initialize the variable.
>
>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +
>> +if (mem->targetNode != 0) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("unsupport non-zero target node for memory
>> device"));
> Probably changing the message to "non-zero target node not supported"
> instead? The
> messages sounds like you are removing support for it. But english is not my
> mothertongue so may be it's just my wrong reading :)
>
> Should we fail with other error as this patch, or VIR_WARN the user and ignore
Agree to use VIR_WARN(), will be updated.
> targetNode. AFAIK libxl doesn't yet support ballooning for a specific node.
> What do
> others think?
>
>> +return -1;
>> +}
>> +
>> +res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1);
> Should we unlock virDomainObj while ballooning, as similarly done in
> libxlDomainSetMemory* ?
>
Yes, that's necessary.
>> +if (res < 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to attach %lluKB memory for domain %d"),
>> + mem->size, vm->def->id);
>> +return -1;
>> +}
>> +return 0;
>> +}
>> +
>> +static int
>> libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
>> virDomainObjPtr vm,
>> virDomainHostdevDefPtr hostdev)
>> @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr
>> driver,
>> dev->data.hostdev = NULL;
>> break;
>>
>> +case VIR_DOMAIN_DEVICE_MEMORY:
>> +ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
>> +dev->data.memory = NULL;
> This should probably be:
>
> ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
> if (!ret)
> dev->data.memory = NULL;
>
> Right?
>
This code was from qemu:
7408 /* note that qemuDomainAttachMemory always consumes
dev->data.memory
7409 * and dispatches DeviceAdded event on success */
7410 ret = qemuDomainAttachMemory(driver, vm,
7411 dev->data.memory);
7412 dev->data.memory = NULL;
But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the
reminding.
>> +break;
>> +
>> default:
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("device type '%s' cannot be attached"),
>>
--
Regards,
-Bob
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 08/31/2016 02:12 PM, Tian, Kevin wrote:
>> From: Alex Williamson [mailto:alex.william...@redhat.com]
>> Sent: Wednesday, August 31, 2016 12:17 AM
>>
>> Hi folks,
>>
>> At KVM Forum we had a BoF session primarily around the mediated device
>> sysfs interface. I'd like to share what I think we agreed on and the
>> "problem areas" that still need some work so we can get the thoughts
>> and ideas from those who weren't able to attend.
>>
>> DanPB expressed some concern about the mdev_supported_types sysfs
>> interface, which exposes a flat csv file with fields like "type",
>> "number of instance", "vendor string", and then a bunch of type
>> specific fields like "framebuffer size", "resolution", "frame rate
>> limit", etc. This is not entirely machine parsing friendly and sort of
>> abuses the sysfs concept of one value per file. Example output taken
>> from Neo's libvirt RFC:
>>
>> cat /sys/bus/pci/devices/:86:00.0/mdev_supported_types
>> # vgpu_type_id, vgpu_type, max_instance, num_heads, frl_config, framebuffer,
>> max_resolution
>> 11 ,"GRID M60-0B", 16, 2, 45, 512M,2560x1600
>> 12 ,"GRID M60-0Q", 16, 2, 60, 512M,2560x1600
>> 13 ,"GRID M60-1B", 8, 2, 45,1024M,2560x1600
>> 14 ,"GRID M60-1Q", 8, 2, 60,1024M,2560x1600
>> 15 ,"GRID M60-2B", 4, 2, 45,2048M,2560x1600
>> 16 ,"GRID M60-2Q", 4, 4, 60,2048M,2560x1600
>> 17 ,"GRID M60-4Q", 2, 4, 60,4096M,3840x2160
>> 18 ,"GRID M60-8Q", 1, 4, 60,8192M,3840x2160
>>
>> The create/destroy then looks like this:
>>
>> echo "$mdev_UUID:vendor_specific_argument_list" >
>> /sys/bus/pci/devices/.../mdev_create
>>
>> echo "$mdev_UUID:vendor_specific_argument_list" >
>> /sys/bus/pci/devices/.../mdev_destroy
>>
>> "vendor_specific_argument_list" is nebulous.
>>
>> So the idea to fix this is to explode this into a directory structure,
>> something like:
>>
>> ├── mdev_destroy
>> └── mdev_supported_types
>> ├── 11
>> │ ├── create
>> │ ├── description
>> │ └── max_instances
>> ├── 12
>> │ ├── create
>> │ ├── description
>> │ └── max_instances
>> └── 13
>> ├── create
>> ├── description
>> └── max_instances
>>
>> Note that I'm only exposing the minimal attributes here for simplicity,
>> the other attributes would be included in separate files and we would
>> require vendors to create standard attributes for common device classes.
>
> I like this idea. All standard attributes are reflected into this hierarchy.
> In the meantime, can we still allow optional vendor string in create
> interface? libvirt doesn't need to know the meaning, but allows upper
> layer to do some vendor specific tweak if necessary.
>
Not sure whether this can done within MDEV framework (attrs provided by
vendor driver of course), or must be within the vendor driver.
>>
>> For vGPUs like NVIDIA where we don't support multiple types
>> concurrently, this directory structure would update as mdev devices are
>> created, removing no longer available types. I carried forward
>
> or keep the type with max_instances cleared to ZERO.
>
+1 :)
>> max_instances here, but perhaps we really want to copy SR-IOV and
>> report a max and current allocation. Creation and deletion is
>
> right, cur/max_instances look reasonable.
>
>> simplified as we can simply "echo $UUID > create" per type. I don't
>> understand why destroy had a parameter list, so here I imagine we can
>> simply do the same... in fact, I'd actually rather see a "remove" sysfs
>> entry under each mdev device, so we remove it at the device rather than
>> in some central location (any objections?).
>
> OK to me.
IIUC, "destroy" has a parameter list is only because the previous
$VM_UUID + instnace implementation. It should be safe to move the "destroy"
file under mdev now.
>> We discussed how this might look with Intel devices which do allow
>> mixed vGPU types concurrently. We believe, but need confirmation, that
>> the vendor driver could still make a finite set of supported types,
>> perhaps with additional module options to the vendor driver to enable
>> more "exotic" types. So for instance if IGD vGPUs are based on
>> power-of-2 portions of the framebuffer size, then the vendor driver
>> could list types with 32MB, 64MB, 128MB, etc in useful and popular
>> sizes. As vGPUs are allocated, the larger sizes may become unavailable.
>
> Yes, Intel can do such type of definition. One thing I'm not sure is
> about impact cross listed types, i.e. when creating a new instance
> under a given type, max_instances under other types would be
> dynamically decremented based on available resource. Would it be
> a problem for libvirt or upper level stack, since a natural interpretation
> of max_instances should be a static number?
>
