Re: [PATCH 04/14] qemu: Generalize filtering in qemuMonitorJSONParsePropsList
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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