RE: join running core dump job

2024-03-04 Thread Thanos Makatos
> -Original Message-
> From: Thanos Makatos 
> Sent: Monday, March 4, 2024 5:24 PM
> To: devel@lists.libvirt.org
> Subject: join running core dump job
> 
> Is there a way to programmatically wait for a previously initiated
> virDomainCoreDumpWithFormat() where the process that started it died? I'm
> looking at the API and don't seem to find anything relevant.  I suppose I 
> could
> poll via virDomainGetJobStats(), but, ideally, I'd like a function that would 
> join
> the dump job and return when the dump job finishes.
> ___
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-le...@lists.libvirt.org

I see there's qemuDumpWaitForCompletion(), looks promising.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH 1/5] qemu.conf changes to support multiple memory backend directories

2024-03-04 Thread Michael Galaxy

Hi Martin,

Answers inline. Thanks for helping with the review and all the tips!

On 3/1/24 04:00, Martin Kletzander wrote:

On Mon, Jan 29, 2024 at 04:43:53PM -0500, mgal...@akamai.com wrote:

From: Michael Galaxy 




In our case, we almost always have two NUMA nodes, so in that
example, we have two PMEM regions which are created on the
Linux kernel command line that get mounted into those two
locations for libvirt to use.



There are PMEM devices which you then expose as filesystems to use for
libvirt as a backing for VM's PMEMs.  Do I understand that correctly?

If yes, how are these different?  Can't they be passed through?

So, these are very different. QEMU currently already supports passing 
through
PMEM for guest internal use (The guest puts its own filesystem onto the 
passed-through

PMEM device).

In our case, we are using the PMEM area only in the host to place the 
QEMU memory backing

for all guests into a single PMEM area.

To support NUMA correctly, QEMU needs to support mutiple host-level PMEM 
areas which
have been pre-configured to be NUMA aware. This is strictly for the 
purposes of doing live updates,
not as a mechanism for guests to internally take advantage of persistent 
memory... that's
a completely different use case (which in and of itself is very 
interesting, but not what we are

using it for).

That's how it works. Does that make sense?


(I'll work on those other requests, thank you)

- Michael
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 2/2] storage: fix testcases on changing the default qcow2 compat to 1.1

2024-03-04 Thread Abhiram Tilak
After changing the default qcow2 image to 1.1 from 0.10. Some of the
testcases in `storagevolxml2` here need fixing. This patch changes the
expected compat version in each of these files that call qemu-img.

As per qemu's Qcow docs the qemu-img command gives a 1.1 compatible
version image. These testcases are written for 0.10, and should be
upgraded.

Also there is a testcase `qcow2-1.1.argv`. Which is aimed at testing
1.1 version specifically, I think a new testcase should be made to
replace it called `qcow2-0.10.argv`. This requires renaming test files
and changes at many places, not sure so didn't include in the patch.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602
Signed-off-by: Abhiram Tilak 
---
 .../storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-compat.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv | 2 +-
 tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv| 2 +-
 .../qcow2-luks-convert-encrypt2fileqcow2.argv   | 2 +-
 tests/storagevolxml2argvdata/qcow2-luks.argv| 2 +-
 .../qcow2-nobacking-convert-prealloc-compat.argv| 2 +-
 .../storagevolxml2argvdata/qcow2-nobacking-prealloc-compat.argv | 2 +-
 .../qcow2-nocapacity-convert-prealloc.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-nocapacity.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-nocow-compat.argv| 2 +-
 tests/storagevolxml2argvdata/qcow2-zerocapacity.argv| 2 +-
 12 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv 
b/tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
index 4b9ccfe8dc..705604b162 100644
--- a/tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
+++ b/tests/storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv
@@ -1,7 +1,7 @@
 qemu-img \
 create \
 -f qcow2 \
--o compat=0.10 \
+-o compat=1.1 \
 /var/lib/libvirt/images/sparse-qcow2.img \
 1073741824K
 qemu-img \
diff --git a/tests/storagevolxml2argvdata/qcow2-compat.argv 
b/tests/storagevolxml2argvdata/qcow2-compat.argv
index bf3a50a7f3..40fbe065e0 100644
--- a/tests/storagevolxml2argvdata/qcow2-compat.argv
+++ b/tests/storagevolxml2argvdata/qcow2-compat.argv
@@ -2,6 +2,6 @@ qemu-img \
 create \
 -f qcow2 \
 -b /dev/null \
--o backing_fmt=raw,compat=0.10 \
+-o backing_fmt=raw,compat=1.1 \
 /var/lib/libvirt/images/OtherDemo.img \
 5242880K
diff --git a/tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv 
b/tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv
index dbc7deb16a..b68da425d9 100644
--- a/tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv
+++ b/tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv
@@ -2,6 +2,6 @@ qemu-img \
 convert \
 -f raw \
 -O qcow2 \
--o compat=0.10 \
+-o compat=1.1 \
 /dev/HostVG/Swap \
 /var/lib/libvirt/images/OtherDemo.img
diff --git a/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv 
b/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv
index d89622d4a6..3068b4b38d 100644
--- a/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv
+++ b/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv
@@ -2,7 +2,7 @@ qemu-img \
 create \
 -f qcow2 \
 --object secret,id=OtherDemoLuks.img_encrypt0,file=/path/to/secretFile \
--o 
encrypt.format=luks,encrypt.key-secret=OtherDemoLuks.img_encrypt0,compat=0.10 \
+-o 
encrypt.format=luks,encrypt.key-secret=OtherDemoLuks.img_encrypt0,compat=1.1 \
 /var/lib/libvirt/images/OtherDemoLuks.img \
 5242880K
 qemu-img \
diff --git 
a/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt2fileqcow2.argv 
b/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt2fileqcow2.argv
index 4d910552d0..948e9ac66d 100644
--- a/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt2fileqcow2.argv
+++ b/tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt2fileqcow2.argv
@@ -1,7 +1,7 @@
 qemu-img \
 create \
 -f qcow2 \
--o compat=0.10 \
+-o compat=1.1 \
 /var/lib/libvirt/images/sparse-qcow2.img \
 1073741824K
 qemu-img \
diff --git a/tests/storagevolxml2argvdata/qcow2-luks.argv 
b/tests/storagevolxml2argvdata/qcow2-luks.argv
index 308316c90c..a3be41a406 100644
--- a/tests/storagevolxml2argvdata/qcow2-luks.argv
+++ b/tests/storagevolxml2argvdata/qcow2-luks.argv
@@ -3,6 +3,6 @@ create \
 -f qcow2 \
 -b /dev/null \
 --object secret,id=OtherDemoLuks.img_encrypt0,file=/path/to/secretFile \
--o 
backing_fmt=raw,encrypt.format=luks,encrypt.key-secret=OtherDemoLuks.img_encrypt0,compat=0.10
 \
+-o 
backing_fmt=raw,encrypt.format=luks,encrypt.key-secret=OtherDemoLuks.img_encrypt0,compat=1.1
 \
 /var/lib/libvirt/images/OtherDemoLuks.img \
 5242880K
diff --git 
a/tests/storagevolxml2argvdata/qcow2-nobacking-convert-prealloc-compat.argv 
b/tests/storagevolxml2argvdata/qcow2-nobacking-convert-prealloc-compat.argv
index 

[PATCH 1/2] storage: change assigning qcow2 compat to 1.1 from 0.10 automatically

2024-03-04 Thread Abhiram Tilak
In the file `storage/storage_util.c` currently `compat` varible is begin
assigned to 0.10 by default. This patch changes this default value to 1.1.

This is done in efforts to upgrade the default qcow2 image version to
1.1.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602
Signed-off-by: Abhiram Tilak 
---
 src/storage/storage_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 7bf815d978..28d5fce4f0 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -765,7 +765,7 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDef 
*encinfo,
 if (info->compat)
 virBufferAsprintf(, "compat=%s,", info->compat);
 else if (info->format == VIR_STORAGE_FILE_QCOW2)
-virBufferAddLit(, "compat=0.10,");
+virBufferAddLit(, "compat=1.1,");
 
 if (info->clusterSize > 0)
 virBufferAsprintf(, "cluster_size=%llu,", info->clusterSize);
-- 
2.42.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH 0/2] storage: Upgrade default qcow2 verion to 1.1

2024-03-04 Thread Abhiram Tilak
Right now the default qcow2 version if not specified explicitly,
defaults to 0.10. Certain features like live snapshots, cluster
size specification require 1.1 to work.

This patch series aims at upgrading the qcow2 default image version,
and use compatibility version 1.1 unless specified. This also requires
correcting multiple testcases related to this compat version.

If any other locations require changing, they will be
added as a part of this series.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/602

Abhiram Tilak (2):
  storage: change assigning qcow2 compat to 1.1 from 0.10 automatically
  storage: fix testcases on changing the default qcow2 compat to 1.1

 src/storage/storage_util.c  | 2 +-
 .../storagevolxml2argvdata/luks-convert-encrypt2fileqcow2.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-compat.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-from-logical-compat.argv | 2 +-
 tests/storagevolxml2argvdata/qcow2-luks-convert-encrypt.argv| 2 +-
 .../qcow2-luks-convert-encrypt2fileqcow2.argv   | 2 +-
 tests/storagevolxml2argvdata/qcow2-luks.argv| 2 +-
 .../qcow2-nobacking-convert-prealloc-compat.argv| 2 +-
 .../storagevolxml2argvdata/qcow2-nobacking-prealloc-compat.argv | 2 +-
 .../qcow2-nocapacity-convert-prealloc.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-nocapacity.argv  | 2 +-
 tests/storagevolxml2argvdata/qcow2-nocow-compat.argv| 2 +-
 tests/storagevolxml2argvdata/qcow2-zerocapacity.argv| 2 +-
 13 files changed, 13 insertions(+), 13 deletions(-)

--
2.42.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Prasad Pandit
Hello Zhao,

On Mon, 4 Mar 2024 at 12:19, Zhao Liu  wrote:
> > unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>
> This indicates the default maxcpus is initialized as 0 if user doesn't
> specifies it.

* 'has_maxcpus' should be set only if maxcpus > 0. If maxcpus == 0,
then setting 'has_maxcpus=1' seems convoluted.

> However, we could initialize maxcpus as other default value, e.g.,
>
> maxcpus = config->has_maxcpus ? config->maxcpus : 1.
===
hw/core/machine.c
 machine_initfn
/* default to mc->default_cpus */
ms->smp.cpus = mc->default_cpus;
ms->smp.max_cpus = mc->default_cpus;

   static void machine_class_base_init(ObjectClass *oc, void *data)
   {
   MachineClass *mc = MACHINE_CLASS(oc);
   mc->max_cpus = mc->max_cpus ?: 1;
   mc->min_cpus = mc->min_cpus ?: 1;
   mc->default_cpus = mc->default_cpus ?: 1;
   }
===
* Looking at the above bits, it seems smp.cpus & smp.max_cpus are
initialised to 1 via default_cpus in MachineClass object.

>>  if (config->has_maxcpus && config->maxcpus == 0)
> This check only wants to identify the case that user sets the 0.
> If the default maxcpus is initialized as 0, then (maxcpus == 0) will
> fail if user doesn't set maxcpus.
>
> But it is still necessary to distinguish whether maxcpus is user-set or
> auto-initialized.

* If it is set to zero(0) either by user or by auto-initialise, it is
still invalid, right?

> If it is user-set, -smp should fail is there's invalid maxcpus/invalid
> topology.
>
> Otherwise, if it is auto-initialized, its value should be adjusted based
> on other topology components as the above calculation in (*).

* Why have such diverging ways?
* Could we simplify it as
   - If cpus/maxcpus==0, it is invalid, show an error and exit.
   - If cpus/maxcpus > 0, but incorrect for topology, then
re-calculate the correct value based on topology parameters. If the
re-calculated value is still incorrect or unsatisfactory, then show an
error and exit.

* Saying that user setting cpu/maxcpus=0 is invalid and
auto-initialising it to zero(0) is valid, is not consistent.

...wdyt?
---
  - Prasad
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] ci: refresh with lcitool manifest

2024-03-04 Thread Andrea Bolognani
On Mon, Mar 04, 2024 at 04:34:35PM +, Daniel P. Berrangé wrote:
> Picks up the switch from FreeBSD 13.2 to 13.3
>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  ci/gitlab/builds.yml  | 2 +-
>  ci/lcitool/targets/freebsd-13.yml | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
>  delete mode 100644 ci/lcitool/targets/freebsd-13.yml

Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


join running core dump job

2024-03-04 Thread Thanos Makatos
Is there a way to programmatically wait for a previously initiated 
virDomainCoreDumpWithFormat() where the process that started it died? I'm 
looking at the API and don't seem to find anything relevant.  I suppose I could 
poll via virDomainGetJobStats(), but, ideally, I'd like a function that would 
join the dump job and return when the dump job finishes.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH v3 01/12] cpu_map: update script to handle versioned CPUs

2024-03-04 Thread Jim Fehlig

On 3/1/24 10:13, Daniel P. Berrangé wrote:

On Fri, Mar 01, 2024 at 10:36:12AM -0600, Jonathon Jongsma wrote:

On 3/1/24 10:13 AM, Daniel P. Berrangé wrote:

On Tue, Feb 20, 2024 at 05:08:02PM -0700, Jim Fehlig wrote:

On 12/15/23 15:11, Jonathon Jongsma wrote:

Previously, the script only generated the parent CPU and any versions
that had a defined alias. The script now generates all CPU versions. Any
version that had a defined alias will continue to use that alias, but
those without aliases will use the generated name $BASECPUNAME-vN.

The reason for this change is two-fold. First, we need to add new models
that support new features (such as SEV-SNP). To deal with this, the
script now generates model definitions for all versions.

But we also need to ensure that our CPU definitions are migration-safe.
To deal with this issue we need to make sure we're always using the
canonical versioned names for CPUs.


Related to migration safety, do we need to be concerned with the expansion
of 'host-model' CPU? E.g. is it possible 'host-model' expands to EPYC before
introducing the new models, and EPYC-v4 afterwards? If so, what are the
ramifications of that?


Yes, I see that happening on my laptop in domcapabilities:

Currently libvirt reports:

  
Snowridge
Intel



   ...snip...


and after this series it reports:

  
Snowridge-v4
Intel



   ...snip...


That's not wrong per-se, becasue Snowrigde-v4 has a smaller
delta against my host CPU.

The problem is that libvirt updates the *live* XML for the
guest with this expansion.  IIUC, if we now attempt to
live migrate to a compatible machine running older libvirt
the migrate will fail as old libvirt doesn't know the -v4
CPU.


Downstream, we (SUSE) don't really support migrating from new -> old. Is this 
something we aim to support upstream?




I'm not sure how to address this ?


But don't we have this issue any time we add a new CPU model to libvirt?
Anytime there's a new model, it has the potential to be a closer match to
the host CPU than an existing model definition was. As I mentioned in my
previous reply, when e.g. the -noTSX CPU variants were added, didn't the
same sort of thing (potentially) happen? Or am I doing something
meaningfully different in this patch set than what happens in those
scenarios?


I think it probably /did/ happen, but that doesn't make it acceptable.
The noTSX stuff was the cause of massive amounts of compatibility pain
for mgmt apps, so the incompatibility in libvirt might have been glossed
over. We're adding alot of new versions here, so the possibly increasing
the visibility/impact of this libvirt change.


It can happen when we introduce an entirely new CPU model too. E.g. on a Genoa 
machine, prior to commit bfe53e9145c, host model expanded to


 
EPYC-Milan
AMD



































  

After commit bfe53e9145c


EPYC-Genoa
AMD



















  

Regards,
Jim
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] qemu: Optimize CPU check='partial' for usable CPUs

2024-03-04 Thread Jiri Denemark
Ideally check='partial' would check exactly the features QEMU would want
to enable when asked for a specific CPU model (and features). But there
is no way we could ask QEMU how a specific CPU would look like. So we
use our definition from CPU map, which may slightly differ as QEMU adds
or removes features from CPU models, and thus we may end up checking
features which QEMU would not enable while missing some required ones.

We can do better in specific cases, though. If a CPU definition uses
only a model and disabled features (or none at all), we already know
whether QEMU can enable all features required by the CPU model as that's
what we use to set usable='yes' attribute in the list of available CPU
models in domain capbilities XML. So when a usable CPU model is
requested without asking for additional features (disabling features is
fine) we can avoid our possible inaccurate check using our CPU map.

For backward compatibility we only consider usable models. If a
specified model is not usable, we still check it the old way and even
let QEMU start it (and disable some features) in case our definition
lacks some features compared to QEMU.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_capabilities.c | 44 
 src/qemu/qemu_capabilities.h |  3 +++
 src/qemu/qemu_process.c  |  1 +
 3 files changed, 48 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e383d85920..cb9846dfcf 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2433,6 +2433,50 @@ virQEMUCapsIsCPUDeprecated(virQEMUCaps *qemuCaps,
 }
 
 
+/**
+ * virQEMUCapsIsCPUUsable:
+ * @qemuCaps: QEMU capabilities
+ * @type: virtualization type (kvm vs tcg)
+ * @cpu: CPU definition to check, including explicitly configured features
+ *
+ * Checks whether @cpu is considered usable by QEMU, i.e., all features
+ * required by the CPU model are supported and can be enabled. If so, we can
+ * avoid checking the CPU according to its definition in the CPU map when
+ * starting a domain with check='partial'. Our checks could be inaccurate
+ * anyway because QEMU might have changed the definition of the CPU model
+ * since we added it into the CPU map.
+ *
+ * Returns true iff @cpu is usable.
+ */
+bool
+virQEMUCapsIsCPUUsable(virQEMUCaps *qemuCaps,
+   virDomainVirtType type,
+   virCPUDef *cpu)
+{
+qemuMonitorCPUDefs *defs;
+size_t i;
+
+if (!cpu->model ||
+!(defs = virQEMUCapsGetAccel(qemuCaps, type)->cpuModels))
+return false;
+
+/* CPU model usability is valid only when CPU def does not contain any
+ * features or all features are disabled.
+ */
+for (i = 0; i < cpu->nfeatures; i++) {
+if (cpu->features[i].policy != VIR_CPU_FEATURE_DISABLE)
+return false;
+}
+
+for (i = 0; i < defs->ncpus; i++) {
+if (STREQ(defs->cpus[i].name, cpu->model))
+return defs->cpus[i].usable == VIR_DOMCAPS_CPU_USABLE_YES;
+}
+
+return false;
+}
+
+
 bool
 virQEMUCapsIsMachineDeprecated(virQEMUCaps *qemuCaps,
virDomainVirtType type,
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 486a4a6f63..ef1ad2c01c 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -770,6 +770,9 @@ const char *virQEMUCapsGetMachineDefaultCPU(virQEMUCaps 
*qemuCaps,
 bool virQEMUCapsIsCPUDeprecated(virQEMUCaps *qemuCaps,
 virDomainVirtType type,
 const char *model);
+bool virQEMUCapsIsCPUUsable(virQEMUCaps *qemuCaps,
+virDomainVirtType type,
+virCPUDef *cpu);
 bool virQEMUCapsIsMachineDeprecated(virQEMUCaps *qemuCaps,
 virDomainVirtType type,
 const char *machine);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6e51d6586b..8e35d0a73f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6288,6 +6288,7 @@ qemuProcessUpdateGuestCPU(virDomainDef *def,
 g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;
 
 if (def->cpu->check == VIR_CPU_CHECK_PARTIAL &&
+!virQEMUCapsIsCPUUsable(qemuCaps, def->virtType, def->cpu) &&
 virCPUCompare(hostarch,
   virQEMUCapsGetHostModel(qemuCaps, def->virtType,
   VIR_QEMU_CAPS_HOST_CPU_FULL),
-- 
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] cpu: x86: Check for invalid CPU data from hypervisor

2024-03-04 Thread Jiri Denemark
Recently a kernel bug caused QEMU to report a CPU feature as enabled
while listing it in the "unavailable-features" list of features that
were requested, but could not be enabled. The feature was actually
enabled, but we marked it as disabled when starting a domain. Later when
the domain is migrated, the destination requests the feature to be
disabled, which breaks the guest ABI or if we are lucky QEMU just fails
to load the migration stream.

Let's make similar bugs more visible in the future by refusing to even
start the domain.

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu_x86.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 6b2531b360..e8409ce616 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -3019,6 +3019,14 @@ virCPUx86UpdateLive(virCPUDef *cpu,
  x86DataIsSubset(>data, >data))
 expected = VIR_CPU_FEATURE_DISABLE;
 
+if (x86DataIsSubset(, >data) &&
+x86DataIsSubset(, >data)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("hypervisor provided conflicting CPU data: 
feature '%1$s' is both enabled and disabled at the same time"),
+   feature->name);
+return -1;
+}
+
 if (expected == VIR_CPU_FEATURE_DISABLE &&
 x86DataIsSubset(, >data)) {
 VIR_DEBUG("Feature '%s' enabled by the hypervisor", feature->name);
-- 
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] ci: refresh with lcitool manifest

2024-03-04 Thread Daniel P . Berrangé
Picks up the switch from FreeBSD 13.2 to 13.3

Signed-off-by: Daniel P. Berrangé 
---
 ci/gitlab/builds.yml  | 2 +-
 ci/lcitool/targets/freebsd-13.yml | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
 delete mode 100644 ci/lcitool/targets/freebsd-13.yml

diff --git a/ci/gitlab/builds.yml b/ci/gitlab/builds.yml
index 49a0b27029..b23a3989b9 100644
--- a/ci/gitlab/builds.yml
+++ b/ci/gitlab/builds.yml
@@ -619,7 +619,7 @@ x86_64-freebsd-13:
   needs: []
   allow_failure: false
   variables:
-CIRRUS_VM_IMAGE_NAME: freebsd-13-2
+CIRRUS_VM_IMAGE_NAME: freebsd-13-3
 CIRRUS_VM_IMAGE_SELECTOR: image_family
 CIRRUS_VM_INSTANCE_TYPE: freebsd_instance
 INSTALL_COMMAND: pkg install -y
diff --git a/ci/lcitool/targets/freebsd-13.yml 
b/ci/lcitool/targets/freebsd-13.yml
deleted file mode 100644
index 0337926479..00
--- a/ci/lcitool/targets/freebsd-13.yml
+++ /dev/null
@@ -1,2 +0,0 @@
-cirrus:
-  image_name: freebsd-13-2
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Zhao Liu
Hi Prasad,

On Mon, Mar 04, 2024 at 11:23:58AM +0530, Prasad Pandit wrote:
> Date: Mon, 4 Mar 2024 11:23:58 +0530
> From: Prasad Pandit 
> Subject: Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0"
>  SMP configurations
> 
> On Mon, 4 Mar 2024 at 10:02, Zhao Liu  wrote:
> > diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> > index 25019c91ee36..96533886b14e 100644
> > --- a/hw/core/machine-smp.c
> > +++ b/hw/core/machine-smp.c
> > @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
> >  (config->has_cores && config->cores == 0) ||
> >  (config->has_threads && config->threads == 0) ||
> >  (config->has_maxcpus && config->maxcpus == 0)) {
> > -warn_report("Deprecated CPU topology (considered invalid): "
> > -"CPU topology parameters must be greater than zero");
> > +error_setg(errp, "Invalid CPU topology: "
> > +   "CPU topology parameters must be greater than zero");
> > +return;
> >  }
> 
> unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;

This indicates the default maxcpus is initialized as 0 if user doesn't
specifies it.

For this case - no user configuration - maxcpus will be re-calculated
as:

maxcpus = maxcpus > 0 ? maxcpus : drawers * books * sockets * dies *
  clusters * cores * threads; (*)

>  ...
>  if (config->has_maxcpus && config->maxcpus == 0)

This check only wants to identify the case that user sets the 0.

> 
> * The check (has_maxcpus && maxcpus == 0) seems to be repeating above,
> maybe we could check if (maxcpus == 0) error_setg().

If the default maxcpus is initialized as 0, then (maxcpus == 0) will
fail if user doesn't set maxcpus.

However, we could initialize maxcpus as other default value, e.g., 

maxcpus = config->has_maxcpus ? config->maxcpus : 1.

But it is still necessary to distinguish whether maxcpus is user-set or
auto-initialized.

If it is user-set, -smp should fail is there's invalid maxcpus/invalid
topology.

Otherwise, if it is auto-initialized, its value should be adjusted based
on other topology components as the above calculation in (*).

> And same for
> other topology parameters?

Other parameters also have the similar needs to distinguish if they're
set by user. So the check needs to also cover has_* fields.

> * Also a check to ensure cpus <= maxcpus is required I think.
>

Yes, the valid topology needs this. This code block already covers this
case ;-):

if (maxcpus < cpus) {
g_autofree char *topo_msg = cpu_hierarchy_to_string(ms);
error_setg(errp, "Invalid CPU topology: "
   "maxcpus must be equal to or greater than smp: "
   "%s == maxcpus (%u) < smp_cpus (%u)",
   topo_msg, maxcpus, cpus);
return;
}

Thanks,
Zhao

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


Re: [PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Prasad Pandit
On Mon, 4 Mar 2024 at 10:02, Zhao Liu  wrote:
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 25019c91ee36..96533886b14e 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
>  (config->has_cores && config->cores == 0) ||
>  (config->has_threads && config->threads == 0) ||
>  (config->has_maxcpus && config->maxcpus == 0)) {
> -warn_report("Deprecated CPU topology (considered invalid): "
> -"CPU topology parameters must be greater than zero");
> +error_setg(errp, "Invalid CPU topology: "
> +   "CPU topology parameters must be greater than zero");
> +return;
>  }

unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
 ...
 if (config->has_maxcpus && config->maxcpus == 0)

* The check (has_maxcpus && maxcpus == 0) seems to be repeating above,
maybe we could check if (maxcpus == 0) error_setg(). And same for
other topology parameters?
* Also a check to ensure cpus <= maxcpus is required I think.

Thank you.
---
  - Prasad
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org


[PATCH] hw/core/machine-smp: Remove deprecated "parameter=0" SMP configurations

2024-03-04 Thread Zhao Liu
From: Zhao Liu 

The "parameter=0" SMP configurations have been marked as deprecated
since v6.2.

For these cases, -smp currently returns the warning and adjusts the
zeroed parameters to 1 by default.

Remove the above compatibility logic in v9.0, and return error directly
if any -smp parameter is set as 0.

Signed-off-by: Zhao Liu 
---
 docs/about/deprecated.rst   | 16 
 docs/about/removed-features.rst | 15 +++
 hw/core/machine-smp.c   |  5 +++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 36bd3e15ef06..872974640252 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -36,22 +36,6 @@ and will cause a warning.
 The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on``
 rather than ``delay=off``.
 
-``-smp`` ("parameter=0" SMP configurations) (since 6.2)
-'''
-
-Specified CPU topology parameters must be greater than zero.
-
-In the SMP configuration, users should either provide a CPU topology
-parameter with a reasonable value (greater than zero) or just omit it
-and QEMU will compute the missing value.
-
-However, historically it was implicitly allowed for users to provide
-a parameter with zero value, which is meaningless and could also possibly
-cause unexpected results in the -smp parsing. So support for this kind of
-configurations (e.g. -smp 8,sockets=0) is deprecated since 6.2 and will
-be removed in the near future, users have to ensure that all the topology
-members described with -smp are greater than zero.
-
 Plugin argument passing through ``arg=`` (since 6.1)
 
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 417a0e4fa1d9..f9cf874f7b1f 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -489,6 +489,21 @@ The ``-singlestep`` option has been turned into an 
accelerator property,
 and given a name that better reflects what it actually does.
 Use ``-accel tcg,one-insn-per-tb=on`` instead.
 
+``-smp`` ("parameter=0" SMP configurations) (removed in 9.0)
+
+
+Specified CPU topology parameters must be greater than zero.
+
+In the SMP configuration, users should either provide a CPU topology
+parameter with a reasonable value (greater than zero) or just omit it
+and QEMU will compute the missing value.
+
+However, historically it was implicitly allowed for users to provide
+a parameter with zero value, which is meaningless and could also possibly
+cause unexpected results in the -smp parsing. So support for this kind of
+configurations (e.g. -smp 8,sockets=0) is removed since 9.0, users have
+to ensure that all the topology members described with -smp are greater
+than zero.
 
 User-mode emulator command line arguments
 -
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 25019c91ee36..96533886b14e 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -105,8 +105,9 @@ void machine_parse_smp_config(MachineState *ms,
 (config->has_cores && config->cores == 0) ||
 (config->has_threads && config->threads == 0) ||
 (config->has_maxcpus && config->maxcpus == 0)) {
-warn_report("Deprecated CPU topology (considered invalid): "
-"CPU topology parameters must be greater than zero");
+error_setg(errp, "Invalid CPU topology: "
+   "CPU topology parameters must be greater than zero");
+return;
 }
 
 /*
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org