Re: [PATCH v1 01/10] monitor: expose monitor_puts to rest of code
Alex Bennée writes: > 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: Markus Armbruster
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
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
>
>
>
> 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 <<
> >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);
>
> >--
>
> >2.27.0
>
> >
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
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!
> >
> > 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
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
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.
>
> 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
Re: [PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
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.
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 << 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);
>--
>2.27.0
>
Re: [PATCH v1 01/10] monitor: expose monitor_puts to rest of code
On 21/9/22 18:07, Alex Bennée wrote: 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 --- v2 - s/monitor_printf(mon, "%s"/monitor_puts(mon, / --- docs/devel/writing-monitor-commands.rst | 2 +- include/monitor/monitor.h | 1 + monitor/monitor-internal.h | 1 - block/monitor/block-hmp-cmds.c | 10 +- hw/misc/mos6522.c | 2 +- monitor/hmp-cmds.c | 8 monitor/hmp.c | 2 +- target/i386/helper.c| 2 +- 8 files changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH v1 01/10] monitor: expose monitor_puts to rest of code
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
---
v2
- s/monitor_printf(mon, "%s"/monitor_puts(mon, /
---
docs/devel/writing-monitor-commands.rst | 2 +-
include/monitor/monitor.h | 1 +
monitor/monitor-internal.h | 1 -
block/monitor/block-hmp-cmds.c | 10 +-
hw/misc/mos6522.c | 2 +-
monitor/hmp-cmds.c | 8
monitor/hmp.c | 2 +-
target/i386/helper.c| 2 +-
8 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/docs/devel/writing-monitor-commands.rst
b/docs/devel/writing-monitor-commands.rst
index 4aa2bb904d..2fefedcd98 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -716,7 +716,7 @@ message. Here's the implementation of the "info roms" HMP
command::
if (hmp_handle_error(mon, err)) {
return;
}
- monitor_printf(mon, "%s", info->human_readable_text);
+ monitor_puts(mon, info->human_readable_text);
}
Also, you have to add the function's prototype to the hmp.h file.
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a4b40e8391..737e750670 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -31,6 +31,7 @@ void monitor_resume(Monitor *mon);
int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
+int monitor_puts(Monitor *mon, const char *str);
int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
G_GNUC_PRINTF(2, 0);
int monitor_printf(Monitor *mon, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index caa2e90ef2..a2cdbbf646 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -174,7 +174,6 @@ extern int mon_refcount;
extern HMPCommand hmp_cmds[];
-int monitor_puts(Monitor *mon, const char *str);
void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
bool use_io_thread);
void monitor_data_destroy(Monitor *mon);
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bfb3c043a0..939a520d17 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -638,16 +638,16 @@ static void print_block_info(Monitor *mon, BlockInfo
*info,
assert(!info || !info->has_inserted || info->inserted == inserted);
if (info && *info->device) {
-monitor_printf(mon, "%s", info->device);
+monitor_puts(mon, info->device);
if (inserted && inserted->has_node_name) {
monitor_printf(mon, " (%s)", inserted->node_name);
}
} else {
assert(info || inserted);
-monitor_printf(mon, "%s",
- inserted && inserted->has_node_name ?
inserted->node_name
- : info && info->has_qdev ? info->qdev
- : "");
+monitor_puts(mon,
+ inserted && inserted->has_node_name ? inserted->node_name
+ : info && info->has_qdev ? info->qdev
+ : "");
}
if (inserted) {
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index f9e646350e..fe38c44426 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -595,7 +595,7 @@ void hmp_info_via(Monitor *mon, const QDict *qdict)
if (hmp_handle_error(mon, err)) {
return;
}
-monitor_printf(mon, "%s", info->human_readable_text);
+monitor_puts(mon, info->human_readable_text);
}
static const MemoryRegionOps mos6522_ops = {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index c6cd6f91dd..f90eea8d01 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -730,7 +730,7 @@ static void hmp_info_pci_device(Monitor *mon, const
PciDeviceInfo *dev)
monitor_printf(mon, "");
if (dev->class_info->has_desc) {
-monitor_printf(mon, "%s", dev->class_info->desc);
+monitor_puts(mon, dev->class_info->desc);
} else {
monitor_printf(mon, "Class %04" PRId64, dev->class_info->q_class);
}
@@ -2258,12 +2258,12 @@ static void print_stats_schema_value(Monitor *mon,
StatsSchemaValue *value)
if (unit && value->base == 10 &&
value->exponent >= -18 && value->exponent <= 18 &&
value->exponent % 3 == 0) {
-monitor_printf(mon, "%s", si_prefix(value->exponent));
+monitor_puts(mon, si_prefix(value->exponent));
} else if (unit && value->base == 2 &&
value->exponent >= 0 && value->exponent <= 60 &&
value->exponent % 10 == 0) {
-
RE: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
-Original Message-
From: Philippe Mathieu-Daudé On Behalf Of
Philippe Mathieu-Daudé
Sent: Sunday, September 18, 2022 5:32 AM
To: Bin Meng ; qemu-de...@nongnu.org
Cc: Meng, Bin ; Hanna Reitz ; Kevin
Wolf ; Peter Lieven ; qemu-block@nongnu.org
Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
[Please note: This e-mail is from an EXTERNAL e-mail address]
On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng
>
> libnfs.h declares nfs_fstat() as the following for win32:
>
>int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
> struct __stat64 *st);
>
> The 'st' parameter should be of type 'struct __stat64'. The codes
> happen to build successfully for 64-bit Windows, but it does not build
> for 32-bit Windows.
>
> Fixes: 6542aa9c75bc ("block: add native support for NFS")
> Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for
> read-only files")
> Signed-off-by: Bin Meng
> ---
>
> block/nfs.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd
> 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client,
> BlockdevOptionsNfs *opts,
> int flags, int open_flags, Error **errp)
> {
> int64_t ret = -EINVAL;
> +#ifdef _WIN32
> +struct __stat64 st;
> +#else
> struct stat st;
> +#endif
> char *file = NULL, *strp = NULL;
>
> qemu_mutex_init(>mutex); @@ -781,7 +785,11 @@ static int
> nfs_reopen_prepare(BDRVReopenState *state,
> BlockReopenQueue *queue, Error **errp)
> {
> NFSClient *client = state->bs->opaque;
> +#ifdef _WIN32
> +struct __stat64 st;
> +#else
> struct stat st;
> +#endif
> int ret = 0;
>
> if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs))
> {
What about the field in struct NFSRPC?
NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and
nfs_get_allocated_file_size() that are not built on win32, so there is no
problem.
Regards,
Bin
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
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...
Thomas
Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging
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?
Regards,
Bin
Re: [PATCH 0/9] Deprecate sysbus_get_default() and get_system_memory() et. al
On Tue, 20 Sept 2022 at 23:50, Bernhard Beschow wrote: > > Am 20. September 2022 09:55:37 UTC schrieb Peter Maydell > : > >On Tue, 20 Sept 2022 at 00:18, Bernhard Beschow wrote: > >> > >> In address-spaces.h it can be read that get_system_memory() and > >> get_system_io() are temporary interfaces which "should only be used > >> temporarily > >> until a proper bus interface is available". This statement certainly > >> extends to > >> the address_space_memory and address_space_io singletons. > > > >This is a long standing "we never really completed a cleanup"... > > > >> This series attempts > >> to stop further proliferation of their use by turning TYPE_SYSTEM_BUS into > >> an > >> object-oriented, "proper bus interface" inspired by PCIBus. > >> > >> While at it, also the main_system_bus singleton is turned into an > >> attribute of > >> MachineState. Together, this resolves five singletons in total, making the > >> ownership relations much more obvious which helps comprehension. > > > >...but I don't think this is the direction we want to go. > >Overall the reason that the "system memory" and "system IO" > >singletons are weird is that in theory they should not be necessary > >at all -- board code should create devices and map them into an > >entirely arbitrary MemoryRegion or set of MemoryRegions corresponding > >to address space(s) for the CPU and for DMA-capable devices. > > My intention was to allow exactly that: By turning sytem memory and system IO > into non-singletons, one could have many of them, thus allowing boards to > create arbitrary mappings of memory and IO. Since QEMU currently assumes one > set (memory and IO) of addresses, I for now instantiated the SysBus once in > the machine class to preserve behavior. You can already create arbitrary mappings of memory and IO (look at the virt board for an example). The existence of the legacy singleton system-memory and system-io doesn't prevent that, and stuffing the singletons into the MachineState doesn't do anything to change the code that is relying on the singletons. > >Retaining the whole-system singleton but shoving it into MachineState > >doesn't really change much, IMHO. > > > >More generally, sysbus is rather weird because it isn't really a > >bus. Every device in the system of TYPE_SYS_BUS_DEVICE is "on" > >the unique TYPE_SYSTEM_BUS bus, but that doesn't mean they're > >all in the same address space or that in real hardware they'd > >all be on the same bus. > > Again, having multiple SysBuses may solve that issue. We definitely don't want multiple sysbuses. thanks -- PMM
Re: [PATCH v9 3/7] block: add block layer APIs resembling Linux ZonedBlockDevice ioctls
On Sep 21 13:44, Damien Le Moal wrote:
> On 9/20/22 17:51, Klaus Jensen wrote:
> > On Sep 10 13:27, Sam Li wrote:
> > > Add a new zoned_host_device BlockDriver. The zoned_host_device option
> > > accepts only zoned host block devices. By adding zone management
> > > operations in this new BlockDriver, users can use the new block
> > > layer APIs including Report Zone and four zone management operations
> > > (open, close, finish, reset).
> > >
> > > Qemu-io uses the new APIs to perform zoned storage commands of the device:
> > > zone_report(zrp), zone_open(zo), zone_close(zc), zone_reset(zrs),
> > > zone_finish(zf).
> > >
> > > For example, to test zone_report, use following command:
> > > $ ./build/qemu-io --image-opts -n driver=zoned_host_device,
> > > filename=/dev/nullb0
> > > -c "zrp offset nr_zones"
> > >
> > > Signed-off-by: Sam Li
> > > Reviewed-by: Hannes Reinecke
> > > ---
> > > block/block-backend.c | 145 ++
> > > block/file-posix.c| 323 +-
> > > block/io.c| 41
> > > include/block/block-io.h | 7 +
> > > include/block/block_int-common.h | 21 ++
> > > include/block/raw-aio.h | 6 +-
> > > include/sysemu/block-backend-io.h | 17 ++
> > > meson.build | 1 +
> > > qapi/block-core.json | 8 +-
> > > qemu-io-cmds.c| 143 +
> > > 10 files changed, 708 insertions(+), 4 deletions(-)
> > >
> > > +/*
> > > + * zone management operations - Execute an operation on a zone
> > > + */
> > > +static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs,
> > > BlockZoneOp op,
> > > +int64_t offset, int64_t len) {
> > > +#if defined(CONFIG_BLKZONED)
> > > +BDRVRawState *s = bs->opaque;
> > > +RawPosixAIOData acb;
> > > +int64_t zone_sector, zone_sector_mask;
> > > +const char *zone_op_name;
> > > +unsigned long zone_op;
> > > +bool is_all = false;
> > > +
> > > +zone_sector = bs->bl.zone_sectors;
> > > +zone_sector_mask = zone_sector - 1;
> > > +if (offset & zone_sector_mask) {
> > > +error_report("sector offset %" PRId64 " is not aligned to zone
> > > size "
> > > + "%" PRId64 "", offset, zone_sector);
> > > +return -EINVAL;
> > > +}
> > > +
> > > +if (len & zone_sector_mask) {
> > > +error_report("number of sectors %" PRId64 " is not aligned to
> > > zone size"
> > > + " %" PRId64 "", len, zone_sector);
> > > +return -EINVAL;
> > > +}
> >
> > These checks impose a power-of-two constraint on the zone size. Can they
> > be changed to divisions to lift that constraint? I don't see anything in
> > this patch set that relies on power of two zone sizes.
>
> Given that Linux will only expose zoned devices that have a zone size that
> is a power of 2 number of LBAs, this will work as is and avoid divisions in
> the IO path. But given that zone management operations are not performance
> critical, generalizing the code should be fine.
>
Aight. That's fine, we can relax the constraint later. But I don't see why.
> However, once we start adding the code for full zone emulation on top of a
> regular file or qcow image, sector-to-zone conversions requiring divisions
> will hurt. So I really would prefer the code be left as-is for now.
>
Block driver based zone emulation would not hit the above code path
anyway (there would be no iotcl to call), so I don't think that is an
argument for keeping it as-is.
On the sector-to-zone convertions. Yes, they may potentially hurt, but
it would be emulation after all. Why would we care about optimizing
those conversions at the expense of not being able to emulate all valid
geometries? The performance option (which this series enables) is to use
an underlying zoned device through virtio (or, potentially, hw/nvme).
What would be the usecase for deploying a performance sensitive emulated
zoned device?
signature.asc
Description: PGP signature
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value
On Wed 21 Sep 2022 09:47:32 AM +08, Wang Liang wrote:
>> > -return limit->slice_end_time - now;
>> > +return MAX(limit->slice_end_time - now, 0);
>>
>> How can this be negative? slice_end_time is guaranteed to be larger
>> than
>> now:
>>
>> if (limit->slice_end_time < now) {
>> /* Previous, possibly extended, time slice finished; reset
>> the
>> * accounting. */
>> limit->slice_start_time = now;
>> limit->slice_end_time = now + limit->slice_ns;
>> limit->dispatched = 0;
>> }
>>
> This is just a guarantee.
>
> If slice_end_time is assigned later by
> limit->slice_end_time = limit->slice_start_time +
> (uint64_t)(delay_slices * limit->slice_ns);
Ok, on a closer look, if at the start of the function
limit->slice_start_time < now, and
limit->slice_end_time >= now
it seems that in principle what you say can happen.
If it's so, it would be good to know under what conditions this happens,
because this hasn't changed in years.
Berto
Re: [PATCH] ratelimit: restrict the delay time to a non-negative value
On Wed, 2022-09-21 at 06:53 +0200, Markus Armbruster wrote:
> Wang Liang writes:
>
> > On Tue, 2022-09-20 at 13:18 +, Alberto Garcia wrote:
> > > On Tue 20 Sep 2022 08:33:50 PM +08, wanglian...@126.com wrote:
> > > > From: Wang Liang
> > > >
> > > > The delay time should never be a negative value.
> > > >
> > > > -return limit->slice_end_time - now;
> > > > +return MAX(limit->slice_end_time - now, 0);
> > >
> > > How can this be negative? slice_end_time is guaranteed to be
> > > larger
> > > than
> > > now:
> > >
> > > if (limit->slice_end_time < now) {
> > > /* Previous, possibly extended, time slice finished;
> > > reset
> > > the
> > > * accounting. */
> > > limit->slice_start_time = now;
> > > limit->slice_end_time = now + limit->slice_ns;
> > > limit->dispatched = 0;
> > > }
> > >
> > This is just a guarantee.
>
> Smells like an invariant to me.
>
> > If slice_end_time is assigned later by
> > limit->slice_end_time = limit->slice_start_time +
> > (uint64_t)(delay_slices * limit->slice_ns);
> > There may be precision issues at that time.
>
> What are the issues exactly? What misbehavior are you observing?
>
> Your commit message should show how delay time can become negative,
> and
> why that's bad.
It was observed in a production environment based on qemu v2.12.1.
The block-stream job delayed a very long time and do not get any
progress since ratelimit_calculate_delay returns a negative value.
Sorry, I don't have an environment to reproduce it in the mainline
version now.
Re: [PATCH] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
在 2022/9/21 13:28, Raphael Norwitz 写道:
I have some concerns from the vhost-user-blk side.
>On Tue, Sep 13, 2022 at 5:13 PM Hao Chen wrote:
>>
>> 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.
>>
>> Signed-off-by: Hao Chen
>
>Adding Cindy for comments.
>
>Thanks
>
>> ---
>>hw/block/vhost-user-blk.c |1 -
>>hw/net/virtio-net.c |3 ++-
>>hw/virtio/vhost-user.c| 19 ---
>>3 files changed, 2 insertions(+), 21 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;
vhost-user-blk requires the backend to support
VHOST_USER_PROTOCOL_F_CONFIG
and vhost_user.supports_config is used to enforce that.
Why are you removing it here?
>>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..274ea84644 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -149,7 +149,8 @@ static void virtio_net_get_config(VirtIODevice *vdev,
uint8_t *config)
>> * Is this VDPA? No peer means not VDPA: there's no way to
>> * disconnect/reconnect a VDPA peer.
>> */
>> -if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>> +if ((nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) ||
>> +(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) {
>> 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,
>>}
>>
Why are you removing this? Can you expand on how it helps dpdk-vdpa.
I implemented a function by modifying these places in the patch, and
then implementing vhost msg ops '.get_config' in the driver under
'DPDK_SRC/drivers/vdpa/' , qemu can use 'vhost_user_get_config' to send
vhost msg to calls the dpdk vdpa vendor driver's ops 'get_config' to
obtain the mac address information of the vdpa device. In this way, even
if the mac address of the network device parameters configured by the
user for qemu vhost user is inconsistent with the mac address of the
vdpa hardware, qemu can be corrected to be consistent with the mac
address of the vdpa hardware.
When vhost-user interface is used by dpdk-vdpa & qemu,
'vhost_user_get_config' will get the vdpa device's mac address if dpdk
vdpa vendor driver implement 'get_config',otherwise, it won't do anything.
When vhost-user interface is used by dpdk vhost-user, it won't do anything.
I think the functions provided by the VHOST_USER_PROTOCOL_F_CONFIG
feature can be available separately, but the check here caused
VHOST_USER_PROTOCOL_F_CONFIG feature is cleared, which will cause
vhost_user_get_config is not working properly in virtio-net at
hw/net/virtio_net.c.
Maybe the check here should be moved other side?
The new patch v2 has been sent, please reply there.
>>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);
>> --
>> 2.27.0
>>
>
[PATCH v2] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically
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 << 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);
--
2.27.0
