Re: [RFC PATCH 4/6] Convert query-block/info_block to coroutine

[Lin Ma]

The query-named-block-nodes is only availabe for qmp, not support hmp yet.

Lin


From: Claudio Fontana 
Sent: Wednesday, May 24, 2023 4:49 PM
To: Fabiano Rosas; qemu-devel@nongnu.org
Cc: qemu-bl...@nongnu.org; Kevin Wolf; Hanna Reitz; Markus Armbruster; João 
Silva; Lin Ma; Dario Faggioli; Eric Blake
Subject: Re: [RFC PATCH 4/6] Convert query-block/info_block to coroutine

On 5/23/23 23:39, Fabiano Rosas wrote:
> From: Lin Ma 
>
> Sometimes the query-block performs time-consuming I/O(say waiting for
> the fstat of NFS complete), So let's make this QMP handler runs in a
> coroutine.
>
> The following patch moves the fstat() into a thread pool.
>
> Signed-off-by: Lin Ma 
> Signed-off-by: Fabiano Rosas 

Apart from the wrong subject,

why is this change not including the update to:

hmp-commands-info.hx

like the previous one?

Thanks,

C

> ---
>  blockdev.c   | 6 +++---
>  qapi/block-core.json | 3 ++-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5d56b79df4..6412548662 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2804,9 +2804,9 @@ void qmp_drive_backup(DriveBackup *backup, Error **errp)
>  blockdev_do_action(, errp);
>  }
>
> -BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
> - bool flat,
> - Error **errp)
> +BlockDeviceInfoList *coroutine_fn qmp_query_named_block_nodes(bool has_flat,
> +  bool flat,
> +  Error **errp)
>  {
>  bool return_flat = has_flat && flat;
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index da95fe456c..0559c38412 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1972,7 +1972,8 @@
>  { 'command': 'query-named-block-nodes',
>'returns': [ 'BlockDeviceInfo' ],
>'data': { '*flat': 'bool' },
> -  'allow-preconfig': true }
> +  'allow-preconfig': true,
> +  'coroutine': true}
>
>  ##
>  # @XDbgBlockGraphNodeType:

Re: [RFC PATCH 4/6] Convert query-block/info_block to coroutine

[Lin Ma]

The commit title/message are duplicated to previous one, Here should
use "query-named-block-nodes" instead.

Lin


From: Fabiano Rosas 
Sent: Wednesday, May 24, 2023 5:39 AM
To: qemu-devel@nongnu.org
Cc: qemu-bl...@nongnu.org; Kevin Wolf; Hanna Reitz; Markus Armbruster; João 
Silva; Lin Ma; Claudio Fontana; Dario Faggioli; Eric Blake
Subject: [RFC PATCH 4/6] Convert query-block/info_block to coroutine

From: Lin Ma 

Sometimes the query-block performs time-consuming I/O(say waiting for
the fstat of NFS complete), So let's make this QMP handler runs in a
coroutine.

The following patch moves the fstat() into a thread pool.

Signed-off-by: Lin Ma 
Signed-off-by: Fabiano Rosas 
---
 blockdev.c   | 6 +++---
 qapi/block-core.json | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5d56b79df4..6412548662 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2804,9 +2804,9 @@ void qmp_drive_backup(DriveBackup *backup, Error **errp)
 blockdev_do_action(, errp);
 }

-BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
- bool flat,
- Error **errp)
+BlockDeviceInfoList *coroutine_fn qmp_query_named_block_nodes(bool has_flat,
+  bool flat,
+  Error **errp)
 {
 bool return_flat = has_flat && flat;

diff --git a/qapi/block-core.json b/qapi/block-core.json
index da95fe456c..0559c38412 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1972,7 +1972,8 @@
 { 'command': 'query-named-block-nodes',
   'returns': [ 'BlockDeviceInfo' ],
   'data': { '*flat': 'bool' },
-  'allow-preconfig': true }
+  'allow-preconfig': true,
+  'coroutine': true}

 ##
 # @XDbgBlockGraphNodeType:
--
2.35.3

[PATCH v2] scsi-generic: replace logical block count of response of READ CAPACITY

[Lin Ma]

While using SCSI passthrough, Following scenario makes qemu doesn't
realized the capacity change of remote scsi target:
1. online resize the scsi target.
2. issue 'rescan-scsi-bus.sh -s ...' in host.
3. issue 'rescan-scsi-bus.sh -s ...' in vm.

In above scenario I used to experienced errors while accessing the
additional disk space in vm. I think the reasonable operations should
be:
1. online resize the scsi target.
2. issue 'rescan-scsi-bus.sh -s ...' in host.
3. issue 'block_resize' via qmp to notify qemu.
4. issue 'rescan-scsi-bus.sh -s ...' in vm.

The errors disappear once I notify qemu by block_resize via qmp.

So this patch replaces the number of logical blocks of READ CAPACITY
response from scsi target by qemu's bs->total_sectors. If the user in
vm wants to access the additional disk space, The administrator of
host must notify qemu once resizeing the scsi target.

Bonus is that domblkinfo of libvirt can reflect the consistent capacity
information between host and vm in case of missing block_resize in qemu.
E.g:
...

  
  
  
  
  
  

...

Before:
1. online resize the scsi target.
2. host:~  # rescan-scsi-bus.sh -s /dev/sdc
3. guest:~ # rescan-scsi-bus.sh -s /dev/sda
4  host:~  # virsh domblkinfo --domain $DOMAIN --human --device sda
Capacity:   4.000 GiB
Allocation: 0.000 B
Physical:   8.000 GiB

5. guest:~ # lsblk /dev/sda
NAME   MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda  8:00   8G  0 disk
└─sda1   8:10   2G  0 part

After:
1. online resize the scsi target.
2. host:~  # rescan-scsi-bus.sh -s /dev/sdc
3. guest:~ # rescan-scsi-bus.sh -s /dev/sda
4  host:~  # virsh domblkinfo --domain $DOMAIN --human --device sda
Capacity:   4.000 GiB
Allocation: 0.000 B
Physical:   8.000 GiB

5. guest:~ # lsblk /dev/sda
NAME   MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda  8:00   4G  0 disk
└─sda1   8:10   2G  0 part

Signed-off-by: Lin Ma 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/scsi/scsi-generic.c | 18 --
 hw/scsi/trace-events   |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 0306ccc7b1..c9b08a9926 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -256,6 +256,18 @@ static int 
scsi_generic_emulate_block_limits(SCSIGenericReq *r, SCSIDevice *s)
 return r->buflen;
 }
 
+static void refresh_max_lba(SCSIDevice *s)
+{
+BlockBackend *blk = s->conf.blk;
+BlockDriverState *bs = blk_bs(blk);
+uint64_t max_lba = bs->total_sectors - 1;
+
+if (max_lba != s->max_lba) {
+trace_scsi_generic_max_lba_refreshed(s->max_lba, max_lba);
+s->max_lba = max_lba;
+}
+}
+
 static void scsi_read_complete(void * opaque, int ret)
 {
 SCSIGenericReq *r = (SCSIGenericReq *)opaque;
@@ -315,11 +327,13 @@ static void scsi_read_complete(void * opaque, int ret)
 if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
 (ldl_be_p(>buf[0]) != 0xU || s->max_lba == 0)) {
 s->blocksize = ldl_be_p(>buf[4]);
-s->max_lba = ldl_be_p(>buf[0]) & 0xULL;
+refresh_max_lba(s);
+stl_be_p(>buf[0], s->max_lba);
 } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
(r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
 s->blocksize = ldl_be_p(>buf[8]);
-s->max_lba = ldq_be_p(>buf[0]);
+refresh_max_lba(s);
+stq_be_p(>buf[0], s->max_lba);
 }
 blk_set_guest_block_size(s->conf.blk, s->blocksize);
 
diff --git a/hw/scsi/trace-events b/hw/scsi/trace-events
index ae8551f279..44d064a656 100644
--- a/hw/scsi/trace-events
+++ b/hw/scsi/trace-events
@@ -352,3 +352,4 @@ scsi_generic_realize_blocksize(int blocksize) "block size 
%d"
 scsi_generic_aio_sgio_command(uint32_t tag, uint8_t cmd, uint32_t timeout) 
"generic aio sgio: tag=0x%x cmd=0x%x timeout=%u"
 scsi_generic_ioctl_sgio_command(uint8_t cmd, uint32_t timeout) "generic ioctl 
sgio: cmd=0x%x timeout=%u"
 scsi_generic_ioctl_sgio_done(uint8_t cmd, int ret, uint8_t status, uint8_t 
host_status) "generic ioctl sgio: cmd=0x%x ret=%d status=0x%x host_status=0x%x"
+scsi_generic_max_lba_refreshed(uint64_t old_max_lba, uint64_t new_max_lba) 
"old max_lba %" PRIu64 ",new max_lba %" PRIu64
-- 
2.26.2

[PATCH] scsi-generic: replace logical block count of response of READ CAPACITY

[Lin Ma]

While using SCSI passthrough, Following scenario makes qemu doesn't
realized the capacity change of remote scsi target:
1. online resize the scsi target.
2. issue 'rescan-scsi-bus.sh -s ...' in host.
3. issue 'rescan-scsi-bus.sh -s ...' in vm.

In above scenario I used to experienced errors while accessing the
additional disk space in vm. I think the reasonable operations should
be:
1. online resize the scsi target.
2. issue 'rescan-scsi-bus.sh -s ...' in host.
3. issue 'block_resize' via qmp to notify qemu.
4. issue 'rescan-scsi-bus.sh -s ...' in vm.

The errors disappear once I notify qemu by block_resize via qmp.

So this patch replaces the number of logical blocks of READ CAPACITY
response from scsi target by qemu's bs->total_sectors. If the user in
vm wants to access the additional disk space, The administrator of
host must notify qemu once resizeing the scsi target.

Bonus is that domblkinfo of libvirt can reflect the consistent capacity
information between host and vm in case of missing block_resize in qemu.
E.g:
...

  
  
  
  
  
  

...

Before:
1. online resize the scsi target.
2. host:~  # rescan-scsi-bus.sh -s /dev/sdc
3. guest:~ # rescan-scsi-bus.sh -s /dev/sda
4  host:~  # virsh domblkinfo --domain $DOMAIN --human --device sda
Capacity:   4.000 GiB
Allocation: 0.000 B
Physical:   8.000 GiB

5. guest:~ # lsblk /dev/sda
NAME   MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda  8:00   8G  0 disk
└─sda1   8:10   2G  0 part

After:
1. online resize the scsi target.
2. host:~  # rescan-scsi-bus.sh -s /dev/sdc
3. guest:~ # rescan-scsi-bus.sh -s /dev/sda
4  host:~  # virsh domblkinfo --domain $DOMAIN --human --device sda
Capacity:   4.000 GiB
Allocation: 0.000 B
Physical:   8.000 GiB

5. guest:~ # lsblk /dev/sda
NAME   MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda  8:00   4G  0 disk
└─sda1   8:10   2G  0 part

Signed-off-by: Lin Ma 
---
 hw/scsi/scsi-generic.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 0306ccc7b1..343b51c2c0 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -315,11 +315,17 @@ static void scsi_read_complete(void * opaque, int ret)
 if (r->req.cmd.buf[0] == READ_CAPACITY_10 &&
 (ldl_be_p(>buf[0]) != 0xU || s->max_lba == 0)) {
 s->blocksize = ldl_be_p(>buf[4]);
-s->max_lba = ldl_be_p(>buf[0]) & 0xULL;
+BlockBackend *blk = s->conf.blk;
+BlockDriverState *bs = blk_bs(blk);
+s->max_lba = bs->total_sectors - 1;
+stl_be_p(>buf[0], s->max_lba);
 } else if (r->req.cmd.buf[0] == SERVICE_ACTION_IN_16 &&
(r->req.cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
 s->blocksize = ldl_be_p(>buf[8]);
-s->max_lba = ldq_be_p(>buf[0]);
+BlockBackend *blk = s->conf.blk;
+BlockDriverState *bs = blk_bs(blk);
+s->max_lba = bs->total_sectors - 1;
+stq_be_p(>buf[0], s->max_lba);
 }
 blk_set_guest_block_size(s->conf.blk, s->blocksize);
 
-- 
2.26.2

[PATCH 3/3] tests: add postcopy-uffd-usermode-only capability into migration-test

[Lin Ma]

Signed-off-by: Lin Ma 
---
 tests/qtest/migration-test.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index cc5e83d98a..0cd4f49bed 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -38,6 +38,7 @@
 unsigned start_address;
 unsigned end_address;
 static bool uffd_feature_thread_id;
+static bool uffd_usermode_only;
 
 /* A downtime where the test really should converge */
 #define CONVERGE_DOWNTIME 1000
@@ -60,8 +61,12 @@ static bool ufd_version_check(void)
 int ufd = syscall(__NR_userfaultfd, O_CLOEXEC);
 
 if (ufd == -1) {
-g_test_message("Skipping test: userfaultfd not available");
-return false;
+ufd = syscall(__NR_userfaultfd, O_CLOEXEC | UFFD_USER_MODE_ONLY);
+if (ufd == -1) {
+   g_test_message("Skipping test: userfaultfd not available");
+return false;
+   } else
+uffd_usermode_only = true;
 }
 
 api_struct.api = UFFD_API;
@@ -670,6 +675,8 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
 }
 
 migrate_set_capability(from, "postcopy-ram", true);
+if (uffd_usermode_only)
+migrate_set_capability(to, "postcopy-uffd-usermode-only", true);
 migrate_set_capability(to, "postcopy-ram", true);
 migrate_set_capability(to, "postcopy-blocktime", true);
 
-- 
2.26.2

[PATCH 2/3] migration: postcopy-uffd-usermode-only documentation

[Lin Ma]

Signed-off-by: Lin Ma 
---
 docs/devel/migration.rst | 9 +
 1 file changed, 9 insertions(+)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 2401253482..dfdd3f20b4 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -639,6 +639,15 @@ postcopy-blocktime value of qmp command will show 
overlapped blocking
 time for all vCPU, postcopy-vcpu-blocktime will show list of blocking
 time per vCPU.
 
+Since kernel v5.11, Unprivileged user (without SYS_CAP_PTRACE capability)
+must pass UFFD_USER_MODE_ONLY to userfaultd if the unprivileged_userfaultfd
+sysctl knob is 0.
+
+To allow unprivileged user postcopy, Issue this command on destination
+monitor prior to turning on postcopy-ram:
+
+``migrate_set_capability postcopy-uffd-usermode-only on``
+
 .. note::
   During the postcopy phase, the bandwidth limits set using
   ``migrate_set_parameter`` is ignored (to avoid delaying requested pages that
-- 
2.26.2

[PATCH 1/3] migration: introduce postcopy-uffd-usermode-only capability

[Lin Ma]

The default value of unprivileged_userfaultfd sysctl knob was changed to
0 since kernel v5.11 by commit d0d4730a:
userfaultfd: add user-mode only option to unprivileged_userfaultfd sysctl knob.

In this mode, An unprivileged user (without SYS_CAP_PTRACE capability) must
pass UFFD_USER_MODE_ONLY to userfaultd or the API will fail with EPERM.

So add a capability to pass UFFD_USER_MODE_ONLY to support it.

Signed-off-by: Lin Ma 
---
 migration/migration.c|  9 +
 migration/migration.h|  1 +
 migration/postcopy-ram.c | 22 +++---
 qapi/migration.json  |  8 +++-
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 6ac807ef3d..86212dcb70 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2380,6 +2380,15 @@ bool migrate_postcopy_blocktime(void)
 return s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_BLOCKTIME];
 }
 
+bool migrate_postcopy_uffd_usermode_only(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+return 
s->enabled_capabilities[MIGRATION_CAPABILITY_POSTCOPY_UFFD_USERMODE_ONLY];
+}
+
 bool migrate_use_compression(void)
 {
 MigrationState *s;
diff --git a/migration/migration.h b/migration/migration.h
index 7a5aa8c2fd..a516d7f59f 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -358,6 +358,7 @@ int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
 bool migrate_background_snapshot(void);
+bool migrate_postcopy_uffd_usermode_only(void);
 
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_shut(MigrationIncomingState *mis,
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 2e9697bdd2..078c558626 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -206,9 +206,14 @@ static bool receive_ufd_features(uint64_t *features)
 struct uffdio_api api_struct = {0};
 int ufd;
 bool ret = true;
+int flags;
+
+flags = O_CLOEXEC;
+if (migrate_postcopy_uffd_usermode_only())
+flags |= UFFD_USER_MODE_ONLY;
 
 /* if we are here __NR_userfaultfd should exists */
-ufd = syscall(__NR_userfaultfd, O_CLOEXEC);
+ufd = syscall(__NR_userfaultfd, flags);
 if (ufd == -1) {
 error_report("%s: syscall __NR_userfaultfd failed: %s", __func__,
  strerror(errno));
@@ -352,13 +357,18 @@ bool 
postcopy_ram_supported_by_host(MigrationIncomingState *mis)
 struct uffdio_range range_struct;
 uint64_t feature_mask;
 Error *local_err = NULL;
+int flags;
 
 if (qemu_target_page_size() > pagesize) {
 error_report("Target page size bigger than host page size");
 goto out;
 }
 
-ufd = syscall(__NR_userfaultfd, O_CLOEXEC);
+flags = O_CLOEXEC;
+if (migrate_postcopy_uffd_usermode_only())
+flags |= UFFD_USER_MODE_ONLY;
+
+ufd = syscall(__NR_userfaultfd, flags);
 if (ufd == -1) {
 error_report("%s: userfaultfd not available: %s", __func__,
  strerror(errno));
@@ -1064,8 +1074,14 @@ retry:
 
 int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
 {
+int flags;
+
+flags = O_CLOEXEC | O_NONBLOCK;
+if (migrate_postcopy_uffd_usermode_only())
+flags |= UFFD_USER_MODE_ONLY;
+
 /* Open the fd for the kernel to give us userfaults */
-mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
+mis->userfault_fd = syscall(__NR_userfaultfd, flags);
 if (mis->userfault_fd == -1) {
 error_report("%s: Failed to open userfault fd: %s", __func__,
  strerror(errno));
diff --git a/qapi/migration.json b/qapi/migration.json
index 88f07baedd..3af1ec4cec 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -452,6 +452,11 @@
 #   procedure starts. The VM RAM is saved with running VM.
 #   (since 6.0)
 #
+# @postcopy-uffd-usermode-only: If enabled, It allows unprivileged users to use
+#   userfaultfd but with the restriction that page
+#   faults from only user mode can be handled.
+#   (since 6.2.0)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
@@ -459,7 +464,8 @@
'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
'block', 'return-path', 'pause-before-switchover', 'multifd',
'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
-   'x-ignore-shared', 'validate-uuid', 'background-snapshot'] }
+   'x-ignore-shared', 'validate-uuid', 'background-snapshot',
+   'postcopy-uffd-usermode-only'] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.26.2

[PATCH 0/3] Postcopy migration: Add userfaultfd- user-mode-only capability

[Lin Ma]

Since kernel v5.11, Unprivileged user (without SYS_CAP_PTRACE capability)
must pass UFFD_USER_MODE_ONLY to userfaultd in case unprivileged_userfaultfd
sysctl knob is 0.
Please refer to https://lwn.net/Articles/819834/ and the kernel commits:
37cd0575 userfaultfd: add UFFD_USER_MODE_ONLY
d0d4730a userfaultfd: add user-mode only option to unprivileged_userfaultfd 
sysctl knob

This patch set adds a migration capability to pass UFFD_USER_MODE_ONLY
for postcopy migration.

Lin Ma (3):
  migration: introduce postcopy-uffd-usermode-only capability
  migration: postcopy-uffd-usermode-only documentation
  tests: add postcopy-uffd-usermode-only capability into migration-test

 docs/devel/migration.rst |  9 +
 migration/migration.c|  9 +
 migration/migration.h|  1 +
 migration/postcopy-ram.c | 22 +++---
 qapi/migration.json  |  8 +++-
 tests/qtest/migration-test.c | 11 +--
 6 files changed, 54 insertions(+), 6 deletions(-)

-- 
2.26.2

Question about virtio-scsi max_segments setting

[Lin Ma]

Hi all,

We know that the seg_max of virtio-scsi respects to virtqueue size:
scsiconf->seg_max is set to virtqueue size - 2.

Some of my scsi HBAs have max_segments as low as 64 in host(max_sectors_kb is 
256).
When I use scsi lun passthrough disk in guest, In case of default
virtqueue size(256), The max_segments of the disk in guest is 254.

In this scenario:
host:~ # sg_vpd --page=bl /dev/sdb
Block limits VPD page (SBC):
  Write same non-zero (WSNZ): 0
  Maximum compare and write length: 0 blocks [Command not implemented]
  Optimal transfer length granularity: 512 blocks
  Maximum transfer length: 512 blocks
  Optimal transfer length: 512 blocks

host:~ # cat /sys/block/sdb/queue/max_sectors_kb
256
host:~ # cat /sys/block/sdb/queue/max_hw_sectors_kb
256
host:~ # cat /sys/block/sdb/queue/max_segments
64
host:~ # cat /sys/block/sdb/queue/max_hw_sectors_kb
256

guest:~ # sg_vpd --page=bl /dev/sdb
Block limits VPD page (SBC):
  Write same non-zero (WSNZ): 0
  Maximum compare and write length: 0 blocks [Command not implemented]
  Optimal transfer length granularity: 512 blocks
  Maximum transfer length: 512 blocks
  Optimal transfer length: 512 blocks

guest:~ # cat /sys/block/sdb/queue/max_sectors_kb 
256
guest:~ # cat /sys/block/sdb/queue/max_hw_sectors_kb
32767
guest:~ # cat /sys/block/sdb/queue/max_segments
254
guest:~ # cat /sys/block/sdb/queue/max_segment_size
65536

Performing "mkfs.xfs -f /dev/sdb" in guest triggers I/O errors.
The error message in stdout:
mkfs.xfs: libxfs_device_zero write failed: Remote I/O error

And the error message in dmesg:
[  887.867763] sd 6:0:0:2: [sdb] tag#147 FAILED Result: hostbyte=DID_OK 
driverbyte=DRIVER_SENSE
[  887.867766] sd 6:0:0:2: [sdb] tag#147 Sense Key : Illegal Request [current] 
[  887.867773] sd 6:0:0:2: [sdb] tag#147 Add. Sense: Invalid field in cdb
[  887.867775] sd 6:0:0:2: [sdb] tag#147 CDB: Write(10) 2a 00 0f a3 00 40 00 02 
00 00
[  887.867778] blk_update_request: critical target error, dev sdb, sector 
262340672 op 0x1:(WRITE) flags 0x8800 phys_seg 65 prio class 0


I found 2 ways to workaround the I/O error:
* Reduce the max_sectors_kb value in guest. E.g: 256->255 or lower.
* Modify virtio-scsi code to assign scsiconf->seg_max = max_hw_iov - 2
  (in my scenario, the max_hw_iov is 64)

I'm wondering that the seg_max always respects to virtqueue size, Is it
reasonable?
In case the max_segments of host scsi HBA is very low, say 64, Does the
max_segments=254 in guest make sense? Obviously the value exceed the
host limit, Does it cause I/O issue?

Thanks,
Lin

[REBASE] [PATCH v5] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
Reviewed-by: Marc-André Lureau 
---
 qga/commands-posix.c | 43 ++-
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 8dd94a3314..530c98344c 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2385,24 +2385,6 @@ error:
 return NULL;
 }
 
-#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
-
-static long sysconf_exact(int name, const char *name_str, Error **errp)
-{
-long ret;
-
-errno = 0;
-ret = sysconf(name);
-if (ret == -1) {
-if (errno == 0) {
-error_setg(errp, "sysconf(%s): value indefinite", name_str);
-} else {
-error_setg_errno(errp, errno, "sysconf(%s)", name_str);
-}
-}
-return ret;
-}
-
 /* Transfer online/offline status between @vcpu and the guest system.
  *
  * On input either @errp or *@errp must be NULL.
@@ -2473,30 +2455,33 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-int64_t current;
 GuestLogicalProcessorList *head, **tail;
-long sc_max;
+const char *cpu_dir = "/sys/devices/system/cpu";
+const gchar *line;
+g_autoptr(GDir) cpu_gdir = NULL;
 Error *local_err = NULL;
 
-current = 0;
 head = NULL;
 tail = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
 
-while (local_err == NULL && current < sc_max) {
-GuestLogicalProcessor *vcpu;
-int64_t id = current++;
-char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- id);
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
 
-if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
+GuestLogicalProcessor *vcpu;
+int64_t id;
+if (sscanf(line, "cpu%ld", )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
 transfer_vcpu(vcpu, true, path, _err);
 QAPI_LIST_APPEND(tail, vcpu);
 }
-g_free(path);
 }
 
 if (local_err == NULL) {
-- 
2.26.2

[PATCH v5] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 43 ++-
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..f66bf84126 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2376,24 +2376,6 @@ error:
 return NULL;
 }
 
-#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
-
-static long sysconf_exact(int name, const char *name_str, Error **errp)
-{
-long ret;
-
-errno = 0;
-ret = sysconf(name);
-if (ret == -1) {
-if (errno == 0) {
-error_setg(errp, "sysconf(%s): value indefinite", name_str);
-} else {
-error_setg_errno(errp, errno, "sysconf(%s)", name_str);
-}
-}
-return ret;
-}
-
 /* Transfer online/offline status between @vcpu and the guest system.
  *
  * On input either @errp or *@errp must be NULL.
@@ -2464,24 +2446,28 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+const char *cpu_dir = "/sys/devices/system/cpu";
+const gchar *line;
+g_autoptr(GDir) cpu_gdir = NULL;
 Error *local_err = NULL;
 
-current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
+
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
-int64_t id = current++;
-char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- id);
-
-if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+int64_t id;
+if (sscanf(line, "cpu%ld", )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
@@ -2491,7 +2477,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 *link = entry;
 link = >next;
 }
-g_free(path);
 }
 
 if (local_err == NULL) {
-- 
2.26.0

[PATCH v4] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 44 +++-
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..48dcdae239 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2376,24 +2376,6 @@ error:
 return NULL;
 }
 
-#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
-
-static long sysconf_exact(int name, const char *name_str, Error **errp)
-{
-long ret;
-
-errno = 0;
-ret = sysconf(name);
-if (ret == -1) {
-if (errno == 0) {
-error_setg(errp, "sysconf(%s): value indefinite", name_str);
-} else {
-error_setg_errno(errp, errno, "sysconf(%s)", name_str);
-}
-}
-return ret;
-}
-
 /* Transfer online/offline status between @vcpu and the guest system.
  *
  * On input either @errp or *@errp must be NULL.
@@ -2464,24 +2446,28 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+const char *cpu_dir = "/sys/devices/system/cpu";
+const gchar *line;
+GDir *cpu_gdir = NULL;
 Error *local_err = NULL;
 
-current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
+
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
-int64_t id = current++;
-char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- id);
-
-if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+int64_t id;
+if (sscanf(line, "cpu%ld", )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
@@ -2491,8 +2477,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 *link = entry;
 link = >next;
 }
-g_free(path);
 }
+g_dir_close(cpu_gdir);
 
 if (local_err == NULL) {
 /* there's no guest with zero VCPUs */
-- 
2.26.0

[PATCH v3] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 45 
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..ee05e694d3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2376,24 +2376,6 @@ error:
 return NULL;
 }
 
-#define SYSCONF_EXACT(name, errp) sysconf_exact((name), #name, (errp))
-
-static long sysconf_exact(int name, const char *name_str, Error **errp)
-{
-long ret;
-
-errno = 0;
-ret = sysconf(name);
-if (ret == -1) {
-if (errno == 0) {
-error_setg(errp, "sysconf(%s): value indefinite", name_str);
-} else {
-error_setg_errno(errp, errno, "sysconf(%s)", name_str);
-}
-}
-return ret;
-}
-
 /* Transfer online/offline status between @vcpu and the guest system.
  *
  * On input either @errp or *@errp must be NULL.
@@ -2464,24 +2446,29 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, 
bool sys2vcpu,
 
 GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
 {
-int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+g_autofree char *cpu_dir = NULL;
+const gchar *line;
+GDir *cpu_gdir = NULL;
 Error *local_err = NULL;
 
-current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+cpu_dir = g_strdup_printf("/sys/devices/system/cpu");
+cpu_gdir = g_dir_open(cpu_dir, 0, NULL);
+
+if (cpu_gdir == NULL) {
+error_setg_errno(errp, errno, "failed to list entries: %s", cpu_dir);
+return NULL;
+}
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && (line = g_dir_read_name(cpu_gdir)) != NULL) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
-int64_t id = current++;
-char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/",
- id);
-
-if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+int64_t id;
+if (sscanf(line, "cpu%ld", )) {
+g_autofree char *path = g_strdup_printf("/sys/devices/system/cpu/"
+"cpu%" PRId64 "/", id);
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
@@ -2491,8 +2478,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 *link = entry;
 link = >next;
 }
-g_free(path);
 }
+g_dir_close(cpu_gdir);
 
 if (local_err == NULL) {
 /* there's no guest with zero VCPUs */
-- 
2.26.0

[PATCH v2] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..d37525b5a3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2467,6 +2467,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 int64_t current;
 GuestLogicalProcessorList *head, **link;
 long sc_max;
+int i = 0;
 Error *local_err = NULL;
 
 current = 0;
@@ -2474,7 +2475,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 link = 
 sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && i < sc_max) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
 int64_t id = current++;
@@ -2482,6 +2483,7 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
  id);
 
 if (g_file_test(path, G_FILE_TEST_EXISTS)) {
+i++;
 vcpu = g_malloc0(sizeof *vcpu);
 vcpu->logical_id = id;
 vcpu->has_can_offline = true; /* lolspeak ftw */
-- 
2.26.0

Re: [PATCH] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

On 2020-11-19 14:46, Marc-André Lureau wrote:

Hi

On Thu, Nov 19, 2020 at 12:48 PM Lin Ma  wrote:


The guest-get-vcpus returns incorrect vcpu info in case we hotunplug
vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and
cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, 
"vcpus-count": 1,

 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, 
"vcpus-count": 1,

 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, 
"vcpus-count": 1,

 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, 
"vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": 
"host-x86_64-cpu"}

]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, 
"vcpus-count": 1,

 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, 
"vcpus-count": 1,

 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, 
"vcpus-count": 1,

 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, 
"vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": 
"host-x86_64-cpu"}

]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"execute":"guest-get-vcpus"}
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..accc893373 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2182,15 +2182,15 @@ GuestLogicalProcessorList
*qmp_guest_get_vcpus(Error **errp)
 {
 int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+long max_loop_count;
 Error *local_err = NULL;

 current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);

-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && current < max_loop_count) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
 int64_t id = current++;
@@ -2206,6 +2206,8 @@ GuestLogicalProcessorList 
*qmp_guest_get_vcpus(Error

**errp)
 entry->value = vcpu;
 *link = entry;
 link = >next;
+} else {
+max_loop_count += 1;



This looks like a recipe for infinite loop on error.

Emm...It is possible.


Shouldn't we loop over all the /sys/devices/system/cpu/cpu#/ instead?
Originally I'd like to use the function fnmatch to handle pattern cpu# 
to
loop over all of the /sys/devices/system/cpu/cpu#/, But it introduces 
the

header file fnmatch.h and make things complicated a little.



(possibly parse /sys/devices/system/cpu/present, but I doubt it's 
necessary)

IMO the 'present' won't help.

I'm about to post the V2, I made tiny change in the V2, Please help to 
review.


BTW, The local_err will be set in case of error, right? It could avoid 
infinite loop.


Thanks a lot,
Lin

[PATCH] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"execute":"guest-get-vcpus"}
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 3bffee99d4..accc893373 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2182,15 +2182,15 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 {
 int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+long max_loop_count;
 Error *local_err = NULL;
 
 current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && current < max_loop_count) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
 int64_t id = current++;
@@ -2206,6 +2206,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 entry->value = vcpu;
 *link = entry;
 link = >next;
+} else {
+max_loop_count += 1;
 }
 g_free(path);
 }
-- 
2.26.0

[PATCH] qga: Correct loop count in qmp_guest_get_vcpus()

[Lin Ma]

The guest-get-vcpus returns incorrect vcpu info in case we hotunplug vcpus(not
the last one).
e.g.:
A VM has 4 VCPUs: cpu0 + 3 hotunpluggable online vcpus(cpu1, cpu2 and cpu3).
Hotunplug cpu2,  Now only cpu0, cpu1 and cpu3 are present & online.

./qmp-shell /tmp/qmp-monitor.sock
(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu2", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

(QEMU) device_del id=cpu2
{"return": {}}

(QEMU) query-hotpluggable-cpus
{"return": [
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 3}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu3", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 2}, "vcpus-count": 1,
 "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 1}, "vcpus-count": 1,
 "qom-path": "/machine/peripheral/cpu1", "type": "host-x86_64-cpu"},
{"props": {"core-id": 0, "thread-id": 0, "socket-id": 0}, "vcpus-count": 1,
 "qom-path": "/machine/unattached/device[0]", "type": "host-x86_64-cpu"}
]}

Before:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1}]}

After:
./qmp-shell -N /tmp/qmp-ga.sock
Welcome to the QMP low-level shell!
Connected
(QEMU) guest-get-vcpus
{"execute":"guest-get-vcpus"}
{"return": [
{"online": true, "can-offline": false, "logical-id": 0},
{"online": true, "can-offline": true, "logical-id": 1},
{"online": true, "can-offline": true, "logical-id": 3}]}

Signed-off-by: Lin Ma 
---
 qga/commands-posix.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..6452e14a0f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2466,15 +2466,15 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 {
 int64_t current;
 GuestLogicalProcessorList *head, **link;
-long sc_max;
+long max_loop_count;
 Error *local_err = NULL;
 
 current = 0;
 head = NULL;
 link = 
-sc_max = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
+max_loop_count = SYSCONF_EXACT(_SC_NPROCESSORS_CONF, _err);
 
-while (local_err == NULL && current < sc_max) {
+while (local_err == NULL && current < max_loop_count) {
 GuestLogicalProcessor *vcpu;
 GuestLogicalProcessorList *entry;
 int64_t id = current++;
@@ -2491,6 +2491,8 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error 
**errp)
 *link = entry;
 link = >next;
 }
+else
+max_loop_count += 1;
 g_free(path);
 }
 
-- 
2.29.1

Re: The issue about adding multipath device's targets into qemu-pr-helper's namespace

[Lin Ma]

On 2020-07-14 13:30, Lin Ma wrote:

Hi all,

I have a namespace question about passthrough disk(multipath device).
In case of enabling namespace and cgroups in qemu.conf, The target(s) 
of the
multipath device won't be added into qemu-pr-helper's namespace under 
certain

situation, It causes the PERSISTENT RESERVE command failure in guest.

While user starts a vm,
To build namespace, The qemuDomainSetupDisk() will be invoked via 
threadA(this

thread id will be the qemu's pid),
To build cgroup, The qemuSetupImagePathCgroup() will be invoked via 
threadB.


Both of the functions invoke the virDevMapperGetTargets() trying to 
parse a

multipath device to target paths string, Then fill the targetPaths[].

The issue I experienced is:
After libvirtd started, Everything works well for the first booted vm 
which has

the passthrough multipath device.
But If I shut it down & start it again, OR keep it running & start 
another vm
which has other passthrough multipath device, Then the target(s) of the 
fresh
started vm won't be added into the related qemu-pr-helper's namespace 
and it

causes PERSISTENT RESERVE command failure in the corresponding guest.
I digged into code, In this situation, The targetPaths[] in
qemuDomainSetupDisk()
won't be filled, it keeps NULL after virDevMapperGetTargets() returns.
The virDevMapperGetTargets doesn't fill targetPaths[] because the 
dm_task_run()

of libdevmapper returns 0 with errno 9(Bad file descriptor).
So far, I don't understand why the dm_task_run() return 0 in this 
situation.
BTW, The virDevMapperGetTargets() can always successfully fill the 
targetPaths[]

in qemuSetupImagePathCgroup().

Please refer to the following 2 tests:
The multipath configuration on host:
host:~ # multipath -l
vm1-data (3600140582d9024bc13f4b8db5ff12de0) dm-11 FreeNAS,lv68
size=6.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=0 status=active
  `- 2:0:0:2 sdd 8:48 active undef running
vm2-data (36001405fc5f29ace3ec4fb8acd32aae5) dm-8 FreeNAS,lv46
size=4.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=0 status=active
  `- 2:0:0:1 sde 8:64 active undef running

===
Test A:
host:~ # systemctl restart libvirtd
host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh domblklist vm1
 Target   Source
--
 vda  /opt/vms/vm1/disk0.qcow2
 sda  /dev/mapper/vm1-data

host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
brw-rw 1 root disk 8, 48 Jul 14 16:30 /dev/sdd
host:~ # exit
exit
host:~ #

vm1:~ # lsscsi
[0:0:0:0]diskFreeNAS  lv68 0123   /dev/sda
vm1:~ #
vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
  PR generation=0x0, there are NO registered reservation keys
vm1:~ #

host:~ # virsh shutdown vm1
Domain vm1 is being shutdown

host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 2vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
ls: cannot access '/dev/sd*': No such file or directory
host:~ # exit
exit
host:~ #

vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
PR in (Read keys): Aborted command
Aborted command
vm1:~ #
===
Test B:
host:~ # systemctl restart libvirtd
host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh domblklist vm1
 Target   Source
--
 vda  /opt/vms/vm1/disk0.qcow2
 sda  /dev/mapper/vm1-data

host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
brw-rw 1 root disk 8, 48 Jul 14 17:28 /dev/sdd
host:~ # exit
exit
host:~ #

vm1:~ # lsscsi
[2:0:0:0]diskFreeNAS  lv68 0123   /dev/sda
vm1:~ #
vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
  PR generation=0x0, there are NO registered reservation keys
vm1:~ #

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ #
host:~ # virsh domblklist vm2
 Target   Source
--
 vda  /opt/vms/vm2/disk0.qcow2
 sda  /dev/mapper/vm2-data

host:~ #
host:~ # virsh start vm2
Domain vm2 started

host:~ # virsh list
 Id   NameState
---
 1vm1running
 

The issue about adding multipath device's targets into qemu-pr-helper's namespace

[Lin Ma]

Hi all,

I have a namespace question about passthrough disk(multipath device).
In case of enabling namespace and cgroups in qemu.conf, The target(s) of 
the
multipath device won't be added into qemu-pr-helper's namespace under 
certain

situation, It causes the PERSISTENT RESERVE command failure in guest.

While user starts a vm,
To build namespace, The qemuDomainSetupDisk() will be invoked via 
threadA(this

thread id will be the qemu's pid),
To build cgroup, The qemuSetupImagePathCgroup() will be invoked via 
threadB.


Both of the functions invoke the virDevMapperGetTargets() trying to 
parse a

multipath device to target paths string, Then fill the targetPaths[].

The issue I experienced is:
After libvirtd started, Everything works well for the first booted vm 
which has

the passthrough multipath device.
But If I shut it down & start it again, OR keep it running & start 
another vm
which has other passthrough multipath device, Then the target(s) of the 
fresh
started vm won't be added into the related qemu-pr-helper's namespace 
and it

causes PERSISTENT RESERVE command failure in the corresponding guest.
I digged into code, In this situation, The targetPaths[] in 
qemuDomainSetupDisk()

won't be filled, it keeps NULL after virDevMapperGetTargets() returns.
The virDevMapperGetTargets doesn't fill targetPaths[] because the 
dm_task_run()

of libdevmapper returns 0 with errno 9(Bad file descriptor).
So far, I don't understand why the dm_task_run() return 0 in this 
situation.
BTW, The virDevMapperGetTargets() can always successfully fill the 
targetPaths[]

in qemuSetupImagePathCgroup().

Please refer to the following 2 tests:
The multipath configuration on host:
host:~ # multipath -l
vm1-data (3600140582d9024bc13f4b8db5ff12de0) dm-11 FreeNAS,lv68
size=6.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=0 status=active
  `- 2:0:0:2 sdd 8:48 active undef running
vm2-data (36001405fc5f29ace3ec4fb8acd32aae5) dm-8 FreeNAS,lv46
size=4.0G features='0' hwhandler='1 alua' wp=rw
`-+- policy='service-time 0' prio=0 status=active
  `- 2:0:0:1 sde 8:64 active undef running

===
Test A:
host:~ # systemctl restart libvirtd
host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh domblklist vm1
 Target   Source
--
 vda  /opt/vms/vm1/disk0.qcow2
 sda  /dev/mapper/vm1-data

host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
brw-rw 1 root disk 8, 48 Jul 14 16:30 /dev/sdd
host:~ # exit
exit
host:~ #

vm1:~ # lsscsi
[0:0:0:0]diskFreeNAS  lv68 0123   /dev/sda
vm1:~ #
vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
  PR generation=0x0, there are NO registered reservation keys
vm1:~ #

host:~ # virsh shutdown vm1
Domain vm1 is being shutdown

host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 2vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
ls: cannot access '/dev/sd*': No such file or directory
host:~ # exit
exit
host:~ #

vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
PR in (Read keys): Aborted command
Aborted command
vm1:~ #
===
Test B:
host:~ # systemctl restart libvirtd
host:~ # virsh list
 Id   Name   State


host:~ #
host:~ # virsh domblklist vm1
 Target   Source
--
 vda  /opt/vms/vm1/disk0.qcow2
 sda  /dev/mapper/vm1-data

host:~ #
host:~ # virsh start vm1
Domain vm1 started

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ # nsenter -t $(pidof qemu-pr-helper) -a bash
host:~ # ls -l /dev/sd*
brw-rw 1 root disk 8, 48 Jul 14 17:28 /dev/sdd
host:~ # exit
exit
host:~ #

vm1:~ # lsscsi
[2:0:0:0]diskFreeNAS  lv68 0123   /dev/sda
vm1:~ #
vm1:~ # sg_persist --in -k /dev/sda
  FreeNAS   lv68  0123
  Peripheral device type: disk
  PR generation=0x0, there are NO registered reservation keys
vm1:~ #

host:~ # virsh list
 Id   NameState
---
 1vm1running

host:~ #
host:~ # virsh domblklist vm2
 Target   Source
--
 vda  /opt/vms/vm2/disk0.qcow2
 sda  /dev/mapper/vm2-data

host:~ #
host:~ # virsh start vm2
Domain vm2 started

host:~ # virsh list
 Id   NameState
---
 1vm1running
 2vm2running

host:~ # nsenter -t 

Re: Questions about online resizing a lun passthrough disk with virtio-scsi

[Lin Ma]

On 2020-07-09 12:00, Paolo Bonzini wrote:

On 09/07/20 13:52, Lin Ma wrote:

It's not recommended however, because block_resize will report the
change to the guest directly with a CAPACITY HAS CHANGED unit 
attention

condition.


Got it, The 'block_resize' is the recommended or necessary step, Even 
for

passthrough disk online resizing.


If your target is able to report the unit attention itself, it is okay
to skip it.  AFAIK drivers/target/ doesn't, though.


Got you.
I happen to use the drivers/target/ :-)

Thank you very much,
Lin

Re: Questions about online resizing a lun passthrough disk with virtio-scsi

[Lin Ma]

On 2020-07-08 15:11, Paolo Bonzini wrote:

On 08/07/20 16:44, lma wrote:


Is the 'block_resize' mandatory to notify guest os after online 
resizing
a lun passed through disk? I'm curious it because I found there're 
couple

of ways can make guest os realize the disk capacity change.
e.g:
* run 'block_resize' via qmp to let virtio-scsi notify the frontend 
about

  capacity change.
* run 'rescan-scsi-bus.sh -s' inside guest.
* run 'sg_readcap --16 /dev/sda' inside guest.

I knew that the purpose of 'block_resize' is not only to notify guest 
os,
but also to update some internal structure's member, say 
bs->total_sectors.
What if I forgot to run 'block_resize', but run 'rescan-scsi-bus.sh 
-s'

in guest?


Request start and length are checked even for passthrough disks (see
scsi_disk_dma_command in hw/scsi/scsi-disk.c, called by
scsi_block_dma_command), but the maximum LBA is snooped from READ
CAPACITY commands (see scsi_read_complete in hw/scsi/scsi-generic.c).
So as long as rescan-scsi-bus.sh results in a READ CAPACITY command, it
should work.


Yeah, the rescan-scsi-bus.sh does result in a READ CAPACITY command.


It's not recommended however, because block_resize will report the
change to the guest directly with a CAPACITY HAS CHANGED unit attention
condition.


Got it, The 'block_resize' is the recommended or necessary step, Even 
for

passthrough disk online resizing.

Thanks for your information,
Lin

回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

> -邮件原件-
> 发件人: Stefan Hajnoczi 
> 发送时间: 2020年6月22日 20:14
> 收件人: Lin Ma 
> 抄送: qemu-devel@nongnu.org; f...@euphon.net; kw...@redhat.com;
> mre...@redhat.com; pbonz...@redhat.com
> 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> command
> 
> On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> > Signed-off-by: Lin Ma 
> > ---
> >  hw/scsi/scsi-disk.c| 90
> ++
> >  include/block/accounting.h |  1 +
> >  include/scsi/constants.h   |  1 +
> >  3 files changed, 92 insertions(+)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index
> > 387503e11b..9e3002ddaf 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -1866,6 +1866,89 @@ static void
> scsi_disk_emulate_write_data(SCSIRequest *req)
> >  }
> >  }
> >
> > +typedef struct GetLbaStatusCBData {
> > +uint32_t num_blocks;
> > +uint32_t is_deallocated;
> > +SCSIDiskReq *r;
> > +} GetLbaStatusCBData;
> > +
> > +static void scsi_get_lba_status_complete(void *opaque, int ret);
> > +
> > +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData
> > +*data, int ret) {
> > +SCSIDiskReq *r = data->r;
> > +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> > +
> > +assert(r->req.aiocb == NULL);
> > +
> > +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
> > + s->qdev.blocksize,
> BLOCK_ACCT_GET_LBA_STATUS);
> > +
> > +r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> > +  r->req.cmd.lba *
> s->qdev.blocksize,
> > +  s->qdev.blocksize,
> > +
> > +scsi_get_lba_status_complete, data); }
> > +
> > +static void scsi_get_lba_status_complete(void *opaque, int ret) {
> > +GetLbaStatusCBData *data = opaque;
> > +SCSIDiskReq *r = data->r;
> > +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> > +
> > +assert(r->req.aiocb != NULL);
> > +r->req.aiocb = NULL;
> > +
> > +aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> > +if (scsi_disk_req_check_error(r, ret, true)) {
> > +g_free(data);
> > +goto done;
> > +}
> > +
> > +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
> > +scsi_req_unref(>req);
> > +g_free(data);
> > +
> > +done:
> > +aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> > +}
> > +
> > +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req,
> > +uint8_t *outbuf) {
> > +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> > +GetLbaStatusCBData *data;
> > +uint32_t *num_blocks;
> > +uint32_t *is_deallocated;
> > +
> > +data = g_new0(GetLbaStatusCBData, 1);
> > +data->r = r;
> > +num_blocks = &(data->num_blocks);
> > +is_deallocated = &(data->is_deallocated);
> > +
> > +scsi_req_ref(>req);
> > +scsi_get_lba_status_complete_noio(data, 0);
> 
> scsi_get_lba_status_complete_noio() looks asynchronous. If the BlockDriver
> yields in .bdrv_co_block_status() then the operation has not completed yet
> when scsi_get_lba_status_complete_noio() returns. It is not safe to access the
> GetLbaStatusCBData data until the async operation is complete.
> 
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.

Got it, I'll fill the outbuf[] in the completion function in V3.

> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in the 
> completion
> function before g_free(data) is called.

I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' 
and the
'bdrv_co_flush()', They look similiar, They called corresponding 
bs->drv->bdrv_co_*()
or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or 
bs->drv->bdrv_aio_pdiscard()
The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or 
bs->drv->bdrv_aio_flush().
The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains 
the
coroutine version of block_status, no aio version of block_status.

About "making this code asynchronous", Well, In fact I havn't reali

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

On 2020-06-25 21:25, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 8:14 PM
> ...
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index 387503e11b..9e3002ddaf 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest 
>> *req)
>>  }
>>  }
>>
>> +typedef struct GetLbaStatusCBData {
>> +uint32_t num_blocks;
>> +uint32_t is_deallocated;
>> +SCSIDiskReq *r;
>> +} GetLbaStatusCBData;
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret);
>> +
>> +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int 
>> ret)
>> +{
>> +SCSIDiskReq *r = data->r;
>> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> +assert(r->req.aiocb == NULL);
>> +
>> +block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
>> + s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
>> +
>> +r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
>> +  r->req.cmd.lba * 
>> s->qdev.blocksize,
>> +  s->qdev.blocksize,
>> +  scsi_get_lba_status_complete, 
>> data);
>> +}
>> +
>> +static void scsi_get_lba_status_complete(void *opaque, int ret)
>> +{
>> +GetLbaStatusCBData *data = opaque;
>> +SCSIDiskReq *r = data->r;
>> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
>> +
>> +assert(r->req.aiocb != NULL);
>> +r->req.aiocb = NULL;
>> +
>> +aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
>> +if (scsi_disk_req_check_error(r, ret, true)) {
>> +g_free(data);
>> +goto done;
>> +}
>> +
>> +block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
>> +scsi_req_unref(>req);
>> +g_free(data);
>> +
>> +done:
>> +aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
>> +}
>> +
>> +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t 
>> *outbuf)
>> +{
>> +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
>> +GetLbaStatusCBData *data;
>> +uint32_t *num_blocks;
>> +uint32_t *is_deallocated;
>> +
>> +data = g_new0(GetLbaStatusCBData, 1);
>> +data->r = r;
>> +num_blocks = &(data->num_blocks);
>> +is_deallocated = &(data->is_deallocated);
>> +
>> +scsi_req_ref(>req);
>> +scsi_get_lba_status_complete_noio(data, 0);
> 
> scsi_get_lba_status_complete_noio() looks asynchronous. If the
> BlockDriver yields in .bdrv_co_block_status() then the operation has not
> completed yet when scsi_get_lba_status_complete_noio() returns. It is
> not safe to access the GetLbaStatusCBData data until the async operation
> is complete.
> 
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.
> 
> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in
> the completion function before g_free(data) is called.

Emm, I need to dig into the code for better understanding your suggestion.
I used to think that I already completely make 
'scsi_get_lba_status_complete_noio'
asynchronous.

>> +
>> +/*
>> + * 8 + 16 is the length in bytes of response header and
>> + * one LBA status descriptor
>> + */
>> +memset(outbuf, 0, 8 + 16);
>> +outbuf[3] = 20;
>> +outbuf[8] = (req->cmd.lba >> 56) & 0xff;
>> +outbuf[9] = (req->cmd.lba >> 48) & 0xff;
>> +outbuf[10] = (req->cmd.lba >> 40) & 0xff;
>> +outbuf[11] = (req->cmd.lba >> 32) & 0xff;
>> +outbuf[12] = (req->cmd.lba >> 24) & 0xff;
>> +outbuf[13] = (req->cmd.lba >> 16) & 0xff;
>> +outbuf[14] = (req->cmd.lba >> 8) & 0xff;
>> +outbuf[15] = req->cmd.lba & 0xff;
>> +outbuf[16] = (*num_blocks >> 24) & 0xff;
>> +outbuf[17] = (*num_blocks >> 16) & 0xff;
>> +outbuf[18] = (*num_blocks >> 8) & 0xff;
>> +outbuf[19] = *num_blocks & 0xff;
>> +outbuf[20] = *is_deallocated ? 1 : 0;
> 
> SCSI de

Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

On 2020-06-25 20:59, Lin Ma wrote:
> From: Stefan Hajnoczi
> Sent: Monday, June 22, 2020 6:25 PM
> ...
>> diff --git a/block/io.c b/block/io.c
>> index df8f2a98d4..2064016b19 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2208,6 +2208,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
>> *child, int64_t offset,
>> BDRV_REQ_ZERO_WRITE | flags);
>>  }
>>
>> +int coroutine_fn
>> +bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
>> +   uint32_t *num_blocks, uint32_t *is_deallocated)
>
> Missing doc comments.

I'll add the comments.

>> +{
>> +BlockDriverState *bs = child->bs;
>
> Why does this function take a BdrvChild argument instead of
> BlockDriverState? Most I/O functions take BlockDriverState.

OK, I'll use BlockDriverState instead.

>> +int ret = 0;
>> +int64_t target_size, count = 0;
>> +bool first = true;
>> +uint8_t wanted_bit1 = 0;
>> +
>> +target_size = bdrv_getlength(bs);
>> +if (target_size < 0) {
>> +return -EIO;
>> +}
>> +
>> +if (offset < 0 || bytes < 0) {
>> +return -EIO;
>> +}
>> +
>> +for ( ; offset <= target_size - bytes; offset += count) {
>> +ret = bdrv_block_status(bs, offset, bytes, , NULL, NULL);
>> +if (ret < 0) {
>> +goto out;
>> +}
>> +if (first) {
>> +if (ret & BDRV_BLOCK_ZERO) {
>> +wanted_bit1 = BDRV_BLOCK_ZERO >> 1;
>> +*is_deallocated = 1;
>
> This is a boolean. Please use bool instead of uint32_t.

As you mentioned in comment of patch 3/3, is_deallocated boolean is not enough
to represent 3 states. I'll keep the uint32_t *, but rename 'is_deallocated' to
'provisioning_status'

> Please initialize is_deallocated to false at the beginning of the
> function to avoid accidental uninitialized variable accesses in the
> caller.

It has already been initialized to 0 by 'data = g_new0(GetLbaStatusCBData, 1);'
in function scsi_disk_emulate_get_lba_status of patch 3/3, Do I still need to
initialize it at the beginning of this function?

>> +} else {
>> +wanted_bit1 = 0;
>> +}
>> +first = false;
>> +}
>> +if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
>> +(*num_blocks)++;
>
> If there is a long span of allocated/deallocated blocks then this
> function only increments num_blocks once without counting the number of
> blocks. I expected something like num_blocks += pnum / block_size.  What
> is the relationship between bytes, count, and blocks in this function?

OK, I'll change it to '*num_blocks += pnum / bytes;'
The 'bytes' is the logical block size(default value is 512).
In fact, 'count' has the same meaning as 'pnum', It is the number of bytes
(including and immediately following the specified offset) that are known to
be in the same allocated/unallocated state. I'll rename the 'count' to 'pnum'.

Once this function returns, The number of contiguous logical blocks sharing
the same provisioning status as the specified logical block will be saved into
'num_blocks'.

>> +} else {
>> +break;
>> +}
>> +}
>> +out:
>> +return ret;
>> +}
>> +
>>  /*
>>   * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or 
>> not.
>>   */
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 791de6a59c..43f90591b9 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1296,6 +1296,11 @@ int coroutine_fn 
>> bdrv_co_block_status_from_backing(BlockDriverState *bs,
>> int64_t *pnum,
>> int64_t *map,
>> BlockDriverState **file);
>> +int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
>> +int64_t offset,
>> +int64_t bytes,
>> +uint32_t *num_blocks,
>> +uint32_t *is_deallocated);
>
> Should this function be in include/block/block.h (the public API) so
> that any part of QEMU can call it? It's not an internal API.

I'll move it to include/block/block.h.

[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.


In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0400  blocks: 32  deallocated

Lin Ma (3):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c  | 38 ++
 block/io.c | 43 
 hw/scsi/scsi-disk.c| 90 ++
 include/block/accounting.h |  1 +
 include/block/block_int.h  |  5 ++
 include/scsi/constants.h   |  1 +
 include/sysemu/block-backend.h |  2 +
 7 files changed, 180 insertions(+)

-- 
2.26.0

[PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma 
---
 hw/scsi/scsi-disk.c| 90 ++
 include/block/accounting.h |  1 +
 include/scsi/constants.h   |  1 +
 3 files changed, 92 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest 
*req)
 }
 }
 
+typedef struct GetLbaStatusCBData {
+uint32_t num_blocks;
+uint32_t is_deallocated;
+SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int 
ret)
+{
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb == NULL);
+
+block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
+ s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+  r->req.cmd.lba * s->qdev.blocksize,
+  s->qdev.blocksize,
+  scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+GetLbaStatusCBData *data = opaque;
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb != NULL);
+r->req.aiocb = NULL;
+
+aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+if (scsi_disk_req_check_error(r, ret, true)) {
+g_free(data);
+goto done;
+}
+
+block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
+scsi_req_unref(>req);
+g_free(data);
+
+done:
+aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+GetLbaStatusCBData *data;
+uint32_t *num_blocks;
+uint32_t *is_deallocated;
+
+data = g_new0(GetLbaStatusCBData, 1);
+data->r = r;
+num_blocks = &(data->num_blocks);
+is_deallocated = &(data->is_deallocated);
+
+scsi_req_ref(>req);
+scsi_get_lba_status_complete_noio(data, 0);
+
+/*
+ * 8 + 16 is the length in bytes of response header and
+ * one LBA status descriptor
+ */
+memset(outbuf, 0, 8 + 16);
+outbuf[3] = 20;
+outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+outbuf[15] = req->cmd.lba & 0xff;
+outbuf[16] = (*num_blocks >> 24) & 0xff;
+outbuf[17] = (*num_blocks >> 16) & 0xff;
+outbuf[18] = (*num_blocks >> 8) & 0xff;
+outbuf[19] = *num_blocks & 0xff;
+outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
 SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 
 /* Protection, exponent and lowest lba field left blank. */
 break;
+} else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+if (req->cmd.lba > s->qdev.max_lba) {
+goto illegal_lba;
+}
+scsi_disk_emulate_get_lba_status(req, outbuf);
+r->iov.iov_len = req->cmd.xfer;
+return r->iov.iov_len;
 }
 trace_scsi_disk_emulate_command_SAI_unsupported();
 goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
 BLOCK_ACCT_WRITE,
 BLOCK_ACCT_FLUSH,
 BLOCK_ACCT_UNMAP,
+BLOCK_ACCT_GET_LBA_STATUS,
 BLOCK_MAX_IOTYPE,
 };
 
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS0x12
 
 /*
  * READ POSITION service action codes
-- 
2.26.0

[PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

The get lba status wrapper based on the bdrv_block_status. The following
patches will add GET LBA STATUS 16 support for scsi emulation layer.

Signed-off-by: Lin Ma 
---
 block/io.c| 43 +++
 include/block/block_int.h |  5 +
 2 files changed, 48 insertions(+)

diff --git a/block/io.c b/block/io.c
index df8f2a98d4..2064016b19 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2208,6 +2208,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, 
int64_t offset,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
+int coroutine_fn
+bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
+   uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+BlockDriverState *bs = child->bs;
+int ret = 0;
+int64_t target_size, count = 0;
+bool first = true;
+uint8_t wanted_bit1 = 0;
+
+target_size = bdrv_getlength(bs);
+if (target_size < 0) {
+return -EIO;
+}
+
+if (offset < 0 || bytes < 0) {
+return -EIO;
+}
+
+for ( ; offset <= target_size - bytes; offset += count) {
+ret = bdrv_block_status(bs, offset, bytes, , NULL, NULL);
+if (ret < 0) {
+goto out;
+}
+if (first) {
+if (ret & BDRV_BLOCK_ZERO) {
+wanted_bit1 = BDRV_BLOCK_ZERO >> 1;
+*is_deallocated = 1;
+} else {
+wanted_bit1 = 0;
+}
+first = false;
+}
+if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
+(*num_blocks)++;
+} else {
+break;
+}
+}
+out:
+return ret;
+}
+
 /*
  * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
  */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c..43f90591b9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1296,6 +1296,11 @@ int coroutine_fn 
bdrv_co_block_status_from_backing(BlockDriverState *bs,
int64_t *pnum,
int64_t *map,
BlockDriverState **file);
+int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
+int64_t offset,
+int64_t bytes,
+uint32_t *num_blocks,
+uint32_t *is_deallocated);
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
 void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
 bool blk_dev_has_removable_media(BlockBackend *blk);
-- 
2.26.0

[PATCH v2 2/3] block: Add GET LBA STATUS support

[Lin Ma]

Signed-off-by: Lin Ma 
---
 block/block-backend.c  | 38 ++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..6d08dd5e0d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1650,6 +1650,44 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int 
bytes)
 return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
 }
 
+static int coroutine_fn
+blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+  uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+int ret;
+
+blk_wait_while_drained(blk);
+
+ret = blk_check_byte_request(blk, offset, bytes);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
+  is_deallocated);
+}
+
+static void blk_aio_get_lba_status_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+void *data = acb->common.opaque;
+uint32_t *num_blocks = (uint32_t *)data;
+uint32_t *is_deallocated = (uint32_t *)(data + sizeof(uint32_t));
+
+rwco->ret = blk_do_get_lba_status(rwco->blk, rwco->offset, acb->bytes,
+  num_blocks, is_deallocated);
+blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque)
+{
+return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_get_lba_status_entry,
+0, cb, opaque);
+}
+
 /* To be called between exactly one pair of blk_inc/dec_in_flight() */
 static int coroutine_fn blk_do_flush(BlockBackend *blk)
 {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..cd527ec0c9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,6 +171,8 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
  BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
-- 
2.26.0

[PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma 
---
 hw/scsi/scsi-disk.c| 90 ++
 include/block/accounting.h |  1 +
 include/scsi/constants.h   |  1 +
 3 files changed, 92 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..9e3002ddaf 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,89 @@ static void scsi_disk_emulate_write_data(SCSIRequest 
*req)
 }
 }
 
+typedef struct GetLbaStatusCBData {
+uint32_t num_blocks;
+uint32_t is_deallocated;
+SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int 
ret)
+{
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb == NULL);
+
+block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
+ s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+  r->req.cmd.lba * s->qdev.blocksize,
+  s->qdev.blocksize,
+  scsi_get_lba_status_complete, data);
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+GetLbaStatusCBData *data = opaque;
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb != NULL);
+r->req.aiocb = NULL;
+
+aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+if (scsi_disk_req_check_error(r, ret, true)) {
+g_free(data);
+goto done;
+}
+
+block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
+scsi_req_unref(>req);
+g_free(data);
+
+done:
+aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+GetLbaStatusCBData *data;
+uint32_t *num_blocks;
+uint32_t *is_deallocated;
+
+data = g_new0(GetLbaStatusCBData, 1);
+data->r = r;
+num_blocks = &(data->num_blocks);
+is_deallocated = &(data->is_deallocated);
+
+scsi_req_ref(>req);
+scsi_get_lba_status_complete_noio(data, 0);
+
+/*
+ * 8 + 16 is the length in bytes of response header and
+ * one LBA status descriptor
+ */
+memset(outbuf, 0, 8 + 16);
+outbuf[3] = 20;
+outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+outbuf[15] = req->cmd.lba & 0xff;
+outbuf[16] = (*num_blocks >> 24) & 0xff;
+outbuf[17] = (*num_blocks >> 16) & 0xff;
+outbuf[18] = (*num_blocks >> 8) & 0xff;
+outbuf[19] = *num_blocks & 0xff;
+outbuf[20] = *is_deallocated ? 1 : 0;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
 SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2159,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 
 /* Protection, exponent and lowest lba field left blank. */
 break;
+} else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+if (req->cmd.lba > s->qdev.max_lba) {
+goto illegal_lba;
+}
+scsi_disk_emulate_get_lba_status(req, outbuf);
+r->iov.iov_len = req->cmd.xfer;
+return r->iov.iov_len;
 }
 trace_scsi_disk_emulate_command_SAI_unsupported();
 goto illegal_request;
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
 BLOCK_ACCT_WRITE,
 BLOCK_ACCT_FLUSH,
 BLOCK_ACCT_UNMAP,
+BLOCK_ACCT_GET_LBA_STATUS,
 BLOCK_MAX_IOTYPE,
 };
 
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..b18377b214 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS0x12
 
 /*
  * READ POSITION service action codes
-- 
2.26.0

[PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Lin Ma]

The get lba status wrapper based on the bdrv_block_status. The following
patches will add GET LBA STATUS 16 support for scsi emulation layer.

Signed-off-by: Lin Ma 
---
 block/io.c| 43 +++
 include/block/block_int.h |  5 +
 2 files changed, 48 insertions(+)

diff --git a/block/io.c b/block/io.c
index df8f2a98d4..2064016b19 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2208,6 +2208,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, 
int64_t offset,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
+int coroutine_fn
+bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
+   uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+BlockDriverState *bs = child->bs;
+int ret = 0;
+int64_t target_size, count = 0;
+bool first = true;
+uint8_t wanted_bit1 = 0;
+
+target_size = bdrv_getlength(bs);
+if (target_size < 0) {
+return -EIO;
+}
+
+if (offset < 0 || bytes < 0) {
+return -EIO;
+}
+
+for ( ; offset <= target_size - bytes; offset += count) {
+ret = bdrv_block_status(bs, offset, bytes, , NULL, NULL);
+if (ret < 0) {
+goto out;
+}
+if (first) {
+if (ret & BDRV_BLOCK_ZERO) {
+wanted_bit1 = BDRV_BLOCK_ZERO >> 1;
+*is_deallocated = 1;
+} else {
+wanted_bit1 = 0;
+}
+first = false;
+}
+if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
+(*num_blocks)++;
+} else {
+break;
+}
+}
+out:
+return ret;
+}
+
 /*
  * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
  */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 791de6a59c..43f90591b9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1296,6 +1296,11 @@ int coroutine_fn 
bdrv_co_block_status_from_backing(BlockDriverState *bs,
int64_t *pnum,
int64_t *map,
BlockDriverState **file);
+int coroutine_fn bdrv_co_get_lba_status(BdrvChild *child,
+int64_t offset,
+int64_t bytes,
+uint32_t *num_blocks,
+uint32_t *is_deallocated);
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
 void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp);
 bool blk_dev_has_removable_media(BlockBackend *blk);
-- 
2.26.0

[PATCH v2 2/3] block: Add GET LBA STATUS support

[Lin Ma]

Signed-off-by: Lin Ma 
---
 block/block-backend.c  | 38 ++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..6d08dd5e0d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1650,6 +1650,44 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int 
bytes)
 return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
 }
 
+static int coroutine_fn
+blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+  uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+int ret;
+
+blk_wait_while_drained(blk);
+
+ret = blk_check_byte_request(blk, offset, bytes);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
+  is_deallocated);
+}
+
+static void blk_aio_get_lba_status_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+void *data = acb->common.opaque;
+uint32_t *num_blocks = (uint32_t *)data;
+uint32_t *is_deallocated = (uint32_t *)(data + sizeof(uint32_t));
+
+rwco->ret = blk_do_get_lba_status(rwco->blk, rwco->offset, acb->bytes,
+  num_blocks, is_deallocated);
+blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque)
+{
+return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_get_lba_status_entry,
+0, cb, opaque);
+}
+
 /* To be called between exactly one pair of blk_inc/dec_in_flight() */
 static int coroutine_fn blk_do_flush(BlockBackend *blk)
 {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..cd527ec0c9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,6 +171,8 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
  BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
-- 
2.26.0

[PATCH v2 0/3] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

v2->v1:
Follow Claudio's suggestions and the docker test result, Fix the dereferencing
'void *' pointer issues and the coding style issues.


In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0400  blocks: 32  deallocated

Lin Ma (3):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c  | 38 ++
 block/io.c | 43 
 hw/scsi/scsi-disk.c| 90 ++
 include/block/accounting.h |  1 +
 include/block/block_int.h  |  5 ++
 include/scsi/constants.h   |  1 +
 include/sysemu/block-backend.h |  2 +
 7 files changed, 180 insertions(+)

-- 
2.26.0

Re: [question] Partial sector issue while discard in qcow2 image

[Lin Ma]

Hi Kevin,

Thanks for the explanation!

Lin


From: Kevin Wolf 
Sent: Tuesday, June 2, 2020 7:06 PM
To: Lin Ma 
Cc: qemu-devel@nongnu.org ; pbonz...@redhat.com 

Subject: Re: [question] Partial sector issue while discard in qcow2 image

Am 02.06.2020 um 09:45 hat Lin Ma geschrieben:
> Hi all,
>
> During  woring to add GET LBA STATUS support in qemu scsi emulation
> layer, I encountered an unmap issue with qcow2 image, It's likely
> about how to unmap partial clusters. e.g.:
>
> With these default values:
> * the default value of s->qdev.blocksize: 512
> * the default value of s->cluster_size of qcow2 image: 65536
>
> Running 'sg_unmap -l 1024 -n 32 /dev/sda' hits the condition
> 'if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size))' in the
> function qcow2_co_pdiscard, Then it won't perform
> qcow2_cluster_discard(), So the corresponding clusters won't be
> discard in this situation, Correct?

Yes, this is correct. Discard is just a hint, so doing nothing is a
perfectly valid implementation. In the case of qcow2, only full clusters
are discarded.

> Of cause, with the default blocksize and cluster_size, The below
> examples won't hit the condition 'if (!QEMU_IS_ALIGNED...'.
> sg_unmap -l 1024 -n 128 /dev/sda
> or
> sg_unmap -l 256 -n 128 /dev/sda

Yes, and when discarding whole block devices (e.g. while creating a new
filesystem on them) or large files, you'll probably get this case for
most parts.

Kevin

[question] Partial sector issue while discard in qcow2 image

[Lin Ma]

Hi all,

During  woring to add GET LBA STATUS support in qemu scsi emulation layer, I 
encountered
an unmap issue with qcow2 image, It's likely about how to unmap partial 
clusters. e.g.:

With these default values:
* the default value of s->qdev.blocksize: 512
* the default value of s->cluster_size of qcow2 image: 65536

Running 'sg_unmap -l 1024 -n 32 /dev/sda' hits the condition
'if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size))' in the function 
qcow2_co_pdiscard,
Then it won't perform qcow2_cluster_discard(), So the corresponding clusters 
won't be discard
in this situation, Correct?

Of cause, with the default blocksize and cluster_size, The below examples won't 
hit the condition 'if (!QEMU_IS_ALIGNED...'.
sg_unmap -l 1024 -n 128 /dev/sda
or
sg_unmap -l 256 -n 128 /dev/sda

Thanks,
Lin

[PATCH 2/4] block: Add GET LBA STATUS support

[Lin Ma]

Signed-off-by: Lin Ma 
---
 block/block-backend.c  | 38 ++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c83..feb1f38b98 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1650,6 +1650,44 @@ int blk_pdiscard(BlockBackend *blk, int64_t offset, int 
bytes)
 return blk_prw(blk, offset, NULL, bytes, blk_pdiscard_entry, 0);
 }
 
+static int coroutine_fn
+blk_do_get_lba_status(BlockBackend *blk, int64_t offset, int bytes,
+  uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+int ret;
+
+blk_wait_while_drained(blk);
+
+ret = blk_check_byte_request(blk, offset, bytes);
+if (ret < 0) {
+return ret;
+}
+
+return bdrv_co_get_lba_status(blk->root, offset, bytes, num_blocks,
+  is_deallocated);
+}
+
+static void blk_aio_get_lba_status_entry(void *opaque)
+{
+BlkAioEmAIOCB *acb = opaque;
+BlkRwCo *rwco = >rwco;
+
+void *data = acb->common.opaque;
+uint32_t *num_blocks = [0];
+uint32_t *is_deallocated = [sizeof(uint32_t)];
+
+rwco->ret = blk_do_get_lba_status(rwco->blk, rwco->offset, acb->bytes,
+  num_blocks, is_deallocated);
+blk_aio_complete(acb);
+}
+
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque)
+{
+return blk_aio_prwv(blk, offset, bytes, NULL, blk_aio_get_lba_status_entry,
+0, cb, opaque);
+}
+
 /* To be called between exactly one pair of blk_inc/dec_in_flight() */
 static int coroutine_fn blk_do_flush(BlockBackend *blk)
 {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8203d7f6f9..cd527ec0c9 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -171,6 +171,8 @@ BlockAIOCB *blk_aio_flush(BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque);
 BlockAIOCB *blk_aio_pdiscard(BlockBackend *blk, int64_t offset, int bytes,
  BlockCompletionFunc *cb, void *opaque);
+BlockAIOCB *blk_aio_get_lba_status(BlockBackend *blk, int64_t offset, int 
bytes,
+   BlockCompletionFunc *cb, void *opaque);
 void blk_aio_cancel(BlockAIOCB *acb);
 void blk_aio_cancel_async(BlockAIOCB *acb);
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf);
-- 
2.24.0

[PATCH 4/4] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

Signed-off-by: Lin Ma 
---
 hw/scsi/scsi-disk.c  | 92 
 include/scsi/constants.h |  1 +
 2 files changed, 93 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 387503e11b..2d2c6b4b82 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1866,6 +1866,91 @@ static void scsi_disk_emulate_write_data(SCSIRequest 
*req)
 }
 }
 
+typedef struct GetLbaStatusCBData {
+uint32_t num_blocks;
+uint32_t is_deallocated;
+SCSIDiskReq *r;
+} GetLbaStatusCBData;
+
+static void scsi_get_lba_status_complete(void *opaque, int ret);
+
+static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData *data, int 
ret)
+{
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb == NULL);
+
+block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
+ s->qdev.blocksize, BLOCK_ACCT_GET_LBA_STATUS);
+
+r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
+  r->req.cmd.lba * s->qdev.blocksize,
+  s->qdev.blocksize,
+  scsi_get_lba_status_complete, data);
+return;
+}
+
+static void scsi_get_lba_status_complete(void *opaque, int ret)
+{
+GetLbaStatusCBData *data = opaque;
+SCSIDiskReq *r = data->r;
+SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
+
+assert(r->req.aiocb != NULL);
+r->req.aiocb = NULL;
+
+aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
+if (scsi_disk_req_check_error(r, ret, true)) {
+g_free(data);
+goto done;
+}
+
+block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
+scsi_req_unref(>req);
+g_free(data);
+
+done:
+aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
+}
+
+static void scsi_disk_emulate_get_lba_status(SCSIRequest *req, uint8_t *outbuf)
+{
+SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
+GetLbaStatusCBData *data;
+uint32_t *num_blocks;
+uint32_t *is_deallocated;
+
+data = g_new0(GetLbaStatusCBData, 1);
+data->r = r;
+num_blocks = &(data->num_blocks);
+is_deallocated = &(data->is_deallocated);
+
+scsi_req_ref(>req);
+scsi_get_lba_status_complete_noio(data, 0);
+
+/* 8 + 16 is the length in bytes of response header and
+ * one LBA status descriptor
+ */
+memset(outbuf, 0, 8 + 16);
+outbuf[3] = 20;
+outbuf[8] = (req->cmd.lba >> 56) & 0xff;
+outbuf[9] = (req->cmd.lba >> 48) & 0xff;
+outbuf[10] = (req->cmd.lba >> 40) & 0xff;
+outbuf[11] = (req->cmd.lba >> 32) & 0xff;
+outbuf[12] = (req->cmd.lba >> 24) & 0xff;
+outbuf[13] = (req->cmd.lba >> 16) & 0xff;
+outbuf[14] = (req->cmd.lba >> 8) & 0xff;
+outbuf[15] = req->cmd.lba & 0xff;
+outbuf[16] = (*num_blocks >> 24) & 0xff;
+outbuf[17] = (*num_blocks >> 16) & 0xff;
+outbuf[18] = (*num_blocks >> 8) & 0xff;
+outbuf[19] = *num_blocks & 0xff;
+outbuf[20] = *is_deallocated ? 1 : 0;
+
+return;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
 SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2076,6 +2161,13 @@ static int32_t scsi_disk_emulate_command(SCSIRequest 
*req, uint8_t *buf)
 
 /* Protection, exponent and lowest lba field left blank. */
 break;
+} else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
+if (req->cmd.lba > s->qdev.max_lba) {
+goto illegal_lba;
+}
+scsi_disk_emulate_get_lba_status(req, outbuf);
+r->iov.iov_len = req->cmd.xfer;
+return r->iov.iov_len;
 }
 trace_scsi_disk_emulate_command_SAI_unsupported();
 goto illegal_request;
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 874176019e..1b6417898a 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -154,6 +154,7 @@
  * SERVICE ACTION IN subcodes
  */
 #define SAI_READ_CAPACITY_16  0x10
+#define SAI_GET_LBA_STATUS  0x12
 
 /*
  * READ POSITION service action codes
-- 
2.24.0

[PATCH 3/4] block: Add block accounting code for GET LBA STATUS

[Lin Ma]

Signed-off-by: Lin Ma 
---
 include/block/accounting.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..645014fb0b 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -38,6 +38,7 @@ enum BlockAcctType {
 BLOCK_ACCT_WRITE,
 BLOCK_ACCT_FLUSH,
 BLOCK_ACCT_UNMAP,
+BLOCK_ACCT_GET_LBA_STATUS,
 BLOCK_MAX_IOTYPE,
 };
 
-- 
2.24.0

[PATCH 0/4] Add Support for GET LBA STATUS 16 command in scsi emulation

[Lin Ma]

In this current design, The GET LBA STATUS parameter data only contains
an eight-byte header + one LBA status descriptor.

How to test:
host:~ # qemu-system-x86_64 \
...
-drive file=/vm0/disk0.raw,format=raw,if=none,id=drive0,discard=unmap \
-device scsi-hd,id=scsi0,drive=drive0 \
...


guest:~ # dd if=/dev/zero of=/dev/sda bs=512 seek=1024 count=256

guest:~ # sg_unmap -l 1024 -n 32 /dev/sda

guest:~ # sg_get_lba_status /dev/sda -l 1024
No indication of the completion condition
RTP=0
descriptor LBA: 0x0400  blocks: 32  deallocated

Lin Ma (4):
  block: Add bdrv_co_get_lba_status
  block: Add GET LBA STATUS support
  block: Add block accounting code for GET LBA STATUS
  scsi-disk: Add support for the GET LBA STATUS 16 command

 block/block-backend.c  | 38 ++
 block/io.c | 43 
 hw/scsi/scsi-disk.c| 92 ++
 include/block/accounting.h |  1 +
 include/scsi/constants.h   |  1 +
 include/sysemu/block-backend.h |  2 +
 6 files changed, 177 insertions(+)

-- 
2.24.0

[PATCH 1/4] block: Add bdrv_co_get_lba_status

[Lin Ma]

The get lba status wrapper based on the bdrv_block_status. The following
patches will add GET LBA STATUS 16 support for scsi emulation layer.

Signed-off-by: Lin Ma 
---
 block/io.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/block/io.c b/block/io.c
index 121ce17a49..dacc3c2471 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2186,6 +2186,49 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, 
int64_t offset,
BDRV_REQ_ZERO_WRITE | flags);
 }
 
+int coroutine_fn
+bdrv_co_get_lba_status(BdrvChild *child, int64_t offset, int64_t bytes,
+   uint32_t *num_blocks, uint32_t *is_deallocated)
+{
+BlockDriverState *bs = child->bs;
+int ret;
+int64_t target_size, count = 0;
+bool first = true;
+uint8_t wanted_bit1 = 0;
+
+target_size = bdrv_getlength(bs);
+if (target_size < 0) {
+return -EIO;
+}
+
+if (offset < 0 || bytes < 0) {
+return -EIO;
+}
+
+for ( ; offset <= target_size - bytes; offset += count) {
+ret = bdrv_block_status(bs, offset, bytes, , NULL, NULL);
+if (ret < 0) {
+goto out;
+}
+if (first) {
+if (ret & BDRV_BLOCK_ZERO) {
+wanted_bit1 = BDRV_BLOCK_ZERO >> 1;;
+*is_deallocated = 1;
+} else {
+wanted_bit1 = 0;
+}
+first = false;
+}
+if ((ret & BDRV_BLOCK_ZERO) >> 1 == wanted_bit1) {
+(*num_blocks)++;
+} else {
+break;
+}
+}
+out:
+return ret;
+}
+
 /*
  * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
  */
-- 
2.24.0

Re: [Question] About GET LBA STATUS(16) support in qemu scsi emulation layer

[Lin Ma]

Due to no users complain about it so far, It seems not many people need it.
I'll spend some time reading some code of qemu block and diving into SCSI 
command manual, then try to implement it.
If anyone's willing to implement it before this, it will be appreciated.

Thanks for your valuable information,
Lin


From: Stefan Hajnoczi
Sent: Tuesday, March 10, 2020 5:24 PM
To: Lin Ma
Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; h...@suse.de; Eric Blake; Jon 
Maloy; qemu-bl...@nongnu.org
Subject: Re: [Question] About GET LBA STATUS(16) support in qemu scsi emulation 
layer

On Tue, Mar 10, 2020 at 06:18:00AM +, Lin Ma wrote:
> First of all, Thanks for your reply, Stefan.
>
> We know that the GET LBA STATUS works well under scsi lun passthrough due to 
> the vm directly talks to the scsi target.
> I'm curious that if I use file backend image(say qcow2) + qemu scsi 
> emulation, Does it make sense if I issue sg_get_lba_status in vm to get the 
> lba status?
> If it doesn't make sense, That could explain why qemu scsi emulation layer 
> lack of this support for a long time and no user complains.

It does make sense to implement GET LBA STATUS because QEMU emulates the
UNMAP command.

Be careful though because there is no asynchronous bdrv_block_status()
API yet.  Internally the BlockDriver->bdrv_co_block_status() function is
already asynchronous because it runs in a coroutine.  It will be
necessary to expose a new bdrv_aio_get_block_status() or similar API so
the device models (i.e. SCSI emulation code) can take advantage of that.

Stefan

Re: [Question] About GET LBA STATUS(16) support in qemu scsi emulation layer

[Lin Ma]

First of all, Thanks for your reply, Stefan.

We know that the GET LBA STATUS works well under scsi lun passthrough due to 
the vm directly talks to the scsi target.
I'm curious that if I use file backend image(say qcow2) + qemu scsi emulation, 
Does it make sense if I issue sg_get_lba_status in vm to get the lba status?
If it doesn't make sense, That could explain why qemu scsi emulation layer lack 
of this support for a long time and no user complains.

Thanks,
Lin

From: Stefan Hajnoczi
Sent: Friday, March 6, 2020 9:01 PM
To: Lin Ma
Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; h...@suse.de; Eric Blake; Jon 
Maloy
Subject: Re: [Question] About GET LBA STATUS(16) support in qemu scsi emulation 
layer

On Sun, Mar 01, 2020 at 01:01:29PM +, Lin Ma wrote:
> Hi all,
>
> I'm not familiar with scsi, I'm curious why there is no GET LBA STATUS(16) 
> support in qemu scsi emulation layer.
>
> So far, There is only one subcommand of SERVICE ACTION was implemented: The 
> READ CAPACITY(16)
> e.g.
> static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> [..]
> case SERVICE_ACTION_IN_16:
> /* Service Action In subcommands. */
> if ((req->cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
> [..]
> }
> trace_scsi_disk_emulate_command_SAI_unsupported();
> goto illegal_request;
> [..]
>
>
> It seems that this situation has been for a long time. Is the GET LBA STATUS 
> (16 or 32) unnessesary for qemu scsi emulation or did I misunderstand 
> something?

GET LBA STATUS is optional according to the SBC specification so QEMU's
SCSI target is conformant.

I guess the question is which applications need this command?

It's probably a case of no one needing this command enough to implement
it yet.

Stefan

[Question] About GET LBA STATUS(16) support in qemu scsi emulation layer

[Lin Ma]

Hi all,

I'm not familiar with scsi, I'm curious why there is no GET LBA STATUS(16) 
support in qemu scsi emulation layer.

So far, There is only one subcommand of SERVICE ACTION was implemented: The 
READ CAPACITY(16)
e.g.
static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
[..]
case SERVICE_ACTION_IN_16:
/* Service Action In subcommands. */
if ((req->cmd.buf[1] & 31) == SAI_READ_CAPACITY_16) {
[..]
}
trace_scsi_disk_emulate_command_SAI_unsupported();
goto illegal_request;
[..]


It seems that this situation has been for a long time. Is the GET LBA STATUS 
(16 or 32) unnessesary for qemu scsi emulation or did I misunderstand something?

TIA,
Lin

[Qemu-devel] 回复: 回复: 答复: migrate_set_speed has no effect if the guest is using hugepages.

[Lin Ma]

Hi Dave,

May I ask that do you have any update about the fix?

Thanks,
Lin

> -邮件原件-
> 发件人: Qemu-devel  代
> 表 Lin Ma
> 发送时间: 2019年7月15日 17:43
> 收件人: Dr. David Alan Gilbert 
> 抄送: qemu-devel@nongnu.org
> 主题: [Qemu-devel] 回复: 答复: migrate_set_speed has no effect if the guest
> is using hugepages.
> 
> 
> 
> > -邮件原件-
> > 发件人: Dr. David Alan Gilbert 
> > 发送时间: 2019年7月12日 20:34
> > 收件人: Lin Ma 
> > 抄送: qemu-devel@nongnu.org
> > 主题: Re: 答复: [Qemu-devel] migrate_set_speed has no effect if the guest
> > is using hugepages.
> >
> > * Lin Ma (l...@suse.com) wrote:
> > >
> > >
> > > > -邮件原件-
> > > > 发件人: Dr. David Alan Gilbert 
> > > > 发送时间: 2019年7月11日 18:24
> > > > 收件人: Lin Ma 
> > > > 抄送: qemu-devel@nongnu.org
> > > > 主题: Re: [Qemu-devel] migrate_set_speed has no effect if the guest
> > > > is using hugepages.
> > > >
> > > > * Lin Ma (l...@suse.com) wrote:
> > > > > Hi all,
> > > >
> > > > Hi Lin,
> > >
> > > Hi Dave,
> > > >
> > > > > When I live migrate a qemu/kvm guest, If the guest is using huge
> > > > > pages, I found that the migrate_set_speed command had no effect
> > > > > during
> > > > stage 2.
> > > >
> > > > Can you explain what you mean by 'stage 2'?
> > > We know that the live migration contains 3 stages:
> > > Stage 1: Mark all of RAM dirty.
> > > Stage 2: Keep sending dirty RAM pages since last iteration Stage 3:
> > > Stop guest, transfer remaining dirty RAM, device state (Please refer
> > > to
> > > https://developers.redhat.com/blog/2015/03/24/live-migrating-qemu-kv
> > > m- virtual-machines/#live-migration for further details)
> >
> > OK, yeh the numbering is pretty arbitrary so it's not something I
> > normally think about like that.
> >
> > >
> > > > > It was caused by commit 4c011c3 postcopy: Send whole huge pages
> > > > >
> > > > > I'm wondering that is it by design or is it a bug waiting for fix?
> > > >
> > > > This is the first report I've seen for it.  How did you conclude
> > > > that
> > > > 4c011c3 caused it?  While I can see it might have some effect on
> > > > the bandwidth management, I'm surprised it has this much effect.
> > >
> > > While digging into the bandwidth issue, Git bisect shows that this
> > > commit was
> > the first bad commit.
> >
> > OK.
> >
> > > > What size huge pages are you using - 2MB or 1GB?
> > >
> > > When I hit this issue I was using 1GB huge page size.
> > > I tested this issue with 2MB page size today On Gigabit LAN,
> > > Although the bandwidth control looks a little better than using 1GB, But 
> > > not
> too much.
> > Please refer to the below test result.
> >
> > OK, I can certainly see why this might happen with 1GB huge pages; I
> > need to have a think about a fix.
> >
> > > > I can imagine we might have a problem that since we only do the
> > > > sleep between the hugepages, if we were using 1GB hugepages then
> > > > we'd see  > > > data>[sleep][sleep] which isn't as smooth as it
> > > > data>used to
> > be.
> > > >
> > > > Can you give me some more details of your test?
> > >
> > > Live migration bandwidth management testing with 2MB hugepage size:
> > > sles12sp4_i440fx is a qemu/kvm guest with 6GB memory size.
> > > Note: the throughput value is approximating value.
> > >
> > > Terminal 1:
> > > virsh migrate-setspeed sles12sp4_i440fx $bandwidth && virsh migrate
> > > --live sles12sp4_i440fx qemu+tcp://5810f/system
> > >
> > > Terminal 2:
> > > virsh qemu-monitor-command sles12sp4_i440fx --hmp "info migrate"
> > >
> > > bandwidth=5
> > > throughput: 160 mbps
> > >
> > > bandwidth=10
> > > throughput: 167 mbps
> > >
> > > bandwidth=15
> > > throughput: 168 mbps
> > >
> > > bandwidth=20
> > > throughput: 168 mbps
> > >
> > > bandwidth=21
> > > throughput: 336 mbps
> > >
> > > bandwidth=22
> > > throughput: 336 mbps
> > >
> > > bandwidth=25
> > > throughput: 335.87 mbps
> > >
> > > bandwidth=30
> > > throughput: 335 mbps
> > >
> > > bandwidth=35
> > > throughput: 335 mbps
> > >
> > > bandwidth=40
> > > throughput: 335 mbps
> > >
> > > bandwidth=45
> > > throughput: 504.00 mbps
> > >
> > > bandwidth=50
> > > throughput: 500.00 mbps
> > >
> > > bandwidth=55
> > > throughput: 500.00 mbps
> > >
> > > bandwidth=60
> > > throughput: 500.00 mbps
> > >
> > > bandwidth=65
> > > throughput: 650.00 mbps
> > >
> > > bandwidth=70
> > > throughput: 660.00 mbps
> >
> > OK, so migrate-setspeed takes a bandwidth in MBytes/sec and I guess
> > you're throughput is in MBit/sec - so at the higher end it's about
> > right, and at the lower end it's way off.
> >
> > Let me think about a fix for this.
> >
> > What are you using to measure throughput?
> 
> I use 'watch' command to observe the output of qemu hmp command 'info
> migrate', calculate the average value of throughput field during stage 2 of 
> live
> migration.
> 
> Thanks for taking time to dig into this issue, Lin

[Qemu-devel] 回复: 答复: migrate_set_speed has no effect if the guest is using hugepages.

[Lin Ma]

> -邮件原件-
> 发件人: Dr. David Alan Gilbert 
> 发送时间: 2019年7月12日 20:34
> 收件人: Lin Ma 
> 抄送: qemu-devel@nongnu.org
> 主题: Re: 答复: [Qemu-devel] migrate_set_speed has no effect if the guest is
> using hugepages.
> 
> * Lin Ma (l...@suse.com) wrote:
> >
> >
> > > -邮件原件-
> > > 发件人: Dr. David Alan Gilbert 
> > > 发送时间: 2019年7月11日 18:24
> > > 收件人: Lin Ma 
> > > 抄送: qemu-devel@nongnu.org
> > > 主题: Re: [Qemu-devel] migrate_set_speed has no effect if the guest is
> > > using hugepages.
> > >
> > > * Lin Ma (l...@suse.com) wrote:
> > > > Hi all,
> > >
> > > Hi Lin,
> >
> > Hi Dave,
> > >
> > > > When I live migrate a qemu/kvm guest, If the guest is using huge
> > > > pages, I found that the migrate_set_speed command had no effect
> > > > during
> > > stage 2.
> > >
> > > Can you explain what you mean by 'stage 2'?
> > We know that the live migration contains 3 stages:
> > Stage 1: Mark all of RAM dirty.
> > Stage 2: Keep sending dirty RAM pages since last iteration Stage 3:
> > Stop guest, transfer remaining dirty RAM, device state (Please refer
> > to
> > https://developers.redhat.com/blog/2015/03/24/live-migrating-qemu-kvm-
> > virtual-machines/#live-migration for further details)
> 
> OK, yeh the numbering is pretty arbitrary so it's not something I normally 
> think
> about like that.
> 
> >
> > > > It was caused by commit 4c011c3 postcopy: Send whole huge pages
> > > >
> > > > I'm wondering that is it by design or is it a bug waiting for fix?
> > >
> > > This is the first report I've seen for it.  How did you conclude
> > > that
> > > 4c011c3 caused it?  While I can see it might have some effect on the
> > > bandwidth management, I'm surprised it has this much effect.
> >
> > While digging into the bandwidth issue, Git bisect shows that this commit 
> > was
> the first bad commit.
> 
> OK.
> 
> > > What size huge pages are you using - 2MB or 1GB?
> >
> > When I hit this issue I was using 1GB huge page size.
> > I tested this issue with 2MB page size today On Gigabit LAN, Although
> > the bandwidth control looks a little better than using 1GB, But not too 
> > much.
> Please refer to the below test result.
> 
> OK, I can certainly see why this might happen with 1GB huge pages; I need to
> have a think about a fix.
> 
> > > I can imagine we might have a problem that since we only do the
> > > sleep between the hugepages, if we were using 1GB hugepages then
> > > we'd see  > > data>[sleep][sleep] which isn't as smooth as it used to
> be.
> > >
> > > Can you give me some more details of your test?
> >
> > Live migration bandwidth management testing with 2MB hugepage size:
> > sles12sp4_i440fx is a qemu/kvm guest with 6GB memory size.
> > Note: the throughput value is approximating value.
> >
> > Terminal 1:
> > virsh migrate-setspeed sles12sp4_i440fx $bandwidth && virsh migrate
> > --live sles12sp4_i440fx qemu+tcp://5810f/system
> >
> > Terminal 2:
> > virsh qemu-monitor-command sles12sp4_i440fx --hmp "info migrate"
> >
> > bandwidth=5
> > throughput: 160 mbps
> >
> > bandwidth=10
> > throughput: 167 mbps
> >
> > bandwidth=15
> > throughput: 168 mbps
> >
> > bandwidth=20
> > throughput: 168 mbps
> >
> > bandwidth=21
> > throughput: 336 mbps
> >
> > bandwidth=22
> > throughput: 336 mbps
> >
> > bandwidth=25
> > throughput: 335.87 mbps
> >
> > bandwidth=30
> > throughput: 335 mbps
> >
> > bandwidth=35
> > throughput: 335 mbps
> >
> > bandwidth=40
> > throughput: 335 mbps
> >
> > bandwidth=45
> > throughput: 504.00 mbps
> >
> > bandwidth=50
> > throughput: 500.00 mbps
> >
> > bandwidth=55
> > throughput: 500.00 mbps
> >
> > bandwidth=60
> > throughput: 500.00 mbps
> >
> > bandwidth=65
> > throughput: 650.00 mbps
> >
> > bandwidth=70
> > throughput: 660.00 mbps
> 
> OK, so migrate-setspeed takes a bandwidth in MBytes/sec and I guess you're
> throughput is in MBit/sec - so at the higher end it's about right, and at the 
> lower
> end it's way off.
> 
> Let me think about a fix for this.
> 
> What are you using to measure throughput?

I use 'watch' command to observe the output of qemu hmp command 'info migrate', 
calculate the
average value of throughput field during stage 2 of live migration.

Thanks for taking time to dig into this issue,
Lin

[Qemu-devel] migrate_set_speed has no effect if the guest is using hugepages.

[Lin Ma]

Hi all,

When I live migrate a qemu/kvm guest, If the guest is using huge pages, I found 
that
the migrate_set_speed command had no effect during stage 2.
It was caused by commit 4c011c3 postcopy: Send whole huge pages

I'm wondering that is it by design or is it a bug waiting for fix?


Thanks,
Lin

Re: [Qemu-devel] About pt-br keyboard layout

[Lin Ma]

On 4/26/19 7:48 PM, Gerd Hoffmann wrote:

So -k must match the *hosts* keyboard layout.

So, that means, If I launch qemu with -k pt-br on a host, I need to ensure
the keyboard layout
must be pt-br as well onthathypervisorhost,Then when I connected to this vnc
server(qemu)
throughvncviewer on mylaptop(us keyboard layout), I type shift + 6, I can
get¨(diaeresis)
character, am I right?

No.  I've simplified a bit, assuming both vncviewer and qemu run on the
host machine.  -k $layout must match the keyboard layout of the machine
running vncviewer.  So, in your case it should be us (the laptops
layout).

:-)  Thank you very much!
Lin

Re: [Qemu-devel] About pt-br keyboard layout

[Lin Ma]

On 4/26/19 4:38 PM, Gerd Hoffmann wrote:

On Fri, Apr 26, 2019 at 04:09:12PM +0800, Lin Ma wrote:

Hi all,

While I launch qemu with vnc + pt-br keyboard layout on my pc, If I type
shift + 6 in iPXE shell or grub shell via my usual 105-key keyboard,
shift + 6 would be mapped to "(apostrophe), But IIUC the correct character
should be ¨(diaeresis) in pt-br layout.

I'm wondering that is it the expected behavior? In other words, Does it make
sense if I want to type multi-character characters(say Ç or ¨) without guest
os's help?

No.  -k is for translating keysyms back into the correct scancodes,
because this is what the (virtual) keyboard passes to the guest.
So -k must match the *hosts* keyboard layout.
So, that means, If I launch qemu with -k pt-br on a host, I need to 
ensure the keyboard layout
must be pt-br as well onthathypervisorhost,Then when I connected to this 
vnc server(qemu)
throughvncviewer on mylaptop(us keyboard layout), I type shift + 6, I 
can get¨(diaeresis)

character, am I right?

Translating the scancodes into keysyms again is the job of the guest,
so that requires the keymapping you want to use being active in the
guest os.  Typically that would be the same you have on the host, but
that isn't required.  The guest can work with -- for example -- us
layout (like it is often the case in boot loaders).  Behavior should be
identical to physical hardware here.

HTH,
   Gerd

[Qemu-devel] About pt-br keyboard layout

[Lin Ma]

Hi all,

While I launch qemu with vnc + pt-br keyboard layout on my pc, If I type 
shift + 6 in iPXE shell or grub shell via my usual 105-key keyboard,
shift + 6 would be mapped to "(apostrophe), But IIUC the correct 
character should be ¨(diaeresis) in pt-br layout.


I'm wondering that is it the expected behavior? In other words, Does it 
make sense if I want to type multi-character characters(say Ç or ¨) 
without guest os's help?




About pt-br keyboard (ABNT2), please refer to:
http://www.cjdinfo.com.br/utilitario-tabela-caracteres
https://docs.microsoft.com/en-us/globalization/keyboards/kbdbr_2.html


Any feedback would be appreciated,
Lin

[Qemu-devel] [PATCH v3] net: Fix a potential segfault

[Lin Ma]

If user forgets to provide any backend types for '-netdev' in qemu CLI,
It triggers seg fault.

e.g.

Expected:
$ qemu -netdev id=net0
qemu-system-x86_64: Parameter 'type' is missing

Actual:
$ qemu -netdev id=net0
Segmentation fault (core dumped)

Signed-off-by: Lin Ma 
---
 net/net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/net.c b/net/net.c
index efb9eaf779..2a3133990c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1093,7 +1093,9 @@ static int net_client_init(QemuOpts *opts, bool 
is_netdev, Error **errp)
 int ret = -1;
 Visitor *v = opts_visitor_new(opts);
 
-if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
+const char *type = qemu_opt_get(opts, "type");
+
+if (is_netdev && type && is_help_option(type)) {
 show_netdevs();
 exit(0);
 } else {
-- 
2.16.2

[Qemu-devel] [PATCH v2] net: Fix a potential segfault

[Lin Ma]

If user forgets to provide any backend types for '-netdev' in qemu CLI,
It triggers seg fault.

e.g.

Expected:
$ qemu -netdev id=net0
qemu-system-x86_64: Parameter 'type' is missing

Actual:
$ qemu -netdev id=net0
Segmentation fault (core dumped)

Signed-off-by: Lin Ma 
---
 net/net.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index efb9eaf779..f89790be4a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1093,9 +1093,12 @@ static int net_client_init(QemuOpts *opts, bool 
is_netdev, Error **errp)
 int ret = -1;
 Visitor *v = opts_visitor_new(opts);
 
-if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
-show_netdevs();
-exit(0);
+if (is_netdev) {
+const char *type = qemu_opt_get(opts, "type");
+if (type && is_help_option(type)) {
+show_netdevs();
+exit(0);
+}
 } else {
 /* Parse convenience option format ip6-net=fec0::0[/64] */
 const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
-- 
2.16.2

[Qemu-devel] [PATCH] net: Fix a potential segfault

[Lin Ma]

Signed-off-by: Lin Ma 
---
 net/net.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index efb9eaf779..f89790be4a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1093,9 +1093,12 @@ static int net_client_init(QemuOpts *opts, bool 
is_netdev, Error **errp)
 int ret = -1;
 Visitor *v = opts_visitor_new(opts);
 
-if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
-show_netdevs();
-exit(0);
+if (is_netdev) {
+const char *type = qemu_opt_get(opts, "type");
+if (type && is_help_option(type)) {
+show_netdevs();
+exit(0);
+}
 } else {
 /* Parse convenience option format ip6-net=fec0::0[/64] */
 const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
-- 
2.16.2

Re: [Qemu-devel] [PATCH] raw: Disable probing if image format is given by driver-specific options

[Lin Ma]

On 01/23/2018 12:45 AM, Max Reitz wrote:

On 2018-01-22 08:21, Lin Ma wrote:

If the user specifies image format through driver-specific options, The
format probing should be prohibited and the warning message should not
be printed.

e.g.:
$ qemu-system-x86_64 ... -drive file.file.filename=disk0.raw,file.driver=raw ...
WARNING: Image format was not specified for 'disk0.raw' and probing guessed raw.
  Automatically detecting the format is dangerous for raw images, ...

Signed-off-by: Lin Ma <l...@suse.com>
---
  blockdev.c | 2 ++
  1 file changed, 2 insertions(+)

But what has been proped is the driver for the root BDS, so the warning
is actually correct.

(That command line is creating a chain of three BDS:
raw (probed) -> raw (explicitly specified) -> file (default protocol))

Max


OK, Thanks
Lin

[Qemu-devel] [PATCH] raw: Disable probing if image format is given by driver-specific options

[Lin Ma]

If the user specifies image format through driver-specific options, The
format probing should be prohibited and the warning message should not
be printed.

e.g.:
$ qemu-system-x86_64 ... -drive file.file.filename=disk0.raw,file.driver=raw ...
WARNING: Image format was not specified for 'disk0.raw' and probing guessed raw.
 Automatically detecting the format is dangerous for raw images, ...

Signed-off-by: Lin Ma <l...@suse.com>
---
 blockdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 29d569a24e..ef4c167235 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -529,6 +529,8 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 goto early_err;
 }
 qdict_put_str(bs_opts, "driver", buf);
+} else if (qdict_haskey(bs_opts, "file.driver")) {
+qdict_put_str(bs_opts, "driver", qdict_get_str(bs_opts, 
"file.driver"));
 }
 
 on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
-- 
2.15.1

[Qemu-devel] [PATCH] build: Fix typo in scripts/git-submodule.sh

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 scripts/git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
index bc7224a..807ca0b 100755
--- a/scripts/git-submodule.sh
+++ b/scripts/git-submodule.sh
@@ -28,7 +28,7 @@ error() {
 echo
 echo "and then manually update submodules prior to running make, with:"
 echo
-echo " $ scripts/git-sbumodule.sh update $modules"
+echo " $ scripts/git-submodule.sh update $modules"
 echo
 exit 1
 }
-- 
2.9.2

[Qemu-devel] [PATCH] configure: add the missing help output for optional features

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 configure | 12 
 1 file changed, 12 insertions(+)

diff --git a/configure b/configure
index 6c21975..2603e10 100755
--- a/configure
+++ b/configure
@@ -1335,6 +1335,12 @@ Advanced options (experts only):
   --with-vss-sdk=SDK-path  enable Windows VSS support in QEMU Guest Agent
   --with-win-sdk=SDK-path  path to Windows Platform SDK (to build VSS .tlb)
   --tls-priority   default TLS protocol/cipher priority string
+  --enable-gprof   QEMU profiling with gprof
+  --enable-profilerprofiler support
+  --enable-xen-pv-domain-build
+   xen pv domain builder
+  --enable-debug-stack-usage
+   track the maximum stack usage of stacks created by 
qemu_alloc_stack
 
 Optional features, enabled with --enable-FEATURE and
 disabled with --disable-FEATURE, default is enabled if available:
@@ -1403,6 +1409,12 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   tcmalloctcmalloc support
   jemallocjemalloc support
   replication replication support
+  vhost-vsock virtio sockets device support
+  opengl  opengl support
+  virglrenderer   virgl rendering support
+  xfsctl  xfsctl support
+  qom-cast-debug  cast debugging support
+  tools   build qemu-io, qemu-nbd and qemu-image tools
 
 NOTE: The object files are built at the place where configure is launched
 EOF
-- 
2.9.2

Re: [Qemu-devel] 答复： Re: [RFC] virtio-fc: draft idea of virtual fibre channel HBA

[Lin Ma]

Hi Hannes,

>>> Hannes Reinecke <h...@suse.de> 2017/2/16 星期四 下午 5:56 >>>
>On 02/16/2017 09:39 AM, Paolo Bonzini wrote:
>> 
>> 
>> On 16/02/2017 08:16, Lin Ma wrote:
>>>> What are the benefits of having FC access from the guest?
>>>
>>> Actually, I havn't dug it too much, Just thought that from virtualization's
>>> perspective, when interact with FC storage, having complete FC access
>>> from the guest is the way it should use.
>> 
>> How much of this requires a completely new spec?  Could we get enough of
>> the benefit (e.g. the ability to issue rescans or LIPs on the host) by
>> extending virtio-scsi?
>> 
>I guess I'd need to chime in with my favourite topic :-)
>
>Initially I really would go with extending the virtio-scsi spec, as
>'real' virtio-fc suffers from some issues:
>While it's of course possible to implement a full fc stack in qemu
>itself, it's not easily possible assign 'real' FC devices to the guest.
>Problem here is that any virtio-fc would basically have to use the
>standard FC frame format for virtio itself, but all 'real' FC HBAs
>(excluding FCoE drivers) do _not_ allow access to the actual FC frames,
>but rather present you with a 'cooked' version of them.
>So if you were attempting to pass FC devices to the guest you would have
>to implement yet-another conversion between raw FC frames and the
>version the HBA would like.
>And that doesn't even deal with the real complexity like generating
>correct OXID/RXIDs etc.
>
>So initially I would propose to update the virtio spec to include:
>- Full 64bit LUNs
>- A 64bit target port ID (well, _actually_ it should be a SCSI-compliant
>  target port ID, but as this essentially is a free text field I'd
>  restrict it to something sensible)
>For full compability we'd also need a (64-bit) initiator ID, but that is
>essentially a property of the virtio queue, so we don't need to transmit
>it with every command; once during queue setup is enough.
>And if we restrict virtio-scsi to point-to-point topology we can even
>associate the target port ID with the virtio queue, making
>implementation even easier.
>I'm not sure if that is a good idea long-term, as one might want to
>identify an NPIV host with a virtio device, in which case we're having a
>1-M topology and that model won't work anymore.
>
>To be precise:
>
>I'd propose to update
>
>struct virtio_scsi_config
>with a field 'u8 initiator_id[8]'
>
>and
>
>struct virtio_scsi_req_cmd
>with a field 'u8 target_id[8]'
>
>and do away with the weird LUN remapping qemu has nowadays.
Does it mean we dont need to provide '-drive' and '-device scsi-hd'
option in qemu command line? so the guest can get its own LUNs
through fc switch, right?

>That should be enough to get proper NPIV passthrough working.

Lin

Re: [Qemu-devel] 答复： Re: [RFC] virtio-fc: draft idea of virtual fibre channel HBA

[Lin Ma]

>>> Paolo Bonzini <pbonz...@redhat.com> 2/16/2017 4:39 下午 >>>
>
>
>On 16/02/2017 08:16, Lin Ma wrote:
>>> What are the benefits of having FC access from the guest?
>> 
>> Actually, I havn't dug it too much, Just thought that from virtualization's
>> perspective, when interact with FC storage, having complete FC access
>> from the guest is the way it should use.
>
>How much of this requires a completely new spec?  Could we get enough of
>the benefit (e.g. the ability to issue rescans or LIPs on the host) by
>extending virtio-scsi?

I understand, It needs too much stuff for building such an all new device. 
Furthermore, from performance's perspective, Extending virtio-scsi way doesn't
involve in the fc fabric level, leave it to physical HBA to handle, may get 
better
performance.
 
Lin

[Qemu-devel] 答复： Re: [PATCH v2 RESEND] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

>>> Paolo Bonzini <pbonz...@redhat.com> 2017/2/13 星期一 下午 6:07 >>>
>
>
>On 13/02/2017 10:51, Lin Ma wrote:
>> By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
>> In 'make' stage a qemu-version.h.tmp will be generated. If the content of
>> qemu-version.h.tmp and qemu-version.h aren't consistent, The 
>> qemu-version.h.tmp
>> will be renamed to qemu-version.h. Because of the target FORCE, The same 
>> action
>> will be do again in 'make install' stage.
>
>But why does the content of .h and .h.tmp not match during "make install"?
The content of qemu-version.h recorded the git head info of last build.
After 'git pull && make ', Because the content of qemu-version.h.tmp is 
generated
based on the lastest git describe, Now this .h.tmp and the old .h aren't 
consistent,
So this .h.tmp will be renamed to qemu-version.h.
Then during 'sudo make install', because there is no .h.tmp any more, a new one 
will be
generated with privileged permissions.

Thanks,
Lin

[Qemu-devel] 答复： Re: [PATCH] trace: fix trace-events-all dependencies in Makefile.objs

[Lin Ma]

Yeah, I just noticed, Thank you.

>>> "Daniel P. Berrange" <berra...@redhat.com> 2017/2/13 星期一 下午 5:38 >>>
On Mon, Feb 13, 2017 at 05:27:09PM +0800, Lin Ma wrote:
> It causes that file trace-events-all isn't generated during build, make
> install reports 'No such file or directory'. The patch fixes it.
> 
> Signed-off-by: Lin Ma <l...@suse.com>
> ---
>  trace/Makefile.objs | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/trace/Makefile.objs b/trace/Makefile.objs
> index 7de840a..8e5e85a 100644
> --- a/trace/Makefile.objs
> +++ b/trace/Makefile.objs
> @@ -17,7 +17,7 @@ $(BUILD_DIR)/trace-events-all: $(trace-events-files)
>  
>  $(obj)/generated-helpers-wrappers.h: 
> $(obj)/generated-helpers-wrappers.h-timestamp
>@cmp $< $@ >/dev/null 2>&1 || cp $< $@
> -$(obj)/generated-helpers-wrappers.h-timestamp: $(trace-events-files) 
> $(BUILD_DIR)/config-host.mak $(tracetool-y)
> +$(obj)/generated-helpers-wrappers.h-timestamp: $(BUILD_DIR)/trace-events-all 
> $(BUILD_DIR)/config-host.mak $(tracetool-y)

This dependancy change is not right. The correct fix is here

  https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00959.html


Regards,
Daniel
-- 
|: http://berrange.com-o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- 
http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH v2 RESEND] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
In 'make' stage a qemu-version.h.tmp will be generated. If the content of
qemu-version.h.tmp and qemu-version.h aren't consistent, The qemu-version.h.tmp
will be renamed to qemu-version.h. Because of the target FORCE, The same action
will be do again in 'make install' stage.

In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
'make install' with sudo, The owner and group of new qemu-version.h.tmp will be
privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
be overwritten because of permission issue.

This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
wont be set to privileged user/group.

Signed-off-by: Lin Ma <l...@suse.com>
Reviewed-by: Fam Zheng <f...@redhat.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a8bfb2..1ca45b2 100644
--- a/Makefile
+++ b/Makefile
@@ -184,7 +184,7 @@ qemu-version.h: FORCE
printf '""\n'; \
fi; \
fi) > $@.tmp)
-   $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
+   $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)
 
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
-- 
2.9.2

[Qemu-devel] 答复： [PATCH] trace: fix trace-events-all dependencies in Makefile.objs

[Lin Ma]

Oops, I didn't notice that the issue was fixed already, Sorry about that.
Lin

>>> Lin Ma <l...@suse.com> 2017/2/13 星期一 下午 5:27 >>>
It causes that file trace-events-all isn't generated during build, make
install reports 'No such file or directory'. The patch fixes it.

Signed-off-by: Lin Ma <l...@suse.com>
---
trace/Makefile.objs | 8 
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/trace/Makefile.objs b/trace/Makefile.objs
index 7de840a..8e5e85a 100644
--- a/trace/Makefile.objs
+++ b/trace/Makefile.objs
@@ -17,7 +17,7 @@ $(BUILD_DIR)/trace-events-all: $(trace-events-files)

$(obj)/generated-helpers-wrappers.h: 
$(obj)/generated-helpers-wrappers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers-wrappers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers-wrappers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-wrapper-h \
@@ -26,7 +26,7 @@ $(obj)/generated-helpers-wrappers.h-timestamp: 
$(trace-events-files) $(BUILD_DIR

$(obj)/generated-helpers.h: $(obj)/generated-helpers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-h \
@@ -35,7 +35,7 @@ $(obj)/generated-helpers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-

$(obj)/generated-helpers.c: $(obj)/generated-helpers.c-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers.c-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers.c-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-c \
@@ -49,7 +49,7 @@ target-obj-y += generated-helpers.o

$(obj)/generated-tcg-tracers.h: $(obj)/generated-tcg-tracers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-tcg-tracers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-tcg-tracers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-h \
-- 
2.9.2

[Qemu-devel] [PATCH] trace: fix trace-events-all dependencies in Makefile.objs

[Lin Ma]

It causes that file trace-events-all isn't generated during build, make
install reports 'No such file or directory'. The patch fixes it.

Signed-off-by: Lin Ma <l...@suse.com>
---
 trace/Makefile.objs | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/trace/Makefile.objs b/trace/Makefile.objs
index 7de840a..8e5e85a 100644
--- a/trace/Makefile.objs
+++ b/trace/Makefile.objs
@@ -17,7 +17,7 @@ $(BUILD_DIR)/trace-events-all: $(trace-events-files)
 
 $(obj)/generated-helpers-wrappers.h: 
$(obj)/generated-helpers-wrappers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers-wrappers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers-wrappers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-wrapper-h \
@@ -26,7 +26,7 @@ $(obj)/generated-helpers-wrappers.h-timestamp: 
$(trace-events-files) $(BUILD_DIR
 
 $(obj)/generated-helpers.h: $(obj)/generated-helpers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-h \
@@ -35,7 +35,7 @@ $(obj)/generated-helpers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-
 
 $(obj)/generated-helpers.c: $(obj)/generated-helpers.c-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-helpers.c-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-helpers.c-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-helper-c \
@@ -49,7 +49,7 @@ target-obj-y += generated-helpers.o
 
 $(obj)/generated-tcg-tracers.h: $(obj)/generated-tcg-tracers.h-timestamp
@cmp $< $@ >/dev/null 2>&1 || cp $< $@
-$(obj)/generated-tcg-tracers.h-timestamp: $(trace-events-files) 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
+$(obj)/generated-tcg-tracers.h-timestamp: $(BUILD_DIR)/trace-events-all 
$(BUILD_DIR)/config-host.mak $(tracetool-y)
$(call quiet-command,$(TRACETOOL) \
--group=all \
--format=tcg-h \
-- 
2.9.2

[Qemu-devel] [PATCH v2] qmp: Fix argument name in error message of device-list-properties

[Lin Ma]

The argument is called "typename", not "name".

[Thanks to Markus for correcting the commit message]

Signed-off-by: Lin Ma <l...@suse.com>
---
 qmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qmp.c b/qmp.c
index 0028f0b..886059e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -531,12 +531,12 @@ DevicePropertyInfoList *qmp_device_list_properties(const 
char *typename,
 
 klass = object_class_dynamic_cast(klass, TYPE_DEVICE);
 if (klass == NULL) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name", TYPE_DEVICE);
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", 
TYPE_DEVICE);
 return NULL;
 }
 
 if (object_class_is_abstract(klass)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name",
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
"non-abstract device type");
 return NULL;
 }
-- 
2.9.2

[Qemu-devel] [PATCH] qmp: set accurate parameter name for error msg of device-list-properties

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qmp.c b/qmp.c
index 0028f0b..812db6c 100644
--- a/qmp.c
+++ b/qmp.c
@@ -531,7 +531,7 @@ DevicePropertyInfoList *qmp_device_list_properties(const 
char *typename,
 
 klass = object_class_dynamic_cast(klass, TYPE_DEVICE);
 if (klass == NULL) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "name", TYPE_DEVICE);
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename", 
TYPE_DEVICE);
 return NULL;
 }
 
-- 
2.9.2

[Qemu-devel] 答复： [PATCH] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

Oops, Sorry that I forgot to add 'V2' in email's title.
Thank Fam Zheng to review the patch.
 
Lin

>>> Lin Ma <l...@suse.com> 2017/1/17 星期二 下午 12:33 >>>
By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
In 'make' stage a qemu-version.h.tmp will be generated. If the content of
qemu-version.h.tmp and qemu-version.h aren't consistent, The qemu-version.h.tmp
will be renamed to qemu-version.h. Because of the target FORCE, The same action
will be do again in 'make install' stage.

In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
'make install' with sudo, The owner and group of new qemu-version.h.tmp will be
privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
be overwritten because of permission issue.

This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
wont be set to privileged user/group.

Signed-off-by: Lin Ma <l...@suse.com>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a8bfb2..1ca45b2 100644
--- a/Makefile
+++ b/Makefile
@@ -184,7 +184,7 @@ qemu-version.h: FORCE
printf '""\n'; \
fi; \
fi) > $@.tmp)
-   $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
+   $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)

config-host.h: config-host.h-timestamp
config-host.h-timestamp: config-host.mak
-- 
2.9.2

[Qemu-devel] [PATCH] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
In 'make' stage a qemu-version.h.tmp will be generated. If the content of
qemu-version.h.tmp and qemu-version.h aren't consistent, The qemu-version.h.tmp
will be renamed to qemu-version.h. Because of the target FORCE, The same action
will be do again in 'make install' stage.

In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
'make install' with sudo, The owner and group of new qemu-version.h.tmp will be
privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
be overwritten because of permission issue.

This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
wont be set to privileged user/group.

Signed-off-by: Lin Ma <l...@suse.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a8bfb2..1ca45b2 100644
--- a/Makefile
+++ b/Makefile
@@ -184,7 +184,7 @@ qemu-version.h: FORCE
printf '""\n'; \
fi; \
fi) > $@.tmp)
-   $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
+   $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)
 
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
-- 
2.9.2

[Qemu-devel] [PATCH] Makefile: Fix owner and group for qemu-version.h.tmp

[Lin Ma]

By commit 67a1de0d, When we perform 'git pull && make && sudo make install',
In 'make' stage a qemu-version.h.tmp will be generated. If the content of
qemu-version.h.tmp and qemu-version.h aren't consistent, The qemu-version.h.tmp
will be renamed to qemu-version.h. Because of the target FORCE, The same action
will be do again in 'make install' stage.

In 'make install' stage, If there is no qemu-version.h.tmp exists and we run
'make install' with sudo, The owner and group of new qemu-version.h.tmp will be
privileged user/group. When we run 'make' next time, qemu-version.h.tmp can't
be overwrited because of permission issue.

This patch uses 'cp' instead of 'mv' to keep the qemu-version.h.tmp file, So
during the 'sudo make install' stage, new qemu-version.h.tmp's owner and group
wont be set to privileged user/group.

Signed-off-by: Lin Ma <l...@suse.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1a8bfb2..1ca45b2 100644
--- a/Makefile
+++ b/Makefile
@@ -184,7 +184,7 @@ qemu-version.h: FORCE
printf '""\n'; \
fi; \
fi) > $@.tmp)
-   $(call quiet-command, cmp -s $@ $@.tmp || mv $@.tmp $@)
+   $(call quiet-command, cmp -s $@ $@.tmp || cp $@.tmp $@)
 
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
-- 
2.9.2

[Qemu-devel] [PATCH] tests: Ignore test-char binary

[Lin Ma]

Commit ea3af47d added test for chardev unit tests, but didn't add
the name of generated binary in .gitignore.

Signed-off-by: Lin Ma <l...@suse.com>
---
 tests/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/.gitignore b/tests/.gitignore
index c0d7857..99f2aac 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -82,5 +82,6 @@ test-xbzrle
 test-netfilter
 test-filter-mirror
 test-filter-redirector
+test-char
 *-test
 qapi-schema/*.test.*
-- 
2.9.2

[Qemu-devel] 答复： Re: About vNVDIMM question in TCG

[Lin Ma]

You've been very helpful, haozhong, Thanks for your comments, I appreciate.

>>> Haozhong Zhang <haozhong.zh...@intel.com> 2016/11/21 星期一 上午 10:55 >>>
On 11/20/16 10:58 -0700, Lin Ma wrote:
>Hi Guangrong,
>
>I'm interested in vNVDIMM on qemu. I'd like to give it a try on my laptop.
>Because I dont have skylake server hardware, So I want to expose cpu flags
>clflushopt, clwb and pcommit to guest for NVDIMM drivers on TCG.
>
>I saw clflushopt and clwb feature flags in cpuinfo of guest, but no pcommit.
>Could you please tell me is there anyhing wrong in my qemu CLI or anything
>I misunderstand?

pcommit had been deprecated (see KVM commit
dfa169bbee00671288df25f8ef8a2f6e13fe2581), so it's correct to not have
it in guest.

>
>qemu-system-x86_64 \
>-cpu Skylake-Client,+pcommit,+clflushopt,+clwb \
>-machine pc-i440fx-2.8,nvdimm=on,accel=tcg \
>-m 4G,maxmem=... \
>-object memory-backend-file,... \
>-device nvdimm,... \
>
>BTW, In kvm mode, Even if I used '-cpu qemu64' without flags pcommit,
>clflushopt and clwb, The block device /dev/pmem0 is still there and can
>be used successfully. Does it make sense? Doesn't nvdimm kernel driver
>need pcommit, clflushopt and clwb to work?
>

If my memory is correct, Linux NVDIMM driver will fallback to clflush
when neither clflushopt nor clwb is available.

Haozhong

[Qemu-devel] About vNVDIMM question in TCG

[Lin Ma]

Hi Guangrong,
 
I'm interested in vNVDIMM on qemu. I'd like to give it a try on my laptop.
Because I dont have skylake server hardware, So I want to expose cpu flags
clflushopt, clwb and pcommit to guest for NVDIMM drivers on TCG.
 
I saw clflushopt and clwb feature flags in cpuinfo of guest, but no pcommit.
Could you please tell me is there anyhing wrong in my qemu CLI or anything
I misunderstand?
 
qemu-system-x86_64 \
-cpu Skylake-Client,+pcommit,+clflushopt,+clwb \
-machine pc-i440fx-2.8,nvdimm=on,accel=tcg \
-m 4G,maxmem=... \
-object memory-backend-file,... \
-device nvdimm,... \
 
BTW, In kvm mode, Even if I used '-cpu qemu64' without flags pcommit,
clflushopt and clwb, The block device /dev/pmem0 is still there and can 
be used successfully. Does it make sense? Doesn't nvdimm kernel driver
need pcommit, clflushopt and clwb to work?
 
Thanks,
Lin

Re: [Qemu-devel] 答复： [PATCH] smbios: Add 1 terminator if there is any string field defined in given table.

[Lin Ma]

>>> "Daniel P. Berrange" berra...@redhat.com> 2016/11/8 星期二 上午 12:31 >>
( mailto:berra...@redhat.com) 
..
>>
>
>Code identation is this patch looks totally mangled.
>
How about this one: (From the code style's perspective, it should be no big 
problem)
http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg01024.html
 
Thanks,
Lin

[Qemu-devel] 答复： [PATCH] smbios: Add 1 terminator if there is any string field defined in given table.

[Lin Ma]

Ping.

>>> Lin Ma <l...@suse.com> 2016/9/6 星期二 下午 4:28 >>>
If user specifies binary file on command line to load smbios entries, then
will get error messages while decoding them in guest.

Reproducer:
1. dump a smbios table to a binary file from host or guest.(says table 1)
2. load the binary file through command line: 'qemu -smbios file=...'.
3. perform 'dmidecode' or 'dmidecode -t 1' in guest.

It reports 'Invalid entry length...' because qemu doesn't add terminator(s) for
the table correctly.
For smbios tables which have string field provided, qemu should add 1 
terminator.
For smbios tables which dont have string field provided, qemu should add 2.

This patch fixed the issue.

Signed-off-by: Lin Ma <l...@suse.com>
---
hw/smbios/smbios.c   | 90 
++
include/hw/smbios/smbios.h | 44 +++
2 files changed, 134 insertions(+)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 74c7102..6293bc5 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -885,6 +885,9 @@ void smbios_entry_add(QemuOpts *opts)
{
 const char *val;

+int i, terminator_count = 2, table_str_field_count = 0;
+int *tables_str_field_offset = NULL;
+
 assert(!smbios_immutable);

 val = qemu_opt_get(opts, "file");
@@ -926,7 +929,94 @@ void smbios_entry_add(QemuOpts *opts)
 smbios_type4_count++;
 }

+   switch (header->type) {
+   case 0:
+   tables_str_field_offset = g_malloc0(sizeof(int) * \
+   
TYPE_0_STR_FIELD_COUNT);
+   tables_str_field_offset = (int []){\
+   
TYPE_0_STR_FIELD_OFFSET_VENDOR, \
+   
TYPE_0_STR_FIELD_OFFSET_BIOS_VERSION, \
+   
TYPE_0_STR_FIELD_OFFSET_BIOS_RELEASE_DATE};
+   table_str_field_count = sizeof(tables_str_field_offset) / \
+   
sizeof(tables_str_field_offset[0]);
+   break;
+   case 1:
+   tables_str_field_offset = g_malloc0(sizeof(int) * \
+   
TYPE_1_STR_FIELD_COUNT);
+   tables_str_field_offset = (int []){
+   
TYPE_1_STR_FIELD_OFFSET_MANUFACTURER, \
+   
TYPE_1_STR_FIELD_OFFSET_PRODUCT, \
+   
TYPE_1_STR_FIELD_OFFSET_VERSION, \
+   
TYPE_1_STR_FIELD_OFFSET_SERIAL, \
+   
TYPE_1_STR_FIELD_OFFSET_SKU, \
+   
TYPE_1_STR_FIELD_OFFSET_FAMILY};
+   table_str_field_count = sizeof(tables_str_field_offset) / \
+   
sizeof(tables_str_field_offset[0]);
+   break;
+   case 2:
+   tables_str_field_offset = g_malloc0(sizeof(int) * \
+   
TYPE_2_STR_FIELD_COUNT);
+   tables_str_field_offset = (int []){\
+   
TYPE_2_STR_FIELD_OFFSET_MANUFACTURER, \
+   
TYPE_2_STR_FIELD_OFFSET_PRODUCT, \
+   
TYPE_2_STR_FIELD_OFFSET_VERSION, \
+   
TYPE_2_STR_FIELD_OFFSET_SERIAL, \
+   
TYPE_2_STR_FIELD_OFFSET_ASSET, \
+   
TYPE_2_STR_FIELD_OFFSET_LOCATION};
+   table_str_field_count = sizeof(tables_str_field_offset) / \
+   
sizeof(tables_str_field_offset[0]);
+   break;
+   case 3:
+   tables_str_field_offset = g_malloc0(sizeof(int) * \
+   
TYPE_3_STR_FIELD_COUNT);
+   tables_str_field_offset = (int []){\
+   
TYPE_3_STR_FIELD_OFFSET_MANUFACTURER, \
+

[Qemu-devel] 答复： Re: [PATCH v4 5/5] object: Add 'help' option for all available backends and properties

[Lin Ma]

>>> Markus Armbruster <arm...@redhat.com> 2016/11/4 星期五 上午 3:50 >>>
>Lin Ma <l...@suse.com> writes:
>
>> '-object help' prints available user creatable backends.
>> '-object $typename,help' prints relevant properties.
>>
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  include/qom/object_interfaces.h |  2 ++
>>  qemu-options.hx   |  7 +-
>>  qom/object_interfaces.c   | 55 +
>>  vl.c |  5 
>>  4 files changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qom/object_interfaces.h 
>> b/include/qom/object_interfaces.h
>> index 8b17f4d..197cd5f 100644
>> --- a/include/qom/object_interfaces.h
>> +++ b/include/qom/object_interfaces.h
>> @@ -165,4 +165,6 @@ int user_creatable_add_opts_foreach(void *opaque,
>>   */
>>  void user_creatable_del(const char *id, Error **errp);
>>  
>> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp);
>> +
>
>Ugly interface: two arguments that aren't used, one of them void *.
>Suggest to follow the device help example: do a non-ugly interface here,
>and an ugly, but static wrapper in vl.c
ok, will do.

>>  #endif
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index b1fbdb0..b9573ce 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3761,7 +3761,9 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
>>"  create a new object of type TYPENAME setting 
>> properties\n"
>>"  in the order they are specified.  Note that 
>> the 'id'\n"
>>"  property must be set.  These objects are 
>> placed in the\n"
>> -"  '/objects' path.\n",
>> +"  '/objects' path.\n"
>> +"  Use '-object help' to print available 
>> backend types and\n"
>> +"  '-object typename,help' to print relevant 
>> properties.\n",
>
>The existing text talks about "object of type TYPENAME".  The new text
>suddenly talks about "backend types".  Switching terminology is not a
>good idea.  Call them "object types".
>
>For comparison, here's the text we use for -device:
>
>   use '-device help' to print all possible drivers
>   use '-device driver,help' to print all possible 
> properties
>
>Let's make this one similar:
>
>   use '-object help' to print all possible object 
> types
>   use '-object driver,help' to print all possible 
> properties
ok, will do.
 
>>QEMU_ARCH_ALL)
>>  STEXI
>>  @item -object @var{typename}[,@var{prop1}=@var{value1},...]
>> @@ -3771,6 +3773,9 @@ in the order they are specified.  Note that the 'id'
>>  property must be set.  These objects are placed in the
>>  '/objects' path.
>>  
>> +Use @code{-object help} to print available backend types and
>> +@code{-object @var{typename},help} to print relevant properties.
>> +
>
>-device's text:
>
>To get help on possible drivers and properties, use @code{-device
>help} and @code{-device @var{driver},help}.
>
>Suggest:
>
>To get help on possible object types and properties, use @code{-object
>help} and @code{-object @var{type},help}.
ok, will do.

>>  @table @option
>>  
>>  @item -object 
>> memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index bf59846..da8be39 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -5,6 +5,7 @@
>>  #include "qapi-visit.h"
>>  #include "qapi/qmp-output-visitor.h"
>>  #include "qapi/opts-visitor.h"
>> +#include "qemu/help_option.h"
>>  
>>  void user_creatable_complete(Object *obj, Error **errp)
>>  {
>> @@ -212,6 +213,60 @@ void user_creatable_del(const char *id, Error **errp)
>>object_unparent(obj);
>>  }
>>  
>> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
>> +{
>> +const char *type = NULL;
>> +Object *obj = NULL;
>> +ObjectClass *klass;
>> +ObjectProperty *prop;
>> +ObjectPropertyIterator iter;
>> +
>> +type = qem

[Qemu-devel] 答复： Re: [PATCH v4 4/5] backends: add description for enum class properties

[Lin Ma]

>>> Markus Armbruster <arm...@redhat.com> 2016/11/4 星期五 上午 3:58 >>>
>Lin Ma <l...@suse.com> writes:
>
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  backends/hostmem.c | 4 
>>  crypto/secret.c| 4 
>>  crypto/tlscreds.c  | 4 
>>  net/filter.c   | 4 
>>  4 files changed, 16 insertions(+)
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 4256d24..25f303d 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -377,6 +377,10 @@ host_memory_backend_class_init(ObjectClass *oc, void 
>> *data)
>>HostMemPolicy_lookup,
>>host_memory_backend_get_policy,
>>host_memory_backend_set_policy, _abort);
>> +object_class_property_set_description(oc, "policy",
>> +"Data 
>> format: one of "
>> +
>> HostMemPolicy_value_str,
>> +
>> _abort);
>>  }
>>  
>>  static const TypeInfo host_memory_backend_info = {
>> diff --git a/crypto/secret.c b/crypto/secret.c
>> index 285ab7a..71d06e3 100644
>> --- a/crypto/secret.c
>> +++ b/crypto/secret.c
>> @@ -382,6 +382,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
>>   
>> qcrypto_secret_prop_get_format,
>>   
>> qcrypto_secret_prop_set_format,
>>   NULL);
>> +object_class_property_set_description(oc, "format",
>> +
>>   "Data format: one of "
>> +
>>   QCryptoSecretFormat_value_str,
>> +
>>   _abort);
>>object_class_property_add_str(oc, "data",
>>  
>> qcrypto_secret_prop_get_data,
>>  
>> qcrypto_secret_prop_set_data,
>> diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
>> index a896553..d3af38a 100644
>> --- a/crypto/tlscreds.c
>> +++ b/crypto/tlscreds.c
>> @@ -237,6 +237,10 @@ qcrypto_tls_creds_class_init(ObjectClass *oc, void 
>> *data)
>>   
>> qcrypto_tls_creds_prop_get_endpoint,
>>   
>> qcrypto_tls_creds_prop_set_endpoint,
>>   NULL);
>> +object_class_property_set_description(oc, "endpoint",
>> +
>>   "Data format: one of "
>> +
>>   QCryptoTLSCredsEndpoint_value_str,
>> +
>>   _abort);
>>object_class_property_add_str(oc, "priority",
>>  
>> qcrypto_tls_creds_prop_get_priority,
>>  
>> qcrypto_tls_creds_prop_set_priority,
>> diff --git a/net/filter.c b/net/filter.c
>> index 1dfd2ca..205fb49 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -182,6 +182,10 @@ static void netfilter_init(Object *obj)
>> 
>> NetFilterDirection_lookup,
>> 
>> netfilter_get_direction, netfilter_set_direction,
>> NULL);
>> +object_property_set_description(obj, "queue",
>> +"Data 
>> format: one of "
>> +
>> NetFilterDirection_value_str,
>> +
>> _abort);
>>object_property_add_str(obj, "status",
>>  

[Qemu-devel] 答复： Re: [PATCH v4 2/5] qapi: auto generate enum value strings

[Lin Ma]

>>> Markus Armbruster <arm...@redhat.com> 2016/11/4 星期五 上午 3:17 >>>
>Lin Ma <l...@suse.com> writes:
>
>> Automatically generate enum value strings that containing the acceptable 
>> values.
>> (Borrowed Daniel's code.)
>>
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  scripts/qapi-types.py | 2 ++
>>  scripts/qapi.py   | 9 +
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index dabc42e..0446839 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -202,9 +202,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>self._btin += gen_enum(name, values, prefix)
>>if do_builtins:
>>self.defn += gen_enum_lookup(name, values, 
>> prefix)
>> +self._btin += gen_enum_value_str(name, values)
>>else:
>>self._fwdecl += gen_enum(name, values, prefix)
>>self.defn += gen_enum_lookup(name, values, prefix)
>> +self._fwdecl += gen_enum_value_str(name, values)
>>  
>>def visit_array_type(self, name, info, element_type):
>>if isinstance(element_type, QAPISchemaBuiltinType):
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 21bc32f..d11c414 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1649,6 +1649,15 @@ const char *const %(c_name)s_lookup[] = {
>>return ret
>>  
>>  
>> +def gen_enum_value_str(name, values):
>> +return mcgen('''
>> +
>> +#define %(c_name)s_value_str "%(value_str)s"
>> +''',
>> +c_name=c_name(name),
>> +value_str=", ".join(["'%s'" % c for c in values]))
>> +
>> +
>>  def gen_enum(name, values, prefix=None):
>># append automatically generated _MAX value
>>enum_values = values + ['_MAX']
>
>This function is generating a macro definition, not a string.  We could
>call it gen_enum_values_define().  But I'd simply fold it into
>gen_enum().
>
>Adds another 9KiB to qapi-types.h, which is included widely.  Instead of
>defining these macros, we could also iterate over FOO_lookup[] at
>run-time.  But let's first review how the macro is used.
Sorry, I'm not familiar with writing qemu test code. I didn't understand it.
What's the meaning of 'Adds another 9KiB to qapi-types.h'? What is '9KiB'?
 
Thanks,
Lin
 

 

[Qemu-devel] 答复： Re: [PATCH v4 1/5] qom: Add interface check in object_class_is_abstract

[Lin Ma]

>>> Markus Armbruster <arm...@redhat.com> 2016/11/4 星期五 上午 2:18 >>>
>Lin Ma <l...@suse.com> writes:
>
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  qom/object.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 7a05e35..4096645 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -747,7 +747,11 @@ ObjectClass *object_get_class(Object *obj)
>>  
>>  bool object_class_is_abstract(ObjectClass *klass)
>>  {
>> -return klass->type->abstract;
>> +if (type_is_ancestor(klass->type, type_interface)) {
>> +return true;
>> +} else {
>> +return klass->type->abstract;
>> +}
>>  }
>>  
>>  const char *object_class_get_name(ObjectClass *klass)
>
>Pardon my ignorance...
>
>If all types derived from type_interface are abstract, why aren't we
>setting ->abstract right when such a type is defined?
For lots of interfaces, they already explicitly define abstract as true, say:
generic-pc-machine in hw/i386/pc.c, or memory-backend in backends/hostmem.c
 
Please refer to 
http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg01955.html
hope it could give you some clue.
but I see that other interfaces (I randomly picked generic-pc-machine and 
fw_cfg as examples)
still explicitly define 'abstract = true'.

>Hmm, perhaps we do?  type_initialize_interface() sets info.abstract =
>true...
For interfaces, I guess not.

>In case we don't: what about other uses of ->abstract?  Why is it okay
>not to check whether type_interface is an ancestore there?
For lots of interfaces, they already explicitly define abstract as true, say:
generic-pc-machine in hw/i386/pc.c, or memory-backend in backends/hostmem.c
So they can be checked through  ->abstract, dont need to  check whether 
type_interface is
an ancestore.
 
Thanks,
Lin

[Qemu-devel] 答复： [PATCH v4 5/5] object: Add 'help' option for all available backends and properties

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
'-object help' prints available user creatable backends.
'-object $typename,help' prints relevant properties.

Signed-off-by: Lin Ma <l...@suse.com>
---
include/qom/object_interfaces.h |  2 ++
qemu-options.hx  |  7 +-
qom/object_interfaces.c  | 55 +
vl.c|  5 
4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 8b17f4d..197cd5f 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -165,4 +165,6 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
void user_creatable_del(const char *id, Error **errp);

+int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp);
+
#endif
diff --git a/qemu-options.hx b/qemu-options.hx
index b1fbdb0..b9573ce 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3761,7 +3761,9 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
 "  create a new object of type TYPENAME 
setting properties\n"
 "  in the order they are specified.  Note that 
the 'id'\n"
 "  property must be set.  These objects are 
placed in the\n"
-" '/objects' path.\n",
+" '/objects' path.\n"
+" Use '-object help' to print available backend types 
and\n"
+" '-object typename,help' to print relevant 
properties.\n",
 QEMU_ARCH_ALL)
STEXI
@item -object @var{typename}[,@var{prop1}=@var{value1},...]
@@ -3771,6 +3773,9 @@ in the order they are specified.  Note that the 'id'
property must be set.  These objects are placed in the
'/objects' path.

+Use @code{-object help} to print available backend types and
+@code{-object @var{typename},help} to print relevant properties.
+
@table @option

@item -object 
memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf59846..da8be39 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -5,6 +5,7 @@
#include "qapi-visit.h"
#include "qapi/qmp-output-visitor.h"
#include "qapi/opts-visitor.h"
+#include "qemu/help_option.h"

void user_creatable_complete(Object *obj, Error **errp)
{
@@ -212,6 +213,60 @@ void user_creatable_del(const char *id, Error **errp)
 object_unparent(obj);
}

+int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *type = NULL;
+Object *obj = NULL;
+ObjectClass *klass;
+ObjectProperty *prop;
+ObjectPropertyIterator iter;
+
+type = qemu_opt_get(opts, "qom-type");
+if (type && is_help_option(type)) {
+   GSList *list;
+   printf("Available object backend types:\n");
+   for (list = object_class_get_list(TYPE_USER_CREATABLE, false);  \
+   list;   
  \
+   list = list->next) {
+   const char *name;
+   name = object_class_get_name(OBJECT_CLASS(list->data));
+   if (strcmp(name, TYPE_USER_CREATABLE)) {
+   printf("%s\n", name);
+   }
+   }
+   g_slist_free(list);
+   goto out;
+}
+
+if (!type || !qemu_opt_has_help_opt(opts)) {
+   return 0;
+}
+
+klass = object_class_by_name(type);
+if (!klass) {
+   printf("invalid object type: %s\n", type);
+   goto out;
+}
+if (object_class_is_abstract(klass)) {
+   printf("object type '%s' is abstract\n", type);
+   goto out;
+}
+obj = object_new(type);
+object_property_iter_init(, obj);
+
+while ((prop = object_property_iter_next())) {
+   if (prop->description) {
+   printf("%s (%s, %s)\n", prop->name, prop->type, 
prop->description);
+   } else {
+   printf("%s (%s)\n", prop->name, prop->type);
+   }
+}
+
+out:
+object_unref(obj);
+return 1;
+}
+
static void register_types(void)
{
 static const TypeInfo uc_interface_info = {
diff --git a/vl.c b/vl.c
index ebd47af..145
6eca 100644
--- a/vl.c
+++ b/vl.c
@@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp)
 exit(0);
 }

+if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func,
+ NULL, NULL)) {
+   exit(1);
+}
+
 if (!trace_init_backends()) {
 exit(1);
 }
-- 
2.9.2

[Qemu-devel] 答复： [PATCH v4 3/5] qapi: add test case for the generated enum value str

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
Signed-off-by: Lin Ma <l...@suse.com>
---
tests/test-qmp-commands.c | 18 ++
1 file changed, 18 insertions(+)

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 81cbe54..9cd61b2 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -262,6 +262,23 @@ static void test_dealloc_partial(void)
 qapi_free_UserDefTwo(ud2);
}

+/* test generated enum value str */
+static void test_enum_value_str(void)
+{
+EnumOne i;
+char *expected_str = NULL;
+
+for (i = 0; i < ENUM_ONE__MAX; i++) {
+   if (i == 0) {
+   expected_str = g_strdup_printf("\'%s\'", EnumOne_lookup[i]);
+   } else {
+   expected_str = g_strdup_printf("%s, \'%s\'",
+   
expected_str, EnumOne_lookup[i]);
+   }
+}
+g_assert_cmpstr(EnumOne_value_str, ==, expected_str);
+}
+

int main(int argc, char **argv)
{
@@ -272,6 +289,7 @@ int main(int argc, char **argv)
 g_test_add_func("/0.15/dispatch_cmd_io", test_dispatch_cmd_io);
 g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
 g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
+g_test_add_func("/0.15/enum_value_str", test_enum_value_str);

 module_call_init(MODULE_INIT_QAPI);
 g_test_run();
-- 
2.9.2

[Qemu-devel] 答复： [PATCH v4 2/5] qapi: auto generate enum value strings

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
Automatically generate enum value strings that containing the acceptable values.
(Borrowed Daniel's code.)

Signed-off-by: Lin Ma <l...@suse.com>
---
scripts/qapi-types.py | 2 ++
scripts/qapi.py| 9 +
2 files changed, 11 insertions(+)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index dabc42e..0446839 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -202,9 +202,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self._btin += gen_enum(name, values, prefix)
 if do_builtins:
 self.defn += gen_enum_lookup(name, values, 
prefix)
+   self._btin += gen_enum_value_str(name, values)
 else:
 self._fwdecl += gen_enum(name, values, prefix)
 self.defn += gen_enum_lookup(name, values, prefix)
+   self._fwdecl += gen_enum_value_str(name, values)

 def visit_array_type(self, name, info, element_type):
 if isinstance(element_type, QAPISchemaBuiltinType):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 21bc32f..d11c414 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1649,6 +1649,15 @@ const char *const %(c_name)s_lookup[] = {
 return ret


+def gen_enum_value_str(name, values):
+return mcgen('''
+
+#define %(c_name)s_value_str "%(value_str)s"
+''',
+   c_name=c_name(name),
+   value_str=", ".join(["'%s'" % c for c in values]))
+
+
def gen_enum(name, values, prefix=None):
 # append automatically generated _MAX value
 enum_values = values + ['_MAX']
-- 
2.9.2

[Qemu-devel] 答复： [PATCH v4 4/5] backends: add description for enum class properties

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
Signed-off-by: Lin Ma <l...@suse.com>
---
backends/hostmem.c | 4 
crypto/secret.c| 4 
crypto/tlscreds.c  | 4 
net/filter.c   | 4 
4 files changed, 16 insertions(+)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4256d24..25f303d 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -377,6 +377,10 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
 HostMemPolicy_lookup,
 host_memory_backend_get_policy,
 host_memory_backend_set_policy, _abort);
+object_class_property_set_description(oc, "policy",
+   "Data 
format: one of "
+   
HostMemPolicy_value_str,
+   
_abort);
}

static const TypeInfo host_memory_backend_info = {
diff --git a/crypto/secret.c b/crypto/secret.c
index 285ab7a..71d06e3 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -382,6 +382,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)

qcrypto_secret_prop_get_format,

qcrypto_secret_prop_set_format,
NULL);
+object_class_property_set_description(oc, "format",
+   
  "Data format: one of "
+   
  QCryptoSecretFormat_value_str,
+   
  _abort);
 object_class_property_add_str(oc, "data",
   
qcrypto_secret_prop_get_data,
   
qcrypto_secret_prop_set_data,
diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index a896553..d3af38a 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -237,6 +237,10 @@ qcrypto_tls_creds_class_init(ObjectClass *oc, void *data)

qcrypto_tls_creds_prop_get_endpoint,

qcrypto_tls_creds_prop_set_endpoint,
NULL);
+object_class_property_set_description(oc, "endpoint",
+   
  "Data format: one of "
+   
  QCryptoTLSCredsEndpoint_value_str,
+   
  _abort);
 object_class_property_add_str(oc, "priority",
   
qcrypto_tls_creds_prop_get_priority,
   
qcrypto_tls_creds_prop_set_priority,
diff --git a/net/filter.c b/net/filter.c
index 1dfd2ca..205fb49 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -182,6 +182,10 @@ static void netfilter_init(Object *obj)
  
NetFilterDirection_lookup,
  
netfilter_get_direction, netfilter_set_direction,
  NULL);
+object_property_set_description(obj, "queue",
+   "Data 
format: one of "
+   
NetFilterDirection_value_str,
+   
_abort);
 object_property_add_str(obj, "status",
 netfilter_get_status, 
netfilter_set_status,
 NULL);
-- 
2.9.2

[Qemu-devel] 答复： [PATCH v4 1/5] qom: Add interface check in object_class_is_abstract

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
Signed-off-by: Lin Ma <l...@suse.com>
---
qom/object.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 7a05e35..4096645 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -747,7 +747,11 @@ ObjectClass *object_get_class(Object *obj)

bool object_class_is_abstract(ObjectClass *klass)
{
-return klass->type->abstract;
+if (type_is_ancestor(klass->type, type_interface)) {
+   return true;
+} else {
+   return klass->type->abstract;
+}
}

const char *object_class_get_name(ObjectClass *klass)
-- 
2.9.2

[Qemu-devel] 答复： [PATCH v4 0/5] object: Add 'help' option for all available backends and properties

[Lin Ma]

ping...

>>> Lin Ma <l...@suse.com> 2016/10/20 星期四 下午 7:28 >>>
V3->V4:
* drop the code about manually define interface 'user-creatable' as abstract.
* add test case for the generated enum value str.
* split the code about adding descriptions to class properties into a separate 
patch.
* minor changes in user_creatable_help_func to ignore interface 
TYPE_USER_CREATABLE.

V2->v3:
* make type user-creatable abstract.
* auto generate enum value strings during qemu configuration.(Borrowwed 
Daniel's code)
* save the generated enum value strings into member description of 
ObjectProperty.
* drop the judgement logic of whether a property has an enumeration type 
anymore,
  output member description of ObjectProperty directly.
* at least, user_creatable_help_func should be put after
  'object_property_add_child(object_get_root(), 
"machine",OBJECT(current_machine), ...)',
  because host_memory_backend_init needs to access an instance of type machine.

V1->V2:
* Output the acceptable values of enum types by "-object TYPE-NAME,help"

Lin Ma (5):
  qom: Add interface check in object_class_is_abstract
  qapi: auto generate enum value strings
  qapi: add test case for the generated enum value str
  backends: add description for enum class properties
  object: Add 'help' option for all available backends and properties

backends/hostmem.c|  4 +++
crypto/secret.c  |  4 +++
crypto/tlscreds.c  |  4 +++
include/qom/object_interfaces.h |  2 ++
net/filter.c|  4 +++
qemu-options.hx  |  7 +-
qom/object.c|  6 -
qom/object_interfaces.c  | 55 +
scripts/qapi-types.py  |  2 ++
scripts/qapi.py  |  9 +++
tests/test-qmp-commands.c  | 18 ++
vl.c|  5 
12 files changed, 118 insertions(+), 2 deletions(-)

-- 
2.9.2

[Qemu-devel] [PATCH v4 4/5] backends: add description for enum class properties

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 backends/hostmem.c | 4 
 crypto/secret.c| 4 
 crypto/tlscreds.c  | 4 
 net/filter.c   | 4 
 4 files changed, 16 insertions(+)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 4256d24..25f303d 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -377,6 +377,10 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
 HostMemPolicy_lookup,
 host_memory_backend_get_policy,
 host_memory_backend_set_policy, _abort);
+object_class_property_set_description(oc, "policy",
+"Data format: one of "
+HostMemPolicy_value_str,
+_abort);
 }
 
 static const TypeInfo host_memory_backend_info = {
diff --git a/crypto/secret.c b/crypto/secret.c
index 285ab7a..71d06e3 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -382,6 +382,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
qcrypto_secret_prop_get_format,
qcrypto_secret_prop_set_format,
NULL);
+object_class_property_set_description(oc, "format",
+  "Data format: one of "
+  QCryptoSecretFormat_value_str,
+  _abort);
 object_class_property_add_str(oc, "data",
   qcrypto_secret_prop_get_data,
   qcrypto_secret_prop_set_data,
diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index a896553..d3af38a 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -237,6 +237,10 @@ qcrypto_tls_creds_class_init(ObjectClass *oc, void *data)
qcrypto_tls_creds_prop_get_endpoint,
qcrypto_tls_creds_prop_set_endpoint,
NULL);
+object_class_property_set_description(oc, "endpoint",
+  "Data format: one of "
+  QCryptoTLSCredsEndpoint_value_str,
+  _abort);
 object_class_property_add_str(oc, "priority",
   qcrypto_tls_creds_prop_get_priority,
   qcrypto_tls_creds_prop_set_priority,
diff --git a/net/filter.c b/net/filter.c
index 1dfd2ca..205fb49 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -182,6 +182,10 @@ static void netfilter_init(Object *obj)
  NetFilterDirection_lookup,
  netfilter_get_direction, netfilter_set_direction,
  NULL);
+object_property_set_description(obj, "queue",
+"Data format: one of "
+NetFilterDirection_value_str,
+_abort);
 object_property_add_str(obj, "status",
 netfilter_get_status, netfilter_set_status,
 NULL);
-- 
2.9.2

[Qemu-devel] [PATCH v4 1/5] qom: Add interface check in object_class_is_abstract

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 qom/object.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 7a05e35..4096645 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -747,7 +747,11 @@ ObjectClass *object_get_class(Object *obj)
 
 bool object_class_is_abstract(ObjectClass *klass)
 {
-return klass->type->abstract;
+if (type_is_ancestor(klass->type, type_interface)) {
+return true;
+} else {
+return klass->type->abstract;
+}
 }
 
 const char *object_class_get_name(ObjectClass *klass)
-- 
2.9.2

[Qemu-devel] [PATCH v4 5/5] object: Add 'help' option for all available backends and properties

[Lin Ma]

'-object help' prints available user creatable backends.
'-object $typename,help' prints relevant properties.

Signed-off-by: Lin Ma <l...@suse.com>
---
 include/qom/object_interfaces.h |  2 ++
 qemu-options.hx |  7 +-
 qom/object_interfaces.c | 55 +
 vl.c|  5 
 4 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 8b17f4d..197cd5f 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -165,4 +165,6 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 void user_creatable_del(const char *id, Error **errp);
 
+int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index b1fbdb0..b9573ce 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3761,7 +3761,9 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
 "create a new object of type TYPENAME setting properties\n"
 "in the order they are specified.  Note that the 'id'\n"
 "property must be set.  These objects are placed in the\n"
-"'/objects' path.\n",
+"'/objects' path.\n"
+"Use '-object help' to print available backend types and\n"
+"'-object typename,help' to print relevant properties.\n",
 QEMU_ARCH_ALL)
 STEXI
 @item -object @var{typename}[,@var{prop1}=@var{value1},...]
@@ -3771,6 +3773,9 @@ in the order they are specified.  Note that the 'id'
 property must be set.  These objects are placed in the
 '/objects' path.
 
+Use @code{-object help} to print available backend types and
+@code{-object @var{typename},help} to print relevant properties.
+
 @table @option
 
 @item -object 
memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf59846..da8be39 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -5,6 +5,7 @@
 #include "qapi-visit.h"
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/opts-visitor.h"
+#include "qemu/help_option.h"
 
 void user_creatable_complete(Object *obj, Error **errp)
 {
@@ -212,6 +213,60 @@ void user_creatable_del(const char *id, Error **errp)
 object_unparent(obj);
 }
 
+int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
+{
+const char *type = NULL;
+Object *obj = NULL;
+ObjectClass *klass;
+ObjectProperty *prop;
+ObjectPropertyIterator iter;
+
+type = qemu_opt_get(opts, "qom-type");
+if (type && is_help_option(type)) {
+GSList *list;
+printf("Available object backend types:\n");
+for (list = object_class_get_list(TYPE_USER_CREATABLE, false);  \
+list;   \
+list = list->next) {
+const char *name;
+name = object_class_get_name(OBJECT_CLASS(list->data));
+if (strcmp(name, TYPE_USER_CREATABLE)) {
+printf("%s\n", name);
+}
+}
+g_slist_free(list);
+goto out;
+}
+
+if (!type || !qemu_opt_has_help_opt(opts)) {
+return 0;
+}
+
+klass = object_class_by_name(type);
+if (!klass) {
+printf("invalid object type: %s\n", type);
+goto out;
+}
+if (object_class_is_abstract(klass)) {
+printf("object type '%s' is abstract\n", type);
+goto out;
+}
+obj = object_new(type);
+object_property_iter_init(, obj);
+
+while ((prop = object_property_iter_next())) {
+if (prop->description) {
+printf("%s (%s, %s)\n", prop->name, prop->type, prop->description);
+} else {
+printf("%s (%s)\n", prop->name, prop->type);
+}
+}
+
+out:
+object_unref(obj);
+return 1;
+}
+
 static void register_types(void)
 {
 static const TypeInfo uc_interface_info = {
diff --git a/vl.c b/vl.c
index ebd47af..1456eca 100644
--- a/vl.c
+++ b/vl.c
@@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp)
 exit(0);
 }
 
+if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func,
+  NULL, NULL)) {
+exit(1);
+}
+
 if (!trace_init_backends()) {
 exit(1);
 }
-- 
2.9.2

[Qemu-devel] [PATCH v4 0/5] object: Add 'help' option for all available backends and properties

[Lin Ma]

V3->V4:
* drop the code about manually define interface 'user-creatable' as abstract.
* add test case for the generated enum value str.
* split the code about adding descriptions to class properties into a separate 
patch.
* minor changes in user_creatable_help_func to ignore interface 
TYPE_USER_CREATABLE.

V2->v3:
* make type user-creatable abstract.
* auto generate enum value strings during qemu configuration.(Borrowwed 
Daniel's code)
* save the generated enum value strings into member description of 
ObjectProperty.
* drop the judgement logic of whether a property has an enumeration type 
anymore,
  output member description of ObjectProperty directly.
* at least, user_creatable_help_func should be put after
  'object_property_add_child(object_get_root(), 
"machine",OBJECT(current_machine), ...)',
  because host_memory_backend_init needs to access an instance of type machine.

V1->V2:
* Output the acceptable values of enum types by "-object TYPE-NAME,help"

Lin Ma (5):
  qom: Add interface check in object_class_is_abstract
  qapi: auto generate enum value strings
  qapi: add test case for the generated enum value str
  backends: add description for enum class properties
  object: Add 'help' option for all available backends and properties

 backends/hostmem.c  |  4 +++
 crypto/secret.c |  4 +++
 crypto/tlscreds.c   |  4 +++
 include/qom/object_interfaces.h |  2 ++
 net/filter.c|  4 +++
 qemu-options.hx |  7 +-
 qom/object.c|  6 -
 qom/object_interfaces.c | 55 +
 scripts/qapi-types.py   |  2 ++
 scripts/qapi.py |  9 +++
 tests/test-qmp-commands.c   | 18 ++
 vl.c|  5 
 12 files changed, 118 insertions(+), 2 deletions(-)

-- 
2.9.2

[Qemu-devel] [PATCH v4 2/5] qapi: auto generate enum value strings

[Lin Ma]

Automatically generate enum value strings that containing the acceptable values.
(Borrowed Daniel's code.)

Signed-off-by: Lin Ma <l...@suse.com>
---
 scripts/qapi-types.py | 2 ++
 scripts/qapi.py   | 9 +
 2 files changed, 11 insertions(+)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index dabc42e..0446839 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -202,9 +202,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self._btin += gen_enum(name, values, prefix)
 if do_builtins:
 self.defn += gen_enum_lookup(name, values, prefix)
+self._btin += gen_enum_value_str(name, values)
 else:
 self._fwdecl += gen_enum(name, values, prefix)
 self.defn += gen_enum_lookup(name, values, prefix)
+self._fwdecl += gen_enum_value_str(name, values)
 
 def visit_array_type(self, name, info, element_type):
 if isinstance(element_type, QAPISchemaBuiltinType):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 21bc32f..d11c414 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1649,6 +1649,15 @@ const char *const %(c_name)s_lookup[] = {
 return ret
 
 
+def gen_enum_value_str(name, values):
+return mcgen('''
+
+#define %(c_name)s_value_str "%(value_str)s"
+''',
+c_name=c_name(name),
+value_str=", ".join(["'%s'" % c for c in values]))
+
+
 def gen_enum(name, values, prefix=None):
 # append automatically generated _MAX value
 enum_values = values + ['_MAX']
-- 
2.9.2

[Qemu-devel] [PATCH v4 3/5] qapi: add test case for the generated enum value str

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 tests/test-qmp-commands.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 81cbe54..9cd61b2 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -262,6 +262,23 @@ static void test_dealloc_partial(void)
 qapi_free_UserDefTwo(ud2);
 }
 
+/* test generated enum value str */
+static void test_enum_value_str(void)
+{
+EnumOne i;
+char *expected_str = NULL;
+
+for (i = 0; i < ENUM_ONE__MAX; i++) {
+if (i == 0) {
+expected_str = g_strdup_printf("\'%s\'", EnumOne_lookup[i]);
+} else {
+expected_str = g_strdup_printf("%s, \'%s\'",
+expected_str, EnumOne_lookup[i]);
+}
+}
+g_assert_cmpstr(EnumOne_value_str, ==, expected_str);
+}
+
 
 int main(int argc, char **argv)
 {
@@ -272,6 +289,7 @@ int main(int argc, char **argv)
 g_test_add_func("/0.15/dispatch_cmd_io", test_dispatch_cmd_io);
 g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
 g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
+g_test_add_func("/0.15/enum_value_str", test_enum_value_str);
 
 module_call_init(MODULE_INIT_QAPI);
 g_test_run();
-- 
2.9.2

Re: [Qemu-devel] 答复： Re: [PATCH v3 2/3] qapi: auto generate enum value strings

[Lin Ma]

>>> Markus Armbruster <arm...@redhat.com> 2016/10/11 星期二 下午 2:56 >>>
>Eric Blake <ebl...@redhat.com> writes:
>
>> On 10/10/2016 10:09 AM, Lin Ma wrote:
>>> 
>>> 
>>>>>> Eric Blake <ebl...@redhat.com> 2016/9/27 星期二 上午 4:17 >>>
>>>> On 09/26/2016 05:38 AM, Daniel P. Berrange wrote:
>>>>> On Mon, Sep 26, 2016 at 06:16:26PM +0800, Lin Ma wrote:
>>>>>> Automatically generate enum value strings that containing the acceptable 
>>>>>> values.
>>>>>> (Borrowwed Daniel's code.)
>>>>
>>>> s/Borrowwed/Borrowed/
>>> Sorry for the late reply, I was on vacation.
>>> Thanks for the review.
>>>>
>>>>>>
>>>>>> Signed-off-by: Lin Ma <l...@suse.com>
>>>>>> ---
>>>>>>  scripts/qapi-types.py | 2 ++
>>>>>>  scripts/qapi.py   | 9 +
>>>>>>  2 files changed, 11 insertions(+)
>>>>>
>>>>> This will need some test case coverage in tests/ somewhere, but I'm
>>>>> not sure exactly which place is best - Eric/Markus can probably advise
>>>>
>>>> tests/test-qmp-commands.c is the first one that comes to mind, for
>>>> adding another test case to an existing program.
>
>Yes, that's the closest we got.
>
>>> I'm not familiar with how to write qapi generator code and related test
>>> code at all. I'll start to dig, Any guidance is appreciated.
>>> For adding test case, Only this tests/test-qmp-commands.c needs to be
>>> modified, right? 
>>
>> Yes, I think the easiest approach is to add a new line in the main()
>> file that calls out to a new function, and the new function tests that
>> an existing QAPI enum (from tests/qapi-schema/qapi-schema-test.json) has
>> a sane conversion to a string listing all its members.  Markus may have
>> better ideas on where to place a new test, though.
>
>I think tests/test-qmp-commands.c should be split.  See
>Message-ID: <8760p7yv8n@dusky.pond.sub.org>
>http://lists.gnu.org/archive/html/qemu-devel/2016-10/msg00664.html
>
>However, splitting it out of scope of Lin Ma's work.  Go ahead and add
>to tests/test-qmp-commands.c.
>
Thanks for your information.
 
How about these changes in tests/test-qmp-commands.c ?
@@ -262,6 +262,23 @@ static void test_dealloc_partial(void)
 qapi_free_UserDefTwo(ud2);
 }
 
+/* test generated enum value str */
+static void test_enum_value_str(void)
+{
+EnumOne i;
+char *expected_str = NULL;
+
+for (i = 0; i < ENUM_ONE__MAX; i++) {
+   if (i == 0) {
+   expected_str = g_strdup_printf("\'%s\'", EnumOne_lookup[i]);
+   } else {
+   expected_str = g_strdup_printf("%s, \'%s\'",
+   
expected_str, EnumOne_lookup[i]);
+   }
+}
+g_assert_cmpstr(EnumOne_value_str, ==, expected_str);
+}
+
 
 int main(int argc, char **argv)
 {
@@ -272,6 +289,7 @@ int main(int argc, char **argv)
 g_test_add_func("/0.15/dispatch_cmd_io", test_dispatch_cmd_io);
 g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
 g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
+g_test_add_func("/0.15/enum_value_str", test_enum_value_str);
 
 module_call_init(MODULE_INIT_QAPI);
 g_test_run();
 
 
Thanks,
Lin

[Qemu-devel] 答复： Re: [PATCH v3 2/3] qapi: auto generate enum value strings

[Lin Ma]

>>> Eric Blake <ebl...@redhat.com> 2016/9/27 星期二 上午 4:17 >>>
>On 09/26/2016 05:38 AM, Daniel P. Berrange wrote:
>> On Mon, Sep 26, 2016 at 06:16:26PM +0800, Lin Ma wrote:
>>> Automatically generate enum value strings that containing the acceptable 
>>> values.
>>> (Borrowwed Daniel's code.)
>
>s/Borrowwed/Borrowed/
Sorry for the late reply, I was on vacation.
Thanks for the review.
>
>>>
>>> Signed-off-by: Lin Ma <l...@suse.com>
>>> ---
>>>  scripts/qapi-types.py | 2 ++
>>>  scripts/qapi.py   | 9 +
>>>  2 files changed, 11 insertions(+)
>> 
>> This will need some test case coverage in tests/ somewhere, but I'm
>> not sure exactly which place is best - Eric/Markus can probably advise
>
>tests/test-qmp-commands.c is the first one that comes to mind, for
>adding another test case to an existing program.
>
I'm not familiar with how to write qapi generator code and related test
code at all. I'll start to dig, Any guidance is appreciated.
For adding test case, Only this tests/test-qmp-commands.c needs to be
modified, right? 
 
Thanks,
Lin

[Qemu-devel] 答复： Re: [PATCH v3 3/3] object: Add 'help' option for all available backends and properties

[Lin Ma]

>>> "Daniel P. Berrange" <berra...@redhat.com> 2016/9/26 星期一 下午 6:41 >>>
>On Mon, Sep 26, 2016 at 06:16:27PM +0800, Lin Ma wrote:
>> '-object help' prints available user creatable backends.
>> '-object $typename,help' prints relevant properties.
>> 
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  backends/hostmem.c |  4 
>>  crypto/secret.c   |  4 
>>  crypto/tlscreds.c   |  4 
>>  include/qom/object_interfaces.h |  2 ++
>>  net/filter.c |  4 
>>  qemu-options.hx   |  7 +-
>>  qom/object_interfaces.c   | 48 +
>>  vl.c |  5 +
>>  8 files changed, 77 insertions(+), 1 deletion(-)
>> 
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index b7a208d..eea9dce 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -261,6 +261,10 @@ static void host_memory_backend_init(Object *obj)
>> HostMemPolicy_lookup,
>> 
>> host_memory_backend_get_policy,
>> 
>> host_memory_backend_set_policy, NULL);
>> +object_property_set_description(obj, "policy",
>> +"Data 
>> format: one of "
>> +
>> HostMemPolicy_value_str,
>> +
>> _abort);
>>  }
>>
>
>Adding descriptions to properties should be done separately from
>your impl of help printing, as they're independant concepts.
>
Sorry for the late reply, I was on vacation.
Ok , I'll seprate them to another patch.
 
Thanks,
Lin

[Qemu-devel] 答复： Re: [PATCH v3 1/3] qom: make base type user-creatable abstract

[Lin Ma]

>>> "Daniel P. Berrange" <berra...@redhat.com> 2016/9/26 星期一 下午 6:37 >>>
>On Mon, Sep 26, 2016 at 06:16:25PM +0800, Lin Ma wrote:
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  qom/object_interfaces.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index bf59846..9288242 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -217,6 +217,7 @@ static void register_types(void)
>>static const TypeInfo uc_interface_info = {
>>.name= TYPE_USER_CREATABLE,
>>.parent= TYPE_INTERFACE,
>> +.abstract= true,
>>.class_size = sizeof(UserCreatableClass),
>>};
>
>This doesn't make any conceptual sense. UserCreatable is an inteface and
>by definition all interfaces are abstract.
>
Sorry for the late reply, I was on vacation.
oh...indeed, add '.abstract = true' for an interface is not a good idea.

>Were you trying to fix some particular real bug here ? If so, we almost
>certainly need a different fix to what's suggested here, because QOM
>should automatically treat all interfaces as abstract by their very
>nature.
>
For other interfaces, say 'acpi-device-interface' or 'ipmi-interface', it's fine
as they are not user creatable. but for 'user-creatable', when performing
'qemu-system-x86_64 -object user-creatable,id=foo', segfault happens:
 
$ qemu-system-x86_64 -object user-creatable,id=foo
**
ERROR:/work/armbru/qemu/qom/object.c:361:object_initialize_with_type: 
assertion failed (type->instance_size >= sizeof(Object)): (0 >= 40)
Aborted (core dumped)
 
 
How about the following changes to skip TYPE_USER_CREATABLE in function
user_creatable_add_type ?
 
@@ -102,7 +102,8 @@ Object *user_creatable_add_type(const char *type, const 
char *id,
 return NULL;
 }
 
-if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
+if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE) ||
+   !strcmp(type, TYPE_USER_CREATABLE)) {
 error_setg(errp, "object type '%s' isn't supported by 
object-add",
type);
 return NULL;
...
 
@@ -229,7 +230,8 @@ int user_creatable_help_func(void *opaque, QemuOpts *opts, 
Error **errp)
 list = list->next) {
 const char *name;
 name = object_class_get_name(OBJECT_CLASS(list->data));
-   printf("%s\n", name);
+   if (strcmp(name, TYPE_USER_CREATABLE))
+   printf("%s\n", name);
...
 
 
Thanks,
Lin

[Qemu-devel] [PATCH v3 0/3] object: Add 'help' option for all available backends and properties

[Lin Ma]

Print available object backend types and the relevant properties.

V2->v3:
* make type user-creatable abstract.
* auto generate enum value strings during qemu configuration.(Borrowwed 
Daniel's code)
* save the generated enum value strings into member description of 
ObjectProperty.
* drop the judgement logic of whether a property has an enumeration type 
anymore,
  output member description of ObjectProperty directly.
* at least, user_creatable_help_func should be put after
  'object_property_add_child(object_get_root(), 
"machine",OBJECT(current_machine), ...)',
  because host_memory_backend_init needs to access an instance of type machine.

V1->V2:
* Output the acceptable values of enum types by "-object TYPE-NAME,help"

Lin Ma (3):
  qom: make base type user-creatable abstract
  qapi: auto generate enum value strings
  object: Add 'help' option for all available backends and properties

 backends/hostmem.c  |  4 
 crypto/secret.c |  4 
 crypto/tlscreds.c   |  4 
 include/qom/object_interfaces.h |  2 ++
 net/filter.c|  4 
 qemu-options.hx |  7 +-
 qom/object_interfaces.c | 49 +
 scripts/qapi-types.py   |  2 ++
 scripts/qapi.py |  9 
 vl.c|  5 +
 10 files changed, 89 insertions(+), 1 deletion(-)

-- 
2.9.2

[Qemu-devel] [PATCH v3 3/3] object: Add 'help' option for all available backends and properties

[Lin Ma]

'-object help' prints available user creatable backends.
'-object $typename,help' prints relevant properties.

Signed-off-by: Lin Ma <l...@suse.com>
---
 backends/hostmem.c  |  4 
 crypto/secret.c |  4 
 crypto/tlscreds.c   |  4 
 include/qom/object_interfaces.h |  2 ++
 net/filter.c|  4 
 qemu-options.hx |  7 +-
 qom/object_interfaces.c | 48 +
 vl.c|  5 +
 8 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index b7a208d..eea9dce 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -261,6 +261,10 @@ static void host_memory_backend_init(Object *obj)
  HostMemPolicy_lookup,
  host_memory_backend_get_policy,
  host_memory_backend_set_policy, NULL);
+object_property_set_description(obj, "policy",
+"Data format: one of "
+HostMemPolicy_value_str,
+_abort);
 }
 
 MemoryRegion *
diff --git a/crypto/secret.c b/crypto/secret.c
index 285ab7a..71d06e3 100644
--- a/crypto/secret.c
+++ b/crypto/secret.c
@@ -382,6 +382,10 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data)
qcrypto_secret_prop_get_format,
qcrypto_secret_prop_set_format,
NULL);
+object_class_property_set_description(oc, "format",
+  "Data format: one of "
+  QCryptoSecretFormat_value_str,
+  _abort);
 object_class_property_add_str(oc, "data",
   qcrypto_secret_prop_get_data,
   qcrypto_secret_prop_set_data,
diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index a896553..d3af38a 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -237,6 +237,10 @@ qcrypto_tls_creds_class_init(ObjectClass *oc, void *data)
qcrypto_tls_creds_prop_get_endpoint,
qcrypto_tls_creds_prop_set_endpoint,
NULL);
+object_class_property_set_description(oc, "endpoint",
+  "Data format: one of "
+  QCryptoTLSCredsEndpoint_value_str,
+  _abort);
 object_class_property_add_str(oc, "priority",
   qcrypto_tls_creds_prop_get_priority,
   qcrypto_tls_creds_prop_set_priority,
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 8b17f4d..197cd5f 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -165,4 +165,6 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 void user_creatable_del(const char *id, Error **errp);
 
+int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp);
+
 #endif
diff --git a/net/filter.c b/net/filter.c
index 1dfd2ca..205fb49 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -182,6 +182,10 @@ static void netfilter_init(Object *obj)
  NetFilterDirection_lookup,
  netfilter_get_direction, netfilter_set_direction,
  NULL);
+object_property_set_description(obj, "queue",
+"Data format: one of "
+NetFilterDirection_value_str,
+_abort);
 object_property_add_str(obj, "status",
 netfilter_get_status, netfilter_set_status,
 NULL);
diff --git a/qemu-options.hx b/qemu-options.hx
index 0b621bb..978d37b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3759,7 +3759,9 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
 "create a new object of type TYPENAME setting properties\n"
 "in the order they are specified.  Note that the 'id'\n"
 "property must be set.  These objects are placed in the\n"
-"'/objects' path.\n",
+"'/objects' path.\n"
+"Use '-object help' to print available backend types and\n"
+"'-object typename,help' to print relevant properties.\n",
 QEMU_ARCH_ALL)
 STEXI
 @item -object @var{typename}[,@var{prop1}=@var{value1},...]
@@ -3769,6 +3771,9 @@ in the order they are specifi

[Qemu-devel] [PATCH v3 1/3] qom: make base type user-creatable abstract

[Lin Ma]

Signed-off-by: Lin Ma <l...@suse.com>
---
 qom/object_interfaces.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf59846..9288242 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -217,6 +217,7 @@ static void register_types(void)
 static const TypeInfo uc_interface_info = {
 .name  = TYPE_USER_CREATABLE,
 .parent= TYPE_INTERFACE,
+.abstract  = true,
 .class_size = sizeof(UserCreatableClass),
 };
 
-- 
2.9.2

[Qemu-devel] [PATCH v3 2/3] qapi: auto generate enum value strings

[Lin Ma]

Automatically generate enum value strings that containing the acceptable values.
(Borrowwed Daniel's code.)

Signed-off-by: Lin Ma <l...@suse.com>
---
 scripts/qapi-types.py | 2 ++
 scripts/qapi.py   | 9 +
 2 files changed, 11 insertions(+)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index dabc42e..0446839 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -202,9 +202,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
 self._btin += gen_enum(name, values, prefix)
 if do_builtins:
 self.defn += gen_enum_lookup(name, values, prefix)
+self._btin += gen_enum_value_str(name, values)
 else:
 self._fwdecl += gen_enum(name, values, prefix)
 self.defn += gen_enum_lookup(name, values, prefix)
+self._fwdecl += gen_enum_value_str(name, values)
 
 def visit_array_type(self, name, info, element_type):
 if isinstance(element_type, QAPISchemaBuiltinType):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 21bc32f..d11c414 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1649,6 +1649,15 @@ const char *const %(c_name)s_lookup[] = {
 return ret
 
 
+def gen_enum_value_str(name, values):
+return mcgen('''
+
+#define %(c_name)s_value_str "%(value_str)s"
+''',
+c_name=c_name(name),
+value_str=", ".join(["'%s'" % c for c in values]))
+
+
 def gen_enum(name, values, prefix=None):
 # append automatically generated _MAX value
 enum_values = values + ['_MAX']
-- 
2.9.2

[Qemu-devel] [PATCH] iothread: check iothread->ctx before aio_context_unref to avoid assertion

[Lin Ma]

if iothread->ctx is set to NULL, aio_context_unref triggers the assertion:
g_source_unref: assertion 'source != NULL' failed.
The patch fixes it.

Signed-off-by: Lin Ma <l...@suse.com>
---
 iothread.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/iothread.c b/iothread.c
index fb08a60..fbeb8de 100644
--- a/iothread.c
+++ b/iothread.c
@@ -75,6 +75,9 @@ static void iothread_instance_finalize(Object *obj)
 iothread_stop(obj, NULL);
 qemu_cond_destroy(>init_done_cond);
 qemu_mutex_destroy(>init_done_lock);
+if (!iothread->ctx) {
+return;
+}
 aio_context_unref(iothread->ctx);
 }
 
-- 
2.9.2

[Qemu-devel] [PATCH] maint: Add module_block.h to .gitignore

[Lin Ma]

Commit 0c0c1fd9 generated module_block.h automatically, Add it to .gitignore to
avoid checking in it by 'git add .'.

Signed-off-by: Lin Ma <l...@suse.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index c91d018..97aca6c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -55,6 +55,7 @@
 /qemu-monitor-info.texi
 /qemu-version.h
 /qemu-version.h.tmp
+/module_block.h
 /vscclient
 /fsdev/virtfs-proxy-helper
 *.[1-9]
-- 
2.9.2

Re: [Qemu-devel] 答复： Re: [PATCH v2] object: Add 'help' option for all available backends and properties

[Lin Ma]

>>> Markus Armbruster  2016/9/20 星期二 上午 1:13 >>>
>Andreas Färber  writes:
>
>> Hi Lin and Markus,
>>
>> Am 19.09.2016 um 13:58 schrieb Markus Armbruster:
>[...]
>>> You're messing with struct EnumProperty because you want more help than
>>> what ObjectPropertyInfo can provice.
>>> 
>>> Digression on the meaning of ObjectPropertyInfo.  This is its
>>> definition:
>>> 
>>> ##
>>> # @ObjectPropertyInfo:
>>> #
>>> # @name: the name of the property
>>> #
>>> # @type: the type of the property.  This will typically come in one of four
>>> #   forms:
>>> #
>>> #   1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
>>> #  These types are mapped to the appropriate JSON type.
>>> #
>>> #   2) A child type in the form 'child' where subtype is a qdev
>>> #  device type name.  Child properties create the composition 
>>> tree.
>>> #
>>> #   3) A link type in the form 'link' where subtype is a qdev
>>> #  device type name.  Link properties form the device model 
>>> graph.
>>> #
>>> # Since: 1.2
>>> ##
>>> { 'struct': 'ObjectPropertyInfo',
>>>   'data': { 'name': 'str', 'type': 'str' } }
>>> 
>>> @type is documented to be either a primitive type, a child type or a
>>> link.  "Primitive type" isn't defined.  The examples given suggest it's
>>> a QAPI built-in type.  If that's correct, clause 1) does not cover
>>> enumeration types.  However, it doesn't seem to be correct: I observ
>>> 'string', not 'str'.  Paolo, Andreas, any idea what @type is supposed to
>>> mean?
>>> 
>>> End of digression.
>>> 
>>> All ObjectPropertyInfo gives you is two strings: a name and a type.  If
>>> you need more information than that, you have to add a proper interface
>>> to get it!  Say a function that takes an object and a property name, and
>>> returns information about the property's type.  Probably should be two
>>> functions, one that maps QOM object and property name to the property's
>>> QOM type, one that maps a QOM type to QOM type information.
>>> 
>>> In other words, you need QOM object and type introspection.  Paolo,
>>> Andreas, if that already exists in some form, please point us to it.
>>
>> Could you say what exactly you want to introspect here?
>
>Context: Lin wants to implement "-object TYPE-NAME,help", similar to
>"-device DRIVER-NAME,help", i.e. list the available properties of
>TYPE-NAME.
>
>His proposed patch tries to give better help for enumeration types by
>listing the acceptable values.  The code that does it is an unacceptable
>hack, though.  We're trying to figure out a way to provide such help
>cleanly.
>
>One way to do it would be introspecting QOM *types*.  Find the
>property's type, figure out what kind of type it is, if it's an
>enumeration type, find its members, ...
>
>> Both ObjectClass and Object have a list of properties that together form
>> the list of properties that can be set on an instance. So you'll need to
>> instantiate the object like I think we do for devices. Then you can
>> recursively enumerate the properties to get their names and types (and
>> possibly put them into a new list for alphabetical sorting if desired).
>
>Lin's code uses object_new() to instantiate a dummy object,
>object_property_iter_init() and object_property_iter_next() to find the
>properties.  Sounds okay so far, doesn't it?
>
>Hmm, ObjectProperty has members name, type, description.  Could
>description be useful?  I guess it's set with
>object_property_set_description().  I can see only a few dozen calls,
>which makes me suspect it's null more often than not.

Saving acceptable values of enumeration types into member description
of ObjectProperty is a good idea.
 
The member description of ObjectProperty instance of any user-creatable
object is NULL so far, If I use object_property_set_description() to fill the
acceptable values of enumeration type into the description in function
object_property_add_enum and object_cl
ass_property_add_enum, Then I
can use this description to judge whether a ObjectProperty instance' type
is enumeration or not in function user_creatable_help_func. In this case, If
member description is not NULL, it means this ObjectProperty instance is
an enumeration.
Any suggestions?
 
If this way makes sense, Then Should I add a member description for
ObjectPropertyInfo as well?
 
>
>When it's null we could still fall back to a description of the type.
>Does such a thing exist?  Enumeration types could provide one listing
>their values.
>
>Other ideas?

[Qemu-devel] 答复： Re: [PATCH v2] object: Add 'help' option for all available backends and properties

[Lin Ma]

>>> "Daniel P. Berrange" <berra...@redhat.com> 2016/9/12 星期一 下午 11:56 >>>
>On Sun, Sep 11, 2016 at 01:53:01PM +0800, Lin Ma wrote:
>> '-object help' prints available user creatable backends.
>> '-object $typename,help' prints relevant properties.
>> 
>> Signed-off-by: Lin Ma <l...@suse.com>
>> ---
>>  include/qom/object_interfaces.h |   2 +
>>  qemu-options.hx   |   7 ++-
>>  qom/object_interfaces.c   | 112 
>>  vl.c |   5 ++
>>  4 files changed, 125 insertions(+), 1 deletion(-)
>
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index bf59846..4ee8643 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -5,6 +5,7 @@
>>  #include "qapi-visit.h"
>>  #include "qapi/qmp-output-visitor.h"
>>  #include "qapi/opts-visitor.h"
>> +#include "qemu/help_option.h"
>>  
>>  void user_creatable_complete(Object *obj, Error **errp)
>>  {
>> @@ -212,6 +213,117 @@ void user_creatable_del(const char *id, Error **errp)
>>object_unparent(obj);
>>  }
>>  
>> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
>> +{
>> +Visitor *v;
>> +char *type = NULL;
>> +Error *local_err = NULL;
>> +
>> +int i;
>> +char *values = NULL;
>> +Object *obj;
>> +ObjectPropertyInfoList *props = NULL;
>> +ObjectProperty *prop;
>> +ObjectPropertyIterator iter;
>> +ObjectPropertyInfoList *start;
>> +
>> +struct EnumProperty {
>> +const char * const *strings;
>> +int (*get)(Object *, Error **);
>> +void (*set)(Object *, int, Error **);
>> +} *enumprop;
>
>Ewww, this struct is declared privately in object.c and
>you're declaring it here so you can poke at private
>data belonging to the object internal impl. This is
>not ok in any way.
>
Yes, this way is ugly.
What I want to do is parsing the enum <-> string mappings through the 
EnumProperty
to make the output more reasonable.
It should be parsed in object.c, But I can't assume the size of enum str list, 
then can't
pre-alloc it in user_creatable_help_func. So far I havn't figured out a good 
way to return
a string array that including the enum str list to user_creatable_help_func for 
printing.
May I have your suggestions?

>> +v = opts_visitor_new(opts);
>> +visit_start_struct(v, NULL, NULL, 0, _err);
>> +if (local_err) {
>> +goto out;
>> +}
>> +
>> +visit_type_str(v, "qom-type", , _err);
>> +if (local_err) {
>> +goto out_visit;
>> +}
>> +
>> +if (type && is_help_option(type)) {
>> +printf("Available object backend types:\n");
>> +GSList *list = object_class_get_list(TYPE_USER_CREATABLE, false);
>> +while (list) {
>> +const char *name;
>> +name = object_class_get_name(OBJECT_CLASS(list->data));
>> +if (strcmp(name, TYPE_USER_CREATABLE)) {
>> +printf("%s\n", name);
>> +}
>> +list = list->next;
>> +}
>> +g_slist_free(list);
>> +goto out_visit;
>> +}
>> +
>> +if (!type || !qemu_opt_has_help_opt(opts)) {
>> +visit_end_struct(v, NULL);
>> +return 0;
>> +}
>> +
>> +if (!object_class_by_name(type)) {
>> +printf("invalid object type: %s\n", type);
>> +goto out_visit;
>> +}
>> +obj = object_new(type);
>> +object_property_iter_init(, obj);
>> +
>> +while ((prop = object_property_iter_next())) {
>> +ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));
>> +entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
>> +entry->next = props;
>> +props = entry;
>> +entry->value->name = g_strdup(prop->name);
>> +i = 0;
>> +enumprop = prop->opaque;
>> +if (!g_str_equal(prop->type, "string") && \
>> +!g_str_equal(prop->type, "bool") && \
>> +!g_str_equal(prop->type, "struct tm") && \
>> +!g_str_equal(prop->type, "int") && \
>> +!g_str_equal(