Re: [PATCH] Add support for network backed NVRAM
On Tue, Jun 21, 2022 at 5:53 PM Rohit Kumar wrote: > > Signed-off-by: Rohit Kumar Reviewed-by: Ani Sinha > --- > NEWS.rst | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index 9a8624a97e..9766dbe1df 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -17,6 +17,11 @@ v8.5.0 (unreleased) > > * **New features** > > + * qemu: Introduce support for network backed NVRAM > + > +Users can now use remote store NVRAM image by specifying attribute I would say 'newly introduced attribute' so that it is clear what already exists and what was newly introduced. > +`type='network'` with `` element. > + >* qemu: Add support for post-copy migration recovery > > A new ``VIR_MIGRATE_POSTCOPY_RESUME`` flag (``virsh migrate > --postcopy-resume``) > -- > 2.25.1 >
Re: [PATCH v3 0/5] Introduce network backed NVRAM
On Mon, Jun 13, 2022 at 10:15 AM Rohit Kumar wrote: > > > On 03/06/22 5:21 pm, Peter Krempa wrote: > > On Thu, Jun 02, 2022 at 16:50:42 +0530, Rohit Kumar wrote: > >> On 17/05/22 8:55 pm, Peter Krempa wrote: > >>> On Mon, May 16, 2022 at 16:03:21 +0530, Rohit Kumar wrote: > Ping. > > Hi Peter, > can you please take a look on this v3 patchset ? > >>> Yes, don't worry and please be patient. There are some intricacies that > >>> are not properly handled by your series and rather than me using you as > >>> a email operated remote editor I'll have a proper look and fix certain > >>> aspects. > >> > >> Peter, > >> Thanks a lot for taking the time to look at patches and volunteering to > >> fix the bits that are not handled properly in this patch. Do let me know > >> if there is anything I can assist you with, I would be happy to be a part > >> on this. > > Hi Rohit, > > > > I've finally managed to make all adjustments I'd be proposing. I've > > posted the series: > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2022-2DJune_232167.html=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=yp7ODgTWyCPv4vE2exi6noSZ2iNzPUlNTiNvSh5A4l9SQR1Av9Z_g-GX2oAQM89T=kKHpBT0wOfZSxDEjkUfvIo6xT7S1-GPz0KJGNmzkk0o= > > > > alternatively you can fetch it at: > > > > git fetch > > https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_pipo.sk_libvirt.git=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=yp7ODgTWyCPv4vE2exi6noSZ2iNzPUlNTiNvSh5A4l9SQR1Av9Z_g-GX2oAQM89T=hA8g0qVcO6Et2emoksZNeXyQf_p5k9ik72d4bnOPWyw= > > network-nvram2 > > > > Please give it a try and test all your cases. I've ran out of time to > > give it testing this week. Ideally also test the old cases. > > > Hi Peter, > > Thanks for the working on this. > > I have tested the v4 patchset with tests related to UEFI and secureboot. > All tests are passing. > > > > > You can now also provide a 'NEWS.rst' entry I've stripped out of the > > original series as we require that changes to 'NEWS.rst' are done > > separately from anything else. > > Sure. I will send a patch soon to update 'NEWS.rst'. When you send updated patchset with NEWS.rst, please add Tested-by: Rohit Kumar tag to the patches.
Re: [libvirt RFCv8 12/27] qemu: capabilities: add multifd to the probed migration capabilities
Qemu folks,
It seems we do officially support multifd from version 4.0 :
commit cbfd6c957a4437d4759ca660e621daa381bf2898
Author: Juan Quintela
Date: Wed Feb 6 13:54:06 2019 +0100
multifd: Drop x-
We make it supported from now on.
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Markus Armbruster
Signed-off-by: Juan Quintela
$ git tag --contains cbfd6c957a4437d4759ca660e621daa381bf2898 | sort
-V | grep -v list | head -1
v4.0.0
Yet it seems we continue to prefix the migration property with "x-"
(x-multifd). This prop was added here and we have continued to use it
as is:
commit 30126bbf1f7fcad0bf4c65b01a21ff22a36a9759
Author: Juan Quintela
Date: Thu Jan 14 12:23:00 2016 +0100
migration: Add multifd capability
Signed-off-by: Juan Quintela
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Peter Xu
Reviewed-by: Daniel P. Berrange
Can anyone explain why?
On Sat, May 7, 2022 at 7:13 PM Claudio Fontana wrote:
>
> Signed-off-by: Claudio Fontana
other than the question above,
Reviewed-by: Ani Sinha
> ---
> src/qemu/qemu_capabilities.c | 4
> src/qemu/qemu_capabilities.h | 3 +++
> tests/qemucapabilitiesdata/caps_4.0.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.0.0.riscv32.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.0.0.riscv64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.1.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.2.0.s390x.xml | 1 +
> tests/qemucapabilitiesdata/caps_4.2.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.0.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.0.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.0.0.riscv64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.0.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.1.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.2.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.2.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.2.0.riscv64.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.2.0.s390x.xml | 1 +
> tests/qemucapabilitiesdata/caps_5.2.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.0.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.0.0.s390x.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.0.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.2.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.2.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
> tests/qemucapabilitiesdata/caps_7.0.0.aarch64.xml | 1 +
> tests/qemucapabilitiesdata/caps_7.0.0.ppc64.xml | 1 +
> tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
> 34 files changed, 39 insertions(+)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 1ed4cda7f0..581b6a40df 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -672,6 +672,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"virtio-iommu-pci", /* QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI */
>"virtio-iommu.boot-bypass", /*
> QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS */
>"virtio-net.rss", /* QEMU_CAPS_VIRTIO_NET_RSS */
> +
> + /* 430 */
> + "migrate-multifd", /* QEMU_CAPS_MIGRATE_MULTIFD */
> );
>
>
> @@ -1230,6 +1233,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
>
> struct virQEMUCapsStringFlags virQEMUCapsMigration[] = {
> { "rdma-pin-all", QEMU_CAPS_MIGRATE_RDMA },
> +{ "multifd", QEMU_CAPS_MIGRATE_MULTIFD },
> };
>
> /* Use virQEMUCapsQMPSchemaQueries for querying parameters of events */
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 9b240e47fb..b089f83da1 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -648,6 +648,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
> syntax-check */
> QEMU_CAPS_VIRTIO_IOMMU_BOOT_BYPASS, /* virtio-iommu.boot-bypass */
> QEMU_CAPS_VIRTIO_NET_RSS, /* virtio-net rss feature */
>
> +/* 430 */
> +QEMU_CAPS_MIGRATE_MULTIFD, /* migrate can set multifd parameter */
> +
> QEMU_CAPS_LAST /* this must always be the last item */
> } virQEMUCapsFlags;
>
> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.aar
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin wrote: > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote: > > > > Change log: > > v2: rebased the patchset. Laine's response is appended at the end. > > > > I am re-introducing the patchset for which got > > reverted here few months back: > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html > > > > The reason for the reversal was that there seemed to be some > > instability/issues around the use of the qemu commandline which this > > patchset tries to support. In particular, some guest operating systems > > did not like the way QEMU was trying to disable native hotplug on pcie > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism > > using which we disable native hotplug. As I understand, we do not have > > any reported issues so far in 6.2 around this area. QEMU will enter a > > soft feature freeze in the first week of march in prep for 7.0 release. > > Right. But unfortunately we did not yet really work on > a sane interface for this. Ok so are we going to do something about this? I am still very unclear as to what would be a sane interface both for i440fx and q35 (pci and pcie). > > The way I see it, at high level we thinkably need two flags > - disable ACPI hotplug > - enable native hotplug (maybe separately for pci and pcie?) > > and with both enabled guests actually can switch between > the two. > > This will at least reflect the hardware, so has a chance to be > stable. > > The big question however would be what is the actual use-case. > Without that this begs the question of why do we bother at all. > To allow hotplug of bridges? If it is really necessary for us then > we should think hard about questions that surround this: > > - how does one hotplug a pcie switch? > - any way to use e.g. dynamic ACPI to support hotplug of bridges? > - do we want to bite the bullet and create an option for management > to fully control guest memory layout including all pci devices? > > > > > Libvirt is also entering a new release cycle phaze. Hence, I am > > introducing this patchset early enough in the release cycles so that if > > we do see any issues on the qemu side during the rc0, rc1 cycles and if > > reversal of this patchset is again required, it can be done in time > > before the next libvirt release end of March. > > > > All the patches in this series had been previously reviewed. Some > > subsequent fixes were made after my initial patches were pushed. I have > > squashed all those fixes and consolidated them into four patches. I have > > also updated the documentation to reflect the new changes from the QEMU > > side and rebased my changes fixing the tests in the process. > > > > What changed in QEMU post version 6.1 ? > > = > > > > We have made basically two major changes in QEMU. First is this change: > > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 > > Author: Julia Suvorova > > Date: Fri Nov 12 06:08:56 2021 -0500 > > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC > > > > There are two ways to enable ACPI PCI Hot-plug: > > > > * Disable the Hot-plug Capable bit on PCIe slots. > > > > This was the first approach which led to regression [1-2], as > > I/O space for a port is allocated only when it is hot-pluggable, > > which is determined by HPC bit. > > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC > > method. > > > > This removes the (future) ability of hot-plugging switches with PCIe > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged > > bridges. If the user wants to explicitely use this feature, they can > > disable ACPI PCI Hot-plug with: > > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off > > > > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug > > instead of PCIe Native. > > > > [1] https://gitlab.com/qemu-project/qemu/-/issues/641 > > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 > > > > Signed-off-by: Julia Suvorova > > Signed-off-by: Igor Mammedov > > Message-Id: <2022110857.3116853-5-imamm...@redhat.com> > > Reviewed-by: Ani Sinha > > Reviewed-by: Michael S. Tsirkin > > Signed-off-by: Michael S. Tsirkin > > > > > > The patch description says it all. Instead of masking out t
Re: [PATCH] tests: qemucapabilities: Update qemu caps dump for the qemu-7.0.0 release on x86_64
On Wed, Apr 20, 2022 at 4:46 PM Igor Mammedov wrote:
>
> On Wed, 20 Apr 2022 13:02:12 +0200
> Peter Krempa wrote:
>
> > Few minor changes in qemu since the last update:
> > - PIIX4_PM gained 'x-not-migrate-acpi-index' property
>
> do you do this for just for every new property?
> (nothing outside of QEMU needs to know about x-not-migrate-acpi-index,
> unless one is interested in whether it works or not)
Any new property that is prefixed with "x-" should not be exposed
through libvirt. They are experimental, unstable and can change any
time in the future.
>
> > - 'cocoa' display and corresponding props (not present in this build)
> >
> > Changes in build:
> > - dbus display driver re-enabled
> > - gtk display support re-disabled
> > - xen support re-disabled
> >
> > Signed-off-by: Peter Krempa
> > ---
> > .../caps_7.0.0.x86_64.replies | 583 --
> > .../caps_7.0.0.x86_64.xml | 10 +-
> > 2 files changed, 257 insertions(+), 336 deletions(-)
> >
> > diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
> > b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
> > index d1f453dcca..620442704a 100644
> > --- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
> > +++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.replies
> > @@ -17,11 +17,11 @@
> > {
> >"return": {
> > "qemu": {
> > - "micro": 92,
> > - "minor": 2,
> > - "major": 6
> > + "micro": 0,
> > + "minor": 0,
> > + "major": 7
> > },
> > -"package": "v7.0.0-rc2"
> > +"package": "v7.0.0"
> >},
> >"id": "libvirt-2"
> > }
> > @@ -5119,10 +5119,6 @@
> >"name": "135",
> >"tag": "type",
> >"variants": [
> > -{
> > - "case": "gtk",
> > - "type": "358"
> > -},
> > {
> >"case": "curses",
> >"type": "360"
> > @@ -5131,6 +5127,10 @@
> >"case": "egl-headless",
> >"type": "361"
> > },
> > +{
> > + "case": "dbus",
> > + "type": "362"
> > +},
> > {
> >"case": "default",
> >"type": "0"
> > @@ -10498,6 +10498,10 @@
> >"case": "qemu-vdagent",
> >"type": "518"
> > },
> > +{
> > + "case": "dbus",
> > + "type": "519"
> > +},
> > {
> >"case": "vc",
> >"type": "520"
> > @@ -11756,9 +11760,6 @@
> > {
> >"name": "none"
> > },
> > -{
> > - "name": "gtk"
> > -},
> > {
> >"name": "sdl"
> > },
> > @@ -11770,17 +11771,20 @@
> > },
> > {
> >"name": "spice-app"
> > +},
> > +{
> > + "name": "dbus"
> > }
> >],
> >"meta-type": "enum",
> >"values": [
> > "default",
> > "none",
> > -"gtk",
> > "sdl",
> > "egl-headless",
> > "curses",
> > -"spice-app"
> > +"spice-app",
> > +"dbus"
> >]
> > },
> > {
> > @@ -16067,6 +16071,9 @@
> > {
> >"name": "qemu-vdagent"
> > },
> > +{
> > + "name": "dbus"
> > +},
> > {
> >"name": "vc"
> > },
> > @@ -16097,6 +16104,7 @@
> > "spicevmc",
> > "spiceport",
> > "qemu-vdagent",
> > +"dbus",
> > "vc",
> > "ringbuf",
> > "memory"
> > @@ -16202,6 +16210,16 @@
> >],
> >"meta-type": "object"
> > },
> > +{
> > + "name": "519",
> > + "members": [
> > +{
> > + "name": "data",
> > + "type": "618"
> > +}
> > + ],
> > + "meta-type": "object"
> > +},
> > {
> >"name": "520",
> >"members": [
> > @@ -18460,6 +18478,26 @@
> >],
> >"meta-type": "object"
> > },
> > +{
> > + "name": "618",
> > + "members": [
> > +{
> > + "name": "logfile",
> > + "default": null,
> > + "type": "str"
> > +},
> > +{
> > + "name": "logappend",
> > + "default": null,
> > + "type": "bool"
> > +},
> > +{
> > + "name": "name",
> > + "type": "str"
> > +}
> > + ],
> > + "meta-type": "object"
> > +},
> > {
> >"name": "619",
> >"members": [
> > @@ -20363,10 +20401,6 @@
> >"name": "acpi-erst",
> >"parent": "pci-device"
> > },
> > -{
> > - "name": "virtio-crypto-device",
> > - "parent": "virtio-device"
> > -},
> > {
> >"name": "isa-applesmc",
> >"parent": "isa-device"
> > @@ -20379,49 +20413,53 @@
> >"name": "vhost-user-input-pci",
> >"parent": "vhost-user-input-pci-base-type"
> > },
> > +{
>
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Apr 12, 2022 at 12:41 PM Michael S. Tsirkin wrote: > > On Tue, Apr 12, 2022 at 09:52:26AM +0530, Ani Sinha wrote: > > On Tue, Apr 12, 2022 at 9:50 AM Ani Sinha wrote: > > > > > > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin > > > wrote: > > > > > > > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote: > > > > > > > > > > Change log: > > > > > v2: rebased the patchset. Laine's response is appended at the end. > > > > > > > > > > I am re-introducing the patchset for which got > > > > > reverted here few months back: > > > > > > > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html > > > > > > > > > > The reason for the reversal was that there seemed to be some > > > > > instability/issues around the use of the qemu commandline which this > > > > > patchset tries to support. In particular, some guest operating systems > > > > > did not like the way QEMU was trying to disable native hotplug on pcie > > > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism > > > > > using which we disable native hotplug. As I understand, we do not have > > > > > any reported issues so far in 6.2 around this area. QEMU will enter a > > > > > soft feature freeze in the first week of march in prep for 7.0 > > > > > release. > > > > > > > > Right. But unfortunately we did not yet really work on > > > > a sane interface for this. > > > > > > > > The way I see it, at high level we thinkably need two flags > > > > - disable ACPI hotplug > > > > - enable native hotplug (maybe separately for pci and pcie?) > > I still think this is the case. > > > > pci does not have native hotplug. so this would be applicable only for > > > q35. For i440fx we have two separate flags already to disable acpi > > > hotplug, one for root bus and another for bridges. > > > > > > > > > > > and with both enabled guests actually can switch between > > > > the two. > > > > > > > > This will at least reflect the hardware, so has a chance to be > > > > stable. > > > > > > > > The big question however would be what is the actual use-case. > > > > Without that this begs the question of why do we bother at all. > > > > > > To me the main motivation is as I have described here: > > > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html > > > > > > One concrete example of why one might still want to use native hotplug > > > with > > > pcie-root-port controller is the fact that we are still discovering > > > issues with > > > acpi hotplug on PCIE. One such issue is: > > > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html > > This one was fixed, right? yes > > > > > Another reason is that users have been using native hotplug on pcie root > > > ports > > > up until now. They have built and tested their systems based on native > > > hotplug. > > > They may not want to suddenly move to acpi based hotplug just because it > > > is now > > > the default in qemu. Supporting the option to chose one or the other > > > through > > > libvirt makes things simpler for end users. > > > > Essentially what I do not like is that we are imposing acpi hotplug on > > q35 for the entire community without giving them a choice to revert > > back to native hotplug though libvirt. > > The reason qemu did it is because it was expected it's more or less > transparent. Barring bugs bug hey, there's always bugs with any change. Right and it takes time to say confidently that we have ironed out almost all the issues. > > > > > > > > To allow hotplug of bridges? If it is really necessary for us then > > > > we should think hard about questions that surround this: > > > > > > > > - how does one hotplug a pcie switch? > > > > - any way to use e.g. dynamic ACPI to support hotplug of bridges? > > > > - do we want to bite the bullet and create an option for management > > > > to fully control guest memory layout including all pci devices? > > > > > > > > > > > > > > > > > Libvirt is also entering a new release cycle phaze. Hence, I am > > > > > introducing
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Apr 12, 2022 at 12:34 PM Michael S. Tsirkin wrote: > > On Tue, Apr 12, 2022 at 09:50:15AM +0530, Ani Sinha wrote: > > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin wrote: > > > > > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote: > > > > > > > > Change log: > > > > v2: rebased the patchset. Laine's response is appended at the end. > > > > > > > > I am re-introducing the patchset for which got > > > > reverted here few months back: > > > > > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html > > > > > > > > The reason for the reversal was that there seemed to be some > > > > instability/issues around the use of the qemu commandline which this > > > > patchset tries to support. In particular, some guest operating systems > > > > did not like the way QEMU was trying to disable native hotplug on pcie > > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism > > > > using which we disable native hotplug. As I understand, we do not have > > > > any reported issues so far in 6.2 around this area. QEMU will enter a > > > > soft feature freeze in the first week of march in prep for 7.0 release. > > > > > > Right. But unfortunately we did not yet really work on > > > a sane interface for this. > > > > > > The way I see it, at high level we thinkably need two flags > > > - disable ACPI hotplug > > > - enable native hotplug (maybe separately for pci and pcie?) > > > > pci does not have native hotplug. > > shpc? > > > so this would be applicable only for > > q35. For i440fx we have two separate flags already to disable acpi > > hotplug, one for root bus and another for bridges. > > > > > > > > and with both enabled guests actually can switch between > > > the two. > > > > > > This will at least reflect the hardware, so has a chance to be > > > stable. > > > > > > The big question however would be what is the actual use-case. > > > Without that this begs the question of why do we bother at all. > > > > To me the main motivation is as I have described here: > > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html > > > > One concrete example of why one might still want to use native hotplug with > > pcie-root-port controller is the fact that we are still discovering issues > > with > > acpi hotplug on PCIE. One such issue is: > > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html > > Another reason is that users have been using native hotplug on pcie root > > ports > > up until now. They have built and tested their systems based on native > > hotplug. > > They may not want to suddenly move to acpi based hotplug just because it is > > now > > the default in qemu. Supporting the option to chose one or the other through > > libvirt makes things simpler for end users. > > To work around bugs then. Bugs that we might have not discovered yet. Let's look at end user scenario. Users might have spent enormous QA time to stabilize and test hotplug using pcie native. pcie native has been around for a while and has been thus getting tested widely. acpi was recently introduced. I think we should at least give end users an option to opt out of acpi hotplug if they wanted to. Also opt out gracefully, through a mechanism from libvirt other than passthrough qemu commandline. > > > > To allow hotplug of bridges? If it is really necessary for us then > > > we should think hard about questions that surround this: > > > > > > - how does one hotplug a pcie switch? > > > - any way to use e.g. dynamic ACPI to support hotplug of bridges? > > > - do we want to bite the bullet and create an option for management > > > to fully control guest memory layout including all pci devices? > > > > > > > > > > > > > Libvirt is also entering a new release cycle phaze. Hence, I am > > > > introducing this patchset early enough in the release cycles so that if > > > > we do see any issues on the qemu side during the rc0, rc1 cycles and if > > > > reversal of this patchset is again required, it can be done in time > > > > before the next libvirt release end of March. > > > > > > > > All the patches in this series had been previously reviewed. Some > > > > subsequent fixes were made after my initial patches were pushed. I have &g
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
>
> align the "save" with the "restore" code,
> by only using the wrapper when using --bypass-cache.
>
> This avoids a copy, resulting in better performance.
>
> Signed-off-by: Claudio Fontana
Reviewed-by: Ani Sinha
> ---
> src/qemu/qemu_saveimage.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 4fd4c5cfcd..5ea1b2fbcc 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) <
> 0)
> goto cleanup;
>
> -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> -goto cleanup;
> +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> +if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> +goto cleanup;
> +}
>
> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> goto cleanup;
> --
> 2.34.1
>
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Wed, Apr 13, 2022 at 11:57 AM Claudio Fontana wrote:
>
> On 4/13/22 6:13 AM, Ani Sinha wrote:
> > On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana wrote:
> >>
> >> On 4/12/22 7:23 PM, Ani Sinha wrote:
> >>> On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha wrote:
> >>>
> >>>> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
> >>>>>
> >>>>> align the "save" with the "restore" code,
> >>>>> by only using the wrapper when using --bypass-cache.
> >>>>>
> >>>>> This avoids a copy, resulting in better performance.
> >>>>>
> >>>>> Signed-off-by: Claudio Fontana
> >>>>> ---
> >>>>> src/qemu/qemu_saveimage.c | 6 --
> >>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> >>>>> index 4fd4c5cfcd..5ea1b2fbcc 100644
> >>>>> --- a/src/qemu/qemu_saveimage.c
> >>>>> +++ b/src/qemu/qemu_saveimage.c
> >>>>> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> >>>>> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
> >>>> fd) < 0)
> >>>>> goto cleanup;
> >>>>>
> >>>>> -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> >>>>> -goto cleanup;
> >>>>> +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> >>>>> +if (!(wrapperFd = virFileWrapperFdNew(, path,
> >>>>> wrapperFlags)))
> >>>>> +goto cleanup;
> >>>>> +}
> >>>>
> >>>> There is an obvious issue with this code. We are trying to close and
> >>>> free a file descriptor that we have not opened when
> >>>> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> >>>
> >>>
> >>> I meant *not* set in flags.
> >>
> >> I don't think so. I don't think it is obvious, so can you be more specific?
> >
> > See
> >if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> > goto cleanup;
> >
> > and under cleanup:
> >if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> > ret = -1;
> >
> > if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> > function calls use wrapperFd initialized to NULL.
> > I think this patch would be incomplete if you do not handle all these
> > scenarios.
> >
> >>
> >> From the function header comments it seems lecit and defined to call the
> >> function with NULL:
> >>
> >> /**
> >> * virFileWrapperFdClose:
> >> * @wfd: fd wrapper, or NULL
> >> *
> >> * If @wfd is valid, then ensure that I/O has completed, which may
> >> * include reaping a child process. Return 0 if all data for the
> >> * wrapped fd is complete, or -1 on failure with an error emitted.
> >> * This function intentionally returns 0 when @wfd is NULL, so that
> >> * callers can conditionally create a virFileWrapperFd wrapper but
> >> * unconditionally call the cleanup code. To avoid deadlock, only
> >> * call this after closing the fd resulting from virFileWrapperFdNew().
> >> *
> >> * This function can be safely called multiple times on the same @wfd.
> >> */
> >>
> >> Seems it has been specifically designed to simplify situations like this.
> >>
>
> Hi Ani, did you read the comments snippet above?
yes and I get that the called functions today are written with safety
for such cases in mind (check for null file descriptors). However, I
still think it is quite unclean (and looks buggy) to do stuff that
does not apply. With this change, wrapperFd only used when
VIR_DOMAIN_SAVE_BYPASS_CACHE is set and we should avoid any operations
on the fd when VIR_DOMAIN_SAVE_BYPASS_CACHE is not set.
That being said, previous to this patch, under error conditions from
call to virFileWrapperFdNew() also would call close() and free()
without checks for null fd from cleanup. So I guess we are not at
least making things any worse by not checking for null fd from the
caller of those two functions.
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Wed, Apr 13, 2022 at 9:43 AM Ani Sinha wrote:
>
> On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana wrote:
> >
> > On 4/12/22 7:23 PM, Ani Sinha wrote:
> > > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha wrote:
> > >
> > >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
> > >>>
> > >>> align the "save" with the "restore" code,
> > >>> by only using the wrapper when using --bypass-cache.
> > >>>
> > >>> This avoids a copy, resulting in better performance.
> > >>>
> > >>> Signed-off-by: Claudio Fontana
> > >>> ---
> > >>> src/qemu/qemu_saveimage.c | 6 --
> > >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> > >>> index 4fd4c5cfcd..5ea1b2fbcc 100644
> > >>> --- a/src/qemu/qemu_saveimage.c
> > >>> +++ b/src/qemu/qemu_saveimage.c
> > >>> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> > >>> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
> > >> fd) < 0)
> > >>> goto cleanup;
> > >>>
> > >>> -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> > >>> -goto cleanup;
> > >>> +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> > >>> +if (!(wrapperFd = virFileWrapperFdNew(, path,
> > >>> wrapperFlags)))
> > >>> +goto cleanup;
> > >>> +}
> > >>
> > >> There is an obvious issue with this code. We are trying to close and
> > >> free a file descriptor that we have not opened when
> > >> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> > >
> > >
> > > I meant *not* set in flags.
> >
> > I don't think so. I don't think it is obvious, so can you be more specific?
>
> See
>if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> goto cleanup;
>
> and under cleanup:
>if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
> ret = -1;
Also see virFileWrapperFdFree() in cleanup.
>
> if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
> function calls use wrapperFd initialized to NULL.
> I think this patch would be incomplete if you do not handle all these
> scenarios.
>
> >
> > From the function header comments it seems lecit and defined to call the
> > function with NULL:
> >
> > /**
> > * virFileWrapperFdClose:
> > * @wfd: fd wrapper, or NULL
> > *
> > * If @wfd is valid, then ensure that I/O has completed, which may
> > * include reaping a child process. Return 0 if all data for the
> > * wrapped fd is complete, or -1 on failure with an error emitted.
> > * This function intentionally returns 0 when @wfd is NULL, so that
> > * callers can conditionally create a virFileWrapperFd wrapper but
> > * unconditionally call the cleanup code. To avoid deadlock, only
> > * call this after closing the fd resulting from virFileWrapperFdNew().
> > *
> > * This function can be safely called multiple times on the same @wfd.
> > */
> >
> > Seems it has been specifically designed to simplify situations like this.
> >
> > Thanks,
> >
> > Claudio
> >
> >
> > >
> > >
> > >>
> > >>>
> > >>> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> > >>> goto cleanup;
> > >>> --
> > >>> 2.34.1
> > >>>
> > >>
> > >
> >
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Wed, Apr 13, 2022 at 12:49 AM Claudio Fontana wrote:
>
> On 4/12/22 7:23 PM, Ani Sinha wrote:
> > On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha wrote:
> >
> >> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
> >>>
> >>> align the "save" with the "restore" code,
> >>> by only using the wrapper when using --bypass-cache.
> >>>
> >>> This avoids a copy, resulting in better performance.
> >>>
> >>> Signed-off-by: Claudio Fontana
> >>> ---
> >>> src/qemu/qemu_saveimage.c | 6 --
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> >>> index 4fd4c5cfcd..5ea1b2fbcc 100644
> >>> --- a/src/qemu/qemu_saveimage.c
> >>> +++ b/src/qemu/qemu_saveimage.c
> >>> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> >>> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
> >> fd) < 0)
> >>> goto cleanup;
> >>>
> >>> -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> >>> -goto cleanup;
> >>> +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> >>> +if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> >>> +goto cleanup;
> >>> +}
> >>
> >> There is an obvious issue with this code. We are trying to close and
> >> free a file descriptor that we have not opened when
> >> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
> >
> >
> > I meant *not* set in flags.
>
> I don't think so. I don't think it is obvious, so can you be more specific?
See
if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
goto cleanup;
and under cleanup:
if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0)
ret = -1;
if VIR_DOMAIN_SAVE_BYPASS_CACHE not set in flags, both the above
function calls use wrapperFd initialized to NULL.
I think this patch would be incomplete if you do not handle all these scenarios.
>
> From the function header comments it seems lecit and defined to call the
> function with NULL:
>
> /**
> * virFileWrapperFdClose:
> * @wfd: fd wrapper, or NULL
> *
> * If @wfd is valid, then ensure that I/O has completed, which may
> * include reaping a child process. Return 0 if all data for the
> * wrapped fd is complete, or -1 on failure with an error emitted.
> * This function intentionally returns 0 when @wfd is NULL, so that
> * callers can conditionally create a virFileWrapperFd wrapper but
> * unconditionally call the cleanup code. To avoid deadlock, only
> * call this after closing the fd resulting from virFileWrapperFdNew().
> *
> * This function can be safely called multiple times on the same @wfd.
> */
>
> Seems it has been specifically designed to simplify situations like this.
>
> Thanks,
>
> Claudio
>
>
> >
> >
> >>
> >>>
> >>> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> >>> goto cleanup;
> >>> --
> >>> 2.34.1
> >>>
> >>
> >
>
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Tue, Apr 12, 2022 at 10:09 PM Ani Sinha wrote:
> On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
> >
> > align the "save" with the "restore" code,
> > by only using the wrapper when using --bypass-cache.
> >
> > This avoids a copy, resulting in better performance.
> >
> > Signed-off-by: Claudio Fontana
> > ---
> > src/qemu/qemu_saveimage.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> > index 4fd4c5cfcd..5ea1b2fbcc 100644
> > --- a/src/qemu/qemu_saveimage.c
> > +++ b/src/qemu/qemu_saveimage.c
> > @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> > if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def,
> fd) < 0)
> > goto cleanup;
> >
> > -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> > -goto cleanup;
> > +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> > +if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> > +goto cleanup;
> > +}
>
> There is an obvious issue with this code. We are trying to close and
> free a file descriptor that we have not opened when
> VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
I meant *not* set in flags.
>
> >
> > if (virQEMUSaveDataWrite(data, fd, path) < 0)
> > goto cleanup;
> > --
> > 2.34.1
> >
>
Re: [libvirt RFC] qemu_saveimage: only use wrapper when using --bypass-cache
On Tue, Apr 12, 2022 at 2:48 PM Claudio Fontana wrote:
>
> align the "save" with the "restore" code,
> by only using the wrapper when using --bypass-cache.
>
> This avoids a copy, resulting in better performance.
>
> Signed-off-by: Claudio Fontana
> ---
> src/qemu/qemu_saveimage.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 4fd4c5cfcd..5ea1b2fbcc 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -289,8 +289,10 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) <
> 0)
> goto cleanup;
>
> -if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> -goto cleanup;
> +if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> +if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> +goto cleanup;
> +}
There is an obvious issue with this code. We are trying to close and
free a file descriptor that we have not opened when
VIR_DOMAIN_SAVE_BYPASS_CACHE is set in flags.
>
> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> goto cleanup;
> --
> 2.34.1
>
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Apr 12, 2022 at 9:50 AM Ani Sinha wrote: > > On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin wrote: > > > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote: > > > > > > Change log: > > > v2: rebased the patchset. Laine's response is appended at the end. > > > > > > I am re-introducing the patchset for which got > > > reverted here few months back: > > > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html > > > > > > The reason for the reversal was that there seemed to be some > > > instability/issues around the use of the qemu commandline which this > > > patchset tries to support. In particular, some guest operating systems > > > did not like the way QEMU was trying to disable native hotplug on pcie > > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism > > > using which we disable native hotplug. As I understand, we do not have > > > any reported issues so far in 6.2 around this area. QEMU will enter a > > > soft feature freeze in the first week of march in prep for 7.0 release. > > > > Right. But unfortunately we did not yet really work on > > a sane interface for this. > > > > The way I see it, at high level we thinkably need two flags > > - disable ACPI hotplug > > - enable native hotplug (maybe separately for pci and pcie?) > > pci does not have native hotplug. so this would be applicable only for > q35. For i440fx we have two separate flags already to disable acpi > hotplug, one for root bus and another for bridges. > > > > > and with both enabled guests actually can switch between > > the two. > > > > This will at least reflect the hardware, so has a chance to be > > stable. > > > > The big question however would be what is the actual use-case. > > Without that this begs the question of why do we bother at all. > > To me the main motivation is as I have described here: > https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html > > One concrete example of why one might still want to use native hotplug with > pcie-root-port controller is the fact that we are still discovering issues > with > acpi hotplug on PCIE. One such issue is: > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html > Another reason is that users have been using native hotplug on pcie root ports > up until now. They have built and tested their systems based on native > hotplug. > They may not want to suddenly move to acpi based hotplug just because it is > now > the default in qemu. Supporting the option to chose one or the other through > libvirt makes things simpler for end users. Essentially what I do not like is that we are imposing acpi hotplug on q35 for the entire community without giving them a choice to revert back to native hotplug though libvirt. > > > To allow hotplug of bridges? If it is really necessary for us then > > we should think hard about questions that surround this: > > > > - how does one hotplug a pcie switch? > > - any way to use e.g. dynamic ACPI to support hotplug of bridges? > > - do we want to bite the bullet and create an option for management > > to fully control guest memory layout including all pci devices? > > > > > > > > > Libvirt is also entering a new release cycle phaze. Hence, I am > > > introducing this patchset early enough in the release cycles so that if > > > we do see any issues on the qemu side during the rc0, rc1 cycles and if > > > reversal of this patchset is again required, it can be done in time > > > before the next libvirt release end of March. > > > > > > All the patches in this series had been previously reviewed. Some > > > subsequent fixes were made after my initial patches were pushed. I have > > > squashed all those fixes and consolidated them into four patches. I have > > > also updated the documentation to reflect the new changes from the QEMU > > > side and rebased my changes fixing the tests in the process. > > > > > > What changed in QEMU post version 6.1 ? > > > = > > > > > > We have made basically two major changes in QEMU. First is this change: > > > > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 > > > Author: Julia Suvorova > > > Date: Fri Nov 12 06:08:56 2021 -0500 > > > > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC > > > > > > There are two ways to enable ACPI PCI Hot-plug: &
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Mar 8, 2022 at 10:28 PM Michael S. Tsirkin wrote: > > On Tue, Mar 08, 2022 at 10:15:49PM +0530, Ani Sinha wrote: > > > > Change log: > > v2: rebased the patchset. Laine's response is appended at the end. > > > > I am re-introducing the patchset for which got > > reverted here few months back: > > > > https://www.spinics.net/linux/fedora/libvir/msg224089.html > > > > The reason for the reversal was that there seemed to be some > > instability/issues around the use of the qemu commandline which this > > patchset tries to support. In particular, some guest operating systems > > did not like the way QEMU was trying to disable native hotplug on pcie > > root ports. Subsequently, in QEMU 6.2, we have changed our mechanism > > using which we disable native hotplug. As I understand, we do not have > > any reported issues so far in 6.2 around this area. QEMU will enter a > > soft feature freeze in the first week of march in prep for 7.0 release. > > Right. But unfortunately we did not yet really work on > a sane interface for this. > > The way I see it, at high level we thinkably need two flags > - disable ACPI hotplug > - enable native hotplug (maybe separately for pci and pcie?) pci does not have native hotplug. so this would be applicable only for q35. For i440fx we have two separate flags already to disable acpi hotplug, one for root bus and another for bridges. > > and with both enabled guests actually can switch between > the two. > > This will at least reflect the hardware, so has a chance to be > stable. > > The big question however would be what is the actual use-case. > Without that this begs the question of why do we bother at all. To me the main motivation is as I have described here: https://listman.redhat.com/archives/libvir-list/2021-October/msg00068.html One concrete example of why one might still want to use native hotplug with pcie-root-port controller is the fact that we are still discovering issues with acpi hotplug on PCIE. One such issue is: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02146.html Another reason is that users have been using native hotplug on pcie root ports up until now. They have built and tested their systems based on native hotplug. They may not want to suddenly move to acpi based hotplug just because it is now the default in qemu. Supporting the option to chose one or the other through libvirt makes things simpler for end users. > To allow hotplug of bridges? If it is really necessary for us then > we should think hard about questions that surround this: > > - how does one hotplug a pcie switch? > - any way to use e.g. dynamic ACPI to support hotplug of bridges? > - do we want to bite the bullet and create an option for management > to fully control guest memory layout including all pci devices? > > > > > Libvirt is also entering a new release cycle phaze. Hence, I am > > introducing this patchset early enough in the release cycles so that if > > we do see any issues on the qemu side during the rc0, rc1 cycles and if > > reversal of this patchset is again required, it can be done in time > > before the next libvirt release end of March. > > > > All the patches in this series had been previously reviewed. Some > > subsequent fixes were made after my initial patches were pushed. I have > > squashed all those fixes and consolidated them into four patches. I have > > also updated the documentation to reflect the new changes from the QEMU > > side and rebased my changes fixing the tests in the process. > > > > What changed in QEMU post version 6.1 ? > > = > > > > We have made basically two major changes in QEMU. First is this change: > > > > (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 > > Author: Julia Suvorova > > Date: Fri Nov 12 06:08:56 2021 -0500 > > > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC > > > > There are two ways to enable ACPI PCI Hot-plug: > > > > * Disable the Hot-plug Capable bit on PCIe slots. > > > > This was the first approach which led to regression [1-2], as > > I/O space for a port is allocated only when it is hot-pluggable, > > which is determined by HPC bit. > > > > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC > > method. > > > > This removes the (future) ability of hot-plugging switches with PCIe > > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged > > bridges. If the user wants to explicitely use this feature, they can > > disable
Re: [PATCH v2 0/8] Introduce network backed NVRAM
On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar wrote: > > Libvirt domain XML currently allows only local filepaths > that can be used to specify a NVRAM disk. > Since, VMs can migrate across hypervisor hosts, it should be > possible to allocate NVRAM disks on network storage for > uninterrupted access. > > This series extends the NVRAM element to support hosting over > network-backed disks, for high availability. > It achieves this by embedding virStorageSource pointer for > nvram into _virDomainLoaderDef. > > It introduces a 'type' attribute for NVRAM element to > specify 'file' vs 'network' backed NVRAM. > > Changes v1->v2: > - Split the patch into smaller patches > - Added unit test > - Updated the doc > - Addressed Peter's comment on v1 > (https://listman.redhat.com/archives/libvir-list/2022-March/229684.html) Ok so without going deeper into the actual change, following are some quick comments based on some of my own experience of introducing new conf options in libvirt: (a) Please update NEWS.rst t to document the new xml attribute support for the next release. This should be the last patch in the series preferrably. (b) Please put patch #2 and #5 together. Also please prefix the first line with "conf:" I think patch #4 should also go together but I will let others comment. (c) It's better that the unit tests (patches #7 and #8) go along with the changes in the same patch. Then when cherry picking the unit tests will be picked along with the change itself. (d) also I have commented separately, your validation patch needs additional unit tests to check the validation actually works. > > Rohit Kumar (8): > Make NVRAM a virStorageSource type. > Add support to parse/format virStorageSource type NVRAM > Validate remote store NVRAM > Cleanup diskSourceNetwork and diskSourceFile schema > Update XML schema to support network backed NVRAM > Update NVRAM documentation > Add unit test for network backed NVRAM > Add unit test to support new 'file' type NVRAM > > docs/formatdomain.rst | 43 +++-- > src/conf/domain_conf.c| 88 --- > src/conf/domain_conf.h| 2 +- > src/conf/schemas/domaincommon.rng | 80 +++-- > src/qemu/qemu_cgroup.c| 3 +- > src/qemu/qemu_command.c | 2 +- > src/qemu/qemu_domain.c| 14 +-- > src/qemu/qemu_driver.c| 5 +- > src/qemu/qemu_firmware.c | 23 +++-- > src/qemu/qemu_namespace.c | 5 +- > src/qemu/qemu_process.c | 5 +- > src/qemu/qemu_validate.c | 22 + > src/security/security_dac.c | 6 +- > src/security/security_selinux.c | 6 +- > src/security/virt-aa-helper.c | 5 +- > src/vbox/vbox_common.c| 2 +- > .../bios-nvram-file.x86_64-latest.args| 37 > tests/qemuxml2argvdata/bios-nvram-file.xml| 23 + > .../bios-nvram-network.x86_64-latest.args | 37 > tests/qemuxml2argvdata/bios-nvram-network.xml | 25 ++ > tests/qemuxml2argvtest.c | 2 + > 21 files changed, 360 insertions(+), 75 deletions(-) > create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.xml > create mode 100644 > tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml > > -- > 2.25.1 >
Re: [PATCH v2 3/8] Validate remote store NVRAM
On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar wrote:
>
> Remote store NVRAM feature is being enabled only
> if it supports 'blockdev' capability.
>
> Signed-off-by: Prerna Saxena
> Signed-off-by: Florian Schmidt
> Signed-off-by: Rohit Kumar
Please add negative unit tests to check the validation along with this
patch. Prefix the patch with "qemu:".
> ---
> src/qemu/qemu_validate.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 96f5427678..2a961b1f50 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -611,6 +611,25 @@ qemuValidateDomainDefBoot(const virDomainDef *def)
> }
>
>
> +static int
> +qemuValidateDomainDefNvram(const virDomainDef *def,
> + virQEMUCaps *qemuCaps)
> +{
> +if (def->os.loader && def->os.loader->nvram) {
> +if (def->os.loader->nvram->type != VIR_STORAGE_TYPE_FILE &&
> +!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("This Qemu does not support 'blockdev'
> capability "
> + "for remote store NVRAM. NVRAM type other than "
> + "'file' is not supported with this QEMU"));
> +return -1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +
> /**
> * qemuValidateDefGetVcpuHotplugGranularity:
> * @def: domain definition
> @@ -1185,6 +1204,9 @@ qemuValidateDomainDef(const virDomainDef *def,
> if (qemuValidateDomainDefBoot(def) < 0)
> return -1;
>
> +if (qemuValidateDomainDefNvram(def, qemuCaps) < 0)
> +return -1;
> +
> if (qemuValidateDomainVCpuTopology(def, qemuCaps) < 0)
> return -1;
>
> --
> 2.25.1
>
Re: [libvirt RFC] virFile: new VIR_FILE_WRAPPER_BIG_PIPE to improve performance
On Mon, Mar 14, 2022 at 5:06 PM Michal Prívozník wrote:
>
> On 3/12/22 17:30, Claudio Fontana wrote:
> > the first user is the qemu driver,
> >
> > virsh save/resume would slow to a crawl with a default pipe size (64k).
> >
> > This improves the situation by 400%.
> >
> > Going through io_helper still seems to incur in some penalty (~15%-ish)
> > compared with direct qemu migration to a nc socket to a file.
> >
> > Signed-off-by: Claudio Fontana
> > ---
> > src/qemu/qemu_driver.c| 6 +++---
> > src/qemu/qemu_saveimage.c | 11 ++-
> > src/util/virfile.c| 12
> > src/util/virfile.h| 1 +
> > 4 files changed, 22 insertions(+), 8 deletions(-)
> >
> > Hello, I initially thought this to be a qemu performance issue,
> > so you can find the discussion about this in qemu-devel:
> >
> > "Re: bad virsh save /dev/null performance (600 MiB/s max)"
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03142.html
> >
> > RFC since need to validate idea, and it is only lightly tested:
> >
> > save - about 400% benefit in throughput, getting around 20 Gbps to
> > /dev/null,
> >and around 13 Gbps to a ramdisk.
> > By comparison, direct qemu migration to a nc socket is around
> > 24Gbps.
> >
> > restore - not tested, _should_ also benefit in the "bypass_cache" case
> > coredump - not tested, _should_ also benefit like for save
> >
> > Thanks for your comments and review,
> >
> > Claudio
>
> Hey, I like this idea, but couple of points below.
>
> >
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index c1b3bd8536..be248c1e92 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -3044,7 +3044,7 @@ doCoreDump(virQEMUDriver *driver,
> > virFileWrapperFd *wrapperFd = NULL;
> > int directFlag = 0;
> > bool needUnlink = false;
> > -unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING;
> > +unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING |
> > VIR_FILE_WRAPPER_BIG_PIPE;
> > const char *memory_dump_format = NULL;
> > g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> > g_autoptr(virCommand) compressor = NULL;
> > @@ -3059,7 +3059,7 @@ doCoreDump(virQEMUDriver *driver,
> >
> > /* Create an empty file with appropriate ownership. */
> > if (dump_flags & VIR_DUMP_BYPASS_CACHE) {
> > -flags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
> > +wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
> > directFlag = virFileDirectFdFlag();
> > if (directFlag < 0) {
> > virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > @@ -3072,7 +3072,7 @@ doCoreDump(virQEMUDriver *driver,
> > )) < 0)
> > goto cleanup;
> >
> > -if (!(wrapperFd = virFileWrapperFdNew(, path, flags)))
> > +if (!(wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> > goto cleanup;
> >
> > if (dump_flags & VIR_DUMP_MEMORY_ONLY) {
> > diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> > index c0139041eb..1b522a1542 100644
> > --- a/src/qemu/qemu_saveimage.c
> > +++ b/src/qemu/qemu_saveimage.c
> > @@ -267,7 +267,7 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> > int fd = -1;
> > int directFlag = 0;
> > virFileWrapperFd *wrapperFd = NULL;
> > -unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
> > +unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING |
> > VIR_FILE_WRAPPER_BIG_PIPE;
> >
> > /* Obtain the file handle. */
> > if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
> > @@ -463,10 +463,11 @@ qemuSaveImageOpen(virQEMUDriver *driver,
> > if ((fd = qemuDomainOpenFile(cfg, NULL, path, oflags, NULL)) < 0)
> > return -1;
> >
> > -if (bypass_cache &&
> > -!(*wrapperFd = virFileWrapperFdNew(, path,
> > - VIR_FILE_WRAPPER_BYPASS_CACHE)))
> > -return -1;
> > +if (bypass_cache) {
> > +unsigned int wrapperFlags = VIR_FILE_WRAPPER_BYPASS_CACHE |
> > VIR_FILE_WRAPPER_BIG_PIPE;
> > +if (!(*wrapperFd = virFileWrapperFdNew(, path, wrapperFlags)))
> > +return -1;
> > +}
> >
> > data = g_new0(virQEMUSaveData, 1);
> >
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index a04f888e06..fdacd17890 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -282,6 +282,18 @@ virFileWrapperFdNew(int *fd, const char *name,
> > unsigned int flags)
> >
> > ret->cmd = virCommandNewArgList(iohelper_path, name, NULL);
> >
> > +if (flags & VIR_FILE_WRAPPER_BIG_PIPE) {
>
>
> I believe we don't need this flag. I mean, the plain fact that
> virFileWrapper is used means that caller wants to avoid VFS because it's
> interested in speed. Therefore, this code could be done unconditionally.
>
> > +/*
> > + * virsh save/resume would slow to a crawl with a default pipe
> > size (usually
Re: [libvirt RFC] virFile: new VIR_FILE_WRAPPER_BIG_PIPE to improve performance
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index a04f888e06..fdacd17890 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -282,6 +282,18 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned
> int flags)
>
> ret->cmd = virCommandNewArgList(iohelper_path, name, NULL);
>
> +if (flags & VIR_FILE_WRAPPER_BIG_PIPE) {
> +/*
> + * virsh save/resume would slow to a crawl with a default pipe size
> (usually 64k).
> + * This improves the situation by 400%, although going through
> io_helper still incurs
> + * in a performance penalty compared with a direct qemu migration to
> a socket.
> + */
> +int pipe_sz, rv = virFileReadValueInt(_sz,
> "/proc/sys/fs/pipe-max-size");
> +if (rv != 0) {
> +pipe_sz = 1024 * 1024; /* common default for pipe-max-size */
> +}
> +fcntl(pipefd[output ? 0 : 1], F_SETPIPE_SZ, pipe_sz);
> +}
I believe this entire hunk of code should be ifdef'd within #ifdef
__linux__. non-windows does not necessarily mean only linux.
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, 8 Mar 2022, Michael S. Tsirkin wrote: > On Tue, Mar 08, 2022 at 10:23:20PM +0530, Ani Sinha wrote: > > On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin wrote: > > > > > > On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Tue, 8 Mar 2022, Laine Stump wrote: > > > > > > > > > Aha! the domain of qemu-de...@nongnu.org was incorrect in the > > > > > original send > > > > > (it was "nognu.org"), so none of this thread was making it to that > > > > > list. > > > > > > > > > > > > Not to give any excuses but this happened because on Qemu side I never > > > > have to type this manually. My git config is set up so that > > > > the cc in send-email is filled up automatically using > > > > scripts/get_maintainer.pl. On libvirt side also the domain and mailing > > > > list is easy to remember. Its only when I have to manually type stuff > > > > that > > > > shit happens :-) > > > > > > Donnu about alpine, but with mutt you can easily set up > > > and alias and then it expands for you. > > > > I use alpine to only reply/review patches. I use git send-email to > > actually send the patch. There I am not sure the best way to avoid > > manually typing in the mailing list address. > > send-email supports aliases too. Ah cool. I just set this up with some help from https://felipec.wordpress.com/2009/10/25/git-send-email-tricks/ . Now I can simply say $ git send-email --to=qemu-list without worrying about typo :-) Thanks for the pointer.
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Mar 8, 2022 at 10:17 PM Michael S. Tsirkin wrote: > > On Tue, Mar 08, 2022 at 10:15:11PM +0530, Ani Sinha wrote: > > > > > > On Tue, 8 Mar 2022, Laine Stump wrote: > > > > > Aha! the domain of qemu-de...@nongnu.org was incorrect in the original > > > send > > > (it was "nognu.org"), so none of this thread was making it to that list. > > > > > > Not to give any excuses but this happened because on Qemu side I never > > have to type this manually. My git config is set up so that > > the cc in send-email is filled up automatically using > > scripts/get_maintainer.pl. On libvirt side also the domain and mailing > > list is easy to remember. Its only when I have to manually type stuff that > > shit happens :-) > > Donnu about alpine, but with mutt you can easily set up > and alias and then it expands for you. I use alpine to only reply/review patches. I use git send-email to actually send the patch. There I am not sure the best way to avoid manually typing in the mailing list address.
[libvirt] [PATCH RESEND v2 4/4] NEWS: document new acpi pci hotplug config option
Added the following new libvirt conf option to the release note to indicate their availability with the next release: Signed-off-by: Ani Sinha --- NEWS.rst | 8 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index a5b6106bc2..e474b32e69 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,14 @@ v8.2.0 (unreleased) * **New features** + * qemu: Add a new global feature for controlling ACPI PCI hotplug on bridges + +A new config option under +sub-element have been added to allow users enable/disable ACPI based PCI +hotplug for cold plugged bridges (that is, bridges that were present in the +domain definition when the guest was first started).This setting is only +applicable for x86 guests using qemu driver. + * **Improvements** * **Bug fixes** -- 2.25.1
[libvirt] [PATCH RESEND v2 3/4] qemu: command: add support for acpi-bridge-hotplug feature
This change adds backend qemu command line support for new libvirt
global feature 'acpi-bridge-hotplug'. This option can be used as
following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to
qemu for x86 guests:
(pc): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
This change also adds the required qemuxml2argv unit tests in order to
test correct qemu arguments. Unit tests have also been added to test
qemu capability validation checks as well as checks for using this
option with the right architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 19 ++
...-hotplug-bridge-disable.aarch64-latest.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 35 +
...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 38 +++
tests/qemuxml2argvtest.c | 7
6 files changed, 101 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c836799888..206a9794cb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6280,6 +6280,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG];
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6325,6 +6326,24 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
+const char *pm_object = NULL;
+
+if (!qemuDomainIsQ35(def))
+pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
+
+if (pm_object != NULL) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
new file mode 100644
index 00..90527dfefd
--- /dev/null
+++
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}'
\
+-machine pc-i440fx-2.5,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 1024 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
+-boot strict=on \
+-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":&quo
[libvirt] [PATCH RESEND v2 0/4] re-introduce
Change log: v2: rebased the patchset. Laine's response is appended at the end. I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of hot-plugging switches with PCIe Native hotplug since ACPI PCI Hot-plug only works with cold-plugged bridges. If the user wants to explicitely use this feature, they can disable ACPI PCI Hot-plug with: --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug instead of PCIe Native. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-5-imamm...@redhat.com> Reviewed-by: Ani Sinha Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin The patch description says it all. Instead of masking out the HPC bit in pcie slots, we keep them turned on. Instead, we do not advertize native hotplug capability for PCIE using _OSC control method. See section 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for these slots so now the guest OS can select ACPI hotplug instead. The second change is introduction of a property with which we keep the existing behavior for pc-q35-6.1 machines. This means HPC bit is masked and ACPI hotplug is enabled by default for pcie root ports. The QEMU commit is: (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb Author: Julia Suvorova Date: Fri Nov 12 06:08:54 2021 -0500 hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be turned on, while the switch to ACPI Hot-plug will be done in the DSDT table. Introducing 'x-keep-native-hpc' property disables the HPC bit only in 6.1 and as a result keeps the forced 'reserve-io' on pcie-root-ports in 6.1 too. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-3-imamm...@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Lastly, as a related side note, because from QEMU 6.2 onwards, we do not mask out HPC bit in PCIE, the work done by this patch is no longer needed: (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820 Author: Marcel Apfelbaum Date: Mon Aug 2 12:00:57 2021 +0300 hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Q35 has now ACPI hotplug enabled by default for PCI(e) devices. As opposed to native PCIe hotplug, guests like Fedora 34 will not assign IO range to pcie-root-ports not supporting native hotplug, resulting into
[libvirt] [PATCH RESEND v2 2/4] conf: introduce support for acpi-bridge-hotplug feature
This change introduces a new libvirt sub-element under
that can be used to configure all pci related features.
Currently the only sub-sub element supported by this sub-element is
'acpi-bridge-hotplug' as shown below:
The above option is only available for the QEMU driver, for x86 guests
only. It is a global option, affecting all PCI bridge controllers on
the guest.
The 'acpi-bridge-hotplug' option enables or disables ACPI hotplug
support for cold-plugged pci bridges. Examples of bridges include the
PCI-PCI bridge (pci-bridge controller) for pc (i440fx) machinetypes,
or PCIe-PCI bridges and pcie-root-port controllers for q35
machinetypes.
For pc machinetypes in x86, this option has been available in QEMU
since version 2.1. Please see the following changes in qemu repo:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
bridge hotplug is disabled")
For q35 machinetypes, this was introduced in QEMU 6.1 with the
following changes in qemu repo:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
(b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based
machines (as opposed to native hotplug) are outlined in (b). There are
use cases where users would still want to use native
hotplug. Therefore, this config option enables users to choose either
ACPI based hotplug or native hotplug for bridges (for example for pcie
root port controller in q35 machines).
Qemu capability validation checks have also been added along with
related unit tests to exercise the new conf option.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 32 +++
docs/schemas/domaincommon.rng | 15
src/conf/domain_conf.c| 89 ++-
src/conf/domain_conf.h| 9 ++
src/qemu/qemu_validate.c | 42 +
.../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
.../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36
.../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36
.../q35-acpi-hotplug-bridge-disable.xml | 53 +++
.../q35-acpi-hotplug-bridge-enable.xml| 53 +++
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
15 files changed, 385 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9202cd3107..864dfd8bd8 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1879,6 +1879,9 @@ Hypervisors may allow certain CPU / machine features to
be toggled on/off.
+
+
+
@@ -1997,6 +2000,35 @@ are:
passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode -
optional string sync_pt or share_pt :since:`6.3.0`
=== ==
=== ==
+``pci``
+ Various PCI bus related features of the hypervisor.
+
+
=
=== ==
+ Feature Description
Value Since
+
=
=== ==
+ acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI
bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`8.2.0`
+
=
=== ==
+
+ Note:
+ pc machine
[libvirt] [PATCH RESEND v2 1/4] qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
qemu added support for i440fx specific global boolean flag
PIIX4_PM.acpi-pci-hotplug-with-bridge-support
around version 2.1. This flag is enabled by default. When disabled, it
turns off acpi pci hotplug for cold plugged pci bridges in i440fx
machine types.
Very recently, in qemu version 6.1, the same global option was also
added for q35 machine types as well.
ICH9-LPC.acpi-pci-hotplug-with-bridge-support
This option turns on or off acpi based hotplug for cold plugged pcie
bridges like pcie root ports. This flag is also enabled by
default. Please refer to the following qemu changes:
c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
This patch adds the corresponding qemu capabilities in libvirt. For
i440fx, the capability is detected as
QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is
detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
Please note that the test specific qemu capabilities .replies files
has already been updated as a part of regular refreshing them when a
new qemu version is released. Hence, no updates to those files are
required.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
5 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 529e9ceaf5..08d5d733ce 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -665,6 +665,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"virtio-mem-pci.prealloc", /*
QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC */
"calc-dirty-rate", /* QEMU_CAPS_CALC_DIRTY_RATE */
"dirtyrate-param.mode", /* QEMU_CAPS_DIRTYRATE_MODE */
+
+ /* 425 */
+ "ich9.acpi-hotplug-bridge", /*
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */
);
@@ -1551,6 +1554,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsVirtioGpu[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = {
{ "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL },
+{ "acpi-pci-hotplug-with-bridge-support",
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] =
{
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f6188b42de..51dc668913 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -641,6 +641,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_CALC_DIRTY_RATE, /* accepts calc-dirty-rate */
QEMU_CAPS_DIRTYRATE_MODE , /* calc-dirty-rate accepts mode parameter */
+/* 425 */
+QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35
ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index ba1aecc37e..51e1e07d2f 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -239,6 +239,7 @@
+
6001000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index d77907af55..7b665c82e8 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
+
6002000
0
43100244
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index ae800abcc4..692e2f14da 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6002050
0
43100243
--
2.25.1
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, 8 Mar 2022, Laine Stump wrote: > Aha! the domain of qemu-de...@nongnu.org was incorrect in the original send > (it was "nognu.org"), so none of this thread was making it to that list. Not to give any excuses but this happened because on Qemu side I never have to type this manually. My git config is set up so that the cc in send-email is filled up automatically using scripts/get_maintainer.pl. On libvirt side also the domain and mailing list is easy to remember. Its only when I have to manually type stuff that shit happens :-)
Re: [libvirt] [PATCH RESEND v2 0/4] re-introduce
On Tue, Mar 8, 2022 at 21:21 Laine Stump wrote: > Aha! the domain of qemu-de...@nongnu.org was incorrect in the original > send (it was "nognu.org"), so none of this thread was making it to that > list. I've corrected it in this message, but interested parties from > qemu-devel will need to look on the libvir-list archives for the actual > patch mails: > > https://listman.redhat.com/archives/libvir-list/2022-March/229089.html > > Anyone else who responds to any of the mail on this thread should fix > the qemu-devel address accordingly. This patch set has been a true test of my diligence and perseverance > > On 3/8/22 10:33 AM, Laine Stump wrote: > > On 3/8/22 1:39 AM, Ani Sinha wrote: > >> Changelog: > >> v2 - rebased the patch series to latest master. > >> > >> I am re-introducing the patchset for which got > >> reverted here few months back: > >> > >> https://www.spinics.net/linux/fedora/libvir/msg224089.html > >> > >> The reason for the reversal was that there seemed to be some > >> instability/issues around the use of the qemu commandline which this > >> patchset tries to support. In particular, some guest operating systems > >> did not like the way QEMU was trying to disable native hotplug on pcie > >> root ports. > > > > My memory isn't completely clear, but I think there was also the issue > > that the option claims to enable ACPI hotplug when set to on, but > > instead what it actually does (in the Q35 case at least) is to enable > > native PCI hotplug when set to off (without actually disabling ACPI > > hotplug) and disable native PCI hotplug when set to on, or something > > like that. This ends up leaving it up to the guest OS to decide which > > type of hotplug to use, meaning its decision could override what's in > > the libvirt config, thus confusing everyone. Again, I probably have the > > details mixed up, but it was something like this. > > > > I asked mst about this this morning, and he suggested something that > > you've already done - Cc'ing the series to qemu-devel and the relevant > > maintainers so we can have a discussion with all involved parties about > > their opinions on whether we really should expose this existing option > > in libvirt, or if we should instead have two new options that are more > > orthogonal about enabling/disabling the two types of hotplug, so that > > libvirt config can more accurately represent what is being presented to > > the guest rather than a "best guess" of what we think the guest is going > > to do with what is presented. > > > > (Michael did also say that, with the current flurry of bug reports for > > the QEMU rc's, this discusion may not happen until closer to release > > when the bug reports die down. I know this doesn't mesh with your desire > > to "push now to allow for testing" (which in general would be a good > > thing if we were certain that we wanted the option like this and were > > just expecting some minor bugs that could be fixed), but my opinion is > > that 1) it's possible for anyone interested to test the functionality > > using , and 2) we should avoid turning libvirt git > > into a revolving door of experiments. The only practical difference > > between using and having a dedicated option is that > > the use of causes the domain to be tainted, and the > > XML is a bit more complicated. But since the people we're talking about > > here will already have built their own libvirt binaries, the tainted > > status of any guests is irrelevant and the extra complexity of using > > is probably trivial to them :-). > > > >> Subsequently, in QEMU 6.2, we have changed our mechanism > >> using which we disable native hotplug. As I understand, we do not have > >> any reported issues so far in 6.2 around this area. QEMU will enter a > >> soft feature freeze in the first week of march in prep for 7.0 release. > >> Libvirt is also entering a new release cycle phaze. Hence, I am > >> introducing this patchset early enough in the release cycles so that if > >> we do see any issues on the qemu side during the rc0, rc1 cycles and if > >> reversal of this patchset is again required, it can be done in time > >> before the next libvirt release end of March. > >> > >> All the patches in this series had been previously reviewed. Some > >> subsequent fixes were made after my initial patches were pushed. I have > >> squashed all those fixes and consolidated them into four patches. I have > >> also updated the docume
[libvirt] [PATCH RESEND v2 4/4] NEWS: document new acpi pci hotplug config option
Added the following new libvirt conf option to the release note to indicate their availability with the next release: Signed-off-by: Ani Sinha --- NEWS.rst | 8 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index a5b6106bc2..e474b32e69 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,14 @@ v8.2.0 (unreleased) * **New features** + * qemu: Add a new global feature for controlling ACPI PCI hotplug on bridges + +A new config option under +sub-element have been added to allow users enable/disable ACPI based PCI +hotplug for cold plugged bridges (that is, bridges that were present in the +domain definition when the guest was first started).This setting is only +applicable for x86 guests using qemu driver. + * **Improvements** * **Bug fixes** -- 2.25.1
[libvirt] [PATCH RESEND v2 2/4] conf: introduce support for acpi-bridge-hotplug feature
This change introduces a new libvirt sub-element under
that can be used to configure all pci related features.
Currently the only sub-sub element supported by this sub-element is
'acpi-bridge-hotplug' as shown below:
The above option is only available for the QEMU driver, for x86 guests
only. It is a global option, affecting all PCI bridge controllers on
the guest.
The 'acpi-bridge-hotplug' option enables or disables ACPI hotplug
support for cold-plugged pci bridges. Examples of bridges include the
PCI-PCI bridge (pci-bridge controller) for pc (i440fx) machinetypes,
or PCIe-PCI bridges and pcie-root-port controllers for q35
machinetypes.
For pc machinetypes in x86, this option has been available in QEMU
since version 2.1. Please see the following changes in qemu repo:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
bridge hotplug is disabled")
For q35 machinetypes, this was introduced in QEMU 6.1 with the
following changes in qemu repo:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
(b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based
machines (as opposed to native hotplug) are outlined in (b). There are
use cases where users would still want to use native
hotplug. Therefore, this config option enables users to choose either
ACPI based hotplug or native hotplug for bridges (for example for pcie
root port controller in q35 machines).
Qemu capability validation checks have also been added along with
related unit tests to exercise the new conf option.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 32 +++
docs/schemas/domaincommon.rng | 15
src/conf/domain_conf.c| 89 ++-
src/conf/domain_conf.h| 9 ++
src/qemu/qemu_validate.c | 42 +
.../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
.../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36
.../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36
.../q35-acpi-hotplug-bridge-disable.xml | 53 +++
.../q35-acpi-hotplug-bridge-enable.xml| 53 +++
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
15 files changed, 385 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9202cd3107..864dfd8bd8 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1879,6 +1879,9 @@ Hypervisors may allow certain CPU / machine features to
be toggled on/off.
+
+
+
@@ -1997,6 +2000,35 @@ are:
passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode -
optional string sync_pt or share_pt :since:`6.3.0`
=== ==
=== ==
+``pci``
+ Various PCI bus related features of the hypervisor.
+
+
=
=== ==
+ Feature Description
Value Since
+
=
=== ==
+ acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI
bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`8.2.0`
+
=
=== ==
+
+ Note:
+ pc machine
[libvirt] [PATCH RESEND v2 1/4] qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
qemu added support for i440fx specific global boolean flag
PIIX4_PM.acpi-pci-hotplug-with-bridge-support
around version 2.1. This flag is enabled by default. When disabled, it
turns off acpi pci hotplug for cold plugged pci bridges in i440fx
machine types.
Very recently, in qemu version 6.1, the same global option was also
added for q35 machine types as well.
ICH9-LPC.acpi-pci-hotplug-with-bridge-support
This option turns on or off acpi based hotplug for cold plugged pcie
bridges like pcie root ports. This flag is also enabled by
default. Please refer to the following qemu changes:
c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
This patch adds the corresponding qemu capabilities in libvirt. For
i440fx, the capability is detected as
QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is
detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
Please note that the test specific qemu capabilities .replies files
has already been updated as a part of regular refreshing them when a
new qemu version is released. Hence, no updates to those files are
required.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
5 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 529e9ceaf5..08d5d733ce 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -665,6 +665,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"virtio-mem-pci.prealloc", /*
QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC */
"calc-dirty-rate", /* QEMU_CAPS_CALC_DIRTY_RATE */
"dirtyrate-param.mode", /* QEMU_CAPS_DIRTYRATE_MODE */
+
+ /* 425 */
+ "ich9.acpi-hotplug-bridge", /*
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */
);
@@ -1551,6 +1554,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsVirtioGpu[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = {
{ "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL },
+{ "acpi-pci-hotplug-with-bridge-support",
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] =
{
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f6188b42de..51dc668913 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -641,6 +641,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_CALC_DIRTY_RATE, /* accepts calc-dirty-rate */
QEMU_CAPS_DIRTYRATE_MODE , /* calc-dirty-rate accepts mode parameter */
+/* 425 */
+QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35
ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index ba1aecc37e..51e1e07d2f 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -239,6 +239,7 @@
+
6001000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index d77907af55..7b665c82e8 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
+
6002000
0
43100244
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index ae800abcc4..692e2f14da 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6002050
0
43100243
--
2.25.1
[libvirt] [PATCH RESEND v2 3/4] qemu: command: add support for acpi-bridge-hotplug feature
This change adds backend qemu command line support for new libvirt
global feature 'acpi-bridge-hotplug'. This option can be used as
following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to
qemu for x86 guests:
(pc): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
This change also adds the required qemuxml2argv unit tests in order to
test correct qemu arguments. Unit tests have also been added to test
qemu capability validation checks as well as checks for using this
option with the right architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 19 ++
...-hotplug-bridge-disable.aarch64-latest.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 35 +
...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 38 +++
tests/qemuxml2argvtest.c | 7
6 files changed, 101 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c836799888..206a9794cb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6280,6 +6280,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG];
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6325,6 +6326,24 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
+const char *pm_object = NULL;
+
+if (!qemuDomainIsQ35(def))
+pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
+
+if (pm_object != NULL) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
new file mode 100644
index 00..90527dfefd
--- /dev/null
+++
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}'
\
+-machine pc-i440fx-2.5,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 1024 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
+-boot strict=on \
+-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":&quo
[libvirt] [PATCH RESEND v2 0/4] re-introduce
Changelog: v2 - rebased the patch series to latest master. I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of hot-plugging switches with PCIe Native hotplug since ACPI PCI Hot-plug only works with cold-plugged bridges. If the user wants to explicitely use this feature, they can disable ACPI PCI Hot-plug with: --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug instead of PCIe Native. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-5-imamm...@redhat.com> Reviewed-by: Ani Sinha Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin The patch description says it all. Instead of masking out the HPC bit in pcie slots, we keep them turned on. Instead, we do not advertize native hotplug capability for PCIE using _OSC control method. See section 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for these slots so now the guest OS can select ACPI hotplug instead. The second change is introduction of a property with which we keep the existing behavior for pc-q35-6.1 machines. This means HPC bit is masked and ACPI hotplug is enabled by default for pcie root ports. The QEMU commit is: (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb Author: Julia Suvorova Date: Fri Nov 12 06:08:54 2021 -0500 hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be turned on, while the switch to ACPI Hot-plug will be done in the DSDT table. Introducing 'x-keep-native-hpc' property disables the HPC bit only in 6.1 and as a result keeps the forced 'reserve-io' on pcie-root-ports in 6.1 too. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-3-imamm...@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Lastly, as a related side note, because from QEMU 6.2 onwards, we do not mask out HPC bit in PCIE, the work done by this patch is no longer needed: (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820 Author: Marcel Apfelbaum Date: Mon Aug 2 12:00:57 2021 +0300 hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Q35 has now ACPI hotplug enabled by default for PCI(e) devices. As opposed to native PCIe hotplug, guests like Fedora 34 will not assign IO range to pcie-root-ports not supporting native hotplug, resulting into a regression.
[libvirt] [PATCH RESEND 4/4] NEWS: document new acpi pci hotplug config option
Added the following new libvirt conf option to the release note to indicate their availability with the next release: Signed-off-by: Ani Sinha --- NEWS.rst | 21 + 1 file changed, 21 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 65fb112d11..85e796f274 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -8,6 +8,27 @@ the changes introduced by each of them. For a more fine-grained view, use the `git log`_. +v8.2.0 (unreleased) +=== + +* **Security** + +* **Removed features** + +* **New features** + + * qemu: Add a new global feature for controlling ACPI PCI hotplug on bridges + +A new config option under +sub-element have been added to allow users enable/disable ACPI based PCI +hotplug for cold plugged bridges (that is, bridges that were present in the +domain definition when the guest was first started).This setting is only +applicable for x86 guests using qemu driver. + +* **Improvements** + +* **Bug fixes** + v8.1.0 (unreleased) === -- 2.25.1
[libvirt] [PATCH RESEND 3/4] qemu: command: add support for acpi-bridge-hotplug feature
This change adds backend qemu command line support for new libvirt
global feature 'acpi-bridge-hotplug'. This option can be used as
following:
The '' sub-element under '' is also newly introduced.
'acpi-bridge-hotplug' turns on the following command line option to
qemu for x86 guests:
(pc): -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=
(q35): -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=
This change also adds the required qemuxml2argv unit tests in order to
test correct qemu arguments. Unit tests have also been added to test
qemu capability validation checks as well as checks for using this
option with the right architecture.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_command.c | 19 ++
...-hotplug-bridge-disable.aarch64-latest.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 35 +
...pi-hotplug-bridge-disable.x86_64-6.0.0.err | 1 +
...-hotplug-bridge-disable.x86_64-latest.args | 38 +++
tests/qemuxml2argvtest.c | 7
6 files changed, 101 insertions(+)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-6.0.0.err
create mode 100644
tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.args
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c836799888..206a9794cb 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6280,6 +6280,7 @@ qemuBuildPMCommandLine(virCommand *cmd,
qemuDomainObjPrivate *priv)
{
virQEMUCaps *qemuCaps = priv->qemuCaps;
+int acpihp_br = def->pci_features[VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG];
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SET_ACTION)) {
/* with new qemu we always want '-no-shutdown' on startup and we set
@@ -6325,6 +6326,24 @@ qemuBuildPMCommandLine(virCommand *cmd,
pm_object, def->pm.s4 == VIR_TRISTATE_BOOL_NO);
}
+if (acpihp_br != VIR_TRISTATE_SWITCH_ABSENT) {
+const char *pm_object = NULL;
+
+if (!qemuDomainIsQ35(def))
+pm_object = "PIIX4_PM";
+
+if (qemuDomainIsQ35(def) &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE))
+pm_object = "ICH9-LPC";
+
+if (pm_object != NULL) {
+virCommandAddArg(cmd, "-global");
+virCommandAddArgFormat(cmd,
"%s.acpi-pci-hotplug-with-bridge-support=%s",
+ pm_object,
+ virTristateSwitchTypeToString(acpihp_br));
+}
+}
+
return 0;
}
diff --git
a/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
new file mode 100644
index 00..9f0a88b826
--- /dev/null
+++
b/tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.aarch64-latest.err
@@ -0,0 +1 @@
+unsupported configuration: acpi-bridge-hotplug is not available for
architecture 'aarch64'
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
new file mode 100644
index 00..90527dfefd
--- /dev/null
+++
b/tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.args
@@ -0,0 +1,35 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/tmp/lib/domain--1-i440fx \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/tmp/lib/domain--1-i440fx/.local/share \
+XDG_CACHE_HOME=/tmp/lib/domain--1-i440fx/.cache \
+XDG_CONFIG_HOME=/tmp/lib/domain--1-i440fx/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=i440fx,debug-threads=on \
+-S \
+-object
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-i440fx/master-key.aes"}'
\
+-machine pc-i440fx-2.5,usb=off,dump-guest-core=off,memory-backend=pc.ram \
+-accel tcg \
+-cpu qemu64 \
+-m 1024 \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":1073741824}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid 56f5055c-1b8d-490c-844a-ad646a1c \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-no-acpi \
+-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off \
+-boot strict=on \
+-device
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":&quo
[libvirt] [PATCH RESEND 2/4] conf: introduce support for acpi-bridge-hotplug feature
This change introduces a new libvirt sub-element under
that can be used to configure all pci related features.
Currently the only sub-sub element supported by this sub-element is
'acpi-bridge-hotplug' as shown below:
The above option is only available for the QEMU driver, for x86 guests
only. It is a global option, affecting all PCI bridge controllers on
the guest.
The 'acpi-bridge-hotplug' option enables or disables ACPI hotplug
support for cold-plugged pci bridges. Examples of bridges include the
PCI-PCI bridge (pci-bridge controller) for pc (i440fx) machinetypes,
or PCIe-PCI bridges and pcie-root-port controllers for q35
machinetypes.
For pc machinetypes in x86, this option has been available in QEMU
since version 2.1. Please see the following changes in qemu repo:
9e047b982452c6 ("piix4: add acpi pci hotplug support")
133a2da488062e ("pc: acpi: generate AML only for PCI0 devices if PCI
bridge hotplug is disabled")
For q35 machinetypes, this was introduced in QEMU 6.1 with the
following changes in qemu repo:
(a) c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
(b) 17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
Q35")
The reasons for enabling ACPI based hotplug for PCIe (q35) based
machines (as opposed to native hotplug) are outlined in (b). There are
use cases where users would still want to use native
hotplug. Therefore, this config option enables users to choose either
ACPI based hotplug or native hotplug for bridges (for example for pcie
root port controller in q35 machines).
Qemu capability validation checks have also been added along with
related unit tests to exercise the new conf option.
Signed-off-by: Ani Sinha
---
docs/formatdomain.rst | 32 +++
docs/schemas/domaincommon.rng | 15
src/conf/domain_conf.c| 89 ++-
src/conf/domain_conf.h| 9 ++
src/qemu/qemu_validate.c | 42 +
.../aarch64-acpi-hotplug-bridge-disable.xml | 13 +++
.../pc-i440fx-acpi-hotplug-bridge-disable.xml | 36
.../pc-i440fx-acpi-hotplug-bridge-enable.xml | 36
.../q35-acpi-hotplug-bridge-disable.xml | 53 +++
.../q35-acpi-hotplug-bridge-enable.xml| 53 +++
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
...i-hotplug-bridge-disable.x86_64-latest.xml | 1 +
...pi-hotplug-bridge-enable.x86_64-latest.xml | 1 +
tests/qemuxml2xmltest.c | 4 +
15 files changed, 385 insertions(+), 1 deletion(-)
create mode 100644
tests/qemuxml2argvdata/aarch64-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-disable.xml
create mode 100644
tests/qemuxml2argvdata/pc-i440fx-acpi-hotplug-bridge-enable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-disable.xml
create mode 100644 tests/qemuxml2argvdata/q35-acpi-hotplug-bridge-enable.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/pc-i440fx-acpi-hotplug-bridge-enable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-disable.x86_64-latest.xml
create mode 12
tests/qemuxml2xmloutdata/q35-acpi-hotplug-bridge-enable.x86_64-latest.xml
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 9202cd3107..864dfd8bd8 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -1879,6 +1879,9 @@ Hypervisors may allow certain CPU / machine features to
be toggled on/off.
+
+
+
@@ -1997,6 +2000,35 @@ are:
passthrough Enable IOMMU mappings allowing PCI passthrough on, off; mode -
optional string sync_pt or share_pt :since:`6.3.0`
=== ==
=== ==
+``pci``
+ Various PCI bus related features of the hypervisor.
+
+
=
=== ==
+ Feature Description
Value Since
+
=
=== ==
+ acpi-bridge-hotplug Enable ACPI based hotplug on the cold-plugged PCI
bridges (pc) and pcie-root-ports (q35) (also see notes) on, off :since:`8.2.0`
+
=
=== ==
+
+ Note:
+ pc machine
[libvirt] [PATCH RESEND 1/4] qemu: capablities: detect acpi-pci-hotplug-with-bridge-support
qemu added support for i440fx specific global boolean flag
PIIX4_PM.acpi-pci-hotplug-with-bridge-support
around version 2.1. This flag is enabled by default. When disabled, it
turns off acpi pci hotplug for cold plugged pci bridges in i440fx
machine types.
Very recently, in qemu version 6.1, the same global option was also
added for q35 machine types as well.
ICH9-LPC.acpi-pci-hotplug-with-bridge-support
This option turns on or off acpi based hotplug for cold plugged pcie
bridges like pcie root ports. This flag is also enabled by
default. Please refer to the following qemu changes:
c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
17858a16950860 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
This patch adds the corresponding qemu capabilities in libvirt. For
i440fx, the capability is detected as
QEMU_CAPS_PIIX_ACPI_HOTPLUG_BRIDGE. For q35, the capability is
detected as QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE.
Please note that the test specific qemu capabilities .replies files
has already been updated as a part of regular refreshing them when a
new qemu version is released. Hence, no updates to those files are
required.
Signed-off-by: Ani Sinha
---
src/qemu/qemu_capabilities.c | 4
src/qemu/qemu_capabilities.h | 3 +++
tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml | 1 +
5 files changed, 10 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 529e9ceaf5..08d5d733ce 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -665,6 +665,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
"virtio-mem-pci.prealloc", /*
QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI_PREALLOC */
"calc-dirty-rate", /* QEMU_CAPS_CALC_DIRTY_RATE */
"dirtyrate-param.mode", /* QEMU_CAPS_DIRTYRATE_MODE */
+
+ /* 425 */
+ "ich9.acpi-hotplug-bridge", /*
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE */
);
@@ -1551,6 +1554,7 @@ static struct virQEMUCapsDevicePropsFlags
virQEMUCapsDevicePropsVirtioGpu[] = {
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsICH9[] = {
{ "disable_s3", QEMU_CAPS_ICH9_DISABLE_S3, NULL },
{ "disable_s4", QEMU_CAPS_ICH9_DISABLE_S4, NULL },
+{ "acpi-pci-hotplug-with-bridge-support",
QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, NULL },
};
static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsUSBNECXHCI[] =
{
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f6188b42de..51dc668913 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -641,6 +641,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for
syntax-check */
QEMU_CAPS_CALC_DIRTY_RATE, /* accepts calc-dirty-rate */
QEMU_CAPS_DIRTYRATE_MODE , /* calc-dirty-rate accepts mode parameter */
+/* 425 */
+QEMU_CAPS_ICH9_ACPI_HOTPLUG_BRIDGE, /* -M q35
ICH9-LPC.acpi-pci-hotplug-with-bridge-support */
+
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
index ba1aecc37e..51e1e07d2f 100644
--- a/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.1.0.x86_64.xml
@@ -239,6 +239,7 @@
+
6001000
0
43100243
diff --git a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
index d77907af55..7b665c82e8 100644
--- a/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml
@@ -241,6 +241,7 @@
+
6002000
0
43100244
diff --git a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
index ae800abcc4..692e2f14da 100644
--- a/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_7.0.0.x86_64.xml
@@ -243,6 +243,7 @@
+
6002050
0
43100243
--
2.25.1
[libvirt] [PATCH RESEND 0/4] re-introduce
I am re-introducing the patchset for which got reverted here few months back: https://www.spinics.net/linux/fedora/libvir/msg224089.html The reason for the reversal was that there seemed to be some instability/issues around the use of the qemu commandline which this patchset tries to support. In particular, some guest operating systems did not like the way QEMU was trying to disable native hotplug on pcie root ports. Subsequently, in QEMU 6.2, we have changed our mechanism using which we disable native hotplug. As I understand, we do not have any reported issues so far in 6.2 around this area. QEMU will enter a soft feature freeze in the first week of march in prep for 7.0 release. Libvirt is also entering a new release cycle phaze. Hence, I am introducing this patchset early enough in the release cycles so that if we do see any issues on the qemu side during the rc0, rc1 cycles and if reversal of this patchset is again required, it can be done in time before the next libvirt release end of March. All the patches in this series had been previously reviewed. Some subsequent fixes were made after my initial patches were pushed. I have squashed all those fixes and consolidated them into four patches. I have also updated the documentation to reflect the new changes from the QEMU side and rebased my changes fixing the tests in the process. What changed in QEMU post version 6.1 ? = We have made basically two major changes in QEMU. First is this change: (1) commit 211afe5c69b597acf85fdd577eb497f5be1ffbd8 Author: Julia Suvorova Date: Fri Nov 12 06:08:56 2021 -0500 hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC There are two ways to enable ACPI PCI Hot-plug: * Disable the Hot-plug Capable bit on PCIe slots. This was the first approach which led to regression [1-2], as I/O space for a port is allocated only when it is hot-pluggable, which is determined by HPC bit. * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC method. This removes the (future) ability of hot-plugging switches with PCIe Native hotplug since ACPI PCI Hot-plug only works with cold-plugged bridges. If the user wants to explicitely use this feature, they can disable ACPI PCI Hot-plug with: --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug instead of PCIe Native. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-5-imamm...@redhat.com> Reviewed-by: Ani Sinha Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin The patch description says it all. Instead of masking out the HPC bit in pcie slots, we keep them turned on. Instead, we do not advertize native hotplug capability for PCIE using _OSC control method. See section 6.2.11 in ACPI spec 6.2. At the same time, we turn on ACPI hotplug for these slots so now the guest OS can select ACPI hotplug instead. The second change is introduction of a property with which we keep the existing behavior for pc-q35-6.1 machines. This means HPC bit is masked and ACPI hotplug is enabled by default for pcie root ports. The QEMU commit is: (2) commit c318bef76206c2ecb6016e8e68c4ac6ff9a4c8cb Author: Julia Suvorova Date: Fri Nov 12 06:08:54 2021 -0500 hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type To solve issues [1-2] the Hot Plug Capable bit in PCIe Slots will be turned on, while the switch to ACPI Hot-plug will be done in the DSDT table. Introducing 'x-keep-native-hpc' property disables the HPC bit only in 6.1 and as a result keeps the forced 'reserve-io' on pcie-root-ports in 6.1 too. [1] https://gitlab.com/qemu-project/qemu/-/issues/641 [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 Signed-off-by: Julia Suvorova Signed-off-by: Igor Mammedov Message-Id: <2022110857.3116853-3-imamm...@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Lastly, as a related side note, because from QEMU 6.2 onwards, we do not mask out HPC bit in PCIE, the work done by this patch is no longer needed: (3) commit e2a6290aab578b2170c1f5909fa556385dc0d820 Author: Marcel Apfelbaum Date: Mon Aug 2 12:00:57 2021 +0300 hw/pcie-root-port: Fix hotplug for PCI devices requiring IO Q35 has now ACPI hotplug enabled by default for PCI(e) devices. As opposed to native PCIe hotplug, guests like Fedora 34 will not assign IO range to pcie-root-ports not supporting native hotplug, resulting into a regression. Reproduce by: qemu-bin -M q35 -device pcie-roo
Re: [PATCH] openrc: Make init scripts executable on install
On Tue, Feb 15, 2022 at 21:57 Michal Privoznik wrote:
> When installing openrc init scripts, we take whatever mode the
> generated files are in an copy them under /etc/init.d/. This is
> not ideal, because those files are not executable and they should
> be.
>
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/250
> Signed-off-by: Michal Privoznik
Reviewed-by: Ani Sinha
> ---
> src/meson.build | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/meson.build b/src/meson.build
> index 3890df7124..b2d951d36c 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -848,6 +848,7 @@ if conf.has('WITH_LIBVIRTD')
>install_data(
> init_file,
> install_dir: sysconfdir / 'init.d',
> +install_mode: 'rwxr-xr-x',
> rename: [ init['name'] ],
>)
>
> --
> 2.34.1
>
>
Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine
On Mon, Feb 7, 2022 at 4:58 PM Peter Krempa wrote: > > On Mon, Feb 07, 2022 at 16:50:28 +0530, Ani Sinha wrote: > > On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa wrote: > > > > > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > > > Igor Mammedov wrote: > > > > > > [...] > > > > > > > Even if we change it in libvirt right away, changing qemu will break > > > > forward compatibility. While we don't guarantee it, it still creates > > > > user grief. > > > > > > I've filed an upstream issue: > > > > > > https://gitlab.com/libvirt/libvirt/-/issues/272 > > > > I can look into this bug. Feel free to assign it to me. > > No need to. I've noticed that we already call query-hotpluggable-cpus > so with a simple modification the VM startup code path can be easily > fixed so I've done so. Ok I will look for your patch in the mailing list and review them. >
Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine
On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa wrote: > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > Igor Mammedov wrote: > > [...] > > > Even if we change it in libvirt right away, changing qemu will break > > forward compatibility. While we don't guarantee it, it still creates > > user grief. > > I've filed an upstream issue: > > https://gitlab.com/libvirt/libvirt/-/issues/272 I can look into this bug. Feel free to assign it to me.
Re: [libvirt][PATCH v10 5/5] Update default CPU location in qemu QOM tree
On Fri, 28 Jan 2022, Haibin Huang wrote:
> From: Lin Yang
>
How do we propose to maintain compatibility with older versions of Qemu?
> ---
> src/qemu/qemu_monitor_json.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 811db233c4..8c7f088775 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -48,7 +48,7 @@
>
> VIR_LOG_INIT("qemu.qemu_monitor_json");
>
> -#define QOM_CPU_PATH "/machine/unattached/device[0]"
> +#define QOM_CPU_PATH "/machine/cpu[0]"
>
> #define LINE_ENDING "\r\n"
>
> @@ -1679,7 +1679,7 @@ int qemuMonitorJSONSystemReset(qemuMonitor *mon)
> * A s390 cpu entry could look like this
> * { "arch": "s390",
> *"cpu-index": 0,
> - *"qom-path": "/machine/unattached/device[0]",
> + *"qom-path": "/machine/cpu[0]",
> *"thread_id": 3081,
> *"cpu-state": "operating" }
> *
> @@ -1711,7 +1711,7 @@ qemuMonitorJSONExtractCPUS390Info(virJSONValue *jsoncpu,
> * [{ "arch": "x86",
> *"current": true,
> *"CPU": 0,
> - *"qom_path": "/machine/unattached/device[0]",
> + *"qom_path": "/machine/cpu[0]",
> *"pc": -2130415978,
> *"halted": true,
> *"thread_id": 2631237,
> @@ -1726,7 +1726,7 @@ qemuMonitorJSONExtractCPUS390Info(virJSONValue *jsoncpu,
> * "thread-id": 0,
> * "socket-id": 0
> *},
> - *"qom-path": "/machine/unattached/device[0]",
> + *"qom-path": "/machine/cpu[0]",
> *"thread-id": 2631237,
> *...},
> *{...}
> @@ -1737,7 +1737,7 @@ qemuMonitorJSONExtractCPUS390Info(virJSONValue *jsoncpu,
> *"props": {
> * "core-id": 0
> *},
> - *"qom-path": "/machine/unattached/device[0]",
> + *"qom-path": "/machine/cpu[0]",
> *"thread-id": 1237,
> *"cpu-state": "operating",
> *...},
> @@ -7853,7 +7853,7 @@ qemuMonitorQueryHotpluggableCpusFree(struct
> qemuMonitorQueryHotpluggableCpusEntr
> * "socket-id": 0
> *},
> *"vcpus-count": 1,
> - *"qom-path": "/machine/unattached/device[0]",
> + *"qom-path": "/machine/cpu[0]",
> *"type": "qemu64-x86_64-cpu"
> * },
> * {...}
> --
> 2.17.1
>
>
Re: [PATCH] lib: Drop '&*' from '&*variable'
On Mon, Jan 31, 2022 at 3:12 PM Michal Privoznik wrote:
>
> Apparently, some of '&*variable' slipped in. Drop '&*' and access
> the variable directly.
>
> Signed-off-by: Michal Privoznik
Reviewed-by: Ani Sinha
> ---
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_migration_cookie.c | 2 +-
> src/rpc/virnetclientstream.c | 8
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index aa8f6b8d05..6b915d7535 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1765,7 +1765,7 @@
> qemuStorageSourcePrivateDataAssignSecinfo(qemuDomainSecretInfo **secinfo,
> *secinfo = g_new0(qemuDomainSecretInfo, 1);
> }
>
> -(*secinfo)->alias = g_steal_pointer(&*alias);
> +(*secinfo)->alias = g_steal_pointer(alias);
>
> return 0;
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1141efef4b..370d223198 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12221,7 +12221,7 @@ qemuConnectStealCPUModelFromInfo(virCPUDef *dst,
>
> virCPUDefFreeModel(dst);
>
> -info = g_steal_pointer(&*src);
> +info = g_steal_pointer(src);
> dst->model = g_steal_pointer(>name);
>
> for (i = 0; i < info->nprops; i++) {
> diff --git a/src/qemu/qemu_migration_cookie.c
> b/src/qemu/qemu_migration_cookie.c
> index bffab7c13d..ba05a5a07f 100644
> --- a/src/qemu/qemu_migration_cookie.c
> +++ b/src/qemu/qemu_migration_cookie.c
> @@ -423,7 +423,7 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookie *mig,
> if (!def || !*def)
> return 0;
>
> -mig->persistent = g_steal_pointer(&*def);
> +mig->persistent = g_steal_pointer(def);
> mig->flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
> mig->flagsMandatory |= QEMU_MIGRATION_COOKIE_PERSISTENT;
> return 0;
> diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
> index eb4dc6854d..e8e8ca2af2 100644
> --- a/src/rpc/virnetclientstream.c
> +++ b/src/rpc/virnetclientstream.c
> @@ -286,18 +286,18 @@ int virNetClientStreamSetError(virNetClientStream *st,
> st->err.code = err.code;
> }
> if (err.message) {
> -st->err.message = g_steal_pointer(&*err.message);
> +st->err.message = g_steal_pointer(err.message);
> }
> st->err.domain = err.domain;
> st->err.level = err.level;
> if (err.str1) {
> -st->err.str1 = g_steal_pointer(&*err.str1);
> +st->err.str1 = g_steal_pointer(err.str1);
> }
> if (err.str2) {
> -st->err.str2 = g_steal_pointer(&*err.str2);
> +st->err.str2 = g_steal_pointer(err.str2);
> }
> if (err.str3) {
> -st->err.str3 = g_steal_pointer(&*err.str3);
> +st->err.str3 = g_steal_pointer(err.str3);
> }
> st->err.int1 = err.int1;
> st->err.int2 = err.int2;
> --
> 2.34.1
>
Re: [PATCH v2] virProcessGetStatInfo: add a comment describing why we can not report error
Pinging again in case there is any interest ..
On Tue, Jan 25, 2022 at 4:34 PM Ani Sinha wrote:
>
> ping ...
>
> On Fri, 21 Jan 2022, Ani Sinha wrote:
>
> > virProcessGetStatInfo() currently is unable to report error conditions
> > because
> > that breaks libvirt's public best effort APIs. We add a comment in the
> > function
> > to indicate this. Adding comment here prevents others from going down the
> > path
> > of reporting error conditions in this functions in the future. It also
> > reminds
> > us that at some point in the future we need to fix the code so that this
> > limitations no longer exists.
> >
> > Please also see commit
> > 105dace22cc7 ("Revert "report error when virProcessGetStatInfo() is unable
> > to parse data"")
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/util/virprocess.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index b559a4257e..9422829b8b 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1784,6 +1784,12 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > ) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> > 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > ) < 0) {
> > +/* This function can not report error at present. Reporting error
> > here
> > + * causes some of libvirt's best effort public APIs to fail. This
> > + * resuts in external API behavior change. Until we can fix this in
> > + * a way so that public API behavior remains unchanged, we can only
> > + * write a warning log here.
> > + */
> > VIR_WARN("cannot parse process status data");
> > }
> >
> > --
> > 2.25.1
> >
> >
Re: [PATCH v2] virProcessGetStatInfo: add a comment describing why we can not report error
ping ...
On Fri, 21 Jan 2022, Ani Sinha wrote:
> virProcessGetStatInfo() currently is unable to report error conditions because
> that breaks libvirt's public best effort APIs. We add a comment in the
> function
> to indicate this. Adding comment here prevents others from going down the path
> of reporting error conditions in this functions in the future. It also reminds
> us that at some point in the future we need to fix the code so that this
> limitations no longer exists.
>
> Please also see commit
> 105dace22cc7 ("Revert "report error when virProcessGetStatInfo() is unable to
> parse data"")
>
> Signed-off-by: Ani Sinha
> ---
> src/util/virprocess.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index b559a4257e..9422829b8b 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1784,6 +1784,12 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> ) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0
> ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> ) < 0) {
> +/* This function can not report error at present. Reporting error
> here
> + * causes some of libvirt's best effort public APIs to fail. This
> + * resuts in external API behavior change. Until we can fix this in
> + * a way so that public API behavior remains unchanged, we can only
> + * write a warning log here.
> + */
> VIR_WARN("cannot parse process status data");
> }
>
> --
> 2.25.1
>
>
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Fri, Jan 21, 2022 at 8:56 PM Daniel P. Berrangé wrote: > On Fri, Jan 21, 2022 at 08:39:47PM +0530, Ani Sinha wrote: > > On Fri, Jan 21, 2022 at 20:33 Daniel P. Berrangé > > wrote: > > > > > On Fri, Jan 21, 2022 at 08:16:01PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Fri, 21 Jan 2022, Daniel P. Berrangé wrote: > > > > > > > > > On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > > > > > > > > > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník < > > > mpriv...@redhat.com > > > > > > > > <mailto:mpriv...@redhat.com>> wrote: > > > > > > > > > > > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time > > > > > > > > > > > > > > > > Well, I should have been more careful and not merge the > > > patch in the > > > > > > > > first place. Changing API behavior is something we should > > > never do. > > > > > > > > > > > > > > > > Looking at the code closer, it looks like all callers of > > > this function > > > > > > > > would need to ignore the reported error so that their > > > behavior is not > > > > > > > > changed. At this point, does it make sense to report an > > > error in the > > > > > > > > function? > > > > > > > > > > > > > > > > > > > > > > > > The callers can decide what do with the error raised by the > > > function. We > > > > > > > > should not write functions that cannot fail. > > > > > > > > > > > > > > > > > > > > > > But that's not what the commit does, is it. It changed some > public > > > APIs > > > > > > > from best effort to fail early. > > > > > > > > > > > > That is the side effect and I consider it as a bug. Imagine we > add > > > some > > > > > > more code into that function tomorrow and there is an error > path. Now > > > > > > because of this bug, we will have to ignore the error condition > and > > > not > > > > > > report it. How ridiculous is that? > > > > > > > > > > > > We should have kept the patch as is and fix the callers so that > the > > > public > > > > > > APIs were not broken. > > > > > > > > > > That is not the libvirt approach. Our #1 priority is public API > > > > > compatibility, everything else is subservient to that. So when we > > > > > have a regression we fix that in the quickest possible way. Simply > > > > > reverting the broken patch> > > > > > > > > (a) I take exception to the fact that the patch reverted was > "broken". > > > > > > Did you actually read the bug report in the first message in this > > > series ? An end user hit a behaviour regression breaking the > > > installation process, and was caused by the patch that's been > > > reverted. > > > > > > No I did not read the bug report because it required me to create an > > account and then login. I do not have my redhat BZ account credentials > > handy. > > Urgh, sorry I missed that the BZ was private. Private BZs are not > supposed to be listed in commit messages for precisely the reason > that people can't see them. The commit message should have showed > the problem listed in the private BZ description instead. Turns out even if I logged into BZ with my old account credentials ( I couldn't find the password, so reset it) I still can't access the bug info: You are not authorized to access bug #2041610. Most likely the bug has been restricted for internal development processes and we cannot grant access. If you are a Red Hat customer with an active subscription, please visit the Red Hat Customer Portal <https://access.redhat.com/> for assistance with your issue If you are a Fedora Project user and require assistance, please consider using one of the mailing lists <https://fedoraproject.org/wiki/Communicate> we host for the Fedora Project. > >
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Fri, Jan 21, 2022 at 20:33 Daniel P. Berrangé wrote: > On Fri, Jan 21, 2022 at 08:16:01PM +0530, Ani Sinha wrote: > > > > > > On Fri, 21 Jan 2022, Daniel P. Berrangé wrote: > > > > > On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > > > > > > > > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > > > > > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník < > mpriv...@redhat.com > > > > > > <mailto:mpriv...@redhat.com>> wrote: > > > > > > > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time > > > > > > > > > > > > Well, I should have been more careful and not merge the > patch in the > > > > > > first place. Changing API behavior is something we should > never do. > > > > > > > > > > > > Looking at the code closer, it looks like all callers of > this function > > > > > > would need to ignore the reported error so that their > behavior is not > > > > > > changed. At this point, does it make sense to report an > error in the > > > > > > function? > > > > > > > > > > > > > > > > > > The callers can decide what do with the error raised by the > function. We > > > > > > should not write functions that cannot fail. > > > > > > > > > > > > > > > > But that's not what the commit does, is it. It changed some public > APIs > > > > > from best effort to fail early. > > > > > > > > That is the side effect and I consider it as a bug. Imagine we add > some > > > > more code into that function tomorrow and there is an error path. Now > > > > because of this bug, we will have to ignore the error condition and > not > > > > report it. How ridiculous is that? > > > > > > > > We should have kept the patch as is and fix the callers so that the > public > > > > APIs were not broken. > > > > > > That is not the libvirt approach. Our #1 priority is public API > > > compatibility, everything else is subservient to that. So when we > > > have a regression we fix that in the quickest possible way. Simply > > > reverting the broken patch> > > > > (a) I take exception to the fact that the patch reverted was "broken". > > Did you actually read the bug report in the first message in this > series ? An end user hit a behaviour regression breaking the > installation process, and was caused by the patch that's been > reverted. No I did not read the bug report because it required me to create an account and then login. I do not have my redhat BZ account credentials handy.
[PATCH v2] virProcessGetStatInfo: add a comment describing why we can not report error
virProcessGetStatInfo() currently is unable to report error conditions because
that breaks libvirt's public best effort APIs. We add a comment in the function
to indicate this. Adding comment here prevents others from going down the path
of reporting error conditions in this functions in the future. It also reminds
us that at some point in the future we need to fix the code so that this
limitations no longer exists.
Please also see commit
105dace22cc7 ("Revert "report error when virProcessGetStatInfo() is unable to
parse data"")
Signed-off-by: Ani Sinha
---
src/util/virprocess.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b559a4257e..9422829b8b 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1784,6 +1784,12 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
+/* This function can not report error at present. Reporting error here
+ * causes some of libvirt's best effort public APIs to fail. This
+ * resuts in external API behavior change. Until we can fix this in
+ * a way so that public API behavior remains unchanged, we can only
+ * write a warning log here.
+ */
VIR_WARN("cannot parse process status data");
}
--
2.25.1
[PATCH] virProcessGetStatInfo: add a comment describing why we can not report error
virProcessGetStatInfo() currently is unable to report error conditions because
that breaks libvirt's public best effort APIs. We add a comment in the function
to indicate this. Adding comment here prevents others from going down the path
of reporting error conditions in this functions in the future. It also reminds
us that at some point in the future we need to fix the code so that this
limitations no longer exists.
Signed-off-by: Ani Sinha
---
src/util/virprocess.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b559a4257e..9422829b8b 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1784,6 +1784,12 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
+/* This function can not report error at present. Reporting error here
+ * causes some of libvirt's best effort public APIs to fail. This
+ * resuts in external API behavior change. Until we can fix this in
+ * a way so that public API behavior remains unchanged, we can only
+ * write a warning log here.
+ */
VIR_WARN("cannot parse process status data");
}
--
2.25.1
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Fri, 21 Jan 2022, Daniel P. Berrangé wrote: > On Fri, Jan 21, 2022 at 07:38:35PM +0530, Ani Sinha wrote: > > > > > > On Fri, 21 Jan 2022, Michal Prívozník wrote: > > > > > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník > > > <mailto:mpriv...@redhat.com>> wrote: > > > > > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > > > > > > > > > > > AKA kicking the can one more time > > > > > > > > Well, I should have been more careful and not merge the patch in the > > > > first place. Changing API behavior is something we should never do. > > > > > > > > Looking at the code closer, it looks like all callers of this > > > > function > > > > would need to ignore the reported error so that their behavior is > > > > not > > > > changed. At this point, does it make sense to report an error in the > > > > function? > > > > > > > > > > > > The callers can decide what do with the error raised by the function. We > > > > should not write functions that cannot fail. > > > > > > > > > > But that's not what the commit does, is it. It changed some public APIs > > > from best effort to fail early. > > > > That is the side effect and I consider it as a bug. Imagine we add some > > more code into that function tomorrow and there is an error path. Now > > because of this bug, we will have to ignore the error condition and not > > report it. How ridiculous is that? > > > > We should have kept the patch as is and fix the callers so that the public > > APIs were not broken. > > That is not the libvirt approach. Our #1 priority is public API > compatibility, everything else is subservient to that. So when we > have a regression we fix that in the quickest possible way. Simply > reverting the broken patch> (a) I take exception to the fact that the patch reverted was "broken". (b) I have sent a patch adding a comment to that function as to why we cannot report any error there. Wider people can message the comment in any way they like. It would remind us to fix the code in the future and at the same time prevent others from wasting their time going down the path I took.
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Fri, 21 Jan 2022, Michal Prívozník wrote: > On 1/20/22 18:23, Ani Sinha wrote: > > > > > > On Thu, Jan 20, 2022 at 21:29 Michal Prívozník > <mailto:mpriv...@redhat.com>> wrote: > > > > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > > > > > > AKA kicking the can one more time > > > > Well, I should have been more careful and not merge the patch in the > > first place. Changing API behavior is something we should never do. > > > > Looking at the code closer, it looks like all callers of this function > > would need to ignore the reported error so that their behavior is not > > changed. At this point, does it make sense to report an error in the > > function? > > > > > > The callers can decide what do with the error raised by the function. We > > should not write functions that cannot fail. > > > > But that's not what the commit does, is it. It changed some public APIs > from best effort to fail early. That is the side effect and I consider it as a bug. Imagine we add some more code into that function tomorrow and there is an error path. Now because of this bug, we will have to ignore the error condition and not report it. How ridiculous is that? We should have kept the patch as is and fix the callers so that the public APIs were not broken. > Libvirt's promise and value is in stability of its APIs. We want users > to update libvirt without having to rewrite their app (or even rebuild > it = ABI stability). And the commit broke that promise. It's only fair > that it is reverted until proper solution is found. > I have no argument with the above.
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Thu, Jan 20, 2022 at 21:29 Michal Prívozník wrote: > On 1/20/22 16:48, Ani Sinha wrote: > > > > > > > > > AKA kicking the can one more time > > Well, I should have been more careful and not merge the patch in the > first place. Changing API behavior is something we should never do. > > Looking at the code closer, it looks like all callers of this function > would need to ignore the reported error so that their behavior is not > changed. At this point, does it make sense to report an error in the > function? The callers can decide what do with the error raised by the function. We should not write functions that cannot fail. >
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Thu, Jan 20, 2022 at 21:14 Michal Prívozník wrote:
> On 1/20/22 16:31, Ani Sinha wrote:
> >
> >
> > On Thu, 20 Jan 2022, Andrea Bolognani wrote:
> >
> >> On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
> >>> +++ b/src/util/virprocess.c
> >>> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long
> *cpuTime,
> >>> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL,
> 10, ) < 0 ||
> >>> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10,
> ) < 0 ||
> >>> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL,
> 10, ) < 0) {
> >>> -virReportError(VIR_ERR_INTERNAL_ERROR,
> >>> - _("cannot parse process status data for pid
> '%d/%d'"),
> >>> - (int) pid, (int) tid);
> >>> -return -1;
> >>> +VIR_WARN("cannot parse process status data");
> >>
> >> Shame to lose the improved error/warning message. Perhaps it could be
> >> reintroduced separately.
> >>
> >
> > Functions in general are not coded around success path only. Most
> > well written functions have a success path and an error path. In case of
> > error, they should be able to report warning/errors. If raising an error
> > from a function causes a breakage of an external api, then that is an
> > architectural problem imho. Instead of reverting the error/warning, we
> > should try to fix the larger problem at hand in the longer term.
> >
>
> Well, until we get there we should fix the upstream so that we don't
> have another broken release.
AKA kicking the can one more time
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Thu, 20 Jan 2022, Andrea Bolognani wrote:
> On Tue, Jan 18, 2022 at 12:44:51PM +0100, Michal Privoznik wrote:
> > +++ b/src/util/virprocess.c
> > @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > ) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> > 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > ) < 0) {
> > -virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("cannot parse process status data for pid
> > '%d/%d'"),
> > - (int) pid, (int) tid);
> > -return -1;
> > +VIR_WARN("cannot parse process status data");
>
> Shame to lose the improved error/warning message. Perhaps it could be
> reintroduced separately.
>
Functions in general are not coded around success path only. Most
well written functions have a success path and an error path. In case of
error, they should be able to report warning/errors. If raising an error
from a function causes a breakage of an external api, then that is an
architectural problem imho. Instead of reverting the error/warning, we
should try to fix the larger problem at hand in the longer term.
Re: [PATCH] remote: systemd: Remove unix sockets from filesystem when disabling a '.socket' unit
On Tue, 18 Jan 2022, Peter Krempa wrote: > The existence of the unix socket path is used by the remote driver to > determine whether modular daemons are in use, so if the socket file > stays behind and the user decided to switch from modular to monolithic > daemon which was socket activated, the remote driver will insist on > picking '/var/run/libvirt/virtqemud-sock', even when it's no longer in > use: > > # systemctl start libvirtd.service > # virsh list > Id Name State > > > # systemctl stop libvirtd.service > Warning: Stopping libvirtd.service, but it can still be activated by: >libvirtd.socket >libvirtd-ro.socket >libvirtd-admin.socket > # systemctl start virtqemud.socket > # virsh list > Id Name State > > > # systemctl stop virtqemud.socket > # systemctl start libvirtd.service > # virsh list > error: failed to connect to the hypervisor > error: Failed to connect socket to '/var/run/libvirt/virtqemud-sock': > Connection refused > > # virsh -c 'qemu:///system?socket=/var/run/libvirt/libvirt-sock' list > Id Name State > > > Fix this by instructing systemd to delete the socket file when > deactivating the unit file for the socket. > > Signed-off-by: Peter Krempa Reviewed-by: Ani Sinha > --- > src/remote/libvirtd.socket.in | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/remote/libvirtd.socket.in b/src/remote/libvirtd.socket.in > index 85b4aa800a..0f349656f5 100644 > --- a/src/remote/libvirtd.socket.in > +++ b/src/remote/libvirtd.socket.in > @@ -9,6 +9,7 @@ Before=@service@.service > ListenStream=@runstatedir@/libvirt/@sockprefix@-sock > Service=@service@.service > SocketMode=@mode@ > +RemoveOnStop=yes > > [Install] > WantedBy=sockets.target > -- > 2.34.1 > >
Re: [PATCH] Revert "report error when virProcessGetStatInfo() is unable to parse data"
On Tue, Jan 18, 2022 at 5:15 PM Michal Privoznik
wrote:
> This reverts commit 938382b60ae5bd1f83b5cb09e1ce68b9a88f679a.
>
> Turns out, the commit did more harm than good. It changed
> semantics on some public APIs. For instance, while
> qemuDomainGetInfo() previously did not returned an error it does
> now. While the calls to virProcessGetStatInfo() is guarded with
> virDomainObjIsActive() it doesn't necessarily mean that QEMU's
> PID is still alive. QEMU might be gone but we just haven't
> realized it (e.g. because the eof handler thread is waiting for a
> job).
Doh!
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2041610
> Signed-off-by: Michal Privoznik
> ---
> src/ch/ch_driver.c | 2 ++
> src/qemu/qemu_driver.c | 7 ++-
> src/util/virprocess.c | 8 ++--
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 3cbc668489..53e0872207 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -1073,6 +1073,8 @@ chDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(>cpuTime,
>>cpu, NULL,
>vm->pid, vcpupid) < 0) {
> +virReportSystemError(errno, "%s",
> + _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e150b86cef..373cd62536 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1357,6 +1357,8 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(>cpuTime,
>>cpu, NULL,
>vm->pid, vcpupid) < 0) {
> +virReportSystemError(errno, "%s",
> + _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> @@ -2517,6 +2519,8 @@ qemuDomainGetInfo(virDomainPtr dom,
> if (virDomainObjIsActive(vm)) {
> if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
>vm->pid, 0) < 0) {
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("cannot read cputime for domain"));
> goto cleanup;
> }
> }
> @@ -10524,7 +10528,8 @@ qemuDomainMemoryStatsInternal(virQEMUDriver
> *driver,
> }
>
> if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
> -virResetLastError();
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("cannot get RSS for domain"));
> } else {
> stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
> stats[ret].val = rss;
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 85d8c8e747..b559a4257e 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1784,10 +1784,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> ) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> 0 ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> ) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot parse process status data for pid
> '%d/%d'"),
> - (int) pid, (int) tid);
> -return -1;
> +VIR_WARN("cannot parse process status data");
> }
>
> /* We got jiffies
> @@ -1884,8 +1881,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime
> G_GNUC_UNUSED,
>pid_t pid G_GNUC_UNUSED,
>pid_t tid G_GNUC_UNUSED)
> {
> -virReportSystemError(ENOSYS, "%s",
> - _("Process statistics data is not supported on
> this platform"));
> +errno = ENOSYS;
> return -1;
> }
>
> --
> 2.34.1
>
>
Re: [libvirt PATCH 01/20] qemu: Only probe KVM on Linux
On Mon, Jan 17, 2022 at 4:23 PM Andrea Bolognani
wrote:
> We already know it's not going to be available on other
> platforms.
>
> Suggested-by: Daniel P. Berrangé
> Signed-off-by: Andrea Bolognani
> Tested-by: Brad Laue
Reviewed-by: Ani Sinha
> ---
> src/qemu/qemu_process.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5c9ca0fe4f..c2c10d282a 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -9269,6 +9269,12 @@ qemuProcessQMPInit(qemuProcessQMP *proc)
> }
>
>
> +#if defined(__linux__)
> +# define hwaccel "kvm:tcg"
> +#else
> +# define hwaccel "tcg"
> +#endif
> +
> static int
> qemuProcessQMPLaunch(qemuProcessQMP *proc)
> {
> @@ -9279,7 +9285,7 @@ qemuProcessQMPLaunch(qemuProcessQMP *proc)
> if (proc->forceTCG)
> machine = "none,accel=tcg";
> else
> -machine = "none,accel=kvm:tcg";
> +machine = "none,accel=" hwaccel;
>
> VIR_DEBUG("Try to probe capabilities of '%s' via QMP, machine %s",
>proc->binary, machine);
> --
> 2.34.1
>
>
Re: [libvirt PATCH 4/4] docs: coding-style: One variable declaration per line
On Sat, Jan 15, 2022 at 02:22 Laine Stump wrote: > On 1/14/22 3:29 PM, Ján Tomko wrote: > > On a Friday in 2022, Laine Stump wrote: > >> Since it's Friday and we're talking about personal preferences - I > >> personally dislike the use of i and j (and anything else with a single > >> letter) as variable names, because it makes using a text search for > >> occurences pointless. Sure, longer variable names could also be a > >> substring of something else, and any variable could be re-used > >> elsewhere, but even then a search is mildly usable. > > > > Well, you need to search for the word i instead of the letter i. > > > > grep has the '-w' switch for that, or you can specify some boundaries: > > \bi\b > > \ > > > > vim searches for the word under the cursor with '*' by default > > > > Surely other search tools have some equivalent. > > This forced me to go look for it in emacs, and after 28 years, I've > learned about isearch-forward-symbol-at-point, which is by default bound > to [alt-s .]. But that's just another different keystroke I have to > remember. Much easier if I can just use an expansion of the ctl-s > (incremental search) that I already know and use for pretty much all > searching within a single file. Haha ! I use emacs as well and I never knew about this too. Will try it too. Thanks!
Re: [PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.
> > > index 144ba4dd12..a8f41dc8c2 100644
> > > --- a/src/conf/domain_conf.h
> > > +++ b/src/conf/domain_conf.h
> > > @@ -1187,6 +1187,12 @@ typedef enum {
> > > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST
> > > } virDomainChrConsoleTargetType;
> > >
> > > +/*
> > > + * The value of VIR_MAX_ISA_SERIAL_PORTS corresponds to
> > > MAX_ISA_SERIAL_PORTS
> > > + * set in qemu code base.
> >
> > We prefer upper case spelling of QEMU. And it's been a long time since
> > I've last booted up my machine with ISA, but IIRC it could only have 4
> > COM ports, so maybe the limit doesn't come from QEMU really but BIOS of
> > that time? What I'm trying to say is, if this is a limit shared across
> > other hypervisors then it can live under src/conf/ but if isn't shared
> > then it has to go under hypervisor specific dir (src/qemu/ in this case).
> >
> > I'm just going to assume the limit is shared and not QEMU specific.
Ok I spoke to Peter about the following commit on @qemu-devel. The
underlying limitation is indeed dictated by the hw it is emulating. Fair
enough! I still think though that we should leave the above comment as is
because this validation in libvirt is dependent on what Qemu allows today.
The reference is important IMHO.
>
> Maybe I read this wrong but I interpreted this commit message in QEMU
> repo to mean that the limitation is qemu specific :
>
> commit def337ffda34d331404bd7f1a42726b71500df22
> Author: Peter Maydell
> Date: Fri Apr 20 15:52:46 2018 +0100
>
> serial-isa: Use MAX_ISA_SERIAL_PORTS instead of MAX_SERIAL_PORTS
>
> The ISA serial port handling in serial-isa.c imposes a limit
> of 4 serial ports. This is because we only know of 4 IO port
> and IRQ settings for them, and is unrelated to the generic
> MAX_SERIAL_PORTS limit, though they happen to both be set at
> 4 currently.
>
> Use a new MAX_ISA_SERIAL_PORTS wherever that is the correct
> limit to be checking against.
>
> Signed-off-by: Peter Maydell
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Thomas Huth
> Tested-by: Philippe Mathieu-Daudé
> Message-id: 20180420145249.32435-11-peter.mayd...@linaro.org
>
>
Re: [PATCH 0/2 for 8.0] Update index after allocating port for isa-serial device
On Thu, Jan 13, 2022 at 10:30 PM Divya Garg wrote: > > On 13/01/22 9:41 pm, Peter Krempa wrote: > > On Thu, Jan 13, 2022 at 21:27:24 +0530, Ani Sinha wrote: > > On Thu, Jan 13, 2022 at 20:13 Michal Prívozník > wrote: > > > On 1/13/22 08:33, Divya Garg wrote: > > Speaking of which, we > > are currently in freeze, preparing for tomorrow's release. so I'll push > these tomorrow. > > > > Shouldn’t the bug fix patch have been part of tomorrows release? I thought > bug fixes can be merged even when there is a code freeze. Just trying to > understand the process here. > > No, this change is too invasive IMO and Michal seems to think the same. > The boundaries are not exact and it's up to the person > commiting/approving the code to make the ultimative decision. But the > gist is that you don't push into a RC something which might break the > release. > > Hi ! Thankyou so much. So will you like me to post a new iteration, since > today has a code freeze anyway ? Or will it be okay to merge this change ? > Michal has already said he will push your change after the release tomorrow. At the same time he will fix things he was suggesting. So you can relax and wait for him to push. No need to churn out a new iteration.
Re: [PATCH 0/2 for 8.0] Update index after allocating port for isa-serial device
On Thu, Jan 13, 2022 at 20:13 Michal Prívozník wrote: > On 1/13/22 08:33, Divya Garg wrote: > > Speaking of which, we > are currently in freeze, preparing for tomorrow's release. so I'll push > these tomorrow. Shouldn’t the bug fix patch have been part of tomorrows release? I thought bug fixes can be merged even when there is a code freeze. Just trying to understand the process here. > > >
Re: [PATCH 1/2 for 8.0] Add the port allocation logic for isa-serial devices.
On Thu, 13 Jan 2022, Michal Prívozník wrote:
> > index 144ba4dd12..a8f41dc8c2 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1187,6 +1187,12 @@ typedef enum {
> > VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST
> > } virDomainChrConsoleTargetType;
> >
> > +/*
> > + * The value of VIR_MAX_ISA_SERIAL_PORTS corresponds to
> > MAX_ISA_SERIAL_PORTS
> > + * set in qemu code base.
>
> We prefer upper case spelling of QEMU. And it's been a long time since
> I've last booted up my machine with ISA, but IIRC it could only have 4
> COM ports, so maybe the limit doesn't come from QEMU really but BIOS of
> that time? What I'm trying to say is, if this is a limit shared across
> other hypervisors then it can live under src/conf/ but if isn't shared
> then it has to go under hypervisor specific dir (src/qemu/ in this case).
>
> I'm just going to assume the limit is shared and not QEMU specific.
Maybe I read this wrong but I interpreted this commit message in QEMU
repo to mean that the limitation is qemu specific :
commit def337ffda34d331404bd7f1a42726b71500df22
Author: Peter Maydell
Date: Fri Apr 20 15:52:46 2018 +0100
serial-isa: Use MAX_ISA_SERIAL_PORTS instead of MAX_SERIAL_PORTS
The ISA serial port handling in serial-isa.c imposes a limit
of 4 serial ports. This is because we only know of 4 IO port
and IRQ settings for them, and is unrelated to the generic
MAX_SERIAL_PORTS limit, though they happen to both be set at
4 currently.
Use a new MAX_ISA_SERIAL_PORTS wherever that is the correct
limit to be checking against.
Signed-off-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Thomas Huth
Tested-by: Philippe Mathieu-Daudé
Message-id: 20180420145249.32435-11-peter.mayd...@linaro.org
Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port
On Thu, Jan 13, 2022 at 4:50 PM John Levon wrote:
> On Thu, Jan 13, 2022 at 03:11:51PM +0530, Ani Sinha wrote:
>
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index d822533ccb..4130df0ed9 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const
> virDomainDef *def,
> > > g_autoptr(virJSONValue) props = NULL;
> > > g_autofree char *chardev = g_strdup_printf("char%s",
> serial->info.alias);
> > > virQEMUCapsFlags caps;
> > > +const char *typestr;
> > > +int ret;
> >
> > type should match the return type of this function.
>
> It does match return type of virDomainChrSerialTargetModelTypeToString():
>
> 47 #define VIR_ENUM_DECL(name) \
>
> 48 const char *name ## TypeToString(int type); \
Yes you are correct. I was thinking of qemuBuildSerialChrDeviceProps() but
it does not return ret directly from this function. Another reason it makes
me uncomfortable.
> > I preferred your previous style to this one.
>
> Below is cleaner IMO: we don't repeat code, and the flow is much clearer.
If you look at other functions for example qemuBuildVirtioDevProps() etc they
follow the previous style. I think it's fine to return NULL from multiple
points.
In any case if the maintainers are fine with it , I'm ok. Style is trivial.
Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port
On Thu, 13 Jan 2022, Ani Sinha wrote:
>
>
> On Wed, 12 Jan 2022, Divya Garg wrote:
>
> > VM XML accepts target.port but this does not get passed while building the
> > qemu
> > command line for this VM.
> >
> > Signed-off-by: Divya Garg
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index d822533ccb..4130df0ed9 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const virDomainDef
> > *def,
> > g_autoptr(virJSONValue) props = NULL;
> > g_autofree char *chardev = g_strdup_printf("char%s",
> > serial->info.alias);
> > virQEMUCapsFlags caps;
> > +const char *typestr;
> > +int ret;
>
> type should match the return type of this function. ret should be defined
> as virJSONValue. I preferred your previous style to this one.
>
> Also please rebase this patch to the latest git HEAD and run "ninja test"
> to make sure all tests passes. When I applied this patch to my tree,
> qemuxml2argvtest failed. I think some more xmls needs fixing.
oops! forgot to prune the huge set of xml changes not relavent to the
context of my response from my last email. Apologies for that.
Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port
On Wed, 12 Jan 2022, Divya Garg wrote:
> VM XML accepts target.port but this does not get passed while building the
> qemu
> command line for this VM.
>
> Signed-off-by: Divya Garg
> ---
> src/qemu/qemu_command.c | 25 +++
> tests/qemuxml2argvdata/bios.args | 2 +-
> .../qemuxml2argvdata/console-compat-auto.args | 2 +-
> .../console-compat-auto.x86_64-latest.args| 2 +-
> .../console-compat-chardev.args | 2 +-
> .../console-compat-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/console-compat.args| 2 +-
> .../console-compat.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/console-virtio-many.args | 2 +-
> tests/qemuxml2argvdata/controller-order.args | 2 +-
> .../name-escape.x86_64-2.11.0.args| 4 +--
> .../name-escape.x86_64-latest.args| 4 +--
> .../q35-virt-manager-basic.args | 2 +-
> .../serial-dev-chardev-iobase.args| 2 +-
> ...rial-dev-chardev-iobase.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-dev-chardev.args | 2 +-
> .../serial-dev-chardev.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-file-chardev.args | 2 +-
> .../serial-file-chardev.x86_64-latest.args| 2 +-
> tests/qemuxml2argvdata/serial-file-log.args | 2 +-
> .../serial-file-log.x86_64-latest.args| 2 +-
> .../qemuxml2argvdata/serial-many-chardev.args | 4 +--
> .../serial-many-chardev.x86_64-latest.args| 4 +--
> .../qemuxml2argvdata/serial-pty-chardev.args | 2 +-
> .../serial-pty-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/serial-spiceport.args | 2 +-
> .../serial-spiceport.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-tcp-chardev.args | 2 +-
> .../serial-tcp-chardev.x86_64-latest.args | 2 +-
> .../serial-tcp-telnet-chardev.args| 2 +-
> ...rial-tcp-telnet-chardev.x86_64-latest.args | 2 +-
> .../serial-tcp-tlsx509-chardev-notls.args | 4 +--
> ...p-tlsx509-chardev-notls.x86_64-latest.args | 4 +--
> .../serial-tcp-tlsx509-chardev-verify.args| 4 +--
> ...-tlsx509-chardev-verify.x86_64-latest.args | 4 +--
> .../serial-tcp-tlsx509-chardev.args | 4 +--
> ...ial-tcp-tlsx509-chardev.x86_64-latest.args | 4 +--
> .../serial-tcp-tlsx509-secret-chardev.args| 4 +--
> ...-tlsx509-secret-chardev.x86_64-latest.args | 4 +--
> .../qemuxml2argvdata/serial-udp-chardev.args | 4 +--
> .../serial-udp-chardev.x86_64-latest.args | 4 +--
> .../qemuxml2argvdata/serial-unix-chardev.args | 4 +--
> .../serial-unix-chardev.x86_64-latest.args| 4 +--
> tests/qemuxml2argvdata/serial-vc-chardev.args | 2 +-
> .../serial-vc-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/user-aliases.args | 4 +--
> .../virtio-9p-createmode.x86_64-latest.args | 2 +-
> .../virtio-9p-multidevs.x86_64-latest.args| 2 +-
> .../x86_64-pc-graphics.x86_64-latest.args | 2 +-
> .../x86_64-pc-headless.x86_64-latest.args | 2 +-
> .../x86_64-q35-graphics.x86_64-latest.args| 2 +-
> .../x86_64-q35-headless.x86_64-latest.args| 2 +-
> 52 files changed, 88 insertions(+), 73 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d822533ccb..4130df0ed9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
> g_autoptr(virJSONValue) props = NULL;
> g_autofree char *chardev = g_strdup_printf("char%s", serial->info.alias);
> virQEMUCapsFlags caps;
> +const char *typestr;
> +int ret;
type should match the return type of this function. ret should be defined
as virJSONValue. I preferred your previous style to this one.
Also please rebase this patch to the latest git HEAD and run "ninja test"
to make sure all tests passes. When I applied this patch to my tree,
qemuxml2argvtest failed. I think some more xmls needs fixing.
>
> switch ((virDomainChrSerialTargetModel) serial->targetModel) {
> case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
> @@ -10750,11 +10752,24 @@ qemuBuildSerialChrDeviceProps(const virDomainDef
> *def,
> return NULL;
> }
>
> -if (virJSONValueObjectAdd(,
> - "s:driver",
> virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> - "s:chardev", chardev,
> - "s:id", serial->info.alias,
> - NULL) < 0)
> +typestr = virDomainChrSerialTargetModelTypeToString(serial->targetModel);
> +
> +if (serial->targetModel ==
> VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> +ret = virJSONValueObjectAdd(,
> +"s:driver", typestr,
> +"s:chardev", chardev,
> +"s:id", serial->info.alias,
Re: [PATCH v4] report error when virProcessGetStatInfo() is unable to parse data
On Wed, Jan 12, 2022 at 6:49 PM Michal Prívozník
wrote:
> On 1/11/22 19:13, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs
> error
> > when it is unable to parse the data. Make this function actually report
> the
> > error and return a negative value in this error scenario.
> >
> > Fix the callers so that they do not override the error generated.
> > Also fix non-linux implementation of this function so as to report error.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/ch/ch_driver.c | 2 --
> > src/qemu/qemu_driver.c | 7 +--
> > src/util/virprocess.c | 17 +
> > 3 files changed, 14 insertions(+), 12 deletions(-)
> >
> > changelog:
> > v4: on freebsd error on the logs is a problem appaently. try to fix
> > that.
> > v3: fix non linux implementation
> > v2: fix callers
> >
> > diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> > index 53e0872207..3cbc668489 100644
> > --- a/src/ch/ch_driver.c
> > +++ b/src/ch/ch_driver.c
> > @@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
> > if (virProcessGetStatInfo(>cpuTime,
> >>cpu, NULL,
> >vm->pid, vcpupid) < 0) {
> > -virReportSystemError(errno, "%s",
> > - _("cannot get vCPU placement &
> pCPU time"));
> > return -1;
> > }
> > }
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index d3d76c003f..65ac5ef367 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
> > if (virProcessGetStatInfo(>cpuTime,
> >>cpu, NULL,
> >vm->pid, vcpupid) < 0) {
> > -virReportSystemError(errno, "%s",
> > - _("cannot get vCPU placement &
> pCPU time"));
> > return -1;
> > }
> > }
> > @@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
> > if (virDomainObjIsActive(vm)) {
> > if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
> >vm->pid, 0) < 0) {
> > -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > - _("cannot read cputime for domain"));
> > goto cleanup;
> > }
> > }
> > @@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver
> *driver,
> > }
> >
> > if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
> > -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > - _("cannot get RSS for domain"));
> > +virResetLastError();
> > } else {
> > stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
> > stats[ret].val = rss;
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index b559a4257e..cbb31441cc 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1779,12 +1779,20 @@ virProcessGetStatInfo(unsigned long long
> *cpuTime,
> > long rss = 0;
> > int cpu = 0;
> >
> > -if (!proc_stat ||
> > -virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
> ) < 0 ||
> > +if (!proc_stat) {
> > +VIR_WARN("Unable to get proc stats from the kernel");
> > +return 0;
> > +}
> > +
>
> I don't think I've objected to this part in v3, any reason for changing
> it? Because now it's actually worse. For instance in
> qemuDomainMemoryStatsInternal() there's the following:
>
> long rss;
>
> virProcessGetStatInfo(NULL, NULL, , vm->pid, 0);
>
> Now, if virProcessGetStatInfo() returns 0 is @rss set? Depends whether
> @proc_stat was NULL or not. But the caller has no way of figuring that out.
>
> I believe all I wanted for you to do was to not change the semantics of
> qemuDomainMemoryStatsInternal(). I even suggested how to do that.
Yes I did that in v4.
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+virResetLastError();
I was also trying to find a solution for the logging issue as well as a
part of v4 which clearly was incorrect.
>
Re: [PATCH v4] report error when virProcessGetStatInfo() is unable to parse data
I just realized after sending this patch that I made the change in the
Linux version of the function. It may not be applicable for FreeBSD .
In any case I simply don’t understand if errors on the logs is a problem.
If it is maybe we need a FreeBSD version of this function as well not just
a larger bucket on non Linux flavours that report error .
On Tue, Jan 11, 2022 at 23:43 Ani Sinha wrote:
> Currently virProcessGetStatInfo() always returns success and only logs
> error
> when it is unable to parse the data. Make this function actually report the
> error and return a negative value in this error scenario.
>
> Fix the callers so that they do not override the error generated.
> Also fix non-linux implementation of this function so as to report error.
>
> Signed-off-by: Ani Sinha
> ---
> src/ch/ch_driver.c | 2 --
> src/qemu/qemu_driver.c | 7 +--
> src/util/virprocess.c | 17 +
> 3 files changed, 14 insertions(+), 12 deletions(-)
>
> changelog:
> v4: on freebsd error on the logs is a problem appaently. try to fix
> that.
> v3: fix non linux implementation
> v2: fix callers
>
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 53e0872207..3cbc668489 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(>cpuTime,
>>cpu, NULL,
>vm->pid, vcpupid) < 0) {
> -virReportSystemError(errno, "%s",
> - _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d3d76c003f..65ac5ef367 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
> if (virProcessGetStatInfo(>cpuTime,
>>cpu, NULL,
>vm->pid, vcpupid) < 0) {
> -virReportSystemError(errno, "%s",
> - _("cannot get vCPU placement & pCPU
> time"));
> return -1;
> }
> }
> @@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
> if (virDomainObjIsActive(vm)) {
> if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
>vm->pid, 0) < 0) {
> -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("cannot read cputime for domain"));
> goto cleanup;
> }
> }
> @@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver
> *driver,
> }
>
> if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
> -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> - _("cannot get RSS for domain"));
> +virResetLastError();
> } else {
> stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
> stats[ret].val = rss;
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index b559a4257e..cbb31441cc 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1779,12 +1779,20 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> long rss = 0;
> int cpu = 0;
>
> -if (!proc_stat ||
> -virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
> ) < 0 ||
> +if (!proc_stat) {
> +VIR_WARN("Unable to get proc stats from the kernel");
> +return 0;
> +}
> +
> +if (virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
> ) < 0 ||
> virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> ) < 0 ||
> virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> 0 ||
> virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> ) < 0) {
> -VIR_WARN("cannot parse process status data");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot parse process status data for pid
> '%d/%d'"),
> + (int) pid, (int) tid);
> +
> +return -1;
> }
>
> /* We got jiffies
> @@ -1881,7 +1889,8 @@ virProcessGetStatInfo(unsigned long long *cpuTime
> G_GNUC_UNUSED,
>pid_t pid G_GNUC_UNUSED,
>pid_t tid G_GNUC_UNUSED)
> {
> -errno = ENOSYS;
> +virReportSystemError(ENOSYS, "%s",
> + _("Process statistics data is not supported on
> this platform"));
> return -1;
> }
>
> --
> 2.25.1
>
>
Re: [PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
On Tue, 11 Jan 2022, Michal Prívozník wrote:
> On 1/11/22 11:20, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs error
> > when it is unable to parse the data. Make this function actually report the
> > error and return a negative value in this error scenario.
> >
> > Fix the callers so that they do not override the error generated.
> > Also fix non-linux implementation of this function so as to report error.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/ch/ch_driver.c | 2 --
> > src/qemu/qemu_driver.c | 7 +--
> > src/util/virprocess.c | 9 +++--
> > 3 files changed, 8 insertions(+), 10 deletions(-)
> >
> > changelog:
> > v3: fix non-linux implementation
> > v2: fix callers
> >
>
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index d3d76c003f..9a17c93b08 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
>
> > @@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
> > }
> >
> > if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
> > -virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > - _("cannot get RSS for domain"));
> > +return -1;
>
> Returning -1 changes semantics of this function. Previously it was
> tolerant to errors and now it isn't. For instance, if this function was
> called on a FreeBSD machine (yes, QEMU driver can be enabled there) then
> despite fetching mem stats earlier through monitor (not visible in the
> context) an error is now returned which in turn makes whole
> virDomainMemoryStats() API fail.
>
> The 'return -1' should be replaced with virResetLastError().
>
> Having said that, now there will be an error reported in the logs every
> time the API is called. I wonder what we can do about it. Previously it
> was "just" a warning.
Does v4 help?
[PATCH v4] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Also fix non-linux implementation of this function so as to report error.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 17 +
3 files changed, 14 insertions(+), 12 deletions(-)
changelog:
v4: on freebsd error on the logs is a problem appaently. try to fix
that.
v3: fix non linux implementation
v2: fix callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d3d76c003f..65ac5ef367 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
if (virDomainObjIsActive(vm)) {
if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot read cputime for domain"));
goto cleanup;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+virResetLastError();
} else {
stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b559a4257e..cbb31441cc 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1779,12 +1779,20 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
long rss = 0;
int cpu = 0;
-if (!proc_stat ||
-virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
) < 0 ||
+if (!proc_stat) {
+VIR_WARN("Unable to get proc stats from the kernel");
+return 0;
+}
+
+if (virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10,
) < 0 ||
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
@@ -1881,7 +1889,8 @@ virProcessGetStatInfo(unsigned long long *cpuTime
G_GNUC_UNUSED,
pid_t pid G_GNUC_UNUSED,
pid_t tid G_GNUC_UNUSED)
{
-errno = ENOSYS;
+virReportSystemError(ENOSYS, "%s",
+ _("Process statistics data is not supported on this
platform"));
return -1;
}
--
2.25.1
Re: [RFC v3 1/2] Add the port allocation logic for isa-serial devices.
On Tue, 11 Jan 2022, Divya Garg wrote: > > On 11/01/22 4:09 pm, Ani Sinha wrote: > > Please add SOB. > Yes Thankyou so much. Should I have a new version for it or on the same thread > send > the updated patches ? I am fine with your bugfix patch patch 2. DanPB and others, could you guys provide some feedback on both patches before she spins out another version with SOB added?
Re: [RFC v3 1/2] Add the port allocation logic for isa-serial devices.
Please add SOB.
On Mon, 10 Jan 2022, Divya Garg wrote:
> This commit takes care of following cases:
> -> Check availability of requested ports.
> ->The total number of requested ports should not be more than
> VIR_MAX_ISA_SERIAL_PORTS.
> ->The ports requested should be less than VIR_MAX_ISA_SERIAL_PORTS.
> ->VIR_MAX_ISA_SERIAL_PORTS should correspond to MAX_ISA_SERIAL_PORTS
> specified in qemu code commit def337ffda34d331404bd7f1a42726b71500df22.
> -> Prevent duplicate device assignments to the same port.
> -> In case no ports are provided in the XML, this patch scans the list of
> unused
>isa-serial indices to automatically assign available ports for this VM.
> ---
> src/conf/domain_conf.c| 61 ---
> src/conf/domain_conf.h| 6 ++
> ...g-console-compat-2-live+console-virtio.xml | 4 +-
> .../qemuhotplug-console-compat-2-live.xml | 4 +-
> .../serial-tcp-tlsx509-chardev-notls.xml | 2 +-
> .../serial-tcp-tlsx509-chardev-verify.xml | 2 +-
> .../serial-tcp-tlsx509-chardev.xml| 2 +-
> .../serial-tcp-tlsx509-secret-chardev.xml | 2 +-
> .../serial-tcp-tlsx509-chardev.xml| 2 +-
> 9 files changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5691b8d2d5..e468e98045 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5330,6 +5330,56 @@ virDomainHostdevDefPostParse(virDomainHostdevDef *dev,
> }
>
>
> +static int
> +virDomainChrIsaSerialDefPostParse(virDomainDef *def)
> +{
> +size_t i, j;
> +size_t isa_serial_count = 0;
> +bool used_serial_port[VIR_MAX_ISA_SERIAL_PORTS] = {false};
> +
> +/* Perform all the required checks. */
> +for (i = 0; i < def->nserials; i++) {
> +
> +if (def->serials[i]->targetType !=
> +VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL)
> +continue;
> +
> +if (isa_serial_count++ >= VIR_MAX_ISA_SERIAL_PORTS ||
> +def->serials[i]->target.port >= VIR_MAX_ISA_SERIAL_PORTS) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Maximum supported number of ISA serial ports
> is '%d'."), VIR_MAX_ISA_SERIAL_PORTS);
> +return -1;
> +}
> +
> +if (def->serials[i]->target.port != -1) {
> +if (used_serial_port[def->serials[i]->target.port]) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("target port '%d' already allocated."),
> def->serials[i]->target.port);
> +return -1;
> +}
> +used_serial_port[def->serials[i]->target.port] = true;
> +}
> +}
> +
> +/* Assign the ports to the devices. */
> +for (i = 0; i < def->nserials; i++) {
> +
> +if (def->serials[i]->targetType !=
> +VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL ||
> +def->serials[i]->target.port != -1)
> +continue;
> +
> +for (j = 0; j < VIR_MAX_ISA_SERIAL_PORTS; j++) {
> +if (!used_serial_port[j]) {
> +def->serials[i]->target.port = j;
> +used_serial_port[j] = true;
> +break;
> +}
> +}
> +}
> +return 0;
> +}
> +
> static void
> virDomainChrDefPostParse(virDomainChrDef *chr,
> const virDomainDef *def)
> @@ -6197,6 +6247,9 @@ virDomainDefPostParse(virDomainDef *def,
> goto cleanup;
> }
>
> +if (virDomainChrIsaSerialDefPostParse(def) < 0)
> +return -1;
> +
> /* iterate the devices */
> ret = virDomainDeviceInfoIterateFlags(def,
>
> virDomainDefPostParseDeviceIterator,
> @@ -19929,14 +19982,6 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt,
> if (!chr)
> return NULL;
>
> -if (chr->target.port == -1) {
> -int maxport = -1;
> -for (j = 0; j < i; j++) {
> -if (def->serials[j]->target.port > maxport)
> -maxport = def->serials[j]->target.port;
> -}
> -chr->target.port = maxport + 1;
> -}
> def->serials[def->nserials++] = chr;
> }
> VIR_FREE(nodes);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 144ba4dd12..a8f41dc8c2 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1187,6 +1187,12 @@ typedef enum {
> VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_LAST
> } virDomainChrConsoleTargetType;
>
> +/*
> + * The value of VIR_MAX_ISA_SERIAL_PORTS corresponds to MAX_ISA_SERIAL_PORTS
> + * set in qemu code base.
> + */
> +#define VIR_MAX_ISA_SERIAL_PORTS 4
> +
> typedef enum {
> VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_NONE = 0,
> VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL,
> diff --git
>
Re: [RFC v3 2/2] qemu: add index for isa-serial device using target.port
SOB line is missing. Patches are not accepted without SOB.
On Mon, 10 Jan 2022, Divya Garg wrote:
> VM XML accepts target.port but this does not get passed while building the
> qemu
> command line for this VM.
> ---
> src/qemu/qemu_command.c | 22 ++-
> tests/qemuxml2argvdata/bios.args | 2 +-
> .../qemuxml2argvdata/console-compat-auto.args | 2 +-
> .../console-compat-auto.x86_64-latest.args| 2 +-
> .../console-compat-chardev.args | 2 +-
> .../console-compat-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/console-compat.args| 2 +-
> .../console-compat.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/console-virtio-many.args | 2 +-
> tests/qemuxml2argvdata/controller-order.args | 2 +-
> .../name-escape.x86_64-2.11.0.args| 4 ++--
> .../name-escape.x86_64-latest.args| 4 ++--
> .../q35-virt-manager-basic.args | 2 +-
> .../serial-dev-chardev-iobase.args| 2 +-
> ...rial-dev-chardev-iobase.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-dev-chardev.args | 2 +-
> .../serial-dev-chardev.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-file-chardev.args | 2 +-
> .../serial-file-chardev.x86_64-latest.args| 2 +-
> tests/qemuxml2argvdata/serial-file-log.args | 2 +-
> .../serial-file-log.x86_64-latest.args| 2 +-
> .../qemuxml2argvdata/serial-many-chardev.args | 4 ++--
> .../serial-many-chardev.x86_64-latest.args| 4 ++--
> .../qemuxml2argvdata/serial-pty-chardev.args | 2 +-
> .../serial-pty-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/serial-spiceport.args | 2 +-
> .../serial-spiceport.x86_64-latest.args | 2 +-
> .../qemuxml2argvdata/serial-tcp-chardev.args | 2 +-
> .../serial-tcp-chardev.x86_64-latest.args | 2 +-
> .../serial-tcp-telnet-chardev.args| 2 +-
> ...rial-tcp-telnet-chardev.x86_64-latest.args | 2 +-
> .../serial-tcp-tlsx509-chardev-notls.args | 4 ++--
> ...p-tlsx509-chardev-notls.x86_64-latest.args | 4 ++--
> .../serial-tcp-tlsx509-chardev-verify.args| 4 ++--
> ...-tlsx509-chardev-verify.x86_64-latest.args | 4 ++--
> .../serial-tcp-tlsx509-chardev.args | 4 ++--
> ...ial-tcp-tlsx509-chardev.x86_64-latest.args | 4 ++--
> .../serial-tcp-tlsx509-secret-chardev.args| 4 ++--
> ...-tlsx509-secret-chardev.x86_64-latest.args | 4 ++--
> .../qemuxml2argvdata/serial-udp-chardev.args | 4 ++--
> .../serial-udp-chardev.x86_64-latest.args | 4 ++--
> .../qemuxml2argvdata/serial-unix-chardev.args | 4 ++--
> .../serial-unix-chardev.x86_64-latest.args| 4 ++--
> tests/qemuxml2argvdata/serial-vc-chardev.args | 2 +-
> .../serial-vc-chardev.x86_64-latest.args | 2 +-
> tests/qemuxml2argvdata/user-aliases.args | 4 ++--
> .../virtio-9p-createmode.x86_64-latest.args | 2 +-
> .../virtio-9p-multidevs.x86_64-latest.args| 2 +-
> .../x86_64-pc-graphics.x86_64-latest.args | 2 +-
> .../x86_64-pc-headless.x86_64-latest.args | 2 +-
> .../x86_64-q35-graphics.x86_64-latest.args| 2 +-
> .../x86_64-q35-headless.x86_64-latest.args| 2 +-
> 52 files changed, 85 insertions(+), 73 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d822533ccb..a1fd5443be 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10717,6 +10717,7 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
> {
> g_autoptr(virJSONValue) props = NULL;
> g_autofree char *chardev = g_strdup_printf("char%s", serial->info.alias);
> +const char *typestr;
> virQEMUCapsFlags caps;
>
> switch ((virDomainChrSerialTargetModel) serial->targetModel) {
> @@ -10750,11 +10751,22 @@ qemuBuildSerialChrDeviceProps(const virDomainDef
> *def,
> return NULL;
> }
>
> -if (virJSONValueObjectAdd(,
> - "s:driver",
> virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> - "s:chardev", chardev,
> - "s:id", serial->info.alias,
> - NULL) < 0)
> +typestr = virDomainChrSerialTargetModelTypeToString(serial->targetModel);
> +
> +if (serial->targetModel ==
> VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> +if (virJSONValueObjectAdd(,
> + "s:driver", typestr,
> + "s:chardev", chardev,
> + "s:id", serial->info.alias,
> + "k:index", serial->target.port,
> + NULL) < 0)
> +return NULL;
> +}
> +else if (virJSONValueObjectAdd(,
> + "s:driver", typestr,
> + "s:chardev", chardev,
> +
[PATCH v3] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Also fix non-linux implementation of this function so as to report error.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 9 +++--
3 files changed, 8 insertions(+), 10 deletions(-)
changelog:
v3: fix non-linux implementation
v2: fix callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d3d76c003f..9a17c93b08 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
if (virDomainObjIsActive(vm)) {
if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot read cputime for domain"));
goto cleanup;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+return -1;
} else {
stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b559a4257e..709ec616de 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1784,7 +1784,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
@@ -1881,7 +1885,8 @@ virProcessGetStatInfo(unsigned long long *cpuTime
G_GNUC_UNUSED,
pid_t pid G_GNUC_UNUSED,
pid_t tid G_GNUC_UNUSED)
{
-errno = ENOSYS;
+virReportSystemError(ENOSYS, "%s",
+ _("Process statistics data is not supported on this
platform"));
return -1;
}
--
2.25.1
Re: [PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
On Tue, 11 Jan 2022, Michal Prívozník wrote:
> On 1/11/22 04:42, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs error
> > when it is unable to parse the data. Make this function actually report the
> > error and return a negative value in this error scenario.
> >
> > Fix the callers so that they do not override the error generated.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/ch/ch_driver.c | 2 --
> > src/qemu/qemu_driver.c | 7 +--
> > src/util/virprocess.c | 6 +-
> > 3 files changed, 6 insertions(+), 9 deletions(-)
> >
> > changelog:
> > v2: fixed the callers
> >
>
>
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index c74bd16fe6..b9f498d5d8 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > ) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> > 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > ) < 0) {
> > -VIR_WARN("cannot parse process status data");
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("cannot parse process status data for pid
> > '%d/%d'"),
> > + (int) pid, (int) tid);
> > +
> > +return -1;
> > }
> >
> > /* We got jiffies
>
> There's an alternative implementation for non-Linux platforms, but you
but that has not been pushed yet I believe. Correct me if I am mistaken.
> are introducing error reporting only to this implementation. Both
> versions have to behave the same, i.e. the non-Linux implementation now
> has to report error too. Can you fix that in v3?
I think we should keep the scope of this patch as is. Why don't we do
this. Lets push this patch. Then I will fix the patch for non-linux
platforms and send it over.
[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 6 +-
3 files changed, 6 insertions(+), 9 deletions(-)
changelog:
v2: fixed the callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4974450333..015ffb2ce7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
if (virDomainObjIsActive(vm)) {
if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot read cputime for domain"));
goto cleanup;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+return -1;
} else {
stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
--
2.25.1
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Michal Prívozník wrote: > On 1/7/22 13:38, Ani Sinha wrote: > > > > > > > Ok fine but still, life is not ideal ... libraries do have bugs. > > In that case, where do we draw the line? Say pthread has a bug and it > doesn't spawn threads. Worse, it doesn't even return any error value. > Should we mitigate that too? > > I'd say stick with what's documented (abort on OOM and/or !NULL > returned) and make our lives simpler. In fact, if we'd crash because we > accessed NULL we will immediately see where and why and can report the > bug to glib for benefit of us and others. Yes, so if I were you, I would not simply remove the check. I would replace it with assert().
Re: [PATCH] do not report generic OPERATION_FAILED error when calling virConnectOpenAuth()
Ping ...
On Thu, 6 Jan 2022, Ani Sinha wrote:
> virConnectOpenAuth() calls virConnectOpenInternal(). This later function
> generates fine grained errors arising from various failure conditions that are
> more accurate than a "catch all" broader VIR_ERR_OPERATION_FAILED error that
> the callers of this function generates. Remove the broader error so that more
> specific errors can be caught and processed.
>
> Signed-off-by: Ani Sinha
> ---
> src/libxl/libxl_migration.c | 3 ---
> src/qemu/qemu_migration.c | 3 ---
> 2 files changed, 6 deletions(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 6d0ab4ee28..bc2b5401da 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -1134,9 +1134,6 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivate
> *driver,
> virObjectLock(vm);
>
> if (dconn == NULL) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Failed to connect to remote libvirt URI %s: %s"),
> - dconnuri, virGetLastErrorMessage());
> return ret;
> }
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9d7d582f5..2635ef1162 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5145,9 +5145,6 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
> goto cleanup;
>
> if (dconn == NULL) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Failed to connect to remote libvirt URI %s: %s"),
> - dconnuri, virGetLastErrorMessage());
> return -1;
> }
>
> --
> 2.25.1
>
>
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> On Fri, Jan 07, 2022 at 06:04:04PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> >
> > > On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> > > >
> > > >
> > > > On Fri, 7 Jan 2022, Ján Tomko wrote:
> > > >
> > > > > On a Friday in 2022, Ani Sinha wrote:
> > > > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > > > I don't think so. Just like we've discussed under one patch of
> > > > > > > yours, a
> > > > > > > function should either report error in all cases or none. And in
> > > > > > > case of
> > > > > > > virProcessGetSchedInfo() the linux version does report error
> > > > > >
> > > > > > I see your point but there is also a bug in that function - not all
> > > > > > error
> > > > > > paths report errors. For example, !proc and !lines cases. We need
> > > > > > to fix
> > > > > > that.
> > > > > >
> > > > >
> > > > > I don't see a !proc error path in virProcessGetSchedInfo.
> > > > >
> > > >
> > > >if (tid)
> > > > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid,
> > > > (int)
> > > > tid);
> > > > else
> > > > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > > > if (!proc)
> > > > return -1; <=== not reported
> > >
> > > g_strdup_printf can't fail unless we mangled the printf format string
> > > (which we havent), so that 'if (!proc)' check is redundant / unreachable
> > >
> >
> > I am ok with Michal's patch that removes this check. However, such
> > assumptutions does makes me nervous. Since we do not really have
> > control over the library that implements g_strdup_printf, what if the
> > "can't fail" logic changes one day? This can happen deliberately or due to
> > some bug in the library. Does it not make sense to be defensive as the
> > consumer of this function and write code that is future proof?
>
> Their API spec guarantees we are safe
>
> https://docs.gtk.org/glib/func.strdup_printf.html
>
> "The returned string is guaranteed to be non-NULL, unless format contains
> %lc or %ls conversions, which can fail if no multibyte representation is
> available for the given character."
>
Ok fine but still, life is not ideal ... libraries do have bugs.
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Daniel P. Berrangé wrote:
> On Fri, Jan 07, 2022 at 05:24:18PM +0530, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours,
> > > > > a
> > > > > function should either report error in all cases or none. And in case
> > > > > of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all
> > > > error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> >if (tid)
> > proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> > else
> > proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> > if (!proc)
> > return -1; <=== not reported
>
> g_strdup_printf can't fail unless we mangled the printf format string
> (which we havent), so that 'if (!proc)' check is redundant / unreachable
>
I am ok with Michal's patch that removes this check. However, such
assumptutions does makes me nervous. Since we do not really have
control over the library that implements g_strdup_printf, what if the
"can't fail" logic changes one day? This can happen deliberately or due to
some bug in the library. Does it not make sense to be defensive as the
consumer of this function and write code that is future proof?
[PATCH v2] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Fix the callers so that they do not override the error generated.
Signed-off-by: Ani Sinha
---
src/ch/ch_driver.c | 2 --
src/qemu/qemu_driver.c | 7 +--
src/util/virprocess.c | 6 +-
3 files changed, 6 insertions(+), 9 deletions(-)
changelog:
v2: fixed the callers
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
index 53e0872207..3cbc668489 100644
--- a/src/ch/ch_driver.c
+++ b/src/ch/ch_driver.c
@@ -1073,8 +1073,6 @@ chDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4974450333..015ffb2ce7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1359,8 +1359,6 @@ qemuDomainHelperGetVcpus(virDomainObj *vm,
if (virProcessGetStatInfo(>cpuTime,
>cpu, NULL,
vm->pid, vcpupid) < 0) {
-virReportSystemError(errno, "%s",
- _("cannot get vCPU placement & pCPU
time"));
return -1;
}
}
@@ -2521,8 +2519,6 @@ qemuDomainGetInfo(virDomainPtr dom,
if (virDomainObjIsActive(vm)) {
if (virProcessGetStatInfo(&(info->cpuTime), NULL, NULL,
vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot read cputime for domain"));
goto cleanup;
}
}
@@ -10530,8 +10526,7 @@ qemuDomainMemoryStatsInternal(virQEMUDriver *driver,
}
if (virProcessGetStatInfo(NULL, NULL, , vm->pid, 0) < 0) {
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("cannot get RSS for domain"));
+return -1;
} else {
stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
stats[ret].val = rss;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
--
2.25.1
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Ján Tomko wrote:
> On a Friday in 2022, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Ján Tomko wrote:
> >
> > > On a Friday in 2022, Ani Sinha wrote:
> > > > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > > > I don't think so. Just like we've discussed under one patch of yours,
> > > a
> > > > > function should either report error in all cases or none. And in case
> > > of
> > > > > virProcessGetSchedInfo() the linux version does report error
> > > >
> > > > I see your point but there is also a bug in that function - not all
> > > error
> > > > paths report errors. For example, !proc and !lines cases. We need to fix
> > > > that.
> > > >
> > >
> > > I don't see a !proc error path in virProcessGetSchedInfo.
> > >
> >
> > if (tid)
> >proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
> > tid);
> >else
> >proc = g_strdup_printf("/proc/%d/sched", (int) pid);
> >if (!proc)
> >return -1; <=== not reported
> >
>
> Oh, I did not realize that I had Michal's patch that removes it applied
> locally:
> https://listman.redhat.com/archives/libvir-list/2022-January/msg00270.html
Ah ok, all makes sense now.
Re: [PATCH] report error when virProcessGetStatInfo() is unable to parse data
On Fri, 7 Jan 2022, Michal Prívozník wrote:
> On 1/7/22 10:09, Ani Sinha wrote:
> > Currently virProcessGetStatInfo() always returns success and only logs error
> > when it is unable to parse the data. Make this function actually report the
> > error and return a negative value in this error scenario.
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/util/virprocess.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index c74bd16fe6..b9f498d5d8 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
> > virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
> > ) < 0 ||
> > virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) <
> > 0 ||
> > virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10,
> > ) < 0) {
> > -VIR_WARN("cannot parse process status data");
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("cannot parse process status data for pid
> > '%d/%d'"),
> > + (int) pid, (int) tid);
> > +
> > +return -1;
> > }
> >
> > /* We got jiffies
>
> Couple of problems with this patch as is. I'm not against the idea of
> reporting an error here.
Good. now we are moving in the right direction.
But couple of things needs to be addressed first:
>
> 1) Currently, all callers check for retval and report an error if -1 was
> returned. This means, that even though this new message is reported it
> is immediately overwritten in caller.
Let me fix the callers and send an updated patch. Meanwhile ...
>
> 2) The non-linux implementation now has to report error too. I believe
> it's obvious why from our previous discussion this morning.
Maybe you can fix your patch.
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Ján Tomko wrote:
> On a Friday in 2022, Ani Sinha wrote:
> > On Fri, 7 Jan 2022, Michal Prívozník wrote:
> > > I don't think so. Just like we've discussed under one patch of yours, a
> > > function should either report error in all cases or none. And in case of
> > > virProcessGetSchedInfo() the linux version does report error
> >
> > I see your point but there is also a bug in that function - not all error
> > paths report errors. For example, !proc and !lines cases. We need to fix
> > that.
> >
>
> I don't see a !proc error path in virProcessGetSchedInfo.
>
if (tid)
proc = g_strdup_printf("/proc/%d/task/%d/sched", (int) pid, (int)
tid);
else
proc = g_strdup_printf("/proc/%d/sched", (int) pid);
if (!proc)
return -1; <=== not reported
> The !lines case is inconsistent but thankfully it can only happen
> if /proc/%d/sched is empty.
>
> > hence the
> > > non-linux variant should report an error too. And in case of
> > > virProcessGetStatInfo() no error is reported for linux version thus
> > > non-linux version shouldn't report an error.
> > >
> >
> > No this is not my understanding from the discussion. What I understood is
> > that the lowest level of functions should always report error when an
> > error path is encountered.
>
> Errors should be reported by the function that has the context to
> provide a helpful error message.
Correct. Most often it is the lowest level functions.
For some low-level helpers, we rely
> on errno's and let the caller report a less-specific error - either
> out of laziness because (like above) there would have to be a buggy
> kernel, or because most of the code paths are syscalls which set errno.
>
> Jano
>
> > For example virFileReadAll() does this nicely.
> > Currently there is no error path in virProcessGetStatInfo() and it
> > uncondiotionally returns 0. For non-linux variant, I think it would be
> > correct to report an error.
> >
> > Now having done that, we should also fix the callers so that the callers
> > are not overriding the narrower errors with broader ones.
>
[PATCH] report error when virProcessGetStatInfo() is unable to parse data
Currently virProcessGetStatInfo() always returns success and only logs error
when it is unable to parse the data. Make this function actually report the
error and return a negative value in this error scenario.
Signed-off-by: Ani Sinha
---
src/util/virprocess.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index c74bd16fe6..b9f498d5d8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1783,7 +1783,11 @@ virProcessGetStatInfo(unsigned long long *cpuTime,
virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10,
) < 0 ||
virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, ) < 0 ||
virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, )
< 0) {
-VIR_WARN("cannot parse process status data");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot parse process status data for pid '%d/%d'"),
+ (int) pid, (int) tid);
+
+return -1;
}
/* We got jiffies
--
2.25.1
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Michal Prívozník wrote:
> On 1/7/22 09:05, Ani Sinha wrote:
> >
> >
> > On Fri, 7 Jan 2022, Michal Privoznik wrote:
> >
> >> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> >> Linux centric. Provide stubs for non-Linux platforms.
> >>
> >> Signed-off-by: Michal Privoznik
> >> ---
> >> src/util/virprocess.c | 24
> >> 1 file changed, 24 insertions(+)
> >>
> >> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> >> index c74bd16fe6..5788faea9c 100644
> >> --- a/src/util/virprocess.c
> >> +++ b/src/util/virprocess.c
> >> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
> >> }
> >>
> >>
> >> +#ifdef __linux__
> >> int
> >> virProcessGetStatInfo(unsigned long long *cpuTime,
> >>int *lastCpu,
> >> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
> >>
> >> return 0;
> >> }
> >> +
> >> +#else
> >> +int
> >> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> >> + int *lastCpu G_GNUC_UNUSED,
> >> + long *vm_rss G_GNUC_UNUSED,
> >> + pid_t pid G_GNUC_UNUSED,
> >> + pid_t tid G_GNUC_UNUSED)
> >> +{
> >> +errno = ENOSYS;
> >> +return -1;
> >> +}
> >> +
> >> +int
> >> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> >> + pid_t pid G_GNUC_UNUSED,
> >> + pid_t tid G_GNUC_UNUSED)
> >> +{
> >> +virReportSystemError(ENOSYS, "%s",
> >> + _("scheduler information is not supported on
> >> this platform"));
> >
> > We should do this for both functions for non-linux platforms or not do it
> > at all. Like it should be consistent IMHO.
>
> I don't think so. Just like we've discussed under one patch of yours, a
> function should either report error in all cases or none. And in case of
> virProcessGetSchedInfo() the linux version does report error
I see your point but there is also a bug in that function - not all error
paths report errors. For example, !proc and !lines cases. We need to fix
that.
hence the
> non-linux variant should report an error too. And in case of
> virProcessGetStatInfo() no error is reported for linux version thus
> non-linux version shouldn't report an error.
>
No this is not my understanding from the discussion. What I understood is
that the lowest level of functions should always report error when an
error path is encountered. For example virFileReadAll() does this nicely.
Currently there is no error path in virProcessGetStatInfo() and it
uncondiotionally returns 0. For non-linux variant, I think it would be
correct to report an error.
Now having done that, we should also fix the callers so that the callers
are not overriding the narrower errors with broader ones.
> Think of it like this. You have a function that's called from somewhere.
> However, the function has two (or more) implementations (e.g. one for
> Linux, the other for FreeBSD, another one for MinGW, and so on). And you
> call the function like this:
>
> var = func();
> if (!var) {
> ...
>
> now should there be an error reported inside the if()? If all
> implementations of the func() are consistent then there's a clear answer.
>
> Hope this sheds more light.
>
> Michal
>
>
Re: [PATCH] virprocess: Provide non-Linux stubs for virProcessGet{Stat, Sched}Info
On Fri, 7 Jan 2022, Michal Privoznik wrote:
> Both virProcessGetStatInfo() and virProcessGetSchedInfo() are
> Linux centric. Provide stubs for non-Linux platforms.
>
> Signed-off-by: Michal Privoznik
> ---
> src/util/virprocess.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index c74bd16fe6..5788faea9c 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1766,6 +1766,7 @@ virProcessGetStat(pid_t pid,
> }
>
>
> +#ifdef __linux__
> int
> virProcessGetStatInfo(unsigned long long *cpuTime,
>int *lastCpu,
> @@ -1873,3 +1874,26 @@ virProcessGetSchedInfo(unsigned long long *cpuWait,
>
> return 0;
> }
> +
> +#else
> +int
> +virProcessGetStatInfo(unsigned long long *cpuTime G_GNUC_UNUSED,
> + int *lastCpu G_GNUC_UNUSED,
> + long *vm_rss G_GNUC_UNUSED,
> + pid_t pid G_GNUC_UNUSED,
> + pid_t tid G_GNUC_UNUSED)
> +{
> +errno = ENOSYS;
> +return -1;
> +}
> +
> +int
> +virProcessGetSchedInfo(unsigned long long *cpuWait G_GNUC_UNUSED,
> + pid_t pid G_GNUC_UNUSED,
> + pid_t tid G_GNUC_UNUSED)
> +{
> +virReportSystemError(ENOSYS, "%s",
> + _("scheduler information is not supported on this
> platform"));
We should do this for both functions for non-linux platforms or not do it
at all. Like it should be consistent IMHO.
> +return -1;
> +}
> +#endif /* __linux__ */
> --
> 2.34.1
>
>
Re: [PATCH] Make virConnectOpenInternal() report error in all cases
On Thu, 6 Jan 2022, Daniel P. Berrangé wrote: > On Thu, Jan 06, 2022 at 08:34:26PM +0530, Ani Sinha wrote: > > virConnectOpenInternal() does not report error in all failure scenarios, > > except > > in some specific cases. > > I don't believe this is correct. My reading of the code is that it > is already reporting errors in all exit paths that can return NULL. Yes you are likely right. I should have gone one level deeper in the call chain. I have not checked all the code paths exhaistively but for the one I checked, the lowest level functions are indeed reporting the more finer grained errors to their callers. I have sent an updated patch that just has the last two hunks. I have kept virConnectOpenInternal() unchanged.
[PATCH] do not report generic OPERATION_FAILED error when calling virConnectOpenAuth()
virConnectOpenAuth() calls virConnectOpenInternal(). This later function
generates fine grained errors arising from various failure conditions that are
more accurate than a "catch all" broader VIR_ERR_OPERATION_FAILED error that
the callers of this function generates. Remove the broader error so that more
specific errors can be caught and processed.
Signed-off-by: Ani Sinha
---
src/libxl/libxl_migration.c | 3 ---
src/qemu/qemu_migration.c | 3 ---
2 files changed, 6 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 6d0ab4ee28..bc2b5401da 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1134,9 +1134,6 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivate
*driver,
virObjectLock(vm);
if (dconn == NULL) {
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("Failed to connect to remote libvirt URI %s: %s"),
- dconnuri, virGetLastErrorMessage());
return ret;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index b9d7d582f5..2635ef1162 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5145,9 +5145,6 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
goto cleanup;
if (dconn == NULL) {
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("Failed to connect to remote libvirt URI %s: %s"),
- dconnuri, virGetLastErrorMessage());
return -1;
}
--
2.25.1
[PATCH] change return type of virURIParamAppend() to void type
virURIParamAppend() unconditionally returns 0. Simplify and make the return type
as void type.
Signed-off-by: Ani Sinha
---
src/util/viruri.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/util/viruri.c b/src/util/viruri.c
index 3c73188a55..88bb0cc1f8 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -34,7 +34,7 @@
VIR_LOG_INIT("util.uri");
-static int
+static void
virURIParamAppend(virURI *uri,
const char *name,
const char *value)
@@ -52,7 +52,7 @@ virURIParamAppend(virURI *uri,
uri->params[uri->paramsCount].ignore = false;
uri->paramsCount++;
-return 0;
+return;
}
@@ -113,8 +113,7 @@ virURIParseParams(virURI *uri)
}
/* Append to the parameter set. */
-if (virURIParamAppend(uri, name, NULLSTR_EMPTY(value)) < 0)
-return -1;
+virURIParamAppend(uri, name, NULLSTR_EMPTY(value));
next:
query = end;
--
2.25.1
[PATCH] Make virConnectOpenInternal() report error in all cases
virConnectOpenInternal() does not report error in all failure scenarios, except
in some specific cases. This inconsistent behavior forces the caller of this
function report a generic error for all failure modes which then hides specific
error scenarios. This change makes virConnectOpenInternal() report failure in
all cases so that it can generate specific errors based on the type of failure
encountered. The reporiting of the errors can be made more fine grained in
subsequent changes.
Signed-off-by: Ani Sinha
---
src/libvirt.c | 24
src/libxl/libxl_migration.c | 3 ---
src/qemu/qemu_migration.c | 3 ---
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 45315f484c..53ceee1359 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -893,8 +893,12 @@ virConnectOpenInternal(const char *name,
bool embed = false;
ret = virGetConnect();
-if (ret == NULL)
+if (ret == NULL) {
+virReportError(VIR_ERR_INVALID_CONN,
+ _("Failed to create connection object for URI %s"),
+ NULLSTR(name));
return NULL;
+}
if (virConfLoadConfig(, "libvirt.conf") < 0)
goto failed;
@@ -974,7 +978,7 @@ virConnectOpenInternal(const char *name,
virReportError(VIR_ERR_NO_CONNECT,
_("URI '%s' does not include a driver name"),
name);
-goto failed;
+goto failed_no_report;
}
if (virConnectCheckURIMissingSlash(uristr,
@@ -992,7 +996,7 @@ virConnectOpenInternal(const char *name,
virReportError(VIR_ERR_NO_CONNECT,
_("URI scheme '%s' for embedded driver is not
valid"),
ret->uri->scheme);
-goto failed;
+goto failed_no_report;
}
root = virURIGetParam(ret->uri, "root");
@@ -1002,7 +1006,7 @@ virConnectOpenInternal(const char *name,
if (!g_path_is_absolute(root)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("root path must be absolute"));
-goto failed;
+goto failed_no_report;
}
if (virEventRequireImpl() < 0)
@@ -1062,7 +1066,7 @@ virConnectOpenInternal(const char *name,
__FILE__, __FUNCTION__, __LINE__,
_("libvirt was built without the '%s'
driver"),
ret->uri->scheme);
-goto failed;
+goto failed_no_report;
}
VIR_DEBUG("trying driver %zu (%s) ...",
@@ -1112,13 +1116,13 @@ virConnectOpenInternal(const char *name,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Driver %s cannot be used in embedded mode"),
virConnectDriverTab[i]->hypervisorDriver->name);
-goto failed;
+goto failed_no_report;
}
/* before starting the new connection, check if the driver only works
* with a server, and so return an error if the server is missing */
if (virConnectDriverTab[i]->remoteOnly && ret->uri &&
!ret->uri->server) {
virReportError(VIR_ERR_INVALID_ARG, "%s", _("URI is missing the
server part"));
-goto failed;
+goto failed_no_report;
}
ret->driver = virConnectDriverTab[i]->hypervisorDriver;
@@ -1155,7 +1159,7 @@ virConnectOpenInternal(const char *name,
if (!ret->driver) {
/* If we reach here, then all drivers declined the connection. */
virReportError(VIR_ERR_NO_CONNECT, "%s", NULLSTR(name));
-goto failed;
+goto failed_no_report;
}
VIR_FREE(uristr);
@@ -1163,6 +1167,10 @@ virConnectOpenInternal(const char *name,
return ret;
failed:
+virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Failed to connect to remote libvirt URI %s"),
+ NULLSTR(uristr));
+ failed_no_report:
VIR_FREE(uristr);
virObjectUnref(ret);
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 6d0ab4ee28..bc2b5401da 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -1134,9 +1134,6 @@ libxlDomainMigrationSrcPerformP2P(libxlDriverPrivate
*driver,
virObjectLock(vm);
if (dconn == NULL) {
-virReportError(VIR_ERR_OPERATION_FAILED,
- _("Failed to connect to remote libvirt URI %s: %s"),
- dconnuri, virGetLastErrorMessage());
return ret;
}
diff --git a/src/qemu/qemu_mi
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
On Thu, 6 Jan 2022, Michal Prívozník wrote:
> On 1/6/22 13:42, Ani Sinha wrote:
> >
> >
> > On Thu, 6 Jan 2022, Michal Prívozník wrote:
> >
> >> On 1/6/22 02:33, Raphael Norwitz wrote:
> >>> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> >>> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> >>> to VIR_ERR_NO_CONNECT, which is more accurate.
> >>>
> >>> This should help libvirt consumers more intellegently retry migrations
> >>> on intermittent connection failures.
> >>>
> >>> CC: Bhuvnesh Jain
> >>> Suggested-by: John Levon
> >>> Signed-off-by: Raphael Norwitz
> >>> ---
> >>> src/qemu/qemu_migration.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> >>> index b9d7d582f5..f7ced209a4 100644
> >>> --- a/src/qemu/qemu_migration.c
> >>> +++ b/src/qemu/qemu_migration.c
> >>> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver
> >>> *driver,
> >>> goto cleanup;
> >>>
> >>> if (dconn == NULL) {
> >>> -virReportError(VIR_ERR_OPERATION_FAILED,
> >>> +virReportError(VIR_ERR_NO_CONNECT,
> >>> _("Failed to connect to remote libvirt URI %s:
> >>> %s"),
> >>> dconnuri, virGetLastErrorMessage());
> >>> return -1;
> >>
> >> I'm not exactly sure why we need this virReportError() in the first
> >> place. Basically, we are just overwriting a more accurate error with
> >> this generic one. Doesn't removing this virReportError() fix the problem?
> >
> > Not all failure scenarios in virConnectOpenInternal() are
> > caught with a virReportError(). Hence, we do need this.
>
> Well, then the problem is in virConnectOpenInternal(). Either it should
> report error in all cases or none, because then the caller would have to
> check if error was reported or not.
>
OK fine. I will post a patch soon.
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
On Thu, 6 Jan 2022, Michal Prívozník wrote:
> On 1/6/22 02:33, Raphael Norwitz wrote:
> > Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> > returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> > to VIR_ERR_NO_CONNECT, which is more accurate.
> >
> > This should help libvirt consumers more intellegently retry migrations
> > on intermittent connection failures.
> >
> > CC: Bhuvnesh Jain
> > Suggested-by: John Levon
> > Signed-off-by: Raphael Norwitz
> > ---
> > src/qemu/qemu_migration.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index b9d7d582f5..f7ced209a4 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver
> > *driver,
> > goto cleanup;
> >
> > if (dconn == NULL) {
> > -virReportError(VIR_ERR_OPERATION_FAILED,
> > +virReportError(VIR_ERR_NO_CONNECT,
> > _("Failed to connect to remote libvirt URI %s: %s"),
> > dconnuri, virGetLastErrorMessage());
> > return -1;
>
> I'm not exactly sure why we need this virReportError() in the first
> place. Basically, we are just overwriting a more accurate error with
> this generic one. Doesn't removing this virReportError() fix the problem?
Not all failure scenarios in virConnectOpenInternal() are
caught with a virReportError(). Hence, we do need this.
Re: [RFC] qemu_migration: Fix virConnectOpenAuth error code
On Thu, 6 Jan 2022, Raphael Norwitz wrote:
> Today if virConnectOpenAuth fails, qemuMigrationSrcPerformPeer2Peer()
> returns VIR_ERR_OPERATION_FAILED. This change switches that error code
> to VIR_ERR_NO_CONNECT, which is more accurate.
Hmm, I am not sure if this would be the right thing.
virConnectOpenInternal() fails under many conditions, two of which
actiually qualifies for VIR_ERR_NO_CONNECT. This error symbolizes "no
connection driver available" which to me has a much narrower scope.
virConnectOpenInternal() has other failure codepaths, for example
VIR_ERR_CONFIG_UNSUPPORTED. Catching all those failures as
VIR_ERR_OPERATION_FAILED that has a wider scope seems to be correct thing
to do in the existing code.
YMMV.
>
> This should help libvirt consumers more intellegently retry migrations
> on intermittent connection failures.
>
> CC: Bhuvnesh Jain
> Suggested-by: John Levon
> Signed-off-by: Raphael Norwitz
> ---
> src/qemu/qemu_migration.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index b9d7d582f5..f7ced209a4 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -5145,7 +5145,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriver *driver,
> goto cleanup;
>
> if (dconn == NULL) {
> -virReportError(VIR_ERR_OPERATION_FAILED,
> +virReportError(VIR_ERR_NO_CONNECT,
> _("Failed to connect to remote libvirt URI %s: %s"),
> dconnuri, virGetLastErrorMessage());
> return -1;
> --
> 2.20.1
>
>
>
Re: [PATCH] qemu: agent: remove all code around disabled DEBUG_IO/DEBUG_RAW_IO definitions
On Thu, 6 Jan 2022, Peter Krempa wrote:
> On Tue, Jan 04, 2022 at 17:47:43 +0530, Ani Sinha wrote:
> > DEBUG_IO and DEBUG_RAW_IO are disabled and hence the code #defined under
> > them
> > are useless. Remove them. Also remove similar useless code from
> > qemu_monitor_json.
> >
> > See also 4aae00bf1287f ("qemu: monitor: Remove disabled debug
> > infrastructure")
>
> ~/libvirt $ git show 4aae00bf1287f
> fatal: ambiguous argument '4aae00bf1287f': unknown revision or path not in
> the working tree.
>
> Which is expected as I didn't yet push the commit you are refering to.
Right but I had applied your commit to my tree and I think the hash would
be the same. I just assumed you would push your commit first and then push
mine and adjust the hash appropriately (if needed at all).
> I'll drop it along with the last sentence of the previous paragraph
> before pushing.
>
> >
> > Signed-off-by: Ani Sinha
> > ---
> > src/qemu/qemu_agent.c| 51
> > src/qemu/qemu_monitor_json.c | 4 ---
> > 2 files changed, 55 deletions(-)
>
> Reviewed-by: Peter Krempa
>
>
Re: [PATCH v3] Add VM info to improve error log message for qemu monitor
On Tue, 4 Jan 2022, Rohit Kumar wrote:
> This change adds the domain name in the error and debug logs during
> monitor IO processing so that we may infer which VM experienced
> errors such as IO or socket hangup. This may help in debugging
> monitor IO errors.
LGTM. If you wish you can add a comment why the new member domainName in
qemuMonitor stuct was essential.
>
> Signed-off-by: Rohit Kumar
Reviewed-by: Ani Sinha
> ---
> src/qemu/qemu_monitor.c | 36 +---
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index dda6ae9796..2b4582578a 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -80,6 +80,7 @@ struct _qemuMonitor {
> GSource *watch;
>
> virDomainObj *vm;
> +char *domainName;
>
> qemuMonitorCallbacks *cb;
> void *callbackOpaque;
> @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
> virCondDestroy(>notify);
> g_free(mon->buffer);
> g_free(mon->balloonpath);
> +g_free(mon->domainName);
> }
>
>
> @@ -583,8 +585,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
>
> if (!error && !mon->goteof &&
> cond & G_IO_ERR) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Invalid file descriptor while waiting for
> monitor"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Invalid file descriptor while waiting for
> monitor (vm='%s')"), mon->domainName);
> mon->goteof = true;
> }
> }
> @@ -609,13 +611,14 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> virResetLastError();
> } else {
> if (virGetLastErrorCode() == VIR_ERR_OK && !mon->goteof)
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Error while processing monitor IO"));
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Error while processing monitor IO
> (vm='%s')"), mon->domainName);
> virCopyLastError(>lastError);
> virResetLastError();
> }
>
> -VIR_DEBUG("Error on monitor %s", NULLSTR(mon->lastError.message));
> +VIR_DEBUG("Error on monitor %s mon=%p vm=%p name=%s",
> + NULLSTR(mon->lastError.message), mon, mon->vm,
> mon->domainName);
> /* If IO process resulted in an error & we have a message,
> * then wakeup that waiter */
> if (mon->msg && !mon->msg->finished) {
> @@ -636,7 +639,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> /* Make sure anyone waiting wakes up now */
> virCondSignal(>notify);
> virObjectUnlock(mon);
> -VIR_DEBUG("Triggering EOF callback");
> +VIR_DEBUG("Triggering EOF callback mon=%p vm=%p name=%s",
> + mon, mon->vm, mon->domainName);
> (eofNotify)(mon, vm, mon->callbackOpaque);
> virObjectUnref(mon);
> } else if (error) {
> @@ -646,7 +650,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> /* Make sure anyone waiting wakes up now */
> virCondSignal(>notify);
> virObjectUnlock(mon);
> -VIR_DEBUG("Triggering error callback");
> +VIR_DEBUG("Triggering error callback mon=%p vm=%p name=%s",
> + mon, mon->vm, mon->domainName);
> (errorNotify)(mon, vm, mon->callbackOpaque);
> virObjectUnref(mon);
> } else {
> @@ -694,6 +699,7 @@ qemuMonitorOpenInternal(virDomainObj *vm,
> mon->fd = fd;
> mon->context = g_main_context_ref(context);
> mon->vm = virObjectRef(vm);
> +mon->domainName = g_strdup(NULLSTR(vm->def->name));
> mon->waitGreeting = true;
> mon->cb = cb;
> mon->callbackOpaque = opaque;
> @@ -935,14 +941,14 @@ qemuMonitorSend(qemuMonitor *mon,
>
> /* Check whether qemu quit unexpectedly */
> if (mon->lastError.code != VIR_ERR_OK) {
> -VIR_DEBUG("Attempt to send command while error is set %s",
> - NULLSTR(mon->lastError.message));
> +VIR_DEBUG("Attempt to send command while error is set %s mon=%p
> vm=%p name=%s",
> + NULLSTR(mon->lastError.message), mon, mon->vm,
> mon->domainName);
> virSetError(>lastError);
> retu
Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port
On Tue, 4 Jan 2022, Divya Garg wrote: > On 04/01/22 5:47 pm, Ani Sinha wrote: > > > > On Mon, 3 Jan 2022, Divya Garg wrote: > > > > > Thankyou Ani for the review. I will be taking up the comments > > > in next patchset along with other comments. > > > > > > On 03/01/22 1:44 pm, Ani Sinha wrote: > > > > > > > On Mon, 3 Jan 2022, Divya Garg wrote: > > > > > > > > > Hi all ! > > > > > > > > > > Looking forward for the review comments on the patch. > > > > > > > > > For single patches, no need to use numbering. > > > I have also added the cover letter with the patch. Hence added the > > > numbering > > > here. > > btw, cover letters are not essential for single patches: > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__libvirt.org_submitting-2Dpatches.html=DwIBAg=s883GpUCOChKOHiocYtGcg=2QGHz-fTCVWImEBKe1ZcSe5t6UfasnhvdzD5DcixwOE=neaBi0ZuC9uGNNnNZGkXsPK_JEReYD2oA8csE4KE4LUYwtOWFxn6ipCd7_mPXNll=u-g3In7hJ90OdEzNmUj41bzzvmAIvOmLJSBpcAB2kQc= > > Thankyou so much Ani. I added the cover letter because i was not sure if the > checks I am applying are needed or not. Hence wanted to explain all my cases > that I am handling through the cover letter. yes that's perfectly fine.
[PATCH] qemu: agent: remove all code around disabled DEBUG_IO/DEBUG_RAW_IO definitions
DEBUG_IO and DEBUG_RAW_IO are disabled and hence the code #defined under them
are useless. Remove them. Also remove similar useless code from
qemu_monitor_json.
See also 4aae00bf1287f ("qemu: monitor: Remove disabled debug infrastructure")
Signed-off-by: Ani Sinha
---
src/qemu/qemu_agent.c| 51
src/qemu/qemu_monitor_json.c | 4 ---
2 files changed, 55 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index cb3bf97415..f33cd47078 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -48,9 +48,6 @@ VIR_LOG_INIT("qemu.qemu_agent");
#define LINE_ENDING "\n"
-#define DEBUG_IO 0
-#define DEBUG_RAW_IO 0
-
/* We read from QEMU until seeing a \r\n pair to indicate a
* completed reply or event. To avoid memory denial-of-service
* though, we must have a size limit on amount of data we
@@ -137,25 +134,6 @@ static int qemuAgentOnceInit(void)
VIR_ONCE_GLOBAL_INIT(qemuAgent);
-#if DEBUG_RAW_IO
-static char *
-qemuAgentEscapeNonPrintable(const char *text)
-{
-size_t i;
-g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
-for (i = 0; text[i] != '\0'; i++) {
-if (text[i] == '\\')
-virBufferAddLit(, "");
-else if (g_ascii_isprint(text[i]) || text[i] == '\n' ||
- (text[i] == '\r' && text[i+1] == '\n'))
-virBufferAddChar(, text[i]);
-else
-virBufferAsprintf(, "\\x%02x", text[i]);
-}
-return virBufferContentAndReset();
-}
-#endif
-
static void qemuAgentDispose(void *obj)
{
@@ -300,14 +278,6 @@ static int qemuAgentIOProcessData(qemuAgent *agent,
{
int used = 0;
size_t i = 0;
-#if DEBUG_IO
-# if DEBUG_RAW_IO
-g_autofree char *str1 = qemuAgentEscapeNonPrintable(data);
-VIR_ERROR(_("[%s]"), str1);
-# else
-VIR_DEBUG("Data %zu bytes [%s]", len, data);
-# endif
-#endif
while (used < len) {
char *nl = strstr(data + used, LINE_ENDING);
@@ -344,17 +314,6 @@ qemuAgentIOProcess(qemuAgent *agent)
if (agent->msg && agent->msg->txOffset == agent->msg->txLength)
msg = agent->msg;
-#if DEBUG_IO
-# if DEBUG_RAW_IO
-g_autofree char *str1 = qemuAgentEscapeNonPrintable(msg ? msg->txBuffer :
"");
-g_autofree char *str2 = qemuAgentEscapeNonPrintable(agent->buffer);
-VIR_ERROR(_("Process %zu %p %p [[[%s]]][[[%s]]]"),
- agent->bufferOffset, agent->msg, msg, str1, str2);
-# else
-VIR_DEBUG("Process %zu", agent->bufferOffset);
-# endif
-#endif
-
len = qemuAgentIOProcessData(agent,
agent->buffer, agent->bufferOffset,
msg);
@@ -369,9 +328,6 @@ qemuAgentIOProcess(qemuAgent *agent)
VIR_FREE(agent->buffer);
agent->bufferOffset = agent->bufferLength = 0;
}
-#if DEBUG_IO
-VIR_DEBUG("Process done %zu used %d", agent->bufferOffset, len);
-#endif
if (msg && msg->finished)
virCondBroadcast(>notify);
return len;
@@ -455,10 +411,6 @@ qemuAgentIORead(qemuAgent *agent)
agent->buffer[agent->bufferOffset] = '\0';
}
-#if DEBUG_IO
-VIR_DEBUG("Now read %zu bytes of data", agent->bufferOffset);
-#endif
-
return ret;
}
@@ -527,9 +479,6 @@ qemuAgentIO(GSocket *socket G_GNUC_UNUSED,
virObjectRef(agent);
/* lock access to the agent and protect fd */
virObjectLock(agent);
-#if DEBUG_IO
-VIR_DEBUG("Agent %p I/O on watch %d socket %p cond %d", agent,
agent->socket, cond);
-#endif
if (agent->fd == -1 || !agent->watch) {
virObjectUnlock(agent);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e45b43eb5a..0f1999f413 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -280,10 +280,6 @@ int qemuMonitorJSONIOProcess(qemuMonitor *mon,
}
}
-#if DEBUG_IO
-VIR_DEBUG("Total used %d bytes out of %zd available in buffer", used, len);
-#endif
-
return used;
}
--
2.25.1
Re: [RFC v2 1/1] qemu: add index for isa-serial device using target.port
On Mon, 3 Jan 2022, Divya Garg wrote: > Thankyou Ani for the review. I will be taking up the comments > in next patchset along with other comments. > > On 03/01/22 1:44 pm, Ani Sinha wrote: > > > > > On Mon, 3 Jan 2022, Divya Garg wrote: > > > > > Hi all ! > > > > > > Looking forward for the review comments on the patch. > > > > > For single patches, no need to use numbering. > I have also added the cover letter with the patch. Hence added the numbering > here. btw, cover letters are not essential for single patches: https://libvirt.org/submitting-patches.html
Re: [PATCH v2] Add VM info to improve error log message for qemu monitor
On Tue, 4 Jan 2022, Peter Krempa wrote: > On Tue, Jan 04, 2022 at 15:30:00 +0530, Ani Sinha wrote: > > On Tue, 4 Jan 2022, Rohit Kumar wrote: > > > On 03/01/22 7:12 pm, Ani Sinha wrote: > > > > On Wed, 22 Dec 2021, Rohit Kumar wrote: > > [...] > > > > > > @@ -694,6 +702,7 @@ qemuMonitorOpenInternal(virDomainObj *vm, > > > > > mon->fd = fd; > > > > > mon->context = g_main_context_ref(context); > > > > > mon->vm = virObjectRef(vm); > > > > > +mon->domainName = g_strdup(vm->def->name); > > > > do not forget to g_free() this during cleanup in the same function. > > > So, in cleanup: qemuMonitorDispose is called. And there I have added > > > g_free() > > > to clean mon->domainName. > > > > No, in cleanup, I see qemuMonitorClose() is called where do you do not add > > any additional code to free the allocation. > > > > This is what I see in cleanup code: > > > > ``` > > cleanup: > > /* We don't want the 'destroy' callback invoked during > > * cleanup from construction failure, because that can > > * give a double-unref on virDomainObj *in the caller, > > * so kill the callbacks now. > > */ > > mon->cb = NULL; > > /* The caller owns 'fd' on failure */ > > mon->fd = -1; > > qemuMonitorClose(mon); > > qemuMonitorClose() eventually calls virObjectUnref(mon). Once the last > reference on the monitor object is removed the object is freed via > qemuMonitorDispose(). > > This patch has: > > @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj) > virCondDestroy(>notify); > g_free(mon->buffer); > g_free(mon->balloonpath); > +g_free(mon->domainName); > } Oh ok. I assumed that there was two cleanup paths : one is the natural tear down where qemuMonitorDispose() would be called. Other was failure during contruction itself which I thought needed additional handling. Its ok then, no need to add additional g_free in monitorOpen()
Re: [PATCH v2] Add VM info to improve error log message for qemu monitor
On Tue, 4 Jan 2022, Rohit Kumar wrote:
>
> On 03/01/22 7:12 pm, Ani Sinha wrote:
> >
> > On Wed, 22 Dec 2021, Rohit Kumar wrote:
> >
> > > This patch is to determine the VM which had IO or socket hangup error.
> > > Accessing directly vm->def->name inside qemuMonitorIO() or
> > > qemuMonitorSend()
> > > might leads to illegal access as we are out of 'vm' context and vm->def
> > > might
> > > not exist. Adding a field "domainName" inside mon object to access vm name
> > > and initialising it when creating mon object.
> > The commit message should desribe the purpose of the change, not how it is
> > done. That can be easily understood reading the code. As for why the
> > implementation follows a particular way, we can explain it in the code
> > comments.
> >
> > This change adds the domain name in the error and debug logs during
> > monitor IO processing so that we may infer which VM experienced those
> > errors etc. This may help in debugging monitor IO errors. IMHO this
> > decription is enough.
> Thanks Ani, I will update the commit message in the next patch.
> >
> > > Signed-off-by: Rohit Kumar
> > > ---
> > > diff to v1:
> > >- Adding a field domainName inside _qemuMonitor struct for accessing vm
> > > name
> > > instead of directly accessing mon->vm->def->name.
> > >- Link to v1:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2021-2DDecember_msg00217.html=DwIBAg=s883GpUCOChKOHiocYtGcg=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs=W2jEgk2s_xjLOLC-j8lPBnwVgteHS0-yYEGMqin16nmfzndzNgBE9mEvtbvyVzQo=Sa2q9edb4cvXo-EUriIPwlJxNo-itpp6--ZxY7rla2U=
> > >- Talked with peter on RFC and he suggested me to add a field inside
> > > monitor struct to get VM name.
> > >
> > > src/qemu/qemu_monitor.c | 47 +
> > > 1 file changed, 29 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > > index dda6ae9796..c3a0227795 100644
> > > --- a/src/qemu/qemu_monitor.c
> > > +++ b/src/qemu/qemu_monitor.c
> > > @@ -80,6 +80,7 @@ struct _qemuMonitor {
> > > GSource *watch;
> > >
> > > virDomainObj *vm;
> > > +char *domainName;
> > >
> > Here we can explain why we are adding an extra member as opposed to
> > referencing it directly.
> Right, we can access it directly.
> >
> > > qemuMonitorCallbacks *cb;
> > > void *callbackOpaque;
> > > @@ -243,6 +244,7 @@ qemuMonitorDispose(void *obj)
> > > virCondDestroy(>notify);
> > > g_free(mon->buffer);
> > > g_free(mon->balloonpath);
> > > +g_free(mon->domainName);
> > > }
> > >
> > >
> > > @@ -530,13 +532,18 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > > qemuMonitor *mon = opaque;
> > > bool error = false;
> > > bool hangup = false;
> > > +virDomainObj *vm = NULL;
> > > +char *vmName = NULL;
> > >
> > These initializations are redundant since ...
> Acknowledged.
> > > virObjectRef(mon);
> > >
> > > +vm = mon->vm;
> > > +vmName = mon->domainName;
> > you initialize them here anyway.
> I will remove these in next patch.
> >
> > > +
> > > /* lock access to the monitor and protect fd */
> > > virObjectLock(mon);
> > > #if DEBUG_IO
> > > -VIR_DEBUG("Monitor %p I/O on socket %p cond %d", mon, socket, cond);
> > > +VIR_DEBUG("Monitor %p I/O on socket %p cond %d vm=%p name=%s", mon,
> > > socket, cond, vm, NULLSTR(vmName));
> > > #endif
> > > if (mon->fd == -1 || !mon->watch) {
> > > virObjectUnlock(mon);
> > > @@ -583,8 +590,8 @@ qemuMonitorIO(GSocket *socket G_GNUC_UNUSED,
> > >
> > > if (!error && !mon->goteof &&
> > > cond & G_IO_ERR) {
> > > -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > > - _("Invalid file descriptor while waiting for
> > > monitor"));
> > > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > > + _("%s: Invalid file descriptor while waiting
> > > for monitor"), NULLSTR(vmName));
&
