Re: [PATCH] qemu: Move some enums impl to qemu_monitor.c
On 2/21/22 17:17, Andrea Bolognani wrote: > On Mon, Feb 21, 2022 at 04:10:25PM +0100, Michal Privoznik wrote: >> +VIR_ENUM_IMPL(qemuMonitorCPUProperty, >> + QEMU_MONITOR_CPU_PROPERTY_LAST, >> + "boolean", >> + "string", >> + "number", >> +); >> >> VIR_ENUM_IMPL(qemuMonitorMigrationStatus, >>QEMU_MONITOR_MIGRATION_STATUS_LAST, >> @@ -4473,6 +4505,14 @@ qemuMonitorTransactionBackup(virJSONValue *actions, >> } >> >> >> +VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode, >> + QEMU_MONITOR_DIRTYRATE_CALC_MODE_LAST, >> + "page-sampling", >> + "dirty-bitmap", >> + "dirty-ring", >> +); >> + >> + > > Kinda weird that this one ends up in the middle of the file instead > of being grouped with the rest. I'd keep them together, unless > there's a good reason for doing things this way that I missed. My idea was to keep it close to the rest of dirty rate related functions. But I can move it next to the rest. I don't have strong preference. > > Either way, > > Reviewed-by: Andrea Bolognani > Thanks, I'll wait for your reply before pushing. Michal
Re: [PATCH v2 09/29] qemu: introduce QEMU_CAPS_DEVICE_PNV_PHB3
On 2/21/22 10:07, Ján Tomko wrote:
On a Tuesday in 2022, Daniel Henrique Barboza wrote:
QEMU_CAPS_DEVICE_PNV_PHB3 indicates binary support for the pnv-phb3
device, the pcie-root controller for PowerNV8 domains, and also
the pnv-phb3-root-port device, its pcie-root-port device.
This capability is present in QEMU since 5.0.0 but these devices are
user-creatable only after QEMU 6.2.0. This means that probing it as
default will be misleading for users. Instead, let's use
virQEMUCapsInitQMPVersionCaps() to check for the adequate QEMU version
and arch and set it manually.
Suggested-by: Peter Krempa
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_capabilities.c | 19 +++
src/qemu/qemu_capabilities.h | 1 +
.../qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 1 +
3 files changed, 21 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bb90715569..d60240912c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -662,6 +662,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 420 */
"device.json+hotplug", /* QEMU_CAPS_DEVICE_JSON */
"hvf", /* QEMU_CAPS_HVF */
+ "pnv-phb3", /* QEMU_CAPS_DEVICE_PNV_PHB3 */
);
@@ -1397,6 +1398,17 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] =
{
{ "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
{ "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
+ /*
+ * We don't probe the following PowerNV devices:
+ *
If we don't probe them, then this comment does not belong here. Rather
it should be documented in the commit message and the place that sets it
based on the version.
Ok.
+ * { "pnv-phb3", QEMU_CAPS_DEVICE_PNV_PHB3 },
+ * { "pnv-phb3-root-port", QEMU_CAPS_DEVICE_PNV_PHB3 },
+ *
+ * Because they are present in QEMU binaries since QEMU 5.0.0
+ * but became user creatable only in the QEMU 7.0.0 development
+ * cycle. Their respective capabilities are being set in
+ * virQEMUCapsInitQMPVersionCaps().
Alternatively, you can probe the device here (just "pnv-phb3", no need
to look at both) and clear the capability if QEMU is too old to let the
user create it. That way it will be easier to delete years from now when
we raise minimum QEMU version.
Got it.
+ */
};
@@ -5231,6 +5243,13 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCaps *qemuCaps)
*/
if (qemuCaps->version < 5002000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_ENABLE_FIPS);
+
+ /* PowerNV pnv-phb devices weren't user creatable up to
+ * QEMU 6.2.0. The version value set here was taken from a
+ * binary post 6.2.0 release that has user creatable pnv-phb
+ * support. */
+ if (qemuCaps->version >= 6002050 && ARCH_IS_PPC64(qemuCaps->arch))
I'd rather not use the git version in here.
If nothing better, at least rewrite this to: > 6002000
I'll change to 6002000 and, together with the change you proposed above, we will
assume that the caps exists and remove them if we're running with QEMU 6.2.0 or
older.
This support went live right after 6.2.0 released, so even if in theory there
is some
QEMU builds where this assumption would fail, it is highly unlikely for one to
grab a
post 6.2.0 QEMU commit that happens to not have it today.
Even better if we waited for 7.0.0 or QEMU had some property we could
What if I bump the minimal version to 7.0.0 after QEMU 7.0.0 is released?
This would prevent these series to be postponed for another 2 months, we would
cover the
small version gap that I mentioned above and I can proceed with adding more
devices
on top of it (e.g. the BMC devices).
look for (looking at QEMU history there were some properties removed
for phb4 as a part of making them user creatable but I don't see
anything for phb3).
There's a bit of QEMU lore here but long story short: both powernv8 and
powernv9 machines
weren't libvirt material (the PHBs weren't user creatable, so -nodefaults would
create
a domain without any PHBs).
Upstream QEMU made post 6.2.0 changes to make it happen. The changes you are
referring
to were part of this process. The powernv9 machine was a little bloated and we
took this
opportunity to clean up the model.
Thanks,
Daniel
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_PNV_PHB3);
}
With the comment removed and/or the code probing first then clearing
based on version number:
Reviewed-by: Ján Tomko
Jano
Re: [PATCH v2 01/29] qemu_domain.c: add PowerNV machine helpers
On 2/21/22 09:47, Ján Tomko wrote:
On a Tuesday in 2022, Daniel Henrique Barboza wrote:
The PowerNV (Power Non-Virtualized) QEMU ppc64 machine is the emulation
of a bare-metal IBM Power host. It follows the OPAL (OpenPower
Abstration Layer) API/ABI, most specifically Skiboot [1]. For now,
Libvirt has support for the pSeries QEMU machine, which is the emulation
s/Libvirt/libvirt/
I think I've been using capital L for a long time now. I'll make sure to pay
attention next time.
of a logical partition (guest) that would run on top of a bare-metal
system.
This patch introduces the helpers that are going to be used to add a
basic support for PowerNV domains in Libvirt. Given that there are quite
s/Libvirt/libvirt/
a few similarities in how pSeries and PowerNVv should be handled, we're
also adding a 'qemuDomainIsPowerPC' helper that will be used in those
instances.
[1] https://open-power.github.io/skiboot/doc/overview.html
Reviewed-by: Peter Krempa
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 41 +
src/qemu/qemu_domain.h | 4
2 files changed, 45 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index aa8f6b8d05..2e21a95f38 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8719,6 +8719,47 @@ qemuDomainIsPSeries(const virDomainDef *def)
}
+/*
+ * The PowerNV machine does not support KVM acceleration in Power
+ * hosts.
We can skip the usual ARCH_IS_PPC64() since this machine
+ * is usable with TCG in all host archs.
I don't understand the comment. We use these ARCH_ macros
to check the guest arch, not the host arch. So the check
should be here too.
True. I removed the comment.
Thanks,
Daniel
+ */
+bool
+qemuDomainIsPowerNV(const virDomainDef *def)
+{
+ if (STRPREFIX(def->os.machine, "powernv"))
+ return true;
+
+ return false;
+}
+
+
Reviewed-by: Ján Tomko
Jano
Re: Libvirt Rust bindings could use some work
On Mon, Feb 21, 2022 at 05:28:24PM +0100, Wim de With wrote: > On Mon, Feb 21, 2022 at 10:21:32AM -0500, Andrea Bolognani wrote: > > We are aware of the issues with the current API of libvirt-rs, but > > unfortunately nobody has been able to dedicate time to addressing > > them. Any contributions towards that goal that you or anyone else > > could make would certainly be greatly appreciated! > > > > Breaking the API should be fine I think. The current version number > > is 0.2.11, so there shouldn't be any expectation in terms of API > > stability. > > I realized that GitLab is nowadays used for communication so I made more > specific issues there. Do you prefer merge requests there or patches on > the mailing list? Merge requests exclusively for anything other than libvirt.git > > Note that libvirt-python is mostly autogenerated, and there is an > > ongoing effort to make the same happen for libvirt-go-module. Ideally > > libvirt-rust would also follow this trend and end up with very little > > manually-written code in it. > > I've done some experiments here with bindgen, and it is pretty > straightforward to generate FFI bindings using it. The problem is that > generating safe idiomatic Rust wrappers on top of these bindings is > non-trivial. Still, most wrapper functions will probably follow the same > pattern, so it may be useful to investigate if macros could be used to > make it easier to add new functions. > > >From looking at the Git diff from v2.5.0 to v8.0.0, the libvirt API > doesn't seem to change that often. Almost all changes consist of adding > new enum variants and some new functions. Am I correct in assuming that > an application that was written for v2.5.0 would still work with v8.0.0? Correct, libvirt never knowingly breaks API compatibility. The API is strictly append-only. > To me it seems the best course of action would be to pick some minimum > version of libvirt to support, and make sure that the Rust API wraps all > functions there. From that point, adding functions and enum variants > introduced in later versions of libvirt can be incremental changes and > will (hopefully) be fairly easy to add. Rust has the concept of features > to enable conditional compiling of those functions and enum variants > only when the underlying libvirt supports them. Conditional compilation is what we do in Go / Python bindings too. 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 Rust bindings could use some work
On Mon, Feb 21, 2022 at 10:21:32AM -0500, Andrea Bolognani wrote: > On Fri, Feb 18, 2022 at 10:28:29AM +0100, Wim de With wrote: > > Hello, > > > > I was reading the Rust bindings for libvirt and I noticed that they are > > not very idiomatic. A couple of examples: > > > > 1. It is conventional to have a *-sys crate containing only the C > > interface and the linking configuration. The virt crate would then > > depend on this libvirt-sys crate to implement a safe and ergonomic > > wrapper. > > 2. The API heavily uses integers to indicate flags. Sometimes those > > integers are type aliased and sometimes they are not. In Rust, this can > > be much nicer, using enums to represent only valid flags. The same > > applies to some return values. > > 3. Since libvirt is thread-safe, the Rust types could be Send and Sync, > > so that they can also be used in a multithreaded context. Special care > > needs to be taken with translating virGetLastError to Rust Results, but > > it shouldn't be a problem. > > > > For a nice example of a Rust wrapper around a C library, see: > > https://github.com/rust-lang/git2-rs > > > > I am willing to send some patches to fix some of these problems, but > > this will almost certainly lead to breaking API changes, and I am not > > sure what your opinion is on that. > > We are aware of the issues with the current API of libvirt-rs, but > unfortunately nobody has been able to dedicate time to addressing > them. Any contributions towards that goal that you or anyone else > could make would certainly be greatly appreciated! > > Breaking the API should be fine I think. The current version number > is 0.2.11, so there shouldn't be any expectation in terms of API > stability. > > Note that libvirt-python is mostly autogenerated, and there is an > ongoing effort to make the same happen for libvirt-go-module. Ideally > libvirt-rust would also follow this trend and end up with very little > manually-written code in it. Note that the auto-generating part in Go only applies to the very low level shim in CGo, which was originally only needed to ensure serialization for accessing thread locals errors, and now is in progress of being repurposed as a dlopen layer. The actual application facing APIs remain entirely hand written today. This lets us expose APIs in a way that looks quite different from the C where we have virTypedParameter APIs, and also where we have event callbacks. > > Finally, while I am not a legal expert, I believe that licensing a Rust > > library as LGPL does not make much sense. Rust does not have a stable > > ABI so everything is statically linked. This means that adding virt to a > > Cargo.toml file of a non-GPL application is already in violation of the > > license. Yes, that is an unfortunate artifact of static linking. > Licensing is a pretty fun minefield :) > > For example, even though libvirt-rs itself would be statically linked > into any application that uses it, by virtue of it being a wrapper > around the C library you'd still end up dynamically linking against > that. > > I tend to agree that language bindings should follow the rest of the > language ecosystem in terms of licensing, and for libvirt-rs > specifically that would probably mean MIT. Perhaps we should consider > looking into relicensing the project? Since the intent of libvirt using LGPL was explicitly to allow non-GPL compatible applications to use libvirt, it is a mistake to be using a license that breaks this ability for Rust. In Golang we've used MIT license For Rust we should aim for whatever is most appropriate - MIT or BSD or Apache - I'm not familiar with which is dominant in the Rust world. The challenge will be getting copyright holder agreement. Sahid has written the vast majority of the code, but has changed employers a few times - most code was while he was at Red Hat, but some was at Canonical. The further complication is that French legal rules often mean an employee retains copyright. To be safe we would need to cover all possiblities and get explicit agreement from Sahid, Canonical and Red Hat. I can give approval on behalf of Red Hat. Then there's a small number of other contributor who would need to be found, or their contribution removed & re-written. 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] qemu: Move some enums impl to qemu_monitor.c
On Mon, Feb 21, 2022 at 04:10:25PM +0100, Michal Privoznik wrote: > +VIR_ENUM_IMPL(qemuMonitorCPUProperty, > + QEMU_MONITOR_CPU_PROPERTY_LAST, > + "boolean", > + "string", > + "number", > +); > > VIR_ENUM_IMPL(qemuMonitorMigrationStatus, >QEMU_MONITOR_MIGRATION_STATUS_LAST, > @@ -4473,6 +4505,14 @@ qemuMonitorTransactionBackup(virJSONValue *actions, > } > > > +VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode, > + QEMU_MONITOR_DIRTYRATE_CALC_MODE_LAST, > + "page-sampling", > + "dirty-bitmap", > + "dirty-ring", > +); > + > + Kinda weird that this one ends up in the middle of the file instead of being grouped with the rest. I'd keep them together, unless there's a good reason for doing things this way that I missed. Either way, Reviewed-by: Andrea Bolognani -- Andrea Bolognani / Red Hat / Virtualization
Re: Libvirt Rust bindings could use some work
On Mon, Feb 21, 2022 at 04:30:28PM +0100, Ján Tomko wrote: > On a Monday in 2022, Andrea Bolognani wrote: > > On Fri, Feb 18, 2022 at 10:28:29AM +0100, Wim de With wrote: > > Licensing is a pretty fun minefield :) > > > > For example, even though libvirt-rs itself would be statically linked > > into any application that uses it, by virtue of it being a wrapper > > around the C library you'd still end up dynamically linking against > > that. > > > > I tend to agree that language bindings should follow the rest of the > > language ecosystem in terms of licensing, and for libvirt-rs > > specifically that would probably mean MIT. Perhaps we should consider > > looking into relicensing the project? > > Alternatively, stay away from "ecosystems" that are not compatible with > copyleft. libvirt itself is LGPL. Proprietary applications can link against it. It's also where all the actual logic lives. Language bindings are supposed to be "uninteresting" in that for the most part they just convert inputs and outputs between the formats understood by the two runtimes, so using a fairly liberal license for them doesn't seem like a big deal to me. -- Andrea Bolognani / Red Hat / Virtualization
Re: Libvirt Rust bindings could use some work
On a Monday in 2022, Andrea Bolognani wrote: On Fri, Feb 18, 2022 at 10:28:29AM +0100, Wim de With wrote: Licensing is a pretty fun minefield :) For example, even though libvirt-rs itself would be statically linked into any application that uses it, by virtue of it being a wrapper around the C library you'd still end up dynamically linking against that. I tend to agree that language bindings should follow the rest of the language ecosystem in terms of licensing, and for libvirt-rs specifically that would probably mean MIT. Perhaps we should consider looking into relicensing the project? Alternatively, stay away from "ecosystems" that are not compatible with copyleft. Jano signature.asc Description: PGP signature
Re: Call for GSoC and Outreachy project ideas for summer 2022
On 2/21/22 12:27, Paolo Bonzini wrote: > On 2/21/22 10:36, Michal Prívozník wrote: >> Indeed. Libvirt's participating on its own since 2016, IIRC. Since we're >> still in org acceptance phase we have some time to decide this, >> actually. We can do the final decision after participating orgs are >> announced. My gut feeling says that it's going to be more work on QEMU >> side which would warrant it to be on the QEMU ideas page. > > There are multiple projects that can be done on this topic, some > QEMU-only, some Libvirt-only. For now I would announce the project on > the Libvirt side (and focus on those projects) since you are comentoring. > Alright then. I've listed the project idea here: https://gitlab.com/libvirt/libvirt/-/issues/276 Please let me know what do you think. Michal
Re: Libvirt Rust bindings could use some work
On Fri, Feb 18, 2022 at 10:28:29AM +0100, Wim de With wrote: > Hello, > > I was reading the Rust bindings for libvirt and I noticed that they are > not very idiomatic. A couple of examples: > > 1. It is conventional to have a *-sys crate containing only the C > interface and the linking configuration. The virt crate would then > depend on this libvirt-sys crate to implement a safe and ergonomic > wrapper. > 2. The API heavily uses integers to indicate flags. Sometimes those > integers are type aliased and sometimes they are not. In Rust, this can > be much nicer, using enums to represent only valid flags. The same > applies to some return values. > 3. Since libvirt is thread-safe, the Rust types could be Send and Sync, > so that they can also be used in a multithreaded context. Special care > needs to be taken with translating virGetLastError to Rust Results, but > it shouldn't be a problem. > > For a nice example of a Rust wrapper around a C library, see: > https://github.com/rust-lang/git2-rs > > I am willing to send some patches to fix some of these problems, but > this will almost certainly lead to breaking API changes, and I am not > sure what your opinion is on that. We are aware of the issues with the current API of libvirt-rs, but unfortunately nobody has been able to dedicate time to addressing them. Any contributions towards that goal that you or anyone else could make would certainly be greatly appreciated! Breaking the API should be fine I think. The current version number is 0.2.11, so there shouldn't be any expectation in terms of API stability. Note that libvirt-python is mostly autogenerated, and there is an ongoing effort to make the same happen for libvirt-go-module. Ideally libvirt-rust would also follow this trend and end up with very little manually-written code in it. > Finally, while I am not a legal expert, I believe that licensing a Rust > library as LGPL does not make much sense. Rust does not have a stable > ABI so everything is statically linked. This means that adding virt to a > Cargo.toml file of a non-GPL application is already in violation of the > license. Licensing is a pretty fun minefield :) For example, even though libvirt-rs itself would be statically linked into any application that uses it, by virtue of it being a wrapper around the C library you'd still end up dynamically linking against that. I tend to agree that language bindings should follow the rest of the language ecosystem in terms of licensing, and for libvirt-rs specifically that would probably mean MIT. Perhaps we should consider looking into relicensing the project? -- Andrea Bolognani / Red Hat / Virtualization
[PATCH] qemu: Move some enums impl to qemu_monitor.c
There are some enums that are declared in qemu_monitor.h but
implemented in qemu_monitor_json.c. While from compiler and
linker POV it doesn't matter, the code is cleaner if an enum is
implemented in .c file that corresponds to .h file which declared
the enum.
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_monitor.c | 40
src/qemu/qemu_monitor_json.c | 34 --
2 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0ff938a577..8fc2a49abf 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -111,6 +111,38 @@ static int qemuMonitorOnceInit(void)
VIR_ONCE_GLOBAL_INIT(qemuMonitor);
+VIR_ENUM_IMPL(qemuMonitorJob,
+ QEMU_MONITOR_JOB_TYPE_LAST,
+ "",
+ "commit",
+ "stream",
+ "mirror",
+ "backup",
+ "create",
+);
+
+VIR_ENUM_IMPL(qemuMonitorJobStatus,
+ QEMU_MONITOR_JOB_STATUS_LAST,
+ "",
+ "created",
+ "running",
+ "paused",
+ "ready",
+ "standby",
+ "waiting",
+ "pending",
+ "aborting",
+ "concluded",
+ "undefined",
+ "null",
+);
+
+VIR_ENUM_IMPL(qemuMonitorCPUProperty,
+ QEMU_MONITOR_CPU_PROPERTY_LAST,
+ "boolean",
+ "string",
+ "number",
+);
VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
QEMU_MONITOR_MIGRATION_STATUS_LAST,
@@ -4473,6 +4505,14 @@ qemuMonitorTransactionBackup(virJSONValue *actions,
}
+VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode,
+ QEMU_MONITOR_DIRTYRATE_CALC_MODE_LAST,
+ "page-sampling",
+ "dirty-bitmap",
+ "dirty-ring",
+);
+
+
int
qemuMonitorStartDirtyRateCalc(qemuMonitor *mon,
int seconds,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 345b81cd12..4d339f29b8 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -53,30 +53,6 @@ VIR_LOG_INIT("qemu.qemu_monitor_json");
#define LINE_ENDING "\r\n"
-VIR_ENUM_IMPL(qemuMonitorJob,
- QEMU_MONITOR_JOB_TYPE_LAST,
- "",
- "commit",
- "stream",
- "mirror",
- "backup",
- "create");
-
-VIR_ENUM_IMPL(qemuMonitorJobStatus,
- QEMU_MONITOR_JOB_STATUS_LAST,
- "",
- "created",
- "running",
- "paused",
- "ready",
- "standby",
- "waiting",
- "pending",
- "aborting",
- "concluded",
- "undefined",
- "null");
-
static void qemuMonitorJSONHandleShutdown(qemuMonitor *mon, virJSONValue
*data);
static void qemuMonitorJSONHandleReset(qemuMonitor *mon, virJSONValue *data);
static void qemuMonitorJSONHandleStop(qemuMonitor *mon, virJSONValue *data);
@@ -5347,11 +5323,6 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitor *mon,
}
-VIR_ENUM_IMPL(qemuMonitorCPUProperty,
- QEMU_MONITOR_CPU_PROPERTY_LAST,
- "boolean", "string", "number",
-);
-
static int
qemuMonitorJSONParseCPUModelProperty(const char *key,
virJSONValue *value,
@@ -8740,11 +8711,6 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitor *mon,
migratable);
}
-VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode,
- QEMU_MONITOR_DIRTYRATE_CALC_MODE_LAST,
- "page-sampling",
- "dirty-bitmap",
- "dirty-ring");
int
qemuMonitorJSONStartDirtyRateCalc(qemuMonitor *mon,
--
2.34.1
Re: [PATCH] NEWS: Document domain dirty page rate calculation APIs
On 2/21/22 14:30, huang...@chinatelecom.cn wrote: > From: Hyman Huang(黄勇) > > The Libvirt API virDomainStartDirtyRateCalc was extended. > Document this change. > > Signed-off-by: Hyman Huang(黄勇) > --- > NEWS.rst | 14 ++ > 1 file changed, 14 insertions(+) Reviewed-by: Michal Privoznik and pushed. Michal
Re: [PATCH] qemu: Use virDomainObjCheckActive() more
On 2/21/22 14:27, Ján Tomko wrote:
> On a Monday in 2022, Michal Privoznik wrote:
>> Using the following spatch, I've identified two places which
>> could be switched from explicit virDomainObjIsActive() +
>> virReportError() to virDomainObjCheckActive():
>>
>> @@
>> expression dom;
>> @@
>> if (
>> - !virDomainObjIsActive(dom)
>> + virDomainObjCheckActive(dom) < 0
>> ) {
>> - virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is
>> not running"));
>> ...
>> }
>>
>> Signed-off-by: Michal Privoznik
>> ---
>>
>> BTW. if I'm more aggressive and let virReportError() have just any args
>> then even more places fit the rule (approx. two dozen more).
>>
>
> Had you sent that as a patch, we would have been able to see what kind
> of error messages are used in those places without having to run the
> spatch.
Here you go. Problem is, we'd be changing the error code which may break
some apps (e.g. those who check for it).
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 093b719b2c..1a20f11227 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3956,9 +3956,7 @@ virDomainObjWait(virDomainObj *vm)
return -1;
}
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("domain is not running"));
+if (virDomainObjCheckActive(vm) < 0) {
return -1;
}
@@ -6361,9 +6359,7 @@ virDomainDefLifecycleActionAllowed(virDomainLifecycle
type,
int
virDomainObjCheckActive(virDomainObj *dom)
{
-if (!virDomainObjIsActive(dom)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+if (virDomainObjCheckActive(dom) < 0) {
return -1;
}
return 0;
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 6944c77eed..4f40dd97e5 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1266,11 +1266,8 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn,
goto cleanup;
/* Check if domain is alive */
-if (!virDomainObjIsActive(vm)) {
+if (virDomainObjCheckActive(vm) < 0) {
/* Migration failed if domain is inactive */
-virReportError(VIR_ERR_OPERATION_FAILED,
- "%s", _("Migration failed. Domain is not running "
- "on destination host"));
goto cleanup;
}
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 2471242e60..3b44ff8e7f 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -801,9 +801,7 @@ qemuBackupBegin(virDomainObj *vm,
qemuDomainJobSetStatsType(priv->job.current,
QEMU_DOMAIN_JOB_STATS_TYPE_BACKUP);
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot perform disk backup for inactive domain"));
+if (virDomainObjCheckActive(vm) < 0) {
goto endjob;
}
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 69f287399b..8701bd8083 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -588,9 +588,7 @@ qemuCheckpointCreateXML(virDomainPtr domain,
}
if (!redefine) {
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot create checkpoint for inactive domain"));
+if (virDomainObjCheckActive(vm) < 0) {
return NULL;
}
@@ -856,9 +854,7 @@ qemuCheckpointDelete(virDomainObj *vm,
return -1;
if (!metadata_only) {
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot delete checkpoint for inactive domain"));
+if (virDomainObjCheckActive(vm) < 0) {
goto endjob;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6a70b72760..cdf523b304 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5866,9 +5866,7 @@ qemuDomainObjEnterMonitorInternal(virQEMUDriver *driver,
int ret;
if ((ret = qemuDomainObjBeginNestedJob(driver, obj, asyncJob)) < 0)
return ret;
-if (!virDomainObjIsActive(obj)) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("domain is no longer running"));
+if (virDomainObjCheckActive(obj) < 0) {
qemuDomainObjEndJob(driver, obj);
return -1;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e417d358cd..3c38ff71cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2652,9 +2652,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver,
VIR_DOMAIN_JOB_OPERATION_SAVE, flags) < 0)
goto cleanup;
-if (!virDomainObjIsActive(vm)) {
Re: [PATCH v2 26/29] domain_conf.c: add phb4-root-port to IsPowerNVRootPort()
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Make the virDomainControllerIsPowerNVRootPort() helper recognize pnv-phb4-root-port as a PowerNV root port. This will spare us from duplicating checks where the constraints of pnv-phb3-root-port also applies for the pnv-phb4-root-port device. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 25/29] conf, qemu: add 'pnv-phb4-root-port' PCI controller model name
On a Tuesday in 2022, Daniel Henrique Barboza wrote: This device is an implementation of pcie-root-port, similar to its sibling pnv-phb3-root-port. Since it's a new model name that Libvirt automatically sets, we refrain from documenting it to users. Signed-off-by: Daniel Henrique Barboza --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_domain_address.c | 14 +- src/qemu/qemu_validate.c | 8 ++-- 5 files changed, 22 insertions(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 18/29] qemu_command.c: add command line for the pnv-phb3 device
On a Tuesday in 2022, Daniel Henrique Barboza wrote: The command line for the pnv-phb3 device is similar to the spapr-pci-host-bridge command line but adding the extra 'chip-id' attribute. Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_command.c | 21 +-- .../powernv8-basic.ppc64-latest.args | 1 + 2 files changed, 20 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 20/29] domain_conf: format pnv-phb3-root-port empty addr
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Hiding the empty (:00:0.0) PCI address in the case of devices that will connect to slot 0x0 can be counterintuitive to the user, which will see something like this: Even if the user deliberately adds the root-port element: We end up removing the element after saving the domain file. This happens because virPCIDeviceAddressIsEmpty() can't distinguish between a zeroed address that was set by the user versus an address that wasn't filled at all. I'm afraid this is incomplete. virPCIDeviceAddressIsEmpty is for example used to figure out whether the device needs an address assigned too. So we'll need an extra field to distinguish between empty and zero addresses, which will probably need adjustment in a lot of callers :( Jano Given that all root-ports of PowerNV domains will connect to slot 0 of the PHB, if the PHB controller has index = 0 this scenario will occur every time. This patch aims to alleaviate this behavior by adding a new virDomainDefFormatFlags that will allow an empty address to be formatted in the XML. This flag is then used only when formatting PowerNV root ports. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 25 - src/conf/domain_conf.h | 2 ++ 2 files changed, 26 insertions(+), 1 deletion(-) signature.asc Description: PGP signature
Re: [PATCH v2 21/29] tests: add pnv-phb3-root-port test
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Signed-off-by: Daniel Henrique Barboza --- .../powernv8-root-port.ppc64-latest.args | 35 + tests/qemuxml2argvdata/powernv8-root-port.xml | 17 tests/qemuxml2argvtest.c | 1 + .../powernv8-root-port.ppc64-latest.xml | 39 +++ tests/qemuxml2xmltest.c | 1 + 5 files changed, 93 insertions(+) create mode 100644 tests/qemuxml2argvdata/powernv8-root-port.ppc64-latest.args create mode 100644 tests/qemuxml2argvdata/powernv8-root-port.xml create mode 100644 tests/qemuxml2xmloutdata/powernv8-root-port.ppc64-latest.xml Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH] NEWS: Document domain dirty page rate calculation APIs
From: Hyman Huang(黄勇) The Libvirt API virDomainStartDirtyRateCalc was extended. Document this change. Signed-off-by: Hyman Huang(黄勇) --- NEWS.rst | 14 ++ 1 file changed, 14 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index f545325..b684416 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -22,6 +22,20 @@ v8.1.0 (unreleased) It works on Intel machines as well as recent machines powered by Apple Silicon. QEMU 6.2.0 is needed for Apple Silicon support. + * qemu: Support mode option for dirtyrate calculation + +Introduce ``virDomainDirtyRateCalcFlags`` as parameter of +``virDomainStartDirtyRateCalc``, which is used to specify the mode of +dirty page rate calculation. + +Add ``--mode`` option to ``virsh domdirtyrate-calc``, which can be +either of the following 3 options: +``page-sampling, dirty-bitmap, dirty-ring``. + +Add ``calc_mode`` field for dirtyrate statistics retured by +``virsh domstats --dirtyrate``, also add ``vCPU dirtyrate`` if +``dirty-ring`` mode was used in last measurement. + * **Improvements** * packaging: sysconfig files no longer installed -- 1.8.3.1
Re: [PATCH v2 27/29] conf, qemu: add 'pnv-phb4' controller model name
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Similar to the existing pnv-phb3 device, pnv-phb4 is also an implementation of pcie-root. No user doc is needed in this case since the user doesn't ideally set PCI model names manually. Signed-off-by: Daniel Henrique Barboza --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_domain_address.c | 14 +- src/qemu/qemu_validate.c | 2 ++ 5 files changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 116fedc19e..dce2548825 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2599,6 +2599,7 @@ i82801b11-bridge pnv-phb3 +pnv-phb4 pcie-pci-bridge diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0305c913d9..585e7d9dae 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -441,6 +441,7 @@ VIR_ENUM_IMPL(virDomainControllerPCIModelName, "pnv-phb3-root-port", "pnv-phb3", "pnv-phb4-root-port", + "pnv-phb4", ); VIR_ENUM_IMPL(virDomainControllerModelSCSI, @@ -2456,7 +2457,6 @@ virDomainControllerIsPowerNVPHB(const virDomainControllerDef *cont) name = cont->opts.pciopts.modelName; -/* The actual device used for PHBs is pnv-phb3 */ This removal is suspicious. Better to not add the comment in the first place. if (name != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PNV_PHB3) return false; Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 28/29] domain_conf.c: add pnv-phb4 to ControllerIsPowerNVPHB()
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Update the virDomainControllerIsPowerNVPHB() helper to make the pnv-phb4 device receive the same handling as the existing pnv-phb3. Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 29/29] tests: add PowerNV9 tests
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Since the nuances of PowerNV PHBs and root ports were already handled when adding support for pnv-phb3* devices, we're already set to support PowerNV9 PHBs and root ports as well. Signed-off-by: Daniel Henrique Barboza --- .../powernv9-dupPHBs.ppc64-latest.err | 1 + tests/qemuxml2argvdata/powernv9-dupPHBs.xml | 27 + .../powernv9-root-port.ppc64-latest.args | 35 + tests/qemuxml2argvdata/powernv9-root-port.xml | 17 tests/qemuxml2argvtest.c | 2 + .../powernv9-root-port.ppc64-latest.xml | 39 +++ .../qemuxml2xmloutdata/powernv9-root-port.xml | 36 + tests/qemuxml2xmltest.c | 1 + 8 files changed, 158 insertions(+) create mode 100644 tests/qemuxml2argvdata/powernv9-dupPHBs.ppc64-latest.err create mode 100644 tests/qemuxml2argvdata/powernv9-dupPHBs.xml create mode 100644 tests/qemuxml2argvdata/powernv9-root-port.ppc64-latest.args create mode 100644 tests/qemuxml2argvdata/powernv9-root-port.xml create mode 100644 tests/qemuxml2xmloutdata/powernv9-root-port.ppc64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/powernv9-root-port.xml Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH] qemu: Use virDomainObjCheckActive() more
On a Monday in 2022, Michal Privoznik wrote:
Using the following spatch, I've identified two places which
could be switched from explicit virDomainObjIsActive() +
virReportError() to virDomainObjCheckActive():
@@
expression dom;
@@
if (
-!virDomainObjIsActive(dom)
+virDomainObjCheckActive(dom) < 0
) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not
running"));
...
}
Signed-off-by: Michal Privoznik
---
BTW. if I'm more aggressive and let virReportError() have just any args
then even more places fit the rule (approx. two dozen more).
Had you sent that as a patch, we would have been able to see what kind
of error messages are used in those places without having to run the
spatch.
src/qemu/qemu_driver.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH v2 15/29] introduce virDomainControllerIsPowerNVPHB
Missing conf: prefix On a Tuesday in 2022, Daniel Henrique Barboza wrote: Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 21 + src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 23 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 16/29] conf, qemu: add default 'chip-id' value for pnv-phb3 controllers
On a Tuesday in 2022, Daniel Henrique Barboza wrote: If ommited from the controller definition, chip-id defaults to zero. Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_domain_address.c | 16 +++- src/qemu/qemu_validate.c | 5 + .../powernv8-basic.ppc64-latest.xml | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 14/29] formatdomain.rst: add 'index' semantics for PowerNV domains
On a Tuesday in 2022, Daniel Henrique Barboza wrote: We're going to use the 'targetIndex' element for PowerNV PHBs. Clarify that the same attribute will have a different meaning in this context. Signed-off-by: Daniel Henrique Barboza --- docs/formatdomain.rst | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 1e44d9a987..d700049c1c 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -3896,8 +3896,10 @@ generated by libvirt. :since:`Since 1.2.19 (QEMU only).` the user of the libvirt API to attach host devices to the correct pci-expander-bus when assigning them to the domain). ``index`` - pci-root controllers for pSeries guests use this attribute to record the - order they will show up in the guest. :since:`Since 3.6.0` + pci-root controllers for ``pSeries`` guests use this attribute to record the + order they will show up in the guest (:since:`Since 3.6.0`). :since:`Since 8.1.0`, + ``powernv`` domains uses this attribute to indicate the chip/socket slot a + pcie-root controller will use. The clarification did not help me. I see no difference between this description and the one for the chip-id attribute Jano ``chip-id`` pcie-root controllers for ``powernv`` domains use this attribute to indicate the chip that will own the controller. A chip is equivalent to a CPU socket. -- 2.34.1 signature.asc Description: PGP signature
Re: [PATCH v2 13/29] conf: parse and format
On a Tuesday in 2022, Daniel Henrique Barboza wrote: The 'chip-id' attribute indicates which chip/socket that owns the PowerNV pcie-root controller. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- docs/formatdomain.rst | 6 ++ docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c| 15 +++ src/conf/domain_conf.h| 1 + 4 files changed, 27 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 12/29] domain_conf.c: fix identation in virDomainControllerDefParseXML()
On a Tuesday in 2022, Daniel Henrique Barboza wrote: The identation of VIR_DOMAIN_CONTROLLER_TYPE_PCI elements are in the same level as the parent 'if (def->type == ...TYPE_PCI)' clause, and the closing bracket of this 'if' looks like a misplaced bracket of the 'targetIndex' clause that comes right before it. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/conf/domain_conf.c | 56 +- 1 file changed, 28 insertions(+), 28 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 11/29] conf, qemu: add 'pnv-phb3' PCI controller model name
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Add support for the pcie-root implementation that PowerNV8 domains uses, pnv-phb3. It consists of a PCI model name that isn't supposed to be changed by users, so no doc changes in formatdomain.rst were made. Signed-off-by: Daniel Henrique Barboza --- docs/schemas/domaincommon.rng| 2 ++ src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_domain_address.c | 3 +++ src/qemu/qemu_validate.c | 8 ++-- tests/qemuxml2xmloutdata/powernv8-basic.ppc64-latest.xml | 4 +++- 6 files changed, 16 insertions(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 10/29] conf, qemu: add 'pnv-phb3-root-port' PCI controller model name
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Apart from being usable only with pnv-phb3 PCIE host bridges (to be added soon), this device acts as a regular pcie-root-port but with a specific model name. No doc changes in formatdomain.rst were made because the PCI model name isn't something that users are supposed to be setting or changing. Signed-off-by: Daniel Henrique Barboza --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_domain_address.c | 5 + src/qemu/qemu_validate.c | 12 +++- 5 files changed, 19 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
[PATCH] qemu: Use virDomainObjCheckActive() more
Using the following spatch, I've identified two places which
could be switched from explicit virDomainObjIsActive() +
virReportError() to virDomainObjCheckActive():
@@
expression dom;
@@
if (
-!virDomainObjIsActive(dom)
+virDomainObjCheckActive(dom) < 0
) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not
running"));
...
}
Signed-off-by: Michal Privoznik
---
BTW. if I'm more aggressive and let virReportError() have just any args
then even more places fit the rule (approx. two dozen more).
src/qemu/qemu_driver.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 70d51855b2..e417d358cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19924,9 +19924,7 @@ qemuDomainGetSEVInfo(virQEMUDriver *driver,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
return -1;
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+if (virDomainObjCheckActive(vm) < 0) {
goto endjob;
}
@@ -20744,9 +20742,7 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
+if (virDomainObjCheckActive(vm) < 0) {
goto endjob;
}
--
2.34.1
Re: [PATCH v2 09/29] qemu: introduce QEMU_CAPS_DEVICE_PNV_PHB3
On a Tuesday in 2022, Daniel Henrique Barboza wrote:
QEMU_CAPS_DEVICE_PNV_PHB3 indicates binary support for the pnv-phb3
device, the pcie-root controller for PowerNV8 domains, and also
the pnv-phb3-root-port device, its pcie-root-port device.
This capability is present in QEMU since 5.0.0 but these devices are
user-creatable only after QEMU 6.2.0. This means that probing it as
default will be misleading for users. Instead, let's use
virQEMUCapsInitQMPVersionCaps() to check for the adequate QEMU version
and arch and set it manually.
Suggested-by: Peter Krempa
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_capabilities.c | 19 +++
src/qemu/qemu_capabilities.h | 1 +
.../qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 1 +
3 files changed, 21 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index bb90715569..d60240912c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -662,6 +662,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
/* 420 */
"device.json+hotplug", /* QEMU_CAPS_DEVICE_JSON */
"hvf", /* QEMU_CAPS_HVF */
+ "pnv-phb3", /* QEMU_CAPS_DEVICE_PNV_PHB3 */
);
@@ -1397,6 +1398,17 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] =
{
{ "virtio-vga-gl", QEMU_CAPS_VIRTIO_VGA_GL },
{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
{ "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
+/*
+ * We don't probe the following PowerNV devices:
+ *
If we don't probe them, then this comment does not belong here. Rather
it should be documented in the commit message and the place that sets it
based on the version.
+ * { "pnv-phb3", QEMU_CAPS_DEVICE_PNV_PHB3 },
+ * { "pnv-phb3-root-port", QEMU_CAPS_DEVICE_PNV_PHB3 },
+ *
+ * Because they are present in QEMU binaries since QEMU 5.0.0
+ * but became user creatable only in the QEMU 7.0.0 development
+ * cycle. Their respective capabilities are being set in
+ * virQEMUCapsInitQMPVersionCaps().
Alternatively, you can probe the device here (just "pnv-phb3", no need
to look at both) and clear the capability if QEMU is too old to let the
user create it. That way it will be easier to delete years from now when
we raise minimum QEMU version.
+ */
};
@@ -5231,6 +5243,13 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCaps *qemuCaps)
*/
if (qemuCaps->version < 5002000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_ENABLE_FIPS);
+
+/* PowerNV pnv-phb devices weren't user creatable up to
+ * QEMU 6.2.0. The version value set here was taken from a
+ * binary post 6.2.0 release that has user creatable pnv-phb
+ * support. */
+if (qemuCaps->version >= 6002050 && ARCH_IS_PPC64(qemuCaps->arch))
I'd rather not use the git version in here.
If nothing better, at least rewrite this to: > 6002000
Even better if we waited for 7.0.0 or QEMU had some property we could
look for (looking at QEMU history there were some properties removed
for phb4 as a part of making them user creatable but I don't see
anything for phb3).
+virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_PNV_PHB3);
}
With the comment removed and/or the code probing first then clearing
based on version number:
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH v6 0/8] support mode option for dirtyrate calculation
在 2022/2/21 20:35, Michal Prívozník 写道: On 2/20/22 14:28, huang...@chinatelecom.cn wrote: From: Hyman Huang(黄勇) v6: - rebase on mastr - drop the commit [PATCH v5 1/9] in this patchset, post an extra commit if needed in the future. Please review, thanks ! Regards Yong v5: - [PATCH v5 9/9]: Fix alignment error in qemuDomainGetStatsDirtyRate. v4: - Rebase the master - [PATCH v4 1/9]: Refactor dirty page rate calculation status implementation, display calc_status as string when 'virsh domstats --dirtyrate' api return. - [PATCH v4 3/9]: Adjust the 'cap check' block before BeginJob(). - [PATCH v4 5/9]: Drop the virDomainDirtyRateCalcMode and introduce internal enum qemuMonitorDirtyRateCalcMode instead. - [PATCH v4 6/9]: Code clean. - [PATCH v4 7/9]: Split qemu_driver logic of domdirtyrate-calc virsh api into a separate commit. - [PATCH v4 8/9]: Change the 'mode' parameter usage as --mode=[xxx|yyy], code clean in qemuDomainStartDirtyRateCalc. - [PATCH v4 9/9]: Display calc_mode as string and do code clean in qemuMonitorJSONExtractDirtyRateInfo. Thanks Michal and Peter for reviewing the previous versions, please review. Regards Yong v3: - Rebase the master - [PATCH v2 2/6]: Fix the usage of virQEMUCapsGet - [PATCH v2 4/6]: Fix the cleanup missed in qemuDomainStartDirtyRateCalc - [PATCH v2 4/6]: Move all blocks below ACL check - [PATCH v2 4/6]: Make the qemuMonitorJSONStartDirtyRateCalc cleaner by merging the different case of qemuMonitorJSONMakeCommand - [PATCH v2 4/6]: Code clean, make the error message not be line-broken - [PATCH v2 5/6]: Abstract the enum definition into a standalone commit - [PATCH v2 5/6]: Move the validations code above calculating flags block - [PATCH v2 6/6]: Change the type of 'value' field to unsigned in struct qemuMonitorDirtyRateVcpu - [PATCH v2 6/6]: Rename the enum type qemuMonitorDirtyRateCalcMode to virDomainDirtyRateCalcMode - [PATCH v2 6/6]: Code clean, align the code in qemuDomainGetStatsDirtyRate Thanks Peter for quick and precise response, please review. Regards Yong v2: Rebase master and fix confilicts with commit "Introduce QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC" Thanks ! v1: This patchset introduce mode option as the supplement of qemuDomainStartDirtyRateCalc api, add calc_mode for dirtyrate statistics correspondingly. Qemu add mode parameter for calc-dirty-rate command since >= 6.2.0, either of these three mode "page-sampling, dirty-bitmap, dirty-ring" can be specified when calculating dirty page rate. Page sampling is the original mode and used as default mode. Dirty bitmap mode use kvm log sync api to fetch the dirty-bitmap and count the increased 1 bits number during measurement, thus, calculate the dirty page rate. Dirty ring mode use the dirty-ring mechanism implemented in Qemu which can count the increased dirty page on virtual cpu granularity, thus, calculate the per-vcpu dirty page rate. These three calculation mode can be used in different scenarios, and the dirty-bitmap, dirty-ring mode may be more accurate to a certain degree. So maybe it's time to support the mode option for dirtyrate calculation. This series make main modifications as the following: 1. introduce QEMU_CAPS_CALC_DIRTY_RATE capability to probe calc-dirty-rate command in case of failure since it just introduced since >= 5.2.0 2. introduce QEMU_CAPS_DIRTYRATE_MODE capability to probe mode option of calc-dirty-rate command in case of failure, same as 1. 3. implement mode option support for dirtyrate calculation. Please review, thanks ! Best Regards ! Hyman Huang(黄勇) (8): qemu_capabilities: Introduce QEMU_CAPS_CALC_DIRTY_RATE capability qemu_driver: Probe capability before calculating dirty page rate qemu_capabilities: Introduce QEMU_CAPS_DIRTYRATE_MODE capability include: Introduce virDomainDirtyRateCalcFlags qemu_driver: Add mode parameter to qemuDomainStartDirtyRateCalc qemu_driver: Extend flags parameter of virDomainStartDirtyRateCalc virsh: Add mode option to domdirtyrate-calc virsh api qemu_driver: Add calc_mode for dirtyrate statistics docs/manpages/virsh.rst | 7 ++- include/libvirt/libvirt-domain.h | 13 + src/libvirt-domain.c | 18 +- src/qemu/qemu_capabilities.c | 4 ++ src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c| 56 -- src/qemu/qemu_monitor.c | 5 +- src/qemu/qemu_monitor.h | 27 - src/qemu/qemu_monitor_json.c | 69 ++- src/qemu/qemu_monitor_json.h | 3 +- tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 +
Re: [PATCH v6 7/8] virsh: Add mode option to domdirtyrate-calc virsh api
在 2022/2/21 20:34, Michal Prívozník 写道: On 2/20/22 14:28, huang...@chinatelecom.cn wrote: From: Hyman Huang(黄勇) Extend domdirtyrate-calc virsh api with mode option, either of these three options "page-sampling,dirty-bitmap,dirty-ring" can be specified when calculating dirty page rate. Signed-off-by: Hyman Huang(黄勇) --- docs/manpages/virsh.rst| 7 +-- src/libvirt-domain.c | 12 +++- tools/virsh-completer-domain.c | 17 + tools/virsh-completer-domain.h | 4 tools/virsh-domain.c | 42 +- tools/virsh-domain.h | 9 + 6 files changed, 87 insertions(+), 4 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 429879d..00d21a1 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1717,13 +1717,16 @@ domdirtyrate-calc :: domdirtyrate-calc [--seconds ] + --mode=[page-sampling | dirty-bitmap | dirty-ring] Calculate an active domain's memory dirty rate which may be expected by user in order to decide whether it's proper to be migrated out or not. The ``seconds`` parameter can be used to calculate dirty rate in a specific time which allows 60s at most now and would be default to 1s -if missing. The calculated dirty rate information is available by calling -'domstats --dirtyrate'. +if missing. These three *page-sampling, dirty-bitmap, dirty-ring* modes +are mutually exclusive and alternative when specify calculation mode, +*page-sampling* is the default mode if missing. The calculated dirty +rate information is available by calling 'domstats --dirtyrate'. domdisplay diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 8be2c21..7be4e02 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -13337,7 +13337,7 @@ virDomainGetMessages(virDomainPtr domain, * virDomainStartDirtyRateCalc: * @domain: a domain object * @seconds: specified calculating time in seconds - * @flags: extra flags; not used yet, so callers should always pass 0 + * @flags: bitwise-OR of supported virDomainDirtyRateCalcFlags * * Calculate the current domain's memory dirty rate in next @seconds
Re: [PATCH v2 06/29] qemu_validate.c: enhance 'machine type not supported' message
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Add 'virt type' to allow for an easier time debugging. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_validate.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 05/29] qemu_domain.c: define ISA as default PowerNV serial
On a Tuesday in 2022, Daniel Henrique Barboza wrote: The PowerNV machines uses ISA as the default serial type. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_domain.c | 2 ++ 1 file changed, 2 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v6 2/8] qemu_driver: Probe capability before calculating dirty page rate
在 2022/2/21 20:34, Michal Prívozník 写道:
On 2/20/22 14:28, huang...@chinatelecom.cn wrote:
From: Hyman Huang(黄勇)
Probing QEMU_CAPS_CALC_DIRTY_RATE capability in advance
in case of failure when calculating dirty page rate.
Signed-off-by: Hyman Huang(黄勇)
---
src/qemu/qemu_driver.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f1f708e..9b50f5f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20693,6 +20693,13 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
if (virDomainStartDirtyRateCalcEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+priv = vm->privateData;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CALC_DIRTY_RATE)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("QEMU does not support calculating dirty page rate"));
+goto cleanup;
+}
+
Sorry for not telling you the whole truth. priv->qemuCaps is populated
only when domain is running. Therefore for inactive domains this ^^^
error message is reported which may be a bit misleading. But the fix is
trivial, just check whether domain is active before querying priv-qemuCaps.
Ok, got it :)
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
@@ -20704,7 +20711,6 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
VIR_DEBUG("Calculate dirty rate in next %d seconds", seconds);
-priv = vm->privateData;
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds);
Michal
--
Best regard
Hyman Huang(黄勇)
Re: [PATCH v2 08/29] tests: add basic PowerNV8 test
On a Tuesday in 2022, Daniel Henrique Barboza wrote: We're now able to boot a simple PowerNV8 domain in Libvirt. *libvirt Signed-off-by: Daniel Henrique Barboza --- .../powernv8-basic.ppc64-latest.args | 33 +++ tests/qemuxml2argvdata/powernv8-basic.xml | 16 + tests/qemuxml2argvtest.c | 2 ++ .../powernv8-basic.ppc64-latest.xml | 31 + tests/qemuxml2xmltest.c | 2 ++ 5 files changed, 84 insertions(+) create mode 100644 tests/qemuxml2argvdata/powernv8-basic.ppc64-latest.args create mode 100644 tests/qemuxml2argvdata/powernv8-basic.xml create mode 100644 tests/qemuxml2xmloutdata/powernv8-basic.ppc64-latest.xml Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 07/29] qemu_domain.c: disable default devices for PowerNV machines
On a Tuesday in 2022, Daniel Henrique Barboza wrote: PowerNV domains will support pcie-root devices as PHBs, in a similar fashion as pSeries domains supports the spapr-pci-host-bridge as a pci-root model. Set 'addPCIRoot' to false since we'll not be using this buses in this machine. 'addDefaultMemballoon' is also set to false since the balloon driver wasn't really tested with the PowerNV kernel. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_domain.c | 9 + 1 file changed, 9 insertions(+) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 04/29] qemu_validate.c: use qemuDomainIsPowerPC() in qemuValidateDomainChrDef()
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Both PowerNV and pSeries machines don't support parallel ports. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_validate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 03/29] qemu_domain: turn qemuDomainMachineIsPSeries() static
On a Tuesday in 2022, Daniel Henrique Barboza wrote: The function is now unused outside of qemu_domain.c. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 02/29] qemu_capabilities.c: use 'MachineIsPowerPC' in DeviceDiskCaps
On a Tuesday in 2022, Daniel Henrique Barboza wrote: Both pSeries and PowerNV machines don't have floppy device support. Reviewed-by: Peter Krempa Signed-off-by: Daniel Henrique Barboza --- src/qemu/qemu_capabilities.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 01/29] qemu_domain.c: add PowerNV machine helpers
On a Tuesday in 2022, Daniel Henrique Barboza wrote:
The PowerNV (Power Non-Virtualized) QEMU ppc64 machine is the emulation
of a bare-metal IBM Power host. It follows the OPAL (OpenPower
Abstration Layer) API/ABI, most specifically Skiboot [1]. For now,
Libvirt has support for the pSeries QEMU machine, which is the emulation
s/Libvirt/libvirt/
of a logical partition (guest) that would run on top of a bare-metal
system.
This patch introduces the helpers that are going to be used to add a
basic support for PowerNV domains in Libvirt. Given that there are quite
s/Libvirt/libvirt/
a few similarities in how pSeries and PowerNVv should be handled, we're
also adding a 'qemuDomainIsPowerPC' helper that will be used in those
instances.
[1] https://open-power.github.io/skiboot/doc/overview.html
Reviewed-by: Peter Krempa
Signed-off-by: Daniel Henrique Barboza
---
src/qemu/qemu_domain.c | 41 +
src/qemu/qemu_domain.h | 4
2 files changed, 45 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index aa8f6b8d05..2e21a95f38 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8719,6 +8719,47 @@ qemuDomainIsPSeries(const virDomainDef *def)
}
+/*
+ * The PowerNV machine does not support KVM acceleration in Power
+ * hosts.
We can skip the usual ARCH_IS_PPC64() since this machine
+ * is usable with TCG in all host archs.
I don't understand the comment. We use these ARCH_ macros
to check the guest arch, not the host arch. So the check
should be here too.
+ */
+bool
+qemuDomainIsPowerNV(const virDomainDef *def)
+{
+if (STRPREFIX(def->os.machine, "powernv"))
+return true;
+
+return false;
+}
+
+
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH v6 4/8] include: Introduce virDomainDirtyRateCalcFlags
On 2/20/22 14:28, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇)
>
> Introduce virDomainDirtyRateCalcFlags to get ready for
> adding mode parameter to qemuDomainStartDirtyRateCalc.
>
> Signed-off-by: Hyman Huang(黄勇)
> ---
> include/libvirt/libvirt-domain.h | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/libvirt/libvirt-domain.h
> b/include/libvirt/libvirt-domain.h
> index 8c16598..2d57183 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -5259,6 +5259,19 @@ typedef enum {
> # endif
> } virDomainDirtyRateStatus;
>
> +/**
> + * virDomainDirtyRateCalcFlags:
> + *
> + * Flags OR'ed together to provide specific behaviour when calculating dirty
> page
> + * rate for a Domain
> + *
> + */
> +typedef enum {
> +VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING = 0,/* default mode -
> page-sampling */
> +VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP = 1 << 0,/* dirty-bitmap mode
> */
> +VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING = 1 << 1, /* dirty-ring mode */
> +} virDomainDirtyRateCalcFlags;
> +
> int virDomainStartDirtyRateCalc(virDomainPtr domain,
> int seconds,
> unsigned int flags);
This is the correct places to introduce VIR_EXCLUSIVE_FLAGS_GOTO()
checks to the public API and also describe flags in the API comment (as
done in 7/8).
Michal
Re: [PATCH v6 0/8] support mode option for dirtyrate calculation
On 2/20/22 14:28, huang...@chinatelecom.cn wrote: > From: Hyman Huang(黄勇) > > v6: > - rebase on mastr > - drop the commit [PATCH v5 1/9] in this patchset, post an extra > commit if needed in the future. > > Please review, thanks ! > > Regards > Yong > > v5: > - [PATCH v5 9/9]: Fix alignment error in qemuDomainGetStatsDirtyRate. > > v4: > - Rebase the master > - [PATCH v4 1/9]: Refactor dirty page rate calculation status > implementation, display calc_status as string when > 'virsh domstats --dirtyrate' api return. > - [PATCH v4 3/9]: Adjust the 'cap check' block before BeginJob(). > - [PATCH v4 5/9]: Drop the virDomainDirtyRateCalcMode and introduce > internal enum qemuMonitorDirtyRateCalcMode instead. > - [PATCH v4 6/9]: Code clean. > - [PATCH v4 7/9]: Split qemu_driver logic of domdirtyrate-calc virsh > api into a separate commit. > - [PATCH v4 8/9]: Change the 'mode' parameter usage as --mode=[xxx|yyy], > code clean in qemuDomainStartDirtyRateCalc. > - [PATCH v4 9/9]: Display calc_mode as string and do code clean in > qemuMonitorJSONExtractDirtyRateInfo. > Thanks Michal and Peter for reviewing the previous versions, please review. > > Regards > Yong > > v3: > - Rebase the master > - [PATCH v2 2/6]: Fix the usage of virQEMUCapsGet > - [PATCH v2 4/6]: Fix the cleanup missed in qemuDomainStartDirtyRateCalc > - [PATCH v2 4/6]: Move all blocks below ACL check > - [PATCH v2 4/6]: Make the qemuMonitorJSONStartDirtyRateCalc cleaner by > merging the different case of qemuMonitorJSONMakeCommand > - [PATCH v2 4/6]: Code clean, make the error message not be line-broken > - [PATCH v2 5/6]: Abstract the enum definition into a standalone commit > - [PATCH v2 5/6]: Move the validations code above calculating flags block > - [PATCH v2 6/6]: Change the type of 'value' field to unsigned in > struct qemuMonitorDirtyRateVcpu > - [PATCH v2 6/6]: Rename the enum type qemuMonitorDirtyRateCalcMode to > virDomainDirtyRateCalcMode > - [PATCH v2 6/6]: Code clean, align the code in qemuDomainGetStatsDirtyRate > > Thanks Peter for quick and precise response, please review. > > Regards > > Yong > > v2: > Rebase master and fix confilicts with commit > "Introduce QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC" > > Thanks ! > > v1: > This patchset introduce mode option as the supplement of > qemuDomainStartDirtyRateCalc api, add calc_mode for dirtyrate > statistics correspondingly. > > Qemu add mode parameter for calc-dirty-rate command since >= 6.2.0, > either of these three mode "page-sampling, dirty-bitmap, dirty-ring" > can be specified when calculating dirty page rate. > > Page sampling is the original mode and used as default mode. > > Dirty bitmap mode use kvm log sync api to fetch the dirty-bitmap > and count the increased 1 bits number during measurement, thus, > calculate the dirty page rate. > > Dirty ring mode use the dirty-ring mechanism implemented in Qemu > which can count the increased dirty page on virtual cpu granularity, > thus, calculate the per-vcpu dirty page rate. > > These three calculation mode can be used in different scenarios, and > the dirty-bitmap, dirty-ring mode may be more accurate to a certain > degree. So maybe it's time to support the mode option for dirtyrate > calculation. > > This series make main modifications as the following: > 1. introduce QEMU_CAPS_CALC_DIRTY_RATE capability to probe >calc-dirty-rate command in case of failure since it just >introduced since >= 5.2.0 > > 2. introduce QEMU_CAPS_DIRTYRATE_MODE capability to probe >mode option of calc-dirty-rate command in case of failure, same >as 1. > > 3. implement mode option support for dirtyrate calculation. > > Please review, thanks ! > > Best Regards ! > > Hyman Huang(黄勇) (8): > qemu_capabilities: Introduce QEMU_CAPS_CALC_DIRTY_RATE capability > qemu_driver: Probe capability before calculating dirty page rate > qemu_capabilities: Introduce QEMU_CAPS_DIRTYRATE_MODE capability > include: Introduce virDomainDirtyRateCalcFlags > qemu_driver: Add mode parameter to qemuDomainStartDirtyRateCalc > qemu_driver: Extend flags parameter of virDomainStartDirtyRateCalc > virsh: Add mode option to domdirtyrate-calc virsh api > qemu_driver: Add calc_mode for dirtyrate statistics > > docs/manpages/virsh.rst | 7 ++- > include/libvirt/libvirt-domain.h | 13 + > src/libvirt-domain.c | 18 +- > src/qemu/qemu_capabilities.c | 4 ++ > src/qemu/qemu_capabilities.h | 2 + > src/qemu/qemu_driver.c| 56 -- > src/qemu/qemu_monitor.c | 5 +- > src/qemu/qemu_monitor.h | 27 - > src/qemu/qemu_monitor_json.c
Re: [PATCH v6 8/8] qemu_driver: Add calc_mode for dirtyrate statistics
On 2/20/22 14:28, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇)
>
> Add calc_mode for dirtyrate statistics retured by
> virsh domstats --dirtyrate api, also add vcpu dirtyrate
> if dirty-ring mode was used in last measurement.
>
> Signed-off-by: Hyman Huang(黄勇)
> ---
> src/libvirt-domain.c | 6 +
> src/qemu/qemu_driver.c | 22 +++---
> src/qemu/qemu_monitor.h | 10 +
> src/qemu/qemu_monitor_json.c | 53
>
> 4 files changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 7be4e02..a197618 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11980,6 +11980,12 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> * "dirtyrate.megabytes_per_second" - the calculated memory dirty rate in
> *MiB/s as long long. It is produced
> *only if the calc_status is
> measured.
> + * "dirtyrate.calc_mode" - the calculation mode used last measurement,
> either
> + * of these 3
> 'page-sampling,dirty-bitmap,dirty-ring'
> + * values returned.
> + * "dirtyrate.vcpu..megabytes_per_second" - the calculated memory
> dirty
> + * rate for a virtual cpu
> as
> + * unsigned long long.
> *
> * Note that entire stats groups or individual stat fields may be missing
> from
> * the output in case they are not supported by the given hypervisor, are not
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3297513..de2fdf7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18556,11 +18556,27 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver,
> "dirtyrate.calc_period") < 0)
> return -1;
>
> -if ((info.status == VIR_DOMAIN_DIRTYRATE_MEASURED) &&
> -virTypedParamListAddLLong(params, info.dirtyRate,
> - "dirtyrate.megabytes_per_second") < 0)
> +if (virTypedParamListAddString(params,
> +
> qemuMonitorDirtyRateCalcModeTypeToString(info.mode),
> + "dirtyrate.calc_mode") < 0)
> return -1;
>
> +if (info.status == VIR_DOMAIN_DIRTYRATE_MEASURED) {
> +if (virTypedParamListAddLLong(params, info.dirtyRate,
> + "dirtyrate.megabytes_per_second") < 0)
> +return -1;
> +
> +if (info.mode == QEMU_MONITOR_DIRTYRATE_CALC_MODE_DIRTY_RING) {
> +int i;
Since info.nvcpus is size_t so must be i. We are not guaranteed that
size_t == int.
> +for (i = 0; i < info.nvcpus; i++) {
> +if (virTypedParamListAddULLong(params, info.rates[i].value,
> +
> "dirtyrate.vcpu.%d.megabytes_per_second",
> + info.rates[i].index) < 0)
> +return -1;
> +}
> +}
> +}
> +
> return 0;
> }
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 053479b..2c0eed5 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1553,6 +1553,12 @@ qemuMonitorStartDirtyRateCalc(qemuMonitor *mon,
>int seconds,
>qemuMonitorDirtyRateCalcMode mode);
>
> +typedef struct _qemuMonitorDirtyRateVcpu qemuMonitorDirtyRateVcpu;
> +struct _qemuMonitorDirtyRateVcpu {
> +int index; /* virtual cpu index */
This makes our syntax-check report an error, because it thinks that
'index' is used to access an array (which must then be type of size_t
because that's the only type which is guaranteed to be big enough to
address individual bytes in memory). Let me change the name to 'idx'.
> +unsigned long long value; /* virtual cpu dirty page rate in MB/s */
> +};
> +
> typedef struct _qemuMonitorDirtyRateInfo qemuMonitorDirtyRateInfo;
> struct _qemuMonitorDirtyRateInfo {
> int status; /* the status of last dirtyrate calculation,
> @@ -1560,6 +1566,10 @@ struct _qemuMonitorDirtyRateInfo {
> int calcTime; /* the period of dirtyrate calculation */
> long long startTime;/* the start time of dirtyrate calculation */
> long long dirtyRate;/* the dirtyrate in MiB/s */
> +qemuMonitorDirtyRateCalcMode mode; /* calculation mode used in
> + last measurement */
> +size_t nvcpus; /* number of virtual cpu */
> +qemuMonitorDirtyRateVcpu *rates; /* array of dirty page rate */
> };
>
> int
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index da9a4d9..a07f8c9 100644
> ---
Re: [PATCH v6 2/8] qemu_driver: Probe capability before calculating dirty page rate
On 2/20/22 14:28, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇)
>
> Probing QEMU_CAPS_CALC_DIRTY_RATE capability in advance
> in case of failure when calculating dirty page rate.
>
> Signed-off-by: Hyman Huang(黄勇)
> ---
> src/qemu/qemu_driver.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f1f708e..9b50f5f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20693,6 +20693,13 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
> if (virDomainStartDirtyRateCalcEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> +priv = vm->privateData;
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CALC_DIRTY_RATE)) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("QEMU does not support calculating dirty page
> rate"));
> +goto cleanup;
> +}
> +
Sorry for not telling you the whole truth. priv->qemuCaps is populated
only when domain is running. Therefore for inactive domains this ^^^
error message is reported which may be a bit misleading. But the fix is
trivial, just check whether domain is active before querying priv-qemuCaps.
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> goto cleanup;
>
> @@ -20704,7 +20711,6 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>
> VIR_DEBUG("Calculate dirty rate in next %d seconds", seconds);
>
> -priv = vm->privateData;
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds);
>
Michal
Re: [PATCH v6 7/8] virsh: Add mode option to domdirtyrate-calc virsh api
On 2/20/22 14:28, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇)
>
> Extend domdirtyrate-calc virsh api with mode option, either
> of these three options "page-sampling,dirty-bitmap,dirty-ring"
> can be specified when calculating dirty page rate.
>
> Signed-off-by: Hyman Huang(黄勇)
> ---
> docs/manpages/virsh.rst| 7 +--
> src/libvirt-domain.c | 12 +++-
> tools/virsh-completer-domain.c | 17 +
> tools/virsh-completer-domain.h | 4
> tools/virsh-domain.c | 42
> +-
> tools/virsh-domain.h | 9 +
> 6 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 429879d..00d21a1 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -1717,13 +1717,16 @@ domdirtyrate-calc
> ::
>
> domdirtyrate-calc [--seconds ]
> + --mode=[page-sampling | dirty-bitmap | dirty-ring]
>
> Calculate an active domain's memory dirty rate which may be expected by
> user in order to decide whether it's proper to be migrated out or not.
> The ``seconds`` parameter can be used to calculate dirty rate in a
> specific time which allows 60s at most now and would be default to 1s
> -if missing. The calculated dirty rate information is available by calling
> -'domstats --dirtyrate'.
> +if missing. These three *page-sampling, dirty-bitmap, dirty-ring* modes
> +are mutually exclusive and alternative when specify calculation mode,
> +*page-sampling* is the default mode if missing. The calculated dirty
> +rate information is available by calling 'domstats --dirtyrate'.
>
>
> domdisplay
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 8be2c21..7be4e02 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -13337,7 +13337,7 @@ virDomainGetMessages(virDomainPtr domain,
> * virDomainStartDirtyRateCalc:
> * @domain: a domain object
> * @seconds: specified calculating time in seconds
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of supported virDomainDirtyRateCalcFlags
> *
> * Calculate the current domain's memory dirty rate in next @seconds.
> * The calculated dirty rate information is available by calling
> @@ -13361,6 +13361,16 @@ virDomainStartDirtyRateCalc(virDomainPtr domain,
>
> virCheckReadOnlyGoto(conn->flags, error);
>
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING,
> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING,
> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP,
> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING,
> + error);
> +
> if (conn->driver->domainStartDirtyRateCalc) {
> int ret;
> ret = conn->driver->domainStartDirtyRateCalc(domain, seconds, flags);
> diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
> index b4e744c..1c2df56 100644
> --- a/tools/virsh-completer-domain.c
> +++ b/tools/virsh-completer-domain.c
> @@ -1150,3 +1150,20 @@ virshDomainNumatuneModeCompleter(vshControl *ctl
> G_GNUC_UNUSED,
>
> return ret;
> }
> +
> +
> +char **
> +virshDomainDirtyRateCalcModeCompleter(vshControl *ctl,
> + const vshCmd *cmd,
> + unsigned int flags)
> +{
> +const char *modes[] = {"page-sampling", "dirty-bitmap", "dirty-ring",
> NULL};
> +const char *mode = NULL;
> +
> +virCheckFlags(0, NULL);
> +
> +if (vshCommandOptStringQuiet(ctl, cmd, "mode", ) < 0)
> +return NULL;
> +
> +return virshCommaStringListComplete(mode, modes);
No, the --mode argument supports only one value. It doesn't support
--mode page-sampling,dirty-bitmap,dirty-ring.
> +}
> diff --git a/tools/virsh-completer-domain.h b/tools/virsh-completer-domain.h
> index 94bb3b5..044c675 100644
> --- a/tools/virsh-completer-domain.h
> +++ b/tools/virsh-completer-domain.h
> @@ -186,3 +186,7 @@ char **
> virshDomainNumatuneModeCompleter(vshControl *ctl,
> const vshCmd *cmd,
> unsigned int flags);
> +char **
> +virshDomainDirtyRateCalcModeCompleter(vshControl *ctl,
> + const vshCmd *cmd,
> + unsigned int flags);
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 433ea67..256258b 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -14486,14 +14486,28 @@ static const vshCmdOptDef opts_domdirtyrate_calc[]
> = {
> .help = N_("calculate memory dirty rate within specified seconds,
Re: [PATCH v2 00/29] ppc64 PowerNV machines support
Ping On 1/25/22 17:48, Daniel Henrique Barboza wrote: Hi, This v2 has changes proposed by Peter and Daniel on the v1 review. Peter's reviewed-by tags were kept when applicable. The usability change made is that, now, we'll fail to launch powernv domains that has a pnv-phb* device and it's running a QEMU version that doesn't support these devices to be user creatable. Trying to run the 'powernv8-basic' domain with a QEMU 6.2.0 binary will result in an error: $ sudo ./run tools/virsh define ../tests/qemuxml2argvdata/powernv8-basic.xml error: Failed to define domain from ../tests/qemuxml2argvdata/powernv8-basic.xml error: unsupported configuration: The 'pnv-phb3' device is not supported by this QEMU binary Using the current QEMU upstream will allow the domain to be defined and started. Changes from v1: - all tests are now using CAPS_LATEST; - QEMU_CAPS_DEVICE_PNV_PHB3_ROOT_PORT is no longer being used. Capability for the pnv-phb3-root-port is infered to exist if the capabilitity for its PHB (QEMU_CAPS_DEVICE_PNV_PHB3) is present. Same thing for the case of QEMU_CAPS_DEVICE_PNV_PHB4_ROOT_PORT and QEMU_CAPS_DEVICE_PNV_PHB4; - QEMU_CAPS_DEVICE_PNV_PHB3 and QEMU_CAPS_DEVICE_PNV_PHB4 are no longer being probed. They are being set by hand after checking for QEMU version in virQEMUCapsInitQMPVersionCaps(); - patch 01 (QEMU ppc64 capabilities for qemu 7.0): * dropped since it's already upstream - patch 09 (forbid powernv domains migration): * removed. This will be handled on QEMU side - patch 14 (new): * added documentation of the different semantics 'targetIndex' will have for PowerNV PHBs - several other minor changes suggested by Peter - v1 link: https://listman.redhat.com/archives/libvir-list/2022-January/msg00902.html Daniel Henrique Barboza (29): qemu_domain.c: add PowerNV machine helpers qemu_capabilities.c: use 'MachineIsPowerPC' in DeviceDiskCaps qemu_domain: turn qemuDomainMachineIsPSeries() static qemu_validate.c: use qemuDomainIsPowerPC() in qemuValidateDomainChrDef() qemu_domain.c: define ISA as default PowerNV serial qemu_validate.c: enhance 'machine type not supported' message qemu_domain.c: disable default devices for PowerNV machines tests: add basic PowerNV8 test qemu: introduce QEMU_CAPS_DEVICE_PNV_PHB3 conf, qemu: add 'pnv-phb3-root-port' PCI controller model name conf, qemu: add 'pnv-phb3' PCI controller model name domain_conf.c: fix identation in virDomainControllerDefParseXML() conf: parse and format formatdomain.rst: add 'index' semantics for PowerNV domains introduce virDomainControllerIsPowerNVPHB conf, qemu: add default 'chip-id' value for pnv-phb3 controllers conf, qemu: add default 'targetIndex' value for pnv-phb3 devs qemu_command.c: add command line for the pnv-phb3 device qemu_domain_address.c: change pnv-phb3 minimal downstream slot domain_conf: format pnv-phb3-root-port empty addr tests: add pnv-phb3-root-port test domain_validate.c: allow targetIndex 0 out of idx 0 for PowerNV PHBs domain_conf.c: reject duplicated pnv-phb3 devices qemu: introduce QEMU_CAPS_DEVICE_PNV_PHB4 conf, qemu: add 'pnv-phb4-root-port' PCI controller model name domain_conf.c: add phb4-root-port to IsPowerNVRootPort() conf, qemu: add 'pnv-phb4' controller model name domain_conf.c: add pnv-phb4 to ControllerIsPowerNVPHB() tests: add PowerNV9 tests docs/formatdomain.rst | 12 +- docs/schemas/domaincommon.rng | 10 ++ src/conf/domain_conf.c| 157 ++ src/conf/domain_conf.h| 8 + src/conf/domain_validate.c| 5 +- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 28 +++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_command.c | 21 ++- src/qemu/qemu_domain.c| 56 ++- src/qemu/qemu_domain.h| 4 +- src/qemu/qemu_domain_address.c| 64 ++- src/qemu/qemu_validate.c | 62 ++- .../qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 2 + .../powernv8-basic.ppc64-latest.args | 34 tests/qemuxml2argvdata/powernv8-basic.xml | 16 ++ tests/qemuxml2argvdata/powernv8-dupPHBs.err | 1 + .../powernv8-dupPHBs.ppc64-latest.err | 1 + tests/qemuxml2argvdata/powernv8-dupPHBs.xml | 27 +++ .../powernv8-root-port.ppc64-latest.args | 35 tests/qemuxml2argvdata/powernv8-root-port.xml | 17 ++ .../powernv8-two-sockets.ppc64-latest.args| 35 .../qemuxml2argvdata/powernv8-two-sockets.xml | 26 +++ .../powernv9-dupPHBs.ppc64-latest.err | 1 + tests/qemuxml2argvdata/powernv9-dupPHBs.xml | 27 +++ .../powernv9-root-port.ppc64-latest.args | 35
Re: [PATCH v2 0/4] qemu: Introduce 'virDomainQemuMonitorCommandWithFiles'
On a Monday in 2022, Peter Krempa wrote: I was doing some tests with 'add-fd' and 'getfd' and made this to help me. v2: - add API support for passing FDs back, but for now we don't have use for it Peter Krempa (4): virnetmessage: Introduce virNetMessageClearFDs lib: Introduce 'virDomainQemuMonitorCommandWithFiles' virsh: Implement support for virDomainQemuMonitorCommandWithFiles qemu: Implement qemuDomainQemuMonitorCommandWithFiles docs/manpages/virsh.rst | 6 +- include/libvirt/libvirt-qemu.h | 8 +++ src/driver-hypervisor.h | 10 src/libvirt-qemu.c | 89 + src/libvirt_qemu.syms | 5 ++ src/libvirt_remote.syms | 1 + src/qemu/qemu_driver.c | 42 -- src/qemu/qemu_monitor.c | 7 ++- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c| 6 +- src/qemu/qemu_monitor_json.h| 2 + src/qemu/qemu_monitor_text.c| 8 +-- src/qemu_protocol-structs | 9 +++ src/remote/qemu_protocol.x | 20 ++- src/remote/remote_daemon_dispatch.c | 62 src/remote/remote_driver.c | 57 ++ src/rpc/virnetmessage.c | 9 ++- src/rpc/virnetmessage.h | 1 + tools/virsh-domain.c| 19 +- 19 files changed, 345 insertions(+), 17 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: Call for GSoC and Outreachy project ideas for summer 2022
On 2/21/22 10:36, Michal Prívozník wrote: Indeed. Libvirt's participating on its own since 2016, IIRC. Since we're still in org acceptance phase we have some time to decide this, actually. We can do the final decision after participating orgs are announced. My gut feeling says that it's going to be more work on QEMU side which would warrant it to be on the QEMU ideas page. There are multiple projects that can be done on this topic, some QEMU-only, some Libvirt-only. For now I would announce the project on the Libvirt side (and focus on those projects) since you are comentoring. Paolo
Re: [PATCH v5 1/9] qemu: Refactor dirty page rate calculation status implementation
On Thu, Feb 17, 2022 at 05:19:32PM +0100, Michal Prívozník wrote:
> On 2/16/22 01:28, huang...@chinatelecom.cn wrote:
> > From: Hyman Huang(黄勇)
> >
> > For any virTypedParameter API normal practice is to use a string
> > to expose the data, not the rather enum integer value.
> >
> > So let's drop the virDomainDirtyRateStatus in public header file
> > and introduce internal enum def qemuMonitorDirtyRateStatus to
> > describe the dirty page rate calculation status.
> >
> > Signed-off-by: Hyman Huang(黄勇)
> > ---
> > include/libvirt/libvirt-domain.h | 18 --
> > src/libvirt-domain.c | 4 ++--
> > src/qemu/qemu_driver.c | 9 ++---
> > src/qemu/qemu_monitor.h | 18 --
> > src/qemu/qemu_monitor_json.c | 4 ++--
> > 5 files changed, 26 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/libvirt/libvirt-domain.h
> > b/include/libvirt/libvirt-domain.h
> > index 8c16598..bf4746a 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -5241,24 +5241,6 @@ int virDomainGetMessages(virDomainPtr domain,
> > char ***msgs,
> > unsigned int flags);
> >
> > -/**
> > - * virDomainDirtyRateStatus:
> > - *
> > - * Details on the cause of a dirty rate calculation status.
> > - */
> > -typedef enum {
> > -VIR_DOMAIN_DIRTYRATE_UNSTARTED = 0, /* the dirtyrate calculation has
> > - not been started */
> > -VIR_DOMAIN_DIRTYRATE_MEASURING = 1, /* the dirtyrate calculation is
> > - measuring */
> > -VIR_DOMAIN_DIRTYRATE_MEASURED = 2, /* the dirtyrate calculation is
> > - completed */
> > -
> > -# ifdef VIR_ENUM_SENTINELS
> > -VIR_DOMAIN_DIRTYRATE_LAST
> > -# endif
> > -} virDomainDirtyRateStatus;
> > -
> > int virDomainStartDirtyRateCalc(virDomainPtr domain,
> > int seconds,
> > unsigned int flags);
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index b8a6f10..f24d072 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -11947,8 +11947,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> > * this format:
> > *
> > * "dirtyrate.calc_status" - the status of last memory dirty rate
> > calculation,
> > - * returned as int from
> > virDomainDirtyRateStatus
> > - * enum.
> > + * either of these 3
> > 'unstarted,measuring,measured'
> > + * values returned.
> > * "dirtyrate.calc_start_time" - the start time of last memory dirty
> > rate
> > * calculation as long long.
> > * "dirtyrate.calc_period" - the period of last memory dirty rate
> > calculation
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index f262020..a22646c 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -18525,6 +18525,8 @@ qemuDomainGetStatsDirtyRateMon(virQEMUDriver
> > *driver,
> > return ret;
> > }
> >
> > +VIR_ENUM_DECL(qemuMonitorDirtyRateStatus);
> > +
> > static int
> > qemuDomainGetStatsDirtyRate(virQEMUDriver *driver,
> > virDomainObj *dom,
> > @@ -18539,8 +18541,9 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver,
> > if (qemuDomainGetStatsDirtyRateMon(driver, dom, ) < 0)
> > return -1;
> >
> > -if (virTypedParamListAddInt(params, info.status,
> > -"dirtyrate.calc_status") < 0)
> > +if (virTypedParamListAddString(params,
> > +
> > qemuMonitorDirtyRateStatusTypeToString(info.status),
> > + "dirtyrate.calc_status") < 0)
>
> I worry that this change is not backwards compatible. I mean, what if I
> had an app that fetches dirtyrate.calc_status, expecting it to be an int?
>
> In some languages this may be less of a problem (e.g. in python) but
> still, I would have to make changes to my app only because of this patch.
Yes, that'll certainly break the Go bindings as we expose everything
in stronglky typed structs.
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: Call for GSoC and Outreachy project ideas for summer 2022
On 2/19/22 14:46, Stefan Hajnoczi wrote: > On Fri, 18 Feb 2022 at 16:03, Paolo Bonzini wrote: >> >> On 2/18/22 12:39, Michal Prívozník wrote: >>> On 2/17/22 18:52, Paolo Bonzini wrote: I would like to co-mentor one or more projects about adding more statistics to Mark Kanda's newly-born introspectable statistics subsystem in QEMU (https://patchew.org/QEMU/20220215150433.2310711-1-mark.ka...@oracle.com/), for example integrating "info blockstats"; and/or, to add matching functionality to libvirt. However, I will only be available for co-mentoring unfortunately. >>> >>> I'm happy to offer my helping hand in this. I mean the libvirt part, >>> since I am a libvirt developer. >>> >>> I believe this will be listed in QEMU's ideas list, right? >> >> Does Libvirt participate to GSoC as an independent organization this >> year? If not, I'll add it as a Libvirt project on the QEMU ideas list. > > Libvirt participates as its own GSoC organization. If a project has > overlap we could do it in either org, or have a QEMU project and a > libvirt project if the amount of work is large enough. Indeed. Libvirt's participating on its own since 2016, IIRC. Since we're still in org acceptance phase we have some time to decide this, actually. We can do the final decision after participating orgs are announced. My gut feeling says that it's going to be more work on QEMU side which would warrant it to be on the QEMU ideas page. But anyway, we can wait an see. Meanwhile, as Stefan is trying to compile the list for org application, I'm okay with putting it onto QEMU's list. Michal
Re: [libvirt PATCH] docs: Fix template matching in page.xsl
On 2/21/22 09:42, Martin Kletzander wrote: > Our last default template had a match of "node()" which incidentally matched > everything, including text nodes. Since this has the same priority according > to > the XSLT spec, section 5.5: > > https://www.w3.org/TR/1999/REC-xslt-19991116#conflict > > this is an error. Also according to the same spec section, the XSLT processor > may signal the error or pick the last rule. > > This was uncovered with libxslt 1.1.35 which contains the following commit: > > > https://gitlab.gnome.org/GNOME/libxslt/-/commit/b0074eeca3c6b21b4da14fdf712b853900c51635 > > which makes the build fail with: > > runtime error: file ../docs/page.xsl line 223 element element > xsl:element: The effective name '' is not a valid QName. > > because our last rule also matches text nodes and we are trying to extract the > node name out of them. > > To fix this we change the match to "*" which only matches elements and not all > the nodes, and to avoid any possible errors with different XSLT processors we > also bump the priority of the match="text()" rule a little higher, just in > case > someone needs to use an XSLT processor that chooses signalling the error > instead > of the optional recovery. > > https://bugs.gentoo.org/833586 > > Signed-off-by: Martin Kletzander > --- > docs/page.xsl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Michal Privoznik Michal
[libvirt PATCH] docs: Fix template matching in page.xsl
Our last default template had a match of "node()" which incidentally matched everything, including text nodes. Since this has the same priority according to the XSLT spec, section 5.5: https://www.w3.org/TR/1999/REC-xslt-19991116#conflict this is an error. Also according to the same spec section, the XSLT processor may signal the error or pick the last rule. This was uncovered with libxslt 1.1.35 which contains the following commit: https://gitlab.gnome.org/GNOME/libxslt/-/commit/b0074eeca3c6b21b4da14fdf712b853900c51635 which makes the build fail with: runtime error: file ../docs/page.xsl line 223 element element xsl:element: The effective name '' is not a valid QName. because our last rule also matches text nodes and we are trying to extract the node name out of them. To fix this we change the match to "*" which only matches elements and not all the nodes, and to avoid any possible errors with different XSLT processors we also bump the priority of the match="text()" rule a little higher, just in case someone needs to use an XSLT processor that chooses signalling the error instead of the optional recovery. https://bugs.gentoo.org/833586 Signed-off-by: Martin Kletzander --- docs/page.xsl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/page.xsl b/docs/page.xsl index fd67918d3b08..72a6fa084235 100644 --- a/docs/page.xsl +++ b/docs/page.xsl @@ -215,11 +215,11 @@ - + - + -- 2.35.1
[PATCH v2 3/4] virsh: Implement support for virDomainQemuMonitorCommandWithFiles
Signed-off-by: Peter Krempa
---
docs/manpages/virsh.rst | 6 +-
tools/virsh-domain.c| 19 ++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 429879d2dd..1e6914a438 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -7894,7 +7894,8 @@ qemu-monitor-command
::
- qemu-monitor-command domain { [--hmp] | [--pretty] [--return-value] }
command...
+ qemu-monitor-command domain { [--hmp] | [--pretty] [--return-value] }
+ [--pass-fds N,M,...] command...
Send an arbitrary monitor command *command* to domain *domain* through the
QEMU monitor. The results of the command will be printed on stdout.
@@ -7927,6 +7928,9 @@ extracted rather than passing through the full reply from
QEMU.
If *--hmp* is passed, the command is considered to be a human monitor command
and libvirt will automatically convert it into QMP and convert the result back.
+If *--pass-fds* is specified, the argument is a comma separated list
+of open file descriptors which should be passed on to qemu along with the
+command.
qemu-agent-command
--
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 433ea674b5..1ebfb3a567 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9721,6 +9721,11 @@ static const vshCmdOptDef opts_qemu_monitor_command[] = {
.type = VSH_OT_BOOL,
.help = N_("extract the value of the 'return' key from the returned
string")
},
+{.name = "pass-fds",
+ .type = VSH_OT_STRING,
+ .completer = virshCompleteEmpty,
+ .help = N_("pass file descriptors N,M,... along with the command")
+},
{.name = "cmd",
.type = VSH_OT_ARGV,
.flags = VSH_OFLAG_REQ,
@@ -9819,6 +9824,8 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd)
bool returnval = vshCommandOptBool(cmd, "return-value");
virJSONValue *formatjson;
g_autofree char *jsonstr = NULL;
+g_autofree int *fds = NULL;
+size_t nfds = 0;
VSH_EXCLUSIVE_OPTIONS("hmp", "pretty");
VSH_EXCLUSIVE_OPTIONS("hmp", "return-value");
@@ -9838,9 +9845,19 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd)
return NULL;
}
-if (virDomainQemuMonitorCommand(dom, monitor_cmd, , flags) < 0)
+if (virshFetchPassFdsList(ctl, cmd, , ) < 0)
return false;
+if (fds) {
+if (virDomainQemuMonitorCommandWithFiles(dom, monitor_cmd, nfds, fds,
+ NULL, NULL,
+ , flags) < 0)
+return false;
+} else {
+if (virDomainQemuMonitorCommand(dom, monitor_cmd, , flags) < 0)
+return false;
+}
+
if (returnval || pretty) {
resultjson = virJSONValueFromString(result);
--
2.35.1
[PATCH v2 4/4] qemu: Implement qemuDomainQemuMonitorCommandWithFiles
Add support for sending one FD from the client along with a monitor
command so that it's possible to use 'getfd' and 'add-fd' to use FDs
passed from the client with other QMP commands.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_driver.c | 42
src/qemu/qemu_monitor.c | 7 +++---
src/qemu/qemu_monitor.h | 1 +
src/qemu/qemu_monitor_json.c | 6 --
src/qemu/qemu_monitor_json.h | 2 ++
src/qemu/qemu_monitor_text.c | 8 +++
6 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f1f708e511..706c47792c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13921,21 +13921,44 @@ qemuDomainBackupGetXMLDesc(virDomainPtr domain,
}
-static int qemuDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd,
-char **result, unsigned int flags)
+static int
+qemuDomainQemuMonitorCommandWithFiles(virDomainPtr domain,
+ const char *cmd,
+ unsigned int ninfds,
+ int *infds,
+ unsigned int *noutfds,
+ int **outfds,
+ char **result,
+ unsigned int flags)
{
virQEMUDriver *driver = domain->conn->privateData;
virDomainObj *vm = NULL;
int ret = -1;
qemuDomainObjPrivate *priv;
bool hmp;
+int fd = -1;
virCheckFlags(VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP, -1);
+/* currently we don't pass back any fds */
+if (outfds)
+*outfds = NULL;
+if (noutfds)
+*noutfds = 0;
+
+if (ninfds > 1) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("at most 1 fd can be passed to qemu along with a
command"));
+return -1;
+}
+
+if (ninfds == 1)
+fd = infds[0];
+
if (!(vm = qemuDomainObjFromDomain(domain)))
goto cleanup;
-if (virDomainQemuMonitorCommandEnsureACL(domain->conn, vm->def) < 0)
+if (virDomainQemuMonitorCommandWithFilesEnsureACL(domain->conn, vm->def) <
0)
goto cleanup;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
@@ -13951,7 +13974,7 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr
domain, const char *cmd,
hmp = !!(flags & VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP);
qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp);
+ret = qemuMonitorArbitraryCommand(priv->mon, cmd, fd, result, hmp);
qemuDomainObjExitMonitor(driver, vm);
endjob:
@@ -13963,6 +13986,16 @@ static int qemuDomainQemuMonitorCommand(virDomainPtr
domain, const char *cmd,
}
+static int
+qemuDomainQemuMonitorCommand(virDomainPtr domain,
+ const char *cmd,
+ char **result,
+ unsigned int flags)
+{
+return qemuDomainQemuMonitorCommandWithFiles(domain, cmd, 0, NULL, NULL,
NULL, result, flags);
+}
+
+
static int
qemuDomainOpenConsole(virDomainPtr dom,
const char *dev_name,
@@ -20873,6 +20906,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainRevertToSnapshot = qemuDomainRevertToSnapshot, /* 0.8.0 */
.domainSnapshotDelete = qemuDomainSnapshotDelete, /* 0.8.0 */
.domainQemuMonitorCommand = qemuDomainQemuMonitorCommand, /* 0.8.3 */
+.domainQemuMonitorCommandWithFiles =
qemuDomainQemuMonitorCommandWithFiles, /* 8.1.0 */
.domainQemuAttach = NULL, /* 0.9.4 - 5.5.0 */
.domainQemuAgentCommand = qemuDomainQemuAgentCommand, /* 0.10.0 */
.connectDomainQemuMonitorEventRegister =
qemuConnectDomainQemuMonitorEventRegister, /* 1.2.3 */
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index dc81e41783..4fcba588c7 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3042,17 +3042,18 @@ qemuMonitorDrivePivot(qemuMonitor *mon,
int
qemuMonitorArbitraryCommand(qemuMonitor *mon,
const char *cmd,
+int fd,
char **reply,
bool hmp)
{
-VIR_DEBUG("cmd=%s, reply=%p, hmp=%d", cmd, reply, hmp);
+VIR_DEBUG("cmd=%s, fd=%d, reply=%p, hmp=%d", cmd, fd, reply, hmp);
QEMU_CHECK_MONITOR(mon);
if (hmp)
-return qemuMonitorJSONHumanCommand(mon, cmd, reply);
+return qemuMonitorJSONHumanCommand(mon, cmd, fd, reply);
else
-return qemuMonitorJSONArbitraryCommand(mon, cmd, reply);
+return qemuMonitorJSONArbitraryCommand(mon, cmd, fd, reply);
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1d21183d82..c8e8036f26 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1088,6 +1088,7 @@ char *qemuMonitorDiskNameLookup(qemuMonitor *mon,
[PATCH v2 2/4] lib: Introduce 'virDomainQemuMonitorCommandWithFiles'
This API has the same semantics as 'virDomainQemuMonitorCommand' but
accepts file descriptors which are then forwarded to qemu.
Signed-off-by: Peter Krempa
---
include/libvirt/libvirt-qemu.h | 8 +++
src/driver-hypervisor.h | 10
src/libvirt-qemu.c | 89 +
src/libvirt_qemu.syms | 5 ++
src/qemu_protocol-structs | 9 +++
src/remote/qemu_protocol.x | 20 ++-
src/remote/remote_daemon_dispatch.c | 62
src/remote/remote_driver.c | 57 ++
8 files changed, 259 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-qemu.h b/include/libvirt/libvirt-qemu.h
index 0cc2872821..98f2ac3823 100644
--- a/include/libvirt/libvirt-qemu.h
+++ b/include/libvirt/libvirt-qemu.h
@@ -37,6 +37,14 @@ typedef enum {
int virDomainQemuMonitorCommand(virDomainPtr domain, const char *cmd,
char **result, unsigned int flags);
+int virDomainQemuMonitorCommandWithFiles(virDomainPtr domain,
+ const char *cmd,
+ unsigned int ninfiles,
+ int *infiles,
+ unsigned int *noutfiles,
+ int **outfiles,
+ char **result,
+ unsigned int flags);
virDomainPtr virDomainQemuAttach(virConnectPtr domain,
unsigned int pid_value,
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index c83fb648a2..4423eb0885 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -874,6 +874,15 @@ typedef int
const char *cmd,
char **result,
unsigned int flags);
+typedef int
+(*virDrvDomainQemuMonitorCommandWithFiles)(virDomainPtr domain,
+ const char *cmd,
+ unsigned int ninfiles,
+ int *infiles,
+ unsigned int *noutfiles,
+ int **outfiles,
+ char **result,
+ unsigned int flags);
typedef char *
(*virDrvDomainQemuAgentCommand)(virDomainPtr domain,
@@ -1597,6 +1606,7 @@ struct _virHypervisorDriver {
virDrvDomainRevertToSnapshot domainRevertToSnapshot;
virDrvDomainSnapshotDelete domainSnapshotDelete;
virDrvDomainQemuMonitorCommand domainQemuMonitorCommand;
+virDrvDomainQemuMonitorCommandWithFiles domainQemuMonitorCommandWithFiles;
virDrvDomainQemuAttach domainQemuAttach;
virDrvDomainQemuAgentCommand domainQemuAgentCommand;
virDrvConnectDomainQemuMonitorEventRegister
connectDomainQemuMonitorEventRegister;
diff --git a/src/libvirt-qemu.c b/src/libvirt-qemu.c
index 1afb5fe529..9e80577b56 100644
--- a/src/libvirt-qemu.c
+++ b/src/libvirt-qemu.c
@@ -96,6 +96,95 @@ virDomainQemuMonitorCommand(virDomainPtr domain, const char
*cmd,
}
+/**
+ * virDomainQemuMonitorCommandWithFiles:
+ * @domain: a domain object
+ * @cmd: the qemu monitor command string
+ * @ninfiles: number of filedescriptors passed in @infiles
+ * @infiles: filedescriptors to be passed to qemu with the command
+ * @noutfiles: if non-NULL filled with number of returned file descriptors
+ * @outfiles: if non-NULL filled with an array of returned file descriptors
+ * @result: a string returned by @cmd
+ * @flags: bitwise-or of supported virDomainQemuMonitorCommandFlags
+ *
+ * This API is QEMU specific, so it will only work with hypervisor
+ * connections to the QEMU driver with local connections using the unix socket.
+ *
+ * Send an arbitrary monitor command @cmd with file descriptors @infiles to
+ * @domain through the qemu monitor and optionally return file descriptors via
+ * @outfiles. There are several requirements to safely and successfully use
+ * this API:
+ *
+ * - A @cmd that queries state without making any modifications is safe
+ * - A @cmd that alters state that is also tracked by libvirt is unsafe,
+ * and may cause libvirtd to crash
+ * - A @cmd that alters state not tracked by the current version of
+ * libvirt is possible as a means to test new qemu features before
+ * they have support in libvirt, but no guarantees are made to safety
+ *
+ * If VIR_DOMAIN_QEMU_MONITOR_COMMAND_HMP is set, the command is
+ * considered to be a human monitor command and libvirt will automatically
+ * convert it into QMP if needed. In that case the @result will also
+ * be converted back from QMP.
+ *
+ * If successful, @result will be filled with the string output of the
+ * @cmd, and the caller must free this string.
+ *
+ * Returns 0 in
[PATCH v2 1/4] virnetmessage: Introduce virNetMessageClearFDs
The helper splits out the clearing of the FDs transacted inside a
virNetMessage.
APIs transacting FDs both from and to the client at the same time will
need to clear the FDs stored in virNetMessage as the structure is
re-used for the reply and without clearing the list of FDs we'd return
the FDs sent by the client in addition to the new FDs sent by the API.t
Signed-off-by: Peter Krempa
---
src/libvirt_remote.syms | 1 +
src/rpc/virnetmessage.c | 9 -
src/rpc/virnetmessage.h | 1 +
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 2fa46e5e0c..f0f90815cf 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -102,6 +102,7 @@ virNetDaemonUpdateServices;
# rpc/virnetmessage.h
virNetMessageAddFD;
virNetMessageClear;
+virNetMessageClearFDs;
virNetMessageClearPayload;
virNetMessageDecodeHeader;
virNetMessageDecodeLength;
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
index ca11f1688e..221da7c59b 100644
--- a/src/rpc/virnetmessage.c
+++ b/src/rpc/virnetmessage.c
@@ -48,7 +48,7 @@ virNetMessage *virNetMessageNew(bool tracked)
void
-virNetMessageClearPayload(virNetMessage *msg)
+virNetMessageClearFDs(virNetMessage *msg)
{
size_t i;
@@ -58,6 +58,13 @@ virNetMessageClearPayload(virNetMessage *msg)
msg->donefds = 0;
msg->nfds = 0;
VIR_FREE(msg->fds);
+}
+
+
+void
+virNetMessageClearPayload(virNetMessage *msg)
+{
+virNetMessageClearFDs(msg);
msg->bufferOffset = 0;
msg->bufferLength = 0;
diff --git a/src/rpc/virnetmessage.h b/src/rpc/virnetmessage.h
index aadf1b69b0..8f878962f8 100644
--- a/src/rpc/virnetmessage.h
+++ b/src/rpc/virnetmessage.h
@@ -49,6 +49,7 @@ struct _virNetMessage {
virNetMessage *virNetMessageNew(bool tracked);
+void virNetMessageClearFDs(virNetMessage *msg);
void virNetMessageClearPayload(virNetMessage *msg);
void virNetMessageClear(virNetMessage *);
--
2.35.1
[PATCH v2 0/4] qemu: Introduce 'virDomainQemuMonitorCommandWithFiles'
I was doing some tests with 'add-fd' and 'getfd' and made this to help me. v2: - add API support for passing FDs back, but for now we don't have use for it Peter Krempa (4): virnetmessage: Introduce virNetMessageClearFDs lib: Introduce 'virDomainQemuMonitorCommandWithFiles' virsh: Implement support for virDomainQemuMonitorCommandWithFiles qemu: Implement qemuDomainQemuMonitorCommandWithFiles docs/manpages/virsh.rst | 6 +- include/libvirt/libvirt-qemu.h | 8 +++ src/driver-hypervisor.h | 10 src/libvirt-qemu.c | 89 + src/libvirt_qemu.syms | 5 ++ src/libvirt_remote.syms | 1 + src/qemu/qemu_driver.c | 42 -- src/qemu/qemu_monitor.c | 7 ++- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c| 6 +- src/qemu/qemu_monitor_json.h| 2 + src/qemu/qemu_monitor_text.c| 8 +-- src/qemu_protocol-structs | 9 +++ src/remote/qemu_protocol.x | 20 ++- src/remote/remote_daemon_dispatch.c | 62 src/remote/remote_driver.c | 57 ++ src/rpc/virnetmessage.c | 9 ++- src/rpc/virnetmessage.h | 1 + tools/virsh-domain.c| 19 +- 19 files changed, 345 insertions(+), 17 deletions(-) -- 2.35.1
