Re: [PATCH v2 07/39] tests: Avoid using hardcoded /tmp in test cases
Marc-André Lureau writes:
> Hi
>
> On Tue, Sep 20, 2022 at 2:47 PM Bin Meng wrote:
>
>> From: Bin Meng
>>
>> Lots of test cases were written to use hardcoded /tmp directory for
>> temporary files. To avoid this, we update them to use g_dir_make_tmp()
>> or g_file_open_tmp() when appropriate.
>>
>> Signed-off-by: Bin Meng
>> ---
>>
>> Changes in v2:
>> - Use g_dir_make_tmp(), g_file_open_tmp() when appropriate
>>
>> tests/qtest/fuzz/generic_fuzz_configs.h | 4 ++--
>> tests/qtest/ahci-test.c | 19 +++
>> tests/qtest/aspeed_smc-test.c | 5 ++---
>> tests/qtest/boot-serial-test.c | 9 +
>> tests/qtest/cxl-test.c | 15 ++-
>> tests/qtest/fdc-test.c | 5 +++--
>> tests/qtest/fuzz/virtio_blk_fuzz.c | 4 ++--
>> tests/qtest/hd-geo-test.c | 24 +++-
>> tests/qtest/ide-test.c | 10 ++
>> tests/qtest/libqtest.c | 12
>> tests/qtest/migration-test.c| 7 ---
>> tests/qtest/pflash-cfi02-test.c | 8 +---
>> tests/qtest/qmp-test.c | 6 --
>> tests/qtest/vhost-user-blk-test.c | 3 ++-
>> tests/qtest/vhost-user-test.c | 8
>> tests/qtest/virtio-blk-test.c | 4 ++--
>> tests/qtest/virtio-scsi-test.c | 4 ++--
>> tests/unit/test-image-locking.c | 8
>> tests/unit/test-qga.c | 2 +-
>> tests/vhost-user-bridge.c | 3 +--
>> 20 files changed, 85 insertions(+), 75 deletions(-)
>>
>> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
>> b/tests/qtest/fuzz/generic_fuzz_configs.h
>> index 0775e6702b..a825b78c14 100644
>> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
>> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
>> @@ -20,8 +20,8 @@ typedef struct generic_fuzz_config {
>> } generic_fuzz_config;
>>
>> static inline gchar *generic_fuzzer_virtio_9p_args(void){
>> -char tmpdir[] = "/tmp/qemu-fuzz.XX";
>> -g_assert_nonnull(g_mkdtemp(tmpdir));
>> +g_autofree char *tmpdir = g_dir_make_tmp("qemu-fuzz.XX", NULL);
>> +g_assert_nonnull(tmpdir);
>>
>> return g_strdup_printf("-machine q35 -nodefaults "
>> "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
>> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
>> index f1e510b0ac..00524f14c6 100644
>> --- a/tests/qtest/ahci-test.c
>> +++ b/tests/qtest/ahci-test.c
>> @@ -44,9 +44,9 @@
>> #define TEST_IMAGE_SIZE_MB_SMALL 64
>>
>> /*** Globals ***/
>> -static char tmp_path[] = "/tmp/qtest.XX";
>> -static char debug_path[] = "/tmp/qtest-blkdebug.XX";
>> -static char mig_socket[] = "/tmp/qtest-migration.XX";
>> +static char *tmp_path;
>> +static char *debug_path;
>> +static char *mig_socket;
>> static bool ahci_pedantic;
>> static const char *imgfmt;
>> static unsigned test_image_size_mb;
>> @@ -1437,10 +1437,10 @@ static void test_ncq_simple(void)
>>
>> static int prepare_iso(size_t size, unsigned char **buf, char **name)
>> {
>> -char cdrom_path[] = "/tmp/qtest.iso.XX";
>> +g_autofree char *cdrom_path;
>>
>
> Whenever introducing g_auto* usage, please make sure to initialize the
> variable to NULL or a correct value.
Potential food for checkpatch.pl.
[...]
Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Fri, Sep 23, 2022 at 11:55 AM ho...@yusur.tech wrote:
>
> On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote:
>
>
> >On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> > wrote:
> >>
> >> If I read your response on the other thread correctly, this change is
> >> intended
> >>
> >> to prioritize the MAC address exposed by DPDK over the one provided by the
> >>
> >> QEMU command line? Sounds reasonable in principle, but I would get
> >> confirmation
> >>
> >> from vDPA/vhost-net maintainers.
>
> >I think the best way is to (and it seems easier)
>
> >1) have the management layer to make sure the mac came from cli
> >matches the underlayer vDPA
>
> Agreed, that's no problem.
>
> >2) having a sanity check and fail the device initialization if they don't
> >match
>
> However, one MAC address for integrity check is from the cli, and the other
> MAC address is from the vDPA device,
> How to get it?
VHOST_USER_GET_CONFIG?
Thanks
>
> The current situation is if MAC came from cli don't match the underlayer
> vDPA, the virtual machine can still start without any hints.
>
> Thanks
>
>
> >Thanks
>
> >>
> >>
> >>
> >> That said the way you’re hacking the vhost-user code breaks a valuable
> >> check for
> >>
> >> bad vhost-user-blk backends. I would suggest finding another
> >> implementation which
> >>
> >> does not regress functionality for other device types.
> >>
> >>
> >>
> >>
> >>
> >> >From: Hao Chen
> >>
> >> >
> >>
> >> >When use dpdk-vdpa tests vdpa device. You need to specify the mac address
> >> >to
> >>
> >> >start the virtual machine through libvirt or qemu, but now, the libvirt or
> >>
> >> >qemu can call dpdk vdpa vendor driver's ops .get_config through
> >> >vhost_net_get_config
> >>
> >> >to get the mac address of the vdpa hardware without manual configuration.
> >>
> >> >
> >>
> >> >v1->v2:
> >>
> >> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
> >>
> >> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> >>
> >> >We only need the mac address and don't care about the status field.
> >>
> >> >
> >>
> >> >Signed-off-by: Hao Chen
> >>
> >> >---
> >>
> >> > hw/block/vhost-user-blk.c | 1 -
> >>
> >> > hw/net/virtio-net.c | 7 +++
> >>
> >> > hw/virtio/vhost-user.c| 19 ---
> >>
> >> > 3 files changed, 7 insertions(+), 20 deletions(-)
> >>
> >> >
> >>
> >> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>
> >> >index 9117222456..5dca4eab09 100644
> >>
> >> >--- a/hw/block/vhost-user-blk.c
> >>
> >> >+++ b/hw/block/vhost-user-blk.c
> >>
> >> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> >> >Error **errp)
> >>
> >> >
> >>
> >> > vhost_dev_set_config_notifier(>dev, _ops);
> >>
> >> >
> >>
> >> >-s->vhost_user.supports_config = true;
> >>
> >> > ret = vhost_dev_init(>dev, >vhost_user,
> >> > VHOST_BACKEND_TYPE_USER, 0,
> >>
> >> > errp);
> >>
> >> > if (ret < 0) {
> >>
> >> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>
> >> >index dd0d056fde..90405083b1 100644
> >>
> >> >--- a/hw/net/virtio-net.c
> >>
> >> >+++ b/hw/net/virtio-net.c
> >>
> >> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice
> >> >*vdev, uint8_t *config)
> >>
> >> > }
> >>
> >> > memcpy(config, , n->config_size);
> >>
> >> > }
> >>
> >> >+} else if (nc->peer && nc->peer->info->type ==
> >> >NET_CLIENT_DRIVER_VHOST_USER) {
> >>
> >> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> >> >*),
> >>
> >> >+ n->config_size);
> >>
> >> >+if (ret != -1) {
> >>
> >> >+ /* Automatically obtain the mac address of the vdpa device
> >>
> >> >+* when using the dpdk vdpa */
> >>
> >> >+memcpy(config, , ETH_ALEN);
> >>
> >> > }
> >>
> >> > }
> >>
> >> >
> >>
> >> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>
> >> >index bd24741be8..8b01078249 100644
> >>
> >> >--- a/hw/virtio/vhost-user.c
> >>
> >> >+++ b/hw/virtio/vhost-user.c
> >>
> >> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> >> >*dev, void *opaque,
> >>
> >> > }
> >>
> >> >
> >>
> >> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >>
> >> >-bool supports_f_config = vus->supports_config ||
> >>
> >> >-(dev->config_ops &&
> >> >dev->config_ops->vhost_dev_config_notifier);
> >>
> >> > uint64_t protocol_features;
> >>
> >> >
> >>
> >> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >>
> >> >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct
> >> >vhost_dev *dev, void *opaque,
> >>
> >> > */
> >>
> >> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >>
> >> >
> >>
> >> >-if (supports_f_config) {
> >>
> >> >-if (!virtio_has_feature(protocol_features,
> >>
>
Re: Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Thu, 22 Sep 2022 09:34:41 +0800 Jason Wang wrote:
>On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> wrote:
>>
>> If I read your response on the other thread correctly, this change is
>> intended
>>
>> to prioritize the MAC address exposed by DPDK over the one provided by the
>>
>> QEMU command line? Sounds reasonable in principle, but I would get
>> confirmation
>>
>> from vDPA/vhost-net maintainers.
>I think the best way is to (and it seems easier)
>1) have the management layer to make sure the mac came from cli
>matches the underlayer vDPA
Agreed, that's no problem.
>2) having a sanity check and fail the device initialization if they don't match
However, one MAC address for integrity check is from the cli, and the other MAC
address is from the vDPA device,
How to get it?
The current situation is if MAC came from cli don't match the underlayer vDPA,
the virtual machine can still start without any hints.
Thanks
>Thanks
>>
>>
>>
>> That said the way you’re hacking the vhost-user code breaks a valuable check
>> for
>>
>> bad vhost-user-blk backends. I would suggest finding another implementation
>> which
>>
>> does not regress functionality for other device types.
>>
>>
>>
>>
>>
>> >From: Hao Chen
>>
>> >
>>
>> >When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
>>
>> >start the virtual machine through libvirt or qemu, but now, the libvirt or
>>
>> >qemu can call dpdk vdpa vendor driver's ops .get_config through
>> >vhost_net_get_config
>>
>> >to get the mac address of the vdpa hardware without manual configuration.
>>
>> >
>>
>> >v1->v2:
>>
>> >Only copy ETH_ALEN data of netcfg for some vdpa device such as
>>
>> >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
>>
>> >We only need the mac address and don't care about the status field.
>>
>> >
>>
>> >Signed-off-by: Hao Chen
>>
>> >---
>>
>> > hw/block/vhost-user-blk.c | 1 -
>>
>> > hw/net/virtio-net.c | 7 +++
>>
>> > hw/virtio/vhost-user.c| 19 ---
>>
>> > 3 files changed, 7 insertions(+), 20 deletions(-)
>>
>> >
>>
>> >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>
>> >index 9117222456..5dca4eab09 100644
>>
>> >--- a/hw/block/vhost-user-blk.c
>>
>> >+++ b/hw/block/vhost-user-blk.c
>>
>> >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
>> >Error **errp)
>>
>> >
>>
>> > vhost_dev_set_config_notifier(>dev, _ops);
>>
>> >
>>
>> >-s->vhost_user.supports_config = true;
>>
>> > ret = vhost_dev_init(>dev, >vhost_user, VHOST_BACKEND_TYPE_USER,
>> > 0,
>>
>> > errp);
>>
>> > if (ret < 0) {
>>
>> >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>
>> >index dd0d056fde..90405083b1 100644
>>
>> >--- a/hw/net/virtio-net.c
>>
>> >+++ b/hw/net/virtio-net.c
>>
>> >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
>> >uint8_t *config)
>>
>> > }
>>
>> > memcpy(config, , n->config_size);
>>
>> > }
>>
>> >+} else if (nc->peer && nc->peer->info->type ==
>> >NET_CLIENT_DRIVER_VHOST_USER) {
>>
>> >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
>> >*),
>>
>> >+ n->config_size);
>>
>> >+if (ret != -1) {
>>
>> >+ /* Automatically obtain the mac address of the vdpa device
>>
>> >+* when using the dpdk vdpa */
>>
>> >+memcpy(config, , ETH_ALEN);
>>
>> > }
>>
>> > }
>>
>> >
>>
>> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>
>> >index bd24741be8..8b01078249 100644
>>
>> >--- a/hw/virtio/vhost-user.c
>>
>> >+++ b/hw/virtio/vhost-user.c
>>
>> >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> >*dev, void *opaque,
>>
>> > }
>>
>> >
>>
>> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>>
>> >-bool supports_f_config = vus->supports_config ||
>>
>> >-(dev->config_ops &&
>> >dev->config_ops->vhost_dev_config_notifier);
>>
>> > uint64_t protocol_features;
>>
>> >
>>
>> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>>
>> >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> >*dev, void *opaque,
>>
>> > */
>>
>> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>>
>> >
>>
>> >-if (supports_f_config) {
>>
>> >-if (!virtio_has_feature(protocol_features,
>>
>> >-VHOST_USER_PROTOCOL_F_CONFIG)) {
>>
>> >-error_setg(errp, "vhost-user device expecting "
>>
>> >- "VHOST_USER_PROTOCOL_F_CONFIG but the
>> >vhost-user backend does "
>>
>> >- "not support it.");
>>
>> >-return -EPROTO;
>>
>> >-}
>>
>> >-} else {
>>
>> >-if (virtio_has_feature(protocol_features,
>>
>> >-
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On 2022/9/22 18:19, Michael S. Tsirkin wrote:
On Thu, Sep 22, 2022 at 11:02:56AM +0100, Alex Bennée wrote:
"Michael S. Tsirkin" writes:
On Wed, Sep 21, 2022 at 07:23:12PM +0100, Alex Bennée wrote:
chenh writes:
From: Hao Chen
When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
start the virtual machine through libvirt or qemu, but now, the libvirt or
qemu can call dpdk vdpa vendor driver's ops .get_config through
vhost_net_get_config
to get the mac address of the vdpa hardware without manual configuration.
v1->v2:
Only copy ETH_ALEN data of netcfg for some vdpa device such as
NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
We only need the mac address and don't care about the status field.
Signed-off-by: Hao Chen
---
hw/block/vhost-user-blk.c | 1 -
hw/net/virtio-net.c | 7 +++
hw/virtio/vhost-user.c| 19 ---
3 files changed, 7 insertions(+), 20 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..5dca4eab09 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error
**errp)
vhost_dev_set_config_notifier(>dev, _ops);
-s->vhost_user.supports_config = true;
NACK from me. The supports_config flag is there for a reason.
Alex please, do not send NACKs. If you feel compelled to stress
your point, provide extra justification instead. Thanks!
OK I was objecting to ripping out the common vhost-user code which was
implemented as a fix for behaviour found while attempting to upstream:
Subject: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups
Date: Tue, 2 Aug 2022 10:49:48 +0100
Message-Id: <20220802095010.3330793-1-alex.ben...@linaro.org>
I vhost-user-blk wants to suppress its use of vhost-user config messages
I guess that should be a control option but it sounds like a buggy
back-end.
Thanks for the review!
QEMU needs to obtain the mac address of the underlying vdpa hardware
through the 'vhost_user_get_config' function, but this part of the check
of virtio_blk makes 'vhost_user_get_config' unusable in virtio-net,
because 'vhost_user_get_config' depends on the
VHOST_USER_PROTOCOL_F_CONFIG feature, which is the key point of the
problem.
Currently I only need to use 'vhost_user_get_config' function, it seems
not necessary to implement dev->config_ops &&
dev->config_ops->vhost_dev_config_notifier in virtio-net.
Can this part of the checks be moved elsewhere? I don't know how to skip
this part of the virtio blk related checks to achieve my functionality,
so I removed those checks.
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index bd24741be8..8b01078249 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev,
void *opaque,
}
if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
-bool supports_f_config = vus->supports_config ||
-(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
uint64_t protocol_features;
dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
*dev, void *opaque,
*/
protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
-if (supports_f_config) {
-if (!virtio_has_feature(protocol_features,
-VHOST_USER_PROTOCOL_F_CONFIG)) {
-error_setg(errp, "vhost-user device expecting "
- "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user backend
does "
- "not support it.");
-return -EPROTO;
-}
-} else {
-if (virtio_has_feature(protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIG)) {
-warn_reportf_err(*errp, "vhost-user backend supports "
- "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does
not.");
-protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
-}
-}
-
/* final set of protocol features */
dev->protocol_features = protocol_features;
err = vhost_user_set_protocol_features(dev, dev->protocol_features);
--
Alex Bennée
--
Alex Bennée
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On 2022/9/22 17:56, Michael S. Tsirkin wrote:
On Thu, Sep 22, 2022 at 09:34:41AM +0800, Jason Wang wrote:
On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
wrote:
If I read your response on the other thread correctly, this change is intended
to prioritize the MAC address exposed by DPDK over the one provided by the
QEMU command line? Sounds reasonable in principle, but I would get confirmation
from vDPA/vhost-net maintainers.
I think the best way is to (and it seems easier)
1) have the management layer to make sure the mac came from cli
matches the underlayer vDPA
2) having a sanity check and fail the device initialization if they don't match
Thanks
I think I agree, we should not overwrite user supplied arguments.
I think the case where we might want to get the mac from VDPA
and use it is when it's been assigned randomly as opposed to coming from cli.
Yes, when the cli passes in a random mac address that is inconsistent
with the vdpa device, qemu still starts the virtual machine as usual,
but in this case, the qemu and vdpa environments do not work correctly.
So I want to get mac address directly from vdpa device instead of cli.
Having a sanity check and fail the device initialization if they don't
match is a good idea, but it seems more convenient to directly overwrite
the cli mac address.
That said the way you’re hacking the vhost-user code breaks a valuable check for
bad vhost-user-blk backends. I would suggest finding another implementation
which
does not regress functionality for other device types.
From: Hao Chen
When use dpdk-vdpa tests vdpa device. You need to specify the mac address to
start the virtual machine through libvirt or qemu, but now, the libvirt or
qemu can call dpdk vdpa vendor driver's ops .get_config through
vhost_net_get_config
to get the mac address of the vdpa hardware without manual configuration.
v1->v2:
Only copy ETH_ALEN data of netcfg for some vdpa device such as
NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
We only need the mac address and don't care about the status field.
Signed-off-by: Hao Chen
---
hw/block/vhost-user-blk.c | 1 -
hw/net/virtio-net.c | 7 +++
hw/virtio/vhost-user.c| 19 ---
3 files changed, 7 insertions(+), 20 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 9117222456..5dca4eab09 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error
**errp)
vhost_dev_set_config_notifier(>dev, _ops);
-s->vhost_user.supports_config = true;
ret = vhost_dev_init(>dev, >vhost_user, VHOST_BACKEND_TYPE_USER, 0,
errp);
if (ret < 0) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..90405083b1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
uint8_t *config)
}
memcpy(config, , n->config_size);
}
+} else if (nc->peer && nc->peer->info->type ==
NET_CLIENT_DRIVER_VHOST_USER) {
+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *),
+ n->config_size);
+if (ret != -1) {
+ /* Automatically obtain the mac address of the vdpa device
+* when using the dpdk vdpa */
+memcpy(config, , ETH_ALEN);
}
}
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index bd24741be8..8b01078249 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev,
void *opaque,
}
if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
-bool supports_f_config = vus->supports_config ||
-(dev->config_ops && dev->config_ops->vhost_dev_config_notifier);
uint64_t protocol_features;
dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
*dev, void *opaque,
*/
protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
-if (supports_f_config) {
-if (!virtio_has_feature(protocol_features,
-VHOST_USER_PROTOCOL_F_CONFIG)) {
-error_setg(errp, "vhost-user device expecting "
- "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user backend
does "
- "not support it.");
-return -EPROTO;
-}
-} else {
-if (virtio_has_feature(protocol_features,
- VHOST_USER_PROTOCOL_F_CONFIG)) {
-warn_reportf_err(*errp, "vhost-user backend supports "
- "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does
not.");
-protocol_features &= ~(1ULL <<
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Fri, Sep 23, 2022 at 11:33 AM 陈浩 wrote:
>
>
> On 2022/9/22 17:56, Michael S. Tsirkin wrote:
> > On Thu, Sep 22, 2022 at 09:34:41AM +0800, Jason Wang wrote:
> >> On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> >> wrote:
> >>> If I read your response on the other thread correctly, this change is
> >>> intended
> >>>
> >>> to prioritize the MAC address exposed by DPDK over the one provided by the
> >>>
> >>> QEMU command line? Sounds reasonable in principle, but I would get
> >>> confirmation
> >>>
> >>> from vDPA/vhost-net maintainers.
> >> I think the best way is to (and it seems easier)
> >>
> >> 1) have the management layer to make sure the mac came from cli
> >> matches the underlayer vDPA
> >> 2) having a sanity check and fail the device initialization if they don't
> >> match
> >>
> >> Thanks
> > I think I agree, we should not overwrite user supplied arguments.
> > I think the case where we might want to get the mac from VDPA
> > and use it is when it's been assigned randomly as opposed to coming from
> > cli.
> Yes, when the cli passes in a random mac address that is inconsistent
> with the vdpa device, qemu still starts the virtual machine as usual,
> but in this case, the qemu and vdpa environments do not work correctly.
> So I want to get mac address directly from vdpa device instead of cli.
Let's teach the management to do that. It has a lot of advantages, more below.
Not sure for the DPDK case, but kernel vDPA allows the mgmt to
provision the vDPA with a mac address which could be used in this
case.
>
> Having a sanity check and fail the device initialization if they don't
> match is a good idea, but it seems more convenient to directly overwrite
> the cli mac address.
This will confuse the management where it may do a lot of mac related setups.
Thanks
> >
> >
> >>>
> >>>
> >>> That said the way you’re hacking the vhost-user code breaks a valuable
> >>> check for
> >>>
> >>> bad vhost-user-blk backends. I would suggest finding another
> >>> implementation which
> >>>
> >>> does not regress functionality for other device types.
> >>>
> >>>
> >>>
> >>>
> >>>
> From: Hao Chen
> When use dpdk-vdpa tests vdpa device. You need to specify the mac
> address to
> start the virtual machine through libvirt or qemu, but now, the libvirt
> or
> qemu can call dpdk vdpa vendor driver's ops .get_config through
> vhost_net_get_config
> to get the mac address of the vdpa hardware without manual configuration.
> v1->v2:
> Only copy ETH_ALEN data of netcfg for some vdpa device such as
> NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> We only need the mac address and don't care about the status field.
> Signed-off-by: Hao Chen
> ---
> hw/block/vhost-user-blk.c | 1 -
> hw/net/virtio-net.c | 7 +++
> hw/virtio/vhost-user.c| 19 ---
> 3 files changed, 7 insertions(+), 20 deletions(-)
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..5dca4eab09 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> Error **errp)
> vhost_dev_set_config_notifier(>dev, _ops);
> -s->vhost_user.supports_config = true;
> ret = vhost_dev_init(>dev, >vhost_user,
> VHOST_BACKEND_TYPE_USER, 0,
> errp);
> if (ret < 0) {
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd0d056fde..90405083b1 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice
> *vdev, uint8_t *config)
> }
> memcpy(config, , n->config_size);
> }
> +} else if (nc->peer && nc->peer->info->type ==
> NET_CLIENT_DRIVER_VHOST_USER) {
> +ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> *),
> + n->config_size);
> +if (ret != -1) {
> + /* Automatically obtain the mac address of the vdpa
> device
> +* when using the dpdk vdpa */
> +memcpy(config, , ETH_ALEN);
> }
> }
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index bd24741be8..8b01078249 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct
> vhost_dev *dev, void *opaque,
> }
> if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> -bool supports_f_config = vus->supports_config ||
> -(dev->config_ops &&
> dev->config_ops->vhost_dev_config_notifier);
> uint64_t protocol_features;
>
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
Hi Stefan,
On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth wrote:
>
> On 21/09/2022 14.18, Bin Meng wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 9:28 PM Bin Meng wrote:
> >>
> >> At present packaging the required DLLs of QEMU executables is a
> >> manual process, and error prone.
> >>
> >> Improve scripts/nsis.py by adding a logic to automatically package
> >> required DLLs of QEMU executables.
> >>
> >> 'make installer' is tested in the cross-build on Linux in CI, but
> >> not in the Windows native build. Update CI to test the installer
> >> generation on Windows too.
> >>
> >> During testing a 32-bit build issue was exposed in block/nfs.c and
> >> the fix is included in this series.
> >>
> >>
> >> Bin Meng (7):
> >>scripts/nsis.py: Drop the unnecessary path separator
> >>scripts/nsis.py: Fix destination directory name when invoked on
> >> Windows
> >>scripts/nsis.py: Automatically package required DLLs of QEMU
> >> executables
> >>.gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> >>block/nfs: Fix 32-bit Windows build
> >>.gitlab-ci.d/windows.yml: Unify the prerequisite packages
> >>.gitlab-ci.d/windows.yml: Test 'make installer' in the CI
> >>
> >> meson.build | 1 +
> >> block/nfs.c | 8 ++
> >> .gitlab-ci.d/windows.yml | 40 ---
> >> scripts/nsis.py | 60 +---
> >> 4 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >
> > I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> > sed processing in the 64-bit build")
> >
> > What about other patches?
>
> I hope that Stefan Weil (our W32 maintainer) could have a look at these
> first...
Would you please comment this series? Thanks!
Regards,
Bin
Re: [PATCH v2 20/39] tests/qtest: {ahci, ide}-test: Use relative path for temporary files for win32
Hi
On Tue, Sep 20, 2022 at 1:50 PM Bin Meng wrote:
> From: Bin Meng
>
> These test cases uses "blkdebug:path/to/config:path/to/image" for
> testing. On Windows, absolute file paths contain the delimiter ':'
> which causes the blkdebug filename parser fail to parse filenames.
>
> Signed-off-by: Bin Meng
>
I don't have a much better solution to propose at this point (to actually
use a temp directory), so:
Reviewed-by: Marc-André Lureau
> ---
>
> (no changes since v1)
>
> tests/qtest/ahci-test.c | 21 ++---
> tests/qtest/ide-test.c | 20 ++--
> 2 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 00524f14c6..c57576b08c 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1833,7 +1833,7 @@ static void create_ahci_io_test(enum IOMode type,
> enum AddrMode addr,
>
> int main(int argc, char **argv)
> {
> -const char *arch;
> +const char *arch, *base;
> int ret;
> int fd;
> int c;
> @@ -1871,8 +1871,22 @@ int main(int argc, char **argv)
> return 0;
> }
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
> /* Create a temporary image */
> -fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> +fd = g_mkstemp(tmp_path);
> g_assert(fd >= 0);
> if (have_qemu_img()) {
> imgfmt = "qcow2";
> @@ -1889,7 +1903,8 @@ int main(int argc, char **argv)
> close(fd);
>
> /* Create temporary blkdebug instructions */
> -fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> +fd = g_mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
> index 25302be6dc..5e3e28aea2 100644
> --- a/tests/qtest/ide-test.c
> +++ b/tests/qtest/ide-test.c
> @@ -1011,16 +1011,32 @@ static void test_cdrom_dma(void)
>
> int main(int argc, char **argv)
> {
> +const char *base;
> int fd;
> int ret;
>
> +/*
> + * "base" stores the starting point where we create temporary files.
> + *
> + * On Windows, this is set to the relative path of current working
> + * directory, because the absolute path causes the blkdebug filename
> + * parser fail to parse "blkdebug:path/to/config:path/to/image".
> + */
> +#ifndef _WIN32
> +base = g_get_tmp_dir();
> +#else
> +base = ".";
> +#endif
> +
> /* Create temporary blkdebug instructions */
> -fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> +debug_path = g_strdup_printf("%s/qtest-blkdebug.XX", base);
> +fd = g_mkstemp(debug_path);
> g_assert(fd >= 0);
> close(fd);
>
> /* Create a temporary raw image */
> -fd = g_file_open_tmp("qtest.XX", _path, NULL);
> +tmp_path = g_strdup_printf("%s/qtest.XX", base);
> +fd = g_mkstemp(tmp_path);
> g_assert(fd >= 0);
> ret = ftruncate(fd, TEST_IMAGE_SIZE);
> g_assert(ret == 0);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 08/39] block/vvfat: Unify the mkdir() call
On Tue, Sep 20, 2022 at 2:58 PM Bin Meng wrote:
> From: Bin Meng
>
> There is a difference in the mkdir() call for win32 and non-win32
> platforms, and currently is handled in the codes with #ifdefs.
>
> glib provides a portable g_mkdir() API and we can use it to unify
> the codes without #ifdefs.
>
> Signed-off-by: Bin Meng
>
Reviewed-by: Marc-André Lureau
> ---
>
> Changes in v2:
> - Change to use g_mkdir()
>
> block/vvfat.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..723beef025 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -25,6 +25,7 @@
>
> #include "qemu/osdep.h"
> #include
> +#include
> #include "qapi/error.h"
> #include "block/block_int.h"
> #include "block/qdict.h"
> @@ -2726,13 +2727,9 @@ static int
> handle_renames_and_mkdirs(BDRVVVFATState* s)
> mapping_t* mapping;
> int j, parent_path_len;
>
> -#ifdef __MINGW32__
> -if (mkdir(commit->path))
> +if (g_mkdir(commit->path, 0755)) {
> return -5;
> -#else
> -if (mkdir(commit->path, 0755))
> -return -5;
> -#endif
> +}
>
> mapping = insert_mapping(s, commit->param.mkdir.cluster,
> commit->param.mkdir.cluster + 1);
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v2 07/39] tests: Avoid using hardcoded /tmp in test cases
Hi
On Tue, Sep 20, 2022 at 2:47 PM Bin Meng wrote:
> From: Bin Meng
>
> Lots of test cases were written to use hardcoded /tmp directory for
> temporary files. To avoid this, we update them to use g_dir_make_tmp()
> or g_file_open_tmp() when appropriate.
>
> Signed-off-by: Bin Meng
> ---
>
> Changes in v2:
> - Use g_dir_make_tmp(), g_file_open_tmp() when appropriate
>
> tests/qtest/fuzz/generic_fuzz_configs.h | 4 ++--
> tests/qtest/ahci-test.c | 19 +++
> tests/qtest/aspeed_smc-test.c | 5 ++---
> tests/qtest/boot-serial-test.c | 9 +
> tests/qtest/cxl-test.c | 15 ++-
> tests/qtest/fdc-test.c | 5 +++--
> tests/qtest/fuzz/virtio_blk_fuzz.c | 4 ++--
> tests/qtest/hd-geo-test.c | 24 +++-
> tests/qtest/ide-test.c | 10 ++
> tests/qtest/libqtest.c | 12
> tests/qtest/migration-test.c| 7 ---
> tests/qtest/pflash-cfi02-test.c | 8 +---
> tests/qtest/qmp-test.c | 6 --
> tests/qtest/vhost-user-blk-test.c | 3 ++-
> tests/qtest/vhost-user-test.c | 8
> tests/qtest/virtio-blk-test.c | 4 ++--
> tests/qtest/virtio-scsi-test.c | 4 ++--
> tests/unit/test-image-locking.c | 8
> tests/unit/test-qga.c | 2 +-
> tests/vhost-user-bridge.c | 3 +--
> 20 files changed, 85 insertions(+), 75 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> b/tests/qtest/fuzz/generic_fuzz_configs.h
> index 0775e6702b..a825b78c14 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -20,8 +20,8 @@ typedef struct generic_fuzz_config {
> } generic_fuzz_config;
>
> static inline gchar *generic_fuzzer_virtio_9p_args(void){
> -char tmpdir[] = "/tmp/qemu-fuzz.XX";
> -g_assert_nonnull(g_mkdtemp(tmpdir));
> +g_autofree char *tmpdir = g_dir_make_tmp("qemu-fuzz.XX", NULL);
> +g_assert_nonnull(tmpdir);
>
> return g_strdup_printf("-machine q35 -nodefaults "
> "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index f1e510b0ac..00524f14c6 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -44,9 +44,9 @@
> #define TEST_IMAGE_SIZE_MB_SMALL 64
>
> /*** Globals ***/
> -static char tmp_path[] = "/tmp/qtest.XX";
> -static char debug_path[] = "/tmp/qtest-blkdebug.XX";
> -static char mig_socket[] = "/tmp/qtest-migration.XX";
> +static char *tmp_path;
> +static char *debug_path;
> +static char *mig_socket;
> static bool ahci_pedantic;
> static const char *imgfmt;
> static unsigned test_image_size_mb;
> @@ -1437,10 +1437,10 @@ static void test_ncq_simple(void)
>
> static int prepare_iso(size_t size, unsigned char **buf, char **name)
> {
> -char cdrom_path[] = "/tmp/qtest.iso.XX";
> +g_autofree char *cdrom_path;
>
Whenever introducing g_auto* usage, please make sure to initialize the
variable to NULL or a correct value.
unsigned char *patt;
> ssize_t ret;
> -int fd = mkstemp(cdrom_path);
> +int fd = g_file_open_tmp("qtest.iso.XX", _path, NULL);
>
> g_assert(fd != -1);
> g_assert(buf);
> @@ -1872,7 +1872,7 @@ int main(int argc, char **argv)
> }
>
> /* Create a temporary image */
> -fd = mkstemp(tmp_path);
> +fd = g_file_open_tmp("qtest.XX", _path, NULL);
> g_assert(fd >= 0);
> if (have_qemu_img()) {
> imgfmt = "qcow2";
> @@ -1889,12 +1889,12 @@ int main(int argc, char **argv)
> close(fd);
>
> /* Create temporary blkdebug instructions */
> -fd = mkstemp(debug_path);
> +fd = g_file_open_tmp("qtest-blkdebug.XX", _path, NULL);
> g_assert(fd >= 0);
> close(fd);
>
> /* Reserve a hollow file to use as a socket for migration tests */
> -fd = mkstemp(mig_socket);
> +fd = g_file_open_tmp("qtest-migration.XX", _socket, NULL);
> g_assert(fd >= 0);
> close(fd);
>
> @@ -1947,8 +1947,11 @@ int main(int argc, char **argv)
>
> /* Cleanup */
> unlink(tmp_path);
> +g_free(tmp_path);
> unlink(debug_path);
> +g_free(debug_path);
> unlink(mig_socket);
> +g_free(mig_socket);
>
> return ret;
> }
> diff --git a/tests/qtest/aspeed_smc-test.c b/tests/qtest/aspeed_smc-test.c
> index 05ce941566..5e16b5c9a5 100644
> --- a/tests/qtest/aspeed_smc-test.c
> +++ b/tests/qtest/aspeed_smc-test.c
> @@ -608,16 +608,15 @@ static void test_write_block_protect_bottom_bit(void)
> flash_reset();
> }
>
> -static char tmp_path[] = "/tmp/qtest.m25p80.XX";
> -
> int main(int argc, char **argv)
> {
> +g_autofree char *tmp_path;
> int ret;
> int fd;
>
> g_test_init(, , NULL);
>
> -fd = mkstemp(tmp_path);
> +fd =
Re: [PATCH v2 03/39] block: Unify the get_tmp_filename() implementation
Hi
On Tue, Sep 20, 2022 at 2:17 PM Bin Meng wrote:
> From: Bin Meng
>
> At present get_tmp_filename() has platform specific implementations
> to get the directory to use for temporary files. Switch over to use
> g_get_tmp_dir() which works on all supported platforms.
>
>
As discussed in v1, there are other things to do while touching this code.
And since it is optional for the series goal, please send this as a
different patch/series.
> Signed-off-by: Bin Meng
> ---
>
> (no changes since v1)
>
> block.c | 16 ++--
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc85f46eed..d06df47f72 100644
> --- a/block.c
> +++ b/block.c
> @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
> */
> int get_tmp_filename(char *filename, int size)
> {
> -#ifdef _WIN32
> -char temp_dir[MAX_PATH];
> -/* GetTempFileName requires that its output buffer (4th param)
> - have length MAX_PATH or greater. */
> -assert(size >= MAX_PATH);
> -return (GetTempPath(MAX_PATH, temp_dir)
> -&& GetTempFileName(temp_dir, "qem", 0, filename)
> -? 0 : -GetLastError());
> -#else
> int fd;
> const char *tmpdir;
> -tmpdir = getenv("TMPDIR");
> -if (!tmpdir) {
> -tmpdir = "/var/tmp";
> -}
> +tmpdir = g_get_tmp_dir();
> +
> if (snprintf(filename, size, "%s/vl.XX", tmpdir) >= size) {
> return -EOVERFLOW;
> }
> @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
> return -errno;
> }
> return 0;
> -#endif
> }
>
> /*
> --
> 2.34.1
>
>
>
--
Marc-André Lureau
[PULL 1/1] virtiofsd: use g_date_time_get_microsecond to get subsecond
From: Yusuke Okada
The "%f" specifier in g_date_time_format() is only available in glib
2.65.2 or later. If combined with older glib, the function returns null
and the timestamp displayed as "(null)".
For backward compatibility, g_date_time_get_microsecond should be used
to retrieve subsecond.
In this patch the g_date_time_format() leaves subsecond field as "%06d"
and let next snprintf to format with g_date_time_get_microsecond.
Signed-off-by: Yusuke Okada
Reviewed-by: Dr. David Alan Gilbert
Message-id: 20220818184618.2205172-1-yokada@gmail.com
Signed-off-by: Stefan Hajnoczi
---
tools/virtiofsd/passthrough_ll.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 371a7bead6..20f0f41f99 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -4185,6 +4185,7 @@ static void setup_nofile_rlimit(unsigned long
rlimit_nofile)
static void log_func(enum fuse_log_level level, const char *fmt, va_list ap)
{
g_autofree char *localfmt = NULL;
+char buf[64];
if (current_log_level < level) {
return;
@@ -4197,9 +4198,11 @@ static void log_func(enum fuse_log_level level, const
char *fmt, va_list ap)
fmt);
} else {
g_autoptr(GDateTime) now = g_date_time_new_now_utc();
-g_autofree char *nowstr = g_date_time_format(now, "%Y-%m-%d
%H:%M:%S.%f%z");
+g_autofree char *nowstr = g_date_time_format(now,
+ "%Y-%m-%d %H:%M:%S.%%06d%z");
+snprintf(buf, 64, nowstr, g_date_time_get_microsecond(now));
localfmt = g_strdup_printf("[%s] [ID: %08ld] %s",
- nowstr, syscall(__NR_gettid), fmt);
+ buf, syscall(__NR_gettid), fmt);
}
fmt = localfmt;
}
--
2.37.3
[PULL 0/1] Block patches
The following changes since commit 6338c30111d596d955e6bc933a82184a0b910c43: Merge tag 'm68k-for-7.2-pull-request' of https://github.com/vivier/qemu-m68k into staging (2022-09-21 13:12:36 -0400) are available in the Git repository at: https://gitlab.com/stefanha/qemu.git tags/block-pull-request for you to fetch changes up to f16d15c9276bd8f501f861c39cbd4adc812d0c1d: virtiofsd: use g_date_time_get_microsecond to get subsecond (2022-09-22 13:13:47 -0400) Pull request Yusuke Okada (1): virtiofsd: use g_date_time_get_microsecond to get subsecond tools/virtiofsd/passthrough_ll.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.37.3
Re: [PATCH 0/3] block: Keep auto_backing_file post-migration
Am 03.08.2022 um 16:44 hat Hanna Reitz geschrieben:
> Hi,
>
> https://gitlab.com/qemu-project/qemu/-/issues/1117 reports the following
> issue:
>
> Say you have a VM with a backing chain of images where the image
> metadata contains json:{} backing file strings, which however will be
> resolved to simple plain filenames when opened[1].
>
> So when these images are opened, bs->auto_backing_file is first read
> directly from the image header, and will thus contain a json:{}
> filename. The backing image is opened based off of this filename, and
> bdrv_refresh_filename() simplfies the filename as shown[1]. We then
> update bs->auto_backing_file from bs->backing->bs->filename, so both are
> equal.
>
> It is quite important that both are equal, because
> bdrv_backing_overridden() checks whether the backing file has been
> changed from the default by comparing bs->auto_backing_file to
> bs->backing->bs->filename.
>
> Because we did set bs->auto_backing_file from bs->backing->bs->filename,
> both are equal, the backing file is not considered overridden, and
> bdrv_refresh_filename(bs) will not consider it necessary to generate a
> json:{} filename for the overlay.
>
> Then the VM is migrated.
>
> The destination side invokes bdrv_invalidate_cache(), which by qcow2 and
> qed is implemented by closing the image and opening it. This re-reads
> the backing file string from disk, resetting bs->auto_backing_file.
> Now, it will contains the json:{} filename again and thus differ from
> bs->backing->bs->filename.
>
> Consequentially, a subsequent bdrv_refresh_filename(bs) will find that
> the overlay’s backing file has been overridden and generate a json:{}
> filename, which isn’t great.
>
> This series fixes that by having qcow2’s and qed’s image-open operations
> not overwrite bs->auto_backing_file unless something has changed since
> the last time we read the backing filename from the metadata.
>
>
> Now, generating a json:{} filename can be a nuisance but shouldn’t be a
> real problem. The actual problem reported in 1117 comes later, namely
> when creating a snapshot overlay post-migration. This overlay image
> will have a json:{} backing filename in its image metadata, which
> contains a 'backing' key[2].
>
> 'qemu-img info' uses the BDRV_O_NO_BACKING flag to open images, which
> conflicts with those backing options: With that flag, nobody processes
> those options, and that’s an error. Therefore, you can’t run 'qemu-img
> info --backing-chain' on that overlay image.
>
> That part of the issue is not fixed in this series, however. I’ll send
> a separate RFC series for it, because I’m honstly not quite certain how
> it should be fixed.
>
>
> [1] Example:
> json:{"driver": "qcow2",
> "file": {"driver": "file", "filename": "img.qcow2"}}
> Will generally be “resolved” by bdrv_refresh_filename() to
> "img.qcow2"
>
> [2] That it contains a 'backing' key is only natural, because the reason
> why bdrv_refresh_filename() decided to generate a json:{} filename
> for the image is because it considered the backing file overridden.
> Hence it must put the actual backing file options into a 'backing'
> object in the json:{} filename.
>
>
> Hanna Reitz (3):
> block/qcow2: Keep auto_backing_file if possible
> block/qed: Keep auto_backing_file if possible
> iotests/backing-file-invalidation: Add new test
Thanks, applied to the block branch.
Kevin
Re: [PATCH for 7.2] minor fixups in block code
Am 17.08.2022 um 10:37 hat Denis V. Lunev geschrieben: > These 2 patches are just minor improvements to make code a bit better. Thanks, applied to the block branch. Kevin
Re: [PATCH] gluster: stop using .bdrv_needs_filename
Am 11.08.2022 um 18:49 hat Stefan Hajnoczi geschrieben:
> The gluster protocol driver used to parse URIs (filenames) but was
> extended with a richer JSON syntax in commit 6c7189bb29de
> ("block/gluster: add support for multiple gluster servers"). The gluster
> drivers that have JSON parsing set .bdrv_needs_filename to false.
>
> The gluster+unix and gluster+rdma drivers still to require a filename
> even though the JSON parser is equipped to parse the same
> volume/path/sockaddr details as the URI parser. Let's allow JSON parsing
> for these drivers too.
>
> Note that the gluster+rdma driver actually uses TCP because RDMA support
> is not available, so the JSON server.type field must be "inet".
>
> Drop .bdrv_needs_filename since both the filename and the JSON parsers
> can handle gluster+unix and gluster+rdma. This change is in preparation
> for eventually removing .bdrv_needs_filename across the entire codebase.
>
> Cc: Prasanna Kumar Kalever
> Signed-off-by: Stefan Hajnoczi
Thanks, applied to the block branch.
Kevin
Re: [PATCH v5 0/2] block: add missed block_acct_setup with new block device init procedure
Am 24.08.2022 um 11:50 hat Denis V. Lunev geschrieben: > Commit 5f76a7aac156ca75680dad5df4a385fd0b58f6b1 is looking harmless from > the first glance, but it has changed things a lot. 'libvirt' uses it to > detect that it should follow new initialization way and this changes > things considerably. With this procedure followed, blockdev_init() is > not called anymore and thus block_acct_setup() helper is not called. > > This means in particular that defaults for block accounting statistics > are changed and account_invalid/account_failed are actually initialized > as false instead of true originally. > > This commit changes things to match original world. There are the following > constraints: > * new default value in block_acct_init() is set to true > * block_acct_setup() inside blockdev_init() is called before > blkconf_apply_backend_options() > * thus newly created option in block device properties has precedence if > specified Thanks, applied to the block branch. Kevin
Re: [PATCH] block/qcow2-bitmap: Add missing cast to silent GCC error
Am 19.09.2022 um 20:27 hat Philippe Mathieu-Daudé geschrieben:
> Commit d1258dd0c8 ("qcow2: autoloading dirty bitmaps") added the
> set_readonly_helper() GFunc handler, correctly casting the gpointer
> user_data in both the g_slist_foreach() caller and the handler.
> Few commits later (commit 1b6b0562db), the handler is reused in
> qcow2_reopen_bitmaps_rw() but missing the gpointer cast, resulting
> in the following error when using Homebrew GCC 12.2.0:
>
> [2/658] Compiling C object libblock.fa.p/block_qcow2-bitmap.c.o
> ../../block/qcow2-bitmap.c: In function 'qcow2_reopen_bitmaps_rw':
> ../../block/qcow2-bitmap.c:1211:60: error: incompatible type for argument 3
> of 'g_slist_foreach'
>1211 | g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
> |^
> ||
> |_Bool
> In file included from
> /opt/homebrew/Cellar/glib/2.72.3_1/include/glib-2.0/glib/gmain.h:26,
>from
> /opt/homebrew/Cellar/glib/2.72.3_1/include/glib-2.0/glib/giochannel.h:33,
>from
> /opt/homebrew/Cellar/glib/2.72.3_1/include/glib-2.0/glib.h:54,
>from /Users/philmd/source/qemu/include/glib-compat.h:32,
>from /Users/philmd/source/qemu/include/qemu/osdep.h:144,
>from ../../block/qcow2-bitmap.c:28:
> /opt/homebrew/Cellar/glib/2.72.3_1/include/glib-2.0/glib/gslist.h:127:61:
> note: expected 'gpointer' {aka 'void *'} but argument is of type '_Bool'
> 127 | gpointer
> user_data);
> |
> ~~^
> At top level:
> FAILED: libblock.fa.p/block_qcow2-bitmap.c.o
>
> Fix by adding the missing gpointer cast.
>
> Fixes: 1b6b0562db ("qcow2: support .bdrv_reopen_bitmaps_rw")
> Signed-off-by: Philippe Mathieu-Daudé
Thanks, applied to the block branch. And in case qemu-trivial picks it
up anyway:
Reviewed-by: Kevin Wolf
Re: [PATCH v11 21/21] job: remove unused functions
> >>> @@ -725,9 +703,6 @@ void job_cancel_sync_all(void); >>> * Returns the return value from the job. >>> * Called with job_lock *not* held. >> >> in comment: with lock held. >> > > No idea what you mean here. Ok you were referring at the function below. Agree, changed. > > Thank you, > Emanuele >
Re: [PATCH] qemu-img: Wean documentation and help output off '?' for help
Am 08.09.2022 um 15:08 hat Markus Armbruster geschrieben: > '?' for help is deprecated since commit c8057f951d "Support 'help' as > a synonym for '?' in command line options", v1.2.0. We neglected to > update output of qemu-img --help and the manual. Do that now. > > Signed-off-by: Markus Armbruster Thanks, applied to the block branch. Kevin
Re: [PATCH 02/26] block: add missing coroutine_fn annotations
On Thu, Sep 22, 2022 at 9:49 AM Paolo Bonzini wrote: > Callers of coroutine_fn must be coroutine_fn themselves, or the call > must be within "if (qemu_in_coroutine())". Apply coroutine_fn to > functions where this holds. > > Signed-off-by: Paolo Bonzini > --- > block.c | 6 +++--- > block/block-backend.c | 10 +- > block/io.c| 22 +++--- > 3 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/block.c b/block.c > index bc85f46eed..5c34ada53f 100644 > --- a/block.c > +++ b/block.c > @@ -631,9 +631,9 @@ static int64_t create_file_fallback_truncate(BlockBackend > *blk, > * Helper function for bdrv_create_file_fallback(): Zero the first > * sector to remove any potentially pre-existing image header. > */ > -static int create_file_fallback_zero_first_sector(BlockBackend *blk, > - int64_t current_size, > - Error **errp) > +static int coroutine_fn create_file_fallback_zero_first_sector(BlockBackend > *blk, > + int64_t > current_size, > + Error **errp) Why mark this coroutine_fn? Maybe the intention was to also replace the call to blk_pwrite_zeroes() with blk_co_pwrite_zeroes()? Regardless: Reviewed-by: Alberto Faria
Re: [PATCH v1 01/10] monitor: expose monitor_puts to rest of code
Am 21.09.2022 um 18:07 hat Alex Bennée geschrieben: > This helps us construct strings elsewhere before echoing to the > monitor. It avoids having to jump through hoops like: > > monitor_printf(mon, "%s", s->str); > > It will be useful in following patches but for now convert all > existing plain "%s" printfs to use the _puts api. > > Signed-off-by: Alex Bennée > Reviewed-by: Richard Henderson Reviewed-by: Kevin Wolf
Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
Am 18/09/2022 um 19:12 schrieb Emanuele Giuseppe Esposito: >> In replication_stop, we call job_cancel_sync() inside >> aio_context_acquire - aio_context_release section. Should it be fixed? > I don't think it breaks anything. The question is: what is the > aiocontext there protecting? Do we need it? I have no idea. Ok Paolo reminded me that job_cancel_sync() calls AIO_WAIT_WHILE_UNLOCKED. This means that it indeed needs to be wrapped in an aiocontext release() + acquire() block. >> Another question, sometimes you move job_start out of >> aio-context-acquire critical section, sometimes not. As I understand, >> it's of for job_start to be called both with acquired aio-context or not >> acquired? >> > Same as above, job_start does not need the aiocontext to be taken > (otherwise job_lock would be useless). I still think here it does not matter if the aiocontext is taken or not. > Thank you, > Emanuele
Re: [PATCH v3] 9pfs: use GHashTable for fid table
On Donnerstag, 22. September 2022 13:43:56 CEST Linus Heckemann wrote: > Christian Schoenebeck writes: > > On Freitag, 9. September 2022 15:10:48 CEST Christian Schoenebeck wrote: > >> On Donnerstag, 8. September 2022 13:23:53 CEST Linus Heckemann wrote: > >> > The previous implementation would iterate over the fid table for > >> > lookup operations, resulting in an operation with O(n) complexity on > >> > the number of open files and poor cache locality -- for every open, > >> > stat, read, write, etc operation. > >> > > >> > This change uses a hashtable for this instead, significantly improving > >> > the performance of the 9p filesystem. The runtime of NixOS's simple > >> > installer test, which copies ~122k files totalling ~1.8GiB from 9p, > >> > decreased by a factor of about 10. > >> > > >> > Signed-off-by: Linus Heckemann > >> > Reviewed-by: Philippe Mathieu-Daudé > >> > Reviewed-by: Greg Kurz > >> > --- > >> > >> Queued on 9p.next: > >> https://github.com/cschoenebeck/qemu/commits/9p.next > >> > >> I retained the BUG_ON() in get_fid(), Greg had a point there that > >> continuing to work on a clunked fid would still be a bug. > >> > >> I also added the suggested TODO comment for > >> g_hash_table_steal_extended(), > >> the actual change would be outside the scope of this patch. > >> > >> And finally I gave this patch a whirl, and what can I say: that's just > >> sick! Compiling sources with 9p is boosted by around factor 6..7 here! > >> And running 9p as root fs also no longer feels sluggish as before. I > >> mean I knew that this fid list traversal performance issue existed and > >> had it on my TODO list, but the actual impact exceeded my expectation by > >> far.> > > Linus, there is still something cheesy. After more testing, at a certain > > point> > > running the VM, the terminal is spilled with this message: > > GLib: g_hash_table_iter_next: assertion 'ri->version == > > ri->hash_table->version' failed> > > Looking at the glib sources, I think this warning means the iterator got > > invalidated. Setting a breakpoint at glib function > > g_return_if_fail_warning I> > > got: > > Thread 1 "qemu-system-x86" hit Breakpoint 1, 0x77aa9d80 in > > g_return_if_fail_warning () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > > (gdb) bt > > #0 0x77aa9d80 in g_return_if_fail_warning () at > > /lib/x86_64-linux-gnu/libglib-2.0.so.0 #1 0x77a8ea18 in > > g_hash_table_iter_next () at /lib/x86_64-linux-gnu/libglib-2.0.so.0 #2 > > 0x55998a7a in v9fs_mark_fids_unreclaim (pdu=0x57a34c90, > > path=0x7ffba8ceff30) at ../hw/9pfs/9p.c:528 #3 0x5599f7a0 in > > v9fs_unlinkat (opaque=0x57a34c90) at ../hw/9pfs/9p.c:3170 #4 > > 0x5606dc4b in coroutine_trampoline (i0=1463900480, i1=21845) at > > ../util/coroutine-ucontext.c:177 #5 0x77749d40 in > > __start_context () at /lib/x86_64-linux-gnu/libc.so.6 #6 > > 0x7fffd5f0 in () > > #7 0x in () > > (gdb) > > > > The while loop in v9fs_mark_fids_unreclaim() holds the hash table iterator > > while the hash table is modified during the loop. > > > > Would you please fix this? If you do, please use my already queued patch > > version as basis. > > > > Best regards, > > Christian Schoenebeck > > Hi Christian, > > Thanks for finding this! > > I think I understand the problem, but I can't reproduce it at all (I've > been trying by hammering the filesystem with thousands of opens/closes > across several processes). Do you have a reliable way? I also didn't hit this issue in my initial tests. I do hit this after about just a minute though when running 9p as root fs [1] and then starting to compile KDE [2] inside the VM. [1] https://wiki.qemu.org/Documentation/9p_root_fs [2] https://community.kde.org/Get_Involved/development Independent of reproduction, let me elaborate what's going on, as this issue is probably not that obvious: 1.) Like with any ordered container, the glib hash iterator becomes invalid as soon as the hash table got modified. Fortunately glib detects this case by maintaining a "version" field on the hash table which is bumped whenever the hash table was modified, and likewise a "version" field on the iterator structure and detecting an invalid iterator by comparing [3] the two "version" fields when calling g_hash_table_iter_next() for instance: g_return_val_if_fail (ri->version == ri->hash_table->version, FALSE); and the g_return_val_if_fail() macro calls g_return_if_fail_warning() to print the warning message in this case and then just immediately returns from g_hash_table_iter_next(). So this is not nitpicking, it will actually start severe 9p server misbehaviour at this point. [3] https://github.com/GNOME/glib/blob/main/glib/ghash.c#L1182 2.) The hash table loop inside v9fs_mark_fids_unreclaim() does not directly modify the "fids" hash table. But inside that loop we get contention, because even
Re: [PATCH v3] 9pfs: use GHashTable for fid table
Christian Schoenebeck writes: > On Freitag, 9. September 2022 15:10:48 CEST Christian Schoenebeck wrote: >> On Donnerstag, 8. September 2022 13:23:53 CEST Linus Heckemann wrote: >> > The previous implementation would iterate over the fid table for >> > lookup operations, resulting in an operation with O(n) complexity on >> > the number of open files and poor cache locality -- for every open, >> > stat, read, write, etc operation. >> > >> > This change uses a hashtable for this instead, significantly improving >> > the performance of the 9p filesystem. The runtime of NixOS's simple >> > installer test, which copies ~122k files totalling ~1.8GiB from 9p, >> > decreased by a factor of about 10. >> > >> > Signed-off-by: Linus Heckemann >> > Reviewed-by: Philippe Mathieu-Daudé >> > Reviewed-by: Greg Kurz >> > --- >> >> Queued on 9p.next: >> https://github.com/cschoenebeck/qemu/commits/9p.next >> >> I retained the BUG_ON() in get_fid(), Greg had a point there that continuing >> to work on a clunked fid would still be a bug. >> >> I also added the suggested TODO comment for g_hash_table_steal_extended(), >> the actual change would be outside the scope of this patch. >> >> And finally I gave this patch a whirl, and what can I say: that's just sick! >> Compiling sources with 9p is boosted by around factor 6..7 here! And >> running 9p as root fs also no longer feels sluggish as before. I mean I >> knew that this fid list traversal performance issue existed and had it on >> my TODO list, but the actual impact exceeded my expectation by far. > > Linus, there is still something cheesy. After more testing, at a certain point > running the VM, the terminal is spilled with this message: > > GLib: g_hash_table_iter_next: assertion 'ri->version == > ri->hash_table->version' failed > > Looking at the glib sources, I think this warning means the iterator got > invalidated. Setting a breakpoint at glib function g_return_if_fail_warning I > got: > > Thread 1 "qemu-system-x86" hit Breakpoint 1, 0x77aa9d80 in > g_return_if_fail_warning () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 > (gdb) bt > #0 0x77aa9d80 in g_return_if_fail_warning () at > /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #1 0x77a8ea18 in g_hash_table_iter_next () at > /lib/x86_64-linux-gnu/libglib-2.0.so.0 > #2 0x55998a7a in v9fs_mark_fids_unreclaim (pdu=0x57a34c90, > path=0x7ffba8ceff30) at ../hw/9pfs/9p.c:528 > #3 0x5599f7a0 in v9fs_unlinkat (opaque=0x57a34c90) at > ../hw/9pfs/9p.c:3170 > #4 0x5606dc4b in coroutine_trampoline (i0=1463900480, i1=21845) at > ../util/coroutine-ucontext.c:177 > #5 0x77749d40 in __start_context () at > /lib/x86_64-linux-gnu/libc.so.6 > #6 0x7fffd5f0 in () > #7 0x in () > (gdb) > > The while loop in v9fs_mark_fids_unreclaim() holds the hash table iterator > while the hash table is modified during the loop. > > Would you please fix this? If you do, please use my already queued patch > version as basis. > > Best regards, > Christian Schoenebeck Hi Christian, Thanks for finding this! I think I understand the problem, but I can't reproduce it at all (I've been trying by hammering the filesystem with thousands of opens/closes across several processes). Do you have a reliable way? Cheers Linus
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Thu, Sep 22, 2022 at 09:34:41AM +0800, Jason Wang wrote:
> On Thu, Sep 22, 2022 at 1:58 AM Raphael Norwitz
> wrote:
> >
> > If I read your response on the other thread correctly, this change is
> > intended
> >
> > to prioritize the MAC address exposed by DPDK over the one provided by the
> >
> > QEMU command line? Sounds reasonable in principle, but I would get
> > confirmation
> >
> > from vDPA/vhost-net maintainers.
>
> I think the best way is to (and it seems easier)
>
> 1) have the management layer to make sure the mac came from cli
> matches the underlayer vDPA
> 2) having a sanity check and fail the device initialization if they don't
> match
>
> Thanks
I think I agree, we should not overwrite user supplied arguments.
I think the case where we might want to get the mac from VDPA
and use it is when it's been assigned randomly as opposed to coming from cli.
> >
> >
> >
> > That said the way you’re hacking the vhost-user code breaks a valuable
> > check for
> >
> > bad vhost-user-blk backends. I would suggest finding another implementation
> > which
> >
> > does not regress functionality for other device types.
> >
> >
> >
> >
> >
> > >From: Hao Chen
> >
> > >
> >
> > >When use dpdk-vdpa tests vdpa device. You need to specify the mac address
> > >to
> >
> > >start the virtual machine through libvirt or qemu, but now, the libvirt or
> >
> > >qemu can call dpdk vdpa vendor driver's ops .get_config through
> > >vhost_net_get_config
> >
> > >to get the mac address of the vdpa hardware without manual configuration.
> >
> > >
> >
> > >v1->v2:
> >
> > >Only copy ETH_ALEN data of netcfg for some vdpa device such as
> >
> > >NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> >
> > >We only need the mac address and don't care about the status field.
> >
> > >
> >
> > >Signed-off-by: Hao Chen
> >
> > >---
> >
> > > hw/block/vhost-user-blk.c | 1 -
> >
> > > hw/net/virtio-net.c | 7 +++
> >
> > > hw/virtio/vhost-user.c| 19 ---
> >
> > > 3 files changed, 7 insertions(+), 20 deletions(-)
> >
> > >
> >
> > >diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >
> > >index 9117222456..5dca4eab09 100644
> >
> > >--- a/hw/block/vhost-user-blk.c
> >
> > >+++ b/hw/block/vhost-user-blk.c
> >
> > >@@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> > >Error **errp)
> >
> > >
> >
> > > vhost_dev_set_config_notifier(>dev, _ops);
> >
> > >
> >
> > >-s->vhost_user.supports_config = true;
> >
> > > ret = vhost_dev_init(>dev, >vhost_user,
> > > VHOST_BACKEND_TYPE_USER, 0,
> >
> > > errp);
> >
> > > if (ret < 0) {
> >
> > >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >
> > >index dd0d056fde..90405083b1 100644
> >
> > >--- a/hw/net/virtio-net.c
> >
> > >+++ b/hw/net/virtio-net.c
> >
> > >@@ -166,6 +166,13 @@ static void virtio_net_get_config(VirtIODevice *vdev,
> > >uint8_t *config)
> >
> > > }
> >
> > > memcpy(config, , n->config_size);
> >
> > > }
> >
> > >+} else if (nc->peer && nc->peer->info->type ==
> > >NET_CLIENT_DRIVER_VHOST_USER) {
> >
> > >+ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t
> > >*),
> >
> > >+ n->config_size);
> >
> > >+if (ret != -1) {
> >
> > >+ /* Automatically obtain the mac address of the vdpa device
> >
> > >+* when using the dpdk vdpa */
> >
> > >+memcpy(config, , ETH_ALEN);
> >
> > > }
> >
> > > }
> >
> > >
> >
> > >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >
> > >index bd24741be8..8b01078249 100644
> >
> > >--- a/hw/virtio/vhost-user.c
> >
> > >+++ b/hw/virtio/vhost-user.c
> >
> > >@@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
> > >*dev, void *opaque,
> >
> > > }
> >
> > >
> >
> > > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >
> > >-bool supports_f_config = vus->supports_config ||
> >
> > >-(dev->config_ops &&
> > >dev->config_ops->vhost_dev_config_notifier);
> >
> > > uint64_t protocol_features;
> >
> > >
> >
> > > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >
> > >@@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
> > >*dev, void *opaque,
> >
> > > */
> >
> > > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >
> > >
> >
> > >-if (supports_f_config) {
> >
> > >-if (!virtio_has_feature(protocol_features,
> >
> > >-VHOST_USER_PROTOCOL_F_CONFIG)) {
> >
> > >-error_setg(errp, "vhost-user device expecting "
> >
> > >- "VHOST_USER_PROTOCOL_F_CONFIG but the
> > >vhost-user backend does "
> >
> > >- "not support it.");
> >
> > >-return -EPROTO;
> >
> > >-}
> >
> >
Re: [PATCH v2] hw/ide/piix: Ignore writes of hardwired PCI command register bits
On Tue, Sep 06, 2022 at 10:23:57AM -0400, Michael S. Tsirkin wrote: > On Thu, Jun 02, 2022 at 08:47:31PM +, Lev Kujawski wrote: > > One method to enable PCI bus mastering for IDE controllers, often used > > by x86 firmware, is to write 0x7 to the PCI command register. Neither > > the PIIX3 specification nor actual hardware (a Tyan S1686D system) > > permit modification of the Memory Space Enable (MSE) bit, 1, and thus > > the command register would be left in an unspecified state without > > this patch. > > > > Signed-off-by: Lev Kujawski > > > I don't think this is appropriate in trivial at all. > > > > > --- > > This revised patch uses QEMU's built-in PCI bit-masking support rather > > than attempting to manually filter writes. Thanks to Philippe Mathieu- > > Daude and Michael S. Tsirkin for review and the pointer. > > But pls note I wrote: > > Might need machine compat machinery > for this. > > without said machinery, if guest set one of the other > bits, migration will fail. I assume v3 will be forthcoming, right? > > > hw/ide/piix.c | 15 +++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > > index 76ea8fd9f6..bd3f397de8 100644 > > --- a/hw/ide/piix.c > > +++ b/hw/ide/piix.c > > @@ -25,6 +25,8 @@ > > * References: > > * [1] 82371FB (PIIX) AND 82371SB (PIIX3) PCI ISA IDE XCELERATOR, > > * 290550-002, Intel Corporation, April 1997. > > + * [2] 82371AB PCI-TO-ISA / IDE XCELERATOR (PIIX4), 290562-001, > > + * Intel Corporation, April 1997. > > */ > > > > #include "qemu/osdep.h" > > @@ -160,6 +162,19 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error > > **errp) > > uint8_t *pci_conf = dev->config; > > int rc; > > > > +/* > > + * Mask all IDE PCI command register bits except for Bus Master > > + * Function Enable (bit 2) and I/O Space Enable (bit 1), as the > > + * remainder are hardwired to 0 [1, p.48] [2, p.89-90]. > > + * > > + * NOTE: According to the PIIX3 datasheet [1], the Memory Space > > + * Enable (MSE bit) is hardwired to 1, but this is contradicted by > > + * actual PIIX3 hardware, the datasheet itself (viz., Default > > + * Value: h), and the PIIX4 datasheet [2]. > > + */ > > +pci_set_word(dev->wmask + PCI_COMMAND, > > + PCI_COMMAND_MASTER | PCI_COMMAND_IO); > > + > > pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode > > > > bmdma_setup_bar(d); > > -- > > 2.34.1 > > > > > >
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
On Thu, Sep 22, 2022 at 11:02:56AM +0100, Alex Bennée wrote:
>
> "Michael S. Tsirkin" writes:
>
> > On Wed, Sep 21, 2022 at 07:23:12PM +0100, Alex Bennée wrote:
> >>
> >> chenh writes:
> >>
> >> > From: Hao Chen
> >> >
> >> > When use dpdk-vdpa tests vdpa device. You need to specify the mac
> >> > address to
> >> > start the virtual machine through libvirt or qemu, but now, the libvirt
> >> > or
> >> > qemu can call dpdk vdpa vendor driver's ops .get_config through
> >> > vhost_net_get_config
> >> > to get the mac address of the vdpa hardware without manual configuration.
> >> >
> >> > v1->v2:
> >> > Only copy ETH_ALEN data of netcfg for some vdpa device such as
> >> > NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
> >> > We only need the mac address and don't care about the status field.
> >> >
> >> > Signed-off-by: Hao Chen
> >> > ---
> >> > hw/block/vhost-user-blk.c | 1 -
> >> > hw/net/virtio-net.c | 7 +++
> >> > hw/virtio/vhost-user.c| 19 ---
> >> > 3 files changed, 7 insertions(+), 20 deletions(-)
> >> >
> >> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >> > index 9117222456..5dca4eab09 100644
> >> > --- a/hw/block/vhost-user-blk.c
> >> > +++ b/hw/block/vhost-user-blk.c
> >> > @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
> >> > Error **errp)
> >> >
> >> > vhost_dev_set_config_notifier(>dev, _ops);
> >> >
> >> > -s->vhost_user.supports_config = true;
> >>
> >>
> >>
> >> NACK from me. The supports_config flag is there for a reason.
> >
> >
> > Alex please, do not send NACKs. If you feel compelled to stress
> > your point, provide extra justification instead. Thanks!
>
> OK I was objecting to ripping out the common vhost-user code which was
> implemented as a fix for behaviour found while attempting to upstream:
>
> Subject: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups
> Date: Tue, 2 Aug 2022 10:49:48 +0100
> Message-Id: <20220802095010.3330793-1-alex.ben...@linaro.org>
>
> I vhost-user-blk wants to suppress its use of vhost-user config messages
> I guess that should be a control option but it sounds like a buggy
> back-end.
Thanks for the review!
> >
> >> >
> >> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> > index bd24741be8..8b01078249 100644
> >> > --- a/hw/virtio/vhost-user.c
> >> > +++ b/hw/virtio/vhost-user.c
> >> > @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct
> >> > vhost_dev *dev, void *opaque,
> >> > }
> >> >
> >> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> >> > -bool supports_f_config = vus->supports_config ||
> >> > -(dev->config_ops &&
> >> > dev->config_ops->vhost_dev_config_notifier);
> >> > uint64_t protocol_features;
> >> >
> >> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> >> > @@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct
> >> > vhost_dev *dev, void *opaque,
> >> > */
> >> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
> >> >
> >> > -if (supports_f_config) {
> >> > -if (!virtio_has_feature(protocol_features,
> >> > -VHOST_USER_PROTOCOL_F_CONFIG)) {
> >> > -error_setg(errp, "vhost-user device expecting "
> >> > - "VHOST_USER_PROTOCOL_F_CONFIG but the
> >> > vhost-user backend does "
> >> > - "not support it.");
> >> > -return -EPROTO;
> >> > -}
> >> > -} else {
> >> > -if (virtio_has_feature(protocol_features,
> >> > - VHOST_USER_PROTOCOL_F_CONFIG)) {
> >> > -warn_reportf_err(*errp, "vhost-user backend supports "
> >> > - "VHOST_USER_PROTOCOL_F_CONFIG but QEMU
> >> > does not.");
> >> > -protocol_features &= ~(1ULL <<
> >> > VHOST_USER_PROTOCOL_F_CONFIG);
> >> > -}
> >> > -}
> >> > -
> >> > /* final set of protocol features */
> >> > dev->protocol_features = protocol_features;
> >> > err = vhost_user_set_protocol_features(dev,
> >> > dev->protocol_features);
> >>
> >>
> >> --
> >> Alex Bennée
>
>
> --
> Alex Bennée
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
"Michael S. Tsirkin" writes:
> On Wed, Sep 21, 2022 at 07:23:12PM +0100, Alex Bennée wrote:
>>
>> chenh writes:
>>
>> > From: Hao Chen
>> >
>> > When use dpdk-vdpa tests vdpa device. You need to specify the mac address
>> > to
>> > start the virtual machine through libvirt or qemu, but now, the libvirt or
>> > qemu can call dpdk vdpa vendor driver's ops .get_config through
>> > vhost_net_get_config
>> > to get the mac address of the vdpa hardware without manual configuration.
>> >
>> > v1->v2:
>> > Only copy ETH_ALEN data of netcfg for some vdpa device such as
>> > NVIDIA BLUEFIELD DPU(BF2)'s netcfg->status is not right.
>> > We only need the mac address and don't care about the status field.
>> >
>> > Signed-off-by: Hao Chen
>> > ---
>> > hw/block/vhost-user-blk.c | 1 -
>> > hw/net/virtio-net.c | 7 +++
>> > hw/virtio/vhost-user.c| 19 ---
>> > 3 files changed, 7 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>> > index 9117222456..5dca4eab09 100644
>> > --- a/hw/block/vhost-user-blk.c
>> > +++ b/hw/block/vhost-user-blk.c
>> > @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev,
>> > Error **errp)
>> >
>> > vhost_dev_set_config_notifier(>dev, _ops);
>> >
>> > -s->vhost_user.supports_config = true;
>>
>>
>>
>> NACK from me. The supports_config flag is there for a reason.
>
>
> Alex please, do not send NACKs. If you feel compelled to stress
> your point, provide extra justification instead. Thanks!
OK I was objecting to ripping out the common vhost-user code which was
implemented as a fix for behaviour found while attempting to upstream:
Subject: [PATCH v4 for 7.2 00/22] virtio-gpio and various virtio cleanups
Date: Tue, 2 Aug 2022 10:49:48 +0100
Message-Id: <20220802095010.3330793-1-alex.ben...@linaro.org>
I vhost-user-blk wants to suppress its use of vhost-user config messages
I guess that should be a control option but it sounds like a buggy
back-end.
>
>> >
>> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> > index bd24741be8..8b01078249 100644
>> > --- a/hw/virtio/vhost-user.c
>> > +++ b/hw/virtio/vhost-user.c
>> > @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> > *dev, void *opaque,
>> > }
>> >
>> > if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
>> > -bool supports_f_config = vus->supports_config ||
>> > -(dev->config_ops &&
>> > dev->config_ops->vhost_dev_config_notifier);
>> > uint64_t protocol_features;
>> >
>> > dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
>> > @@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev
>> > *dev, void *opaque,
>> > */
>> > protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;
>> >
>> > -if (supports_f_config) {
>> > -if (!virtio_has_feature(protocol_features,
>> > -VHOST_USER_PROTOCOL_F_CONFIG)) {
>> > -error_setg(errp, "vhost-user device expecting "
>> > - "VHOST_USER_PROTOCOL_F_CONFIG but the
>> > vhost-user backend does "
>> > - "not support it.");
>> > -return -EPROTO;
>> > -}
>> > -} else {
>> > -if (virtio_has_feature(protocol_features,
>> > - VHOST_USER_PROTOCOL_F_CONFIG)) {
>> > -warn_reportf_err(*errp, "vhost-user backend supports "
>> > - "VHOST_USER_PROTOCOL_F_CONFIG but QEMU
>> > does not.");
>> > -protocol_features &= ~(1ULL <<
>> > VHOST_USER_PROTOCOL_F_CONFIG);
>> > -}
>> > -}
>> > -
>> > /* final set of protocol features */
>> > dev->protocol_features = protocol_features;
>> > err = vhost_user_set_protocol_features(dev,
>> > dev->protocol_features);
>>
>>
>> --
>> Alex Bennée
--
Alex Bennée
[PATCH 22/26] coroutine-lock: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
util/qemu-coroutine-lock.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 9ad24ab1af..15c82d9348 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -135,7 +135,7 @@ typedef struct CoWaitRecord {
QSLIST_ENTRY(CoWaitRecord) next;
} CoWaitRecord;
-static void push_waiter(CoMutex *mutex, CoWaitRecord *w)
+static void coroutine_fn push_waiter(CoMutex *mutex, CoWaitRecord *w)
{
w->co = qemu_coroutine_self();
QSLIST_INSERT_HEAD_ATOMIC(>from_push, w, next);
@@ -332,7 +332,7 @@ void qemu_co_rwlock_init(CoRwlock *lock)
}
/* Releases the internal CoMutex. */
-static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
+static void coroutine_fn qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
{
CoRwTicket *tkt = QSIMPLEQ_FIRST(>tickets);
Coroutine *co = NULL;
@@ -365,7 +365,7 @@ static void qemu_co_rwlock_maybe_wake_one(CoRwlock *lock)
}
}
-void qemu_co_rwlock_rdlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock)
{
Coroutine *self = qemu_coroutine_self();
@@ -390,7 +390,7 @@ void qemu_co_rwlock_rdlock(CoRwlock *lock)
self->locks_held++;
}
-void qemu_co_rwlock_unlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_unlock(CoRwlock *lock)
{
Coroutine *self = qemu_coroutine_self();
@@ -408,7 +408,7 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
qemu_co_rwlock_maybe_wake_one(lock);
}
-void qemu_co_rwlock_downgrade(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_downgrade(CoRwlock *lock)
{
qemu_co_mutex_lock(>mutex);
assert(lock->owners == -1);
@@ -418,7 +418,7 @@ void qemu_co_rwlock_downgrade(CoRwlock *lock)
qemu_co_rwlock_maybe_wake_one(lock);
}
-void qemu_co_rwlock_wrlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock)
{
Coroutine *self = qemu_coroutine_self();
@@ -438,7 +438,7 @@ void qemu_co_rwlock_wrlock(CoRwlock *lock)
self->locks_held++;
}
-void qemu_co_rwlock_upgrade(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_upgrade(CoRwlock *lock)
{
qemu_co_mutex_lock(>mutex);
assert(lock->owners > 0);
--
2.37.3
[PATCH 23/26] raw-format: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/raw-format.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/raw-format.c b/block/raw-format.c
index 69fd650eaf..45440345b6 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -411,7 +411,7 @@ static void raw_lock_medium(BlockDriverState *bs, bool
locked)
bdrv_lock_medium(bs->file->bs, locked);
}
-static int raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
+static int coroutine_fn raw_co_ioctl(BlockDriverState *bs, unsigned long int
req, void *buf)
{
BDRVRawState *s = bs->opaque;
if (s->offset || s->has_size) {
--
2.37.3
[PATCH 16/26] curl: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 1e0f609579..cba4c4cac7 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -855,7 +855,7 @@ out_noclean:
return -EINVAL;
}
-static void curl_setup_preadv(BlockDriverState *bs, CURLAIOCB *acb)
+static void coroutine_fn curl_setup_preadv(BlockDriverState *bs, CURLAIOCB
*acb)
{
CURLState *state;
int running;
--
2.37.3
[PATCH 25/26] migration: add missing coroutine_fn annotations
From: Marc-André Lureau
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Signed-off-by: Marc-André Lureau
Reviewed-by: Juan Quintela
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
migration/migration.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index bb8bbddfe4..739bb683f3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -574,7 +574,8 @@ static void process_incoming_migration_bh(void *opaque)
migration_incoming_state_destroy();
}
-static void process_incoming_migration_co(void *opaque)
+static void coroutine_fn
+process_incoming_migration_co(void *opaque)
{
MigrationIncomingState *mis = migration_incoming_get_current();
PostcopyState ps;
--
2.37.3
[PATCH 14/26] qcow2: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/qcow2-cluster.c | 18 +-
block/qcow2-refcount.c | 2 +-
block/qcow2.c | 4 ++--
block/qcow2.h | 14 +++---
4 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fd32316d6f..f6a12ed510 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -884,7 +884,7 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState
*bs,
return 0;
}
-static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
+static int coroutine_fn perform_cow(BlockDriverState *bs, QCowL2Meta *m)
{
BDRVQcow2State *s = bs->opaque;
Qcow2COWRegion *start = >cow_start;
@@ -1024,7 +1024,7 @@ fail:
return ret;
}
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
+int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta
*m)
{
BDRVQcow2State *s = bs->opaque;
int i, j = 0, l2_index, ret;
@@ -1397,8 +1397,8 @@ static int count_single_write_clusters(BlockDriverState
*bs, int nb_clusters,
* information on cluster allocation may be invalid now. The caller
* must start over anyway, so consider *cur_bytes undefined.
*/
-static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
-uint64_t *cur_bytes, QCowL2Meta **m)
+static int coroutine_fn handle_dependencies(BlockDriverState *bs, uint64_t
guest_offset,
+uint64_t *cur_bytes, QCowL2Meta
**m)
{
BDRVQcow2State *s = bs->opaque;
QCowL2Meta *old_alloc;
@@ -1772,9 +1772,9 @@ out:
*
* Return 0 on success and -errno in error cases
*/
-int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
-unsigned int *bytes, uint64_t *host_offset,
-QCowL2Meta **m)
+int coroutine_fn qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
+ unsigned int *bytes, uint64_t
*host_offset,
+ QCowL2Meta **m)
{
BDRVQcow2State *s = bs->opaque;
uint64_t start, remaining;
@@ -2105,8 +2105,8 @@ out:
return ret;
}
-int qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, int flags)
+int coroutine_fn qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t
offset,
+ uint64_t bytes, int flags)
{
BDRVQcow2State *s = bs->opaque;
uint64_t end_offset = offset + bytes;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1a6277c783..1fbb07ca77 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -3706,7 +3706,7 @@ int64_t qcow2_get_last_cluster(BlockDriverState *bs,
int64_t size)
return -EIO;
}
-int qcow2_detect_metadata_preallocation(BlockDriverState *bs)
+int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
int64_t i, end_cluster, cluster_count = 0, threshold;
diff --git a/block/qcow2.c b/block/qcow2.c
index c6c6692fb7..b4d04e91fa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2436,7 +2436,7 @@ static bool merge_cow(uint64_t offset, unsigned bytes,
* Return 1 if the COW regions read as zeroes, 0 if not, < 0 on error.
* Note that returning 0 does not guarantee non-zero data.
*/
-static int is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
+static int coroutine_fn is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
{
/*
* This check is designed for optimization shortcut so it must be
@@ -2454,7 +2454,7 @@ static int is_zero_cow(BlockDriverState *bs, QCowL2Meta
*m)
m->cow_end.nb_bytes);
}
-static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
+static int coroutine_fn handle_alloc_space(BlockDriverState *bs, QCowL2Meta
*l2meta)
{
BDRVQcow2State *s = bs->opaque;
QCowL2Meta *m;
diff --git a/block/qcow2.h b/block/qcow2.h
index c8d9e8ea79..36495d9051 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -895,7 +895,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int
refcount_order,
void *cb_opaque, Error **errp);
int qcow2_shrink_reftable(BlockDriverState *bs);
int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
-int qcow2_detect_metadata_preallocation(BlockDriverState *bs);
+int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs);
/* qcow2-cluster.c functions */
int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
@@ -908,9 +908,9 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t
sector_num,
int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
[PATCH 21/26] job: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
include/qemu/job.h | 2 +-
job.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index c105b31076..397ac39608 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -436,7 +436,7 @@ void coroutine_fn job_pause_point(Job *job);
*
* Yield the job coroutine.
*/
-void job_yield(Job *job);
+void coroutine_fn job_yield(Job *job);
/**
* @job: The job that calls the function.
diff --git a/job.c b/job.c
index 075c6f3a20..20f0d8b2cd 100644
--- a/job.c
+++ b/job.c
@@ -525,7 +525,7 @@ void coroutine_fn job_pause_point(Job *job)
}
}
-void job_yield(Job *job)
+void coroutine_fn job_yield(Job *job)
{
assert(job->busy);
--
2.37.3
[PATCH 20/26] vmdk: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/vmdk.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index fe07a54866..34b5e3f52e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1787,10 +1787,10 @@ static int coroutine_fn
vmdk_co_block_status(BlockDriverState *bs,
return ret;
}
-static int vmdk_write_extent(VmdkExtent *extent, int64_t cluster_offset,
-int64_t offset_in_cluster, QEMUIOVector *qiov,
-uint64_t qiov_offset, uint64_t n_bytes,
-uint64_t offset)
+static int coroutine_fn vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
+ int64_t offset_in_cluster,
QEMUIOVector *qiov,
+ uint64_t qiov_offset, uint64_t
n_bytes,
+ uint64_t offset)
{
int ret;
VmdkGrainMarker *data = NULL;
@@ -1868,9 +1868,9 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
return ret;
}
-static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
-int64_t offset_in_cluster, QEMUIOVector *qiov,
-int bytes)
+static int coroutine_fn vmdk_read_extent(VmdkExtent *extent, int64_t
cluster_offset,
+int64_t offset_in_cluster,
QEMUIOVector *qiov,
+int bytes)
{
int ret;
int cluster_bytes, buf_bytes;
@@ -2015,9 +2015,9 @@ fail:
*
* Returns: error code with 0 for success.
*/
-static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov,
- bool zeroed, bool zero_dry_run)
+static int coroutine_fn vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
+uint64_t bytes, QEMUIOVector *qiov,
+bool zeroed, bool zero_dry_run)
{
BDRVVmdkState *s = bs->opaque;
VmdkExtent *extent = NULL;
--
2.37.3
[PATCH 18/26] quorum: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/quorum.c | 35 ++-
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/block/quorum.c b/block/quorum.c
index f33f30d36b..5ff69d7443 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -161,11 +161,10 @@ static bool quorum_64bits_compare(QuorumVoteValue *a,
QuorumVoteValue *b)
return a->l == b->l;
}
-static QuorumAIOCB *quorum_aio_get(BlockDriverState *bs,
- QEMUIOVector *qiov,
- uint64_t offset,
- uint64_t bytes,
- int flags)
+static QuorumAIOCB *coroutine_fn quorum_aio_get(BlockDriverState *bs,
+QEMUIOVector *qiov,
+uint64_t offset, uint64_t
bytes,
+int flags)
{
BDRVQuorumState *s = bs->opaque;
QuorumAIOCB *acb = g_new(QuorumAIOCB, 1);
@@ -273,7 +272,7 @@ static void quorum_report_bad_versions(BDRVQuorumState *s,
}
}
-static void quorum_rewrite_entry(void *opaque)
+static void coroutine_fn quorum_rewrite_entry(void *opaque)
{
QuorumCo *co = opaque;
QuorumAIOCB *acb = co->acb;
@@ -574,7 +573,7 @@ free_exit:
quorum_free_vote_list(>votes);
}
-static void read_quorum_children_entry(void *opaque)
+static void coroutine_fn read_quorum_children_entry(void *opaque)
{
QuorumCo *co = opaque;
QuorumAIOCB *acb = co->acb;
@@ -602,7 +601,7 @@ static void read_quorum_children_entry(void *opaque)
}
}
-static int read_quorum_children(QuorumAIOCB *acb)
+static int coroutine_fn read_quorum_children(QuorumAIOCB *acb)
{
BDRVQuorumState *s = acb->bs->opaque;
int i;
@@ -643,7 +642,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
return acb->vote_ret;
}
-static int read_fifo_child(QuorumAIOCB *acb)
+static int coroutine_fn read_fifo_child(QuorumAIOCB *acb)
{
BDRVQuorumState *s = acb->bs->opaque;
int n, ret;
@@ -664,8 +663,9 @@ static int read_fifo_child(QuorumAIOCB *acb)
return ret;
}
-static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t
bytes,
-QEMUIOVector *qiov, BdrvRequestFlags flags)
+static int coroutine_fn quorum_co_preadv(BlockDriverState *bs,
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags
flags)
{
BDRVQuorumState *s = bs->opaque;
QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags);
@@ -684,7 +684,7 @@ static int quorum_co_preadv(BlockDriverState *bs, int64_t
offset, int64_t bytes,
return ret;
}
-static void write_quorum_entry(void *opaque)
+static void coroutine_fn write_quorum_entry(void *opaque)
{
QuorumCo *co = opaque;
QuorumAIOCB *acb = co->acb;
@@ -715,9 +715,9 @@ static void write_quorum_entry(void *opaque)
}
}
-static int quorum_co_pwritev(BlockDriverState *bs, int64_t offset,
- int64_t bytes, QEMUIOVector *qiov,
- BdrvRequestFlags flags)
+static int coroutine_fn quorum_co_pwritev(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
BDRVQuorumState *s = bs->opaque;
QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags);
@@ -746,8 +746,9 @@ static int quorum_co_pwritev(BlockDriverState *bs, int64_t
offset,
return ret;
}
-static int quorum_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
- int64_t bytes, BdrvRequestFlags flags)
+static int coroutine_fn quorum_co_pwrite_zeroes(BlockDriverState *bs,
+int64_t offset, int64_t bytes,
+BdrvRequestFlags flags)
{
return quorum_co_pwritev(bs, offset, bytes, NULL,
--
2.37.3
[PATCH 12/26] nvme: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/nvme.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 3e6abef1ce..c51b9f5cd3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1242,8 +1242,9 @@ static inline bool nvme_qiov_aligned(BlockDriverState *bs,
return true;
}
-static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, bool is_write, int flags)
+static coroutine_fn int nvme_co_prw(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, bool is_write, int
flags)
{
BDRVNVMeState *s = bs->opaque;
int r;
--
2.37.3
[PATCH 10/26] nbd: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/nbd.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 97683cce27..786ee79013 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -983,11 +983,11 @@ static void nbd_iter_request_error(NBDReplyChunkIter
*iter, int ret)
* nbd_reply_chunk_iter_receive
* The pointer stored in @payload requires g_free() to free it.
*/
-static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
- NBDReplyChunkIter *iter,
- uint64_t handle,
- QEMUIOVector *qiov, NBDReply *reply,
- void **payload)
+static bool coroutine_fn nbd_reply_chunk_iter_receive(BDRVNBDState *s,
+ NBDReplyChunkIter *iter,
+ uint64_t handle,
+ QEMUIOVector *qiov,
NBDReply *reply,
+ void **payload)
{
int ret, request_ret;
NBDReply local_reply;
--
2.37.3
[PATCH 17/26] qed: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/qed.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index 40943e679b..8b6a5288c5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -254,7 +254,7 @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s)
return l2_table;
}
-static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
+static bool coroutine_fn qed_plug_allocating_write_reqs(BDRVQEDState *s)
{
qemu_co_mutex_lock(>table_lock);
@@ -273,7 +273,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
return true;
}
-static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
+static void coroutine_fn qed_unplug_allocating_write_reqs(BDRVQEDState *s)
{
qemu_co_mutex_lock(>table_lock);
assert(s->allocating_write_reqs_plugged);
--
2.37.3
[PATCH 24/26] 9p: add missing coroutine_fn annotations
From: Marc-André Lureau
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Signed-off-by: Marc-André Lureau
Acked-by: Greg Kurz
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
hw/9pfs/9p.h | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 994f952600..a523ac34a9 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -424,21 +424,24 @@ typedef struct V9fsGetlock
extern int open_fd_hw;
extern int total_open_fd;
-static inline void v9fs_path_write_lock(V9fsState *s)
+static inline void coroutine_fn
+v9fs_path_write_lock(V9fsState *s)
{
if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) {
qemu_co_rwlock_wrlock(>rename_lock);
}
}
-static inline void v9fs_path_read_lock(V9fsState *s)
+static inline void coroutine_fn
+v9fs_path_read_lock(V9fsState *s)
{
if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) {
qemu_co_rwlock_rdlock(>rename_lock);
}
}
-static inline void v9fs_path_unlock(V9fsState *s)
+static inline void coroutine_fn
+v9fs_path_unlock(V9fsState *s)
{
if (s->ctx.export_flags & V9FS_PATHNAME_FSCONTEXT) {
qemu_co_rwlock_unlock(>rename_lock);
--
2.37.3
[PATCH 19/26] throttle: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/throttle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/throttle.c b/block/throttle.c
index 6e8d52fa24..ddd450593a 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -162,7 +162,7 @@ static int coroutine_fn
throttle_co_pwritev_compressed(BlockDriverState *bs,
BDRV_REQ_WRITE_COMPRESSED);
}
-static int throttle_co_flush(BlockDriverState *bs)
+static int coroutine_fn throttle_co_flush(BlockDriverState *bs)
{
return bdrv_co_flush(bs->file->bs);
}
--
2.37.3
[PATCH 26/26] test-coroutine: add missing coroutine_fn annotations
From: Marc-André Lureau
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Signed-off-by: Marc-André Lureau
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
tests/unit/test-coroutine.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/test-coroutine.c b/tests/unit/test-coroutine.c
index aa77a3bcb3..e16b80c245 100644
--- a/tests/unit/test-coroutine.c
+++ b/tests/unit/test-coroutine.c
@@ -610,7 +610,7 @@ static void perf_baseline(void)
g_test_message("Function call %u iterations: %f s", maxcycles, duration);
}
-static __attribute__((noinline)) void perf_cost_func(void *opaque)
+static __attribute__((noinline)) void coroutine_fn perf_cost_func(void *opaque)
{
qemu_coroutine_yield();
}
--
2.37.3
[PATCH 08/26] file-posix: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 48cd096624..76eea8d350 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2158,7 +2158,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
#endif
}
-static int raw_co_flush_to_disk(BlockDriverState *bs)
+static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs)
{
BDRVRawState *s = bs->opaque;
RawPosixAIOData acb;
--
2.37.3
[PATCH 11/26] nfs: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/nfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/nfs.c b/block/nfs.c
index 444c40b458..596ebe98cb 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -223,7 +223,7 @@ static void nfs_process_write(void *arg)
qemu_mutex_unlock(>mutex);
}
-static void nfs_co_init_task(BlockDriverState *bs, NFSRPC *task)
+static void coroutine_fn nfs_co_init_task(BlockDriverState *bs, NFSRPC *task)
{
*task = (NFSRPC) {
.co = qemu_coroutine_self(),
--
2.37.3
[PATCH 15/26] copy-before-write: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/copy-before-write.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index c24b8dd117..14af7eb753 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -203,9 +203,9 @@ static int coroutine_fn cbw_co_flush(BlockDriverState *bs)
* It's guaranteed that guest writes will not interact in the region until
* cbw_snapshot_read_unlock() called.
*/
-static BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
-int64_t offset, int64_t bytes,
-int64_t *pnum, BdrvChild **file)
+static coroutine_fn BlockReq *cbw_snapshot_read_lock(BlockDriverState *bs,
+ int64_t offset, int64_t
bytes,
+ int64_t *pnum, BdrvChild
**file)
{
BDRVCopyBeforeWriteState *s = bs->opaque;
BlockReq *req = g_new(BlockReq, 1);
@@ -240,7 +240,7 @@ static BlockReq *cbw_snapshot_read_lock(BlockDriverState
*bs,
return req;
}
-static void cbw_snapshot_read_unlock(BlockDriverState *bs, BlockReq *req)
+static coroutine_fn void cbw_snapshot_read_unlock(BlockDriverState *bs,
BlockReq *req)
{
BDRVCopyBeforeWriteState *s = bs->opaque;
--
2.37.3
[PATCH 13/26] parallels: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/parallels.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..5fc726f446 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -165,8 +165,9 @@ static int64_t block_status(BDRVParallelsState *s, int64_t
sector_num,
return start_off;
}
-static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, int *pnum)
+static coroutine_fn int64_t allocate_clusters(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors, int *pnum)
{
int ret = 0;
BDRVParallelsState *s = bs->opaque;
--
2.37.3
[PATCH 06/26] blkdebug: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/blkdebug.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index bbf2948703..a93ba61487 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -587,8 +587,8 @@ out:
return ret;
}
-static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- BlkdebugIOType iotype)
+static int coroutine_fn rule_check(BlockDriverState *bs, uint64_t offset,
uint64_t bytes,
+ BlkdebugIOType iotype)
{
BDRVBlkdebugState *s = bs->opaque;
BlkdebugRule *rule = NULL;
@@ -672,7 +672,7 @@ blkdebug_co_pwritev(BlockDriverState *bs, int64_t offset,
int64_t bytes,
return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
}
-static int blkdebug_co_flush(BlockDriverState *bs)
+static int coroutine_fn blkdebug_co_flush(BlockDriverState *bs)
{
int err = rule_check(bs, 0, 0, BLKDEBUG_IO_TYPE_FLUSH);
@@ -791,7 +791,7 @@ static void blkdebug_close(BlockDriverState *bs)
}
/* Called with lock held. */
-static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
+static void coroutine_fn suspend_request(BlockDriverState *bs, BlkdebugRule
*rule)
{
BDRVBlkdebugState *s = bs->opaque;
BlkdebugSuspendedReq *r;
@@ -810,8 +810,8 @@ static void suspend_request(BlockDriverState *bs,
BlkdebugRule *rule)
}
/* Called with lock held. */
-static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
- int *action_count, int *new_state)
+static void coroutine_fn process_rule(BlockDriverState *bs, struct
BlkdebugRule *rule,
+ int *action_count, int *new_state)
{
BDRVBlkdebugState *s = bs->opaque;
@@ -840,7 +840,7 @@ static void process_rule(BlockDriverState *bs, struct
BlkdebugRule *rule,
}
}
-static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
+static void coroutine_fn blkdebug_debug_event(BlockDriverState *bs,
BlkdebugEvent event)
{
BDRVBlkdebugState *s = bs->opaque;
struct BlkdebugRule *rule, *next;
--
2.37.3
[PATCH 05/26] coroutine: remove incorrect coroutine_fn annotations
qemu_coroutine_get_aio_context inspects a coroutine, but it does
not have to be called from the coroutine itself (or from any
coroutine).
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
include/qemu/coroutine.h | 2 +-
util/qemu-coroutine.c| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index 08c5bb3c76..e55b36f49a 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -92,7 +92,7 @@ void coroutine_fn qemu_coroutine_yield(void);
/**
* Get the AioContext of the given coroutine
*/
-AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co);
+AioContext *qemu_coroutine_get_aio_context(Coroutine *co);
/**
* Get the currently executing coroutine
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index 4a8bd63ef0..356b746f0b 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -213,7 +213,7 @@ bool qemu_coroutine_entered(Coroutine *co)
return co->caller;
}
-AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co)
+AioContext *qemu_coroutine_get_aio_context(Coroutine *co)
{
return co->ctx;
}
--
2.37.3
[PATCH 09/26] iscsi: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/iscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index d707d0b354..b33eeec794 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -290,7 +290,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
}
}
-static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask
*iTask)
+static void coroutine_fn iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct
IscsiTask *iTask)
{
*iTask = (struct IscsiTask) {
.co = qemu_coroutine_self(),
--
2.37.3
[PATCH v3 00/26] block: fix coroutine_fn annotations
As discussed at KVM Forum 2022, I am reposting this series to add more coroutine_fn annotations. Fixing the annotations enables static analysis of which functions are coroutine-only and which are mixed (coroutine/non-coroutine). A lot of the patches are similar to the ones that Marc-André Lureau wrote back in 2017 (posted at [1]) but due to the changes in the code it was easier to redo them. This is a simple rebase of v2 (posted at [2]) with the following changes: - patch 1 is new - create_file_fallback_zero_first_sector is now marked as coroutine_fn - blk_pwrite_zeroes is not a coroutine_fn anymore - blk_pwrite is not a coroutine_fn anymore - tracked_request_end is already a coroutine_fn in master - some changes in commit messages Paolo [1] https://patchew.org/QEMU/20170704220346.29244-1-marcandre.lur...@redhat.com/). [2] https://patchew.org/QEMU/20220509103019.215041-1-pbonz...@redhat.com/ Marc-André Lureau (3): 9p: add missing coroutine_fn annotations migration: add missing coroutine_fn annotations test-coroutine: add missing coroutine_fn annotations Paolo Bonzini (23): block/nvme: separate nvme_get_free_req cases for coroutine/non-coroutine context block: add missing coroutine_fn annotations qcow2: remove incorrect coroutine_fn annotations nbd: remove incorrect coroutine_fn annotations coroutine: remove incorrect coroutine_fn annotations blkdebug: add missing coroutine_fn annotations blkverify: add missing coroutine_fn annotations file-posix: add missing coroutine_fn annotations iscsi: add missing coroutine_fn annotations nbd: add missing coroutine_fn annotations nfs: add missing coroutine_fn annotations nvme: add missing coroutine_fn annotations parallels: add missing coroutine_fn annotations qcow2: add missing coroutine_fn annotations copy-before-write: add missing coroutine_fn annotations curl: add missing coroutine_fn annotations qed: add missing coroutine_fn annotations quorum: add missing coroutine_fn annotations throttle: add missing coroutine_fn annotations vmdk: add missing coroutine_fn annotations job: add missing coroutine_fn annotations coroutine-lock: add missing coroutine_fn annotations raw-format: add missing coroutine_fn annotations block.c | 6 ++--- block/blkdebug.c| 14 +- block/blkverify.c | 2 +- block/block-backend.c | 10 +++ block/copy-before-write.c | 8 +++--- block/curl.c| 2 +- block/file-posix.c | 2 +- block/io.c | 22 +++ block/iscsi.c | 2 +- block/nbd.c | 10 +++ block/nfs.c | 2 +- block/nvme.c| 53 ++--- block/parallels.c | 5 ++-- block/qcow2-cluster.c | 18 ++--- block/qcow2-refcount.c | 6 ++--- block/qcow2.c | 4 +-- block/qcow2.h | 18 ++--- block/qed.c | 4 +-- block/quorum.c | 35 block/raw-format.c | 2 +- block/throttle.c| 2 +- block/vmdk.c| 20 +++--- hw/9pfs/9p.h| 9 --- include/block/nbd.h | 2 +- include/qemu/coroutine.h| 2 +- include/qemu/job.h | 2 +- job.c | 2 +- migration/migration.c | 3 ++- tests/unit/test-coroutine.c | 2 +- util/qemu-coroutine-lock.c | 14 +- util/qemu-coroutine.c | 2 +- 31 files changed, 150 insertions(+), 135 deletions(-) -- 2.37.3
[PATCH 07/26] blkverify: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/blkverify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blkverify.c b/block/blkverify.c
index e4a37af3b2..020b1ae7b6 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -258,7 +258,7 @@ blkverify_co_pwritev(BlockDriverState *bs, int64_t offset,
int64_t bytes,
return blkverify_co_prwv(bs, , offset, bytes, qiov, qiov, flags, true);
}
-static int blkverify_co_flush(BlockDriverState *bs)
+static int coroutine_fn blkverify_co_flush(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
--
2.37.3
[PATCH 04/26] nbd: remove incorrect coroutine_fn annotations
nbd_co_establish_connection_cancel() cancels a coroutine but is not called from coroutine context itself, for example in nbd_cancel_in_flight() and in timer callbacks reconnect_delay_timer_cb() and open_timer_cb(). Reviewed-by: Alberto Faria Reviewed-by: Eric Blake Signed-off-by: Paolo Bonzini --- include/block/nbd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/block/nbd.h b/include/block/nbd.h index c74b7a9d2e..4ede3b2bd0 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -424,6 +424,6 @@ QIOChannel *coroutine_fn nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info, bool blocking, Error **errp); -void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection *conn); +void nbd_co_establish_connection_cancel(NBDClientConnection *conn); #endif -- 2.37.3
[PATCH 01/26] block/nvme: separate nvme_get_free_req cases for coroutine/non-coroutine context
nvme_get_free_req has very difference semantics when called in
coroutine context (where it waits) and in non-coroutine context
(where it doesn't). Split the two cases to make it clear what
is being requested.
Cc: qemu-block@nongnu.org
Reviewed-by: Alberto Faria
Signed-off-by: Paolo Bonzini
---
block/nvme.c | 48
1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 01fb28aa63..3e6abef1ce 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -293,34 +293,42 @@ static void nvme_kick(NVMeQueuePair *q)
q->need_kick = 0;
}
-/* Find a free request element if any, otherwise:
- * a) if in coroutine context, try to wait for one to become available;
- * b) if not in coroutine, return NULL;
- */
-static NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
+static NVMeRequest *nvme_get_free_req_nofail_locked(NVMeQueuePair *q)
{
NVMeRequest *req;
-qemu_mutex_lock(>lock);
-
-while (q->free_req_head == -1) {
-if (qemu_in_coroutine()) {
-trace_nvme_free_req_queue_wait(q->s, q->index);
-qemu_co_queue_wait(>free_req_queue, >lock);
-} else {
-qemu_mutex_unlock(>lock);
-return NULL;
-}
-}
-
req = >reqs[q->free_req_head];
q->free_req_head = req->free_req_next;
req->free_req_next = -1;
-
-qemu_mutex_unlock(>lock);
return req;
}
+/* Return a free request element if any, otherwise return NULL. */
+static NVMeRequest *nvme_get_free_req_nowait(NVMeQueuePair *q)
+{
+QEMU_LOCK_GUARD(>lock);
+if (q->free_req_head == -1) {
+return NULL;
+}
+return nvme_get_free_req_nofail_locked(q);
+}
+
+/*
+ * Wait for a free request to become available if necessary, then
+ * return it.
+ */
+static coroutine_fn NVMeRequest *nvme_get_free_req(NVMeQueuePair *q)
+{
+QEMU_LOCK_GUARD(>lock);
+
+while (q->free_req_head == -1) {
+ trace_nvme_free_req_queue_wait(q->s, q->index);
+ qemu_co_queue_wait(>free_req_queue, >lock);
+}
+
+return nvme_get_free_req_nofail_locked(q);
+}
+
/* With q->lock */
static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req)
{
@@ -506,7 +514,7 @@ static int nvme_admin_cmd_sync(BlockDriverState *bs,
NvmeCmd *cmd)
AioContext *aio_context = bdrv_get_aio_context(bs);
NVMeRequest *req;
int ret = -EINPROGRESS;
-req = nvme_get_free_req(q);
+req = nvme_get_free_req_nowait(q);
if (!req) {
return -EBUSY;
}
--
2.37.3
[PATCH 02/26] block: add missing coroutine_fn annotations
Callers of coroutine_fn must be coroutine_fn themselves, or the call
must be within "if (qemu_in_coroutine())". Apply coroutine_fn to
functions where this holds.
Signed-off-by: Paolo Bonzini
---
block.c | 6 +++---
block/block-backend.c | 10 +-
block/io.c| 22 +++---
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/block.c b/block.c
index bc85f46eed..5c34ada53f 100644
--- a/block.c
+++ b/block.c
@@ -631,9 +631,9 @@ static int64_t create_file_fallback_truncate(BlockBackend
*blk,
* Helper function for bdrv_create_file_fallback(): Zero the first
* sector to remove any potentially pre-existing image header.
*/
-static int create_file_fallback_zero_first_sector(BlockBackend *blk,
- int64_t current_size,
- Error **errp)
+static int coroutine_fn create_file_fallback_zero_first_sector(BlockBackend
*blk,
+ int64_t
current_size,
+ Error **errp)
{
int64_t bytes_to_clear;
int ret;
diff --git a/block/block-backend.c b/block/block-backend.c
index d4a5df2ac2..aa4adf06ae 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1546,7 +1546,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk,
int64_t offset,
return >common;
}
-static void blk_aio_read_entry(void *opaque)
+static void coroutine_fn blk_aio_read_entry(void *opaque)
{
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
@@ -1558,7 +1558,7 @@ static void blk_aio_read_entry(void *opaque)
blk_aio_complete(acb);
}
-static void blk_aio_write_entry(void *opaque)
+static void coroutine_fn blk_aio_write_entry(void *opaque)
{
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
@@ -1669,7 +1669,7 @@ int coroutine_fn blk_co_ioctl(BlockBackend *blk, unsigned
long int req,
return ret;
}
-static void blk_aio_ioctl_entry(void *opaque)
+static void coroutine_fn blk_aio_ioctl_entry(void *opaque)
{
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
@@ -1703,7 +1703,7 @@ blk_co_do_pdiscard(BlockBackend *blk, int64_t offset,
int64_t bytes)
return bdrv_co_pdiscard(blk->root, offset, bytes);
}
-static void blk_aio_pdiscard_entry(void *opaque)
+static void coroutine_fn blk_aio_pdiscard_entry(void *opaque)
{
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
@@ -1747,7 +1747,7 @@ static int coroutine_fn blk_co_do_flush(BlockBackend *blk)
return bdrv_co_flush(blk_bs(blk));
}
-static void blk_aio_flush_entry(void *opaque)
+static void coroutine_fn blk_aio_flush_entry(void *opaque)
{
BlkAioEmAIOCB *acb = opaque;
BlkRwCo *rwco = >rwco;
diff --git a/block/io.c b/block/io.c
index 0a8cbefe86..e3dcb8e7e6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -751,11 +751,11 @@ static void coroutine_fn
tracked_request_end(BdrvTrackedRequest *req)
/**
* Add an active request to the tracked requests list
*/
-static void tracked_request_begin(BdrvTrackedRequest *req,
- BlockDriverState *bs,
- int64_t offset,
- int64_t bytes,
- enum BdrvTrackedRequestType type)
+static void coroutine_fn tracked_request_begin(BdrvTrackedRequest *req,
+ BlockDriverState *bs,
+ int64_t offset,
+ int64_t bytes,
+ enum BdrvTrackedRequestType
type)
{
bdrv_check_request(offset, bytes, _abort);
@@ -794,7 +794,7 @@ static bool tracked_request_overlaps(BdrvTrackedRequest
*req,
}
/* Called with self->bs->reqs_lock held */
-static BdrvTrackedRequest *
+static coroutine_fn BdrvTrackedRequest *
bdrv_find_conflicting_request(BdrvTrackedRequest *self)
{
BdrvTrackedRequest *req;
@@ -1644,10 +1644,10 @@ static bool bdrv_init_padding(BlockDriverState *bs,
return true;
}
-static int bdrv_padding_rmw_read(BdrvChild *child,
- BdrvTrackedRequest *req,
- BdrvRequestPadding *pad,
- bool zero_middle)
+static coroutine_fn int bdrv_padding_rmw_read(BdrvChild *child,
+ BdrvTrackedRequest *req,
+ BdrvRequestPadding *pad,
+ bool zero_middle)
{
QEMUIOVector local_qiov;
BlockDriverState *bs = child->bs;
@@ -3168,7 +3168,7 @@ out:
return ret;
}
-int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
+int coroutine_fn bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
{
BlockDriver *drv = bs->drv;
CoroutineIOCompletion co = {
--
[PATCH 03/26] qcow2: remove incorrect coroutine_fn annotations
This is incorrect because qcow2_mark_clean() calls qcow2_flush_caches().
qcow2_mark_clean() is called from non-coroutine context in
qcow2_inactivate() and qcow2_amend_options().
Reviewed-by: Alberto Faria
Reviewed-by: Eric Blake
Signed-off-by: Paolo Bonzini
---
block/qcow2-refcount.c | 4 ++--
block/qcow2.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c4d99817b6..1a6277c783 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1206,7 +1206,7 @@ void qcow2_free_any_cluster(BlockDriverState *bs,
uint64_t l2_entry,
}
}
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
+int qcow2_write_caches(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
int ret;
@@ -1226,7 +1226,7 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
return 0;
}
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
+int qcow2_flush_caches(BlockDriverState *bs)
{
int ret = qcow2_write_caches(bs);
if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index ba436a8d0d..c8d9e8ea79 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -874,8 +874,8 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t
l2_entry,
int qcow2_update_snapshot_refcount(BlockDriverState *bs,
int64_t l1_table_offset, int l1_size, int addend);
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs);
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs);
+int qcow2_flush_caches(BlockDriverState *bs);
+int qcow2_write_caches(BlockDriverState *bs);
int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix);
--
2.37.3
Re: [PATCH 1/9] hw/riscv/sifive_e: Fix inheritance of SiFiveEState
Am 21. September 2022 04:55:02 UTC schrieb Markus Armbruster : >Bernhard Beschow writes: > >> Am 20. September 2022 11:36:47 UTC schrieb Markus Armbruster >> : >>>Alistair Francis writes: >>> On Tue, Sep 20, 2022 at 9:18 AM Bernhard Beschow wrote: > > SiFiveEState inherits from SysBusDevice while it's TypeInfo claims it to > inherit from TYPE_MACHINE. This is an inconsistency which can cause > undefined behavior such as memory corruption. > > Change SiFiveEState to inherit from MachineState since it is registered > as a machine. > > Signed-off-by: Bernhard Beschow Reviewed-by: Alistair Francis >>> >>>To the SiFive maintainers: since this is a bug fix, let's merge it right >>>away. >> >> I could repost this particular patch with the three new tags (incl. Fixes) >> if desired. > >Can't hurt, and could help the maintainers. [X] Done. Best regards, Bernhard
