On Thu, Nov 15, 2018 at 12:36 AM Andrea Bolognani
wrote:
> On Wed, 2018-11-14 at 15:19 +0800, Han Han wrote:
> > When listing snapshot with VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, it
> > always returns 0 or no snapshot. Because we never implement funtions
> > to list no-metadata snapshot in virDomainSnapshotObjListGetNames():
> >
> > if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
> > VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
> > return 0;
> >
> > Add notes for that flag.
> >
> > Please update the comment and man page of that flag when no-metadata
> > snapshot list is implemented in the future.
>
> I could be missing some information, but from a quick look at the
> commit message and the patch it looks to me like you're documenting
> a known limitation instead of, you know, addressing it :)
>
> Bug filed as : https://bugzilla.redhat.com/show_bug.cgi?id=1650419
> If you are able to fix the issue yourself, then please do so;
> otherwise, filing a bug seems like it would be a more appropriate
> course of action.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
>
--
Best regards,
---
Han Han
Quality Engineer
Redhat.
Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 15, 2018 at 05:29:24PM +0100, Andrea Bolognani wrote:
> On Wed, 2018-11-14 at 21:38 -0200, Eduardo Habkost wrote:
> > Many of the current virtio-*-pci device types actually represent
> > 3 different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > That would be just an annoyance if it didn't break our device/bus
> > compatibility QMP interfaces. With this multi-purpose device
> > type, there's no way to tell management software that
> > transitional devices and legacy devices require a Conventional
> > PCI bus.
> >
> > The multi-purpose device types would also prevent us from telling
> > management software what's the PCI vendor/device ID for them,
> > because their PCI IDs change at runtime depending on the bus
> > where they were plugged.
> >
> > This patch adds separate device types for each of those virtio
> > device flavors:
> >
> > - virtio-*-pci: the existing multi-purpose device types
> > - Configurable using `disable-legacy` and `disable-modern`
> > properties
> > - Legacy driver support is automatically enabled/disabled
> > depending on the bus where it is plugged
> > - Supports Conventional PCI and PCI Express buses
> > (but Conventional PCI is incompatible with
> > disable-legacy=off)
> > - Changes PCI vendor/device IDs at runtime
> > - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
> > - virtio-*-pci-non-transitional: modern-only
> > - Supports both Conventional PCI and PCI Express buses
>
> So, my understanding was that transitional devices would be suitable
> for both PCI and PCIe slots and non-transitional devices could only
> work in PCIe slots, but based on the above it looks like I got it
> pretty much completely wrong? I'm not too surprised that would be
> the case, to be honest: keeping this stuff straight in my head has
> always been a bit of a challenge, so I can't possibly not welcome a
> proposal like this, which will spell it out a bit more :)
That's possibly my fault. I described it completely wrong in one
message in the v1 thread.
>
> Let me try to map the interactions out:
>
> * virtio-*-pci-transitional
> + plugged into PCI slot
> - shows up as vendor1/device1
> + plugged into PCIe slot
> - doesn't work
>
> * virtio-*-pci-non-transitional
> + plugged into PCI slot
> - shows up as vendor2/device2
> + plugged into PCIe slot
> - shows up as vendor2/device2
>
> * virtio-*-pci
> + plugged into PCI slot
> - shows up as vendor1/device1
> (same as virtio-*-pci-transitional)
> + plugged into PCIe slot
> - shows up as vendor2/device2
> (same as virtio-*-pci-non-transitional)
>
> Does that look about right?
Exactly.
>
> Once all the various pieces have fallen into place, when adding a
> device to a guest running a modern OS we would find out through
> libosinfo that it supports vendor2/device2 (and vendor1/device1
> too, I guess?) so we would choose the non-transitional variant and
> plug it into PCIe when possible (q35) or PCI otherwise (pc); on
> the other hand, an older guest OS like CentOS 6 will only advertise
> support for vendor1/device1, so we'd have to use the transitional
> variant instead and plug it into a PCI slot regardless of the
> machine type, which more specifically means building a
> pcie.0 <- pcie-root-port <- pcie-pci-bridge topology for q35
> guests.
>
> If all of the above is correct, then it sounds like a feasible
> enough plan to me, though of course it be a long time before users
> and management applications can rely on these new device types
> being available in downstream distributions...
>
> One thing that I'm very much not convinced about is the naming,
> specifically leaving the virtio revision out: I get it that we
> Should Never Need™ another major version of the spec, but I'm
> afraid discounting the possibility outright might prove to be
> shortsighted and come back to bite us later, so I'd much rather
> keep it.
>
> And once that's done, "non-transitional" (while matching the
> language of the spec) starts to look a bit unnecessary when you
> could simply have
>
> virtio-*-pci
> virtio-*-pci-1
> virtio-*-pci-1-transitional
>
> instead. But I don't feel as strongly about this as I do about
> keeping the virtio revision in the device name :)
I like that suggestion. Makes the device names more explicit
_and_ shorter. I'll do that in v3.
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
> v2:
> - original patch was split in 3 patches as John Ferlan requested;
> - patches 4 and 5 are cleanups;
> - first patch link:
> https://www.redhat.com/archives/libvir-list/2018-October/msg00251.html
>
>
>
> Daniel Henrique Barboza (5):
> qemu_process.c: adding maxCpus value to error message
> qemu_process.c: make qemuValidateCpuCount public
> qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate
> qemu_process.c: moving qemuValidateCpuCount to qemu_domain.c
> qemu_process.c: removing qemuProcessStartValidateXML
>
> src/qemu/qemu_domain.c | 27 +++
> src/qemu/qemu_process.c | 57 -
> 2 files changed, 32 insertions(+), 52 deletions(-)
>
Reviewed-by: John Ferlan
(series) and pushed.
Congrats on your first libvirt patches.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/18 7:40 PM, John Ferlan wrote:
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
v2:
- original patch was split in 3 patches as John Ferlan requested;
- patches 4 and 5 are cleanups;
- first patch link:
https://www.redhat.com/archives/libvir-list/2018-October/msg00251.html
Daniel Henrique Barboza (5):
qemu_process.c: adding maxCpus value to error message
qemu_process.c: make qemuValidateCpuCount public
qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate
qemu_process.c: moving qemuValidateCpuCount to qemu_domain.c
qemu_process.c: removing qemuProcessStartValidateXML
src/qemu/qemu_domain.c | 27 +++
src/qemu/qemu_process.c | 57 -
2 files changed, 32 insertions(+), 52 deletions(-)
Reviewed-by: John Ferlan
(series) and pushed.
Congrats on your first libvirt patches.
Thanks! Looking forward for more!
Daniel
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/18 7:38 PM, John Ferlan wrote:
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
Adding maxCpu validation in qemuDomainDefValidate allows the user to
spot over the board maxCpus counts at editing time, instead of
facing a runtime error when starting the domain. This check is also
arch independent.
This leaves us with 2 calls to qemuProcessValidateCpuCount: one in
qemuProcessStartValidateXML and the new one at qemuDomainDefValidate.
The call in qemuProcessStartValidateXML is redundant. Following
up in that code, there is a call to virDomainDefValidate, which
in turn will call config.domainValidateCallback. In this case, the
callback function is qemuDomainDefValidate. This means that, on startup
time, qemuProcessValidateCpuCount will be called twice.
To avoid that, let's also remove the qemuProcessValidateCpuCount call
from qemuProcessStartValidateXML.
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 4
src/qemu/qemu_process.c | 14 +-
2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 37926850b2..3b86d28216 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def,
}
}
+if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) {
+goto cleanup;
+}
+
make check would have told you to remove the { ... } for the one line
goto cleanup;
I'll take care of it for you (as well as the merge conflict it creates
in the subsequent patch).
I appreciate it. I'll make sure to execute 'make check' before sending
the patch next time.
Thanks,
Daniel
John
if (ARCH_IS_X86(def->os.arch) &&
virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) {
if (!qemuDomainIsQ35(def)) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4d134bd7be..2adbf375ee 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm,
static int
qemuProcessStartValidateXML(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-virQEMUCapsPtr qemuCaps,
virCapsPtr caps,
unsigned int flags)
{
-/* The bits we validate here are XML configs that we previously
- * accepted. We reject them at VM startup time rather than parse
- * time so that pre-existing VMs aren't rejected and dropped from
- * the VM list when libvirt is updated.
- *
- * If back compat isn't a concern, XML validation should probably
- * be done at parse time.
- */
-if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0)
-return -1;
-
/* checks below should not be executed when starting a qemu process for a
* VM that was running before (migration, snapshots, save). It's more
* important to start such VM than keep the configuration clean */
@@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
}
-if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0)
+if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0)
return -1;
if (qemuProcessStartValidateGraphics(vm) < 0)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
> Adding maxCpu validation in qemuDomainDefValidate allows the user to
> spot over the board maxCpus counts at editing time, instead of
> facing a runtime error when starting the domain. This check is also
> arch independent.
>
> This leaves us with 2 calls to qemuProcessValidateCpuCount: one in
> qemuProcessStartValidateXML and the new one at qemuDomainDefValidate.
>
> The call in qemuProcessStartValidateXML is redundant. Following
> up in that code, there is a call to virDomainDefValidate, which
> in turn will call config.domainValidateCallback. In this case, the
> callback function is qemuDomainDefValidate. This means that, on startup
> time, qemuProcessValidateCpuCount will be called twice.
>
> To avoid that, let's also remove the qemuProcessValidateCpuCount call
> from qemuProcessStartValidateXML.
>
> Signed-off-by: Daniel Henrique Barboza
> ---
> src/qemu/qemu_domain.c | 4
> src/qemu/qemu_process.c | 14 +-
> 2 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 37926850b2..3b86d28216 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def,
> }
> }
>
> +if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) {
> +goto cleanup;
> +}
> +
make check would have told you to remove the { ... } for the one line
goto cleanup;
I'll take care of it for you (as well as the merge conflict it creates
in the subsequent patch).
John
> if (ARCH_IS_X86(def->os.arch) &&
> virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) {
> if (!qemuDomainIsQ35(def)) {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 4d134bd7be..2adbf375ee 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm,
> static int
> qemuProcessStartValidateXML(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> -virQEMUCapsPtr qemuCaps,
> virCapsPtr caps,
> unsigned int flags)
> {
> -/* The bits we validate here are XML configs that we previously
> - * accepted. We reject them at VM startup time rather than parse
> - * time so that pre-existing VMs aren't rejected and dropped from
> - * the VM list when libvirt is updated.
> - *
> - * If back compat isn't a concern, XML validation should probably
> - * be done at parse time.
> - */
> -if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0)
> -return -1;
> -
> /* checks below should not be executed when starting a qemu process for a
> * VM that was running before (migration, snapshots, save). It's more
> * important to start such VM than keep the configuration clean */
> @@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
>
> }
>
> -if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0)
> +if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0)
> return -1;
>
> if (qemuProcessStartValidateGraphics(vm) < 0)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/18 4:55 AM, Michal Privoznik wrote:
> On 11/05/2018 01:58 PM, John Ferlan wrote:
>> Create a new API that will allow an adjustment of IOThread
>> polling parameters for the specified IOThread. These parameters
>> will not be saved in the guest XML. Currently the only parameters
>> supported will allow the hypervisor to adjust the parameters used
>> to limit and alter the scope of the polling interval. The polling
>> interval allows the IOThread to spend more or less time processing
>> in the guest.
>>
>> Based on code originally posted by Pavel Hrdina
>> to add virDomainAddIOThreadParams and virDomainModIOThreadParams.
>> Modification of those changes to use virDomainSetIOThreadParams
>> instead and remove concepts related to saving the data in guest
>> XML as well as the way to specifically enable the polling parameters.
>>
>> Signed-off-by: John Ferlan
>> ACKed-by: Michal Privoznik
>> ---
>> include/libvirt/libvirt-domain.h | 44
>> src/driver-hypervisor.h | 8
>> src/libvirt-domain.c | 70
>> src/libvirt_public.syms | 5 +++
>> src/remote/remote_driver.c | 1 +
>> src/remote/remote_protocol.x | 21 +-
>> src/remote_protocol-structs | 10 +
>> 7 files changed, 158 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h
>> b/include/libvirt/libvirt-domain.h
>> index 58fd4bc10c..bf89d0149f 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -1911,6 +1911,50 @@ int
>> virDomainDelIOThread(virDomainPtr domain,
>>unsigned int iothread_id,
>>unsigned int flags);
>>
>> +/* IOThread set parameters */
>> +
>> +/**
>> + * VIR_DOMAIN_IOTHREAD_POLL_MAX_NS:
>> + *
>> + * The maximum polling time that can be used by polling algorithm in ns.
>> + * The polling time starts at 0 (zero) and is the time spent by the guest
>> + * to process IOThread data before returning the CPU to the host. The
>> + * polling time will be dynamically modified over time based on the
>> + * poll_grow and poll_shrink parameters provided. A value set too large
>> + * will cause more CPU time to be allocated the guest. A value set too
>> + * small will not provide enough cycles for the guest to process data.
>> + * The polling interval is not available for statistical purposes.
>> + */
>> +# define VIR_DOMAIN_IOTHREAD_POLL_MAX_NS "poll_max_ns"
>> +
>> +/**
>> + * VIR_DOMAIN_IOTHREAD_POLL_GROW:
>> + *
>> + * This provides a value for the dynamic polling adjustment algorithm to
>> + * use to grow its polling interval up to the poll_max_ns value. A value
>> + * of 0 (zero) allows the hypervisor to choose its own value. The algorithm
>> + * to use for adjustment is hypervisor specific.
>> + */
>> +# define VIR_DOMAIN_IOTHREAD_POLL_GROW "poll_grow"
>> +
>> +/**
>> + * VIR_DOMAIN_IOTHREAD_POLL_SHRINK:
>> + *
>> + * This provides a value for the dynamic polling adjustment algorithm to
>> + * use to shrink its polling interval when the polling interval exceeds
>> + * the poll_max_ns value. A value of 0 (zero) allows the hypervisor to
>> + * choose its own value. The algorithm to use for adjustment is hypervisor
>> + * specific.
>> + */
>> +# define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
>> +
>> +int virDomainSetIOThreadParams(virDomainPtr domain,
>
> On a completely unrelated note, this is stupid. I mean the amount of
> spaces after 'int'. I wonder if a patch that reformats all the header
> files would be accepted.
I agree with the spacing... I assumed it had something to do with the
docs pages generation; however, a quick test shows that by removing the
extraneous spaces doesn't change the format that I can see in the docs.
Furthermore there are other API's which don't do any spacing.
So perhaps a nice task for a first time contributor to remove the spaces
*and* make sure that it doesn't affect the webpage.
Tks -
John
Since it'll conflict - I'll wait for you to push the 'memfd' stuff first
w/ capabilities changes before pushing this... but don't wait too long ;-)
>
> The ACK still holds.
>
>> +unsigned int iothread_id,
>> +virTypedParameterPtr params,
>> +int nparams,
>> +unsigned int flags);
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/18 4:55 AM, Michal Privoznik wrote:
> On 11/05/2018 01:58 PM, John Ferlan wrote:
>> Separate out the fetch of the IOThread monitor call into a separate
>> helper so that a subsequent domain statistics change can fetch the raw
>> IOThread data and parse it as it sees fit.
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/qemu/qemu_driver.c | 48 ++
>> 1 file changed, 30 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a52e2495d5..e13633c1e0 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5486,39 +5486,52 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
>> VIR_DOMAIN_VCPU_MAXIMUM));
>> }
>>
>> +
>> static int
>> -qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
>> - virDomainObjPtr vm,
>> - virDomainIOThreadInfoPtr **info)
>> +qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + qemuMonitorIOThreadInfoPtr **iothreads)
>> {
>> qemuDomainObjPrivatePtr priv;
>> -qemuMonitorIOThreadInfoPtr *iothreads = NULL;
>> -virDomainIOThreadInfoPtr *info_ret = NULL;
>> int niothreads = 0;
>> -size_t i;
>> -int ret = -1;
>> -
>> -if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
>> -goto cleanup;
>>
>> if (!virDomainObjIsActive(vm)) {
>> virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> _("cannot list IOThreads for an inactive domain"));
>
> I wonder if this check should go into qemuDomainGetIOThreadsLive() right
> after BeginJob(). Rationale behind is that in the next patch, the
> qemuDomainGetStatsIOThread() does the same check already and then it
> calls this function which does the check again. Not crucial though.
>
Good point - I'll move it (and change return -1 to goto endjob ;-))
Tks
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
I would say here that we're going to "convert" the rememberOwner
configuration knob into a security driver "lock" knob that will
effectively enable or disable the usage of metadata locking for the object.
When metadata locking is enabled that means the security commit
processing will be run in a fork similar to how namespaces use fork()'s
for processing. This is done to ensure libvirt can properly and
synchronously modify the metadata to store the original owner data.
Since fork()'s (e.g. virFork) have been seen as a performance bottleneck
being able to disable them allows the admin to choose whether the
performance 'hit' is worth the extra 'security' of being able to
remember the original owner of a lock.
[you could move some of the above back a commit too, but I think it may
get lost there... wordsmithing anything I didn't quite explain right is
fine too. the important part being that we're converting bool names and
that it's a performance tradeoff]
On 11/14/18 7:44 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_security.c| 73 -
> src/security/security_dac.c | 56 -
> src/security/security_driver.h | 3 +-
> src/security/security_manager.c | 9 +++-
> src/security/security_manager.h | 3 +-
> src/security/security_selinux.c | 54
> src/security/security_stack.c | 5 ++-
> 7 files changed, 140 insertions(+), 63 deletions(-)
>
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/11/18 10:19 PM, Wang Yechao wrote:
> The commit 89563efc0209b854d2b2e554423423d7602acdbd fix the
> monitor error when closing the QEMU monitor. The QEMU agent
> has a problem similar to QEMU monitor. So fix the QEMU agent
> with the same method.
>
> Signed-off-by: Wang Yechao
> ---
> Changes in v3:
> - change the commit messages
> ---
> src/qemu/qemu_agent.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
Strange somehow this patch got lost and marked as read in my pile of
patches to review so I have essentially forgotten about it until I was
looking back through my email list and wondered why I had an unanswered
entry unmarked... Oh well - in the future if you think your patch is
"forgotten" - after a week or so of inactivity, feel free to "ping" on
it...
Reviewed-by: John Ferlan
(and pushed)
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/18 7:44 AM, Michal Privoznik wrote:
> Trying to use virlockd to lock metadata turns out to be too big
> gun. Since we will always spawn a separate process for relabeling
> we are safe to use thread unsafe POSIX locks and take out
> virtlockd completely out of the picture.
>
NB: This patch has a 1 second delay loop in the event the lock is
already taken by another thread. Since it's not expected that threads
holding the lock will need more than a second or two to process the
various file security operations, it was felt this was a good starting
point. The time taken to execute has two factors - the number of files
to be locked and the time the operation required by the security driver
to perform it's operation. Usage of virTimeBackOffStart may or may not
help. Similarly a configurable delay time or delay maximum time is also
possible, but was deemed unnecessary at this time.
[fair enough statement?? - pick and choose if you want to add all, any
part, or none of it... I think it's a fair representation for someone
coming along and looking at this commit to consider if they wanted to
make a change to help perceived performance if the delay loop becomes
that type of problem. It's not that it wasn't considered, it just wasn't
easily quantifiable].
> Signed-off-by: Michal Privoznik
> ---
> src/security/security_dac.c | 12 +-
> src/security/security_manager.c | 225 +---
> src/security/security_manager.h | 17 ++-
> src/security/security_selinux.c | 11 +-
> 4 files changed, 141 insertions(+), 124 deletions(-)
>
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/18 7:44 AM, Michal Privoznik wrote:
> For metadata locking we might need an extra fork() which given
> latest attempts to do fewer fork()-s is suboptimal. Therefore,
> there will be a qemu.conf knob to enable or this feature. But
or disable or just use "{en|dis}able" if you don't want to reformat all
the lines.
> since the feature is actually not metadata locking itself rather
> than remembering of the original owner of the file this is named
> as 'rememberOwner'. But patches for that feature are not even
> posted yet so there is actually no qemu.conf entry in this patch
> nor a way to enable this feature.
>
> Even though this is effectively a dead code for now it is still
> desired.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_conf.h| 1 +
> src/qemu/qemu_domain.c | 7 +++
> src/qemu/qemu_domain.h | 3 +++
> src/qemu/qemu_process.c | 3 +++
> 4 files changed, 14 insertions(+)
>
I suppose we could just create the @priv entry and leave @cfg for later,
but I'm fine with this anyway.
Thanks - I know it's a pain, but I'm sure it'd be even more painful to
get a review on a larger patch series which would actually set/use the
knob as you expect.
Reviewed-by: John Ferlan
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/18 7:44 AM, Michal Privoznik wrote:
> The TPM code currently accepts pointer to a domain definition.
> This is okay for now, but in near future the security driver APIs
> it calls will require domain object. Therefore, change the TPM
> code to accept the domain object pointer.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_extdevice.c | 16
> src/qemu/qemu_extdevice.h | 4 ++--
> src/qemu/qemu_process.c | 6 +++---
> src/qemu/qemu_security.c | 14 +++---
> src/qemu/qemu_security.h | 4 ++--
> src/qemu/qemu_tpm.c | 24
> src/qemu/qemu_tpm.h | 4 ++--
> 7 files changed, 36 insertions(+), 36 deletions(-)
>
Reviewed-by: John Ferlan
John
At one time I really hoped we could make the obj's transparent to
callers and force usage of virDomainObjGetDef, but I know that's a sheer
impossibility!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-11-14 at 21:38 -0200, Eduardo Habkost wrote:
> Many of the current virtio-*-pci device types actually represent
> 3 different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With this multi-purpose device
> type, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
> - virtio-*-pci-non-transitional: modern-only
> - Supports both Conventional PCI and PCI Express buses
So, my understanding was that transitional devices would be suitable
for both PCI and PCIe slots and non-transitional devices could only
work in PCIe slots, but based on the above it looks like I got it
pretty much completely wrong? I'm not too surprised that would be
the case, to be honest: keeping this stuff straight in my head has
always been a bit of a challenge, so I can't possibly not welcome a
proposal like this, which will spell it out a bit more :)
Let me try to map the interactions out:
* virtio-*-pci-transitional
+ plugged into PCI slot
- shows up as vendor1/device1
+ plugged into PCIe slot
- doesn't work
* virtio-*-pci-non-transitional
+ plugged into PCI slot
- shows up as vendor2/device2
+ plugged into PCIe slot
- shows up as vendor2/device2
* virtio-*-pci
+ plugged into PCI slot
- shows up as vendor1/device1
(same as virtio-*-pci-transitional)
+ plugged into PCIe slot
- shows up as vendor2/device2
(same as virtio-*-pci-non-transitional)
Does that look about right?
Once all the various pieces have fallen into place, when adding a
device to a guest running a modern OS we would find out through
libosinfo that it supports vendor2/device2 (and vendor1/device1
too, I guess?) so we would choose the non-transitional variant and
plug it into PCIe when possible (q35) or PCI otherwise (pc); on
the other hand, an older guest OS like CentOS 6 will only advertise
support for vendor1/device1, so we'd have to use the transitional
variant instead and plug it into a PCI slot regardless of the
machine type, which more specifically means building a
pcie.0 <- pcie-root-port <- pcie-pci-bridge topology for q35
guests.
If all of the above is correct, then it sounds like a feasible
enough plan to me, though of course it be a long time before users
and management applications can rely on these new device types
being available in downstream distributions...
One thing that I'm very much not convinced about is the naming,
specifically leaving the virtio revision out: I get it that we
Should Never Need™ another major version of the spec, but I'm
afraid discounting the possibility outright might prove to be
shortsighted and come back to bite us later, so I'd much rather
keep it.
And once that's done, "non-transitional" (while matching the
language of the spec) starts to look a bit unnecessary when you
could simply have
virtio-*-pci
virtio-*-pci-1
virtio-*-pci-1-transitional
instead. But I don't feel as strongly about this as I do about
keeping the virtio revision in the device name :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/11/18 10:59 PM, Han Han wrote:
> qemuDomainHubIsBusy is to check whether a usb device are attached to the hub
> device. It will be used for hotunplugging and live device update of hub
> device.
>
> Signed-off-by: Han Han
> ---
> src/qemu/qemu_hotplug.c | 64 +
> 1 file changed, 64 insertions(+)
>
This would need to be merged with the subsequent patch because the new
method is static, but it sure makes it easier to review when it's pulled
out.
Beyond that I find I see there is a virDomainUSBDeviceDefForeach which
perhaps could have be used. This code did miss the nserials
The whole assign/remove address space isn't something I know really
well, but in reading the code I note:
1. When a new hub is created, virDomainUSBAddressHubNew will create a
"hub->portmap" to manage the active ports for the hub. This will be
inserted into the priv->usbaddrs address set (something important to
understand later).
2. When a device is assigned to a hub, virDomainUSBAddressSetAddHub is
called which calls virDomainUSBAddressFindPort to find the targetHub for
which a virBitmapSetBit on the portmap is done for the port being used.
3. When a device using a hub is removed, virDomainUSBAddressRelease is
called by either qemuDomainReleaseDeviceAddress or various disk hot
unplug helpers. This in turn calls virDomainUSBAddressFindPort to return
the targetHub which is being unplugged. Then a virBitmapClearBit on done
for the port being used.
4. Nothing seems to care that an address set entry has an empty bitmap.
That is, if the last port is cleared on the hub, there's no automatic
removal, although there is automatic add when space is full. I think
that's an important distinction.
5. The only time virDomainUSBAddressSetFree is called to free
priv->usbaddrs is when a domain is stopped or the domain object is
disposed of (call to privateDataFreeFunc).
So what does this all mean, well if this device were removed it would
likewise call qemuDomainReleaseDeviceAddress in order to "clear" the
targetPort bit on the targetHub that this device was using if by chance
it was plugged into another hub (daisy chaining).
Still, shouldn't checking whether the device about to be deleted hub has
anything attached to be as simple as checking that it's own portmap is
clear? If it is clear, then removing it wouldn't remove any other
devices in collateral damage.
However, there's another gotcha. Recall that when a hub is added the
priv->usbaddrs is referenced, VIR_EXPAND_N called, and the hub placed
into the address set.
If you free something in the address set, then the address set is
potentially pointing at memory that it no longer owns. Once a add a
device to a hub is called, it's going to attempt to reference that
address and "may" or "may not" succeed. It may look like it succeeds,
but corruption is sure to follow. And from experience, I can tell you
that type of corruption is the absolute most difficult thing to find.
So you will need to add code to handle shrinking the priv->usbaddrs
since you're essentially about to free something in it. You should be
able to use logic from patch7 to become a callback/iter function for
virDomainUSBDeviceDefForeach (and it does matter if a hub is plugged
into the hub you'd be attempting to remove). Study the existing
consumers of virDomainUSBDeviceDefForeach - especially how they add
hubdef's and hubaddr's. Then consider how would you safely remove. The
logic seems to be intertwined with controllers too.
John
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1b6cc36bc8..ca73456260 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5332,6 +5332,70 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr
> vm,
> }
> }
>
> +static bool qemuDomainHubIsBusy(virDomainObjPtr vm,
> +virDomainHubDefPtr detach)
> +{
> +size_t i;
> +virDomainDiskDefPtr disk;
> +virDomainHubDefPtr hub;
> +virDomainInputDefPtr input;
> +virDomainSoundDefPtr sound;
> +virDomainHostdevDefPtr hostdev;
> +virDomainRedirdevDefPtr redirdev;
> +virDomainControllerDefPtr controller;
> +
> +for (i = 0; i < vm->def->ndisks; i++) {
> +disk = vm->def->disks[i];
> +if (disk->bus == VIR_DOMAIN_DISK_BUS_USB &&
> +virDomainUSBAddressIsAttachedToHub(&(disk->info), detach))
> +return true;
> +}
> +
> +for (i = 0; i < vm->def->nhubs; i++) {
> +hub = vm->def->hubs[i];
> +if (virDomainUSBAddressIsAttachedToHub(&(hub->info), detach))
> +return true;
> +}
> +
> +for (i = 0; i < vm->def->ninputs; i++) {
> +input = vm->def->inputs[i];
> +if (input->bus == VIR_DOMAIN_INPUT_BUS_USB &&
> +virDomainUSBAddressIsAttachedToHub(&(input->info), detach))
> +return true;
> +}
> +
> +for (i = 0; i < vm->def->nsounds; i++) {
> +sound = vm->def->sounds[i]
On 11/11/18 10:59 PM, Han Han wrote:
> Signed-off-by: Han Han
> ---
> docs/news.xml | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/docs/news.xml b/docs/news.xml
> index 88774c55ae..b677f52efc 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -35,6 +35,16 @@
>
>
>
> +
> +
> + Qemu: Add active and inactive device add or remove for QEMU
> + USB Hub devices
Running check fails here because only expects 1 line.
John
> +
> +
> + Add the ability to attach or detach a USB Hub device either
> + for active or inactive guests.
> +
> +
>
>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
在 2018/11/15 下午8:12, Andrea Bolognani 写道:
On Thu, 2018-11-15 at 10:17 +, Daniel P. Berrangé wrote:
On Tue, Nov 13, 2018 at 03:35:43PM +0100, Andrea Bolognani wrote:
Dan, do you have any remaining concerns about the XML syntax, or can
I go ahead and push?
Honestly, I still don't much like it & would prefer zpci as a top level
address type, but the consensus is in favour of this patch series'
approach, so don't consider me a blocker. Feel free to push if you
think it is ready.
Okay, pushed now.
Thank you very much!
--
Yi Min
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 15 Nov 2018 12:21:55 +0100
Cornelia Huck wrote:
> After a quick look, this seems fine; have not actually tried to run it
> yet.
Played a bit with it (with zpci devices for a s390x machine), seems to
work as expected.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/2018 12:55 PM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Hi,
>
> This is an alternative series from "[PATCH 0/5] Use memfd if
> possible". Instead of automatically using memfd for anonymous memory
> when available (as suggested by Daniel), it introduces the "memfd"
> memory backing type.
>
> Although using memfd transparently when possible is a good idea, it is
> a source of various complications for migration & save/restore. This
> could eventually be challenged in a different series.
>
> *please*:
> The first two patches have been modified and reviewed by John
> Ferlan. Hopefully they can be merged early, regardless of the last
> patch outcome, to avoid the painful rebase conflicts due to
> capabilities checks introduction.
>
> Thanks :)
>
> v3:
> - rebased, to fix capabilities check and ping the series
>
> Marc-André Lureau (3):
> qemu: add memory-backend-memfd capability check
> qemu: check memory-backend-memfd.hugetlb capability
> qemu: add memfd source type
>
> docs/formatdomain.html.in | 9 +-
> docs/schemas/domaincommon.rng | 1 +
> src/conf/domain_conf.c| 3 +-
> src/conf/domain_conf.h| 1 +
> src/qemu/qemu_capabilities.c | 10 ++
> src/qemu/qemu_capabilities.h | 2 +
> src/qemu/qemu_command.c | 69 +++
> src/qemu/qemu_domain.c| 12 +-
> .../caps_2.12.0.aarch64.replies | 94 ---
> .../caps_2.12.0.aarch64.xml | 4 +-
> .../caps_2.12.0.ppc64.replies | 90 +++---
> .../caps_2.12.0.ppc64.xml | 4 +-
> .../caps_2.12.0.s390x.replies | 98
> .../caps_2.12.0.s390x.xml | 4 +-
> .../caps_2.12.0.x86_64.replies| 110 +-
> .../caps_2.12.0.x86_64.xml| 4 +-
> .../caps_3.0.0.ppc64.replies | 90 +++---
> .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 4 +-
> .../caps_3.0.0.riscv32.replies| 86 +++---
> .../caps_3.0.0.riscv32.xml| 2 +
> .../caps_3.0.0.riscv64.replies| 86 +++---
> .../caps_3.0.0.riscv64.xml| 2 +
> .../caps_3.0.0.s390x.replies | 98
> .../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 4 +-
> .../caps_3.0.0.x86_64.replies | 110 +-
> .../caps_3.0.0.x86_64.xml | 4 +-
> .../memfd-memory-numa.x86_64-latest.args | 34 ++
> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++
> tests/qemuxml2argvtest.c | 2 +
> 29 files changed, 869 insertions(+), 204 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml
>
I've did the review and all three patches look good to me. I've fixed
all the issues I've found and I'm keeping them in a local branch of mine
to give others some time to raise their concerns should they have some.
My main concern was that we would be exposing memory backend to users by
giving them a config knob that could be set to enforce memfd usage. But
I don't think there is any other way, esp. since memory backends are not
real backends (one can't migrate a domain that was started with one to
another one).
ACK series
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/2018 12:55 PM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Add a new memoryBacking source type "memfd", supported by QEMU (when
> the capability is available).
>
> A memfd is a specialized anonymous memory kind. As such, an anonymous
> source type could be automatically using a memfd. However, there are
> some complications when migrating from different memory backends in
> qemu (mainly due to the internal object naming at this point, but
> there could be more). For now, it is simpler and safer to simply
> introduce a new source type "memfd". Eventually, the "anonymous" type
> could learn to use memfd transparently in a separate change.
>
> The main benefits are that it doesn't need to create filesystem files,
> and it also enforces sealing, providing a bit more safety.
>
> Signed-off-by: Marc-André Lureau
> ---
> docs/formatdomain.html.in | 9 +--
> docs/schemas/domaincommon.rng | 1 +
> src/conf/domain_conf.c| 3 +-
> src/conf/domain_conf.h| 1 +
> src/qemu/qemu_command.c | 69 +--
> src/qemu/qemu_domain.c| 12 +++-
> .../memfd-memory-numa.x86_64-latest.args | 34 +
> tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++
> tests/qemuxml2argvtest.c | 2 +
> 9 files changed, 140 insertions(+), 27 deletions(-)
> create mode 100644
> tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args
> create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 295f9ff93e..e7f4ad4060 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1126,7 +1126,7 @@
>
>
>
> -
> +
>
>
>
> @@ -1177,9 +1177,10 @@
> suitable for the specific environment at the same time to mitigate
> the risks described above. Since
> 1.0.6
> source
> - Using the type attribute, it's possible to provide
> - "file" to utilize file memorybacking or keep the default
> - "anonymous".
> + Using the type attribute, it's possible to
> + provide "file" to utilize file memorybacking or keep the
> + default "anonymous". Since 4.10.0,
> + you may choose "memfd" backing. (QEMU/KVM only)
> access
> Using the mode attribute, specify if the memory is
> to be "shared" or "private". This can be overridden per numa node by
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index cad189513a..bfa76c4db3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -655,6 +655,7 @@
>
> file
> anonymous
> +memfd
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6c15781dc0..bc82dc3504 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -898,7 +898,8 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState,
> VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
> VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST,
>"none",
>"file",
> - "anonymous")
> + "anonymous",
> + "memfd")
Ah, we don't use TABs rather spaces.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/15/18 1:12 PM, Andrea Bolognani wrote:
On Thu, 2018-11-15 at 10:17 +, Daniel P. Berrangé wrote:
On Tue, Nov 13, 2018 at 03:35:43PM +0100, Andrea Bolognani wrote:
Dan, do you have any remaining concerns about the XML syntax, or can
I go ahead and push?
Honestly, I still don't much like it & would prefer zpci as a top level
address type, but the consensus is in favour of this patch series'
approach, so don't consider me a blocker. Feel free to push if you
think it is ready.
Okay, pushed now.
Thank you very much!
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2018-11-15 at 10:17 +, Daniel P. Berrangé wrote:
> On Tue, Nov 13, 2018 at 03:35:43PM +0100, Andrea Bolognani wrote:
> > Dan, do you have any remaining concerns about the XML syntax, or can
> > I go ahead and push?
>
> Honestly, I still don't much like it & would prefer zpci as a top level
> address type, but the consensus is in favour of this patch series'
> approach, so don't consider me a blocker. Feel free to push if you
> think it is ready.
Okay, pushed now.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Thursday, November 15, 2018 12:19 AM
> To: Wang, Huaqiang ; libvir-list@redhat.com
> Cc: Feng, Shaohe ; Ding, Jian-feng feng.d...@intel.com>; Zang, Rui
> Subject: Re: [PATCHv8 2/2] tools: Add help docs explaining 'domstats'
> cache monitor result
>
>
>
> On 11/13/18 10:25 PM, Wang Huaqiang wrote:
> > Add help document in explaining the cache monitor related 'domstats'
> > result.
> >
> > This patch is written to address John's review comment regarding
> > patch16-v7 and expected to be merged with previous patch and using
> > that patch's committing message.
> >
> > Signed-off-by: Wang Huaqiang
> > ---
> > src/libvirt-domain.c | 21 -
> > tools/virsh.pod | 13 +
> > 2 files changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index
> > 4895f9f..67ff430 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -11345,15 +11345,18 @@
> virConnectGetDomainCapabilities(virConnectPtr conn,
> > * "cpu.user" - user cpu time spent in nanoseconds as unsigned long
> long.
> > * "cpu.system" - system cpu time spent in nanoseconds as unsigned
> long
> > *long.
> > - * "cpu.cache.monitor.count" - tocal cache monitoring groups
> > - * "cpu.cache.monitor.M.name" - name of cache monitoring group 'M'
> > - * "cpu.cache.monitor.M.vcpus" - vcpus for cache monitoring group
> 'M'
> > - * "cpu.cache.monitor.M.bank.count" - total bank number of cache
> monitoring
> > - *group 'M'
> > - * "cpu.cache.monitor.M.bank.N.id" - OS assigned cache bank id for
> cache
> > - *'N' in cache monitoring group 'M'
> > - * "cpu.cache.monitor.M.bank.N.bytes" - monitor's cache occupancy of
> cache
> > - *bank 'N' in cache monitoring group 'M'
> > + * "cpu.cache.monitor.count" - number of cache monitors on this
> domain
>
> for this domain
got.
>
> > + * "cpu.cache.monitor..name" - name of cache monitor
> > + * "cpu.cache.monitor..vcpus" - vcpu list of cache monitor
>
> > + * "cpu.cache.monitor..bank.count" - number of cache banks in
> cache
> > + *monitor
> > + * "cpu.cache.monitor..bank..id" - host allocatd cache id
> for
>
> allocated
>
My fault. Thanks.
> > + * bank in cache
> > + * monitor
> > + * "cpu.cache.monitor..bank..bytes" - the amount of
> last level
> > + *cache that domain is
> > + *using on cache bank
> >
> > + *(in Byte)
>
> the number of bytes of last level cache that the domain is using on cache
> bank
>
Thanks.
> (and likewise below although there was an extra type there)
>
> > *
> > * VIR_DOMAIN_STATS_BALLOON:
> > * Return memory balloon device information.
> > diff --git a/tools/virsh.pod b/tools/virsh.pod index 86c041d..49d9ab6
> > 100644
> > --- a/tools/virsh.pod
> > +++ b/tools/virsh.pod
> > @@ -1011,6 +1011,19 @@ I<--cpu-total> returns:
> > "cpu.time" - total cpu time spent for this domain in nanoseconds
> > "cpu.user" - user cpu time spent in nanoseconds
> > "cpu.system" - system cpu time spent in nanoseconds
> > + "cpu.cache.monitor.count" - number of cache monitors on this domain
> > + "cpu.cache.monitor..name" - name of cache monitor
> > + "cpu.cache.monitor..vcpus" - vcpu list of cache monitor
> > + "cpu.cache.monitor..bank.count" - number of cache banks in
> > +cache monitor
> > + "cpu.cache.monitor..bank..id" - host allocatd cache id
> > + for bank in
> > + caach monitor
>
> cache
My fault. Thanks.
>
>
> John
>
Thanks for review.
Huaqiang
>
> > + "cpu.cache.monitor..bank..bytes" - the amount of last
> > +level cache that
> > +domain is using on
> > +cache bank
> > +(in Byte)
> >
> > I<--balloon> returns:
> >
> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
From: Marc-André Lureau
Add a new memoryBacking source type "memfd", supported by QEMU (when
the capability is available).
A memfd is a specialized anonymous memory kind. As such, an anonymous
source type could be automatically using a memfd. However, there are
some complications when migrating from different memory backends in
qemu (mainly due to the internal object naming at this point, but
there could be more). For now, it is simpler and safer to simply
introduce a new source type "memfd". Eventually, the "anonymous" type
could learn to use memfd transparently in a separate change.
The main benefits are that it doesn't need to create filesystem files,
and it also enforces sealing, providing a bit more safety.
Signed-off-by: Marc-André Lureau
---
docs/formatdomain.html.in | 9 +--
docs/schemas/domaincommon.rng | 1 +
src/conf/domain_conf.c| 3 +-
src/conf/domain_conf.h| 1 +
src/qemu/qemu_command.c | 69 +--
src/qemu/qemu_domain.c| 12 +++-
.../memfd-memory-numa.x86_64-latest.args | 34 +
tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++
tests/qemuxml2argvtest.c | 2 +
9 files changed, 140 insertions(+), 27 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 295f9ff93e..e7f4ad4060 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1126,7 +1126,7 @@
-
+
@@ -1177,9 +1177,10 @@
suitable for the specific environment at the same time to mitigate
the risks described above. Since 1.0.6
source
- Using the type attribute, it's possible to provide
- "file" to utilize file memorybacking or keep the default
- "anonymous".
+ Using the type attribute, it's possible to
+ provide "file" to utilize file memorybacking or keep the
+ default "anonymous". Since 4.10.0,
+ you may choose "memfd" backing. (QEMU/KVM only)
access
Using the mode attribute, specify if the memory is
to be "shared" or "private". This can be overridden per numa node by
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cad189513a..bfa76c4db3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -655,6 +655,7 @@
file
anonymous
+memfd
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6c15781dc0..bc82dc3504 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -898,7 +898,8 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState,
VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST,
"none",
"file",
- "anonymous")
+ "anonymous",
+ "memfd")
VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
"none",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c167f8c43c..467785cd83 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -607,6 +607,7 @@ typedef enum {
VIR_DOMAIN_MEMORY_SOURCE_NONE = 0, /* No memory source defined */
VIR_DOMAIN_MEMORY_SOURCE_FILE, /* Memory source is set as file */
VIR_DOMAIN_MEMORY_SOURCE_ANONYMOUS, /* Memory source is set as anonymous */
+VIR_DOMAIN_MEMORY_SOURCE_MEMFD, /* Memory source is set as memfd */
VIR_DOMAIN_MEMORY_SOURCE_LAST,
} virDomainMemorySource;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d543ada2a1..5b13e3fd86 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3114,6 +3114,26 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
}
+static int
+qemuBuildMemoryBackendPropsShare(virJSONValuePtr props,
+ virDomainMemoryAccess memAccess)
+{
+switch (memAccess) {
+case VIR_DOMAIN_MEMORY_ACCESS_SHARED:
+return virJSONValueObjectAdd(props, "b:share", true, NULL);
+
+case VIR_DOMAIN_MEMORY_ACCESS_PRIVATE:
+return virJSONValueObjectAdd(props, "b:share", false, NULL);
+
+case VIR_DOMAIN_MEMORY_ACCESS_DEFAULT:
+case VIR_DOMAIN_MEMORY_ACCESS_LAST:
+break;
+}
+
+return 0;
+}
+
+
/**
* qemuBuildMemoryBackendProps:
* @backendProps: [out] constructed object
@@ -3133,7 +3153,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
* configurat
From: Marc-André Lureau
Hi,
This is an alternative series from "[PATCH 0/5] Use memfd if
possible". Instead of automatically using memfd for anonymous memory
when available (as suggested by Daniel), it introduces the "memfd"
memory backing type.
Although using memfd transparently when possible is a good idea, it is
a source of various complications for migration & save/restore. This
could eventually be challenged in a different series.
*please*:
The first two patches have been modified and reviewed by John
Ferlan. Hopefully they can be merged early, regardless of the last
patch outcome, to avoid the painful rebase conflicts due to
capabilities checks introduction.
Thanks :)
v3:
- rebased, to fix capabilities check and ping the series
Marc-André Lureau (3):
qemu: add memory-backend-memfd capability check
qemu: check memory-backend-memfd.hugetlb capability
qemu: add memfd source type
docs/formatdomain.html.in | 9 +-
docs/schemas/domaincommon.rng | 1 +
src/conf/domain_conf.c| 3 +-
src/conf/domain_conf.h| 1 +
src/qemu/qemu_capabilities.c | 10 ++
src/qemu/qemu_capabilities.h | 2 +
src/qemu/qemu_command.c | 69 +++
src/qemu/qemu_domain.c| 12 +-
.../caps_2.12.0.aarch64.replies | 94 ---
.../caps_2.12.0.aarch64.xml | 4 +-
.../caps_2.12.0.ppc64.replies | 90 +++---
.../caps_2.12.0.ppc64.xml | 4 +-
.../caps_2.12.0.s390x.replies | 98
.../caps_2.12.0.s390x.xml | 4 +-
.../caps_2.12.0.x86_64.replies| 110 +-
.../caps_2.12.0.x86_64.xml| 4 +-
.../caps_3.0.0.ppc64.replies | 90 +++---
.../qemucapabilitiesdata/caps_3.0.0.ppc64.xml | 4 +-
.../caps_3.0.0.riscv32.replies| 86 +++---
.../caps_3.0.0.riscv32.xml| 2 +
.../caps_3.0.0.riscv64.replies| 86 +++---
.../caps_3.0.0.riscv64.xml| 2 +
.../caps_3.0.0.s390x.replies | 98
.../qemucapabilitiesdata/caps_3.0.0.s390x.xml | 4 +-
.../caps_3.0.0.x86_64.replies | 110 +-
.../caps_3.0.0.x86_64.xml | 4 +-
.../memfd-memory-numa.x86_64-latest.args | 34 ++
tests/qemuxml2argvdata/memfd-memory-numa.xml | 36 ++
tests/qemuxml2argvtest.c | 2 +
29 files changed, 869 insertions(+), 204 deletions(-)
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/memfd-memory-numa.xml
--
2.19.1.708.g4ede3d42df
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
> libvirt wrongly assumes that VF netdev has to have the
> netdev assigned to PF. There is no such requirement in SRIOV standard.
> This patch change the virNetDevSwitchdevFeature() function to deal
> with SRIOV devices which does not have netdev on PF. Also removes
> one comment about PF netdev assumption.
>
> One example of such devices is ThunderX VNIC.
> By applying this change, VF device is used for virNetlinkCommand() as
> it is the only netdev assigned to VNIC.
>
> Signed-off-by: Radoslaw Biernacki
> ---
> src/util/virnetdev.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 5867977df4..e55c538a29 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char
> **pfname)
> }
>
> if (!*pfname) {
> -/* this shouldn't be possible. A VF can't exist unless its
> - * PF device is bound to a network driver
> - */
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("The PF device for VF %s has no network device
> name"),
> ifname);
> @@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
> if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
> return ret;
>
> -if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
> -goto cleanup;
> +if (is_vf == 1) {
> +/* ignore error if PF does noto have netdev assigned
> + * in that case pfname == NULL */
> +ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));
Problem is that virNetDevGetPhysicalFunction() reports error on failure.
So either you need to take that out and put it into the other place that
calls the function (virNetDevReadNetConfig) or call virResetLastError().
> +}
>
> pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
>virNetDevGetPCIDevice(ifname);
>
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/10/2018 01:56 PM, Radoslaw Biernacki wrote:
> linkdev is In/Out function parameter as second order reference pointer
> so requires first order dereference for checking NULLs which can be a
> result from virPCIGetNetName()
>
> Fixes: d6ee56d7237 (util: change virPCIGetNetName() to not return error if
> device has no net name)
> Signed-off-by: Radoslaw Biernacki
> ---
> src/util/virhostdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 14 Nov 2018 21:38:31 -0200
Eduardo Habkost wrote:
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..1d2a11504f 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
(...)
> +/**
> + * VirtioPCIDeviceTypeInfo:
> + *
> + * Template for virtio PCI device types. See virtio_pci_types_register()
> + * for details.
> + */
> +typedef struct VirtioPCIDeviceTypeInfo {
> +/* Prefix for the class names */
> +const char *name_prefix;
Maybe call this the vpci_name instead? It's not really a
prefix in the way I would usually use the term, but rather a type name
with a possible suffix tacked onto it.
> +/* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
> +const char *parent;
> +/* If modern_only is true, only type will be registered */
> +bool modern_only;
> +
> +/* Same as TypeInfo fields: */
> +size_t instance_size;
> +void (*instance_init)(Object *obj);
> +void (*class_init)(ObjectClass *klass, void *data);
> +} VirtioPCIDeviceTypeInfo;
> +
> +/*
> + * Register virtio-pci types. Each virtio-pci device type will be available
> + * in 3 flavors:
> + * - -transitional: modern device supporting legacy drivers
> + * - Supports Conventional PCI buses only
> + * - -non-transitional: modern-only
> + * - Supports Conventional PCI and PCI Express buses
> + * - : virtio version configurable, legacy driver support
> + * automatically selected when device is plugged
> + * _ This was the only flavor supported until QEMU 3.1
s/_/-/
s/until/up to/ ?
> + * - Supports Conventional PCI and PCI Express buses
> + *
> + * All the types above will inherit from "-base", so generic
> + * code can use "-base" on type casts.
> + *
> + * We need a separate "-base" type instead of making all types
> inherit
> + * from for two reasons:
> + * 1) will implement INTERFACE_PCIE_DEVICE, but
> + *-transitional won't.
> + * 2) will have the "disable-legacy" and "disable-modern"
> properties,
> + *-transitional and -non-transitional won't.
I'd formulate this rather as "x implements/has something, y
and z do not", as the code actually does that (and does not just intend
to do so :)
> + */
> +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
> +
> #endif
(...)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index a954799267..0fa248d68c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
(...)
> +static void virtio_pci_1_0_instance_init(Object *obj)
Ditch the _0? I don't expect this to change the name when virtio 1.1
arrives.
> +{
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> +
> +proxy->disable_legacy = ON_OFF_AUTO_ON;
> +proxy->disable_modern = false;
> +}
(...)
After a quick look, this seems fine; have not actually tried to run it
yet.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 15 Nov 2018 10:05:59 +
Daniel P. Berrangé wrote:
> On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
> > Many of the current virtio-*-pci device types actually represent
> > 3 different types of devices:
> > * virtio 1.0 non-transitional devices
> > * virtio 1.0 transitional devices
> > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> >
> > That would be just an annoyance if it didn't break our device/bus
> > compatibility QMP interfaces. With this multi-purpose device
> > type, there's no way to tell management software that
> > transitional devices and legacy devices require a Conventional
> > PCI bus.
> >
> > The multi-purpose device types would also prevent us from telling
> > management software what's the PCI vendor/device ID for them,
> > because their PCI IDs change at runtime depending on the bus
> > where they were plugged.
> >
> > This patch adds separate device types for each of those virtio
> > device flavors:
> >
> > - virtio-*-pci: the existing multi-purpose device types
> > - Configurable using `disable-legacy` and `disable-modern`
> > properties
> > - Legacy driver support is automatically enabled/disabled
> > depending on the bus where it is plugged
> > - Supports Conventional PCI and PCI Express buses
> > (but Conventional PCI is incompatible with
> > disable-legacy=off)
> > - Changes PCI vendor/device IDs at runtime
> > - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology
better.
> > - Supports Conventional PCI buses only, because
> > it has a PIO BAR
>
> Am I right in thinking that this is basically identical
> to virtio-*-pci, aside from only being valid for PCI
> buses ?
>
> IOW, libvirt can expose this device even if QEMU does
> not support it, by simply using the existing device
> type and only ever placing it in a PCI bus ?
>
> If libvirt did this compatibility approach, can you
> confirm this would be live migration state compatible.
>
> ie can live migrate virtio-*-pci -> virtio-*-pci-transitional,
> provided only PCI bus was used.
It also needs to make sure that neither disable-legacy nor
disable-modern is set. Then this would have a compatible state AFAICS.
>
> > - virtio-*-pci-non-transitional: modern-only
> > - Supports both Conventional PCI and PCI Express buses
>
> IIUC, libvirt can again provide compatibility with old
> QEMU by simply using the existing device type and setting
> disable-legacy ? Can you confirm this would be live
> migration compatible
>
> virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
I think yes.
[Out of curiosity, libvirt does not do anything with virtio-ccw's max
revision attribute, does it? QEMU uses this on a machine-type level for
compat handling, but I don't think it is useful beyond that.
Fortunately, virtio-ccw does not have complications like the
PCI/PCI-Express bus dependency.]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 15, 2018 at 11:50:56AM +0100, Cornelia Huck wrote:
> On Thu, 15 Nov 2018 10:05:59 +
> Daniel P. Berrangé wrote:
>
> > On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
> > > Many of the current virtio-*-pci device types actually represent
> > > 3 different types of devices:
> > > * virtio 1.0 non-transitional devices
> > > * virtio 1.0 transitional devices
> > > * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
> > >
> > > That would be just an annoyance if it didn't break our device/bus
> > > compatibility QMP interfaces. With this multi-purpose device
> > > type, there's no way to tell management software that
> > > transitional devices and legacy devices require a Conventional
> > > PCI bus.
> > >
> > > The multi-purpose device types would also prevent us from telling
> > > management software what's the PCI vendor/device ID for them,
> > > because their PCI IDs change at runtime depending on the bus
> > > where they were plugged.
> > >
> > > This patch adds separate device types for each of those virtio
> > > device flavors:
> > >
> > > - virtio-*-pci: the existing multi-purpose device types
> > > - Configurable using `disable-legacy` and `disable-modern`
> > > properties
> > > - Legacy driver support is automatically enabled/disabled
> > > depending on the bus where it is plugged
> > > - Supports Conventional PCI and PCI Express buses
> > > (but Conventional PCI is incompatible with
> > > disable-legacy=off)
> > > - Changes PCI vendor/device IDs at runtime
> > > - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
>
> It's a virtio-1 (not 1.0) device. Otherwise, I like this terminology
> better.
>
> > > - Supports Conventional PCI buses only, because
> > > it has a PIO BAR
> >
> > Am I right in thinking that this is basically identical
> > to virtio-*-pci, aside from only being valid for PCI
> > buses ?
> >
> > IOW, libvirt can expose this device even if QEMU does
> > not support it, by simply using the existing device
> > type and only ever placing it in a PCI bus ?
> >
> > If libvirt did this compatibility approach, can you
> > confirm this would be live migration state compatible.
> >
> > ie can live migrate virtio-*-pci -> virtio-*-pci-transitional,
> > provided only PCI bus was used.
>
> It also needs to make sure that neither disable-legacy nor
> disable-modern is set. Then this would have a compatible state AFAICS.
That's ok, as libvirt doesn't expose disable-modern or
disable-legacy right now.
> > > - virtio-*-pci-non-transitional: modern-only
> > > - Supports both Conventional PCI and PCI Express buses
> >
> > IIUC, libvirt can again provide compatibility with old
> > QEMU by simply using the existing device type and setting
> > disable-legacy ? Can you confirm this would be live
> > migration compatible
> >
> > virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
>
> I think yes.
>
> [Out of curiosity, libvirt does not do anything with virtio-ccw's max
> revision attribute, does it? QEMU uses this on a machine-type level for
> compat handling, but I don't think it is useful beyond that.
> Fortunately, virtio-ccw does not have complications like the
> PCI/PCI-Express bus dependency.]
I don't believe we ever set max revision.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 05, 2018 at 04:41:09PM -0600, Eric Blake wrote:
> On 10/9/18 8:23 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange"
> >
> > Currently any client which can complete the TLS handshake is able to use
> > the NBD server. The server admin can turn on the 'verify-peer' option
> > for the x509 creds to require the client to provide a x509 certificate.
> > This means the client will have to acquire a certificate from the CA
> > before they are permitted to use the NBD server. This is still a fairly
> > low bar to cross.
> >
> > This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> > takes the ID of a previously added 'QAuthZ' object instance. This will
> > be used to validate the client's x509 distinguished name. Clients
> > failing the authorization check will not be permitted to use the NBD
> > server.
> >
> > For example to setup authorization that only allows connection from a client
> > whose x509 certificate distinguished name is
> >
> > CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> >
> > use:
> >
> >qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
> > endpoint=server,verify-peer=yes \
> > --object authz-simple,id=auth0,identity=CN=laptop.example.com,,\
> > O=Example Org,,L=London,,ST=London,,C=GB \
>
> Missing shell quoting around the space in 'Example Org'. It's also fairly
> obvious that actual shell commands can't have leading space between
> \-newline line continuations.
>
> > --tls-creds tls0 \
> > --tls-authz authz0
> >other qemu-nbd args...
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > include/block/nbd.h | 2 +-
> > nbd/server.c| 10 +-
> > qemu-nbd.c | 13 -
> > qemu-nbd.texi | 4
> > 4 files changed, 22 insertions(+), 7 deletions(-)
> >
>
> > +++ b/qemu-nbd.c
> > @@ -52,6 +52,7 @@
> > #define QEMU_NBD_OPT_TLSCREDS 261
> > #define QEMU_NBD_OPT_IMAGE_OPTS262
> > #define QEMU_NBD_OPT_FORK 263
> > +#define QEMU_NBD_OPT_TLSAUTHZ 264
>
> > @@ -532,6 +534,7 @@ int main(int argc, char **argv)
> > { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
> > { "trace", required_argument, NULL, 'T' },
> > { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
> > +{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
> > { NULL, 0, NULL, 0 }
> > };
>
> Missing a change to qemu-nbd --help to describe the new option.
Yes, and it should be 'required_argument' too, not 'no_argument'.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Nov 05, 2018 at 04:41:09PM -0600, Eric Blake wrote:
> On 10/9/18 8:23 AM, Daniel P. Berrangé wrote:
> > From: "Daniel P. Berrange"
> >
> > Currently any client which can complete the TLS handshake is able to use
> > the NBD server. The server admin can turn on the 'verify-peer' option
> > for the x509 creds to require the client to provide a x509 certificate.
> > This means the client will have to acquire a certificate from the CA
> > before they are permitted to use the NBD server. This is still a fairly
> > low bar to cross.
> >
> > This adds a '--tls-authz OBJECT-ID' option to the qemu-nbd command which
> > takes the ID of a previously added 'QAuthZ' object instance. This will
> > be used to validate the client's x509 distinguished name. Clients
> > failing the authorization check will not be permitted to use the NBD
> > server.
> >
> > For example to setup authorization that only allows connection from a client
> > whose x509 certificate distinguished name is
> >
> > CN=laptop.example.com,O=Example Org,L=London,ST=London,C=GB
> >
> > use:
> >
> >qemu-nbd --object tls-creds-x509,id=tls0,dir=/home/berrange/qemutls,\
> > endpoint=server,verify-peer=yes \
> > --object authz-simple,id=auth0,identity=CN=laptop.example.com,,\
> > O=Example Org,,L=London,,ST=London,,C=GB \
>
> Missing shell quoting around the space in 'Example Org'. It's also fairly
> obvious that actual shell commands can't have leading space between
> \-newline line continuations.
Yep, leading space is the tradeoff of sticking to sensible line length
while maintaining clarity.
>
> > --tls-creds tls0 \
> > --tls-authz authz0
> >other qemu-nbd args...
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > include/block/nbd.h | 2 +-
> > nbd/server.c| 10 +-
> > qemu-nbd.c | 13 -
> > qemu-nbd.texi | 4
> > 4 files changed, 22 insertions(+), 7 deletions(-)
> >
>
> > +++ b/qemu-nbd.c
> > @@ -52,6 +52,7 @@
> > #define QEMU_NBD_OPT_TLSCREDS 261
> > #define QEMU_NBD_OPT_IMAGE_OPTS262
> > #define QEMU_NBD_OPT_FORK 263
> > +#define QEMU_NBD_OPT_TLSAUTHZ 264
>
> > @@ -532,6 +534,7 @@ int main(int argc, char **argv)
> > { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
> > { "trace", required_argument, NULL, 'T' },
> > { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
> > +{ "tls-authz", no_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
> > { NULL, 0, NULL, 0 }
> > };
>
> Missing a change to qemu-nbd --help to describe the new option.
Opps, yes.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/14/2018 07:17 PM, Chris Venteicher wrote:
> Quoting Michal Privoznik (2018-11-14 09:45:06)
>> On 11/11/2018 08:59 PM, Chris Venteicher wrote:
>>> Make process code usable outside qemu_capabilities by moving code
>>> from qemu_capabilities to qemu_process and exposing public functions.
>>>
>>> The process code is used to activate a non domain QEMU process for QMP
>>> message exchanges.
>>>
>>> This patch set modifies capabilities to use the new public functions.
>>>
>>> --
>>>
>>> The process code is being decoupled from qemu_capabilities now to
>>> support hypervisor baseline and comparison using QMP commands.
>>>
>>> This patch set was originally submitted as part of the baseline patch set:
>>> [libvirt] [PATCH v4 00/37] BaselineHypervisorCPU using QEMU QMP exchanges
>>> https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
>>
>> Okay, so you want to implement cpu-baseline for s390. But that doesn't
>> really explain the code movement. Also, somehow the code movement makes
>> the code bigger? I guess what I am saying is that I don't see much
>> justification for these patches.
>>
>
> Here is the feedback from an earlier hypervisor baseline review that
> resulted in this patch set.
> https://www.redhat.com/archives/libvir-list/2018-July/msg00881.html
>
> I think Jiri correctly identified capabilities, and now baseline and
> comparison, are unrelated services that all independently need to start
> a non-domain QEMU process for QMP messaging.
>
> I am not sure, but it seems likely there could be other (S390...)
> commands in the future that use QMP messages outside of a domain context
> to get info or do work at the QEMU level.
>
> All the baseline code I had in qemu_capabilities didn't make sense there
> anymore once I moved the process code from qemu_capabilities to
> qemu_process.
>
> Here is the latest baseline patch set:
> https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
>
> In the latest baseline patch set, all the baseline code is in qemu_driver
> and uses the process functions exposed now from qemu_process.
>
> So as best I can tell there main choice is...
>
> 1) Leave process code in qemu_capabilities and make the 4 core
> process functions (new, start, stop, free) and data strut public
> so they can also be used by baseline and comparison from qemu_driver.
>
> 2) Move the process code from qemu_capabilities to qemu_process.
> (this patch set) and expose the functions / data struct from
> qemu_process.
Oh, my bad. I just skimmed through referenced v3 and did not read it
carefully. If I did I would learn that this feature you're adding is not
just like any other feature. Therefore code movement and unification
makes actually sense. So after all this is the right way to go. Sorry
for the noise. All in all, the patches look okayish. But we will have to
see v2 of them, I'm afraid.
>
> In case 1 functions have the virQemuCaps prefix.
> In case 2 functions have the qemuProcess prefix.
>
> In either approach there are some changes needed to the process code to
> decouple it from the capabilities code to support both capabilities and
> baseline.
>
> I did spend a few patches in this patch set breaking out the init,
> process launch and monitor connection code into different static
> functions in the style used elsewhere in qemu_process. That could be
> reversed if it doesn't add enough value if the decision is to move the
> process code to qemu_process.
>
>>
>>>
>>> The baseline and comparison requirements are described here:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1511999
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1511996
>>>
>>>
>>> I am extracting and resubmitting just the process changes as a stand
>>> alone series to try to make review easier.
>>>
>>> The patch set shows capabilities using the public functions.
>>> To see baseline using the public functions...
>>> Look at the "qemu_driver:" patches at the end of
>>> https://www.redhat.com/archives/libvir-list/2018-November/msg00091.html
>>>
>>> Also,
>>> The "qemu_driver: Support feature expansion via QEMU when baselining cpu"
>>> patch might be of particular interest because the same QEMU process is
>>> used for both baseline and expansion using QMP commands.
>>>
>>> --
>>>
>>> Many patches were used to isolate code moves and name changes from other
>>> actual implementation changes.
>>>
>>> The patches reuse the pattern of public qemuProcess{Start,Stop} functions
>>> and internal static qemuProcess{Init,Launch,ConnectMonitor} functions
>>> but adds a "Qmp" suffix to make them unique.
>>>
>>> A number of patches are about re-partitioning the code into static
>>> functions for initialization, process launch and connection monitor
>>> stuff. This matches the established pattern in qemu_process and seemed
>>> to make sense to do.
>>>
>>> For concurrency...
>>> A thread safe library function creates a unique directory under libDir for
>>> each QEMU
>>> process (for QMP messa
On Tue, Nov 13, 2018 at 03:35:43PM +0100, Andrea Bolognani wrote:
> On Thu, 2018-11-08 at 19:00 +0800, Yi Min Zhao wrote:
> > Abstract
> >
> > The PCI representation in QEMU has been extended for S390
> > allowing configuration of zPCI attributes like uid (user-defined
> > identifier) and fid (PCI function identifier).
> > The details can be found here:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
> >
> > To support the new zPCI feature of the S390 platform, a new element of
> > PCI address is introduced. It has two optional attributes, @uid and
> > @fid. For example:
> >
> >
> >
> >
> >
> > > function='0x0'>
> >
> >
> >
> >
> > If they are defined by the user, unique values within the guest domain
> > must be used. If they are not specified and the architecture requires
> > them, they are automatically generated with non-conflicting values.
> >
> > zPCI address as an extension of the PCI address are stored in a new
> > structure 'virZPCIDeviceAddress' which is a member of common PCI
> > Address structure. Additionally, two hashtables are used for assignment
> > and reservation of zPCI uid/fid.
> >
> > In support of extending the PCI address, a new PCI address extension flag is
> > introduced. This extension flag allows is not only dedicated for the S390
> > platform but also other architectures needing certain extensions to PCI
> > address space.
>
> I have now provided R-b for the only patch that was still missing it,
> and as far as I'm concerned the series is ready to be pushed.
>
> Dan, do you have any remaining concerns about the XML syntax, or can
> I go ahead and push?
Honestly, I still don't much like it & would prefer zpci as a top level
address type, but the consensus is in favour of this patch series'
approach, so don't consider me a blocker. Feel free to push if you
think it is ready.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 14, 2018 at 09:38:31PM -0200, Eduardo Habkost wrote:
> Many of the current virtio-*-pci device types actually represent
> 3 different types of devices:
> * virtio 1.0 non-transitional devices
> * virtio 1.0 transitional devices
> * virtio 0.9 ("legacy device" in virtio 1.0 terminology)
>
> That would be just an annoyance if it didn't break our device/bus
> compatibility QMP interfaces. With this multi-purpose device
> type, there's no way to tell management software that
> transitional devices and legacy devices require a Conventional
> PCI bus.
>
> The multi-purpose device types would also prevent us from telling
> management software what's the PCI vendor/device ID for them,
> because their PCI IDs change at runtime depending on the bus
> where they were plugged.
>
> This patch adds separate device types for each of those virtio
> device flavors:
>
> - virtio-*-pci: the existing multi-purpose device types
> - Configurable using `disable-legacy` and `disable-modern`
> properties
> - Legacy driver support is automatically enabled/disabled
> depending on the bus where it is plugged
> - Supports Conventional PCI and PCI Express buses
> (but Conventional PCI is incompatible with
> disable-legacy=off)
> - Changes PCI vendor/device IDs at runtime
> - virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
> - Supports Conventional PCI buses only, because
> it has a PIO BAR
Am I right in thinking that this is basically identical
to virtio-*-pci, aside from only being valid for PCI
buses ?
IOW, libvirt can expose this device even if QEMU does
not support it, by simply using the existing device
type and only ever placing it in a PCI bus ?
If libvirt did this compatibility approach, can you
confirm this would be live migration state compatible.
ie can live migrate virtio-*-pci -> virtio-*-pci-transitional,
provided only PCI bus was used.
> - virtio-*-pci-non-transitional: modern-only
> - Supports both Conventional PCI and PCI Express buses
IIUC, libvirt can again provide compatibility with old
QEMU by simply using the existing device type and setting
disable-legacy ? Can you confirm this would be live
migration compatible
virtio-*-pci + disable-legacy -> virtio-*pci-non-transitional
> Reference to previous discussion that originated this idea:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
>
> Changes v1 -> v2:
> * Removed *-0.9 devices. Nobody will want to use them, if
> transitional devices work with legacy drivers
> (Gerd Hoffmann, Michael S. Tsirkin)
> * Drop virtio version from name: rename -1.0-transitional to
> -transitional (Michael S. Tsirkin)
> * Renamed -1.0 to -non-transitional
> * Don't add any extra variants to modern-only device types
> (they don't need it)
> * Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
> Cornelia Huck)
> * No need to change cast macros for modern-only devices
> * Rename virtio_register_types() to virtio_pci_types_register()
>
> Note about points discussed in the v1 thread:
>
> Andrea suggested making separate transitional Conventional PCi
> and transitional PCI Express device types[1]. I didn't do that
> because I don't see which problems this solves. We have many
> other device types that are hybrid (support both PCI Express and
> Conventional PCI) and this was never a problem for us[2].
>
> [1]
> http://mid.mail-archive.com/6f8d59b8ee2d92d73d2957b520bd8bac989fc796.camel@redhat.com
> [2] http://mid.mail-archive.com/20181017155637.GC31060@habkost.net
> ---
> hw/virtio/virtio-pci.h | 80 +--
> hw/display/virtio-gpu-pci.c| 8 +-
> hw/display/virtio-vga.c| 8 +-
> hw/virtio/virtio-crypto-pci.c | 8 +-
> hw/virtio/virtio-pci.c | 215 -
> tests/acceptance/virtio_version.py | 177
> 6 files changed, 406 insertions(+), 90 deletions(-)
> create mode 100644 tests/acceptance/virtio_version.py
>
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..1d2a11504f 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -216,7 +216,8 @@ static inline void
> virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
> /*
> * virtio-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI_PREFIX "virtio-scsi-pci"
> +#define TYPE_VIRTIO_SCSI_PCI (TYPE_VIRTIO_SCSI_PCI_PREFIX "-base")
> #define VIRTIO_SCSI_PCI(obj) \
> OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
>
> @@ -229,7 +230,8 @@ struct VirtIOSCSIPCI {
> /*
> * vhost-scsi-pci: This extends VirtioPCIProxy.
> */
> -#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI_PREFIX "vhost-scsi-pci"
> +#define TYPE_VHOST_SCSI_PCI (TYPE_VHOST_SCSI_PCI_PREFIX "-base")
> #define VHOST_SCSI_PCI(obj) \
> OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHO
On 11/14/2018 05:45 PM, John Ferlan wrote:
> Add the support to work with libvirt commit 66a85cb13.
>
> Signed-off-by: John Ferlan
> ---
> Although it's a build breaker and this does fix the problem - I'll wait
> to push just to make sure the text used for the message passes muster.
>
> Changes| 2 +-
> lib/Sys/Virt.xs| 1 +
> lib/Sys/Virt/Domain.pm | 4
> 3 files changed, 6 insertions(+), 1 deletion(-)
ACK
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/05/2018 01:58 PM, John Ferlan wrote:
> Create a new API that will allow an adjustment of IOThread
> polling parameters for the specified IOThread. These parameters
> will not be saved in the guest XML. Currently the only parameters
> supported will allow the hypervisor to adjust the parameters used
> to limit and alter the scope of the polling interval. The polling
> interval allows the IOThread to spend more or less time processing
> in the guest.
>
> Based on code originally posted by Pavel Hrdina
> to add virDomainAddIOThreadParams and virDomainModIOThreadParams.
> Modification of those changes to use virDomainSetIOThreadParams
> instead and remove concepts related to saving the data in guest
> XML as well as the way to specifically enable the polling parameters.
>
> Signed-off-by: John Ferlan
> ACKed-by: Michal Privoznik
> ---
> include/libvirt/libvirt-domain.h | 44
> src/driver-hypervisor.h | 8
> src/libvirt-domain.c | 70
> src/libvirt_public.syms | 5 +++
> src/remote/remote_driver.c | 1 +
> src/remote/remote_protocol.x | 21 +-
> src/remote_protocol-structs | 10 +
> 7 files changed, 158 insertions(+), 1 deletion(-)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index 58fd4bc10c..bf89d0149f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1911,6 +1911,50 @@ int virDomainDelIOThread(virDomainPtr
> domain,
>unsigned int iothread_id,
>unsigned int flags);
>
> +/* IOThread set parameters */
> +
> +/**
> + * VIR_DOMAIN_IOTHREAD_POLL_MAX_NS:
> + *
> + * The maximum polling time that can be used by polling algorithm in ns.
> + * The polling time starts at 0 (zero) and is the time spent by the guest
> + * to process IOThread data before returning the CPU to the host. The
> + * polling time will be dynamically modified over time based on the
> + * poll_grow and poll_shrink parameters provided. A value set too large
> + * will cause more CPU time to be allocated the guest. A value set too
> + * small will not provide enough cycles for the guest to process data.
> + * The polling interval is not available for statistical purposes.
> + */
> +# define VIR_DOMAIN_IOTHREAD_POLL_MAX_NS "poll_max_ns"
> +
> +/**
> + * VIR_DOMAIN_IOTHREAD_POLL_GROW:
> + *
> + * This provides a value for the dynamic polling adjustment algorithm to
> + * use to grow its polling interval up to the poll_max_ns value. A value
> + * of 0 (zero) allows the hypervisor to choose its own value. The algorithm
> + * to use for adjustment is hypervisor specific.
> + */
> +# define VIR_DOMAIN_IOTHREAD_POLL_GROW "poll_grow"
> +
> +/**
> + * VIR_DOMAIN_IOTHREAD_POLL_SHRINK:
> + *
> + * This provides a value for the dynamic polling adjustment algorithm to
> + * use to shrink its polling interval when the polling interval exceeds
> + * the poll_max_ns value. A value of 0 (zero) allows the hypervisor to
> + * choose its own value. The algorithm to use for adjustment is hypervisor
> + * specific.
> + */
> +# define VIR_DOMAIN_IOTHREAD_POLL_SHRINK "poll_shrink"
> +
> +int virDomainSetIOThreadParams(virDomainPtr domain,
On a completely unrelated note, this is stupid. I mean the amount of
spaces after 'int'. I wonder if a patch that reformats all the header
files would be accepted.
The ACK still holds.
> +unsigned int iothread_id,
> +virTypedParameterPtr params,
> +int nparams,
> +unsigned int flags);
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/05/2018 01:58 PM, John Ferlan wrote:
> v2: https://www.redhat.com/archives/libvir-list/2018-October/msg01065.html
>
> NB: Minor mods for this are change using 4.10.0 instead of 4.9.0, merge of
> qemu_capabilities conflict, and updated news.xml
>
> .. v2 Cover Letter:
>
> v1: https://www.redhat.com/archives/libvir-list/2018-October/msg00456.html
>
> Changes since v1 (from code review)
>
> Patch 2: Move the job back into qemuDomainGetIOThreadsLive
>
> Patch 3: Add check for ActiveJob before allowing, use true for
> *StatsWorker, and print 'iothread' in output not 'block'
>
> Patch 5: Use virCheckPositiveArgGoto(nparams, error) instead of using
> virCheckNonNegativeArgGoto(nparams, error). And then remove
> the if (nparams) before the virCheckNonNullArgGoto(params, error);
>
> Patch 6: Add ability to determine which parameter was set via bool
> set_poll_{max_ns|grow|shrink} values. Then use those in
> the macro that sets the value to determine whether or not
> the value will be set based on whether it was changed.
>
> Patch 10: Use bool's to set_ when the value is found in the incoming
> params list. Remove the check that says poll_max_ns needs
> to be set. Testing proves that if it's set to 0, then the
> grow and shrink values can be changed (although they do
> nothing)
>
> Patch 12: (NEW) - News article
>
>
> .. v1 Cover Letter:
>
> This series attempts to resurrect the concept of being able to modify
> the IOThread polling parameters; however, in a slightly different
> manner than the previous series posted by posted by Pavel Hrdina
> :
>
> https://www.redhat.com/archives/libvir-list/2017-February/msg01047.html
>
> The work is prompted by continued pleas found in the bz:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1545732
>
> to provide some way to modify the paremters without needing to supply
> QEMU command line pass through values.
>
> It's accepted that the values being changed are fairly or extremely
> low level type knobs; however, it's also shown that by being able to
> turn the knob it is possible for certain, specific appliances to be
> able to gain a performance benefit for the thread at the expense of
> other competing threads.
>
> Unlike the previous series, this series does not attempt to save the
> polling values in the guest XML. Rather, it only modifies the live
> guest's IOThread with new values. It also doesn't provide the polling
> values in a virsh iothread* command, rather it uses the domstats
> in order to fetch and display the values. The theory being this
> leaves the onus on the higher level appliance/application to provide
> the "proper guidance" and "concerns" related to changing the values
> to the consumer. Not saving the values means whatever values that
> are chosen do not "live" in perpetuity. Once the guest is shut down
> or the IOThread removed from guest, the hypervisor default values
> take over again. Perhaps not a perfect situation in terms of what
> the bz requests; however, storage of default values that could
> cause performance issues is not an optimal situation. So this I
> figured is a "comprimise" of sorts.
>
> If it's still felt that no we don't want to do this, then fine,
> but please in doing so own the bz, state your case, and close it.
> I'm 50/50 on it, but figured at least I'd present this option and
> see what the concensus was.
>
>
> John Ferlan (12):
> qemu: Check for and return IOThread polling values if available
> qemu: Split qemuDomainGetIOThreadsLive
> qemu: Implement the ability to return IOThread stats
> virsh: Add ability to display IOThread stats
> lib: Introduce virDomainSetIOThreadParams
> qemu: Add monitor functions to set IOThread params
> qemu: Alter qemuDomainChgIOThread to take enum instead of bool
> qemu: Alter qemuDomainChgIOThread to take qemuMonitorIOThreadInfo
> qemu: Detect whether iothread polling is supported
> qemu: Introduce qemuDomainSetIOThreadParams
> tools: Add virsh iothreadset command
> docs: Add news article for IOThread polling
>
> docs/news.xml | 13 +
> include/libvirt/libvirt-domain.h | 45 ++
> src/driver-hypervisor.h | 8 +
> src/libvirt-domain.c | 108 +
> src/libvirt_public.syms | 5 +
> src/qemu/qemu_capabilities.c | 2 +
> src/qemu/qemu_capabilities.h | 1 +
> src/qemu/qemu_driver.c| 384 --
> src/qemu/qemu_monitor.c | 19 +
> src/qemu/qemu_monitor.h | 9 +
> src/qemu/qemu_monitor_json.c | 50 +++
> src/qemu/qemu_monitor_json.h | 4 +
> src/remote/remote_driver.c| 1 +
> src/remote/remote_protocol.x | 21 +-
> src/remote_p
On Wed, Nov 14, 2018 at 02:22:12PM -0500, John Ferlan wrote:
>
>
> On 11/14/18 4:25 AM, Daniel P. Berrangé wrote:
> > On Tue, Nov 13, 2018 at 03:21:03PM -0500, John Ferlan wrote:
> >> Sending as an RFC primarily because I'm looking for whether either
> >> or both mechanisms in the series is more or less desired. Likewise,
> >> if it's felt that the current process of telling customers to just
> >> delete the cache is acceptible, then so be it. If there's other ideas
> >> I'm willing to give them a go too. I did consider adding a virsh
> >> option to "virsh capabilities" (still possible) and/or a virt-admin
> >> option to force the refresh. These just were "easier" and didn't
> >> require an API adjustment to implement.
> >>
> >> Patch1 is essentially a means to determine if the kernel config
> >> was changed to allow nested virtualization and to force a refresh
> >> of the capabilities in that case. Without doing so the CPU settings
> >> for a guest may not add the vmx=on depending on configuration and
> >> for the user that doesn't make sense. There is a private bz on this
> >> so I won't bother posting it.
> >>
> >> Patch2 and Patch3 make use of the 'service libvirtd reload' function
> >> in order to invalidate all the entries in the internal QEMU capabilities
> >> hash table and then to force a reread. This perhaps has downsides related
> >> to guest usage and previous means to use reload and not refresh if a guest
> >> was running. On the other hand, we tell people to just clear the QEMU
> >> capabilities cache (e.g. rm /var/cache/libvirt/qemu/capabilities/*.xml)
> >> and restart libvirtd, so in essence, the same result. It's not clear
> >> how frequently this is used (it's essentially a SIGHUP to libvirtd).
> >
> > IMHO the fact that we cache stuff should be completely invisible outside
> > of libvirt. Sure we've had some bugs in this area, but they are not very
> > frequent so I'm not enthusiastic to expose any knob to force rebuild beyond
> > just deleting files.
> >
>
> OK - so that more or less obviates patch2 and patch3...
>
> Of course the fact that we cache stuff hasn't been completely invisible
> and telling someone to fix the problem by "simply" removing the cache
> files and pointing them to the cache location seems a bit "awkward" once
> you figure out that is the problem of course. Many times it's just not
> intuitively obvious!
>
> OTOH, baking in the idea that a "reload" could remove the chance that
> caching was the problem could be useful. I guess it just felt like it
> was a perhaps less used option. Assuming of course most would use
> stop/start or restart instead.
Looking at this from a more general POV, cache invalidation bugs are
just one of many different bugs that can & have impacted libvirt over
the years. Adding a force reload API is essentially saying we want to
have
virConnectWhackPossibleBug()
because we think we might have a future bug. I don't think that is a
good design precedent or rationale - essentially its admitting failure.
If caching really were so terribly implemented that this is considered
needed, then I'd argue caching should be deleted. I don't think we are
in such a bad case though - the kind of problems have been fairly
niche in impact.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20181114233831.10374-1-ehabk...@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH for-4.0 v2] virtio: Provide version-specific
variants of virtio PCI devices
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
20332af virtio: Provide version-specific variants of virtio PCI devices
=== OUTPUT BEGIN ===
Checking PATCH 1/1: virtio: Provide version-specific variants of virtio PCI
devices...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#668:
new file mode 100644
ERROR: line over 90 characters
#724: FILE: tests/acceptance/virtio_version.py:52:
+return devtype in [d['name'] for d in vm.command('qom-list-types',
implements=implements)]
ERROR: line over 90 characters
#726: FILE: tests/acceptance/virtio_version.py:54:
+def get_interfaces(vm, devtype, interfaces=['pci-express-device',
'conventional-pci-device']):
WARNING: line over 80 characters
#780: FILE: tests/acceptance/virtio_version.py:108:
+dev_1_0,nt_ifaces = self.run_device('%s-non-transitional' %
(qemu_devtype))
WARNING: line over 80 characters
#804: FILE: tests/acceptance/virtio_version.py:132:
+dev_trans,trans_ifaces = self.run_device('%s-transitional' %
(qemu_devtype))
total: 2 errors, 3 warnings, 732 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
