Re: [PATCH 04/14] qemu: Generalize filtering in qemuMonitorJSONParsePropsList

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:52 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> qemuMonitorJSONParsePropsList supported filtering based on type. Let's
> replace it with a callback supplied by the caller to allow for more
> advanced filtering.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 51 +---
>  1 file changed, 41 insertions(+), 10 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 2/8] qemu: Allow to define NUMA nodes without memory or CPUs assigned

2025-09-08 Thread Daniel P . Berrangé via Devel
On Sat, Sep 06, 2025 at 03:08:57PM +0200, Andrea Righi wrote:
> Allow to define NUMA nodes without memory or CPUs assigned to properly
> support the new acpi-generic-initiator device.
> 
> This is required because the NUMA nodes passed to the
> acpi-generic-initiator object must be independent and not be shared with
> other resources, such as CPU or memory.
> 
> Signed-off-by: Andrea Righi 
> ---
>  src/conf/numa_conf.c|  3 +++
>  src/qemu/qemu_command.c | 19 ---
>  2 files changed, 15 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé 


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



Re: [PATCH 8/8] NEWS: Mention new acpi-generic-initiator support

2025-09-08 Thread Daniel P . Berrangé via Devel
On Sat, Sep 06, 2025 at 03:09:03PM +0200, Andrea Righi wrote:
> Signed-off-by: Andrea Righi 
> ---
>  NEWS.rst | 10 ++
>  1 file changed, 10 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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



Re: [PATCH v3 01/13] ch: add ch_hotplug.{h,c} files to CH build

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> The files are meant to contain all device hotplug related code. The
> first implementation will be live storage attach and detach.
> 
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_hotplug.c | 35 +++
>  src/ch/ch_hotplug.h | 27 +++
>  src/ch/meson.build  |  2 ++
>  3 files changed, 64 insertions(+)
>  create mode 100644 src/ch/ch_hotplug.c
>  create mode 100644 src/ch/ch_hotplug.h

This could have been merged with one of patches that actually fills the
file with useful code. But at this point I'm too lazy to fix that O:-)

Michal



Re: [libvirt PATCH] esx: pass 'long' to curl_easy_setopt when needed

2025-09-08 Thread Peter Krempa via Devel
On Tue, Sep 02, 2025 at 14:34:04 +0200, Ján Tomko via Devel wrote:
> From: Ján Tomko 
> 
> The include header got its type checks fixed in curl 8.14:
> https://github.com/curl/curl/commit/79b4e56b3f30dc1ac28a81128a07d27338e5219e
> https://github.com/curl/curl/pull/17143
> 
> This causes a warning on rawhide with clang:
> ../src/esx/esx_vi.c:318:5: error: call to '_curl_easy_setopt_err_long'
> declared with 'warning' attribute: curl_easy_setopt expects a long
> argument [-Werror,-Wattribute-warning]
>   318 | curl_easy_setopt(curl->handle, CURLOPT_NOSIGNAL, 1);
>   | ^
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/esx/esx_stream.c |  6 +++---
>  src/esx/esx_vi.c | 24 
>  2 files changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 10/14] qemu: Merge qemuMonitorJSONGetCPUData in qemuMonitorJSONGetGuestCPU

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:58 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The qemuMonitorJSONGetCPUData function is just a wrapper around two
> function calls and it is only used by qemuMonitorJSONGetGuestCPU.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 27 ++-
>  1 file changed, 6 insertions(+), 21 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 12/14] qemu: Merge qemuMonitorJSONGetCPUDataDisabled in qemuMonitorJSONGetGuestCPU

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:47:00 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The qemuMonitorJSONGetCPUDataDisabled function is just a wrapper around
> two function calls and it is only used by qemuMonitorJSONGetGuestCPU.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 26 ++
>  1 file changed, 6 insertions(+), 20 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 11/14] qemu: Always fetch disabled features in qemuMonitorJSONGetGuestCPU

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:59 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The function is always called with both enabled and disabled pointers
> set.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 6fa2f447db..06e0f3794e 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6784,13 +6784,11 @@ qemuMonitorJSONGetGuestCPU(qemuMonitor *mon,
>  if (qemuMonitorJSONCPUDataAddFeatures(cpuEnabled, propsEnabled, 
> translate) < 0)
>  return -1;
>  
> -if (disabled &&
> -qemuMonitorJSONGetCPUDataDisabled(mon, cpuQOMPath, translate, 
> cpuDisabled) < 0)
> +if (qemuMonitorJSONGetCPUDataDisabled(mon, cpuQOMPath, translate, 
> cpuDisabled) < 0)
>  return -1;
>  
>  *enabled = g_steal_pointer(&cpuEnabled);
> -if (disabled)
> -*disabled = g_steal_pointer(&cpuDisabled);
> +*disabled = g_steal_pointer(&cpuDisabled);


The common monitor code wrapper:

int
qemuMonitorGetGuestCPU(qemuMonitor *mon,
   virArch arch,
   bool qomListGet,
   const char *cpuQOMPath,
   qemuMonitorCPUFeatureTranslationCallback translate,
   virCPUData **enabled,
   virCPUData **disabled)
{
VIR_DEBUG("arch=%s qomListGet%d cpuQOMPath=%s translate=%p "
  "enabled=%p disabled=%p",
  virArchToString(arch), qomListGet, cpuQOMPath, translate,
  enabled, disabled);

QEMU_CHECK_MONITOR(mon);

*enabled = NULL;
if (disabled)
*disabled = NULL;

return qemuMonitorJSONGetGuestCPU(mon, arch, qomListGet, cpuQOMPath,
  translate, enabled, disabled);
}


Also checks 'disabled'. Fix that one too.

Reviewed-by: Peter Krempa 



Re: [PATCH v3 1/3] qemu: support riscv-aia feature for RISC-V KVM

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 17:04:49 +0800, BillXiang wrote:
> From: xiangwencheng 
> 
> riscv-aia feature was introduced in qemu-8.2 to specify the
> KVM AIA mode. The "riscv-aia" parameter is passed along with
> -accel in QEMU command-line.
> 1) "riscv-aia=emul": IMSIC is emulated by hypervisor
> 2) "riscv-aia=hwaccel": use hardware guest IMSIC
> 3) "riscv-aia=auto": use the hardware guest IMSICs whenever available
> otherwise we fallback to software emulation.
> 
> This patch add the corresponding feature named 'riscv-aia'.
> 
> Signed-off-by: BillXiang 
> ---
>  src/conf/domain_conf.c| 28 +++-
>  src/conf/domain_conf.h| 11 +++
>  src/conf/schemas/domaincommon.rng | 12 

For anything adding XML schema we require that test XMLs excercise it.
Both for the parser and also that some bindings projects take the XMLs
to excercise their XML infra.

>  src/libvirt_private.syms  |  2 ++
>  src/qemu/qemu_command.c   | 13 ++---
>  5 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7766e302ec..a9f1161133 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -224,6 +224,14 @@ VIR_ENUM_IMPL(virDomainKVM,
>"poll-control",
>"pv-ipi",
>"dirty-ring",
> +  "riscv-aia",
> +);
> +
> +VIR_ENUM_IMPL(virDomainKVMRiscvAIAMode,
> +  VIR_DOMAIN_KVM_TISCV_AIA_MODE_LAST,
> +  "auto",
> +  "emul",
> +  "hwaccel",
>  );
>  
>  VIR_ENUM_IMPL(virDomainXen,
> @@ -17183,6 +17191,15 @@ virDomainFeaturesKVMDefParse(virDomainDef *def,
>  return -1;
>  }
>  }
> +
> +if (feature == VIR_DOMAIN_KVM_RISCV_AIA &&
> +value == VIR_TRISTATE_SWITCH_ON) {
> +if (virXMLPropEnum(feat, "mode",
> +virDomainKVMRiscvAIAModeTypeFromString,
> +VIR_XML_PROP_REQUIRED,

[1] declared as REQUIRED here ...

> +&def->kvm_features->kvm_riscv_aia_mode) < 0)

Broken indentation.

> +return -1;
> +}
>  }
>  
>  def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON;
> @@ -21697,6 +21714,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef 
> *src,
>  case VIR_DOMAIN_KVM_POLLCONTROL:
>  case VIR_DOMAIN_KVM_PVIPI:
>  case VIR_DOMAIN_KVM_DIRTY_RING:
> +case VIR_DOMAIN_KVM_RISCV_AIA:
>  if (src->kvm_features->features[i] != 
> dst->kvm_features->features[i]) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("State of KVM feature '%1$s' differs: 
> source: '%2$s', destination: '%3$s'"),
> @@ -28752,7 +28770,15 @@ virDomainDefFormatFeatures(virBuffer *buf,
>  }
>  }
>  break;
> -
> +case VIR_DOMAIN_KVM_RISCV_AIA:
> +if (def->kvm_features->features[j] != 
> VIR_TRISTATE_SWITCH_ABSENT) {
> +virBufferAsprintf(&childBuf, "<%s state='%s'",
> +  virDomainKVMTypeToString(j),
> +  
> virTristateSwitchTypeToString(def->kvm_features->features[j]));
> +virBufferAsprintf(&childBuf, " mode='%s'/>\n",
> +
> virDomainKVMRiscvAIAModeTypeToString(def->kvm_features->kvm_riscv_aia_mode));

broken indent.

> +}
> +break;
>  case VIR_DOMAIN_KVM_LAST:
>  break;
>  }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index eca820892e..0250b3db49 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2270,10 +2270,20 @@ typedef enum {
>  VIR_DOMAIN_KVM_POLLCONTROL,
>  VIR_DOMAIN_KVM_PVIPI,
>  VIR_DOMAIN_KVM_DIRTY_RING,
> +VIR_DOMAIN_KVM_RISCV_AIA,
>  
>  VIR_DOMAIN_KVM_LAST
>  } virDomainKVM;
>  
> +typedef enum {
> +VIR_DOMAIN_KVM_TISCV_AIA_MODE_AUTO = 0,
> +VIR_DOMAIN_KVM_TISCV_AIA_MODE_EMUL,
> +VIR_DOMAIN_KVM_TISCV_AIA_MODE_HWACCEL,
> +
> +VIR_DOMAIN_KVM_TISCV_AIA_MODE_LAST
> +} virDomainKVMRiscvAIAMode;
> +VIR_ENUM_DECL(virDomainKVMRiscvAIAMode);
> +
>  typedef enum {
>  VIR_DOMAIN_MSRS_UNKNOWN = 0,
>  
> @@ -2476,6 +2486,7 @@ struct _virDomainFeatureKVM {
>  int features[VIR_DOMAIN_KVM_LAST];
>  
>  unsigned int dirty_ring_size; /* size of dirty ring for each vCPU, no 
> units */
> +virDomainKVMRiscvAIAMode kvm_riscv_aia_mode;
>  };
>  
>  typedef struct _virDomainFeatureTCG virDomainFeatureTCG;
> diff --git a/src/conf/schemas/domaincommon.rng 
> b/src/conf/schemas/domaincommon.rng
> index e369fb6e81..02771a6b7b 100644
> --- a/src/conf/schemas/domaincommon.rng

Re: [PATCH v3 3/3] docs: formatdomain: add doc for riscv-aia feature

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 17:09:15 +0800, BillXiang wrote:
> From: xiangwencheng 
> 
> Signed-off-by: BillXiang 
> ---
>  docs/formatdomain.rst | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 9f7311b6d5..a694eeb140 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -2085,6 +2085,7 @@ Hypervisors may allow certain CPU / machine features to 
> be toggled on/off.
> 
> 
> 
> +   
>   
>   
> 
> @@ -2206,6 +2207,7 @@ are:
> poll-control   Decrease IO completion latency by introducing a grace 
> period of busy waiting on, off
> :since:`6.10.0 (QEMU 4.2)`
> pv-ipi Paravirtualized send IPIs  
>   on, off
> :since:`7.10.0 (QEMU 3.1)`
> dirty-ring Enable dirty ring feature  
>   on, off; size - must be power of 2, range [1024,65536] 
> :since:`8.0.0 (QEMU 6.1)`
> +   riscv-aia  Set riscv KVM AIA mode. Defaults to 'auto'.
>   on, off; mode - optional string emul, hwaccel or auto  
> :since:`11.8.0 (QEMU 8.2)`

'mode' is not actually optional per the implementation.

> == 
>  
> == 
> 

As noted in 2/3, this documentation is much less detailed. Move the
majority of the details here. Also this really should be squashed into
1/3 or whatever patch is actually dealing with the parser.

Test XMLs can be added separately.



Re: [PATCH 07/14] qemu: Use qom-list-get for checking enabled CPU features

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:55 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> qom-list-get is a new QMP command (since QEMU 10.1) that combines
> qom-list for listing properties of a specified object with qom-get for
> getting a value of a given property. The new command provides an array
> of all properties and their values, which allows us to dramatically
> reduce the number of QMP commands we have to call when starting a domain
> to check which CPU features were actually enabled.
> 
> A simple domain with no disk can now be started with only 15 QMP
> commands in about 200 ms compared to 485 commands and 400 ms startup
> time without this patch.
> 
> https://issues.redhat.com/browse/RHEL-7038
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_capabilities.c  |  2 +
>  src/qemu/qemu_capabilities.h  |  1 +
>  src/qemu/qemu_monitor.c   | 13 +++-
>  src/qemu/qemu_monitor.h   |  1 +
>  src/qemu/qemu_monitor_json.c  | 73 ---
>  src/qemu/qemu_monitor_json.h  |  7 +-
>  src/qemu/qemu_process.c   |  1 +
>  .../caps_10.1.0_x86_64.xml|  1 +
>  .../caps_10.2.0_x86_64.xml|  1 +
>  ...=> get-guest-cpu-SierraForest-legacy.json} |  0
>  ...> get-guest-cpu-SkylakeClient-legacy.json} |  0
>  tests/qemumonitorjsontest.c   | 18 +++--
>  12 files changed, 92 insertions(+), 26 deletions(-)
>  rename tests/qemumonitorjsondata/{get-guest-cpu-SierraForest.json => 
> get-guest-cpu-SierraForest-legacy.json} (100%)
>  rename tests/qemumonitorjsondata/{get-guest-cpu-SkylakeClient.json => 
> get-guest-cpu-SkylakeClient-legacy.json} (100%)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 688d100b01..b7174c657d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -742,6 +742,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"amd-iommu.pci-id", /* QEMU_CAPS_AMD_IOMMU_PCI_ID */
>"usb-bot", /* QEMU_CAPS_DEVICE_USB_BOT */
>"tdx-guest", /* QEMU_CAPS_TDX_GUEST */
> +  "qom-list-get", /* QEMU_CAPS_QOM_LIST_GET */
>  );
>  
>  
> @@ -1256,6 +1257,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = {
>  { "query-stats-schemas", QEMU_CAPS_QUERY_STATS_SCHEMAS },
>  { "display-reload", QEMU_CAPS_DISPLAY_RELOAD },
>  { "blockdev-set-active", QEMU_CAPS_BLOCKDEV_SET_ACTIVE },
> +{ "qom-list-get", QEMU_CAPS_QOM_LIST_GET },
>  };
>  
>  struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 8916973364..f50d908b3f 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -723,6 +723,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
> syntax-check */
>  QEMU_CAPS_AMD_IOMMU_PCI_ID, /* amd-iommu.pci-id */
>  QEMU_CAPS_DEVICE_USB_BOT, /* -device usb-bot */
>  QEMU_CAPS_TDX_GUEST, /* -object tdx-guest,... */
> +QEMU_CAPS_QOM_LIST_GET, /* qom-list-get QMP command */
>  
>  QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;

Preferrably introduce new capabilities in separate patch.



> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 0213bd5af8..176651eab4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3673,6 +3673,8 @@ qemuMonitorSetDomainLog(qemuMonitor *mon,
>   * qemuMonitorGetGuestCPU:
>   * @mon: Pointer to the monitor
>   * @arch: CPU architecture
> + * @qomListGet: QEMU supports getting list of features and their values using
> + *  a single qom-list-get QMP command
>   * @cpuQOMPath: QOM path of a CPU to probe
>   * @translate: callback for translating CPU feature names from QEMU to 
> libvirt
>   * @opaque: data for @translate callback
> @@ -3687,13 +3689,16 @@ qemuMonitorSetDomainLog(qemuMonitor *mon,
>  int
>  qemuMonitorGetGuestCPU(qemuMonitor *mon,
> virArch arch,
> +   bool qomListGet,
> const char *cpuQOMPath,
> qemuMonitorCPUFeatureTranslationCallback translate,
> virCPUData **enabled,
> virCPUData **disabled)
>  {
> -VIR_DEBUG("arch=%s cpuQOMPath=%s translate=%p enabled=%p disabled=%p",
> -  virArchToString(arch), cpuQOMPath, translate, enabled, 
> disabled);
> +VIR_DEBUG("arch=%s qomListGet%d cpuQOMPath=%s translate=%p "

qomListGet=%d instead of qomListGet%d

> +  "enabled=%p disabled=%p",
> +  virArchToString(arch), qomListGet, cpuQOMPath, translate,
> +  enabled, disabled);
>  
>  QEMU_CHECK_MONITOR(mon);
>  

[...]


> @@ -6631,14 +6648,29 @@ qemuMonitorJSONGetCPUProperties(qemuMonitor *mon,
>  virJSONValue *array;
>  struct _qemuMonitorJSONCPUPropsFi

Re: [PATCH 08/14] tests: Test qemuMonitorJSONGetGuestCPU with qom-get-list

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:56 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The test cases show both the legacy method and the new one produce
> identical results.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  .../get-guest-cpu-SierraForest.json   | 2985 +
>  .../get-guest-cpu-SkylakeClient.json  | 2967 
>  tests/qemumonitorjsontest.c   |2 +
>  3 files changed, 5954 insertions(+)
>  create mode 100644 tests/qemumonitorjsondata/get-guest-cpu-SierraForest.json
>  create mode 100644 tests/qemumonitorjsondata/get-guest-cpu-SkylakeClient.json

This data belongs more into the previous patch than the capability
addition.

Anyways since it's a masive diff keep it separate.

Reviewed-by: Peter Krempa 



Re: [PATCH v3 10/13] ch: add virCHMonitorBuildKeyValueJson

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_monitor.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index 710ba06d2d..6ed78c09c7 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -584,15 +584,25 @@ virCHMonitorBuildVMJson(virCHDriver *driver, 
> virDomainDef *vmdef,
>  return 0;
>  }
>  
> +static virJSONValue*
> +virCHMonitorBuildKeyValueJson(const char *key,
> +  const char *value)
> +{
> +g_autoptr(virJSONValue) content = virJSONValueNewObject();
> +
> +if (virJSONValueObjectAppendString(content, key, value) < 0)
> +return NULL;
> +
> +return g_steal_pointer(&content);
> +}
> +
>  static int
>  virCHMonitorBuildKeyValueStringJson(char **jsonstr,
>  const char *key,
>  const char *value)
>  {
> -g_autoptr(virJSONValue) content = virJSONValueNewObject();
> -
> -if (virJSONValueObjectAppendString(content, key, value) < 0)
> -return -1;
> +g_autoptr(virJSONValue) content =
> +virCHMonitorBuildKeyValueJson(key, value);

This fits onto one line perfectly.

>  
>  if (!(*jsonstr = virJSONValueToString(content, false)))
>  return -1;

Michal



Re: [PATCH v3 08/13] ch: implement disk attach in public API

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_driver.c  | 44 +
>  src/ch/ch_hotplug.c | 48 -
>  2 files changed, 87 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index cf6874f22e..4f4783efb1 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -25,6 +25,7 @@
>  #include "ch_conf.h"
>  #include "ch_domain.h"
>  #include "ch_driver.h"
> +#include "ch_hotplug.h"
>  #include "ch_monitor.h"
>  #include "ch_process.h"
>  #include "domain_cgroup.h"
> @@ -2344,6 +2345,47 @@ chDomainInterfaceAddresses(virDomain *dom,
>  return ret;
>  }
>  
> +static int
> +chDomainAttachDeviceFlags(virDomainPtr dom,
> +  const char *xml,
> +  unsigned int flags)
> +{
> +virCHDriver *driver = dom->conn->privateData;
> +virDomainObj *vm = NULL;
> +int ret = -1;
> +
> +if (!(vm = virCHDomainObjFromDomain(dom)))
> +goto cleanup;
> +
> +if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> +goto cleanup;
> +
> +if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> +goto cleanup;
> +
> +if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> +goto endjob;
> +
> +if (chDomainAttachDeviceLiveAndUpdateConfig(vm, driver, xml, flags) < 0) 
> {
> +goto endjob;
> +}
> +
> +ret = 0;
> +
> + endjob:
> +virDomainObjEndJob(vm);
> +
> + cleanup:
> +virDomainObjEndAPI(&vm);
> +return ret;
> +}
> +
> +static int
> +chDomainAttachDevice(virDomainPtr dom,
> + const char *xml)
> +{
> +return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE);
> +}
>  
>  /* Function Tables */
>  static virHypervisorDriver chHypervisorDriver = {
> @@ -2406,6 +2448,8 @@ static virHypervisorDriver chHypervisorDriver = {
>  .connectDomainEventRegisterAny = chConnectDomainEventRegisterAny,   
> /* 10.10.0 */
>  .connectDomainEventDeregisterAny = chConnectDomainEventDeregisterAny,   
> /* 10.10.0 */
>  .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */
> +.domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */
> +.domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */
>  };
>  
>  static virConnectDriver chConnectDriver = {

Until here the change makes sense. But what's below should be part of
previous patch.

> diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c
> index 7de688dc44..524355b93c 100644
> --- a/src/ch/ch_hotplug.c
> +++ b/src/ch/ch_hotplug.c
> @@ -50,7 +50,6 @@ chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, 
> virDomainDiskDef *disk)
>  return 0;
>  }
>  
> -G_GNUC_UNUSED
>  static int
>  chDomainAttachDeviceLive(virDomainObj *vm,
>   virDomainDeviceDef *dev)
> @@ -108,13 +107,52 @@ chDomainAttachDeviceLive(virDomainObj *vm,
>  }
>  
>  int
> -chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED,
> -virCHDriver *driver G_GNUC_UNUSED,
> -const char *xml G_GNUC_UNUSED,
> +chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm,
> +virCHDriver *driver,
> +const char *xml,
>  unsigned int flags)
>  {
> +unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +   VIR_DOMAIN_DEF_PARSE_ABI_UPDATE;
> +g_autoptr(virDomainDeviceDef) devLive = NULL;
> +g_autoptr(virDomainDef) vmdef = NULL;
> +g_autoptr(virCHDriverConfig) cfg = NULL;
> +g_autoptr(virDomainDeviceDef) devConf = NULL;
> +
>  virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>VIR_DOMAIN_AFFECT_CONFIG, -1);
>  
> -return -1;
> +cfg = virCHDriverGetConfig(driver);
> +
> +if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +   _("Persistent domain state changes are not 
> supported"));

At first I was a bit puzzled why have this at all. I mean,
virCheckFlags() could have just contained _AFFECT_LIVE but then it hit
me: this produces much more descriptive error message. So let's keep it
then.

> +return -1;
> +}
> +
> +if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
> +driver->xmlopt, NULL,
> +parse_flags))) {
> +return -1;
> +}
> +
> +if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
> +   VIR_DOMAIN_AFFECT_LIVE) < 
> 0)
> +return -1;
> +
> +if (virDomainDefCompatibleDevice(vm->def, de

Re: [PATCH v3 12/13] ch: implement disk device detach in public API

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_driver.c  |  42 +++
>  src/ch/ch_hotplug.c | 175 
>  src/ch/ch_hotplug.h |   6 ++
>  3 files changed, 223 insertions(+)
> 
> diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c
> index 4f4783efb1..760fccba82 100644
> --- a/src/ch/ch_driver.c
> +++ b/src/ch/ch_driver.c
> @@ -2387,6 +2387,46 @@ chDomainAttachDevice(virDomainPtr dom,
>  return chDomainAttachDeviceFlags(dom, xml, VIR_DOMAIN_AFFECT_LIVE);
>  }
>  
> +static int
> +chDomainDetachDeviceFlags(virDomainPtr dom,
> +  const char *xml,
> +  unsigned int flags)
> +{
> +virCHDriver *driver = dom->conn->privateData;
> +virDomainObj *vm = NULL;
> +int ret = -1;
> +
> +if (!(vm = virCHDomainObjFromDomain(dom)))
> +goto cleanup;
> +
> +if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
> +goto cleanup;
> +
> +if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0)
> +goto cleanup;
> +
> +if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
> +goto endjob;
> +
> +if (chDomainDetachDeviceLiveAndUpdateConfig(driver, vm, xml, flags) < 0)
> +goto endjob;
> +
> +ret = 0;
> +
> + endjob:
> +virDomainObjEndJob(vm);
> +
> + cleanup:
> +virDomainObjEndAPI(&vm);
> +return ret;
> +}
> +
> +static int chDomainDetachDevice(virDomainPtr dom, const char *xml)
> +{
> +return chDomainDetachDeviceFlags(dom, xml,
> + VIR_DOMAIN_AFFECT_LIVE);
> +}
> +
>  /* Function Tables */
>  static virHypervisorDriver chHypervisorDriver = {
>  .name = "CH",
> @@ -2450,6 +2490,8 @@ static virHypervisorDriver chHypervisorDriver = {
>  .domainInterfaceAddresses = chDomainInterfaceAddresses, /* 11.0.0 */
>  .domainAttachDevice = chDomainAttachDevice, /* 11.8.0 */
>  .domainAttachDeviceFlags = chDomainAttachDeviceFlags, /* 11.8.0 */
> +.domainDetachDevice = chDomainDetachDevice, /* 11.8.0 */
> +.domainDetachDeviceFlags = chDomainDetachDeviceFlags, /* 11.8.0 */
>  };
>  
>  static virConnectDriver chConnectDriver = {

Again, until here the patch is fine and what's below should be moved
into a separate patch.

> diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c
> index 524355b93c..95fe1f0f6f 100644
> --- a/src/ch/ch_hotplug.c
> +++ b/src/ch/ch_hotplug.c
> @@ -156,3 +156,178 @@ chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj 
> *vm,
>  
>  return 0;
>  }
> +
> +static int
> +chFindDiskId(virDomainDef *def, const char *dst)
> +{
> +size_t i;
> +
> +for (i = 0; i < def->ndisks; i++) {
> +if (STREQ(def->disks[i]->dst, dst))
> +return i;
> +}
> +
> +return -1;
> +}
> +
> +
> +/**
> + * chDomainFindDisk
> + *
> + * Helper function to find a disk device definition of a domain.
> + *
> + * Searches through the disk devices of a domain by comparing to 'match' and
> + * returns any match via the 'detach' out parameter.
> + */
> +static int
> +chDomainFindDisk(virDomainObj *vm,
> + virDomainDiskDef *match,
> + virDomainDiskDef **detach)
> +{
> +virDomainDiskDef *disk;
> +int idx;
> +
> +if ((idx = chFindDiskId(vm->def, match->dst)) < 0) {
> +virReportError(VIR_ERR_DEVICE_MISSING,
> +   _("disk %1$s not found"), match->dst);
> +return -1;
> +}
> +*detach = disk = vm->def->disks[idx];
> +
> +return 0;
> +}
> +
> +static int
> +chDomainDetachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *match)
> +{
> +virDomainDeviceDef detach = { .type = match->type };
> +virDomainDeviceInfo *info = NULL;
> +virCHDomainObjPrivate *priv = vm->privateData;
> +int idx = 0;
> +
> +switch (match->type) {
> +case VIR_DOMAIN_DEVICE_DISK:
> +if (chDomainFindDisk(vm, match->data.disk,
> + &detach.data.disk) < 0) {
> +return -1;
> +}
> +break;
> +case VIR_DOMAIN_DEVICE_LEASE:
> +case VIR_DOMAIN_DEVICE_FS:
> +case VIR_DOMAIN_DEVICE_NET:
> +case VIR_DOMAIN_DEVICE_INPUT:
> +case VIR_DOMAIN_DEVICE_SOUND:
> +case VIR_DOMAIN_DEVICE_VIDEO:
> +case VIR_DOMAIN_DEVICE_HOSTDEV:
> +case VIR_DOMAIN_DEVICE_WATCHDOG:
> +case VIR_DOMAIN_DEVICE_CONTROLLER:
> +case VIR_DOMAIN_DEVICE_GRAPHICS:
> +case VIR_DOMAIN_DEVICE_HUB:
> +case VIR_DOMAIN_DEVICE_REDIRDEV:
> +case VIR_DOMAIN_DEVICE_SMARTCARD:
> +case VIR_DOMAIN_DEVICE_CHR:
> +case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +case VIR_DOMAIN_DEVICE_NVRAM:
> +case VIR_DOMAIN_DEVICE_RNG:
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +case VIR_DOMAIN_DEVICE_TPM:
> +case VIR_DOMAIN_DEVICE_PANIC:
> +case VIR_DOMAIN_DEVICE_MEMORY:
> +case VIR_DOMAIN_DEVICE_IOMMU:
> +case VIR_DOMAIN_DEVICE

Re: [PATCH v3 07/13] ch: add disk attach helper functions

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  po/POTFILES |  1 +
>  src/ch/ch_hotplug.c | 85 +
>  2 files changed, 86 insertions(+)
> 
> diff --git a/po/POTFILES b/po/POTFILES
> index 181a36f541..23da794f84 100644
> --- a/po/POTFILES
> +++ b/po/POTFILES
> @@ -25,6 +25,7 @@ src/ch/ch_domain.c
>  src/ch/ch_driver.c
>  src/ch/ch_events.c
>  src/ch/ch_hostdev.c
> +src/ch/ch_hotplug.c
>  src/ch/ch_interface.c
>  src/ch/ch_monitor.c
>  src/ch/ch_process.c
> diff --git a/src/ch/ch_hotplug.c b/src/ch/ch_hotplug.c
> index c46628e7e9..7de688dc44 100644
> --- a/src/ch/ch_hotplug.c
> +++ b/src/ch/ch_hotplug.c
> @@ -18,10 +18,95 @@
>  
>  #include 
>  
> +#include "ch_alias.h"
> +#include "ch_domain.h"
>  #include "ch_hotplug.h"
>  

ch_hotplug.h should be included first as it may already include other
files. I mean, you correctly include ch_conf.h inside of ch_hotplug.h
because of the virCHDriver type. Again, not something that is critical
in this particular case, it's mostly good practice.

> +#include "domain_event.h"
> +#include "domain_validate.h"
> +#include "virlog.h"
> +
>  #define VIR_FROM_THIS VIR_FROM_CH
>  
> +VIR_LOG_INIT("ch.ch_hotplug");
> +
> +static int
> +chDomainAddDisk(virCHMonitor *mon, virDomainObj *vm, virDomainDiskDef *disk)
> +{
> +if (chAssignDeviceDiskAlias(disk) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +_("Assigning disk alias failed"));
> +return -1;
> +}
> +
> +if (virCHMonitorAddDisk(mon, disk) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +_("Adding disk to domain failed"));
> +return -1;
> +}
> +
> +virDomainDiskInsert(vm->def, disk);
> +
> +return 0;
> +}
> +
> +G_GNUC_UNUSED
> +static int
> +chDomainAttachDeviceLive(virDomainObj *vm,
> + virDomainDeviceDef *dev)
> +{
> +int ret = -1;
> +virCHDomainObjPrivate *priv = vm->privateData;
> +virCHMonitor *mon = priv->monitor;
> +
> +switch (dev->type) {
> +case VIR_DOMAIN_DEVICE_DISK: {
> +if (chDomainAddDisk(mon, vm, dev->data.disk) < 0) {
> +break;
> +}
> +
> +dev->data.disk = NULL;
> +ret = 0;
> +break;
> +}
> +case VIR_DOMAIN_DEVICE_NET:
> +case VIR_DOMAIN_DEVICE_LEASE:
> +case VIR_DOMAIN_DEVICE_FS:
> +case VIR_DOMAIN_DEVICE_INPUT:
> +case VIR_DOMAIN_DEVICE_HOSTDEV:
> +case VIR_DOMAIN_DEVICE_WATCHDOG:
> +case VIR_DOMAIN_DEVICE_CONTROLLER:
> +case VIR_DOMAIN_DEVICE_REDIRDEV:
> +case VIR_DOMAIN_DEVICE_CHR:
> +case VIR_DOMAIN_DEVICE_RNG:
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +case VIR_DOMAIN_DEVICE_MEMORY:
> +case VIR_DOMAIN_DEVICE_VSOCK:
> +case VIR_DOMAIN_DEVICE_NONE:
> +case VIR_DOMAIN_DEVICE_SOUND:
> +case VIR_DOMAIN_DEVICE_VIDEO:
> +case VIR_DOMAIN_DEVICE_GRAPHICS:
> +case VIR_DOMAIN_DEVICE_HUB:
> +case VIR_DOMAIN_DEVICE_SMARTCARD:
> +case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +case VIR_DOMAIN_DEVICE_NVRAM:
> +case VIR_DOMAIN_DEVICE_TPM:
> +case VIR_DOMAIN_DEVICE_PANIC:
> +case VIR_DOMAIN_DEVICE_IOMMU:
> +case VIR_DOMAIN_DEVICE_AUDIO:
> +case VIR_DOMAIN_DEVICE_CRYPTO:
> +case VIR_DOMAIN_DEVICE_PSTORE:
> +case VIR_DOMAIN_DEVICE_LAST:
> +default:
> +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +   _("live attach of device '%1$s' is not supported"),
> +   virDomainDeviceTypeToString(dev->type));
> +break;
> +}
> +
> +return ret;
> +}
> +
>  int
>  chDomainAttachDeviceLiveAndUpdateConfig(virDomainObj *vm G_GNUC_UNUSED,
>  virCHDriver *driver G_GNUC_UNUSED,


This is great place to implement this function ^^^. IOW, just move
corresponding change from the next patch here. Then you don't need to
annotate a function as unused.

Michal



Re: [PATCH v3 04/13] ch: refactor virCHMonitorBuildDiskJson

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> Refactor BuildDiskJson to return a virJSONValue instead of adding the
> disk json to an json array. This makes the function reusable for
> hotplugging disks.
> 
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_monitor.c | 29 +++--
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index d369236183..f65cca648b 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -234,43 +234,41 @@ virCHMonitorBuildMemoryJson(virJSONValue *content, 
> virDomainDef *vmdef)
>  return 0;
>  }
>  
> -static int
> -virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef)
> +static virJSONValue*
> +virCHMonitorBuildDiskJson(virDomainDiskDef *diskdef)
>  {
>  g_autoptr(virJSONValue) disk = virJSONValueNewObject();
>  
>  if (!diskdef->src)
> -return -1;
> +return NULL;
>  
>  switch (diskdef->src->type) {
>  case VIR_STORAGE_TYPE_FILE:
>  if (!diskdef->src->path) {
>  virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("Missing disk file path in domain"));
> -return -1;
> +return NULL;
>  }
>  if (!diskdef->info.alias) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Missing disk alias"));
> -return -1;
> +return NULL;
>  }
>  if (diskdef->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("Only virtio bus types are supported for 
> '%1$s'"),
> diskdef->src->path);
> -return -1;
> +return NULL;
>  }
>  if (virJSONValueObjectAppendString(disk, "path", diskdef->src->path) 
> < 0)
> -return -1;
> +return NULL;
>  if (diskdef->src->readonly) {
>  if (virJSONValueObjectAppendBoolean(disk, "readonly", true) < 0)
> -return -1;
> +return NULL;
>  }
>  if (virJSONValueObjectAppendString(disk, "id", diskdef->info.alias) 
> < 0) {
> -return -1;
> +return NULL;
>  }
> -if (virJSONValueArrayAppend(disks, &disk) < 0)
> -return -1;
>  
>  break;
>  case VIR_STORAGE_TYPE_NONE:
> @@ -284,23 +282,26 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, 
> virDomainDiskDef *diskdef)
>  case VIR_STORAGE_TYPE_LAST:
>  default:
>  virReportEnumRangeError(virStorageType, diskdef->src->type);
> -return -1;
> +return NULL;
>  }
>  
> -return 0;
> +return g_steal_pointer(&disk);
>  }
>  
>  static int
>  virCHMonitorBuildDisksJson(virJSONValue *content, virDomainDef *vmdef)
>  {
>  g_autoptr(virJSONValue) disks = NULL;
> +g_autoptr(virJSONValue) disk = NULL;

This variable ...

>  size_t i;
>  
>  if (vmdef->ndisks > 0) {
>  disks = virJSONValueNewArray();
>  
>  for (i = 0; i < vmdef->ndisks; i++) {

... is used exclusively in this loop. Declaring it here not only
improves readability (I don't have to hold yet another variable inside
my head when reading this function) but also avoids unexpected reuse of
the variable.

In fact, this whole function could be written a bit better. One level of
nesting could be dropped, but that's outside of the scope of this
particular patch.

> -if (virCHMonitorBuildDiskJson(disks, vmdef->disks[i]) < 0)
> +if ((disk = virCHMonitorBuildDiskJson(vmdef->disks[i])) == NULL)
> +return -1;
> +if (virJSONValueArrayAppend(disks, &disk) < 0)
>  return -1;
>  }
>  if (virJSONValueObjectAppend(content, "disks", &disks) < 0)

Michal



Re: [PATCH v3 05/13] ch: add/use virCHMonitorPut function

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> This allows users to call API endpoints that require passing data in a
> generic way. Previously, only virCHMonitorPutNoContent was offered.
> 
> On-behalf-of: SAP stefan.ko...@sap.com
> Signed-off-by: Stefan Kober 
> ---
>  src/ch/ch_monitor.c | 49 ++---
>  1 file changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
> index f65cca648b..2ebeb46ad4 100644
> --- a/src/ch/ch_monitor.c
> +++ b/src/ch/ch_monitor.c
> @@ -62,6 +62,10 @@ VIR_ONCE_GLOBAL_INIT(virCHMonitor);
>  int virCHMonitorShutdownVMM(virCHMonitor *mon);
>  int virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint,
>   domainLogContext *logCtxt);
> +int
> +virCHMonitorPut(virCHMonitor *mon, const char *endpoint,
> +virJSONValue *payload, domainLogContext *logCtxt,
> +virJSONValue** answer);

This function should be static. It's not exposed outside of this source
file.

>  
>  static int
>  virCHMonitorBuildCPUJson(virJSONValue *content, virDomainDef *vmdef)
> @@ -868,11 +872,15 @@ curl_callback(void *contents, size_t size, size_t 
> nmemb, void *userp)
>  }
>  
>  int
> -virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint,
> - domainLogContext *logCtxt)
> +virCHMonitorPut(virCHMonitor *mon,
> +const char *endpoint,
> +virJSONValue *payload,
> +domainLogContext *logCtxt,
> +virJSONValue **answer)
>  {
>  VIR_LOCK_GUARD lock = virObjectLockGuard(mon);
>  g_autofree char *url = NULL;
> +g_autofree char *payload_str = NULL;
>  int responseCode = 0;
>  int ret = -1;
>  struct curl_data data = {0};
> @@ -890,28 +898,55 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char 
> *endpoint,
>  curl_easy_setopt(mon->handle, CURLOPT_INFILESIZE, 0L);
>  
>  headers = curl_slist_append(headers, "Accept: application/json");
> +
>  curl_easy_setopt(mon->handle, CURLOPT_HTTPHEADER, headers);
>  curl_easy_setopt(mon->handle, CURLOPT_WRITEFUNCTION, curl_callback);
>  curl_easy_setopt(mon->handle, CURLOPT_WRITEDATA, (void *)&data);
>  
> +if (payload) {
> +payload_str = virJSONValueToString(payload, false);
> +curl_easy_setopt(mon->handle, CURLOPT_POSTFIELDS, payload_str);
> +curl_easy_setopt(mon->handle, CURLOPT_CUSTOMREQUEST, "PUT");
> +headers = curl_slist_append(headers, "Content-Type: 
> application/json");
> +}
> +
>  responseCode = virCHMonitorCurlPerform(mon->handle);
>  
> +data.content = g_realloc(data.content, data.size + 1);
> +data.content[data.size] = 0;

Even though I don't know of any architecture, where '\0' byte would be
different to zero we somehow prefer the former. Mostly, because it shows
we're working with characters.

> +
>  if (logCtxt && data.size) {
>  /* Do this to append a NULL char at the end of data */
> -data.content = g_realloc(data.content, data.size + 1);
> -data.content[data.size] = 0;
>  domainLogContextWrite(logCtxt, "HTTP response code from CH: %d\n", 
> responseCode);
>  domainLogContextWrite(logCtxt, "Response = %s\n", data.content);
>  }
>  
> -if (responseCode == 200 || responseCode == 204)
> -ret = 0;
> +if (responseCode != 200 && responseCode != 204) {
> +ret = -1;
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Invalid HTTP response code from CH: %1$d"),
> +   responseCode);
> +goto cleanup;
> +}
>  
> -curl_slist_free_all(headers);
> +if (answer)
> +*answer = virJSONValueFromString(data.content);
> +
> +ret = 0;
>  
> + cleanup:
> +curl_slist_free_all(headers);
> +g_free(data.content);
>  return ret;
>  }
>  
> +int
> +virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint,
> + domainLogContext *logCtxt)
> +{
> +return virCHMonitorPut(mon, endpoint, NULL, logCtxt, NULL);
> +}
> +
>  static int
>  virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue 
> **response)
>  {

Michal



Re: [PATCH v3 00/13] CH: Add disk hotplug support to Cloud Hypervisor domains

2025-09-08 Thread Michal Prívozník via Devel
On 9/4/25 14:10, Stefan Kober wrote:
> This patchset adds the necessary functionality to support disk hotplugging in 
> the CH driver.
> 
> Some alias handling that can be extended to other device types is added, in 
> order to allow detaching the devices via the Cloud Hypervisor API.
> 
> Stefan Kober (13):
>   ch: add ch_hotplug.{h,c} files to CH build
>   ch: pass disk alias to CHV
>   ch: add ch_alias.{c,h} for device alias handling
>   ch: refactor virCHMonitorBuildDiskJson
>   ch: add/use virCHMonitorPut function
>   ch: add monitor disk attach logic
>   ch: add disk attach helper functions
>   ch: implement disk attach in public API
>   ch: assign aliases in ProcessPrepareDomain
>   ch: add virCHMonitorBuildKeyValueJson
>   ch: add virCHMonitorRemoveDevice function
>   ch: implement disk device detach in public API
>   NEWS: announce disk hotplug support for ch
> 
>  NEWS.rst|   5 +
>  po/POTFILES |   2 +
>  src/ch/ch_alias.c   |  62 +
>  src/ch/ch_alias.h   |  27 
>  src/ch/ch_driver.c  |  86 
>  src/ch/ch_hotplug.c | 333 
>  src/ch/ch_hotplug.h |  33 +
>  src/ch/ch_monitor.c | 131 ++---
>  src/ch/ch_monitor.h |   7 +
>  src/ch/ch_process.c |   4 +
>  src/ch/meson.build  |   4 +
>  11 files changed, 671 insertions(+), 23 deletions(-)
>  create mode 100644 src/ch/ch_alias.c
>  create mode 100644 src/ch/ch_alias.h
>  create mode 100644 src/ch/ch_hotplug.c
>  create mode 100644 src/ch/ch_hotplug.h
> 

This looks way better than previous version. Thank you for working in my
suggestions. There are still some small problems, but I'll fix them
before merging.

Also, I'm moving some hunks between some patches AND I'm reordering some
patches too. For instance, patch 02/13 introduces a temporary regression
as it requires aliases to be assigned to disks, but that doesn't happen
until patch 09/13. If these are reordered then no regression occurs.

Anyway, good job! What should be done in a follow up series is to emit
events on device attach/detach. Management apps will certainly value that.

Reviewed-by: Michal Privoznik 

and merged.

Michal



Re: [PATCH 06/14] qemu: Parse properties list from any JSON array

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:54 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The qemuMonitorJSONParsePropsList API expected a QMP reply as an input.
> By generalizing it to work on any JSON array, we can reuse the API even
> for commands which return the array of properties nested in an object.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 26 ++
>  1 file changed, 14 insertions(+), 12 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 09/14] qemu: Add qemuMonitorJSONCPUDataAddFeatures helper

2025-09-08 Thread Peter Krempa via Devel
On Thu, Sep 04, 2025 at 16:46:57 +0200, Jiri Denemark via Devel wrote:
> From: Jiri Denemark 
> 
> The function translates a list of CPU feature names retrieved from QEMU
> and adds them to virCPUData.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_monitor_json.c | 43 
>  1 file changed, 24 insertions(+), 19 deletions(-)

Reviewed-by: Peter Krempa 



Re: [PATCH 0/3] A couple improvements when auto-adding PCI controllers

2025-09-08 Thread Ján Tomko via Devel

On a Wednesday in 2025, Laine Stump via Devel wrote:

The 1st patch fixes a tiny omission from a feature added several years
ago, which slightly improves the error message when you attempt to add
a pcie-to-pci-bridge controller to a domain that has no PCIe bus. The
2nd improves that error message a bit more, along with improving some
comments in the code.

The 3rd eliminates said error message completely when you're adding a
pcie-to-pci-bridge controller to a domain that *does* support PCIe,
but doesn't currently have an open pcie-root-port to plug in the
pcie-to-pci-bridge (by auto-adding another pcie-root-port).

The impetus for these was https://issues.redhat.com/browse/RHEL-62032

Laine Stump (3):
 conf: add forgotten clause to
   virDomainPCIControllerConectTypeToModel()
 conf: improve error message when a PCI controller can't be auto-added
 conf: auto-add a pcie-root-port when needed while plugging in
   pcie-to-pci-bridge

src/conf/domain_addr.c| 43 +++
.../pcie-root-port-too-many.x86_64-latest.err |  2 +-
2 files changed, 35 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature


Re: [PATCH 04/14] qemu: Generalize filtering in qemuMonitorJSONParsePropsList

2025-09-08 Thread Ján Tomko via Devel

On a Thursday in 2025, Jiri Denemark via Devel wrote:

From: Jiri Denemark 

qemuMonitorJSONParsePropsList supported filtering based on type. Let's
replace it with a callback supplied by the caller to allow for more
advanced filtering.

Signed-off-by: Jiri Denemark 
---
src/qemu/qemu_monitor_json.c | 51 +---
1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 6402d18d37..0c78115e9d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5837,10 +5837,21 @@ int qemuMonitorJSONSetObjectProperty(qemuMonitor *mon,
#undef MAKE_SET_CMD


+/* A filter callback for qemuMonitorJSONParsePropsList.
+ *
+ * Returns 0 if the property should be included in the list,
+ * 1 if the property should be ignored,


I would expect these two to be switched, but could not find a filtering
function that can also return an error

(The CheckACL ones return bool)

Jano


+ *-1 on error.
+ */


signature.asc
Description: PGP signature


Re: [PATCH 5/8] qemu: Generate acpi-generic-initiator command from acpi nodeset

2025-09-08 Thread Daniel P . Berrangé via Devel
On Mon, Sep 08, 2025 at 07:15:18PM +0200, Andrea Righi wrote:
> Hi Daniel,
> 
> On Mon, Sep 08, 2025 at 06:02:20PM +0100, Daniel P. Berrangé wrote:
> > On Sat, Sep 06, 2025 at 03:09:00PM +0200, Andrea Righi wrote:
> > > Signed-off-by: Andrea Righi 
> > > ---
> > >  src/qemu/qemu_command.c | 45 +
> > >  1 file changed, 45 insertions(+)
> > 
> > Reviewed-by: Daniel P. Berrangé 
> > 
> > > 
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 3f9b583985..9ca0847789 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -5222,6 +5222,47 @@ qemuBuildHostdevSCSICommandLine(virCommand *cmd,
> > >  }
> > >  
> > >  
> > > +static int
> > > +qemuBuildAcpiNodesetProps(virCommand *cmd,
> > > +  virDomainDeviceInfo *info,
> > > +  virQEMUCaps *qemuCaps)
> > > +{
> > > +static unsigned int giIndex;
> > > +int node = -1;
> > > +
> > > +if (!info->acpiNodeset)
> > > +return 0;
> > > +
> > > +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACPI_GENERIC_INITIATOR))
> > > +return -1;
> > 
> > We can assume the validate function already ran, so we don't
> > need this check here, which is good as this would return an
> > error status without setting an error message.
> 
> Ah yes, this check is redundant, we can definitely drop it.
> Should I send an update patch just with this change?

Don't bother. The rest of the series is fine, so I'll make the obvious
change and push this once I've validated it in CI.


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



Re: [PATCH 7/8] docs: Document acpi nodeset in hostdev

2025-09-08 Thread Daniel P . Berrangé via Devel
On Sat, Sep 06, 2025 at 03:09:02PM +0200, Andrea Righi wrote:
> Add documentation for the new  element in hostdev,
> which allows associating devices with ACPI Generic Initiator objects in
> QEMU.
> 
> A typical use case is NVIDIA Multi-Instance GPU (MIG), where a physical
> GPU is partitioned into multiple isolated instances, each tied to one or
> more virtual NUMA nodes. The documentation includes an example showing
> how to configure  cells together with a MIG device.
> 
> Signed-off-by: Andrea Righi 
> ---
>  docs/formatdomain.rst | 49 +++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 9f7311b6d5..24f7cdd018 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -4894,6 +4894,55 @@ or:
> host device. :since:`Since 1.0.6`, but only works as expected
> :since:`since 1.2.2`.
>  
> +ACPI Generic Initiators
> +
> +
> +A host device may include an  element to create ACPI Generic
> +Initiator objects for the device in QEMU.
> +
> +This can be used for **NVIDIA Multi-Instance GPU (MIG)** configurations,
> +where a physical GPU is partitioned into multiple isolated instances, each
> +associated with one or more virtual NUMA nodes.
> +
> +By attaching an  element to the MIG device in the
> +domain XML, the guest will configure the correct partitioning for that
> +instance.
> +
> +.. code-block:: xml

We can't use code formatting in CI, so I've changed this to a
plain pre-formatted text block like the rest of the doc

> +
> +   
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> +   
> +   ...
> +   
> + 
> +   
> + 
> + 
> +  +  slot='0x02' function='0x0'/>
> +   
> +
> +Attributes of :
> +
> +``nodeset``
> +   A list of NUMA node IDs that will be associated with the device.
> +   Each node in the set causes libvirt to create an
> +   ``acpi-generic-initiator`` object in QEMU, tied to this device.
> +
> +   The value uses the standard libvirt *nodeset* syntax (e.g. ``0-3,5``).
> +
> +If the  element is omitted, no acpi-generic-initiator objects are
> +created for the device.
> +
>  Block / character devices
>  ^
>  
> -- 
> 2.51.0
> 

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



[PATCH] esx: Satisfy curl type checks

2025-09-08 Thread Michal Privoznik via Devel
From: Michal Privoznik 

Since its commit [1] curl now performs type checks on
curl_easy_setopt(). Some options expect long but we're passing an
int. The fix consists mostly of specifying type of numbers passed
to the function. Except for two cases: proxy_type and proxy_port
which are stored in a structure of ours (_esxUtil_ParsedUri).
These have their types switched from int to long and unsigned
long, respectively.

1: https://github.com/curl/curl/commit/79b4e56b3f30dc1ac28a81128a07d27338e5219e
Signed-off-by: Michal Privoznik 
---
 src/esx/esx_stream.c |  2 +-
 src/esx/esx_util.c   |  4 ++--
 src/esx/esx_util.h   |  4 ++--
 src/esx/esx_vi.c | 18 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/esx/esx_stream.c b/src/esx/esx_stream.c
index 1439940330..4f3ca09379 100644
--- a/src/esx/esx_stream.c
+++ b/src/esx/esx_stream.c
@@ -411,7 +411,7 @@ esxStreamOpen(virStreamPtr stream, esxPrivate *priv, const 
char *url,
 curl_easy_setopt(streamPriv->curl->handle, CURLOPT_READDATA, 
streamPriv);
 } else {
 curl_easy_setopt(streamPriv->curl->handle, CURLOPT_UPLOAD, 0);
-curl_easy_setopt(streamPriv->curl->handle, CURLOPT_HTTPGET, 1);
+curl_easy_setopt(streamPriv->curl->handle, CURLOPT_HTTPGET, 1L);
 curl_easy_setopt(streamPriv->curl->handle, CURLOPT_WRITEFUNCTION,
  esxVI_CURL_WriteStream);
 curl_easy_setopt(streamPriv->curl->handle, CURLOPT_WRITEDATA, 
streamPriv);
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 7ee0e5f7c0..8e725ed04f 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -125,8 +125,8 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURI *uri)
 
 *tmp++ = '\0';
 
-if (virStrToLong_i(tmp, NULL, 10,
-   &(*parsedUri)->proxy_port) < 0 ||
+if (virStrToLong_ul(tmp, NULL, 10,
+&(*parsedUri)->proxy_port) < 0 ||
 (*parsedUri)->proxy_port < 1 ||
 (*parsedUri)->proxy_port > 65535) {
 virReportError(VIR_ERR_INVALID_ARG,
diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h
index 58bc44e744..51f1151eb7 100644
--- a/src/esx/esx_util.h
+++ b/src/esx/esx_util.h
@@ -40,9 +40,9 @@ struct _esxUtil_ParsedUri {
 bool noVerify;
 bool autoAnswer;
 bool proxy;
-int proxy_type;
+long proxy_type;
 char *proxy_hostname;
-int proxy_port;
+unsigned long proxy_port;
 char *path;
 char *cacert;
 };
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index d25f819bc5..65e19e179e 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -315,13 +315,13 @@ esxVI_CURL_Connect(esxVI_CURL *curl, esxUtil_ParsedUri 
*parsedUri)
 }
 
 curl_easy_setopt(curl->handle, CURLOPT_USERAGENT, "libvirt-esx");
-curl_easy_setopt(curl->handle, CURLOPT_NOSIGNAL, 1);
-curl_easy_setopt(curl->handle, CURLOPT_HEADER, 0);
-curl_easy_setopt(curl->handle, CURLOPT_FOLLOWLOCATION, 0);
+curl_easy_setopt(curl->handle, CURLOPT_NOSIGNAL, 1L);
+curl_easy_setopt(curl->handle, CURLOPT_HEADER, 0L);
+curl_easy_setopt(curl->handle, CURLOPT_FOLLOWLOCATION, 0L);
 curl_easy_setopt(curl->handle, CURLOPT_SSL_VERIFYPEER,
- parsedUri->noVerify ? 0 : 1);
+ parsedUri->noVerify ? 0L : 1L);
 curl_easy_setopt(curl->handle, CURLOPT_SSL_VERIFYHOST,
- parsedUri->noVerify ? 0 : 2);
+ parsedUri->noVerify ? 0L : 2L);
 curl_easy_setopt(curl->handle, CURLOPT_COOKIEFILE, "");
 curl_easy_setopt(curl->handle, CURLOPT_HTTPHEADER, curl->headers);
 curl_easy_setopt(curl->handle, CURLOPT_READFUNCTION,
@@ -386,8 +386,8 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, char 
**content,
 curl_easy_setopt(curl->handle, CURLOPT_URL, url);
 curl_easy_setopt(curl->handle, CURLOPT_RANGE, range);
 curl_easy_setopt(curl->handle, CURLOPT_WRITEDATA, &buffer);
-curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 0);
-curl_easy_setopt(curl->handle, CURLOPT_HTTPGET, 1);
+curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 0L);
+curl_easy_setopt(curl->handle, CURLOPT_HTTPGET, 1L);
 
 responseCode = esxVI_CURL_Perform(curl, url);
 }
@@ -426,7 +426,7 @@ esxVI_CURL_Upload(esxVI_CURL *curl, const char *url, const 
char *content)
 curl_easy_setopt(curl->handle, CURLOPT_URL, url);
 curl_easy_setopt(curl->handle, CURLOPT_RANGE, NULL);
 curl_easy_setopt(curl->handle, CURLOPT_READDATA, &content);
-curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 1);
+curl_easy_setopt(curl->handle, CURLOPT_UPLOAD, 1L);
 curl_easy_setopt(curl->handle, CURLOPT_INFILESIZE, strlen(content));
 
 responseCode = esxVI_CURL_Perform(curl, url);
@@ -1223,7 +1223,7 @@ esxVI_Context_Execute(esxVI_Context *ctx, const char 
*methodName,
 curl_easy_setopt

Re: [PATCH 3/8] conf: Add nodeset attribute to the element

2025-09-08 Thread Daniel P . Berrangé via Devel
On Sat, Sep 06, 2025 at 03:08:58PM +0200, Andrea Righi wrote:
> This enables partitioning of PCI devices into multiple isolated
> instances, each requiring a dedicated virtual NUMA node definition.
> 
> Link: https://mail.gnu.org/archive/html/qemu-arm/2024-03/msg00358.html
> Signed-off-by: Andrea Righi 
> ---
>  src/conf/device_conf.h|  3 +++
>  src/conf/domain_conf.c| 30 --
>  src/conf/schemas/domaincommon.rng |  5 +
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 2d97410f6e..e570f51824 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -185,6 +185,9 @@ struct _virDomainDeviceInfo {
>   * cases we might want to prevent that from happening by
>   * locking the isolation group */
>  bool isolationGroupLocked;
> +
> +/* NUMA nodeset affinity for this device */
> +virBitmap *acpiNodeset;
>  };

This needed a virBitmapFree added, so I've fixed that too.


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



Re: [PATCH] qemu: block: Always enable discard forwarding for 'throttle' filter layer

2025-09-08 Thread Michal Prívozník via Devel
On 9/5/25 10:13, Peter Krempa via Devel wrote:
> From: Peter Krempa 
> 
> Discards ought to be forwarded to the protocol nodes where we control
> if discard actually happens.
> 
> Unconditionally enable discard='unmap' for the intermediate layer.
> 
> Closes: https://gitlab.com/libvirt/libvirt/-/issues/810
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_block.c  |  1 +
>  .../qemuxmlconfdata/throttlefilter.x86_64-latest.args  | 10 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 

Reviewed-by: Michal Privoznik 
Michal



Re: [PATCH 6/8] qemu: Add acpi-generic-initiator unit test

2025-09-08 Thread Daniel P . Berrangé via Devel
On Sat, Sep 06, 2025 at 03:09:01PM +0200, Andrea Righi wrote:
> Signed-off-by: Andrea Righi 
> ---
>  .../acpi-generic-initiator.x86_64-latest.args | 55 
>  .../acpi-generic-initiator.x86_64-latest.xml  | 63 +++
>  .../acpi-generic-initiator.xml| 63 +++
>  tests/qemuxmlconftest.c   |  1 +
>  4 files changed, 182 insertions(+)
>  create mode 100644 
> tests/qemuxmlconfdata/acpi-generic-initiator.x86_64-latest.args
>  create mode 100644 
> tests/qemuxmlconfdata/acpi-generic-initiator.x86_64-latest.xml
>  create mode 100644 tests/qemuxmlconfdata/acpi-generic-initiator.xml

Reviewed-by: Daniel P. Berrangé 


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



Re: [PATCH v2 00/10] qemu: Fixes to firmware selection

2025-09-08 Thread Jim Fehlig via Devel

Hi Andrea,

Is there anything I can do to help move this series forward? I think 8/10 can 
now be considered for merging, correct?


BTW, I'm fine with the patch order, based on your rational in 4/10

https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/PZEMOM474JUHWIIX3SOUITBHHTDBIVDN/

So my previous comments on patches 4 and 5 aren't really valid. Should I 
re-review those? Were you planning to spin another version which includes patch 8?


Regards,
Jim

On 8/25/25 10:19, Andrea Bolognani via Devel wrote:

Changes from [v1]:

   * pick up Jim's test suite improvements;
   * squash in fixes for issues found during review;
   * add a few commits intented to spark further discussion around
 what the firmware descriptors should look like in the edk2
 package.

[v1] 
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/RKQ3ANKDNJEFJSKQR4FMDU7PVHWYKYSF/

Andrea Bolognani (9):
   tests: Tweak descriptor for combined firmware
   tests: Minimize SEV tests
   tests: Add tests for SEV firmware selection
   qemu: Fix matching for stateless/combined firmware
   qemu: Fix matching for read/write firmware
   news: Update for firmware selection fixes
   DONOTMERGE update firmware data
   DONOTMERGE remove SEV features from non-SEV descriptors
   DONOTMERGE don't explicitly request stateless firmware for SEV

Jim Fehlig (1):
   tests: Improve AMD SEV-related tests

  NEWS.rst  |  5 ++
  src/qemu/qemu_firmware.c  | 47 ++-
  .../firmware/60-edk2-ovmf-x64-amdsev.json |  1 -
  .../50-edk2-ovmf-4m-qcow2-x64-nosb.json   |  2 -
  .../51-edk2-ovmf-2m-raw-x64-nosb.json |  2 -
  .../firmware/60-edk2-ovmf-x64-amdsev.json |  3 +-
  .../firmware/60-edk2-ovmf-x64-amdsevsnp.json} | 14 +++---
  .../usr/share/qemu/firmware/90-combined.json  |  5 +-
  tests/qemufirmwaretest.c  |  4 +-
  ...ware-auto-efi-rw-pflash.x86_64-latest.args | 36 ++
  ...mware-auto-efi-rw-pflash.x86_64-latest.err |  1 -
  ...mware-auto-efi-rw-pflash.x86_64-latest.xml |  6 ++-
  .../firmware-auto-efi-rw.x86_64-latest.args   | 36 ++
  .../firmware-auto-efi-rw.x86_64-latest.err|  1 -
  .../firmware-auto-efi-rw.x86_64-latest.xml|  6 ++-
  ...auto-efi-sev-snp.x86_64-latest+amdsev.args | 35 ++
  ...auto-efi-sev-snp.x86_64-latest+amdsev.xml} |  9 +++-
  .../firmware-auto-efi-sev-snp.xml | 20 
  ...are-auto-efi-sev.x86_64-latest+amdsev.args | 36 ++
  ...are-auto-efi-sev.x86_64-latest+amdsev.xml} |  9 +++-
  .../qemuxmlconfdata/firmware-auto-efi-sev.xml | 20 
  ...urity-sev-direct.x86_64-latest+amdsev.args |  7 ++-
  ...curity-sev-direct.x86_64-latest+amdsev.xml | 19 +++-
  ...nch-security-sev-direct.x86_64-latest.args |  7 ++-
  ...unch-security-sev-direct.x86_64-latest.xml | 19 +++-
  .../launch-security-sev-direct.xml| 19 +---
  ...ng-platform-info.x86_64-latest+amdsev.args |  9 ++--
  ...ing-platform-info.x86_64-latest+amdsev.xml | 29 ++--
  ...nch-security-sev-missing-platform-info.xml | 25 +++---
  ...security-sev-snp.x86_64-latest+amdsev.args | 11 +
  ...-security-sev-snp.x86_64-latest+amdsev.xml | 29 +---
  ...launch-security-sev-snp.x86_64-latest.args | 11 +
  .../launch-security-sev-snp.x86_64-latest.xml | 29 +---
  .../launch-security-sev-snp.xml   | 45 +-
  ...nch-security-sev.x86_64-latest+amdsev.args |  9 ++--
  ...unch-security-sev.x86_64-latest+amdsev.xml | 29 ++--
  tests/qemuxmlconfdata/launch-security-sev.xml | 25 +++---
  tests/qemuxmlconftest.c   | 11 -
  38 files changed, 352 insertions(+), 279 deletions(-)
  copy 
tests/qemufirmwaredata/{out/usr/share/qemu/firmware/60-edk2-ovmf-x64-amdsev.json 
=> usr/share/qemu/firmware/60-edk2-ovmf-x64-amdsevsnp.json} (57%)
  create mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.args
  delete mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-rw-pflash.x86_64-latest.err
  create mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.args
  delete mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-rw.x86_64-latest.err
  create mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.x86_64-latest+amdsev.args
  copy tests/qemuxmlconfdata/{firmware-auto-efi-rw-pflash.x86_64-latest.xml => 
firmware-auto-efi-sev-snp.x86_64-latest+amdsev.xml} (78%)
  create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev-snp.xml
  create mode 100644 
tests/qemuxmlconfdata/firmware-auto-efi-sev.x86_64-latest+amdsev.args
  copy tests/qemuxmlconfdata/{firmware-auto-efi-rw-pflash.x86_64-latest.xml => 
firmware-auto-efi-sev.x86_64-latest+amdsev.xml} (77%)
  create mode 100644 tests/qemuxmlconfdata/firmware-auto-efi-sev.xml