Re: GSoC'20 Interested Student: Adding support to Jailhouse Hypervisor

[PRAKHAR BANSAL]

Hi Jan,

Thanks for the reply. I looked deeper into the libvirt and Jailhouse source
code and found following two things that seem relevant to the project I am
interested in.

- Libvirt driver interface at [libvirt.git]
 / src
 / driver.h

- Jailhouse tool, which is using the ioctl API of the Jailhouse, available
at https://github.com/siemens/jailhouse/blob/master/tools/jailhouse.c.

With the help of the above two, it looks like, a libvirt driver for the
Jailhouse can be implemented. Let me know if I am moving in the right
direction so far.

I have been looking at the other libvirt driver implementations for
hypervisors like HyperV and VMware to understand their implementation and
learn from there.

Thanks & Regards,

Prakhar Bansal
Graduate Student' 20, Computer Engineering
Iowa State University, Ames, IA





On Mon, Mar 23, 2020 at 3:07 AM Jan Kiszka  wrote:

> Hi Prakhar,
>
> On 23.03.20 07:53, PRAKHAR BANSAL wrote:
> > Hello All,
> >
> > My name is Prakhar Bansal and I am a graduate student in Computer
> > Engineering at Iowa State University, US.
> > I have experience with Analysing Performance of Applications running
> > inside multiple virtual machines hosted by the libvirt QEMU-KVM through
> > virt-manager.
> >
> > I am interested in working on the project to develop a Libvirt driver
> > for the Jailhouse hypervisor. I looked into the initial attempt on the
> > Jailhouse driver which seems to be based on the Jailhouse command-line
> > interface. I am currently looking into learning and understanding the
> > kernel APIs for jailhouse hypervisor.
>
> Thanks for your interest!
>
> > I followed the below articles mentioned by Valentine Sinitsyn to begin
> > learning about the Jailhouse hypervisor.
> >
> > https://lwn.net/Articles/578295/
> > https://lwn.net/Articles/578852/
> >
> > I have a few questions regarding this project, please let me know if
> > someone can help me out.
>
> Sure, go ahead. Depending on the scope of the question, libvirt might be
> the better community to ask. Therefore, I'm adding its list to this thread.
>
> Jan
>
> >
> > Thanks & Regards,
> > Prakhar Bansal
> >
>
> --
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux
>

Re: [PATCH v4 0/2] introduction of migration_version attribute for VFIO live migration

[Yan Zhao]

On Tue, Mar 24, 2020 at 10:49:54PM +0800, Alex Williamson wrote:
> On Tue, 24 Mar 2020 09:23:31 +
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Yan Zhao (yan.y.z...@intel.com) wrote:
> > > On Tue, Mar 24, 2020 at 05:29:59AM +0800, Alex Williamson wrote:  
> > > > On Mon, 3 Jun 2019 20:34:22 -0400
> > > > Yan Zhao  wrote:
> > > >   
> > > > > On Tue, Jun 04, 2019 at 03:29:32AM +0800, Alex Williamson wrote:  
> > > > > > On Thu, 30 May 2019 20:44:38 -0400
> > > > > > Yan Zhao  wrote:
> > > > > > 
> > > > > > > This patchset introduces a migration_version attribute under 
> > > > > > > sysfs of VFIO
> > > > > > > Mediated devices.
> > > > > > > 
> > > > > > > This migration_version attribute is used to check migration 
> > > > > > > compatibility
> > > > > > > between two mdev devices of the same mdev type.
> > > > > > > 
> > > > > > > Patch 1 defines migration_version attribute in
> > > > > > > Documentation/vfio-mediated-device.txt
> > > > > > > 
> > > > > > > Patch 2 uses GVT as an example to show how to expose 
> > > > > > > migration_version
> > > > > > > attribute and check migration compatibility in vendor driver.
> > > > > > 
> > > > > > Thanks for iterating through this, it looks like we've settled on
> > > > > > something reasonable, but now what?  This is one piece of the 
> > > > > > puzzle to
> > > > > > supporting mdev migration, but I don't think it makes sense to 
> > > > > > commit
> > > > > > this upstream on its own without also defining the remainder of how 
> > > > > > we
> > > > > > actually do migration, preferably with more than one working
> > > > > > implementation and at least prototyped, if not final, QEMU support. 
> > > > > >  I
> > > > > > hope that was the intent, and maybe it's now time to look at the 
> > > > > > next
> > > > > > piece of the puzzle.  Thanks,
> > > > > > 
> > > > > > Alex
> > > > > 
> > > > > Got it. 
> > > > > Also thank you and all for discussing and guiding all along:)
> > > > > We'll move to the next episode now.  
> > > > 
> > > > Hi Yan,
> > > > 
> > > > As we're hopefully moving towards a migration API, would it make sense
> > > > to refresh this series at the same time?  I think we're still expecting
> > > > a vendor driver implementing Kirti's migration API to also implement
> > > > this sysfs interface for compatibility verification.  Thanks,
> > > >  
> > > Hi Alex
> > > Got it!
> > > Thanks for reminding of this. And as now we have vfio-pci implementing
> > > vendor ops to allow live migration of pass-through devices, is it
> > > necessary to implement similar sysfs node for those devices?
> > > or do you think just PCI IDs of those devices are enough for libvirt to
> > > know device compatibility ?  
> > 
> > Wasn't the problem that we'd have to know how to check for things like:
> >   a) Whether different firmware versions in the device were actually
> > compatible
> >   b) Whether minor hardware differences were compatible - e.g. some
> > hardware might let you migrate to the next version of hardware up.
> 
> Yes, minor changes in hardware or firmware that may not be represented
> in the device ID or hardware revision.  Also the version is as much for
> indicating the compatibility of the vendor defined migration protocol
> as it is for the hardware itself.  I certainly wouldn't be so bold as
> to create a protocol that is guaranteed compatible forever.  We'll need
> to expose the same sysfs attribute in some standard location for
> non-mdev devices.  I assume vfio-pci would provide the vendor ops some
> mechanism to expose these in a standard namespace of sysfs attributes
> under the device itself.  Perhaps that indicates we need to link the
> mdev type version under the mdev device as well to make this
> transparent to userspace tools like libvirt.  Thanks,
>
Got it. will do it.
Thanks!

Yan

[libvirt PATCH] util: keep the pidfile locked

[marcandre . lureau]

From: Marc-André Lureau 

Unfortunately, advisory record locking lose the lock if any fd refering
to the file is closed. There doesn't seem to be a way to preserve the
lock atomically. We could eventually retake the lock if low pidfilefd
is required.

This fixes processes being leaked, as they are not killed in
virPidFileForceCleanupPath() if the lock can be taken. Here also, we may
consider this is not good enough, as a process may leak by simply
closing the pidfilefd.

Fixes commit d146105f1e4a9e0ab179f0b78c070ea38b9d5334 ("virCommand:
Actually acquire pidfile instead of just writing it")

Signed-off-by: Marc-André Lureau 
---
 src/util/vircommand.c   | 12 ++--
 tests/commanddata/test4.log |  2 +-
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 77078d09fb..b84fb40948 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -797,8 +797,7 @@ virExec(virCommandPtr cmd)
 virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
 goto fork_error;
 if (cmd->pidfile) {
-VIR_AUTOCLOSE pidfilefd = -1;
-int newpidfilefd = -1;
+int pidfilefd = -1;
 char c;
 
 pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, getpid());
@@ -818,14 +817,7 @@ virExec(virCommandPtr cmd)
 VIR_FORCE_CLOSE(pipesync[0]);
 VIR_FORCE_CLOSE(pipesync[1]);
 
-/* This is here only to move the pidfilefd
- * to the lowest possible number. */
-if ((newpidfilefd = dup(pidfilefd)) < 0) {
-virReportSystemError(errno, "%s", _("Unable to dup FD"));
-goto fork_error;
-}
-
-/* newpidfilefd is intentionally leaked. */
+/* pidfilefd is intentionally leaked. */
 }
 
 if (cmd->hook) {
diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
index 5820f28307..24a37a7e96 100644
--- a/tests/commanddata/test4.log
+++ b/tests/commanddata/test4.log
@@ -9,7 +9,7 @@ ENV:USER=test
 FD:0
 FD:1
 FD:2
-FD:3
+FD:5
 DAEMON:yes
 CWD:/
 UMASK:0022
-- 
2.26.0.rc2.42.g98cedd0233

Re: [PATCH] docs: fix typo in domcaps host-model CPU description

[Jim Fehlig]

On 3/24/20 8:30 AM, Jim Fehlig wrote:

On 3/24/20 3:44 AM, Jiri Denemark wrote:

The situation on the first machine is a bit strange as there are no
features disabled in host-model CPU definition, which makes it unclear
why QEMU reports Cascadelake-Server as unusable (QEMU reports the
reason, but we don't do so yet).


It's not clear to me even after looking at the output of cpu-gather.


Anyway, would you mind running the tests/cputestdata/cpu-gather.sh
script on both machines (make sure to install qemu, python3, and cpuid
packages first) and send us the output so that we can check the CPU
models are properly detected?


Attached.


While experimenting with some guests on the first machine (kernel 4.12.14, qemu 
3.1.1, libvirt.git master) I encountered 
https://bugs.launchpad.net/intel/+bug/1828495 while trying to restore a guest 
whose CPU was set to host-model. The guest starts fine, although I do see


qemu-system-x86_64: warning: host doesn't support requested feature: 
CPUID.07H:ECX [bit 4]


in the guest log file. Save works as expected but then restore fails with

error: Failed to restore domain from /home/jim/sles12sp5-kvm.save
error: operation failed: guest CPU doesn't match specification: missing 
features: ospke


I wouldn't be surprised if my kernel and qemu are missing some commits mentioned 
in the LP bug (there are a lot of them!), but shouldn't it be possible to 
restore the guest if libvirt previously started it successfully?


Regards,
Jim

Re: [libvirt PATCH v2 9/9] gitlab: add job for building latest potfile

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 06:51:10PM +0100, Andrea Bolognani wrote:
> On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> > +# This artifact published by this job is downloaded to push to Weblate
> > +# for translation usage:
> > +#
> > https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=potfile
> > +potfile:
> > +  stage: prebuild
> > +  only:
> > +refs:
> > +  - master
> 
> As discussed in patch 6/9, it would make more sense for this to be
> in a postbuild stage, after all other builds have succeeded, than
> in a prebuild stage.

We want the pot file build to run regardless of whether the other
builds succeed.  There may be transient failures with builds and
we want to publish the pot & website content regardless, once it
is in master.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2 6/9] gitlab: add x86_64 native CI jobs

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 06:47:01PM +0100, Andrea Bolognani wrote:
> On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> > To control the total CI execution time, we split the native jobs into
> > two distinct stages. A representative set of distros are used as the
> > primary native build sanity test, run for everyone regardless of whether
> > pre/post merge, and on any branch. The remaining distros are set to run
> > after the cross builds, and only execute for master branch, and thus
> > will only run for post-merge. When we switch to using a merge request
> > workflow, these extra jobs can be triggered when the merge request is
> > opened.
> 
> I don't get the rationale behind the split.
> 
> Right now we're not using merge requests, but we're limiting the
> number of jobs for the merge request case; at the same time, we say
> that once we switch to a MR-based workflow, we're going to run the
> extra jobs on each merge request as well. So what does the
> distinction buy us?

With this split today, if I push to my private fork, then the
reduced set of jobs run. This gives quick turnaround for developers
developing patches.

When it gets reviewed & pushed to master, the full set run post
merge.

In the future, when we switch to merge requests, we'll change
it so that the full set run when the merge request is opened,
instad of post-merge.

What is run for developers private branches will remain the
same

> I think a better split would be:
> 
>   * pick a selection of jobs that includes both native and cross
> build, across various distros of different vintage, such that
> they can all run without waiting on shared runners. This can be
> used by developers as a reasonably quick (~20 minutes) smoke
> test while working on a feature;

"without waiting on shared runners" is undefined, as whether you
wait & how long, is dependant on many things outside our control.
As notedin the cover letter though, this minimal set of native
+ cross build jobs gives about a 35 min turn around based on
load I see today. I think that's acceptably fast.

>   * run every other build as a second stage in the pipeline, both
> for master and (later) for merge requests, to validate patches
> as thoroughly as possible before they get into libvirt;

There's no point in doing both master & merge requests - only one
or the other. Right now since we're not doing merge reuqests, I
picked master. Later we'll switch it to be on merge requests.

>   * run the website and potfile jobs for master only, as their
> purpose is not to validate the build (the regular Fedora 31 job
> will have done that already) but to publish the artifacts.

Doing the website job on all branches was intentionale because by
publishing the artifact, developers can now browse the website
for their job during development to see the effect on their changes.
eg for my most recent job I can view:

https://berrange.gitlab.io/-/libvirt/-/jobs/483635204/artifacts/website/index.html

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2 9/9] gitlab: add job for building latest potfile

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> +# This artifact published by this job is downloaded to push to Weblate
> +# for translation usage:
> +#
> https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=potfile
> +potfile:
> +  stage: prebuild
> +  only:
> +refs:
> +  - master

As discussed in patch 6/9, it would make more sense for this to be
in a postbuild stage, after all other builds have succeeded, than
in a prebuild stage.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 8/9] gitlab: restrict git history to 100 commits

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> We don't need the full git history when running CI jobs. From a code POV
> we only need the most recent commit, but we want to be able to run
> checks on the commits too. In particular to validate the DCO signoff for
> each commit.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 1 +
>  1 file changed, 1 insertion(+)

This would also make more sense right at the beginning of the series
instead of in the middle of it.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 7/9] gitlab: add variable for make command name

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> To facilitate future jobs that will use FreeBSD
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)

As mentioned, this should be the very first patch in the series.

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 6/9] gitlab: add x86_64 native CI jobs

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> To control the total CI execution time, we split the native jobs into
> two distinct stages. A representative set of distros are used as the
> primary native build sanity test, run for everyone regardless of whether
> pre/post merge, and on any branch. The remaining distros are set to run
> after the cross builds, and only execute for master branch, and thus
> will only run for post-merge. When we switch to using a merge request
> workflow, these extra jobs can be triggered when the merge request is
> opened.

I don't get the rationale behind the split.

Right now we're not using merge requests, but we're limiting the
number of jobs for the merge request case; at the same time, we say
that once we switch to a MR-based workflow, we're going to run the
extra jobs on each merge request as well. So what does the
distinction buy us?

I think a better split would be:

  * pick a selection of jobs that includes both native and cross
build, across various distros of different vintage, such that
they can all run without waiting on shared runners. This can be
used by developers as a reasonably quick (~20 minutes) smoke
test while working on a feature;

  * run every other build as a second stage in the pipeline, both
for master and (later) for merge requests, to validate patches
as thoroughly as possible before they get into libvirt;

  * run the website and potfile jobs for master only, as their
purpose is not to validate the build (the regular Fedora 31 job
will have done that already) but to publish the artifacts.

Does that sound reasonable?

-- 
Andrea Bolognani / Red Hat / Virtualization

[PATCH v2 3/4] qemu: make Hyperv settings exclusive to x86 and aarch64

[Daniel Henrique Barboza]

Hyperv features are supported by both x86 and aarch64. The 
declaration in the XML by itself is benign to other architectures,
but any of its 14 current features will break QEMU with an error
like this (from ppc64):

qemu-kvm: Expected key=value format, found hv_relaxed

This is a more extreme case than the one for apic eoi because we
would need an extra 'switch' statement, with all current Hyperv
features in the body of qemuDomainDefValidateFeatures(), to
check if the user attempted to activate any of them. It's easier to
simply fail to launch with any 'hyperv' declaration in the XML for
every arch which is not x86 and aarch64.

A fair disclaimer about Windows and PowerPC: the last Windows version
that ran in the architecture is the hall of famer Windows NT 4.0,
launched in 1996 and with end of extended support for the Server
version in 2004 [1]. I am acknowledging that there might be Windows
NT 4.0 users running in PowerPC, but not enough people running it
under KVM/QEMU to justify Libvirt allowing 'hyperv' to exist in the
domain XML of ppc64 domains.

[1] https://en.wikipedia.org/wiki/Windows_NT_4.0

Suggested-by: Andrea Bolognani 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c56b3a893b..b03e7bbb30 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5284,12 +5284,23 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_HYPERV:
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+!ARCH_IS_X86(def->os.arch) && !qemuDomainIsARMVirt(def)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Hyperv features are not supported for "
+ "architecture '%s' or machine type '%s'"),
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
 case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
 case VIR_DOMAIN_FEATURE_PRIVNET:
-case VIR_DOMAIN_FEATURE_HYPERV:
 case VIR_DOMAIN_FEATURE_CAPABILITIES:
 case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_MSRS:
-- 
2.25.1

[PATCH v2 2/4] qemu: avoid launching non-x86 guests with 'pvspinlock' setting

[Daniel Henrique Barboza]

The 'pvspinlock' feature is x86 only. The "" declaration
will always have a value 'on' or 'off', and both will break QEMU when
launching non-x86 guests. This is the error message for
"" when running a ppc64 guest:

qemu-kvm: Expected key=value format, found +kvm_pv_unhalt

A similar error message is thrown for "".

This patch prevents non-x86 guests from launching with any
pvspinlock setting with a more informative error message:

error: unsupported configuration: The 'pvspinlock' feature is not
supported for architecture 'ppc64' or machine type 'pseries'

Suggested-by: Andrea Bolognani 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 73a65128c6..c56b3a893b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5271,13 +5271,25 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_PVSPINLOCK:
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+!ARCH_IS_X86(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The '%s' feature is not supported for "
+ "architecture '%s' or machine type '%s'"),
+ featureName,
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
 case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
 case VIR_DOMAIN_FEATURE_PRIVNET:
 case VIR_DOMAIN_FEATURE_HYPERV:
-case VIR_DOMAIN_FEATURE_PVSPINLOCK:
 case VIR_DOMAIN_FEATURE_CAPABILITIES:
 case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_MSRS:
-- 
2.25.1

[PATCH v2 1/4] qemu: avoid launching non-x86 guests with APIC-EOI setting

[Daniel Henrique Barboza]

The "" feature, although it's available only for x86 guests,
can be declared in the domain XML of other archs without errors.
But setting its 'eoi' attribute will break QEMU. For "",
in a ppc64 guest:

qemu-kvm: Expected key=value format, found +kvm_pv_eoi

A similar error happens with eoi='off'.

One can argue that it's better to simply forbid launching non-x86
guests with "" declared in the XML - it is a feature that
the architecture doesn't support and this would make it clearer
about it. This is sensible, but there are non-x86 guests that are
running with "" declared in the domain (and A LOT of guests
running with "" for that matter, probably reminiscent of x86
templates that were reused for other archs) that will stop working if we
go this route.

A more subtle approach is to detect if the 'eoi' element is being set
for non-x86 guests and warn the user about it with a better error
message than the one QEMU provides. This is the new error message
when any value is set for the 'eoi' element in a ppc64 XML:

error: unsupported configuration: The 'eoi' attribute of the 'apic'
feature is not supported for architecture 'ppc64' or machine type
'pseries'.

Suggested-by: Andrea Bolognani 
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1236440
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_domain.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c9fb47d17..73a65128c6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5255,8 +5255,23 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
 }
 break;
 
-case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_APIC:
+/* The kvm_pv_eoi feature is x86-only. */
+if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+def->apic_eoi != VIR_TRISTATE_SWITCH_ABSENT &&
+!ARCH_IS_X86(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("The 'eoi' attribute of the '%s' feature "
+ "is not supported for architecture '%s' or "
+ "machine type '%s'"),
+ featureName,
+ virArchToString(def->os.arch),
+ def->os.machine);
+ return -1;
+}
+break;
+
+case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
 case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
-- 
2.25.1

[PATCH v2 4/4] qemu: allow PMU feature to be enabled for ppc64 guests

[Daniel Henrique Barboza]

The PMU feature is enabled by default in ppc64 guests and can't
be disabled via Libvirt or QEMU [1]. The current PMU feature
implementation does not allow PMU to enabled or disabled in the
ppc64 guest. Declaring the PMU feature will make the 'pmu'
property to be passed on to QEMU, but this property isn't
available for ppc64:

qemu-kvm: can't apply global host-powerpc64-cpu.pmu=on: Property '.pmu' not 
found

A similar error is thrown when trying to disable the PMU.

This patch standardizes the PMU handling for ppc64 guests by:

- throwing an error if the user attempts to set the feature to
'off', given that this feature can't be turned off at all;

- allowing the feature to be declared as 'on' in the domain XML.
This is done by skipping ppc64 guests when creating the command
line for this feature.

[1] https://www.redhat.com/archives/libvir-list/2020-March/msg00874.html
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_command.c |  4 +++-
 src/qemu/qemu_domain.c  | 11 ++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3a772fa3f3..d1b689dfd3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6784,7 +6784,9 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
 }
 }
 
-if (def->features[VIR_DOMAIN_FEATURE_PMU]) {
+/* ppc64 guests always have PMU enabled, but the 'pmu' option
+ * is not supported. */
+if (def->features[VIR_DOMAIN_FEATURE_PMU] && !ARCH_IS_PPC64(def->os.arch)) 
{
 virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU];
 virBufferAsprintf(, ",pmu=%s",
   virTristateSwitchTypeToString(pmu));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b03e7bbb30..7d29f3f114 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5296,13 +5296,22 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
 }
 break;
 
+case VIR_DOMAIN_FEATURE_PMU:
+if (def->features[i] == VIR_TRISTATE_SWITCH_OFF &&
+ARCH_IS_PPC64(def->os.arch)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("PMU is always enabled for architecture 
'%s'"),
+ virArchToString(def->os.arch));
+ return -1;
+}
+break;
+
 case VIR_DOMAIN_FEATURE_ACPI:
 case VIR_DOMAIN_FEATURE_PAE:
 case VIR_DOMAIN_FEATURE_HAP:
 case VIR_DOMAIN_FEATURE_VIRIDIAN:
 case VIR_DOMAIN_FEATURE_PRIVNET:
 case VIR_DOMAIN_FEATURE_CAPABILITIES:
-case VIR_DOMAIN_FEATURE_PMU:
 case VIR_DOMAIN_FEATURE_MSRS:
 case VIR_DOMAIN_FEATURE_LAST:
 break;
-- 
2.25.1

[PATCH v2 0/4] APIC-EOI, pvspinlock, hyperv and PMU changes

[Daniel Henrique Barboza]

Changes in v2:
- changed patch series name to reflect the new approach
- Make APIC-EIO exclusive to x86
- Make pvspinlock exclusive to x86
- Make hyperv exclusive to x86 and aarch64
- allow PMU to be declared as 'on' for ppc64
- previous version:
https://www.redhat.com/archives/libvir-list/2020-March/msg00718.html

Daniel Henrique Barboza (4):
  qemu: avoid launching non-x86 guests with APIC-EOI setting
  qemu: avoid launching non-x86 guests with 'pvspinlock' setting
  qemu: make Hyperv settings exclusive to x86 and aarch64
  qemu: allow PMU feature to be enabled for ppc64 guests

 src/qemu/qemu_command.c |  4 ++-
 src/qemu/qemu_domain.c  | 55 ++---
 2 files changed, 54 insertions(+), 5 deletions(-)

-- 
2.25.1

Re: [libvirt PATCH v2 5/9] gitlab: add mingw cross build CI jobs

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> This pulls in the mingw cross build jobs using Fedora 30 as a base,
> matching what is done on Jenkins and Travis.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 4/9] gitlab: rename the cross build jobs

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:24 +, Daniel P. Berrangé wrote:
> The pipeline UI will truncate the names of jobs after about 15
> characters. As a result with the cross-builds, we truncate the
> most important part of the job name. Putting the most important
> part first is robust against truncation, and we can drop the
> redundant "-cross" stub.
> 
> Reviewed-by: Erik Skultety 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 3/9] gitlab: group jobs into stages

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:23 +, Daniel P. Berrangé wrote:
> Within a stage all jobs run in parallel. Stages are ordered so later
> stages are only executed if previous stages succeeded. By using separate
> stages for the cross builds, we can avoid wasting CI resources if the
> relatively simple website build fails. Later we can avoid running cross
> builds, if the native build fails too.
> 
> Reviewed-by: Erik Skultety 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 2/9] gitlab: reduce number of cross-build CI jobs

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:23 +, Daniel P. Berrangé wrote:
> We're going to add more build jobs to CI, and users have limited time
> granted on the shared CI runners. The number of cross-build jobs
> currently present is not sustainable, so cut it down to two interesting
> jobs to cover big endian and 32-bit platform variants.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  .gitlab-ci.yml | 37 ++---
>  1 file changed, 6 insertions(+), 31 deletions(-)

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 1/9] gitlab: use CI for building website contents

[Andrea Bolognani]

On Tue, 2020-03-24 at 16:23 +, Daniel P. Berrangé wrote:
> Run the bare minimum build that is possible to create the docs. Ideally
> the '--without-remote' arg would be passed, but there are several bugs
> preventing a build from succeeding without the remote driver built.

This comment is no longer necessary, is it?

[...]
> +# This artifact published by this job is downloaded by libvirt.org to
> +# be deployed to the web root:
> +#
> https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
> +website:
> +  script:
> +- mkdir build
> +- cd build
> +- ../autogen.sh --prefix=$(pwd)/../vroot || (cat config.log && exit 1)
> +- make -j $(getconf _NPROCESSORS_ONLN) -C docs
> +- make -j $(getconf _NPROCESSORS_ONLN) -C docs install

Can you please make patch 7/9 the first in the series, so that you
don't need to go back and change this to $MAKE later?

> +- cd ..
> +- mv vroot/share/doc/libvirt/html/ website
> +  image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest

Since the build job is running on Fedora 31 (at the moment), would
it make sense to call it website-fedora-31? The name of the artifact
would remain the same, so eg. the cron job we're going to have on
libvirt.org will not need to be modified every time we bump this.

> +  artifacts:
> +expose_as: 'Website'
> +name: 'website'

I'm not sure the quotes are necessary. Either way, we can remove
them later if they turn out not to be.

-- 
Andrea Bolognani / Red Hat / Virtualization

[libvirt PATCH] docs: news: fix typo

[Ján Tomko]

s/ommited/omitted/

Signed-off-by: Ján Tomko 
---
Pushed as trivial

 docs/news.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/news.xml b/docs/news.xml
index 504cf52738..bcec1d81e9 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -51,7 +51,7 @@
 
   QEMU 5.0 implements NVDIMM memory support for pSeries guests. This
   is done by adding an 'uuid' element in the memory XML, which can
-  either be provided in the XML or, if ommited, generated
+  either be provided in the XML or, if omitted, generated
   automatically.
 
   
-- 
2.25.1

[libvirt PATCH v2 8/9] gitlab: restrict git history to 100 commits

[Daniel P . Berrangé]

We don't need the full git history when running CI jobs. From a code POV
we only need the most recent commit, but we want to be able to run
checks on the commits too. In particular to validate the DCO signoff for
each commit.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9814b6580a..2286d28707 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -6,6 +6,7 @@ stages:
 
 variables:
   MAKE: make
+  GIT_DEPTH: 100
 
 
 # Common templates
-- 
2.24.1

[libvirt PATCH v2 6/9] gitlab: add x86_64 native CI jobs

[Daniel P . Berrangé]

This patch adds x86_64 native CI jobs for all distros that we currently
build container images for. This is a superset of the Linux jobs run on
current Jenkins and Travis platforms.

The remaining missing platforms are FreeBSD and macOS, neither of which
can use the shared runner container based infrastructure.

We may add further native jobs in the future which are not x86_64 based,
if we get access to suitable hardware, thus the jobs all have an arch
prefix in their name, just like the cross-built jobs do.

To control the total CI execution time, we split the native jobs into
two distinct stages. A representative set of distros are used as the
primary native build sanity test, run for everyone regardless of whether
pre/post merge, and on any branch. The remaining distros are set to run
after the cross builds, and only execute for master branch, and thus
will only run for post-merge. When we switch to using a merge request
workflow, these extra jobs can be triggered when the merge request is
opened.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 78 ++
 1 file changed, 78 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 329374a34f..58abcbe1f3 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,6 +1,34 @@
 stages:
   - prebuild
+  - native_build
   - cross_build
+  - native_build_extra
+
+
+# Common templates
+
+# Native jobs that run every time, pre/post merge and on any branch
+.native_build_job_template: _build_job_definition
+  stage: native_build
+  script:
+- mkdir build
+- cd build
+- ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
+- make -j $(getconf _NPROCESSORS_ONLN) syntax-check
+- make -j $(getconf _NPROCESSORS_ONLN) distcheck
+
+# Native jobs that will only run post merge on master branch
+# Switch to running against merge requests later
+.native_build_extra_job_template: _build_extra_job_definition
+  stage: native_build_extra
+  script:
+- mkdir build
+- cd build
+- ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) check
+  only:
+refs:
+ - master
 
 .cross_build_job_template: _build_job_definition
   stage: cross_build
@@ -10,6 +38,56 @@ stages:
 - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
 - make -j $(getconf _NPROCESSORS_ONLN)
 
+
+# Native architecture build + test jobs
+
+x64-debian-9:
+  <<: *native_build_extra_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-debian-9:latest
+
+x64-debian-10:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-debian-10:latest
+
+x64-debian-sid:
+  <<: *native_build_extra_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-debian-sid:latest
+
+x64-centos-7:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-centos-7:latest
+
+x64-centos-8:
+  <<: *native_build_extra_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-centos-8:latest
+
+x64-fedora-30:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-fedora-30:latest
+
+x64-fedora-31:
+  <<: *native_build_extra_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
+
+x64-fedora-rawhide:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-fedora-rawhide:latest
+
+x64-opensuse-151:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-opensuse-151:latest
+
+x64-ubuntu-1604:
+  <<: *native_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-ubuntu-1604:latest
+
+x64-ubuntu-1804:
+  <<: *native_build_extra_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-ubuntu-1804:latest
+
+
+# Cross compiled build jobs
+
 # There are many possible cross-arch jobs we could do, but to preserve
 # limited CI resource time allocated to users, we cut it down to two
 # interesting variants. The default jobs are x86_64, which means 64-bit
-- 
2.24.1

Re: [libvirt PATCH v2 0/9] gitlab: expand the CI job coverage (RESEND)

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 04:23:56PM +, Daniel P. Berrangé wrote:
> There are main goals with this series
> 
>   - Introduce a minimal job building the website and publishing
> an artifact which can be deployed onto libvirt.org
>   - Introduce a minimal job building the libvirt.pot for import
> into Weblate (only runs on git master branch)
>   - Expanding CI jobs to get coverage closer to Travis/Jenkins
>   - Reducing cross-build jobs to just interesting variants,
> since the full set hasn't shown value in detecting failures
> 
> The Linux native job coverage is now a superset of that achieved
> by Travis/Jenkins.
> 
> For post-merge testing the full set of jobs are run on git
> master (measured approx 50 minutes total duration)
> 
> For pre-merge testing the Linux job count is reduced for quicker
> turnaround time for developers (measured ~35 minutes total
> duration)

I meant to say you can see an illustration of the CI pipeline
in this URL.

   https://gitlab.com/berrange/libvirt/pipelines/129242976

Published artifacts are downloaded from the "jobs" tab, and if
we had unit tests published in junit data format, they'd appear
in the "tests" tab IIUC


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 :|

[libvirt PATCH v2 7/9] gitlab: add variable for make command name

[Daniel P . Berrangé]

To facilitate future jobs that will use FreeBSD

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 58abcbe1f3..9814b6580a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -4,6 +4,9 @@ stages:
   - cross_build
   - native_build_extra
 
+variables:
+  MAKE: make
+
 
 # Common templates
 
@@ -14,8 +17,8 @@ stages:
 - mkdir build
 - cd build
 - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
-- make -j $(getconf _NPROCESSORS_ONLN) syntax-check
-- make -j $(getconf _NPROCESSORS_ONLN) distcheck
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) syntax-check
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) distcheck
 
 # Native jobs that will only run post merge on master branch
 # Switch to running against merge requests later
@@ -36,7 +39,7 @@ stages:
 - mkdir build
 - cd build
 - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
-- make -j $(getconf _NPROCESSORS_ONLN)
+- $MAKE -j $(getconf _NPROCESSORS_ONLN)
 
 
 # Native architecture build + test jobs
@@ -120,8 +123,8 @@ website:
 - mkdir build
 - cd build
 - ../autogen.sh --prefix=$(pwd)/../vroot || (cat config.log && exit 1)
-- make -j $(getconf _NPROCESSORS_ONLN) -C docs
-- make -j $(getconf _NPROCESSORS_ONLN) -C docs install
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) -C docs
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) -C docs install
 - cd ..
 - mv vroot/share/doc/libvirt/html/ website
   image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
-- 
2.24.1

[libvirt PATCH v2 0/9] gitlab: expand the CI job coverage (RESEND)

[Daniel P . Berrangé]

There are main goals with this series

  - Introduce a minimal job building the website and publishing
an artifact which can be deployed onto libvirt.org
  - Introduce a minimal job building the libvirt.pot for import
into Weblate (only runs on git master branch)
  - Expanding CI jobs to get coverage closer to Travis/Jenkins
  - Reducing cross-build jobs to just interesting variants,
since the full set hasn't shown value in detecting failures

The Linux native job coverage is now a superset of that achieved
by Travis/Jenkins.

For post-merge testing the full set of jobs are run on git
master (measured approx 50 minutes total duration)

For pre-merge testing the Linux job count is reduced for quicker
turnaround time for developers (measured ~35 minutes total
duration)

Changed in v2:

  - Add more native test jobs to run on git master
  - Restrict git clone depth
  - User variable for "make" command name
  - Add test job to build the master pot file
  - Remove extra configure args for website job
  - Re-ordered patches to reduce repeated changes

Daniel P. Berrangé (9):
  gitlab: use CI for building website contents
  gitlab: reduce number of cross-build CI jobs
  gitlab: group jobs into stages
  gitlab: rename the cross build jobs
  gitlab: add mingw cross build CI jobs
  gitlab: add x86_64 native CI jobs
  gitlab: add variable for make command name
  gitlab: restrict git history to 100 commits
  gitlab: add job for building latest potfile

 .gitlab-ci.yml | 178 -
 1 file changed, 148 insertions(+), 30 deletions(-)

-- 
2.24.1

[libvirt PATCH v2 4/9] gitlab: rename the cross build jobs

[Daniel P . Berrangé]

The pipeline UI will truncate the names of jobs after about 15
characters. As a result with the cross-builds, we truncate the
most important part of the job name. Putting the most important
part first is robust against truncation, and we can drop the
redundant "-cross" stub.

Reviewed-by: Erik Skultety 
Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 8d6b5a0787..563b126662 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -17,11 +17,11 @@ stages:
 # platform, and s390x as an interesting big endian platform. We split
 # between Debian 10 and sid to help detect problems on the horizon.
 
-debian-10-cross-s390x:
+s390x-debian-10:
   <<: *cross_build_job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-s390x:latest
 
-debian-sid-cross-armv7l:
+armv7l-debian-sid:
   <<: *cross_build_job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest
 
-- 
2.24.1

[libvirt PATCH v2 5/9] gitlab: add mingw cross build CI jobs

[Daniel P . Berrangé]

This pulls in the mingw cross build jobs using Fedora 30 as a base,
matching what is done on Jenkins and Travis.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 8 
 1 file changed, 8 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 563b126662..329374a34f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -25,6 +25,14 @@ armv7l-debian-sid:
   <<: *cross_build_job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest
 
+mingw32-fedora-30:
+  <<: *cross_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw32:latest
+
+mingw64-fedora-30:
+  <<: *cross_build_job_definition
+  image: quay.io/libvirt/buildenv-libvirt-fedora-30-cross-mingw64:latest
+
 # This artifact published by this job is downloaded by libvirt.org to
 # be deployed to the web root:
 #
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
-- 
2.24.1

Re: [PATCH] qemu: softfail for TCG capabilities probe

[Ján Tomko]

[please wrap the lines to something sensible like 72 columns,
especially in the commit message]

On a Monday in 2020, Tobin Feldman-Fitzthum wrote:

As of version 2.10, QEMU can be built without the TCG. When libvirt
determines that capabilities of a QEMU binary using QMP, it launches a
QEMU process with KVM acceleration and TCG as a fallback. If QEMU
supports KVM, a second probe is performed, forcing QEMU to use only
TCG. This causes an error if the QEMU binary was built without TCG.



This patch allows execution to continue when the second probe fails.
Thus libvirt can be used with QEMU built without TCG.


The second probe can fail for other reasons so ideally we would not even
call it if we know TCG is unsupported.

grep'ing through collected capabilitiesdata replies shows that since
2.10.0 the "qom-list-types" command shows two new 'types':
kvm-accel and tcg-accel

So we could introduce QEMU_CAPS_TCG, that would be set to true if
tcg-accel is present, or none of them are (to account for QEMU's earlier
than 2.10 - libvirt currently supports QEMUs back to 1.5.0)


First patch.
Feedback appreciated. If better solution, please advise.



Messages like this should not be in the git history, you can put them
under the three dashes below:


Signed-off-by: Tobin Feldman-Fitzthum 
---
src/qemu/qemu_capabilities.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)



Jano


signature.asc
Description: PGP signature

[libvirt PATCH v2 3/9] gitlab: group jobs into stages

[Daniel P . Berrangé]

Within a stage all jobs run in parallel. Stages are ordered so later
stages are only executed if previous stages succeeded. By using separate
stages for the cross builds, we can avoid wasting CI resources if the
relatively simple website build fails. Later we can avoid running cross
builds, if the native build fails too.

Reviewed-by: Erik Skultety 
Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 1249ec6df5..8d6b5a0787 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,4 +1,9 @@
-.job_template: _definition
+stages:
+  - prebuild
+  - cross_build
+
+.cross_build_job_template: _build_job_definition
+  stage: cross_build
   script:
 - mkdir build
 - cd build
@@ -13,17 +18,18 @@
 # between Debian 10 and sid to help detect problems on the horizon.
 
 debian-10-cross-s390x:
-  <<: *job_definition
+  <<: *cross_build_job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-s390x:latest
 
 debian-sid-cross-armv7l:
-  <<: *job_definition
+  <<: *cross_build_job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest
 
 # This artifact published by this job is downloaded by libvirt.org to
 # be deployed to the web root:
 #
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
 website:
+  stage: prebuild
   script:
 - mkdir build
 - cd build
-- 
2.24.1

[libvirt PATCH v2 9/9] gitlab: add job for building latest potfile

[Daniel P . Berrangé]

Whenever there is a change to the translatable strings we need to push
a new libvirt.pot to weblate. This only needs to be done when code
merges into git master, so the job is restricted to that branch.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 2286d28707..08e8df5d25 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -136,3 +136,29 @@ website:
 expire_in: 30 days
 paths:
   - website
+
+
+# This artifact published by this job is downloaded to push to Weblate
+# for translation usage:
+#
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=potfile
+potfile:
+  stage: prebuild
+  only:
+refs:
+  - master
+  script:
+- mkdir build
+- cd build
+- ../autogen.sh || (cat config.log && exit 1)
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) -C src generated-sources
+- $MAKE -j $(getconf _NPROCESSORS_ONLN) -C po libvirt.pot
+- cd ..
+- mv build/po/libvirt.pot libvirt.pot
+  image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
+  artifacts:
+expose_as: 'Potfile'
+name: 'potfile'
+when: on_success
+expire_in: 30 days
+paths:
+  - libvirt.pot
-- 
2.24.1

[libvirt PATCH v2 2/9] gitlab: reduce number of cross-build CI jobs

[Daniel P . Berrangé]

We're going to add more build jobs to CI, and users have limited time
granted on the shared CI runners. The number of cross-build jobs
currently present is not sustainable, so cut it down to two interesting
jobs to cover big endian and 32-bit platform variants.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 37 ++---
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 180861f082..1249ec6df5 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -5,29 +5,12 @@
 - ../autogen.sh $CONFIGURE_OPTS || (cat config.log && exit 1)
 - make -j $(getconf _NPROCESSORS_ONLN)
 
-# We could run every arch on every versions, but it is a little
-# overkill. Instead we split jobs evenly across 9, 10 and sid
-# to achieve reasonable cross-coverage.
-
-debian-9-cross-armv6l:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-armv6l:latest
-
-debian-9-cross-mips64el:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-mips64el:latest
-
-debian-9-cross-mips:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-9-cross-mips:latest
-
-debian-10-cross-aarch64:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-aarch64:latest
-
-debian-10-cross-ppc64le:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-10-cross-ppc64le:latest
+# There are many possible cross-arch jobs we could do, but to preserve
+# limited CI resource time allocated to users, we cut it down to two
+# interesting variants. The default jobs are x86_64, which means 64-bit
+# and little endian. We thus pick armv7l as an interesting 32-bit
+# platform, and s390x as an interesting big endian platform. We split
+# between Debian 10 and sid to help detect problems on the horizon.
 
 debian-10-cross-s390x:
   <<: *job_definition
@@ -37,14 +20,6 @@ debian-sid-cross-armv7l:
   <<: *job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-armv7l:latest
 
-debian-sid-cross-i686:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-i686:latest
-
-debian-sid-cross-mipsel:
-  <<: *job_definition
-  image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-mipsel:latest
-
 # This artifact published by this job is downloaded by libvirt.org to
 # be deployed to the web root:
 #
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
-- 
2.24.1

[libvirt PATCH v2 1/9] gitlab: use CI for building website contents

[Daniel P . Berrangé]

Run the bare minimum build that is possible to create the docs. Ideally
the '--without-remote' arg would be passed, but there are several bugs
preventing a build from succeeding without the remote driver built.

The generated website is published as an artifact and thus is browsable
on build completion and can be downloaded as a zip file.

Signed-off-by: Daniel P. Berrangé 
---
 .gitlab-ci.yml | 21 +
 1 file changed, 21 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index ea49c6178b..180861f082 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -44,3 +44,24 @@ debian-sid-cross-i686:
 debian-sid-cross-mipsel:
   <<: *job_definition
   image: quay.io/libvirt/buildenv-libvirt-debian-sid-cross-mipsel:latest
+
+# This artifact published by this job is downloaded by libvirt.org to
+# be deployed to the web root:
+#
https://gitlab.com/libvirt/libvirt/-/jobs/artifacts/master/download?job=website
+website:
+  script:
+- mkdir build
+- cd build
+- ../autogen.sh --prefix=$(pwd)/../vroot || (cat config.log && exit 1)
+- make -j $(getconf _NPROCESSORS_ONLN) -C docs
+- make -j $(getconf _NPROCESSORS_ONLN) -C docs install
+- cd ..
+- mv vroot/share/doc/libvirt/html/ website
+  image: quay.io/libvirt/buildenv-libvirt-fedora-31:latest
+  artifacts:
+expose_as: 'Website'
+name: 'website'
+when: on_success
+expire_in: 30 days
+paths:
+  - website
-- 
2.24.1

[PATCH 6/7] vbox: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c |  2 --
 src/vbox/vbox_common.c | 16 
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 53fd13e80f..665bb10b27 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15716,8 +15716,6 @@ virDomainVideoDefaultType(const virDomainDef *def)
 {
 switch ((virDomainVirtType)def->virtType) {
 case VIR_DOMAIN_VIRT_VBOX:
-return VIR_DOMAIN_VIDEO_TYPE_VBOX;
-
 case VIR_DOMAIN_VIRT_TEST:
 case VIR_DOMAIN_VIRT_VMWARE:
 case VIR_DOMAIN_VIRT_VZ:
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 618663952a..e98ae04ec0 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -62,9 +62,25 @@ static virMutex vbox_driver_lock = VIR_MUTEX_INITIALIZER;
 static vboxDriverPtr vbox_driver;
 static vboxDriverPtr vboxDriverObjNew(void);
 
+static int
+vboxDomainDevicesDefPostParse(virDomainDeviceDefPtr dev G_GNUC_UNUSED,
+  const virDomainDef *def G_GNUC_UNUSED,
+  unsigned int parseFlags G_GNUC_UNUSED,
+  void *opaque G_GNUC_UNUSED,
+  void *parseOpaque G_GNUC_UNUSED)
+{
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VBOX;
+}
+
+return 0;
+}
+
 static virDomainDefParserConfig vboxDomainDefParserConfig = {
 .macPrefix = { 0x08, 0x00, 0x27 },
 .features = VIR_DOMAIN_DEF_FEATURE_NAME_SLASH,
+.devicesPostParseCallback = vboxDomainDevicesDefPostParse,
 };
 
 static virCapsPtr
-- 
2.25.1

[PATCH 1/7] bhyve: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/bhyve/bhyve_domain.c | 5 +
 src/conf/domain_conf.c   | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index a2a0619846..40ee461b19 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -161,6 +161,11 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 }
 }
 
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video.type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+dev->data.video.type = VIR_DOMAIN_VIDEO_TYPE_GOP;
+}
+
 return 0;
 }
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a88a5a744e..957989e848 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15738,7 +15738,6 @@ virDomainVideoDefaultType(const virDomainDef *def)
 else
 return VIR_DOMAIN_VIDEO_TYPE_PARALLELS;
 case VIR_DOMAIN_VIRT_BHYVE:
-return VIR_DOMAIN_VIDEO_TYPE_GOP;
 case VIR_DOMAIN_VIRT_QEMU:
 case VIR_DOMAIN_VIRT_KQEMU:
 case VIR_DOMAIN_VIRT_KVM:
-- 
2.25.1

Re: [libvirt PATCH 4/5] gitlab: add several native CI jobs

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 04:41:20PM +0100, Andrea Bolognani wrote:
> On Mon, 2020-03-23 at 15:16 +, Daniel P. Berrangé wrote:
> > On Mon, Mar 23, 2020 at 04:00:17PM +0100, Andrea Bolognani wrote:
> > > Since we're going to need a FreeBSD machine configured as a GitLab
> > > runner, wouldn't it make sense for us to also create one or more
> > > Linux machines that would run jobs on behalf of GitLab CI? That way
> > > we'd remove the limitation on the number of containers we can have
> > > running at the same time, and still have everything controlled by
> > > GitLab.
> > 
> > It is bit more complicated. Custom GitLab runners are associated with
> > a project (and its git repos). Those runners will be used for any CI jobs
> > run in the context of repositories owned by that project.  Obviously this
> > covers post-merge build jobs, or regularly scheduled build jobs.
> > 
> > It gets more interesting with merge requests though, because in a normal
> > project, the merge request is not being submmitted from a branch on the
> > main repo, instead it is submitted from a branch on the user's forked
> > repo copy. As such the CI jobs for the merge request run in the contxt
> > of the forked repo and do not have access to the customer runners owned
> > by the project.
> 
> Where is this documented? I've been looking through the gitlab-runner
> documentation but I haven't been able to find anything about this.

It isn't clearly documented anywhere, but you can see it in practice
quite easily. Fork a project, and open a merge request. You'll see
the pipelines are reported against your fork, and not against the
original repo. Your fork won't have access to anything except the
standard shared runners.

> > This is a intentional security restriction to prevent denial of service
> > from arbitrary users from submitting (malicious) pull requests that
> > trigger builds on 3rd party runners.
> 
> It should be possible to prevent a DoS by configuring a limit on the
> number of concurrent jobs that are allowed for the runner (see [1]),
> so this looks like a moot point.
> 
> As for running the build jobs securely, the Docker executor should
> take care of that when it comes to Linux; for FreeBSD, I think we'd
> have to use a custom executor that drops privileges instead, because
> Docker is not a possibility there.

That's all fine, but there's nothing we cna do about it. This is not
a restriction we have the ability to relax, as it is policy set by
the GitLab CI infra on runner usage.

> > Thus, we want to maximise the amount of build coverage we get on the
> > shared runners, as this is what merge requests will be using, and what
> > contributors will be using for tests before submitting code for review.
> > 
> > We'll have custom runners for providing coverage of platforms where
> > containers aren't viable (non-Linux), as well as providing additional
> > capacity.
> 
> I don't understand how this would work. If the CI configuration
> contains say 20 jobs, 18 of which are to be run on Linux and the
> remaining two are for FreeBSD, and the shared runners are limited to
> 10 jobs per build, how would GitLab decide which 10 jobs to ignore
> when testing a merge request?

I don't know what settings you're referring to, but there's no
hard limit on total job count on the shared runners. There is
some limit on the concurrent jobs for fairness between users, so
further jobs merely have to wait for the previous jobs to complete,
just as we see with Travis.

  https://docs.gitlab.com/ee/ci/runners/README.html#how-shared-runners-pick-jobs

> > We can selectively grant access to the custom runners to regular
> > contributors (whom we trust and would benefit from broader access), but
> > we canot make them freely available to everyone by default.
> 
> Can you please point me to the relevant documentation?

There are some docs here, but they're not comprehensive:

   https://docs.gitlab.com/ee/ci/runners/README.html

> > There's gitlab RFEs about making custom runners more flexible, while still
> > avoiding the security issues. For example, by allowing a project maintainer
> > to explicitly approve CI start for a merge request. No ETA for this though.
> 
> Do you have links to these RFEs handy?

I don't have the links handy any more, as this was a few weeks back, and
anyway we need to set things up with what exists today, not what might
exist in future. 


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 :|

[PATCH 2/7] libxl: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c   |  2 +-
 src/libxl/libxl_domain.c | 60 
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 957989e848..04636e8694 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15716,7 +15716,6 @@ virDomainVideoDefaultType(const virDomainDef *def)
 {
 switch ((virDomainVirtType)def->virtType) {
 case VIR_DOMAIN_VIRT_TEST:
-case VIR_DOMAIN_VIRT_XEN:
 if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
 def->os.type == VIR_DOMAIN_OSTYPE_LINUX)
 return VIR_DOMAIN_VIDEO_TYPE_XEN;
@@ -15737,6 +15736,7 @@ virDomainVideoDefaultType(const virDomainDef *def)
 return VIR_DOMAIN_VIDEO_TYPE_VGA;
 else
 return VIR_DOMAIN_VIDEO_TYPE_PARALLELS;
+case VIR_DOMAIN_VIRT_XEN:
 case VIR_DOMAIN_VIRT_BHYVE:
 case VIR_DOMAIN_VIRT_QEMU:
 case VIR_DOMAIN_VIRT_KQEMU:
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index e3da9f777d..cc53a765e1 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -315,31 +315,43 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN;
 }
 
-if (dev->type == VIR_DOMAIN_DEVICE_VIDEO && def->os.type == 
VIR_DOMAIN_OSTYPE_HVM) {
-int dm_type = libxlDomainGetEmulatorType(def);
-
-switch (dev->data.video->type) {
-case VIR_DOMAIN_VIDEO_TYPE_VGA:
-case VIR_DOMAIN_VIDEO_TYPE_XEN:
-if (dev->data.video->vram == 0) {
-if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
-dev->data.video->vram = 16 * 1024;
-else
-dev->data.video->vram = 8 * 1024;
-}
-break;
-case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
-if (dev->data.video->vram == 0) {
-if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
-dev->data.video->vram = 8 * 1024;
-else
-dev->data.video->vram = 4 * 1024;
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
+if (dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
+def->os.type == VIR_DOMAIN_OSTYPE_LINUX)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
+else if (ARCH_IS_PPC64(def->os.arch))
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+else
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+}
+
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
+int dm_type = libxlDomainGetEmulatorType(def);
+
+switch (dev->data.video->type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+if (dev->data.video->vram == 0) {
+if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
+dev->data.video->vram = 16 * 1024;
+else
+dev->data.video->vram = 8 * 1024;
+}
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+if (dev->data.video->vram == 0) {
+if (dm_type == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN)
+dev->data.video->vram = 8 * 1024;
+else
+dev->data.video->vram = 4 * 1024;
+}
+break;
+case VIR_DOMAIN_VIDEO_TYPE_QXL:
+if (dev->data.video->vram == 0)
+dev->data.video->vram = 128 * 1024;
+break;
 }
-break;
-case VIR_DOMAIN_VIDEO_TYPE_QXL:
-if (dev->data.video->vram == 0)
-dev->data.video->vram = 128 * 1024;
-break;
 }
 }
 
-- 
2.25.1

[PATCH 7/7] conf: domain_conf: remove virDomainVideoDefaultType

[Rafael Fonseca]

The logic has been moved to the individual drivers.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c   | 35 ---
 src/conf/domain_conf.h   |  1 -
 src/libvirt_private.syms |  1 -
 3 files changed, 4 insertions(+), 33 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 665bb10b27..27bc5a797b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15711,32 +15711,6 @@ virDomainVideoDefaultRAM(const virDomainDef *def,
 }
 
 
-int
-virDomainVideoDefaultType(const virDomainDef *def)
-{
-switch ((virDomainVirtType)def->virtType) {
-case VIR_DOMAIN_VIRT_VBOX:
-case VIR_DOMAIN_VIRT_TEST:
-case VIR_DOMAIN_VIRT_VMWARE:
-case VIR_DOMAIN_VIRT_VZ:
-case VIR_DOMAIN_VIRT_PARALLELS:
-case VIR_DOMAIN_VIRT_XEN:
-case VIR_DOMAIN_VIRT_BHYVE:
-case VIR_DOMAIN_VIRT_QEMU:
-case VIR_DOMAIN_VIRT_KQEMU:
-case VIR_DOMAIN_VIRT_KVM:
-case VIR_DOMAIN_VIRT_LXC:
-case VIR_DOMAIN_VIRT_UML:
-case VIR_DOMAIN_VIRT_OPENVZ:
-case VIR_DOMAIN_VIRT_HYPERV:
-case VIR_DOMAIN_VIRT_PHYP:
-case VIR_DOMAIN_VIRT_NONE:
-case VIR_DOMAIN_VIRT_LAST:
-default:
-return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
-}
-}
-
 static virDomainVideoAccelDefPtr
 virDomainVideoAccelDefParseXML(xmlNodePtr node)
 {
@@ -15854,7 +15828,6 @@ static virDomainVideoDefPtr
 virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
   xmlNodePtr node,
   xmlXPathContextPtr ctxt,
-  const virDomainDef *dom,
   unsigned int flags)
 {
 virDomainVideoDefPtr def;
@@ -15925,7 +15898,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
 goto error;
 }
 } else {
-def->type = virDomainVideoDefaultType(dom);
+def->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
 }
 
 if (driver_name) {
@@ -16871,7 +16844,7 @@ virDomainDeviceDefParse(const char *xmlStr,
 break;
 case VIR_DOMAIN_DEVICE_VIDEO:
 if (!(dev->data.video = virDomainVideoDefParseXML(xmlopt, node,
-  ctxt, def, flags)))
+  ctxt, flags)))
 return NULL;
 break;
 case VIR_DOMAIN_DEVICE_HOSTDEV:
@@ -21633,7 +21606,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 ssize_t insertAt = -1;
 
 if (!(video = virDomainVideoDefParseXML(xmlopt, nodes[i],
-ctxt, def, flags)))
+ctxt, flags)))
 goto error;
 
 if (video->primary) {
@@ -24314,7 +24287,7 @@ virDomainDefAddImplicitVideo(virDomainDefPtr def, 
virDomainXMLOptionPtr xmlopt)
 
 if (!(video = virDomainVideoDefNew(xmlopt)))
 goto cleanup;
-video->type = virDomainVideoDefaultType(def);
+video->type = VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
 if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, video) < 0)
 goto cleanup;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b7c31eb62f..33875d942f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3364,7 +3364,6 @@ int virDomainFSInsert(virDomainDefPtr def, 
virDomainFSDefPtr fs);
 int virDomainFSIndexByName(virDomainDefPtr def, const char *name);
 virDomainFSDefPtr virDomainFSRemove(virDomainDefPtr def, size_t i);
 
-int virDomainVideoDefaultType(const virDomainDef *def);
 unsigned int virDomainVideoDefaultRAM(const virDomainDef *def,
   const virDomainVideoType type);
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c9f0da2bf9..3f032c7963 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -633,7 +633,6 @@ virDomainTPMModelTypeFromString;
 virDomainTPMModelTypeToString;
 virDomainUSBDeviceDefForeach;
 virDomainVideoDefaultRAM;
-virDomainVideoDefaultType;
 virDomainVideoDefClear;
 virDomainVideoDefFree;
 virDomainVideoDefNew;
-- 
2.25.1

[PATCH 5/7] test: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c | 10 +-
 src/test/test_driver.c | 23 +++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 53bc791e10..53fd13e80f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15715,18 +15715,10 @@ int
 virDomainVideoDefaultType(const virDomainDef *def)
 {
 switch ((virDomainVirtType)def->virtType) {
-case VIR_DOMAIN_VIRT_TEST:
-if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
-def->os.type == VIR_DOMAIN_OSTYPE_LINUX)
-return VIR_DOMAIN_VIDEO_TYPE_XEN;
-else if (ARCH_IS_PPC64(def->os.arch))
-return VIR_DOMAIN_VIDEO_TYPE_VGA;
-else
-return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
-
 case VIR_DOMAIN_VIRT_VBOX:
 return VIR_DOMAIN_VIDEO_TYPE_VBOX;
 
+case VIR_DOMAIN_VIRT_TEST:
 case VIR_DOMAIN_VIRT_VMWARE:
 case VIR_DOMAIN_VIRT_VZ:
 case VIR_DOMAIN_VIRT_PARALLELS:
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 73fe1ad6ce..7759847c2d 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -407,6 +407,28 @@ testDomainObjPrivateAlloc(void *opaque)
 }
 
 
+static int
+testDomainDevicesDefPostParse(virDomainDeviceDefPtr dev G_GNUC_UNUSED,
+  const virDomainDef *def G_GNUC_UNUSED,
+  unsigned int parseFlags G_GNUC_UNUSED,
+  void *opaque G_GNUC_UNUSED,
+  void *parseOpaque G_GNUC_UNUSED)
+{
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+if (def->os.type == VIR_DOMAIN_OSTYPE_XEN ||
+def->os.type == VIR_DOMAIN_OSTYPE_LINUX)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_XEN;
+else if (ARCH_IS_PPC64(def->os.arch))
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+else
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+}
+
+return 0;
+}
+
+
 static void
 testDomainObjPrivateFree(void *data)
 {
@@ -431,6 +453,7 @@ testDriverNew(void)
 VIR_DOMAIN_DEF_FEATURE_USER_ALIAS |
 VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT |
 VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING,
+.devicesPostParseCallback = testDomainDevicesDefPostParse,
 .defArch = VIR_ARCH_I686,
 };
 virDomainXMLPrivateDataCallbacks privatecb = {
-- 
2.25.1

[PATCH 3/7] vz: openvz: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c   | 4 
 src/openvz/openvz_conf.c | 8 
 src/vz/vz_driver.c   | 8 
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 04636e8694..e6a3500b7a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15732,10 +15732,6 @@ virDomainVideoDefaultType(const virDomainDef *def)
 
 case VIR_DOMAIN_VIRT_VZ:
 case VIR_DOMAIN_VIRT_PARALLELS:
-if (def->os.type == VIR_DOMAIN_OSTYPE_HVM)
-return VIR_DOMAIN_VIDEO_TYPE_VGA;
-else
-return VIR_DOMAIN_VIDEO_TYPE_PARALLELS;
 case VIR_DOMAIN_VIRT_XEN:
 case VIR_DOMAIN_VIRT_BHYVE:
 case VIR_DOMAIN_VIRT_QEMU:
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 1d60afae93..78547b8b28 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -1117,6 +1117,14 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 return -1;
 }
 
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+else
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_PARALLELS;
+}
+
 return 0;
 }
 
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 6605247dd9..d882b91def 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -281,6 +281,14 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 def->os.type == VIR_DOMAIN_OSTYPE_HVM)
 dev->data.net->model = VIR_DOMAIN_NET_MODEL_E1000;
 
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
+if (def->os.type == VIR_DOMAIN_OSTYPE_HVM)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
+else
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_PARALLELS;
+}
+
 return 0;
 }
 
-- 
2.25.1

[PATCH 4/7] vmx: vmware: move video default logic to driver

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code.

Signed-off-by: Rafael Fonseca 
---
 src/conf/domain_conf.c | 2 --
 src/vmware/vmware_driver.c | 4 
 src/vmx/vmx.c  | 4 
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e6a3500b7a..53bc791e10 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15728,8 +15728,6 @@ virDomainVideoDefaultType(const virDomainDef *def)
 return VIR_DOMAIN_VIDEO_TYPE_VBOX;
 
 case VIR_DOMAIN_VIRT_VMWARE:
-return VIR_DOMAIN_VIDEO_TYPE_VMVGA;
-
 case VIR_DOMAIN_VIRT_VZ:
 case VIR_DOMAIN_VIRT_PARALLELS:
 case VIR_DOMAIN_VIRT_XEN:
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c
index 32c81b13a0..d5dd6e4f5e 100644
--- a/src/vmware/vmware_driver.c
+++ b/src/vmware/vmware_driver.c
@@ -137,6 +137,10 @@ vmwareDomainDeviceDefPostParse(virDomainDeviceDefPtr dev 
G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED,
void *parseOpaque G_GNUC_UNUSED)
 {
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VMVGA;
+
 return 0;
 }
 
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 6c6ef7acf3..b1fd1181eb 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -548,6 +548,10 @@ virVMXDomainDevicesDefPostParse(virDomainDeviceDefPtr dev 
G_GNUC_UNUSED,
 void *opaque G_GNUC_UNUSED,
 void *parseOpaque G_GNUC_UNUSED)
 {
+if (dev->type == VIR_DOMAIN_DEVICE_VIDEO &&
+dev->data.video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT)
+dev->data.video->type = VIR_DOMAIN_VIDEO_TYPE_VMVGA;
+
 return 0;
 }
 
-- 
2.25.1

[PATCH 0/7] Move video default logic to individual drivers

[Rafael Fonseca]

The logic setting a device default should be in the post parse function
of individual driver code, not in `src/conf/domain_conf.c`.

Rafael Fonseca (7):
  bhyve: move video default logic to driver
  libxl: move video default logic to driver
  vz: openvz: move video default logic to driver
  vmx: vmware: move video default logic to driver
  test: move video default logic to driver
  vbox: move video default logic to driver
  conf: domain_conf: remove virDomainVideoDefaultType

 src/bhyve/bhyve_domain.c   |  5 
 src/conf/domain_conf.c | 52 +++--
 src/conf/domain_conf.h |  1 -
 src/libvirt_private.syms   |  1 -
 src/libxl/libxl_domain.c   | 60 +++---
 src/openvz/openvz_conf.c   |  8 +
 src/test/test_driver.c | 23 +++
 src/vbox/vbox_common.c | 16 ++
 src/vmware/vmware_driver.c |  4 +++
 src/vmx/vmx.c  |  4 +++
 src/vz/vz_driver.c |  8 +
 11 files changed, 108 insertions(+), 74 deletions(-)

-- 
2.25.1

Re: [libvirt PATCH 4/5] gitlab: add several native CI jobs

[Andrea Bolognani]

On Mon, 2020-03-23 at 15:16 +, Daniel P. Berrangé wrote:
> On Mon, Mar 23, 2020 at 04:00:17PM +0100, Andrea Bolognani wrote:
> > Since we're going to need a FreeBSD machine configured as a GitLab
> > runner, wouldn't it make sense for us to also create one or more
> > Linux machines that would run jobs on behalf of GitLab CI? That way
> > we'd remove the limitation on the number of containers we can have
> > running at the same time, and still have everything controlled by
> > GitLab.
> 
> It is bit more complicated. Custom GitLab runners are associated with
> a project (and its git repos). Those runners will be used for any CI jobs
> run in the context of repositories owned by that project.  Obviously this
> covers post-merge build jobs, or regularly scheduled build jobs.
> 
> It gets more interesting with merge requests though, because in a normal
> project, the merge request is not being submmitted from a branch on the
> main repo, instead it is submitted from a branch on the user's forked
> repo copy. As such the CI jobs for the merge request run in the contxt
> of the forked repo and do not have access to the customer runners owned
> by the project.

Where is this documented? I've been looking through the gitlab-runner
documentation but I haven't been able to find anything about this.

> This is a intentional security restriction to prevent denial of service
> from arbitrary users from submitting (malicious) pull requests that
> trigger builds on 3rd party runners.

It should be possible to prevent a DoS by configuring a limit on the
number of concurrent jobs that are allowed for the runner (see [1]),
so this looks like a moot point.

As for running the build jobs securely, the Docker executor should
take care of that when it comes to Linux; for FreeBSD, I think we'd
have to use a custom executor that drops privileges instead, because
Docker is not a possibility there.

> Thus, we want to maximise the amount of build coverage we get on the
> shared runners, as this is what merge requests will be using, and what
> contributors will be using for tests before submitting code for review.
> 
> We'll have custom runners for providing coverage of platforms where
> containers aren't viable (non-Linux), as well as providing additional
> capacity.

I don't understand how this would work. If the CI configuration
contains say 20 jobs, 18 of which are to be run on Linux and the
remaining two are for FreeBSD, and the shared runners are limited to
10 jobs per build, how would GitLab decide which 10 jobs to ignore
when testing a merge request?

> We can selectively grant access to the custom runners to regular
> contributors (whom we trust and would benefit from broader access), but
> we canot make them freely available to everyone by default.

Can you please point me to the relevant documentation?

> There's gitlab RFEs about making custom runners more flexible, while still
> avoiding the security issues. For example, by allowing a project maintainer
> to explicitly approve CI start for a merge request. No ETA for this though.

Do you have links to these RFEs handy?


[1] 
https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-runners-section
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v4 0/2] introduction of migration_version attribute for VFIO live migration

[Alex Williamson]

On Tue, 24 Mar 2020 09:23:31 +
"Dr. David Alan Gilbert"  wrote:

> * Yan Zhao (yan.y.z...@intel.com) wrote:
> > On Tue, Mar 24, 2020 at 05:29:59AM +0800, Alex Williamson wrote:  
> > > On Mon, 3 Jun 2019 20:34:22 -0400
> > > Yan Zhao  wrote:
> > >   
> > > > On Tue, Jun 04, 2019 at 03:29:32AM +0800, Alex Williamson wrote:  
> > > > > On Thu, 30 May 2019 20:44:38 -0400
> > > > > Yan Zhao  wrote:
> > > > > 
> > > > > > This patchset introduces a migration_version attribute under sysfs 
> > > > > > of VFIO
> > > > > > Mediated devices.
> > > > > > 
> > > > > > This migration_version attribute is used to check migration 
> > > > > > compatibility
> > > > > > between two mdev devices of the same mdev type.
> > > > > > 
> > > > > > Patch 1 defines migration_version attribute in
> > > > > > Documentation/vfio-mediated-device.txt
> > > > > > 
> > > > > > Patch 2 uses GVT as an example to show how to expose 
> > > > > > migration_version
> > > > > > attribute and check migration compatibility in vendor driver.
> > > > > 
> > > > > Thanks for iterating through this, it looks like we've settled on
> > > > > something reasonable, but now what?  This is one piece of the puzzle 
> > > > > to
> > > > > supporting mdev migration, but I don't think it makes sense to commit
> > > > > this upstream on its own without also defining the remainder of how we
> > > > > actually do migration, preferably with more than one working
> > > > > implementation and at least prototyped, if not final, QEMU support.  I
> > > > > hope that was the intent, and maybe it's now time to look at the next
> > > > > piece of the puzzle.  Thanks,
> > > > > 
> > > > > Alex
> > > > 
> > > > Got it. 
> > > > Also thank you and all for discussing and guiding all along:)
> > > > We'll move to the next episode now.  
> > > 
> > > Hi Yan,
> > > 
> > > As we're hopefully moving towards a migration API, would it make sense
> > > to refresh this series at the same time?  I think we're still expecting
> > > a vendor driver implementing Kirti's migration API to also implement
> > > this sysfs interface for compatibility verification.  Thanks,
> > >  
> > Hi Alex
> > Got it!
> > Thanks for reminding of this. And as now we have vfio-pci implementing
> > vendor ops to allow live migration of pass-through devices, is it
> > necessary to implement similar sysfs node for those devices?
> > or do you think just PCI IDs of those devices are enough for libvirt to
> > know device compatibility ?  
> 
> Wasn't the problem that we'd have to know how to check for things like:
>   a) Whether different firmware versions in the device were actually
> compatible
>   b) Whether minor hardware differences were compatible - e.g. some
> hardware might let you migrate to the next version of hardware up.

Yes, minor changes in hardware or firmware that may not be represented
in the device ID or hardware revision.  Also the version is as much for
indicating the compatibility of the vendor defined migration protocol
as it is for the hardware itself.  I certainly wouldn't be so bold as
to create a protocol that is guaranteed compatible forever.  We'll need
to expose the same sysfs attribute in some standard location for
non-mdev devices.  I assume vfio-pci would provide the vendor ops some
mechanism to expose these in a standard namespace of sysfs attributes
under the device itself.  Perhaps that indicates we need to link the
mdev type version under the mdev device as well to make this
transparent to userspace tools like libvirt.  Thanks,

Alex

Re: [PATCH] docs: fix typo in domcaps host-model CPU description

[Jim Fehlig]

On 3/24/20 3:44 AM, Jiri Denemark wrote:

On Mon, Mar 23, 2020 at 15:47:49 -0600, Jim Fehlig wrote:

The domain capabilities documentation contains a small but confusing
error in the host-model CPU description, referencing the element 
instead of . Fix this small typo.

Signed-off-by: Jim Fehlig 
---

I only found this small typo (well, I'm pretty sure it's a typo :-)) by
tring to understand a more confusing observation. On a machine where
capabilities reports CascaseLake-Server, domcapabilities reports

 
   Cascadelake-Server
   Intel
   
   
   
   
   
   
   
   
   
   
 
 
   ...
   Cascadelake-Server
 

So using host-model will result in a CascadeLake-Server CPU, but it
is not supported when specifying a custom CPU? Interestingly, I see
something similar from domcapabilities on machine where capabilities
reports Skylake-Server-IBRS

 
   Cascadelake-Server
   Intel
   
   
   
   
   
   
   
   
   
   
   
 
 
   ...
   Skylake-Server-IBRS
   Skylake-Server
   Cascadelake-Server
 

This seems contradictory to me but perhaps I'm overlooking something?


The usable=yse|no attribute says whether a given CPU model can be
provided to a guest without any modification. That is whether you can
use
 
   Cascadelake-Server
 

When usable='no', QEMU will need to disable some of the features that
are part of the Cascadelake-Server CPU model. In other words, the CPU
model is not usable as is.

On the other hand, the  element in domain
capabilities describes the CPU used as a host-model CPU by adding or
removing some specific features. In other words, while using the simple
CPU definition mentioned above would cause QEMU to drop some features,
using a more verbose

   
 Cascadelake-Server
 Intel
 
 
 
 
 
 
 
 
 
 
 
   

explicitly asks QEMU to disable pku and avx512vnni features and thus
QEMU will not have to disable anything.


Thanks for the detailed explanation!


The situation on the first machine is a bit strange as there are no
features disabled in host-model CPU definition, which makes it unclear
why QEMU reports Cascadelake-Server as unusable (QEMU reports the
reason, but we don't do so yet).


It's not clear to me even after looking at the output of cpu-gather.


Anyway, would you mind running the tests/cputestdata/cpu-gather.sh
script on both machines (make sure to install qemu, python3, and cpuid
packages first) and send us the output so that we can check the CPU
models are properly detected?


Attached.




  docs/formatdomaincaps.html.in | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index 66e758501b..7fd1f91f73 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -232,7 +232,7 @@
host-model

  If host-model is supported by the hypervisor, the
-mode describes the guest CPU which will be used when
+model describes the guest CPU which will be used when
  starting a domain with host-model CPU. The hypervisor
  specifics (such as unsupported CPU models or features, machine type,
  etc.) may be accounted for in this guest CPU specification and thus


As strange as it seems, there's no typo. The  element describes
just a small part of the whole CPU. There are additional  and
 elements which also belong to the CPU definition. All these
elements are children of the  element and thus the  element
describes the guest CPU.


Ah, yes, now it makes sense. Thanks again!

Regards,
Jim
model name  : Intel(R) Xeon(R) Platinum 8260L CPU @ 2.40GHz
CPU:
   0x 0x00: eax=0x0016 ebx=0x756e6547 ecx=0x6c65746e edx=0x49656e69
   0x0001 0x00: eax=0x00050656 ebx=0x06400800 ecx=0x7ffefbff edx=0xbfebfbff
   0x0002 0x00: eax=0x76036301 ebx=0x00f0b6ff ecx=0x edx=0x00c3
   0x0003 0x00: eax=0x ebx=0x ecx=0x edx=0x
   0x0004 0x00: eax=0x7c004121 ebx=0x01c0003f ecx=0x003f edx=0x
   0x0004 0x01: eax=0x7c004122 ebx=0x01c0003f ecx=0x003f edx=0x
   0x0004 0x02: eax=0x7c004143 ebx=0x03c0003f ecx=0x03ff edx=0x
   0x0004 0x03: eax=0x7c0fc163 ebx=0x0280003f ecx=0xcfff edx=0x0005
   0x0005 0x00: eax=0x0040 ebx=0x0040 ecx=0x0003 edx=0x2020
   0x0006 0x00: eax=0x0ef7 ebx=0x0002 ecx=0x0009 edx=0x
   0x0007 0x00: eax=0x ebx=0xd39b ecx=0x0818 edx=0xbc000400
   0x0008 0x00: eax=0x ebx=0x ecx=0x edx=0x
   0x0009 0x00: eax=0x ebx=0x ecx=0x edx=0x
   0x000a 0x00: eax=0x07300804 ebx=0x ecx=0x 

Re: [PATCH v1 1/4] qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting

[Andrea Bolognani]

On Tue, 2020-03-24 at 10:22 -0300, Daniel Henrique Barboza wrote:
> On 3/23/20 2:45 PM, Andrea Bolognani wrote:
> > On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote:
> > > And while we're at it, something that just occurred to me, I'll also gate 
> > > the ppc64
> > > only capabilities as well in a new patch.
> > 
> > Yeah, that makes sense too. We're probably never going to get to a
> > point where these checks are completely accurate, but any improvement
> > can only be a welcome one :)
> 
> Just realized that we're already doing that here:
> 
>  case VIR_DOMAIN_FEATURE_HPT:
>  case VIR_DOMAIN_FEATURE_HTM:
>  case VIR_DOMAIN_FEATURE_NESTED_HV:
>  case VIR_DOMAIN_FEATURE_CCF_ASSIST:
>  if (qemuDomainDefValidatePSeriesFeature(def, qemuCaps, i) < 0)
>  return -1;
>  break;

Right! I had forgotten about writing that validation logic O:-)

> There is no need for an extra patch to handle it.

Nope, we're good :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v1 3/4] qemu_domain.c: do not launch ppc64 guests with 'pmu' setting

[Andrea Bolognani]

On Tue, 2020-03-24 at 11:05 -0300, Daniel Henrique Barboza wrote:
> On 3/24/20 11:00 AM, Andrea Bolognani wrote:
> > On Tue, 2020-03-24 at 09:27 -0300, Daniel Henrique Barboza wrote:
> > > For the sake of completeness, I'll also mention that we can simply allow 
> > >  to be
> > > declared in the XML, handling the  inside the QEMU 
> > > driver to not add the
> > > bogus '.pmu' parameter for QEMU ppc64, forbid  to be 
> > > declared, and
> > > nothing else. No auto-generation of XML indicating that the guest will 
> > > support a PMU.
> > 
> > Looking again at how other architectures, specifically x86 and ARM,
> > handle this, the PMU is generally enabled by default without this
> > fact being reflected in the domain XML; the user can then go ahead
> > and specifically ask for it to be turned on or off, at which point
> > libvirt will add the relevant bits to the QEMU command line.
> > 
> > This is basically the second behavior you're describing above, and
> > I think it would be perfectly fine if that's the one we would adopt
> > for ppc64.
> 
> I guess I'll roll with this one then. I will allow  to be
> declared in the XML without breaking QEMU. For  I'll
> throw an CONFIG_UNSUPPORTED error mentioning that PMU can't be turned off
> for ppc64.

Sounds good.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v1 3/4] qemu_domain.c: do not launch ppc64 guests with 'pmu' setting

[Daniel Henrique Barboza]

On 3/24/20 11:00 AM, Andrea Bolognani wrote:

On Tue, 2020-03-24 at 09:27 -0300, Daniel Henrique Barboza wrote:

Andrea, I see that the behavior of the 'panic' device in the ppc64 XML is to
always add a:


[...]



For the sake of completeness, I'll also mention that we can simply allow  
to be
declared in the XML, handling the  inside the QEMU driver to 
not add the
bogus '.pmu' parameter for QEMU ppc64, forbid  to be 
declared, and
nothing else. No auto-generation of XML indicating that the guest will support 
a PMU.


Looking again at how other architectures, specifically x86 and ARM,
handle this, the PMU is generally enabled by default without this
fact being reflected in the domain XML; the user can then go ahead
and specifically ask for it to be turned on or off, at which point
libvirt will add the relevant bits to the QEMU command line.

This is basically the second behavior you're describing above, and
I think it would be perfectly fine if that's the one we would adopt
for ppc64.



I guess I'll roll with this one then. I will allow  to be
declared in the XML without breaking QEMU. For  I'll
throw an CONFIG_UNSUPPORTED error mentioning that PMU can't be turned off
for ppc64.


Thanks,


DHB

Re: [PATCH v1 3/4] qemu_domain.c: do not launch ppc64 guests with 'pmu' setting

[Andrea Bolognani]

On Tue, 2020-03-24 at 09:27 -0300, Daniel Henrique Barboza wrote:
> Andrea, I see that the behavior of the 'panic' device in the ppc64 XML is to
> always add a:
> 
>  
> 
> Even if I remove it Libvirt will automatically added it back again. How do you
> suggest we handle this PMU?
> 
> We can copy what 'panic' is doing, adding a
> 
>  

This is definitely not the right approach - we should use the
existing interface rather than try to come up with a new one.

> In the XML just like 'panic' does. Or we can add the 'pmu' feature:
> 
>  
>
>  
> 
> PMU can't be left without a state ATM, thus this would translate to:
> 
>  
>
>  

This is what we should do.

> This would require to add the whole  element if none is present as 
> well. Both
> requires some code tweaking here and there. I am not sure which one is 
> better, although
> the first option would require us to handle the  XML feature anyway 
> since we can't
> have both, so there's that.
> 
> For the sake of completeness, I'll also mention that we can simply allow 
>  to be
> declared in the XML, handling the  inside the QEMU driver to 
> not add the
> bogus '.pmu' parameter for QEMU ppc64, forbid  to be 
> declared, and
> nothing else. No auto-generation of XML indicating that the guest will 
> support a PMU.

Looking again at how other architectures, specifically x86 and ARM,
handle this, the PMU is generally enabled by default without this
fact being reflected in the domain XML; the user can then go ahead
and specifically ask for it to be turned on or off, at which point
libvirt will add the relevant bits to the QEMU command line.

This is basically the second behavior you're describing above, and
I think it would be perfectly fine if that's the one we would adopt
for ppc64.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] qemu_monitor_text.c: Use g_autofree

[Michal Prívozník]

On 24. 3. 2020 13:51, Daniel Henrique Barboza wrote:
> 
> 
> On 3/24/20 9:44 AM, Seeteena Thoufeek wrote:
>> Signed-off-by: Seeteena Thoufeek 
>> ---
> 
> 
> Reviewed-by: Daniel Henrique Barboza 
> 

Pushed now.

Michal

Re: [PATCH] commandtest: Fix test28 error detection

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 01:54:35PM +0100, Michal Privoznik wrote:
> As a part of c799d150d5e9dae I've introduced a test case that
> tests whether passing error object between processes works. The
> test spawns a child which reports a system error, parent process
> then reads the error and compares with expected output. Problem
> with this approach is that error message contains stringified
> errno which is not portable. FreeBSD has generally different
> messages than Linux. Therefore, use g_strerror() to do the errno
> to string translation for us.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  tests/commandtest.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v1 1/4] qemu_domain.c: do not launch ppc64 guests with APIC-EOI setting

[Daniel Henrique Barboza]

On 3/23/20 2:45 PM, Andrea Bolognani wrote:

On Mon, 2020-03-23 at 14:21 -0300, Daniel Henrique Barboza wrote:

On 3/23/20 2:01 PM, Andrea Bolognani wrote:

This is

https://bugzilla.redhat.com/show_bug.cgi?id=1236440

Please include the Bugzilla URL for other patches in the series
as well, if applicable.


Sure. I didn't include the link because it can't be opened unless you have a RH 
bugzilla
account and I wasn't sure I could add it here.


Oh, I did not realize that bug was private. That's very silly. I'll
ask for it to be made public.


And while we're at it, something that just occurred to me, I'll also gate the 
ppc64
only capabilities as well in a new patch.


Yeah, that makes sense too. We're probably never going to get to a
point where these checks are completely accurate, but any improvement
can only be a welcome one :)


Just realized that we're already doing that here:

case VIR_DOMAIN_FEATURE_HPT:
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
case VIR_DOMAIN_FEATURE_CCF_ASSIST:
if (qemuDomainDefValidatePSeriesFeature(def, qemuCaps, i) < 0)
return -1;
break;


qemuDomainDefValidatePseriesFeature() filters if the arch is ppc64:

static int
qemuDomainDefValidatePSeriesFeature(const virDomainDef *def,
virQEMUCapsPtr qemuCaps,
int feature)
{
const char *str;

if (def->features[feature] != VIR_TRISTATE_SWITCH_ABSENT &&
!qemuDomainIsPSeries(def)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   _("The '%s' feature is not supported for "
 "architecture '%s' or machine type '%s'"),
   virDomainFeatureTypeToString(feature),
   virArchToString(def->os.arch),
   def->os.machine);
return -1;
}
(...)


There is no need for an extra patch to handle it.



Thanks,


DHB

Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper

[Michal Prívozník]

On 24. 3. 2020 11:52, Pino Toscano wrote:
> On Tuesday, 24 March 2020 11:48:17 CET Daniel P. Berrangé wrote:

>> IIUC the problem here is that the STREQ check is assuming that the
>> ENODATA errno results in the string "No data available". The strings
>> are not standardized by POSIX AFAIK, so C libraries can use any reasonable
>> text for them. So this check is not portable.
>>
>> Perhaps its enough to use  STRPREFIX(err->str2, "some error message:") ?
> 
> Or maybe use g_strerror to get the error message of ENODATA, and
> compare it to the actual message got in the test.
> 

Ah indeed, that is the problem. I've went with Pino's suggestion and
proposed a patch for it.

Michal

[PATCH] commandtest: Fix test28 error detection

[Michal Privoznik]

As a part of c799d150d5e9dae I've introduced a test case that
tests whether passing error object between processes works. The
test spawns a child which reports a system error, parent process
then reads the error and compares with expected output. Problem
with this approach is that error message contains stringified
errno which is not portable. FreeBSD has generally different
messages than Linux. Therefore, use g_strerror() to do the errno
to string translation for us.

Signed-off-by: Michal Privoznik 
---
 tests/commandtest.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/commandtest.c b/tests/commandtest.c
index f4a2c67c05..d5092b7dd0 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -1272,6 +1272,7 @@ test28(const void *unused G_GNUC_UNUSED)
 /* Not strictly a virCommand test, but this is the easiest place
  * to test this lower-level interface. */
 virErrorPtr err;
+g_autofree char *msg = g_strdup_printf("some error message: %s", 
g_strerror(ENODATA));
 
 if (virProcessRunInFork(test28Callback, NULL) != -1) {
 fprintf(stderr, "virProcessRunInFork did not fail\n");
@@ -1285,10 +1286,10 @@ test28(const void *unused G_GNUC_UNUSED)
 
 if (!(err->code == VIR_ERR_SYSTEM_ERROR &&
   err->domain == 0 &&
-  STREQ(err->message, "some error message: No data available") &&
+  STREQ(err->message, msg) &&
   err->level == VIR_ERR_ERROR &&
   STREQ(err->str1, "%s") &&
-  STREQ(err->str2, "some error message: No data available") &&
+  STREQ(err->str2, msg) &&
   err->int1 == ENODATA &&
   err->int2 == -1)) {
 fprintf(stderr, "Unexpected error object\n");
-- 
2.24.1

Re: [PATCH] qemu_monitor_text.c: Use g_autofree

[Daniel Henrique Barboza]

On 3/24/20 9:44 AM, Seeteena Thoufeek wrote:

Signed-off-by: Seeteena Thoufeek 
---



Reviewed-by: Daniel Henrique Barboza 

[PATCH] qemu_monitor_text.c: Use g_autofree

[Seeteena Thoufeek]

Signed-off-by: Seeteena Thoufeek 
---
 src/qemu/qemu_monitor_text.c | 85 
 1 file changed, 30 insertions(+), 55 deletions(-)

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 9135a11..5a890af 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -36,27 +36,26 @@ VIR_LOG_INIT("qemu.qemu_monitor_text");
 int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
 const char *drivestr)
 {
-char *cmd = NULL;
-char *reply = NULL;
-int ret = -1;
+g_autofree char *cmd = NULL;
+g_autofree char *reply = NULL;
 
 /* 'dummy' here is just a placeholder since there is no PCI
  * address required when attaching drives to a controller */
 cmd = g_strdup_printf("drive_add dummy %s", drivestr);
 
 if (qemuMonitorJSONHumanCommand(mon, cmd, ) < 0)
-goto cleanup;
+return -1;
 
 if (strstr(reply, "unknown command:")) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("drive hotplug is not supported"));
-goto cleanup;
+return -1;
 }
 
 if (strstr(reply, "could not open disk image")) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("open disk image file failed"));
-goto cleanup;
+return -1;
 }
 
 if (strstr(reply, "Could not open")) {
@@ -66,48 +65,41 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
 
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
reply);
-goto cleanup;
+return -1;
 }
 
 if (strstr(reply, "Image is not in")) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Incorrect disk format"));
-goto cleanup;
+return -1;
 }
 
 if (strstr(reply, "IOMMU") ||
 strstr(reply, "VFIO")) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
reply);
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-VIR_FREE(cmd);
-VIR_FREE(reply);
-return ret;
+return 0;
 }
 
 
 int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
 const char *drivestr)
 {
-char *cmd = NULL;
-char *reply = NULL;
-int ret = -1;
+g_autofree char *cmd = NULL;
+g_autofree char *reply = NULL;
 
 cmd = g_strdup_printf("drive_del %s", drivestr);
 
 if (qemuMonitorJSONHumanCommand(mon, cmd, ) < 0)
-goto cleanup;
+return -1;
 
 if (strstr(reply, "unknown command:")) {
 VIR_ERROR(_("deleting drive is not supported.  "
 "This may leak data if disk is reassigned"));
-ret = 1;
-goto cleanup;
+return 1;
 
 /* (qemu) drive_del wark
  * Device 'wark' not found */
@@ -117,15 +109,10 @@ int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
 } else if (STRNEQ(reply, "")) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("deleting %s drive failed: %s"), drivestr, reply);
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-VIR_FREE(cmd);
-VIR_FREE(reply);
-return ret;
+return 0;
 }
 
 int
@@ -159,28 +146,27 @@ qemuMonitorTextCreateSnapshot(qemuMonitorPtr mon,
 
 int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const char *name)
 {
-char *cmd = NULL;
-char *reply = NULL;
-int ret = -1;
+g_autofree char *cmd = NULL;
+g_autofree char *reply = NULL;
 
 cmd = g_strdup_printf("loadvm \"%s\"", name);
 
 if (qemuMonitorJSONHumanCommand(mon, cmd, ))
-goto cleanup;
+return -1;
 
 if (strstr(reply, "No block device supports snapshots")) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("this domain does not have a device to load 
snapshots"));
-goto cleanup;
+return -1;
 } else if (strstr(reply, "Could not find snapshot")) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("the snapshot '%s' does not exist, and was not 
loaded"),
name);
-goto cleanup;
+return -1;
 } else if (strstr(reply, "Snapshots not supported on device")) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("Failed to load snapshot: %s"), reply);
-goto cleanup;
+return -1;
 } else if (strstr(reply, "Could not open VM state file") ||
strstr(reply, "Error: ") ||
(strstr(reply, "Error") &&
@@ -188,46 +174,35 @@ int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const 
char *name)
  strstr(reply, "while activating snapshot on" {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to load snapshot: %s"), reply);
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-VIR_FREE(cmd);
-VIR_FREE(reply);
-return ret;
+   

Re: [PATCH v1 3/4] qemu_domain.c: do not launch ppc64 guests with 'pmu' setting

[Daniel Henrique Barboza]

On 3/23/20 9:34 PM, David Gibson wrote:

On Mon, Mar 23, 2020 at 06:28:34PM +0100, Andrea Bolognani wrote:

On Thu, 2020-03-19 at 18:44 -0300, Daniel Henrique Barboza wrote:

The Perfomance Monitoring Unit (PMU) feature is not available for
the Power architecture. The "" feature will always have a value
'on' or 'off' after saving the domain XML, and both will be rejected
by QEMU when launching. This is the error message for
"":


[...]


So it seems to me that, if anything, the PMU feature should be
treated like the  device, that is, automatically added to
pSeries guests if it's not present already.

David, what's your opinion on the matter?


You're correct.  The difference originates at the hardware level.  On
x86 the host always "owns" the PMU, and it requires explicit work in
KVM to make it available to the guest.  On POWER, the guest owns the
PMU.  I'm not sure if it's possible to disable guest access to the PMU
at all.  Even if it is, it must be some obscure bit in the HFCR or
somewhere which I don't believe we've wired up at all.

So for now, certainly, pmu is effectively always on.



Thanks for the info.

Andrea, I see that the behavior of the 'panic' device in the ppc64 XML is to
always add a:



Even if I remove it Libvirt will automatically added it back again. How do you
suggest we handle this PMU?

We can copy what 'panic' is doing, adding a



In the XML just like 'panic' does. Or we can add the 'pmu' feature:


  


PMU can't be left without a state ATM, thus this would translate to:


  



This would require to add the whole  element if none is present as 
well. Both
requires some code tweaking here and there. I am not sure which one is better, 
although
the first option would require us to handle the  XML feature anyway since 
we can't
have both, so there's that.

For the sake of completeness, I'll also mention that we can simply allow  
to be
declared in the XML, handling the  inside the QEMU driver to 
not add the
bogus '.pmu' parameter for QEMU ppc64, forbid  to be 
declared, and
nothing else. No auto-generation of XML indicating that the guest will support 
a PMU.



Thanks,


DHB

Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

[Michal Prívozník]

On 24. 3. 2020 13:12, Peter Krempa wrote:
> 
> Well, the issue is when the virStorageSource is created from the backing
> store rather than parsed from XML which is added later on.
> 

Should have a source post parse callback then? Anyway,

Reviewed-by: Michal Privoznik 

for whole series.

Michal

Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

[Peter Krempa]

On Tue, Mar 24, 2020 at 12:38:58 +0100, Michal Privoznik wrote:
> On 23. 3. 2020 19:11, Peter Krempa wrote:
> > VIR_TRISTATE_BOOL_ABSENT which maps to the 'default' string would not be
> > parsed back, so we shouldn't format it either.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/conf/domain_conf.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f1e4d33a8d..f6a225e4e6 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -24662,8 +24662,9 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf,
> >const virStorageSourceNVMeDef *nvme)
> >  {
> >  virBufferAddLit(attrBuf, " type='pci'");
> > -virBufferAsprintf(attrBuf, " managed='%s'",
> > -  virTristateBoolTypeToString(nvme->managed));
> > +if (nvme->managed != VIR_TRISTATE_BOOL_ABSENT)
> > +virBufferAsprintf(attrBuf, " managed='%s'",
> > +  virTristateBoolTypeToString(nvme->managed));
> >  virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespc);
> >  virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false);
> >  }
> > 
> 
> There is a postparse callback which deals with _ABSENT (see commit
> 8cd7196974d):
> 
> 
> static int
> virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
>   const virDomainDef *def,
>   virDomainXMLOptionPtr xmlopt)
> {
> ...
> if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
> if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
> disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
> }
> ...
> }
> 
> But maybe the callback is not called from tests where virStorageSource
> is parsed directly?

Well, the issue is when the virStorageSource is created from the backing
store rather than parsed from XML which is added later on.

Re: [PATCH RESEND 2/5] qemuProcessStartManagedPRDaemon: Don't pass -f pidfile to the daemon

[Marc-André Lureau]

Hi

On Mon, Mar 23, 2020 at 6:48 PM Michal Prívozník  wrote:
>
> On 23. 3. 2020 17:36, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Mar 23, 2020 at 5:16 PM Michal Privoznik  
> > wrote:
> >>
> >> Now, that our virCommandSetPidFile() is more intelligent we don't
> >> need to rely on the daemon to create and lock the pidfile and use
> >> virCommandSetPidFile() at the same time.
> >>
> >> Signed-off-by: Michal Privoznik 
> >
> > Nice, but doesn't this also fix a temporary regression introduced by
> > previous commit, now that pidfile is locked?
>
> Yeah, I haven't found a way to do this regression free.

Either squash them,

Or add a fat warning on the previous commit, and mention that you fix
it here too.

With that,
Reviewed-by: Marc-André Lureau 



-- 
Marc-André Lureau

Re: [PATCH 03/14] virDomainDiskSourceNVMeFormat: Format only valid 'managed' values

[Michal Prívozník]

On 23. 3. 2020 19:11, Peter Krempa wrote:
> VIR_TRISTATE_BOOL_ABSENT which maps to the 'default' string would not be
> parsed back, so we shouldn't format it either.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/conf/domain_conf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f1e4d33a8d..f6a225e4e6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -24662,8 +24662,9 @@ virDomainDiskSourceNVMeFormat(virBufferPtr attrBuf,
>const virStorageSourceNVMeDef *nvme)
>  {
>  virBufferAddLit(attrBuf, " type='pci'");
> -virBufferAsprintf(attrBuf, " managed='%s'",
> -  virTristateBoolTypeToString(nvme->managed));
> +if (nvme->managed != VIR_TRISTATE_BOOL_ABSENT)
> +virBufferAsprintf(attrBuf, " managed='%s'",
> +  virTristateBoolTypeToString(nvme->managed));
>  virBufferAsprintf(attrBuf, " namespace='%llu'", nvme->namespc);
>  virPCIDeviceAddressFormat(childBuf, nvme->pciAddr, false);
>  }
> 

There is a postparse callback which deals with _ABSENT (see commit
8cd7196974d):


static int
virDomainDiskDefPostParse(virDomainDiskDefPtr disk,
  const virDomainDef *def,
  virDomainXMLOptionPtr xmlopt)
{
...
if (disk->src->type == VIR_STORAGE_TYPE_NVME) {
if (disk->src->nvme->managed == VIR_TRISTATE_BOOL_ABSENT)
disk->src->nvme->managed = VIR_TRISTATE_BOOL_YES;
}
...
}

But maybe the callback is not called from tests where virStorageSource
is parsed directly?

Michal

Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper

[Pino Toscano]

On Tuesday, 24 March 2020 11:48:17 CET Daniel P. Berrangé wrote:
> On Wed, Mar 18, 2020 at 06:32:15PM +0100, Michal Privoznik wrote:
> > When running a function in a forked child, so far the only thing
> > we could report is exit status of the child and the error
> > message. However, it may be beneficial to the caller to know the
> > actual error that happened in the child.
> > 
> > Signed-off-by: Michal Privoznik 
> > ---
> >  build-aux/syntax-check.mk |  2 +-
> >  src/util/virprocess.c | 51 ---
> >  tests/commandtest.c   | 43 +
> >  3 files changed, 91 insertions(+), 5 deletions(-)
> > diff --git a/tests/commandtest.c b/tests/commandtest.c
> > index a64aa9ad33..f4a2c67c05 100644
> > --- a/tests/commandtest.c
> > +++ b/tests/commandtest.c
> > @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED)
> >  }
> >  
> >  
> > +static int
> > +test28Callback(pid_t pid G_GNUC_UNUSED,
> > +   void *opaque G_GNUC_UNUSED)
> > +{
> > +virReportSystemError(ENODATA, "%s", "some error message");
> > +return -1;
> > +}
> > +
> > +
> > +static int
> > +test28(const void *unused G_GNUC_UNUSED)
> > +{
> > +/* Not strictly a virCommand test, but this is the easiest place
> > + * to test this lower-level interface. */
> > +virErrorPtr err;
> > +
> > +if (virProcessRunInFork(test28Callback, NULL) != -1) {
> > +fprintf(stderr, "virProcessRunInFork did not fail\n");
> > +return -1;
> > +}
> > +
> > +if (!(err = virGetLastError())) {
> > +fprintf(stderr, "Expected error but got nothing\n");
> > +return -1;
> > +}
> > +
> > +if (!(err->code == VIR_ERR_SYSTEM_ERROR &&
> > +  err->domain == 0 &&
> > +  STREQ(err->message, "some error message: No data available") &&
> > +  err->level == VIR_ERR_ERROR &&
> > +  STREQ(err->str1, "%s") &&
> > +  STREQ(err->str2, "some error message: No data available") &&
> > +  err->int1 == ENODATA &&
> > +  err->int2 == -1)) {
> > +fprintf(stderr, "Unexpected error object\n");
> > +return -1;
> > +}
> 
> This new test fails on FreeBSD
> 
> $ VIR_TEST_DEBUG=1 VIR_TEST_RANGE=28 ./commandtest 
> TEST: commandtest
> 28) Command Exec test28 test  ... 
> Unexpected error object
> libvirt:  error : some error message: Input/output error
> FAILED
> 
> 
> IIUC the problem here is that the STREQ check is assuming that the
> ENODATA errno results in the string "No data available". The strings
> are not standardized by POSIX AFAIK, so C libraries can use any reasonable
> text for them. So this check is not portable.
> 
> Perhaps its enough to use  STRPREFIX(err->str2, "some error message:") ?

Or maybe use g_strerror to get the error message of ENODATA, and
compare it to the actual message got in the test.

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper

[Daniel P . Berrangé]

On Wed, Mar 18, 2020 at 06:32:15PM +0100, Michal Privoznik wrote:
> When running a function in a forked child, so far the only thing
> we could report is exit status of the child and the error
> message. However, it may be beneficial to the caller to know the
> actual error that happened in the child.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  build-aux/syntax-check.mk |  2 +-
>  src/util/virprocess.c | 51 ---
>  tests/commandtest.c   | 43 +
>  3 files changed, 91 insertions(+), 5 deletions(-)
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index a64aa9ad33..f4a2c67c05 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
> @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED)
>  }
>  
>  
> +static int
> +test28Callback(pid_t pid G_GNUC_UNUSED,
> +   void *opaque G_GNUC_UNUSED)
> +{
> +virReportSystemError(ENODATA, "%s", "some error message");
> +return -1;
> +}
> +
> +
> +static int
> +test28(const void *unused G_GNUC_UNUSED)
> +{
> +/* Not strictly a virCommand test, but this is the easiest place
> + * to test this lower-level interface. */
> +virErrorPtr err;
> +
> +if (virProcessRunInFork(test28Callback, NULL) != -1) {
> +fprintf(stderr, "virProcessRunInFork did not fail\n");
> +return -1;
> +}
> +
> +if (!(err = virGetLastError())) {
> +fprintf(stderr, "Expected error but got nothing\n");
> +return -1;
> +}
> +
> +if (!(err->code == VIR_ERR_SYSTEM_ERROR &&
> +  err->domain == 0 &&
> +  STREQ(err->message, "some error message: No data available") &&
> +  err->level == VIR_ERR_ERROR &&
> +  STREQ(err->str1, "%s") &&
> +  STREQ(err->str2, "some error message: No data available") &&
> +  err->int1 == ENODATA &&
> +  err->int2 == -1)) {
> +fprintf(stderr, "Unexpected error object\n");
> +return -1;
> +}

This new test fails on FreeBSD

$ VIR_TEST_DEBUG=1 VIR_TEST_RANGE=28 ./commandtest 
TEST: commandtest
28) Command Exec test28 test  ... 
Unexpected error object
libvirt:  error : some error message: Input/output error
FAILED


IIUC the problem here is that the STREQ check is assuming that the
ENODATA errno results in the string "No data available". The strings
are not standardized by POSIX AFAIK, so C libraries can use any reasonable
text for them. So this check is not portable.

Perhaps its enough to use  STRPREFIX(err->str2, "some error message:") ?


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH] util: virhostcpu: Fail when fetching CPU Stats for invalid cpu

[Michal Prívozník]

On 21. 2. 2020 19:10, Mauro S. M. Rodrigues wrote:
> virHostCPUGetStatsLinux walks through every cpu in /proc/stat until it
> finds cpu%cpuNum that matches with the requested cpu.
> If none is found it logs the error but it should return -1, instead of 0.
> Otherwise virsh nodecpustats --cpu  and API bindings
> don't fail properly, printing a blank line instead of an error message.
> 
> This patch also includes an additional test for virhostcputest to avoid
> this regression to happen again in the future.
> 
> Reported-by: Satheesh Rajendran 
> Signed-off-by: Mauro S. M. Rodrigues 
> ---
>  src/util/virhostcpu.c  | 2 +-
>  tests/virhostcputest.c | 9 ++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 81293eea8c..20c8d0ce6c 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -847,7 +847,7 @@ virHostCPUGetStatsLinux(FILE *procstat,
>  _("Invalid cpuNum in %s"),
>  __FUNCTION__);
>  
> -return 0;
> +return -1;
>  }

Oops, yes. This was introduced by 93af79fba3fd75a8df6b7ca608719dd97f9511a0.

>  
>  
> diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
> index 7865b61578..2f569d8bd4 100644
> --- a/tests/virhostcputest.c
> +++ b/tests/virhostcputest.c
> @@ -258,14 +258,17 @@ mymain(void)
>  if (virTestRun(nodeData[i].testName, linuxTestHostCPU, [i]) 
> != 0)
>  ret = -1;
>  
> -# define DO_TEST_CPU_STATS(name, ncpus) \
> +# define DO_TEST_CPU_STATS(name, ncpus, shouldFail) \
>  do { \
>  static struct nodeCPUStatsData data = { name, ncpus }; \
> -if (virTestRun("CPU stats " name, linuxTestNodeCPUStats, ) < 0) 
> \
> +if ((virTestRun("CPU stats " name, \
> + linuxTestNodeCPUStats, \
> + ) < 0) != shouldFail) \

We do it slightly differently. We pass shouldFail to the callback and
let it fix its retval so that virTestRun() interprets it properly.

>  ret = -1; \
>  } while (0)
>  
> -DO_TEST_CPU_STATS("24cpu", 24);
> +DO_TEST_CPU_STATS("24cpu", 24, false);
> +DO_TEST_CPU_STATS("24cpu", 25, true);
>  
>  return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> 

I'm fixing the test, adding the referenced commit into the commit
message and pushing.

Reviewed-by: Michal Privoznik 

Congratulations on your first libvirt contribution!

Michal

Re: Hot unplug disabling on pci-pci bridge

[Ani Sinha]

> On Mar 24, 2020, at 2:53 PM, Daniel P. Berrangé  wrote:
> 
> This is a patch against QEMU actually. 

Oops! Yes sorry, not sure what I was thinking. Yes this is Qemu. I will resend 
the email in Qemu-devel mailing list.

ani

Re: [PATCH] docs: fix typo in domcaps host-model CPU description

[Jiri Denemark]

On Mon, Mar 23, 2020 at 15:47:49 -0600, Jim Fehlig wrote:
> The domain capabilities documentation contains a small but confusing
> error in the host-model CPU description, referencing the element 
> instead of . Fix this small typo.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> I only found this small typo (well, I'm pretty sure it's a typo :-)) by
> tring to understand a more confusing observation. On a machine where
> capabilities reports CascaseLake-Server, domcapabilities reports
> 
> 
>   Cascadelake-Server
>   Intel
>   
>   
>   
>   
>   
>   
>   
>   
>   
>   
> 
> 
>   ...
>   Cascadelake-Server
> 
> 
> So using host-model will result in a CascadeLake-Server CPU, but it
> is not supported when specifying a custom CPU? Interestingly, I see
> something similar from domcapabilities on machine where capabilities
> reports Skylake-Server-IBRS
> 
> 
>   Cascadelake-Server
>   Intel
>   
>   
>   
>   
>   
>   
>   
>   
>   
>   
>   
> 
> 
>   ...
>   Skylake-Server-IBRS
>   Skylake-Server
>   Cascadelake-Server
> 
> 
> This seems contradictory to me but perhaps I'm overlooking something?

The usable=yse|no attribute says whether a given CPU model can be
provided to a guest without any modification. That is whether you can
use

  Cascadelake-Server


When usable='no', QEMU will need to disable some of the features that
are part of the Cascadelake-Server CPU model. In other words, the CPU
model is not usable as is.

On the other hand, the  element in domain
capabilities describes the CPU used as a host-model CPU by adding or
removing some specific features. In other words, while using the simple
CPU definition mentioned above would cause QEMU to drop some features,
using a more verbose

  
Cascadelake-Server
Intel











  

explicitly asks QEMU to disable pku and avx512vnni features and thus
QEMU will not have to disable anything.

The situation on the first machine is a bit strange as there are no
features disabled in host-model CPU definition, which makes it unclear
why QEMU reports Cascadelake-Server as unusable (QEMU reports the
reason, but we don't do so yet).

Anyway, would you mind running the tests/cputestdata/cpu-gather.sh
script on both machines (make sure to install qemu, python3, and cpuid
packages first) and send us the output so that we can check the CPU
models are properly detected?

>  docs/formatdomaincaps.html.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
> index 66e758501b..7fd1f91f73 100644
> --- a/docs/formatdomaincaps.html.in
> +++ b/docs/formatdomaincaps.html.in
> @@ -232,7 +232,7 @@
>host-model
>
>  If host-model is supported by the hypervisor, the
> -mode describes the guest CPU which will be used when
> +model describes the guest CPU which will be used when
>  starting a domain with host-model CPU. The hypervisor
>  specifics (such as unsupported CPU models or features, machine type,
>  etc.) may be accounted for in this guest CPU specification and thus

As strange as it seems, there's no typo. The  element describes
just a small part of the whole CPU. There are additional  and
 elements which also belong to the CPU definition. All these
elements are children of the  element and thus the  element
describes the guest CPU. Yes, this is not the best design and having an
entire  element rather than its children in the
 element would be better, but we
can't really do anything about it now.

Jirka

Re: [PATCH v4 0/2] introduction of migration_version attribute for VFIO live migration

[Dr. David Alan Gilbert]

* Yan Zhao (yan.y.z...@intel.com) wrote:
> On Tue, Mar 24, 2020 at 05:29:59AM +0800, Alex Williamson wrote:
> > On Mon, 3 Jun 2019 20:34:22 -0400
> > Yan Zhao  wrote:
> > 
> > > On Tue, Jun 04, 2019 at 03:29:32AM +0800, Alex Williamson wrote:
> > > > On Thu, 30 May 2019 20:44:38 -0400
> > > > Yan Zhao  wrote:
> > > >   
> > > > > This patchset introduces a migration_version attribute under sysfs of 
> > > > > VFIO
> > > > > Mediated devices.
> > > > > 
> > > > > This migration_version attribute is used to check migration 
> > > > > compatibility
> > > > > between two mdev devices of the same mdev type.
> > > > > 
> > > > > Patch 1 defines migration_version attribute in
> > > > > Documentation/vfio-mediated-device.txt
> > > > > 
> > > > > Patch 2 uses GVT as an example to show how to expose migration_version
> > > > > attribute and check migration compatibility in vendor driver.  
> > > > 
> > > > Thanks for iterating through this, it looks like we've settled on
> > > > something reasonable, but now what?  This is one piece of the puzzle to
> > > > supporting mdev migration, but I don't think it makes sense to commit
> > > > this upstream on its own without also defining the remainder of how we
> > > > actually do migration, preferably with more than one working
> > > > implementation and at least prototyped, if not final, QEMU support.  I
> > > > hope that was the intent, and maybe it's now time to look at the next
> > > > piece of the puzzle.  Thanks,
> > > > 
> > > > Alex  
> > > 
> > > Got it. 
> > > Also thank you and all for discussing and guiding all along:)
> > > We'll move to the next episode now.
> > 
> > Hi Yan,
> > 
> > As we're hopefully moving towards a migration API, would it make sense
> > to refresh this series at the same time?  I think we're still expecting
> > a vendor driver implementing Kirti's migration API to also implement
> > this sysfs interface for compatibility verification.  Thanks,
> >
> Hi Alex
> Got it!
> Thanks for reminding of this. And as now we have vfio-pci implementing
> vendor ops to allow live migration of pass-through devices, is it
> necessary to implement similar sysfs node for those devices?
> or do you think just PCI IDs of those devices are enough for libvirt to
> know device compatibility ?

Wasn't the problem that we'd have to know how to check for things like:
  a) Whether different firmware versions in the device were actually
compatible
  b) Whether minor hardware differences were compatible - e.g. some
hardware might let you migrate to the next version of hardware up.

Dave

> Thanks
> Yan
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: Hot unplug disabling on pci-pci bridge

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 07:24:39AM +, Ani Sinha wrote:
> Hi All :
> 
> I have been playing with Qemu trying to disable hot-unplug capability for 
> conventional PCI. I have discussed this briefly on IRC and the plan is to 
> have an option on the pci-pci bridge that would disable SHPC and ACPI hotplug 
> capability for all the slots on that bus. I am _not_ trying to implement a 
> per slot capability for conventional PCI as was previously posted for PCIE 
> slots in this patch : 
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg01833.html (we do 
> not need this atm).
> 
> I am following the conversations which happened few weeks back here:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg00985.html
> 
> To that end, I have been experimenting with Qemu using the patch I mention 
> below. I have attached the virtio balloon driver with bus 1 which is attached 
> to the pci bridge.
> 
>
>   
>
>   
>   
>   
>   
>function='0x0'/>
>
>   
>   
>   
>function='0x0'/>
>
> 
> 
> I am using a windows guest and from the guest I can see that the balloon 
> driver is indeed attached to the pci bridge (see attached screenshot).
> I still find Windows giving me an option to hot eject the pci balloon driver.
> 
> So what am I doing wrong here?
> [cid:87E6C01C-8B8C-4E24-AC4B-C0D746C2F605]
> 
> The patch I am experimenting with (which is currently a hack) is
> attached below. It is based off libvirt 4.5 and not the latest mainline.

This is a patch against QEMU actually.  I doubt many on the libvirt mailing
list are familiar with the PCI / ACPI needs around hotplug, so it is probably
best to re-send this patch to the QEMU mailing list and ask questions there.

> 
> ---
> hw/pci-bridge/pci_bridge_dev.c | 16 
> 1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index b2d861d..e706d49 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -58,7 +58,7 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error 
> **errp)
> 
> pci_bridge_initfn(dev, TYPE_PCI_BUS);
> 
> -if (bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
> +if (0) {//bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_SHPC_REQ)) {
> dev->config[PCI_INTERRUPT_PIN] = 0x1;
> memory_region_init(_dev->bar, OBJECT(dev), "shpc-bar",
>shpc_bar_size(dev));
> @@ -161,7 +161,7 @@ static Property pci_bridge_dev_properties[] = {
> DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
> ON_OFF_AUTO_AUTO),
> DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
> -PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> +PCI_BRIDGE_DEV_F_SHPC_REQ, false),
> DEFINE_PROP_END_OF_LIST(),
> };
> 
> @@ -181,7 +181,7 @@ static const VMStateDescription pci_bridge_dev_vmstate = {
> VMSTATE_END_OF_LIST()
> }
> };
> -
> +#if 0
> static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>   DeviceState *dev, Error **errp)
> {
> @@ -208,12 +208,12 @@ static void 
> pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> }
> shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
> }
> -
> +#endif
> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> +//HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> 
> k->realize = pci_bridge_dev_realize;
> k->exit = pci_bridge_dev_exitfn;
> @@ -227,8 +227,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, 
> void *data)
> dc->props = pci_bridge_dev_properties;
> dc->vmsd = _bridge_dev_vmstate;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -hc->plug = pci_bridge_dev_hotplug_cb;
> -hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
> +//hc->plug = pci_bridge_dev_hotplug_cb;
> +//hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
> }
> 
> static const TypeInfo pci_bridge_dev_info = {
> @@ -238,7 +238,7 @@ static const TypeInfo pci_bridge_dev_info = {
> .class_init= pci_bridge_dev_class_init,
> .instance_finalize = pci_bridge_dev_instance_finalize,
> .interfaces = (InterfaceInfo[]) {
> -{ TYPE_HOTPLUG_HANDLER },
> +//{ TYPE_HOTPLUG_HANDLER },
> { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> { }
> }
> --
> 1.9.4
> 



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt-tck] scripts: fix disk media change test

[Daniel P . Berrangé]

On Mon, Mar 23, 2020 at 05:09:24PM -0600, Jim Fehlig wrote:
> Since the introduction of the 'index' attribute of a disk's
>  element, test 207-disk-media-change.t is failing. The
> test is a bit flawed in that it compares initial and final domXML
> after changing the media of a cdrom disk device. The test only
> needs to check the final domXML to ensure the correct 
> path exists at the end of the test.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  scripts/domain/207-disk-media-change.t | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [libvirt-tck] scripts: fix disk media change test

[Erik Skultety]

On Mon, Mar 23, 2020 at 05:09:24PM -0600, Jim Fehlig wrote:
> Since the introduction of the 'index' attribute of a disk's
>  element, test 207-disk-media-change.t is failing. The
> test is a bit flawed in that it compares initial and final domXML
> after changing the media of a cdrom disk device. The test only
> needs to check the final domXML to ensure the correct 
> path exists at the end of the test.
>
> Signed-off-by: Jim Fehlig 
Reviewed-by: Erik Skultety 

Re: [PATCH v4 0/6] NVDIMM suport for pSeries guests

[Michal Prívozník]

On 23. 3. 2020 20:40, Daniel Henrique Barboza wrote:
> changes in v4:
> - moved label size requirement from virDomainMemoryDefParseXML()
> to virDomainMemoryDefValidate()
> 
> Previous version:
> https://www.redhat.com/archives/libvir-list/2020-March/msg00769.html 
> 
> Daniel Henrique Barboza (6):
>   qemu: capabilities: update qemu-5.0.0 capabilities for ppc64
>   conf: Introduce optional 'uuid' element for NVDIMM memory
>   formatdomain.html.in: document the new 'uuid' NVDIMM element
>   conf, qemu: enable NVDIMM support for ppc64
>   formatdomain.html.in: document NVDIMM 'label' requirement for pSeries
>   news.xml: document the new NVDIMM support for Pseries guests
> 
>  docs/formatdomain.html.in |24 +-
>  docs/news.xml |11 +
>  docs/schemas/domaincommon.rng | 5 +
>  src/conf/domain_conf.c|49 +-
>  src/conf/domain_conf.h| 3 +
>  src/qemu/qemu_command.c   | 7 +
>  src/qemu/qemu_domain.c|63 +-
>  src/qemu/qemu_domain.h| 4 +-
>  src/qemu/qemu_hotplug.c   | 6 +-
>  tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 2 +-
>  .../caps_5.0.0.ppc64.replies  | 10949 +---
>  .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |  1216 +-
>  ...default-video-type-ppc64.ppc64-latest.args | 2 -
>  .../memory-hotplug-nvdimm-ppc64.args  |32 +
>  ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args |36 +
>  .../memory-hotplug-nvdimm-ppc64.xml   |50 +
>  ...ault-cpu-kvm-pseries-2.7.ppc64-latest.args |10 +-
>  ...ault-cpu-kvm-pseries-3.1.ppc64-latest.args |10 +-
>  ...ault-cpu-kvm-pseries-4.2.ppc64-latest.args |10 +-
>  ...ault-cpu-tcg-pseries-2.7.ppc64-latest.args |10 +-
>  ...ault-cpu-tcg-pseries-3.1.ppc64-latest.args |10 +-
>  ...ault-cpu-tcg-pseries-4.2.ppc64-latest.args |10 +-
>  .../ppc64-pseries-graphics.ppc64-latest.args  |10 +-
>  .../ppc64-pseries-headless.ppc64-latest.args  |10 +-
>  .../tpm-emulator-spapr.ppc64-latest.args  | 9 +-
>  tests/qemuxml2argvtest.c  | 1 +
>  .../memory-hotplug-nvdimm-ppc64.xml   |50 +
>  tests/qemuxml2xmltest.c   | 2 +
>  28 files changed, 7028 insertions(+), 5573 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args
>  create mode 100644 
> tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml
>  create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
> 

Reviewed-by: Michal Privoznik 

and pushed.

Michal