[PATCH Libvirt v2 2/3] qemu_command: Generate cmd line for discard and write-zeroes properties

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

Generate cmd line for virtio-blk.discard and virtio-blk.write-zeroes
properties. Also, validate that the requested feature is supported by QEMU.

Signed-off-by: Hyman Huang(黄勇) 
---
 src/qemu/qemu_command.c   |  2 +
 src/qemu/qemu_validate.c  |  9 +++
 .../disk-virtio-discard.x86_64-latest.args| 45 ++
 .../qemuxml2argvdata/disk-virtio-discard.xml  | 45 ++
 tests/qemuxml2argvtest.c  |  1 +
 .../disk-virtio-discard.x86_64-latest.xml | 58 +++
 tests/qemuxml2xmltest.c   |  1 +
 7 files changed, 161 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml
 create mode 100644 
tests/qemuxml2xmloutdata/disk-virtio-discard.x86_64-latest.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..6954fc5d75 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1834,6 +1834,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
   "T:scsi", scsi,
   "p:num-queues", disk->queues,
   "p:queue-size", disk->queue_size,
+  "T:discard", disk->virtio_discard,
+  "T:write-zeroes", disk->virtio_write_zeroes,
   NULL) < 0)
 return NULL;
 }
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..499b49ce74 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2949,6 +2949,15 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
virDomainDiskDef *disk,
 }
 }
 
+if (disk->virtio_discard || disk->virtio_write_zeroes) {
+if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("virtio_discard and virtio_write_zeroes options 
are "
+ "only valid for VIRTIO bus"));
+return -1;
+}
+}
+
 switch (disk->bus) {
 case VIR_DOMAIN_DISK_BUS_SCSI:
 if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) {
diff --git a/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
new file mode 100644
index 00..ee7a7edc9c
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
@@ -0,0 +1,45 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
 \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-accel tcg \
+-cpu qemu64 \
+-m size=219136k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-device 
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-blockdev 
'{"driver":"file","filename":"/tmp/data.img","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-4-format","read-only":false,"driver":"raw","file":"libvirt-4-storage"}'
 \
+-device 
'{"driver":"virtio-blk-pci","discard":true,"write-zeroes":true,"bus":"pci.0","addr":"0x2","drive":"libvirt-4-format","id":"virtio-disk0","bootindex":1}'
 \
+-blockdev 
'{"driver":"file","filename":"/tmp/data1.img","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-3-format","read-only":false,"driver":"raw","file":"libvirt-3-storage"}'
 \
+-device 
'{"driver":"virtio-blk-pci","discard":true,"write-zeroes":false,"bus":"pci.0","addr":"0x3","drive":"libvirt-3-format","id":"virtio-disk1"}'
 \
+-blockdev 
'{"driver":"file","filename":"/tmp/data2.img","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-2-format","read-only":false,"driver":"raw","file":"libvirt-2-storage"}'
 \
+-device 
'{"driver":"virtio-blk-pci","discard":false,"write-zeroes":true,"bus":"pci.0","addr":"0x4","drive":"libvirt-2-format","id":"virtio-disk2"}'
 \
+-blockdev 

[PATCH Libvirt v2 0/3] support discard and write-zeroes options for virtio-blk device

2023-07-17 Thread ~hyman
v2:
This version made some modifications. (include suggestion of Jonathon)
- rebase on master
- assume QEMU support discard and write-zeroes and drop the capability
introduction and probe
- add validation for options, only allowing attributes be configured for
VIRTIO bus
- enrich the test case
- update NEWS

Thanks Jonathon for pointing out the redundant commit in time.
Please review!

Yong

DISCARD and WRITE_ZEROES commands has been implemented in virtio-blk
protocol since qemu >= 4.2.0, may be it's time to introduce discard and
write-zeroes options for virtio-blk device in libvirt so that the upper
layer can enable this feature at disk granularity.
To distinguish the discard option in block drive layer, use the 'virtio'
prefix to indicate that these attributes are specific for virtio-blk
disk.
To try this out, three things has done in this patchset:
1. introduce capabilities of discard and write-zeroes for virtio-blk
2. add virtio_discard and virtio_write_zeroes attributes of driver in
disk xml element
3. generate cmd line when launching vm

Hyman Huang(黄勇) (3):
  conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes
  qemu_command: Generate cmd line for discard and write-zeroes
properties
  NEWS: Mention support for discard and write_zeroes of virtio-blk
device

 NEWS.rst  |  8 +++
 docs/formatdomain.rst |  8 +++
 src/conf/domain_conf.c| 16 +
 src/conf/domain_conf.h|  2 +
 src/conf/schemas/domaincommon.rng | 10 
 src/conf/storage_source_conf.c|  2 +
 src/conf/storage_source_conf.h|  2 +
 src/qemu/qemu_command.c   |  2 +
 src/qemu/qemu_domain.c|  2 +
 src/qemu/qemu_driver.c|  4 +-
 src/qemu/qemu_validate.c  |  9 +++
 src/vz/vz_utils.c | 12 
 .../disk-virtio-discard.x86_64-latest.args| 45 ++
 .../qemuxml2argvdata/disk-virtio-discard.xml  | 45 ++
 tests/qemuxml2argvtest.c  |  1 +
 .../disk-virtio-discard.x86_64-latest.xml | 58 +++
 tests/qemuxml2xmltest.c   |  1 +
 17 files changed, 226 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml
 create mode 100644 
tests/qemuxml2xmloutdata/disk-virtio-discard.x86_64-latest.xml

-- 
2.38.5



[PATCH Libvirt v2 1/3] conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

Add 'virtio_discard' and 'virtio_write_zeroes' attribute to control
whether discard and write-zeroes requests are handled by the
virtio-blk device.

To distinguish the attributes in block drive layer, use the 'virtio'
prefix to indicate that these attributes are specific for virtio-blk.

Signed-off-by: Hyman Huang(黄勇) 
---
 docs/formatdomain.rst |  8 
 src/conf/domain_conf.c| 16 
 src/conf/domain_conf.h|  2 ++
 src/conf/schemas/domaincommon.rng | 10 ++
 src/conf/storage_source_conf.c|  2 ++
 src/conf/storage_source_conf.h|  2 ++
 src/qemu/qemu_domain.c|  2 ++
 src/qemu/qemu_driver.c|  4 +++-
 src/vz/vz_utils.c | 12 
 9 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 4af0b82569..7be12ff08e 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3259,6 +3259,14 @@ paravirtualized driver is specified via the ``disk`` 
element.
   value can be either "unmap" (allow the discard request to be passed) or
   "ignore" (ignore the discard request). :since:`Since 1.0.6 (QEMU and KVM
   only)`
+   -  The optional ``virtio_discard`` and ``virtio_write_zeroes`` are 
attributes
+  that control whether discard and write-zeroes requests are handled by the
+  virtio-blk device. The feature is based on DISCARD and WRITE_ZEROES
+  commands introduced in virtio-blk protocol to improve performance when
+  using SSD backend. The value can be either 'on' or 'off'. Note that
+  ``discard`` and ``write_zeroes`` implementations in the block drive layer
+  are parts of the feature logically and should be turned on when enabling
+  the feature. :since:`Since 9.6.0 (QEMU and KVM only)`
-  The optional ``detect_zeroes`` attribute controls whether to detect zero
   write requests. The value can be "off", "on" or "unmap". First two values
   turn the detection off and on, respectively. The third value ("unmap")
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5ac5c0b771..0f82b489f4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7813,6 +7813,14 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def,
VIR_XML_PROP_NONZERO, >discard) < 0)
 return -1;
 
+if (virXMLPropTristateSwitch(cur, "virtio_discard", VIR_XML_PROP_NONE,
+ >virtio_discard) < 0)
+return -1;
+
+if (virXMLPropTristateSwitch(cur, "virtio_write_zeroes", VIR_XML_PROP_NONE,
+ >virtio_write_zeroes) < 0)
+return -1;
+
 if (virXMLPropUInt(cur, "iothread", 10, VIR_XML_PROP_NONZERO, 
>iothread) < 0)
 return -1;
 
@@ -22514,6 +22522,14 @@ virDomainDiskDefFormatDriver(virBuffer *buf,
 virBufferAsprintf(, " discard='%s'",
   virDomainDiskDiscardTypeToString(disk->discard));
 
+if (disk->virtio_discard)
+virBufferAsprintf(, " virtio_discard='%s'",
+  virTristateSwitchTypeToString(disk->virtio_discard));
+
+if (disk->virtio_write_zeroes)
+virBufferAsprintf(, " virtio_write_zeroes='%s'",
+  
virTristateSwitchTypeToString(disk->virtio_write_zeroes));
+
 if (disk->iothread)
 virBufferAsprintf(, " iothread='%u'", disk->iothread);
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c857ba556f..86e955333e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -607,6 +607,8 @@ struct _virDomainDiskDef {
 unsigned int iothread; /* unused = 0, > 0 specific thread # */
 virDomainDiskDetectZeroes detect_zeroes;
 virTristateSwitch discard_no_unref;
+virTristateSwitch virtio_discard;
+virTristateSwitch virtio_write_zeroes;
 char *domain_name; /* backend domain name */
 unsigned int queues;
 unsigned int queue_size;
diff --git a/src/conf/schemas/domaincommon.rng 
b/src/conf/schemas/domaincommon.rng
index c2f56b0490..c3a59aeb15 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -2510,6 +2510,16 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+
+  
   
 
   
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index dcac3a8ff6..c2fee652f3 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -810,6 +810,8 @@ virStorageSourceCopy(const virStorageSource *src,
 def->discard = src->discard;
 def->detect_zeroes = src->detect_zeroes;
 def->discard_no_unref = src->discard_no_unref;
+def->virtio_discard = src->virtio_discard;
+def->virtio_write_zeroes = src->virtio_write_zeroes;
 def->sslverify = src->sslverify;
 def->readahead = src->readahead;
 def->timeout = src->timeout;
diff --git 

[PATCH Libvirt v2 3/3] NEWS: Mention support for discard and write_zeroes of virtio-blk device

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

Signed-off-by: Hyman Huang(黄勇) 
---
 NEWS.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 42c2c53091..b6fde7c353 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -17,6 +17,14 @@ v9.6.0 (unreleased)
 
 * **New features**
 
+  * qemu: Introducing ``discard`` and ``write_zeroes`` features of virtio-blk 
device
+
+The optional ``virtio_discard`` and ``virtio_write_zeroes`` attributes
+control whether discard and write-zeroes requests are handled by the
+virtio-blk device. The feature is based on DISCARD and WRITE_ZEROES 
commands
+introduced in virtio-blk protocol to improve performance when using SSD
+backend.
+
 * **Improvements**
 
   * apparmor: All profiles and abstractions now support local overrides
-- 
2.38.5



Re: [PATCH] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Benedek Major
This is the patch, as suggested by Daniel, using only the product tag for 
IDE/SATA.
Since qemu supports the model tag, if the drive bus is IDE OR SATA the product 
string gets passed to qemu using the model parameter
If the bus is either SATA or IDE, the product string can have a max lenght of 
40, as per ATAPI spec in the field "model" of the
IDENTIFY PACKET DEVICE. This should be the better solution, since you do not 
need to rely on space checking in the vendor field.
Tell me what you think about this version, and if it is satisfactory, I will 
write the missing tests.

Thanks for your time and effort,
Benedek

Signed-off-by: Benedek Major 
---
 src/conf/domain_validate.c | 18 +++---
 src/qemu/qemu_command.c| 10 --
 src/qemu/qemu_validate.c   | 12 +++-
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 16bf3b559f..b9940c78a5 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -585,7 +585,8 @@ virDomainDiskDefValidateSource(const virStorageSource *src)
 
 
 #define VENDOR_LEN  8
-#define PRODUCT_LEN 16
+#define PRODUCT_LEN_SCSI 16
+#define PRODUCT_LEN_IDE 40
 
 
 /**
@@ -856,10 +857,21 @@ virDomainDiskDefValidate(const virDomainDef *def,
 return -1;
 }
 
-if (strlen(disk->product) > PRODUCT_LEN) {
+if ((disk->bus == VIR_DOMAIN_DISK_BUS_SATA ||
+disk->bus == VIR_DOMAIN_DISK_BUS_IDE) &&
+strlen(disk->product) > PRODUCT_LEN_IDE) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk product is more than %1$d characters"),
-   PRODUCT_LEN);
+   PRODUCT_LEN_IDE);
+return -1;
+}
+
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SATA &&
+disk->bus != VIR_DOMAIN_DISK_BUS_IDE &&
+strlen(disk->product) > PRODUCT_LEN_SCSI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("disk product is more than %1$d characters"),
+   PRODUCT_LEN_SCSI);
 return -1;
 }
 }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..7bc1d8c682 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1762,6 +1762,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
 unsigned int physical_block_size = disk->blockio.physical_block_size;
 g_autoptr(virJSONValue) wwn = NULL;
 g_autofree char *serial = NULL;
+g_autofree char *modelstr = NULL;
+g_autofree char *product = g_strdup(disk->product);
 virTristateSwitch removable = VIR_TRISTATE_SWITCH_ABSENT;
 virTristateSwitch writeCache = VIR_TRISTATE_SWITCH_ABSENT;
 const char *biosCHSTrans = NULL;
@@ -1775,7 +1777,10 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
 driver = "ide-cd";
 else
 driver = "ide-hd";
-
+/* exchange product to model for QEMU ide disk */
+modelstr = g_strdup(product);
+g_free(product);
+product = NULL;
 break;
 
 case VIR_DOMAIN_DISK_BUS_SCSI:
@@ -1944,7 +1949,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
   "A:wwn", ,
   "p:rotation_rate", disk->rotation_rate,
   "S:vendor", disk->vendor,
-  "S:product", disk->product,
+  "S:product", product,
+  "S:model", modelstr,
   "T:removable", removable,
   "S:write-cache", qemuOnOffAuto(writeCache),
   "p:cyls", disk->geometry.cylinders,
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..4893b979d3 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2871,12 +2871,22 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
virDomainDiskDef *disk,
 }
 
 if (disk->vendor || disk->product) {
-if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI && disk->vendor) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Only scsi disk supports vendor and product"));
 return -1;
 }
 
+if (disk->product &&
+disk->bus != VIR_DOMAIN_DISK_BUS_IDE &&
+disk->bus != VIR_DOMAIN_DISK_BUS_SATA &&
+disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Only scsi, ide, sata disk supports product"));
+return -1;
+}
+
+
 /* Properties wwn, vendor and product were introduced in the
  * same QEMU release (1.2.0).
  */
-- 
2.41.0



Re: [PATCH Libvirt 1/3] qemu_capabilities: Introduce virtio-blk DISCARD and WRITE_ZEROES capabilities

2023-07-17 Thread Jonathon Jongsma
I believe that qemu 4.2.0 is the oldest version of qemu that we still 
support, so I don't think that a new capability would actually be 
necessary for this.


Jonathon


On 7/16/23 8:41 AM, ~hyman wrote:

From: Hyman Huang(黄勇) 

DISCARD and WRITE_ZEROES commands has been implemented in virtio-blk
protocol since qemu >= 4.2.0.
Introduce QEMU_CAPS_VIRTIO_BLK_DISCARD and QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES
capability definitions.

Signed-off-by: Hyman Huang(黄勇) 
---
  src/qemu/qemu_capabilities.c  | 4 
  src/qemu/qemu_capabilities.h  | 2 ++
  tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_4.2.0_s390x.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_4.2.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_5.0.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_5.0.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_5.0.0_riscv64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_5.0.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_5.1.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_5.2.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_5.2.0_riscv64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_5.2.0_s390x.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_5.2.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_6.0.0_s390x.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_6.0.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_6.1.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_6.2.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_6.2.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml | 2 ++
  tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_7.0.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_7.0.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_7.1.0_ppc64.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_7.1.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml | 2 ++
  tests/qemucapabilitiesdata/caps_7.2.0_x86_64+hvf.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_7.2.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_8.0.0_riscv64.xml | 2 ++
  tests/qemucapabilitiesdata/caps_8.0.0_x86_64.xml  | 2 ++
  tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml   | 2 ++
  tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml  | 2 ++
  36 files changed, 74 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c9f4b17208..b11bac95e6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -697,6 +697,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
  
/* 450 */

"run-with.async-teardown", /* QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN 
*/
+  "virtio-blk.discard", /* QEMU_CAPS_VIRTIO_BLK_DISCARD */
+  "virtio-blk.write-zeroes", /* QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES 
*/
  );
  
  
@@ -1422,6 +1424,8 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioBlk[] = {

  { "scsi", QEMU_CAPS_VIRTIO_BLK_SCSI, 
virQEMUCapsDevicePropsVirtioBlkSCSIDefault },
  { "queue-size", QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, NULL },
  { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL },
+{ "discard", QEMU_CAPS_VIRTIO_BLK_DISCARD, NULL },
+{ "write-zeroes", QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES, NULL },
  };
  
  static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = {

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 2460fa7fa0..bcf0aaa0a5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -676,6 +676,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
  
  /* 450 */

  QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN, /* asynchronous teardown -run-with 
async-teardown=on|off */
+QEMU_CAPS_VIRTIO_BLK_DISCARD, /* virtio-blk-*.discard */
+QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES, /* virtio-blk-*.write-zeroes */
  
  QEMU_CAPS_LAST /* this must always be the last item */

  } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
index 6a7f33e3c6..3823eb7994 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
@@ -111,6 +111,8 @@



+  
+  
4002000
61700242
v4.1.0-2221-g36609b4fa3
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml
index 2064f07c9c..0760733c7b 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml
+++ 

[PATCH Libvirt 3/3] qemu_command: Generate cmd line for discard and write-zeroes properties

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

Generate cmd line for virtio-blk.discard and virtio-blk.write-zeroes
properties. Also, validate that the requested feature is supported by QEMU.

Signed-off-by: Hyman Huang(黄勇) 
---
 src/qemu/qemu_command.c   |  2 +
 src/qemu/qemu_validate.c  | 14 +
 .../disk-virtio-discard.x86_64-latest.args| 44 +++
 .../qemuxml2argvdata/disk-virtio-discard.xml  | 56 +++
 tests/qemuxml2argvtest.c  |  1 +
 5 files changed, 117 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..6954fc5d75 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1834,6 +1834,8 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
   "T:scsi", scsi,
   "p:num-queues", disk->queues,
   "p:queue-size", disk->queue_size,
+  "T:discard", disk->virtio_discard,
+  "T:write-zeroes", disk->virtio_write_zeroes,
   NULL) < 0)
 return NULL;
 }
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..5abe82e1a5 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2989,6 +2989,20 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
virDomainDiskDef *disk,
_("queue-size property isn't supported by this QEMU 
binary"));
 return -1;
 }
+if (disk->virtio_discard &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_DISCARD)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("discard property for virtio-blk device isn't 
supported "
+ "by this QEMU binary"));
+return -1;
+}
+if (disk->virtio_write_zeroes &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("write_zeroes property for virtio-blk device 
isn't "
+ "supported by this QEMU binary"));
+return -1;
+}
 break;
 
 case VIR_DOMAIN_DISK_BUS_USB:
diff --git a/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
new file mode 100644
index 00..30596a57a5
--- /dev/null
+++ b/tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
@@ -0,0 +1,44 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
 \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-accel tcg \
+-cpu qemu64 \
+-m size=219136k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-device 
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-blockdev 
'{"driver":"file","filename":"/tmp/data.img","node-name":"libvirt-4-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-4-format","read-only":false,"driver":"raw","file":"libvirt-4-storage"}'
 \
+-device 
'{"driver":"virtio-blk-pci","discard":true,"write-zeroes":true,"bus":"pci.0","addr":"0x3","drive":"libvirt-4-format","id":"virtio-disk0","bootindex":1}'
 \
+-blockdev 
'{"driver":"file","filename":"/tmp/data1.img","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-3-format","read-only":false,"driver":"raw","file":"libvirt-3-storage"}'
 \
+-device 
'{"driver":"virtio-blk-pci","discard":true,"write-zeroes":false,"bus":"pci.0","addr":"0x4","drive":"libvirt-3-format","id":"virtio-disk1"}'
 \
+-blockdev 
'{"driver":"file","filename":"/tmp/data2.img","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}'
 \
+-blockdev 
'{"node-name":"libvirt-2-format","read-only":false,"driver":"raw","file":"libvirt-2-storage"}'
 \
+-device 

[PATCH Libvirt 0/3] support discard and write-zeroes options for virtio-blk device

2023-07-17 Thread ~hyman
DISCARD and WRITE_ZEROES commands has been implemented in virtio-blk
protocol since qemu >= 4.2.0, may be it's time to introduce discard and
write-zeroes options for virtio-blk device in libvirt so that the upper
layer can enable this feature at disk granularity.
To distinguish the discard option in block drive layer, use the 'virtio'
prefix to indicate that these attributes are specific for virtio-blk
disk.
To try this out, three things has done in this patchset:
1. introduce capabilities of discard and write-zeroes for virtio-blk
2. add virtio_discard and virtio_write_zeroes attributes of driver in
disk xml element
3. generate cmd line when launching vm

Please review, any comments and suggestions are very appreciated,
thanks!
Yong

Hyman Huang(黄勇) (3):
  qemu_capabilities: Introduce virtio-blk DISCARD and WRITE_ZEROES
capabilities
  conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes
  qemu_command: Generate cmd line for discard and write-zeroes
properties

 docs/formatdomain.rst |  8 +++
 src/conf/domain_conf.c| 16 ++
 src/conf/domain_conf.h|  2 +
 src/conf/schemas/domaincommon.rng | 10 
 src/qemu/qemu_capabilities.c  |  4 ++
 src/qemu/qemu_capabilities.h  |  2 +
 src/qemu/qemu_command.c   |  2 +
 src/qemu/qemu_validate.c  | 14 +
 .../caps_4.2.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_4.2.0_ppc64.xml |  2 +
 .../qemucapabilitiesdata/caps_4.2.0_s390x.xml |  2 +
 .../caps_4.2.0_x86_64.xml |  2 +
 .../caps_5.0.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_5.0.0_ppc64.xml |  2 +
 .../caps_5.0.0_riscv64.xml|  2 +
 .../caps_5.0.0_x86_64.xml |  2 +
 .../caps_5.1.0_x86_64.xml |  2 +
 .../caps_5.2.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_5.2.0_ppc64.xml |  2 +
 .../caps_5.2.0_riscv64.xml|  2 +
 .../qemucapabilitiesdata/caps_5.2.0_s390x.xml |  2 +
 .../caps_5.2.0_x86_64.xml |  2 +
 .../caps_6.0.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_6.0.0_s390x.xml |  2 +
 .../caps_6.0.0_x86_64.xml |  2 +
 .../caps_6.1.0_x86_64.xml |  2 +
 .../caps_6.2.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_6.2.0_ppc64.xml |  2 +
 .../caps_6.2.0_x86_64.xml |  2 +
 .../caps_7.0.0_aarch64+hvf.xml|  2 +
 .../caps_7.0.0_aarch64.xml|  2 +
 .../qemucapabilitiesdata/caps_7.0.0_ppc64.xml |  2 +
 .../caps_7.0.0_x86_64.xml |  2 +
 .../qemucapabilitiesdata/caps_7.1.0_ppc64.xml |  2 +
 .../caps_7.1.0_x86_64.xml |  2 +
 tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml |  2 +
 .../caps_7.2.0_x86_64+hvf.xml |  2 +
 .../caps_7.2.0_x86_64.xml |  2 +
 .../caps_8.0.0_riscv64.xml|  2 +
 .../caps_8.0.0_x86_64.xml |  2 +
 .../qemucapabilitiesdata/caps_8.1.0_s390x.xml |  2 +
 .../caps_8.1.0_x86_64.xml |  2 +
 .../disk-virtio-discard.x86_64-latest.args| 44 +++
 .../qemuxml2argvdata/disk-virtio-discard.xml  | 56 +++
 tests/qemuxml2argvtest.c  |  1 +
 45 files changed, 227 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/disk-virtio-discard.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/disk-virtio-discard.xml

-- 
2.38.5



[PATCH Libvirt 1/3] qemu_capabilities: Introduce virtio-blk DISCARD and WRITE_ZEROES capabilities

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

DISCARD and WRITE_ZEROES commands has been implemented in virtio-blk
protocol since qemu >= 4.2.0.
Introduce QEMU_CAPS_VIRTIO_BLK_DISCARD and QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES
capability definitions.

Signed-off-by: Hyman Huang(黄勇) 
---
 src/qemu/qemu_capabilities.c  | 4 
 src/qemu/qemu_capabilities.h  | 2 ++
 tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_4.2.0_s390x.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_4.2.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_5.0.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_5.0.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_5.0.0_riscv64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_5.0.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_5.1.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_5.2.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_5.2.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_5.2.0_riscv64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_5.2.0_s390x.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_5.2.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_6.0.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_6.0.0_s390x.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_6.0.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_6.1.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_6.2.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_6.2.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_6.2.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_7.0.0_aarch64+hvf.xml | 2 ++
 tests/qemucapabilitiesdata/caps_7.0.0_aarch64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_7.0.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_7.0.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_7.1.0_ppc64.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_7.1.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_7.2.0_ppc.xml | 2 ++
 tests/qemucapabilitiesdata/caps_7.2.0_x86_64+hvf.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_7.2.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_8.0.0_riscv64.xml | 2 ++
 tests/qemucapabilitiesdata/caps_8.0.0_x86_64.xml  | 2 ++
 tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml   | 2 ++
 tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml  | 2 ++
 36 files changed, 74 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c9f4b17208..b11bac95e6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -697,6 +697,8 @@ VIR_ENUM_IMPL(virQEMUCaps,
 
   /* 450 */
   "run-with.async-teardown", /* QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN 
*/
+  "virtio-blk.discard", /* QEMU_CAPS_VIRTIO_BLK_DISCARD */
+  "virtio-blk.write-zeroes", /* QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES 
*/
 );
 
 
@@ -1422,6 +1424,8 @@ static struct virQEMUCapsDevicePropsFlags 
virQEMUCapsDevicePropsVirtioBlk[] = {
 { "scsi", QEMU_CAPS_VIRTIO_BLK_SCSI, 
virQEMUCapsDevicePropsVirtioBlkSCSIDefault },
 { "queue-size", QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE, NULL },
 { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL },
+{ "discard", QEMU_CAPS_VIRTIO_BLK_DISCARD, NULL },
+{ "write-zeroes", QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES, NULL },
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 2460fa7fa0..bcf0aaa0a5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -676,6 +676,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 
 /* 450 */
 QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN, /* asynchronous teardown -run-with 
async-teardown=on|off */
+QEMU_CAPS_VIRTIO_BLK_DISCARD, /* virtio-blk-*.discard */
+QEMU_CAPS_VIRTIO_BLK_WRITE_ZEROES, /* virtio-blk-*.write-zeroes */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
index 6a7f33e3c6..3823eb7994 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0_aarch64.xml
@@ -111,6 +111,8 @@
   
   
   
+  
+  
   4002000
   61700242
   v4.1.0-2221-g36609b4fa3
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml
index 2064f07c9c..0760733c7b 100644
--- a/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.2.0_ppc64.xml
@@ -110,6 +110,8 @@
   
   
   
+  
+  
   4002000
   42900242
   v4.1.0-2198-g9e583f2
diff --git a/tests/qemucapabilitiesdata/caps_4.2.0_s390x.xml 
b/tests/qemucapabilitiesdata/caps_4.2.0_s390x.xml
index b440d9d538..8a0c687cb6 

[PATCH Libvirt 2/3] conf: Add 'virtio_discard' and 'virtio_write_zeroes' attributes

2023-07-17 Thread ~hyman
From: Hyman Huang(黄勇) 

Add 'virtio_discard' and 'virtio_write_zeroes' attribute to control
whether discard and write-zeroes requests are handled by the
virtio-blk device.

To distinguish the attributes in block drive layer, use the 'virtio'
prefix to indicate that these attributes are specific for virtio-blk.

Signed-off-by: Hyman Huang(黄勇) 
---
 docs/formatdomain.rst |  8 
 src/conf/domain_conf.c| 16 
 src/conf/domain_conf.h|  2 ++
 src/conf/schemas/domaincommon.rng | 10 ++
 4 files changed, 36 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 4af0b82569..7be12ff08e 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3259,6 +3259,14 @@ paravirtualized driver is specified via the ``disk`` 
element.
   value can be either "unmap" (allow the discard request to be passed) or
   "ignore" (ignore the discard request). :since:`Since 1.0.6 (QEMU and KVM
   only)`
+   -  The optional ``virtio_discard`` and ``virtio_write_zeroes`` are 
attributes
+  that control whether discard and write-zeroes requests are handled by the
+  virtio-blk device. The feature is based on DISCARD and WRITE_ZEROES
+  commands introduced in virtio-blk protocol to improve performance when
+  using SSD backend. The value can be either 'on' or 'off'. Note that
+  ``discard`` and ``write_zeroes`` implementations in the block drive layer
+  are parts of the feature logically and should be turned on when enabling
+  the feature. :since:`Since 9.6.0 (QEMU and KVM only)`
-  The optional ``detect_zeroes`` attribute controls whether to detect zero
   write requests. The value can be "off", "on" or "unmap". First two values
   turn the detection off and on, respectively. The third value ("unmap")
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5ac5c0b771..0f82b489f4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7813,6 +7813,14 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def,
VIR_XML_PROP_NONZERO, >discard) < 0)
 return -1;
 
+if (virXMLPropTristateSwitch(cur, "virtio_discard", VIR_XML_PROP_NONE,
+ >virtio_discard) < 0)
+return -1;
+
+if (virXMLPropTristateSwitch(cur, "virtio_write_zeroes", VIR_XML_PROP_NONE,
+ >virtio_write_zeroes) < 0)
+return -1;
+
 if (virXMLPropUInt(cur, "iothread", 10, VIR_XML_PROP_NONZERO, 
>iothread) < 0)
 return -1;
 
@@ -22514,6 +22522,14 @@ virDomainDiskDefFormatDriver(virBuffer *buf,
 virBufferAsprintf(, " discard='%s'",
   virDomainDiskDiscardTypeToString(disk->discard));
 
+if (disk->virtio_discard)
+virBufferAsprintf(, " virtio_discard='%s'",
+  virTristateSwitchTypeToString(disk->virtio_discard));
+
+if (disk->virtio_write_zeroes)
+virBufferAsprintf(, " virtio_write_zeroes='%s'",
+  
virTristateSwitchTypeToString(disk->virtio_write_zeroes));
+
 if (disk->iothread)
 virBufferAsprintf(, " iothread='%u'", disk->iothread);
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index c857ba556f..b73b1241ad 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -604,6 +604,8 @@ struct _virDomainDiskDef {
 virTristateBool rawio;
 virDomainDeviceSGIO sgio;
 virDomainDiskDiscard discard;
+virTristateSwitch virtio_discard;
+virTristateSwitch virtio_write_zeroes;
 unsigned int iothread; /* unused = 0, > 0 specific thread # */
 virDomainDiskDetectZeroes detect_zeroes;
 virTristateSwitch discard_no_unref;
diff --git a/src/conf/schemas/domaincommon.rng 
b/src/conf/schemas/domaincommon.rng
index c2f56b0490..c3a59aeb15 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -2510,6 +2510,16 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+
+  
   
 
   
-- 
2.38.5



Re: [libvirt PATCH] docs: expand clangd instructions

2023-07-17 Thread Tim Small

On 14/07/2023 16:44, Jonathon Jongsma wrote:

Add some additional information about running clangd for LSP when clang
is not your normal compiler.

Signed-off-by: Jonathon Jongsma 
---
  docs/clangd.rst | 17 +
  1 file changed, 17 insertions(+)


Reviewed-By: Tim Small 

Tim



[PATCH] conf: domcaps: Add 'async-teardown' domain capability

2023-07-17 Thread Boris Fiuczynski
Add async-teardown to the features list in domain capabilities allowing
high level management to introspect the availability of the asynchronous
teardown feature.

Signed-off-by: Boris Fiuczynski 
---
 docs/formatdomaincaps.rst| 6 ++
 src/conf/domain_capabilities.c   | 1 +
 src/conf/domain_capabilities.h   | 1 +
 src/conf/schemas/domaincaps.rng  | 9 +
 src/qemu/qemu_capabilities.c | 1 +
 tests/domaincapsdata/qemu_4.2.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_4.2.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_4.2.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_4.2.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_4.2.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_4.2.0.s390x.xml| 1 +
 tests/domaincapsdata/qemu_4.2.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.0.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.0.0-tcg-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_5.0.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.0.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_5.0.0-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_5.0.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_5.0.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_5.0.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.1.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.1.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.1.0.sparc.xml| 1 +
 tests/domaincapsdata/qemu_5.1.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.2.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.2.0-tcg-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_5.2.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_5.2.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_5.2.0-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_5.2.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_5.2.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_5.2.0.s390x.xml| 1 +
 tests/domaincapsdata/qemu_5.2.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.0.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.0.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.0.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_6.0.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_6.0.0.s390x.xml| 1 +
 tests/domaincapsdata/qemu_6.0.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.1.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.1.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.1.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.2.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.2.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_6.2.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_6.2.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_6.2.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_6.2.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.0.0-hvf.aarch64+hvf.xml  | 1 +
 tests/domaincapsdata/qemu_7.0.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.0.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.0.0-virt.aarch64.xml | 1 +
 tests/domaincapsdata/qemu_7.0.0.aarch64.xml  | 1 +
 tests/domaincapsdata/qemu_7.0.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_7.0.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.1.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.1.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.1.0.ppc64.xml| 1 +
 tests/domaincapsdata/qemu_7.1.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.2.0-hvf.x86_64+hvf.xml   | 1 +
 tests/domaincapsdata/qemu_7.2.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.2.0-tcg.x86_64+hvf.xml   | 1 +
 tests/domaincapsdata/qemu_7.2.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_7.2.0.ppc.xml  | 1 +
 tests/domaincapsdata/qemu_7.2.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.0.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.0.0-tcg-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_8.0.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.0.0-virt.riscv64.xml | 1 +
 tests/domaincapsdata/qemu_8.0.0.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.1.0-q35.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.1.0-tcg.x86_64.xml   | 1 +
 tests/domaincapsdata/qemu_8.1.0.s390x.xml| 1 +
 tests/domaincapsdata/qemu_8.1.0.x86_64.xml   | 1 +
 74 files changed, 87 insertions(+)

diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst
index 9dae941d18..0f8015d4dd 100644
--- a/docs/formatdomaincaps.rst
+++ b/docs/formatdomaincaps.rst
@@ -647,6 +647,7 @@ capabilities. All features occur as children of the main 
``features`` element.



+   
 

Re: [PATCH] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Daniel P . Berrangé
On Mon, Jul 17, 2023 at 03:04:56PM +0200, Benedek Major wrote:
> Hello,
> thanks for your fast responses.
> I've also recognized flaws in my implementation, after your comments, so
> I'll try to find a solution, that is acceptable, and create a better patch.
> But first, I'll need to know if this approach is correct:
> 
> > my interpretation is that the SCSI "product" field data is identical
> > to the ATAPI "model" field data.
> > AFAICT, ATAPI doesn't provide a way to expose a vendor in string
> > format.  So I would say we accept 'product' for IDE, but reject
> > 'vendor'
> I would also say so.
> Therefore I would need to expand the vendor field to 40 Chars, if a SATA/IDE
> bus is selected. (current is 8 for SCSI).
> 
> about this:
> > an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real*
> > hardware
> With hdparm -I /dev/sdX I got:
> Model Number:   TOSHIBA DT01ACA050
> Model Number:   Samsung SSD 850 EVO 250GB
> 
> I think hdparam just dumps the raw responses, but correct me if I'm wrong.

I've checked the source and you're correct, this the exact model
string.  So it seems in practice the model contains both the vendor
and product strings, which validates your proposal to do the same
in libvirt.

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] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Benedek Major

Hello,
thanks for your fast responses.
I've also recognized flaws in my implementation, after your comments, so 
I'll try to find a solution, that is acceptable, and create a better 
patch. But first, I'll need to know if this approach is correct:


> my interpretation is that the SCSI "product" field data is identical
> to the ATAPI "model" field data.
> AFAICT, ATAPI doesn't provide a way to expose a vendor in string
> format.  So I would say we accept 'product' for IDE, but reject
> 'vendor'
I would also say so.
Therefore I would need to expand the vendor field to 40 Chars, if a 
SATA/IDE bus is selected. (current is 8 for SCSI).


about this:
> an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real*
> hardware
With hdparm -I /dev/sdX I got:
Model Number:   TOSHIBA DT01ACA050
Model Number:   Samsung SSD 850 EVO 250GB

I think hdparam just dumps the raw responses, but correct me if I'm wrong.

>> Therefore I would say, that vendor and product pretty much correlate 
to the model field in QEMU,


>>This feels very subjective. Could you please add a better justification?
>>E.g. show how qemu formats the default vendor and product name?
QEMU formats the defaults as "QEMU HARDDISK" and "QEMU CD-ROM"
This was just an assumption, for which I didn't find any spec yet.
Since every disk I know is in the form of:
MANUFACTURER SOME OTHER MODEL NAME
Maybe it is just a convention, but I have no experience in that field.
In the Windows VM in device manager under the SATA device for
"QEMU HARDDISK" it shows as Device instance path 
SCSI\DISK_QEMU_HARDDISK\, which also made me think that it is 
the norm.


About the coding style, I'll fix those problems.
And thanks for reminding me of the memory leaks in C, im so accustomed 
to writing in code that does not have them, that I almost forgot how 
aware you must be of declaring pointers.


Sincerely,
Benedek




Re: [PATCH] qemu: Add NUMA node automatically for memory hotplug

2023-07-17 Thread Kristina Hanicova
On Thu, Jul 13, 2023 at 4:23 PM Michal Privoznik 
wrote:

> Up until v2.11.0-rc2~19^2~3 QEMU used to require at least one
> NUMA node to be configured when memory hotplug was enabled. After
> that commit, QEMU automatically adds a NUMA node if none was
> specified on the cmd line. Reflect this in domain XML, i.e.
> explicitly add a NUMA node into our domain definition if needed.
>
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c| 53 ++-
>  ...emory-hotplug-ppc64-nonuma-abi-update.args | 11 ++--
>  ...memory-hotplug-ppc64-nonuma-abi-update.xml |  7 ++-
>  3 files changed, 63 insertions(+), 8 deletions(-)
>

Reviewed-by: Kristina Hanicova 
Kristina


Re: Add blocksize properties to libvirt

2023-07-17 Thread Peter Krempa
On Mon, Jul 10, 2023 at 16:39:43 +0300, Андрей Ячменёв wrote:
>Hello!
> 
>I want to make patch for exposing min_io_size and opt_io_size properties
>to
>export block topology information to the guest. This is needed to set
>optimal
>i/o alignment for storages in underlay.
> 
>This option already exists in qemu but haven't been implemented yet in
>libvirt.
> 
>It is assumed if you do not set these values qemu wont be report that into
>the guest. Inside guest undefined opt_io_size value transferred to
>sys/block/{drive}/queue/optimal_io_size like 0. And filesystems like xfs
>or
>ext4 don't set correct aggregated io size (typicaly 32k bytes for xfs).

Note that qemu apart from the possibility to explicitly specify the size
supports also a 'backend_defaults' option, which is by default in _AUTO
mode. Inside of 'blkconf_blocksizes()' the block size defaults are then
populated.

By default in _AUTO mode 'conf->physical_block_size' and
'conf->logical_block_size' are auto-populated but not
'conf->opt_io_size' or the discard granularity setting.

I think we should give the users the possibility to enable the
qemu-configured defaults in this case knowing the proper value to
configure may be non-trivial, more on that below.

> 
>At the moment i think to make a patch with changes to the conf and qemu
>parts
>of libvirt.
> 
>These options should be added to the address tag in the guest xml to
>section
>for disk devices. The values should be positive integers. To use these
>options,
>you must use qemu.

Libvirt already handles some of the sibling values for configuring block
sizes via:

  





  

Thus you should be able to use that placement without the need to
reinvent any placement. Placing it under address would even be wrong.

>Can you tell me if there are any additional restrictions about this
>options
>that should be taken into account?

In libvirt we generally try to expose config knobs which make sense for
the users to set. That requires that the users know how to set the
option properly. Based on that I'd suggest that you first and foremost
allow the users to enable the backend defaults in qemu, as that seems to
set many things optimally without the user needing to know more.

Later then you can also add knobs to fine tune the values if necessary.

Please don't forget to read our contribution guidelines and follow the
coding style we have in place.

Note that some of the knobs may not be available in all supported QEMU
versions (libvirt currently supports qemu-4.2 and newer) which may
require adding a capability check.

If you have any questions regarding specifics feel free to ask on-list
or on our IRC channel.



Re: [PATCH] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Daniel P . Berrangé
On Sat, Jul 15, 2023 at 06:37:10PM +0200, Benedek Major wrote:
> The XML-Schema specifies two tags vendor and product for the disk type. But 
> they only apply to the SCSI bus,
> even though the QEMU driver ide-hd and ide-cd have support for the command 
> line argument -model.
> The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response 
> from the ATAPI spec.
> Words 27-46 is a 40 Char space for the model number of the device. The model 
> number is built by
> combining the vendor and the product fields with a single space as separator 
> in the middle.
> Therefore I would say, that vendor and product pretty much correlate to the 
> model field in QEMU,
> so the ide disk should also have the possibility of adding vendor and product 
> to it.

Lets compare SCSI:

  
https://www.seagate.com/files/staticfiles/support/docs/manual/Interface%20manuals/100293068j.pdf

[quote]
PRODUCT IDENTIFICATION
The PRODUCT IDENTIFICATION field contains sixteen bytes of left-aligned
ASCII data (see 5.4.2) defined by Seagate. Bytes 16 through 31 indicate
the drive model with 20h (space) used as a filler. The table below is
an example of drive test data returned by the drive. Bytes 16 and 17
will contain 53 54 for all drive models.
[/quote/

And ATARPI

  
https://people.freebsd.org/~imp/asiabsdcon2015/works/d2161r5-ATAATAPI_Command_Set_-_3.pdf

[quote]
A.11.7.4 MODEL NUMBER field
The MODEL NUMBER field contains the model number of the device. The
contents of the MODEL NUMBER field is an ATA string of forty bytes in
the format defined by 3.3.10. The device shall pad the string with
spaces (i.e., 20h), if necessary, to ensure that the string is the
proper length. The combination of the serial number (see A.11.7.2)
and the model number shall be unique for a given manufacturer.
The IDENTIFY DEVICE data contains a copy of the MODEL NUMBER field
(see IDENTIFY DEVICE data words 27..46 in table 45).
[/quote]


my interpretation is that the SCSI "product" field data is identical
to the ATAPI "model" field data.

AFAICT, ATAPI doesn't provide a way to expose a vendor in string
format.  So I would say we accept 'product' for IDE, but reject
'vendor'

Having said that it would be interesting to know the results from
an IDENTIFY DEVICE / IDENTIFY PACKET DEVICE command on some *real*
hardware, as an illustration of what vendors actually did in
practice.  I don't have access to any real hardware that still
uses SATA/IDE though.

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 1/2] NEWS: qemu: Support removable attribute for scsi disk

2023-07-17 Thread Han Han
Signed-off-by: Han Han 
---
 NEWS.rst | 5 +
 1 file changed, 5 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 42c2c53091..5ee880efcb 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -27,6 +27,11 @@ v9.6.0 (unreleased)
 ``abstractions/foo`` can be overridden by creating ``local/foo`` and
 ``abstractions/foo.d`` respectively.
 
+  * qemu: Support removable attribute for scsi disk
+
+Now the scsi disk device (``/disk@device='disk'`` and 
``/disk/target@bus='scsi'``) supports
+the removable attribute at ``/disk/target@removable```.
+
 * **Bug fixes**
 
 
-- 
2.41.0



[PATCH 2/2] NEWS: cpu_map: Add SapphireRapids cpu model

2023-07-17 Thread Han Han
Signed-off-by: Han Han 
---
 NEWS.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index 5ee880efcb..7abc871f20 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -106,6 +106,10 @@ v9.4.0 (2023-06-01)
 
 QEMU supports parallel migration to be compressed using either zstd or 
zlib.
 
+  * cpu_map: Add SapphireRapids cpu model
+
+This model is introduced since QEMU 8.0.
+
 * **Improvements**
 
   * Adapt to musl-1.2.4
-- 
2.41.0



[PATCH 0/2] NEWS update for 9.4 and 9.6

2023-07-17 Thread Han Han
Han Han (2):
  NEWS: qemu: Support removable attribute for scsi disk
  NEWS: cpu_map: Add SapphireRapids cpu model

 NEWS.rst | 9 +
 1 file changed, 9 insertions(+)

-- 
2.41.0



Re: [PATCH] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Peter Krempa
On Sat, Jul 15, 2023 at 18:37:10 +0200, Benedek Major wrote:
> The XML-Schema specifies two tags vendor and product for the disk type. But 
> they only apply to the SCSI bus,
> even though the QEMU driver ide-hd and ide-cd have support for the command 
> line argument -model.
> The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response 
> from the ATAPI spec.
> Words 27-46 is a 40 Char space for the model number of the device. The model 
> number is built by
> combining the vendor and the product fields with a single space as separator 
> in the middle.

If there are any formatting requirements (e.g. from this description it
seems that 'vendor' must not contain any space in order to work, then
you'll need to add code enforcing them.

> Therefore I would say, that vendor and product pretty much correlate to the 
> model field in QEMU,

This feels very subjective. Could you please add a better justification?
E.g. show how qemu formats the default vendor and product name?

> so the ide disk should also have the possibility of adding vendor and product 
> to it.


> The tests got changed to incorporate the new error message which now contains 
> that the ide and
> sata bus also supports vendor and product. Also the xml to command line tests 
> got updated, to
> have both fields present for testing.
> The command generator reordered a bit too, to only include disk->vendor and 
> disk->product into the
> json generation, when the scsi bus is selected. The same logic applies to the 
> ide and sata bus.

This bit above can be dropped

> Signed-off-by: Benedek Major 
> ---
>  src/qemu/qemu_command.c   | 21 ---
>  src/qemu/qemu_validate.c  |  6 --
>  .../disk-sata-device.x86_64-latest.args   |  2 +-
>  tests/qemuxml2argvdata/disk-sata-device.xml   |  2 ++
>  ...csi-disk-vpd-build-error.x86_64-latest.err |  2 +-
>  .../disk-sata-device.x86_64-latest.xml|  2 ++
>  6 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ad224571f3..903872091f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1943,8 +1943,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
>"p:physical_block_size", physical_block_size,
>"A:wwn", ,
>"p:rotation_rate", disk->rotation_rate,
> -  "S:vendor", disk->vendor,
> -  "S:product", disk->product,
>"T:removable", removable,
>"S:write-cache", qemuOnOffAuto(writeCache),
>"p:cyls", disk->geometry.cylinders,
> @@ -1956,7 +1954,24 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
>"S:rerror", rpolicy,
>NULL) < 0)
>  return NULL;
> -
> +if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
> +/* add vendor and product in SCSI section, since only SCSI supports
> + * vendor and product tag*/
> +if (virJSONValueObjectAdd(,
> +"S:vendor", disk->vendor,
> +"S:product", disk->product,

Please make your changes conform to the code formatting we use. E.g. the
line above should align all arguments below 

Ideally, to prevent code movement, use local variables to extract
vendor/product only if appropriate and initialize them to NULL

> +NULL) < 0)
> +return NULL;
> +}
> +if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE || disk->bus == 
> VIR_DOMAIN_DISK_BUS_SATA) {
> +/* Repurpose vendor and product, since IDE-Disk only supports model,
> + * which per ATAPI spec is the combination of vendor and product
> + * separated by a single space*/
> +if (virJSONValueObjectAdd(,
> +"S:model", g_strconcat(disk->vendor, " ", disk->product, 
> NULL),

g_strconcat allocates a new string, but the "S:" json building modifier
does NOT consume it, thus memory is leaked here. You'll need a temporary
variable. This aligns pretty well with the above non-movement of the
code where you can simply add a "S:model", modelstr, into the original
formatter and fill it conditionally.

Additionally the above ine would be misformatted otherwise.

> +NULL) < 0)
> +return NULL;

This also looks misformatted as it's returning if virJSONValueObjectAdd
fails.

> +}
>  return g_steal_pointer();
>  }
>  
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 7e09e2c52f..e7312bf789 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2871,9 +2871,11 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
> virDomainDiskDef *disk,
>  }
>  
>  if (disk->vendor || disk->product) {
> -if (disk->bus != 

[PATCH] Added support for vendor and product for qemu ide-hd

2023-07-17 Thread Benedek Major
The XML-Schema specifies two tags vendor and product for the disk type. But 
they only apply to the SCSI bus,
even though the QEMU driver ide-hd and ide-cd have support for the command line 
argument -model.
The model corresponds to words 27-46 of the IDENTIFY PACKET DEVICE response 
from the ATAPI spec.
Words 27-46 is a 40 Char space for the model number of the device. The model 
number is built by
combining the vendor and the product fields with a single space as separator in 
the middle.
Therefore I would say, that vendor and product pretty much correlate to the 
model field in QEMU,
so the ide disk should also have the possibility of adding vendor and product 
to it.
The tests got changed to incorporate the new error message which now contains 
that the ide and
sata bus also supports vendor and product. Also the xml to command line tests 
got updated, to
have both fields present for testing.
The command generator reordered a bit too, to only include disk->vendor and 
disk->product into the
json generation, when the scsi bus is selected. The same logic applies to the 
ide and sata bus.

Signed-off-by: Benedek Major 
---
 src/qemu/qemu_command.c   | 21 ---
 src/qemu/qemu_validate.c  |  6 --
 .../disk-sata-device.x86_64-latest.args   |  2 +-
 tests/qemuxml2argvdata/disk-sata-device.xml   |  2 ++
 ...csi-disk-vpd-build-error.x86_64-latest.err |  2 +-
 .../disk-sata-device.x86_64-latest.xml|  2 ++
 6 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ad224571f3..903872091f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1943,8 +1943,6 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
   "p:physical_block_size", physical_block_size,
   "A:wwn", ,
   "p:rotation_rate", disk->rotation_rate,
-  "S:vendor", disk->vendor,
-  "S:product", disk->product,
   "T:removable", removable,
   "S:write-cache", qemuOnOffAuto(writeCache),
   "p:cyls", disk->geometry.cylinders,
@@ -1956,7 +1954,24 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
   "S:rerror", rpolicy,
   NULL) < 0)
 return NULL;
-
+if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
+/* add vendor and product in SCSI section, since only SCSI supports
+ * vendor and product tag*/
+if (virJSONValueObjectAdd(,
+"S:vendor", disk->vendor,
+"S:product", disk->product,
+NULL) < 0)
+return NULL;
+}
+if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE || disk->bus == 
VIR_DOMAIN_DISK_BUS_SATA) {
+/* Repurpose vendor and product, since IDE-Disk only supports model,
+ * which per ATAPI spec is the combination of vendor and product
+ * separated by a single space*/
+if (virJSONValueObjectAdd(,
+"S:model", g_strconcat(disk->vendor, " ", disk->product, NULL),
+NULL) < 0)
+return NULL;
+}
 return g_steal_pointer();
 }
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 7e09e2c52f..e7312bf789 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2871,9 +2871,11 @@ qemuValidateDomainDeviceDefDiskFrontend(const 
virDomainDiskDef *disk,
 }
 
 if (disk->vendor || disk->product) {
-if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI) {
+if (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI
+&& disk->bus != VIR_DOMAIN_DISK_BUS_IDE
+&& disk->bus != VIR_DOMAIN_DISK_BUS_SATA) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("Only scsi disk supports vendor and product"));
+   _("Only scsi, sata and ide disk supports vendor and 
product"));
 return -1;
 }
 
diff --git a/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args 
b/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
index b60d9b16d4..296f8f1ee0 100644
--- a/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-sata-device.x86_64-latest.args
@@ -30,7 +30,7 @@ 
XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
 -device '{"driver":"ahci","id":"sata0","bus":"pci.0","addr":"0x2"}' \
 -blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUG 
uest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}'
 \
 -blockdev 
'{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}'
 \
--device 
'{"driver":"ide-hd","bus":"sata0.0","drive":"libvirt-1-format","id":"sata0-0-0","bootindex":1}'
 \
+-device 

Re: [PATCH] storage: zfs: Use 'zfs list' to check pool status

2023-07-17 Thread Peter Krempa
On Mon, Jul 03, 2023 at 16:53:28 -0600, Matt Low wrote:
> The current virtStorageBackendZFSCheckPool checks for the existence of a
> path under /dev/zvol/ to determine if the pool is active. ZFS does not
> create a path under /dev/zvol/ if no ZFS volumes have been created under
> a particular dataset, thus, empty ZFS storage pools are deactivated
> whenever checkPool is called on them (as noted in referenced issue).
> 
> This commit changes virStorageBackendZFSCheckPool so that the 'zfs list'
> command is used to explicitly check for the existence a dataset
> specified by the pool's def->source.name.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/221
> 
> Signed-off-by: Matt Low 
> ---
>  src/storage/storage_backend_zfs.c | 20 +---
>  1 file changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Peter Krempa 

I'll push it shortly.



Re: [libvirt PATCH] docs: expand clangd instructions

2023-07-17 Thread Michal Prívozník
On 7/14/23 17:44, Jonathon Jongsma wrote:
> Add some additional information about running clangd for LSP when clang
> is not your normal compiler.
> 
> Signed-off-by: Jonathon Jongsma 
> ---
>  docs/clangd.rst | 17 +
>  1 file changed, 17 insertions(+)
>
Reviewed-by: Michal Privoznik 

Michal



Re: [PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML

2023-07-17 Thread Boris Fiuczynski

On 7/17/23 10:12 AM, Boris Fiuczynski wrote:

Polite ping

On 7/7/23 11:17 AM, Boris Fiuczynski wrote:

On 7/6/23 9:40 PM, Jonathon Jongsma wrote:

On 6/30/23 6:34 AM, Boris Fiuczynski wrote:

Update the optional mdev attributes by running an mdevctl update on a
new created nodedev object representing an mdev.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
Signed-off-by: Boris Fiuczynski 
--- 


Just found that it was push to master last Thursday. Thanks.

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294



Re: [PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML

2023-07-17 Thread Boris Fiuczynski

Polite ping

On 7/7/23 11:17 AM, Boris Fiuczynski wrote:

On 7/6/23 9:40 PM, Jonathon Jongsma wrote:

On 6/30/23 6:34 AM, Boris Fiuczynski wrote:

Update the optional mdev attributes by running an mdevctl update on a
new created nodedev object representing an mdev.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
Signed-off-by: Boris Fiuczynski 
---
  src/node_device/node_device_udev.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/node_device/node_device_udev.c 
b/src/node_device/node_device_udev.c

index 4c37ec3189..fce4212728 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1757,12 +1757,20 @@ nodeStateCleanup(void)
  static int
  udevHandleOneDevice(struct udev_device *device)
  {
+    virNodeDevCapType dev_cap_type;
  const char *action = udev_device_get_action(device);
  VIR_DEBUG("udev action: '%s': %s", action, 
udev_device_get_syspath(device));

-    if (STREQ(action, "add") || STREQ(action, "change"))
-    return udevAddOneDevice(device);
+    if (STREQ(action, "add") || STREQ(action, "change")) {
+    int ret = udevAddOneDevice(device);
+    if (ret == 0 &&
+    udevGetDeviceType(device, _cap_type) == 0 &&
+    dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
+    if (nodeDeviceUpdateMediatedDevices() < 0)
+    VIR_WARN("mdevctl failed to update mediated devices");
+    return ret;
+    }
  if (STREQ(action, "remove"))
  return udevRemoveOneDevice(device);



So, this should work and roughly matches what we've done for other 
similar cases. But this is running from within the udev event handler 
thread, and theoretically there could already be an mdevctl thread 
running concurrently and updating the internal device list. The device 
list update functions are thread-safe so I don't think it'll corrupt 
anything, but it seems better to do all mdevctl updates from  mdevctl 
update thread rather than calling it directly here. The same actually 
applies to a couple other existing calls to 
nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that 
apply on top of yours and refactor things a bit. Let me know what you 
think.


Jonathon



LGTM and testing was successful.
See my direct replies on the patches.





--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294