Re: [PATCH v2 3/8] virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size
On 9/2/22 8:54 PM, Raphael Norwitz wrote:
On Fri, Aug 26, 2022 at 05:32:43PM +0300, Daniil Tatianin wrote:
Use the new common helper. As an added bonus this also makes use of
config size sanity checking via the 'max_size' field.
Signed-off-by: Daniil Tatianin
---
hw/net/virtio-net.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..dfc8dd8562 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -106,6 +106,11 @@ static const VirtIOFeature feature_sizes[] = {
{}
};
+static const VirtIOConfigSizeParams cfg_size_params = {
Can we have a zero length virtio-net config size? The both the v1.0 and
v1.1 section 5.1.4 say “The mac address field always exists (though is
only valid if VIRTIO_NET_F_MAC is set)” so we should probably set
min_size to offset_of status?
It currently hardcodes VIRTIO_NET_F_MAC as always on, but I guess it
doesn't hurt to be more explicit about it. Will add that in the next
version.
(resending because forgot to reply-all last time)
Otherwise LGTM.
+.max_size = sizeof(struct virtio_net_config),
+.feature_sizes = feature_sizes
+};
+
static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
{
VirtIONet *n = qemu_get_nic_opaque(nc);
@@ -3246,8 +3251,7 @@ static void virtio_net_set_config_size(VirtIONet *n,
uint64_t host_features)
{
virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
-n->config_size = virtio_feature_get_config_size(feature_sizes,
-host_features);
+n->config_size = virtio_get_config_size(&cfg_size_params, host_features);
}
void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
--
2.25.1
Re: [PATCH v2 5/8] virtio-blk: move config size params to virtio-blk-common
On 9/2/22 8:57 PM, Raphael Norwitz wrote:
The vhost-user-blk bits in meson.build and Maintainers should probably
be in patch 8?
You're totally right, thanks.
Otherwise LGTM.
On Fri, Aug 26, 2022 at 05:32:45PM +0300, Daniil Tatianin wrote:
This way we can reuse it for other virtio-blk devices, e.g
vhost-user-blk, which currently does not control its config space size
dynamically.
Signed-off-by: Daniil Tatianin
---
MAINTAINERS | 4 +++
hw/block/meson.build | 4 +--
hw/block/virtio-blk-common.c | 39 +++
hw/block/virtio-blk.c | 24 ++---
include/hw/virtio/virtio-blk-common.h | 20 ++
5 files changed, 67 insertions(+), 24 deletions(-)
create mode 100644 hw/block/virtio-blk-common.c
create mode 100644 include/hw/virtio/virtio-blk-common.h
i.e. move this.
@@ -2271,11 +2273,13 @@ S: Maintained
F: contrib/vhost-user-blk/
F: contrib/vhost-user-scsi/
F: hw/block/vhost-user-blk.c
+F: hw/block/virtio-blk-common.c
F: hw/scsi/vhost-user-scsi.c
F: hw/virtio/vhost-user-blk-pci.c
F: hw/virtio/vhost-user-scsi-pci.c
F: include/hw/virtio/vhost-user-blk.h
F: include/hw/virtio/vhost-user-scsi.h
+F: include/hw/virtio/virtio-blk-common.h
vhost-user-gpu
M: Marc-André Lureau
diff --git a/hw/block/meson.build b/hw/block/meson.build
index 2389326112..1908abd45c 100644
--- a/hw/block/meson.build
+++ b/hw/block/meson.build
@@ -16,7 +16,7 @@ softmmu_ss.add(when: 'CONFIG_SWIM', if_true: files('swim.c'))
softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
softmmu_ss.add(when: 'CONFIG_TC58128', if_true: files('tc58128.c'))
-specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
-specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
files('vhost-user-blk.c'))
+specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c',
'virtio-blk-common.c'))
And this
+specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
files('vhost-user-blk.c', 'virtio-blk-common.c'))
Re: [PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size
On 9/2/22 8:52 PM, Raphael Norwitz wrote:
I feel like it would be easier to review if the first 4 patches were
squashed together, but that’s not a big deal.
Yeah, I think that's fair although I initially thought that maybe that
was a bit too big of a change to put in one single commit. I can squash
the first four if that would be better.
For this one:
Reviewed-by: Raphael Norwitz
On Fri, Aug 26, 2022 at 05:32:41PM +0300, Daniil Tatianin wrote:
This is the first step towards moving all device config size calculation
logic into the virtio core code. In particular, this adds a struct that
contains all the necessary information for common virtio code to be able
to calculate the final config size for a device. This is expected to be
used with the new virtio_get_config_size helper, which calculates the
final length based on the provided host features.
This builds on top of already existing code like VirtIOFeature and
virtio_feature_get_config_size(), but adds additional fields, as well as
sanity checking so that device-specifc code doesn't have to duplicate it.
An example usage would be:
static const VirtIOFeature dev_features[] = {
{.flags = 1ULL << FEATURE_1_BIT,
.end = endof(struct virtio_dev_config, feature_1)},
{.flags = 1ULL << FEATURE_2_BIT,
.end = endof(struct virtio_dev_config, feature_2)},
{}
};
static const VirtIOConfigSizeParams dev_cfg_size_params = {
.min_size = DEV_BASE_CONFIG_SIZE,
.max_size = sizeof(struct virtio_dev_config),
.feature_sizes = dev_features
};
// code inside my_dev_device_realize()
size_t config_size = virtio_get_config_size(&dev_cfg_size_params,
host_features);
virtio_init(vdev, VIRTIO_ID_MYDEV, config_size);
Currently every device is expected to write its own boilerplate from the
example above in device_realize(), however, the next step of this
transition is moving VirtIOConfigSizeParams into VirtioDeviceClass,
so that it can be done automatically by the virtio initialization code.
Signed-off-by: Daniil Tatianin
---
hw/virtio/virtio.c | 17 +
include/hw/virtio/virtio.h | 9 +
2 files changed, 26 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..8518382025 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3014,6 +3014,23 @@ size_t virtio_feature_get_config_size(const
VirtIOFeature *feature_sizes,
return config_size;
}
+size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
+ uint64_t host_features)
+{
+size_t config_size = params->min_size;
+const VirtIOFeature *feature_sizes = params->feature_sizes;
+size_t i;
+
+for (i = 0; feature_sizes[i].flags != 0; i++) {
+if (host_features & feature_sizes[i].flags) {
+config_size = MAX(feature_sizes[i].end, config_size);
+}
+}
+
+assert(config_size <= params->max_size);
+return config_size;
+}
+
int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
{
int i, ret;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..1991c58d9b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -44,6 +44,15 @@ typedef struct VirtIOFeature {
size_t end;
} VirtIOFeature;
+typedef struct VirtIOConfigSizeParams {
+size_t min_size;
+size_t max_size;
+const VirtIOFeature *feature_sizes;
+} VirtIOConfigSizeParams;
+
+size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
+ uint64_t host_features);
+
size_t virtio_feature_get_config_size(const VirtIOFeature *features,
uint64_t host_features);
--
2.25.1
Re: [PATCH v2 8/8] vhost-user-blk: dynamically resize config space based on features
On Fri, Aug 26, 2022 at 05:32:48PM +0300, Daniil Tatianin wrote:
> Make vhost-user-blk backwards compatible when migrating from older VMs
> running with modern features turned off, the same way it was done for
> virtio-blk in 20764be0421c ("virtio-blk: set config size depending on the
> features enabled")
>
> It's currently impossible to migrate from an older VM with
> vhost-user-blk (with disable-legacy=off) because of errors like this:
>
> qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: 41
> device: 1 cmask: ff wmask: 80 w1cmask:0
> qemu-system-x86_64: Failed to load PCIDevice:config
> qemu-system-x86_64: Failed to load virtio-blk:virtio
> qemu-system-x86_64: error while loading state for instance 0x0 of device
> ':00:05.0:00.0:02.0/virtio-blk'
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> This is caused by the newer (destination) VM requiring a bigger BAR0
> alignment because it has to cover a bigger configuration space, which
> isn't actually needed since those additional config fields are not
> active (write-zeroes/discard).
>
With the relevant meson.build and MAINTAINERS bits rebased here:
Reviewed-by: Raphael Norwitz
> Signed-off-by: Daniil Tatianin
> ---
> hw/block/vhost-user-blk.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 0d916edefa..8dd063eb96 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -23,6 +23,7 @@
> #include "hw/qdev-core.h"
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> +#include "hw/virtio/virtio-blk-common.h"
> #include "hw/virtio/vhost.h"
> #include "hw/virtio/vhost-user-blk.h"
> #include "hw/virtio/virtio.h"
> @@ -63,7 +64,7 @@ static void vhost_user_blk_update_config(VirtIODevice
> *vdev, uint8_t *config)
> /* Our num_queues overrides the device backend */
> virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
>
> -memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
> +memcpy(config, &s->blkcfg, vdev->config_len);
> }
>
> static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t
> *config)
> @@ -92,12 +93,12 @@ static int vhost_user_blk_handle_config_change(struct
> vhost_dev *dev)
> {
> int ret;
> struct virtio_blk_config blkcfg;
> +VirtIODevice *vdev = dev->vdev;
> VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
> Error *local_err = NULL;
>
> ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> - sizeof(struct virtio_blk_config),
> - &local_err);
> + vdev->config_len, &local_err);
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
> @@ -106,7 +107,7 @@ static int vhost_user_blk_handle_config_change(struct
> vhost_dev *dev)
> /* valid for resize only */
> if (blkcfg.capacity != s->blkcfg.capacity) {
> s->blkcfg.capacity = blkcfg.capacity;
> -memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct
> virtio_blk_config));
> +memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> virtio_notify_config(dev->vdev);
> }
>
> @@ -442,7 +443,7 @@ static int vhost_user_blk_realize_connect(VHostUserBlk
> *s, Error **errp)
> assert(s->connected);
>
> ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> - sizeof(struct virtio_blk_config), errp);
> + s->parent_obj.config_len, errp);
> if (ret < 0) {
> qemu_chr_fe_disconnect(&s->chardev);
> vhost_dev_cleanup(&s->dev);
> @@ -457,6 +458,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> ERRP_GUARD();
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +size_t config_size;
> int retries;
> int i, ret;
>
> @@ -487,8 +489,9 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> return;
> }
>
> -virtio_init(vdev, VIRTIO_ID_BLOCK,
> -sizeof(struct virtio_blk_config));
> +config_size = virtio_get_config_size(&virtio_blk_cfg_size_params,
> + vdev->host_features);
> +virtio_init(vdev, VIRTIO_ID_BLOCK, config_size);
>
> s->virtqs = g_new(VirtQueue *, s->num_queues);
> for (i = 0; i < s->num_queues; i++) {
> --
> 2.25.1
>
Re: [PATCH v2 4/8] virtio: remove the virtio_feature_get_config_size helper
On Fri, Aug 26, 2022 at 05:32:44PM +0300, Daniil Tatianin wrote:
> This has no more users and is superseded by virtio_get_config_size.
>
> Signed-off-by: Daniil Tatianin
Reviewed-by: Raphael Norwitz
> ---
> hw/virtio/virtio.c | 15 ---
> include/hw/virtio/virtio.h | 3 ---
> 2 files changed, 18 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8518382025..c0bf8dd6fd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2999,21 +2999,6 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t
> val)
> return ret;
> }
>
> -size_t virtio_feature_get_config_size(const VirtIOFeature *feature_sizes,
> - uint64_t host_features)
> -{
> -size_t config_size = 0;
> -int i;
> -
> -for (i = 0; feature_sizes[i].flags != 0; i++) {
> -if (host_features & feature_sizes[i].flags) {
> -config_size = MAX(feature_sizes[i].end, config_size);
> -}
> -}
> -
> -return config_size;
> -}
> -
> size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>uint64_t host_features)
> {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 1991c58d9b..f3ff6710d5 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -53,9 +53,6 @@ typedef struct VirtIOConfigSizeParams {
> size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
>uint64_t host_features);
>
> -size_t virtio_feature_get_config_size(const VirtIOFeature *features,
> - uint64_t host_features);
> -
> typedef struct VirtQueue VirtQueue;
>
> #define VIRTQUEUE_MAX_SIZE 1024
> --
> 2.25.1
>
Re: [PATCH v2 3/8] virtio-net: utilize VirtIOConfigSizeParams & virtio_get_config_size
On Fri, Aug 26, 2022 at 05:32:43PM +0300, Daniil Tatianin wrote:
> Use the new common helper. As an added bonus this also makes use of
> config size sanity checking via the 'max_size' field.
>
> Signed-off-by: Daniil Tatianin
> ---
> hw/net/virtio-net.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd0d056fde..dfc8dd8562 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -106,6 +106,11 @@ static const VirtIOFeature feature_sizes[] = {
> {}
> };
>
> +static const VirtIOConfigSizeParams cfg_size_params = {
Can we have a zero length virtio-net config size? The both the v1.0 and
v1.1 section 5.1.4 say “The mac address field always exists (though is
only valid if VIRTIO_NET_F_MAC is set)” so we should probably set
min_size to offset_of status?
Otherwise LGTM.
> +.max_size = sizeof(struct virtio_net_config),
> +.feature_sizes = feature_sizes
> +};
> +
> static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
> {
> VirtIONet *n = qemu_get_nic_opaque(nc);
> @@ -3246,8 +3251,7 @@ static void virtio_net_set_config_size(VirtIONet *n,
> uint64_t host_features)
> {
> virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
>
> -n->config_size = virtio_feature_get_config_size(feature_sizes,
> -host_features);
> +n->config_size = virtio_get_config_size(&cfg_size_params, host_features);
> }
>
> void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> --
> 2.25.1
>
Re: [PATCH v2 7/8] vhost-user-blk: make 'config_wce' part of 'host_features'
On Fri, Aug 26, 2022 at 05:32:47PM +0300, Daniil Tatianin wrote:
> No reason to have this be a separate field. This also makes it more akin
> to what the virtio-blk device does.
>
> Signed-off-by: Daniil Tatianin
Reviewed-by: Raphael Norwitz
> ---
> hw/block/vhost-user-blk.c | 6 ++
> include/hw/virtio/vhost-user-blk.h | 1 -
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 4c9727e08c..0d916edefa 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -260,9 +260,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
> virtio_add_feature(&features, VIRTIO_BLK_F_RO);
>
> -if (s->config_wce) {
> -virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> -}
> if (s->num_queues > 1) {
> virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
> }
> @@ -589,7 +586,8 @@ static Property vhost_user_blk_properties[] = {
> DEFINE_PROP_UINT16("num-queues", VHostUserBlk, num_queues,
> VHOST_USER_BLK_AUTO_NUM_QUEUES),
> DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
> -DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
> +DEFINE_PROP_BIT64("config-wce", VHostUserBlk, parent_obj.host_features,
> + VIRTIO_BLK_F_CONFIG_WCE, true),
> DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
>VIRTIO_BLK_F_DISCARD, true),
> DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
> diff --git a/include/hw/virtio/vhost-user-blk.h
> b/include/hw/virtio/vhost-user-blk.h
> index 7c91f15040..ea085ee1ed 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -34,7 +34,6 @@ struct VHostUserBlk {
> struct virtio_blk_config blkcfg;
> uint16_t num_queues;
> uint32_t queue_size;
> -uint32_t config_wce;
> struct vhost_dev dev;
> struct vhost_inflight *inflight;
> VhostUserState vhost_user;
> --
> 2.25.1
>
Re: [PATCH v2 6/8] vhost-user-blk: make it possible to disable write-zeroes/discard
On Fri, Aug 26, 2022 at 05:32:46PM +0300, Daniil Tatianin wrote:
> It is useful to have the ability to disable these features for
> compatibility with older VMs that don't have these implemented.
>
> Signed-off-by: Daniil Tatianin
Reviewed-by: Raphael Norwitz
> ---
> hw/block/vhost-user-blk.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 9117222456..4c9727e08c 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -259,8 +259,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> virtio_add_feature(&features, VIRTIO_BLK_F_FLUSH);
> virtio_add_feature(&features, VIRTIO_BLK_F_RO);
> -virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
> -virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
>
> if (s->config_wce) {
> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> @@ -592,6 +590,10 @@ static Property vhost_user_blk_properties[] = {
> VHOST_USER_BLK_AUTO_NUM_QUEUES),
> DEFINE_PROP_UINT32("queue-size", VHostUserBlk, queue_size, 128),
> DEFINE_PROP_BIT("config-wce", VHostUserBlk, config_wce, 0, true),
> +DEFINE_PROP_BIT64("discard", VHostUserBlk, parent_obj.host_features,
> + VIRTIO_BLK_F_DISCARD, true),
> +DEFINE_PROP_BIT64("write-zeroes", VHostUserBlk, parent_obj.host_features,
> + VIRTIO_BLK_F_WRITE_ZEROES, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> --
> 2.25.1
>
Re: [PATCH v2 5/8] virtio-blk: move config size params to virtio-blk-common
The vhost-user-blk bits in meson.build and Maintainers should probably
be in patch 8?
Otherwise LGTM.
On Fri, Aug 26, 2022 at 05:32:45PM +0300, Daniil Tatianin wrote:
> This way we can reuse it for other virtio-blk devices, e.g
> vhost-user-blk, which currently does not control its config space size
> dynamically.
>
> Signed-off-by: Daniil Tatianin
> ---
> MAINTAINERS | 4 +++
> hw/block/meson.build | 4 +--
> hw/block/virtio-blk-common.c | 39 +++
> hw/block/virtio-blk.c | 24 ++---
> include/hw/virtio/virtio-blk-common.h | 20 ++
> 5 files changed, 67 insertions(+), 24 deletions(-)
> create mode 100644 hw/block/virtio-blk-common.c
> create mode 100644 include/hw/virtio/virtio-blk-common.h
>
i.e. move this.
> @@ -2271,11 +2273,13 @@ S: Maintained
> F: contrib/vhost-user-blk/
> F: contrib/vhost-user-scsi/
> F: hw/block/vhost-user-blk.c
> +F: hw/block/virtio-blk-common.c
> F: hw/scsi/vhost-user-scsi.c
> F: hw/virtio/vhost-user-blk-pci.c
> F: hw/virtio/vhost-user-scsi-pci.c
> F: include/hw/virtio/vhost-user-blk.h
> F: include/hw/virtio/vhost-user-scsi.h
> +F: include/hw/virtio/virtio-blk-common.h
>
> vhost-user-gpu
> M: Marc-André Lureau
> diff --git a/hw/block/meson.build b/hw/block/meson.build
> index 2389326112..1908abd45c 100644
> --- a/hw/block/meson.build
> +++ b/hw/block/meson.build
> @@ -16,7 +16,7 @@ softmmu_ss.add(when: 'CONFIG_SWIM', if_true:
> files('swim.c'))
> softmmu_ss.add(when: 'CONFIG_XEN', if_true: files('xen-block.c'))
> softmmu_ss.add(when: 'CONFIG_TC58128', if_true: files('tc58128.c'))
>
> -specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c'))
> -specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
> files('vhost-user-blk.c'))
> +specific_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio-blk.c',
> 'virtio-blk-common.c'))
And this
> +specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true:
> files('vhost-user-blk.c', 'virtio-blk-common.c'))
Re: [PATCH v2 2/8] virtio-blk: utilize VirtIOConfigSizeParams & virtio_get_config_size
On Fri, Aug 26, 2022 at 05:32:42PM +0300, Daniil Tatianin wrote:
> Use the common helper instead of duplicating the same logic.
>
> Signed-off-by: Daniil Tatianin
Reviewed-by: Raphael Norwitz
> ---
> hw/block/virtio-blk.c | 16 +++-
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e9ba752f6b..10c47c2934 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -49,13 +49,11 @@ static const VirtIOFeature feature_sizes[] = {
> {}
> };
>
> -static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t
> host_features)
> -{
> -s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
> -virtio_feature_get_config_size(feature_sizes, host_features));
> -
> -assert(s->config_size <= sizeof(struct virtio_blk_config));
> -}
> +static const VirtIOConfigSizeParams cfg_size_params = {
> +.min_size = VIRTIO_BLK_CFG_SIZE,
> +.max_size = sizeof(struct virtio_blk_config),
> +.feature_sizes = feature_sizes
> +};
>
> static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
> VirtIOBlockReq *req)
> @@ -1204,8 +1202,8 @@ static void virtio_blk_device_realize(DeviceState *dev,
> Error **errp)
> return;
> }
>
> -virtio_blk_set_config_size(s, s->host_features);
> -
> +s->config_size = virtio_get_config_size(&cfg_size_params,
> +s->host_features);
> virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
>
> s->blk = conf->conf.blk;
> --
> 2.25.1
>
Re: [PATCH v2 1/8] virtio: introduce VirtIOConfigSizeParams & virtio_get_config_size
I feel like it would be easier to review if the first 4 patches were
squashed together, but that’s not a big deal.
For this one:
Reviewed-by: Raphael Norwitz
On Fri, Aug 26, 2022 at 05:32:41PM +0300, Daniil Tatianin wrote:
> This is the first step towards moving all device config size calculation
> logic into the virtio core code. In particular, this adds a struct that
> contains all the necessary information for common virtio code to be able
> to calculate the final config size for a device. This is expected to be
> used with the new virtio_get_config_size helper, which calculates the
> final length based on the provided host features.
>
> This builds on top of already existing code like VirtIOFeature and
> virtio_feature_get_config_size(), but adds additional fields, as well as
> sanity checking so that device-specifc code doesn't have to duplicate it.
>
> An example usage would be:
>
> static const VirtIOFeature dev_features[] = {
> {.flags = 1ULL << FEATURE_1_BIT,
> .end = endof(struct virtio_dev_config, feature_1)},
> {.flags = 1ULL << FEATURE_2_BIT,
> .end = endof(struct virtio_dev_config, feature_2)},
> {}
> };
>
> static const VirtIOConfigSizeParams dev_cfg_size_params = {
> .min_size = DEV_BASE_CONFIG_SIZE,
> .max_size = sizeof(struct virtio_dev_config),
> .feature_sizes = dev_features
> };
>
> // code inside my_dev_device_realize()
> size_t config_size = virtio_get_config_size(&dev_cfg_size_params,
> host_features);
> virtio_init(vdev, VIRTIO_ID_MYDEV, config_size);
>
> Currently every device is expected to write its own boilerplate from the
> example above in device_realize(), however, the next step of this
> transition is moving VirtIOConfigSizeParams into VirtioDeviceClass,
> so that it can be done automatically by the virtio initialization code.
>
> Signed-off-by: Daniil Tatianin
> ---
> hw/virtio/virtio.c | 17 +
> include/hw/virtio/virtio.h | 9 +
> 2 files changed, 26 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5d607aeaa0..8518382025 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3014,6 +3014,23 @@ size_t virtio_feature_get_config_size(const
> VirtIOFeature *feature_sizes,
> return config_size;
> }
>
> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
> + uint64_t host_features)
> +{
> +size_t config_size = params->min_size;
> +const VirtIOFeature *feature_sizes = params->feature_sizes;
> +size_t i;
> +
> +for (i = 0; feature_sizes[i].flags != 0; i++) {
> +if (host_features & feature_sizes[i].flags) {
> +config_size = MAX(feature_sizes[i].end, config_size);
> +}
> +}
> +
> +assert(config_size <= params->max_size);
> +return config_size;
> +}
> +
> int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> {
> int i, ret;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index db1c0ddf6b..1991c58d9b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -44,6 +44,15 @@ typedef struct VirtIOFeature {
> size_t end;
> } VirtIOFeature;
>
> +typedef struct VirtIOConfigSizeParams {
> +size_t min_size;
> +size_t max_size;
> +const VirtIOFeature *feature_sizes;
> +} VirtIOConfigSizeParams;
> +
> +size_t virtio_get_config_size(const VirtIOConfigSizeParams *params,
> + uint64_t host_features);
> +
> size_t virtio_feature_get_config_size(const VirtIOFeature *features,
>uint64_t host_features);
>
> --
> 2.25.1
>
Re: [PATCH 0/8] tests: Make expliction defaults for tests
On 220902 1851, Juan Quintela wrote: > Hi > > For a long, long time I have had local hacks on my tree to be able to > run "make tests" when I have a minimal configure guest. This is a > first try to upstream some of it. > > - by default we always setup -display none (it already was the > default, but some places added it anyways) > > - by default we always setup -net none. Not clear what was the > default, but no tests use the default net, so it is safe change and > now it is explicit. > > - by default we always setup -vga none. This is a complete difference > can of worms. Every tests that use vga already set vga correctly, > so this is quite obvious, right? Now they are acpi tables. They > are a mess. And basically this means remove a device for each one > of them. Why going through all the trouble? Because while I am > develping, I normall compile out vga. > > - Fix several error strings that were set with copy paste. > > - replication test requires CONFIG_REPLICATION. > - test-crypto-secret requires CONFIG_SECRET_KEYRING. > > Please review. Except for the acpi changes (that I hope I have done > right following the instructions) the rest is quite obvious. I think this might break some of the fuzz regression tests, because they have "baked-in" PCI configuration commands with hard-coded PCI addresses, which will shift around if some device is removed (e.g. with -net none). Probably the fix is to add addr=... to the -device parameter in the fuzz tests to keep the PCI address stable. -Alex > > Later, Juan. > > Juan Quintela (8): > qtest: "-display none" is set in qtest_init() > qtest: Set "-net none" in qtest_init() > tests/acpi: The new default is -vga none > tests/qtest: Add -vga none by default > tests/acpi: Regenerate all needed tables > tests: Fix error strings > meson-build: Enable CONFIG_REPLICATION only when replication is set > meson-build: test-crypto-secret depends on CONFIG_SECRET_KEYRING > > meson.build | 2 +- > tests/qtest/bios-tables-test.c| 2 +- > tests/qtest/boot-serial-test.c| 4 ++-- > tests/qtest/dbus-display-test.c | 2 +- > tests/qtest/display-vga-test.c| 12 ++-- > tests/qtest/e1000-test.c | 2 +- > tests/qtest/es1370-test.c | 2 +- > tests/qtest/fuzz-lsi53c895a-test.c| 2 +- > tests/qtest/fuzz-megasas-test.c | 2 +- > tests/qtest/fuzz-sb16-test.c | 6 +++--- > tests/qtest/fuzz-sdcard-test.c| 6 +++--- > tests/qtest/fuzz-virtio-scsi-test.c | 2 +- > tests/qtest/fuzz-xlnx-dp-test.c | 2 +- > tests/qtest/fuzz/generic_fuzz.c | 3 +-- > tests/qtest/fuzz/i440fx_fuzz.c| 2 +- > tests/qtest/fuzz/qos_fuzz.c | 2 +- > tests/qtest/libqtest.c| 2 ++ > tests/data/acpi/pc/DSDT | Bin 5987 -> 5992 bytes > tests/data/acpi/pc/DSDT.acpierst | Bin 5954 -> 5959 bytes > tests/data/acpi/pc/DSDT.acpihmat | Bin 7312 -> 7317 bytes > tests/data/acpi/pc/DSDT.bridge| Bin 8653 -> 8658 bytes > tests/data/acpi/pc/DSDT.cphp | Bin 6451 -> 6456 bytes > tests/data/acpi/pc/DSDT.dimmpxm | Bin 7641 -> 7646 bytes > tests/data/acpi/pc/DSDT.hpbridge | Bin 5954 -> 5959 bytes > tests/data/acpi/pc/DSDT.hpbrroot | Bin 3069 -> 3023 bytes > tests/data/acpi/pc/DSDT.ipmikcs | Bin 6059 -> 6064 bytes > tests/data/acpi/pc/DSDT.memhp | Bin 7346 -> 7351 bytes > tests/data/acpi/pc/DSDT.nohpet| Bin 5845 -> 5850 bytes > tests/data/acpi/pc/DSDT.numamem | Bin 5993 -> 5998 bytes > tests/data/acpi/pc/DSDT.roothp| Bin 6195 -> 6151 bytes > tests/data/acpi/pc/ERST.acpierst | Bin 912 -> 912 bytes > tests/data/acpi/q35/DMAR.dmar | Bin 120 -> 112 bytes > tests/data/acpi/q35/DSDT | Bin 8274 -> 8228 bytes > tests/data/acpi/q35/DSDT.acpierst | Bin 8291 -> 8245 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9599 -> 9553 bytes > tests/data/acpi/q35/DSDT.applesmc | Bin 8320 -> 8274 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 10988 -> 10944 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8738 -> 8692 bytes > tests/data/acpi/q35/DSDT.cxl | Bin 9600 -> 9502 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9928 -> 9882 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 8349 -> 8303 bytes > tests/data/acpi/q35/DSDT.ipmismbus| Bin 8363 -> 8317 bytes > tests/data/acpi/q35/DSDT.ivrs | Bin 8291 -> 8245 bytes > tests/data/acpi/q35/DSDT.memhp| Bin 9633 -> 9587 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 9404 -> 9358 bytes > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8568 -> 8524 bytes > tests/data/acpi/q35/DSDT.nohpet | Bin 8132 -> 8086 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 8280 -> 8234 bytes > tests/data/acpi/q35/DSDT.pvpanic-isa | Bin 8375 -> 8329 bytes > tests/data/acpi/q35/DSDT.t
[PATCH 7/8] meson-build: Enable CONFIG_REPLICATION only when replication is set
Signed-off-by: Juan Quintela
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 20fddbd707..cab0474d0c 100644
--- a/meson.build
+++ b/meson.build
@@ -1878,7 +1878,7 @@ config_host_data.set('CONFIG_DEBUG_STACK_USAGE',
get_option('debug_stack_usage')
config_host_data.set('CONFIG_GPROF', get_option('gprof'))
config_host_data.set('CONFIG_LIVE_BLOCK_MIGRATION',
get_option('live_block_migration').allowed())
config_host_data.set('CONFIG_QOM_CAST_DEBUG', get_option('qom_cast_debug'))
-config_host_data.set('CONFIG_REPLICATION',
get_option('live_block_migration').allowed())
+config_host_data.set('CONFIG_REPLICATION', get_option('replication').allowed())
# has_header
config_host_data.set('CONFIG_EPOLL', cc.has_header('sys/epoll.h'))
--
2.37.2
[PATCH 8/8] meson-build: test-crypto-secret depends on CONFIG_SECRET_KEYRING
With this change "make check" works when configured with --disable-keyring.
Signed-off-by: Juan Quintela
---
tests/unit/meson.build | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index b497a41378..988aed27cb 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -78,7 +78,6 @@ if have_block
'test-crypto-hmac': [crypto],
'test-crypto-cipher': [crypto],
'test-crypto-akcipher': [crypto],
-'test-crypto-secret': [crypto, keyutils],
'test-crypto-der': [crypto],
'test-authz-simple': [authz],
'test-authz-list': [authz],
@@ -122,6 +121,9 @@ if have_block
if config_host_data.get('CONFIG_EPOLL_CREATE1')
tests += {'test-fdmon-epoll': [testblock]}
endif
+ if config_host_data.get('CONFIG_SECRET_KEYRING')
+tests += {'test-crypto-secret': [crypto, keyutils]}
+ endif
endif
if have_system
--
2.37.2
[PATCH 6/8] tests: Fix error strings
They were copy-pasted from e1000e and never changed. Signed-off-by: Juan Quintela --- tests/qtest/e1000-test.c | 2 +- tests/qtest/es1370-test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qtest/e1000-test.c b/tests/qtest/e1000-test.c index c387984ef6..4e0d7a5607 100644 --- a/tests/qtest/e1000-test.c +++ b/tests/qtest/e1000-test.c @@ -35,7 +35,7 @@ static void *e1000_get_driver(void *obj, const char *interface) return &e1000->dev; } -fprintf(stderr, "%s not present in e1000e\n", interface); +fprintf(stderr, "%s not present in e1000\n", interface); g_assert_not_reached(); } diff --git a/tests/qtest/es1370-test.c b/tests/qtest/es1370-test.c index adccdac1be..97ab65c435 100644 --- a/tests/qtest/es1370-test.c +++ b/tests/qtest/es1370-test.c @@ -28,7 +28,7 @@ static void *es1370_get_driver(void *obj, const char *interface) return &es1370->dev; } -fprintf(stderr, "%s not present in e1000e\n", interface); +fprintf(stderr, "%s not present in es1370\n", interface); g_assert_not_reached(); } -- 2.37.2
[PATCH 4/8] tests/qtest: Add -vga none by default
Nothing in qtest where using vga by default except for
dbus-display-test. We add -device VGA there.
The only other test that fails after this change is:
109/659 qemu:qtest+qtest-ppc64 / qtest-ppc64/qos-test ERROR
21.34s killed by signal 6 SIGABRT
Bail out!
ERROR:../../../../mnt/code/qemu/full/tests/qtest/virtio-iommu-test.c:189:test_attach_detach:
assertion failed (ret == 0): (6 == 0)
stderr:
qemu-system-ppc64: warning: nic e1000.0 has no peer
qemu-system-ppc64: warning: nic e1000-82544gc.0 has no peer
qemu-system-ppc64: warning: nic e1000-82545em.0 has no peer
qemu-system-ppc64: warning: nic i82550.0 has no peer
qemu-system-ppc64: warning: nic i82551.0 has no peer
qemu-system-ppc64: warning: nic i82557a.0 has no peer
qemu-system-ppc64: warning: nic i82557b.0 has no peer
qemu-system-ppc64: warning: nic i82557c.0 has no peer
qemu-system-ppc64: warning: nic i82558a.0 has no peer
qemu-system-ppc64: warning: nic i82558b.0 has no peer
qemu-system-ppc64: warning: nic i82559a.0 has no peer
qemu-system-ppc64: warning: nic i82559b.0 has no peer
qemu-system-ppc64: warning: nic i82559c.0 has no peer
qemu-system-ppc64: warning: nic i82559er.0 has no peer
qemu-system-ppc64: warning: nic i82562.0 has no peer
qemu-system-ppc64: warning: nic i82801.0 has no peer
qemu-system-ppc64: warning: nic ne2k_pci.0 has no peer
qemu-system-ppc64: warning: nic tulip.0 has no peer
qemu-system-ppc64: warning: nic pcnet.0 has no peer
qemu-system-ppc64: warning: nic vmxnet3.0 has no peer
**
ERROR:../../../../mnt/code/qemu/full/tests/qtest/virtio-iommu-test.c:189:test_attach_detach:
assertion failed (ret == 0): (6 == 0)
(test program exited with status code -6)
--
Problem is with virtio-iommu-test.c attach_detach test. My
understanding is that the problem is that for pseries.
$ export QTEST_QEMU_BINARY=./qemu-system-ppc64
$ ./tests/qtest/qos-test -p
/ppc64/pseries/spapr-pci-host-bridge/pci-bus-spapr/pci-bus/virtio-iommu-pci/virtio-iommu/virtio-iommu-tests/attach_detach
**
ERROR:../../../../mnt/code/qemu/full/tests/qtest/virtio-iommu-test.c:189:test_attach_detach:
assertion failed (ret == 0): (6 == 0)
Bail out!
ERROR:../../../../mnt/code/qemu/full/tests/qtest/virtio-iommu-test.c:189:test_attach_detach:
assertion failed (ret == 0): (6 == 0)
Aborted (core dumped)
My understanding is that if I remove the vga device, pseries end
without any device on the pci bus and somehow it completely breaks.
Signed-off-by: Juan Quintela
---
tests/qtest/boot-serial-test.c | 4 ++--
tests/qtest/dbus-display-test.c | 2 +-
tests/qtest/display-vga-test.c | 12 ++--
tests/qtest/libqtest.c | 1 +
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 2f99d71cab..500a312cb5 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -144,7 +144,7 @@ static testdef_t tests[] = {
{ "avr", "arduino-duemilanove", "", "T", sizeof(bios_avr), NULL, bios_avr
},
{ "avr", "arduino-mega-2560-v3", "", "T", sizeof(bios_avr), NULL,
bios_avr},
{ "ppc", "ppce500", "", "U-Boot" },
-{ "ppc", "40p", "-vga none -boot d", "Trying cd:," },
+{ "ppc", "40p", "-boot d", "Trying cd:," },
{ "ppc", "g3beige", "", "PowerPC,750" },
{ "ppc", "mac99", "", "PowerPC,G4" },
{ "ppc", "sam460ex", "-m 256", "DRAM: 256 MiB" },
@@ -175,7 +175,7 @@ static testdef_t tests[] = {
sizeof(kernel_plml605), kernel_plml605 },
{ "arm", "raspi2b", "", "TT", sizeof(bios_raspi2), 0, bios_raspi2 },
/* For hppa, force bios to output to serial by disabling graphics. */
-{ "hppa", "hppa", "-vga none", "SeaBIOS wants SYSTEM HALT" },
+{ "hppa", "hppa", "", "SeaBIOS wants SYSTEM HALT" },
{ "aarch64", "virt", "-cpu max", "TT", sizeof(kernel_aarch64),
kernel_aarch64 },
{ "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
diff --git a/tests/qtest/dbus-display-test.c b/tests/qtest/dbus-display-test.c
index 8be5974763..4e428cff55 100644
--- a/tests/qtest/dbus-display-test.c
+++ b/tests/qtest/dbus-display-test.c
@@ -34,7 +34,7 @@ test_setup(QTestState **qts, GDBusConnection **conn)
{
int pair[2];
-*qts = qtest_init("-display dbus,p2p=yes -name dbus-test");
+*qts = qtest_init("-display dbus,p2p=yes -device VGA -name dbus-test");
g_assert_cmpint(socketpair(AF_UNIX, SOCK_STREAM, 0, pair), ==, 0);
diff --git a/tests/qtest/display-vga-test.c b/tests/qtest/display-vga-test.c
index ace3bb28e0..ba3d1fcb36 100644
--- a/tests/qtest/display-vga-test.c
+++ b/tests/qtest/display-vga-test.c
@@ -12,37 +12,37 @@
static void pci_cirrus(void)
{
-qtest_start("-vga none -device cirrus-vga");
+qtest_start("-device cirrus-vga");
qtest_end();
}
static void pci_stdvga(void)
{
-qtest_start("-vga none -device VGA");
+qtest_start("-device VGA");
qtest_end();
}
static void pci_secondary(void)
{
-qtest_start("-vg
[PATCH 2/8] qtest: Set "-net none" in qtest_init()
This way, we don't have networking by default. If test needs it,
configure it.
Signed-off-by: Juan Quintela
---
tests/qtest/bios-tables-test.c | 2 +-
tests/qtest/libqtest.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 9c68f3658a..a0853744ae 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -725,7 +725,7 @@ static char *test_acpi_create_args(test_data *data, const
char *params,
}
} else {
args = g_strdup_printf("-machine %s %s -accel tcg "
-"-net none %s "
+"%s "
"-drive id=hd0,if=none,file=%s,format=raw "
"-device %s,drive=hd0 ",
data->machine, data->tcg_only ? "" : "-accel kvm",
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 7c9fc07de4..ee84dbfc36 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -290,6 +290,7 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
"-chardev socket,path=%s,id=char0 "
"-mon chardev=char0,mode=control "
"-display none "
+ "-net none "
"%s"
" -accel qtest",
qemu_binary, tracearg, socket_path,
--
2.37.2
[PATCH 3/8] tests/acpi: The new default is -vga none
So we lost a device in each table. Signed-off-by: Juan Quintela --- tests/qtest/bios-tables-test-allowed-diff.h | 39 + 1 file changed, 39 insertions(+) diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index dfb8523c8b..37369aed63 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1 +1,40 @@ /* List of comma-separated changed AML files to ignore */ +"tests/data/acpi/pc/DSDT", +"tests/data/acpi/q35/DSDT", +"tests/data/acpi/q35/DSDT.tis.tpm2", +"tests/data/acpi/q35/DSDT.tis.tpm12", +"tests/data/acpi/q35/DSDT.bridge", +"tests/data/acpi/q35/DSDT.multi-bridge", +"tests/data/acpi/q35/DSDT.mmio64", +"tests/data/acpi/q35/DSDT.ipmibt", +"tests/data/acpi/q35/DSDT.cphp", +"tests/data/acpi/q35/DSDT.memhp", +"tests/data/acpi/q35/DSDT.numamem", +"tests/data/acpi/q35/DSDT.nohpet", +"tests/data/acpi/q35/DSDT.dimmpxm", +"tests/data/acpi/q35/DSDT.acpihmat", +"tests/data/acpi/q35/ERST.acpierst", +"tests/data/acpi/q35/DSDT.acpierst", +"tests/data/acpi/q35/DSDT.applesmc", +"tests/data/acpi/q35/DSDT.pvpanic-isa", +"tests/data/acpi/q35/IVRS.ivrs", +"tests/data/acpi/q35/DSDT.ivrs", +"tests/data/acpi/q35/VIOT.viot", +"tests/data/acpi/q35/DSDT.viot", +"tests/data/acpi/q35/DSDT.cxl", +"tests/data/acpi/q35/DSDT.ipmismbus", +"tests/data/acpi/q35/DSDT.xapic", +"tests/data/acpi/q35/DMAR.dmar", +"tests/data/acpi/pc/DSDT.bridge", +"tests/data/acpi/pc/DSDT.ipmikcs", +"tests/data/acpi/pc/DSDT.cphp", +"tests/data/acpi/pc/DSDT.memhp", +"tests/data/acpi/pc/DSDT.numamem", +"tests/data/acpi/pc/DSDT.nohpet", +"tests/data/acpi/pc/DSDT.dimmpxm", +"tests/data/acpi/pc/DSDT.acpihmat", +"tests/data/acpi/pc/ERST.acpierst", +"tests/data/acpi/pc/DSDT.acpierst", +"tests/data/acpi/pc/DSDT.roothp", +"tests/data/acpi/pc/DSDT.hpbridge", +"tests/data/acpi/pc/DSDT.hpbrroot", -- 2.37.2
[PATCH 1/8] qtest: "-display none" is set in qtest_init()
So we don't need to set anywhere else.
Signed-off-by: Juan Quintela
---
tests/qtest/bios-tables-test.c | 2 +-
tests/qtest/fuzz-lsi53c895a-test.c | 2 +-
tests/qtest/fuzz-megasas-test.c | 2 +-
tests/qtest/fuzz-sb16-test.c| 6 +++---
tests/qtest/fuzz-sdcard-test.c | 6 +++---
tests/qtest/fuzz-virtio-scsi-test.c | 2 +-
tests/qtest/fuzz-xlnx-dp-test.c | 2 +-
tests/qtest/fuzz/generic_fuzz.c | 3 +--
tests/qtest/fuzz/i440fx_fuzz.c | 2 +-
tests/qtest/fuzz/qos_fuzz.c | 2 +-
10 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 36783966b0..9c68f3658a 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -725,7 +725,7 @@ static char *test_acpi_create_args(test_data *data, const
char *params,
}
} else {
args = g_strdup_printf("-machine %s %s -accel tcg "
-"-net none -display none %s "
+"-net none %s "
"-drive id=hd0,if=none,file=%s,format=raw "
"-device %s,drive=hd0 ",
data->machine, data->tcg_only ? "" : "-accel kvm",
diff --git a/tests/qtest/fuzz-lsi53c895a-test.c
b/tests/qtest/fuzz-lsi53c895a-test.c
index b23d3ecf45..137e0bb79b 100644
--- a/tests/qtest/fuzz-lsi53c895a-test.c
+++ b/tests/qtest/fuzz-lsi53c895a-test.c
@@ -21,7 +21,7 @@ static void test_lsi_do_msgout_cancel_req(void)
return;
}
-s = qtest_init("-M q35 -m 4G -display none -nodefaults "
+s = qtest_init("-M q35 -m 4G -nodefaults "
"-device lsi53c895a,id=scsi "
"-device scsi-hd,drive=disk0 "
"-drive file=null-co://,id=disk0,if=none,format=raw");
diff --git a/tests/qtest/fuzz-megasas-test.c b/tests/qtest/fuzz-megasas-test.c
index 287fe19fc7..8d7ed3723a 100644
--- a/tests/qtest/fuzz-megasas-test.c
+++ b/tests/qtest/fuzz-megasas-test.c
@@ -40,7 +40,7 @@ static void test_lp1878263_megasas_zero_iov_cnt(void)
*/
static void test_gitlab_issue521_megasas_sgl_ovf(void)
{
-QTestState *s = qtest_init("-display none -m 32M -machine q35 "
+QTestState *s = qtest_init("-m 32M -machine q35 "
"-nodefaults -device megasas "
"-device scsi-cd,drive=null0 "
"-blockdev "
diff --git a/tests/qtest/fuzz-sb16-test.c b/tests/qtest/fuzz-sb16-test.c
index a65826b943..dd1b4b307f 100644
--- a/tests/qtest/fuzz-sb16-test.c
+++ b/tests/qtest/fuzz-sb16-test.c
@@ -15,7 +15,7 @@
*/
static void test_fuzz_sb16_0x1c(void)
{
-QTestState *s = qtest_init("-M q35 -display none "
+QTestState *s = qtest_init("-M q35 "
"-device sb16,audiodev=snd0 "
"-audiodev none,id=snd0");
qtest_outw(s, 0x22c, 0x41);
@@ -27,7 +27,7 @@ static void test_fuzz_sb16_0x1c(void)
static void test_fuzz_sb16_0x91(void)
{
-QTestState *s = qtest_init("-M pc -display none "
+QTestState *s = qtest_init("-M pc "
"-device sb16,audiodev=none "
"-audiodev id=none,driver=none");
qtest_outw(s, 0x22c, 0xf141);
@@ -43,7 +43,7 @@ static void test_fuzz_sb16_0x91(void)
*/
static void test_fuzz_sb16_0xd4(void)
{
-QTestState *s = qtest_init("-M pc -display none "
+QTestState *s = qtest_init("-M pc "
"-device sb16,audiodev=none "
"-audiodev id=none,driver=none");
qtest_outb(s, 0x22c, 0x41);
diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index e7fd818148..cd134cdf55 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -18,7 +18,7 @@ static void oss_fuzz_29225(void)
{
QTestState *s;
-s = qtest_init(" -display none -m 512m -nodefaults -nographic"
+s = qtest_init(" -m 512m -nodefaults -nographic"
" -device sdhci-pci,sd-spec-version=3"
" -device sd-card,drive=d0"
" -drive if=none,index=0,file=null-co://,format=raw,id=d0");
@@ -61,7 +61,7 @@ static void oss_fuzz_36217(void)
{
QTestState *s;
-s = qtest_init(" -display none -m 32 -nodefaults -nographic"
+s = qtest_init(" -m 32 -nodefaults -nographic"
" -device sdhci-pci,sd-spec-version=3 "
"-device sd-card,drive=d0 "
"-drive if=none,index=0,file=null-co://,format=raw,id=d0");
@@ -95,7 +95,7 @@ static void oss_fuzz_36391(void)
{
QTestState *s;
-s = qtest_init(" -display none -m 512M -nodefaults -nographic"
+s = qtest_init(" -m 512M -nodefaults -nographic"
" -device sdhci-pci,sd-spec-version=3"
" -device sd-card,drive=drv"
" -drive
if=none,index=0,file=null-co://,format=raw,id=drv");
diff --git a/tests/qtest/fuzz-virtio-scsi-test.c
b/tests/qte
[PATCH 0/8] tests: Make expliction defaults for tests
Hi For a long, long time I have had local hacks on my tree to be able to run "make tests" when I have a minimal configure guest. This is a first try to upstream some of it. - by default we always setup -display none (it already was the default, but some places added it anyways) - by default we always setup -net none. Not clear what was the default, but no tests use the default net, so it is safe change and now it is explicit. - by default we always setup -vga none. This is a complete difference can of worms. Every tests that use vga already set vga correctly, so this is quite obvious, right? Now they are acpi tables. They are a mess. And basically this means remove a device for each one of them. Why going through all the trouble? Because while I am develping, I normall compile out vga. - Fix several error strings that were set with copy paste. - replication test requires CONFIG_REPLICATION. - test-crypto-secret requires CONFIG_SECRET_KEYRING. Please review. Except for the acpi changes (that I hope I have done right following the instructions) the rest is quite obvious. Later, Juan. Juan Quintela (8): qtest: "-display none" is set in qtest_init() qtest: Set "-net none" in qtest_init() tests/acpi: The new default is -vga none tests/qtest: Add -vga none by default tests/acpi: Regenerate all needed tables tests: Fix error strings meson-build: Enable CONFIG_REPLICATION only when replication is set meson-build: test-crypto-secret depends on CONFIG_SECRET_KEYRING meson.build | 2 +- tests/qtest/bios-tables-test.c| 2 +- tests/qtest/boot-serial-test.c| 4 ++-- tests/qtest/dbus-display-test.c | 2 +- tests/qtest/display-vga-test.c| 12 ++-- tests/qtest/e1000-test.c | 2 +- tests/qtest/es1370-test.c | 2 +- tests/qtest/fuzz-lsi53c895a-test.c| 2 +- tests/qtest/fuzz-megasas-test.c | 2 +- tests/qtest/fuzz-sb16-test.c | 6 +++--- tests/qtest/fuzz-sdcard-test.c| 6 +++--- tests/qtest/fuzz-virtio-scsi-test.c | 2 +- tests/qtest/fuzz-xlnx-dp-test.c | 2 +- tests/qtest/fuzz/generic_fuzz.c | 3 +-- tests/qtest/fuzz/i440fx_fuzz.c| 2 +- tests/qtest/fuzz/qos_fuzz.c | 2 +- tests/qtest/libqtest.c| 2 ++ tests/data/acpi/pc/DSDT | Bin 5987 -> 5992 bytes tests/data/acpi/pc/DSDT.acpierst | Bin 5954 -> 5959 bytes tests/data/acpi/pc/DSDT.acpihmat | Bin 7312 -> 7317 bytes tests/data/acpi/pc/DSDT.bridge| Bin 8653 -> 8658 bytes tests/data/acpi/pc/DSDT.cphp | Bin 6451 -> 6456 bytes tests/data/acpi/pc/DSDT.dimmpxm | Bin 7641 -> 7646 bytes tests/data/acpi/pc/DSDT.hpbridge | Bin 5954 -> 5959 bytes tests/data/acpi/pc/DSDT.hpbrroot | Bin 3069 -> 3023 bytes tests/data/acpi/pc/DSDT.ipmikcs | Bin 6059 -> 6064 bytes tests/data/acpi/pc/DSDT.memhp | Bin 7346 -> 7351 bytes tests/data/acpi/pc/DSDT.nohpet| Bin 5845 -> 5850 bytes tests/data/acpi/pc/DSDT.numamem | Bin 5993 -> 5998 bytes tests/data/acpi/pc/DSDT.roothp| Bin 6195 -> 6151 bytes tests/data/acpi/pc/ERST.acpierst | Bin 912 -> 912 bytes tests/data/acpi/q35/DMAR.dmar | Bin 120 -> 112 bytes tests/data/acpi/q35/DSDT | Bin 8274 -> 8228 bytes tests/data/acpi/q35/DSDT.acpierst | Bin 8291 -> 8245 bytes tests/data/acpi/q35/DSDT.acpihmat | Bin 9599 -> 9553 bytes tests/data/acpi/q35/DSDT.applesmc | Bin 8320 -> 8274 bytes tests/data/acpi/q35/DSDT.bridge | Bin 10988 -> 10944 bytes tests/data/acpi/q35/DSDT.cphp | Bin 8738 -> 8692 bytes tests/data/acpi/q35/DSDT.cxl | Bin 9600 -> 9502 bytes tests/data/acpi/q35/DSDT.dimmpxm | Bin 9928 -> 9882 bytes tests/data/acpi/q35/DSDT.ipmibt | Bin 8349 -> 8303 bytes tests/data/acpi/q35/DSDT.ipmismbus| Bin 8363 -> 8317 bytes tests/data/acpi/q35/DSDT.ivrs | Bin 8291 -> 8245 bytes tests/data/acpi/q35/DSDT.memhp| Bin 9633 -> 9587 bytes tests/data/acpi/q35/DSDT.mmio64 | Bin 9404 -> 9358 bytes tests/data/acpi/q35/DSDT.multi-bridge | Bin 8568 -> 8524 bytes tests/data/acpi/q35/DSDT.nohpet | Bin 8132 -> 8086 bytes tests/data/acpi/q35/DSDT.numamem | Bin 8280 -> 8234 bytes tests/data/acpi/q35/DSDT.pvpanic-isa | Bin 8375 -> 8329 bytes tests/data/acpi/q35/DSDT.tis.tpm12| Bin 8880 -> 8834 bytes tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8906 -> 8860 bytes tests/data/acpi/q35/DSDT.viot | Bin 9383 -> 9339 bytes tests/data/acpi/q35/DSDT.xapic| Bin 35637 -> 35591 bytes tests/data/acpi/q35/ERST.acpierst | Bin 912 -> 912 bytes tests/data/acpi/q35/IVRS.ivrs | Bin 104 -> 100 bytes tests/data/acpi/q35/VIOT.viot | Bin 112 -> 112 bytes tests/unit/meson.build| 4 +++- 57 files changed, 31 insertions(+), 28 deletions(-) -- 2.37.2
Re: [PATCH 30/51] tests: Skip iotests and qtest when '--without-default-devices'
On Thu, Aug 25, 2022 at 8:04 PM Thomas Huth wrote:
>
> On 24/08/2022 11.40, Bin Meng wrote:
> > From: Bin Meng
> >
> > When QEMU is configured with '--without-default-devices', we should
> > not build and run iotests and qtest because devices used by these
> > test cases are not built in.
> >
> > Signed-off-by: Bin Meng
> > ---
> >
> > tests/qemu-iotests/meson.build | 5 +
> > tests/qtest/meson.build| 5 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
> > index 323a4acb6a..38d9a874d2 100644
> > --- a/tests/qemu-iotests/meson.build
> > +++ b/tests/qemu-iotests/meson.build
> > @@ -2,6 +2,11 @@ if not have_tools or targetos == 'windows' or
> > get_option('gprof')
> > subdir_done()
> > endif
> >
> > +# Skip iotests if configured without a default selection of devices
> > +if not get_option('default_devices')
> > + subdir_done()
> > +endif
> > +
> > foreach cflag: config_host['QEMU_CFLAGS'].split()
> > if cflag.startswith('-fsanitize') and \
> >not cflag.contains('safe-stack') and not cflag.contains('cfi-icall')
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index c97da5a062..0291b3966c 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -4,6 +4,11 @@ if not config_host.has_key('CONFIG_POSIX')
> > subdir_done()
> > endif
> >
> > +# Skip QTests if configured without a default selection of devices
> > +if not get_option('default_devices')
> > + subdir_done()
> > +endif
> > +
> > slow_qtests = {
> > 'ahci-test' : 60,
> > 'bios-tables-test' : 120,
>
> That's a very big hammer already ... I'd prefer if we could work on the
> tests instead to adapt for the availability of devices instead (we've done
> quite a lot of work in this area in the past already, but apparently still
> not enough yet ...)
Adapting tests for the availability of devices is a large scope. I may
not have time to work on this.
I will have to drop this patch in v2, and adjust the patches in the
series to keep bisectability then.
Regards,
Bin
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Hi
On Fri, Sep 2, 2022 at 3:16 PM Markus Armbruster wrote:
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster
> wrote:
> >
> >> Daniel P. Berrangé writes:
> >>
> >> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell <
> peter.mayd...@linaro.org
> >> >
> >> >> wrote:
> >>
> >> [...]
> >>
> >> >> > As Markus says, your branch ends up moving most of qobject into
> >> >> > qemu-common/. We are never going to let that out of QEMU proper,
> >> >> > because we are never going to allow ourselves to be tied to API
> >> >> > compatibility with it as an external library. So anything that
> >> >> >
> >> >>
> >> >> Why is that? We do it with a lot of dependencies already, with stable
> >> APIs.
> >> >>
> >> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
> >> >> continue to link statically and compile against a specific version.
> like
> >> >> with libvfio-user today.
> >> >>
> >> >> And at this point, I am _not_ proposing to have an extra
> "qemu-common"
> >> >> repository. I don't think there are enough reasons to want that
> either.
> >> >>
> >> >>
> >> >>
> >> >> > needs qobject is never going to leave the QEMU codebase. Which
> >> >> > means that there's not much gain from shoving it into subproject/
> >> >> > IMHO.
> >> >>
> >> >>
> >> >> (just to be extra clear, it's qobject not QOM we are talking about)
> >> >>
> >> >> qobject is fundamental to all the QAPI related generated code. Why
> >> should
> >> >> that remain tight to QEMU proper?
> >> >
> >> > Neither qobject nor QOM have ever been designed to be public APIs.
> >> > Though admittedly qobject is quite a bit simpler as an API, I'm
> >> > not convinced its current design is something we want to consider
> >> > public. As an example, just last month Markus proposed changing
> >> > QDict's implementation
> >> >
> >> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
> >> >
> >> >
> >> > If we want external projects to be able to take advantage of QAPI,
> >> > the bare minimum we need to be public is a QAPI parser, from which
> >> > people can then build any code generators desired.
> >>
> >> Basically scripts/qapi/ less the code generators.
> >>
> >>
> > The generated code is used by qemu-ga & storage daemon, at least. They
> are
> > the first potential consumers, after qemu.
> >
> >
> >> Not sure a subproject would be a good fit.
> >>
> >
> > (I won't repeat the arguments of structuring a project)
> >
> >
> >>
> >> Shot from the hip: could the build process spit out something external
> >> projects could consume? It's how "consumables" are commonly delivered.
> >> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
> >> it gets copied into the consuming tree ("vendored").
> >>
> >>
> > Not sure I follow, but that's just the "install" step isn't it ?
> >
> > Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But
> then,
> > you would expect stability/versioning. That's not what I am proposing (at
> > least at this point), which is just about the build system and project
> > structure, so we can build and work on subprojects independently. (for
> ex,
> > in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
> > configure etc)
>
> I'm afraid I'm still wobbly on the benefits of subprojects, or even how
> to work with them.
>
> How exactly would we "build and work independently" on the subprojects
> involving QAPI? git-clone all of QEMU, but build only a subproject (and
> its dependencies)? Am I confused?
>
Yes, QEMU repository would hold some subprojects (like libvhost-user
today), that you can build/develop independently with just meson / ninja.
>
> >> > We don't neccessarily need the current QAPI C code generator. There
> >> > could be a new C generator that didn't use qobject, but instead used
> >> > some standard GLib types like GHashTable/GList instead of QDict/QList.
> >>
> >> Yes, that should be possible.
> >>
> >>
> > I can't see much benefit from doing that extra work. It would create two
> C
> > APIs, making future bindings efforts more difficult etc.
>
> We need to distinguish client-side and server-side APIs / bindings.
>
Indeed.. although imho, it's best when both are similar, or use similar
types / concepts. (gdbus does a pretty good job, for ex)
>
> The existing C generator targets internal, server-side use. It
> generates everything defined in the schema.
>
> The existing introspection generator generates data for external,
> client-side use (via QMP). It generates just the subset visible in QMP.
>
> The proposed Go generator also targets external, client-side use (client
> bindings for QMP), and should also generate just the subset visible in
> QMP.
>
> So should a future C generator of client bindings for QMP.
>
> And then we'd have two distinct C APIs: a server-side one to help us
> implement QMP, a
Re: [PATCH v2 11/15] qemu-common: move scripts/qapi
Marc-André Lureau writes:
> Hi
>
> On Thu, Aug 11, 2022 at 5:35 PM Markus Armbruster wrote:
>
>> Daniel P. Berrangé writes:
>>
>> > On Thu, Aug 11, 2022 at 02:50:01PM +0400, Marc-André Lureau wrote:
>> >> Hi
>> >>
>> >> On Thu, Aug 11, 2022 at 2:22 PM Peter Maydell > >
>> >> wrote:
>>
>> [...]
>>
>> >> > As Markus says, your branch ends up moving most of qobject into
>> >> > qemu-common/. We are never going to let that out of QEMU proper,
>> >> > because we are never going to allow ourselves to be tied to API
>> >> > compatibility with it as an external library. So anything that
>> >> >
>> >>
>> >> Why is that? We do it with a lot of dependencies already, with stable
>> APIs.
>> >>
>> >> Furthermore, we don't "have to" be tied to a specific ABI/API, we can
>> >> continue to link statically and compile against a specific version. like
>> >> with libvfio-user today.
>> >>
>> >> And at this point, I am _not_ proposing to have an extra "qemu-common"
>> >> repository. I don't think there are enough reasons to want that either.
>> >>
>> >>
>> >>
>> >> > needs qobject is never going to leave the QEMU codebase. Which
>> >> > means that there's not much gain from shoving it into subproject/
>> >> > IMHO.
>> >>
>> >>
>> >> (just to be extra clear, it's qobject not QOM we are talking about)
>> >>
>> >> qobject is fundamental to all the QAPI related generated code. Why
>> should
>> >> that remain tight to QEMU proper?
>> >
>> > Neither qobject nor QOM have ever been designed to be public APIs.
>> > Though admittedly qobject is quite a bit simpler as an API, I'm
>> > not convinced its current design is something we want to consider
>> > public. As an example, just last month Markus proposed changing
>> > QDict's implementation
>> >
>> > https://lists.gnu.org/archive/html/qemu-devel/2022-07/msg00758.html
>> >
>> >
>> > If we want external projects to be able to take advantage of QAPI,
>> > the bare minimum we need to be public is a QAPI parser, from which
>> > people can then build any code generators desired.
>>
>> Basically scripts/qapi/ less the code generators.
>>
>>
> The generated code is used by qemu-ga & storage daemon, at least. They are
> the first potential consumers, after qemu.
>
>
>> Not sure a subproject would be a good fit.
>>
>
> (I won't repeat the arguments of structuring a project)
>
>
>>
>> Shot from the hip: could the build process spit out something external
>> projects could consume? It's how "consumables" are commonly delivered.
>> E.g. .so + a bunch of headers. Sometimes that gets packaged. Sometimes
>> it gets copied into the consuming tree ("vendored").
>>
>>
> Not sure I follow, but that's just the "install" step isn't it ?
>
> Sure, we could have a "qemu-devel", that ships qapi-gen, I guess. But then,
> you would expect stability/versioning. That's not what I am proposing (at
> least at this point), which is just about the build system and project
> structure, so we can build and work on subprojects independently. (for ex,
> in my wip branch, qemu-ga meson.build is 115 lines, doesn't need QEMU
> configure etc)
I'm afraid I'm still wobbly on the benefits of subprojects, or even how
to work with them.
How exactly would we "build and work independently" on the subprojects
involving QAPI? git-clone all of QEMU, but build only a subproject (and
its dependencies)? Am I confused?
>> > We don't neccessarily need the current QAPI C code generator. There
>> > could be a new C generator that didn't use qobject, but instead used
>> > some standard GLib types like GHashTable/GList instead of QDict/QList.
>>
>> Yes, that should be possible.
>>
>>
> I can't see much benefit from doing that extra work. It would create two C
> APIs, making future bindings efforts more difficult etc.
We need to distinguish client-side and server-side APIs / bindings.
The existing C generator targets internal, server-side use. It
generates everything defined in the schema.
The existing introspection generator generates data for external,
client-side use (via QMP). It generates just the subset visible in QMP.
The proposed Go generator also targets external, client-side use (client
bindings for QMP), and should also generate just the subset visible in
QMP.
So should a future C generator of client bindings for QMP.
And then we'd have two distinct C APIs: a server-side one to help us
implement QMP, and a client-side one to help use use QMP (QMP bindings).
Whether the two would profit from sharing QObject is not clear to me
at this point.
> QObject is very much like GValue though (the naming is better too :), just
> like the QOM & GObject story.
>
> thanks
Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
On Thu, Sep 1, 2022 at 4:42 PM Marc-André Lureau wrote: > > Hi > > On Wed, Aug 24, 2022 at 2:03 PM Bin Meng wrote: >> >> From: Bin Meng >> >> On Windows, the MinGW provided mkstemp() API opens the file with >> exclusive access, denying other processes to read/write the file. >> Such behavior prevents the QEMU executable from opening the file, >> (e.g.: CreateFile returns ERROR_SHARING_VIOLATION). > > > g_mkstemp() doesn't have this behaviour (after running a quick test). Use it? > Thanks for the suggestion! I've switched to using g_file_open_tmp() in patch #7 "tests: Avoid using hardcoded /tmp in test cases", and testing shows that it does not have such an issue. I checked glib sources and see both g_mkstemp() and g_file_open_tmp() call g_open() which allows shared read/write on Windows. So this patch can be dropped. Regards, Bin
Re: [PATCH 32/51] tests/qtest: Fix ERROR_SHARING_VIOLATION for win32
On Thu, Aug 25, 2022 at 8:06 PM Thomas Huth wrote: > > On 24/08/2022 11.40, Bin Meng wrote: > > From: Bin Meng > > > > On Windows, the MinGW provided mkstemp() API opens the file with > > exclusive access, denying other processes to read/write the file. > > Such behavior prevents the QEMU executable from opening the file, > > (e.g.: CreateFile returns ERROR_SHARING_VIOLATION). > > > > This can be fixed by closing the file and reopening it. > > Would it work to use the glib functions instead (like g_file_open_tmp() ?) > Yep, I've switched to using g_file_open_tmp() in patch #7 "tests: Avoid using hardcoded /tmp in test cases", and testing shows that it does not have such an issue. So this patch can be dropped. Regards, Bin
Re: [PATCH v2] pci: Assert that capabilities never overlap
Akihiko Odaki writes: > pci_add_capability appears most PCI devices. Its error handling required > lots of code, and led to inconsistent behaviors such as: > - passing error_abort > - passing error_fatal > - asserting the returned value > - propagating the error to the caller > - skipping the rest of the function > - just ignoring > > The code generating errors in pci_add_capability had a comment which > says: >> Verify that capabilities don't overlap. Note: device assignment >> depends on this check to verify that the device is not broken. >> Should never trigger for emulated devices, but it's helpful for >> debugging these. > > Indeed vfio has some code that passes capability offsets and sizes from > a physical device, but it explicitly pays attention so that the > capabilities never overlap. I can't see that at a glance. Can you give me a clue? > Therefore, we can always assert that > capabilities never overlap when pci_add_capability is called, resolving > these inconsistencies. > > Signed-off-by: Akihiko Odaki
[PATCH 4/6] parallels: Use highest_offset() helper in leak check
Deduplicate code by using highest_offset() helper.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 339ce45634..688aa081e2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -535,16 +535,9 @@ static int parallels_check_leak(BlockDriverState *bs,
BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
-int64_t off, high_off, count, cut_out;
-uint32_t i;
+int64_t high_off, count, cut_out;
-high_off = 0;
-for (i = 0; i < s->bat_size; i++) {
-off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-if (off > high_off) {
-high_off = off;
-}
-}
+high_off = highest_offset(s);
cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
if (cut_out < 0) {
--
2.34.1
[PATCH 5/6] parallels: Replace fprintf by qemu_log
Use a standard QEMU function for logging.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 688aa081e2..08526196da 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -42,6 +42,7 @@
#include "qemu/bswap.h"
#include "qemu/bitmap.h"
#include "qemu/memalign.h"
+#include "qemu/log-for-trace.h"
#include "migration/blocker.h"
#include "parallels.h"
@@ -448,7 +449,7 @@ static void parallels_check_unclean(BlockDriverState *bs,
return;
}
-fprintf(stderr, "%s image was not closed correctly\n",
+qemu_log("%s image was not closed correctly\n",
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
@@ -476,7 +477,7 @@ static int parallels_check_outside_image(BlockDriverState
*bs,
for (i = 0; i < s->bat_size; i++) {
off = bat2sect(s, i) << BDRV_SECTOR_BITS;
if (off >= size) {
-fprintf(stderr, "%s cluster %u is outside image\n",
+qemu_log("%s cluster %u is outside image\n",
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
@@ -549,7 +550,7 @@ static int parallels_check_leak(BlockDriverState *bs,
}
count = DIV_ROUND_UP(cut_out, s->cluster_size);
-fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+qemu_log("%s space leaked at the end of the image %" PRId64 "\n",
fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
res->leaks += count;
@@ -600,8 +601,7 @@ static int parallels_check_duplicate(BlockDriverState *bs,
cluster_index = host_cluster_index(s, off);
if (test_bit(cluster_index, bitmap)) {
/* this cluster duplicates another one */
-fprintf(stderr,
-"%s duplicate offset in BAT entry %u\n",
+qemu_log("%s duplicate offset in BAT entry %u\n",
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
res->corruptions++;
--
2.34.1
[PATCH 2/6] parallels: Create parallels_handle_leak() to truncate excess clusters
This helper will be reused in the next patch for duplications check.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 65 +++
1 file changed, 43 insertions(+), 22 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7b4e997ebd..dbcaf5d310 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -475,37 +475,23 @@ static int parallels_check_outside_image(BlockDriverState
*bs,
return 0;
}
-static int parallels_check_leak(BlockDriverState *bs,
-BdrvCheckResult *res,
-BdrvCheckMode fix)
+static int64_t parallels_handle_leak(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ int64_t high_off,
+ bool fix)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size, off, high_off, count;
-uint32_t i;
+int64_t size;
int ret;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
-res->check_errors++;
return size;
}
-high_off = 0;
-for (i = 0; i < s->bat_size; i++) {
-off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-if (off > high_off) {
-high_off = off;
-}
-}
-
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-fprintf(stderr, "%s space leaked at the end of the image %" PRId64
"\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-if (fix & BDRV_FIX_LEAKS) {
+if (fix) {
Error *local_err = NULL;
/*
@@ -516,13 +502,48 @@ static int parallels_check_leak(BlockDriverState *bs,
PREALLOC_MODE_OFF, 0, &local_err);
if (ret < 0) {
error_report_err(local_err);
-res->check_errors++;
return ret;
}
-res->leaks_fixed += count;
}
}
+return size - res->image_end_offset;
+}
+
+static int parallels_check_leak(BlockDriverState *bs,
+BdrvCheckResult *res,
+BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t off, high_off, count, cut_out;
+uint32_t i;
+
+high_off = 0;
+for (i = 0; i < s->bat_size; i++) {
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off > high_off) {
+high_off = off;
+}
+}
+
+cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
+if (cut_out < 0) {
+res->check_errors++;
+return cut_out;
+}
+if (cut_out == 0) {
+return 0;
+}
+
+count = DIV_ROUND_UP(cut_out, s->cluster_size);
+fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
+
+res->leaks += count;
+if (fix & BDRV_FIX_LEAKS) {
+res->leaks_fixed += count;
+}
+
return 0;
}
--
2.34.1
[PATCH 3/6] parallels: Add checking and repairing duplicate offsets in BAT
Cluster offsets must be unique among all BAT entries.
Find duplicate offsets in the BAT.
If a duplicated offset is found fix it by copying the content
of the relevant cluster to a new allocated cluster and
set the new cluster offset to the duplicated entry.
Add host_cluster_index() helper to deduplicate the code.
Add highest_offset() helper. It will be used for code deduplication
in the next patch.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 136 ++
1 file changed, 136 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index dbcaf5d310..339ce45634 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,26 @@ static int cluster_remainder(BDRVParallelsState *s,
int64_t sector_num,
return MIN(nb_sectors, ret);
}
+static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
+{
+off -= s->header->data_off << BDRV_SECTOR_BITS;
+return off / s->cluster_size;
+}
+
+static int64_t highest_offset(BDRVParallelsState *s)
+{
+int64_t off, high_off = 0;
+int i;
+
+for (i = 0; i < s->bat_size; i++) {
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off > high_off) {
+high_off = off;
+}
+}
+return high_off;
+}
+
static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
int nb_sectors, int *pnum)
{
@@ -547,6 +567,114 @@ static int parallels_check_leak(BlockDriverState *bs,
return 0;
}
+static int parallels_check_duplicate(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+QEMUIOVector qiov;
+int64_t off, high_off, sector;
+unsigned long *bitmap;
+uint32_t i, bitmap_size, cluster_index;
+int n, ret = 0;
+uint64_t *buf = NULL;
+bool new_allocations = false;
+
+high_off = highest_offset(s);
+if (high_off == 0) {
+return 0;
+}
+
+/*
+ * Create a bitmap of used clusters.
+ * If a bit is set, there is a BAT entry pointing to this cluster.
+ * Loop through the BAT entrues, check bits relevant to an entry offset.
+ * If bit is set, this entry is duplicated. Otherwise set the bit.
+ */
+bitmap_size = host_cluster_index(s, high_off) + 1;
+bitmap = bitmap_new(bitmap_size);
+
+buf = g_malloc(s->cluster_size);
+qemu_iovec_init(&qiov, 0);
+qemu_iovec_add(&qiov, buf, s->cluster_size);
+
+for (i = 0; i < s->bat_size; i++) {
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off == 0) {
+continue;
+}
+
+cluster_index = host_cluster_index(s, off);
+if (test_bit(cluster_index, bitmap)) {
+/* this cluster duplicates another one */
+fprintf(stderr,
+"%s duplicate offset in BAT entry %u\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+
+res->corruptions++;
+
+if (fix & BDRV_FIX_ERRORS) {
+/*
+ * Reset the entry and allocate a new cluster
+ * for the relevant guest offset. In this way we let
+ * the lower layer to place the new cluster properly.
+ * Copy the original cluster to the allocated one.
+ */
+parallels_set_bat_entry(s, i, 0);
+
+ret = bdrv_pread(bs->file, off, s->cluster_size, buf, 0);
+if (ret < 0) {
+res->check_errors++;
+goto out;
+}
+
+sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+off = allocate_clusters(bs, sector, s->tracks, &n);
+if (off < 0) {
+res->check_errors++;
+ret = off;
+goto out;
+}
+off <<= BDRV_SECTOR_BITS;
+if (off > high_off) {
+high_off = off;
+}
+
+ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov,
0);
+if (ret < 0) {
+res->check_errors++;
+goto out;
+}
+
+new_allocations = true;
+res->corruptions_fixed++;
+}
+
+} else {
+bitmap_set(bitmap, cluster_index, 1);
+}
+}
+
+if (new_allocations) {
+/*
+ * When new clusters are allocated, file size increases
+ * by 128 Mb blocks. We need to truncate the file to the
+ * right size.
+ */
+ret = parallels_handle_leak(bs, res, high_off, true);
+if (ret < 0) {
+res->check_errors++;
+goto out;
+}
+}
+
+out:
+qemu_iovec_destroy(&qiov);
+g_free(buf);
+g_free(bitmap);
+return ret;
+}
+
static void parallels_collect_stati
[PATCH 6/6] parallels: Image repairing in parallels_open()
Repair an image at opening if the image is unclean or
out-of-image corruption was detected.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 95 ---
1 file changed, 65 insertions(+), 30 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 08526196da..a7c3af4ef2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -735,6 +735,18 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
return ret;
}
+typedef struct ParallelsOpenCheckCo {
+BlockDriverState *bs;
+BdrvCheckResult *res;
+BdrvCheckMode fix;
+int ret;
+} ParallelsOpenCheckCo;
+
+static void coroutine_fn parallels_co_open_check(void *opaque)
+{
+ParallelsOpenCheckCo *poc = opaque;
+poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
+}
static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
Error **errp)
@@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
{
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
-int ret, size, i;
-int64_t file_size;
+int ret, size;
+int64_t file_size, high_off;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -1027,34 +1039,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->bat_bitmap = (uint32_t *)(s->header + 1);
-for (i = 0; i < s->bat_size; i++) {
-int64_t off = bat2sect(s, i);
-if (off >= file_size) {
-if (flags & BDRV_O_CHECK) {
-continue;
-}
-error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
- "is larger than file size (%" PRIi64 ")",
- off, i, file_size);
-ret = -EINVAL;
-goto fail;
-}
-if (off >= s->data_end) {
-s->data_end = off + s->tracks;
-}
-}
-
-if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-/* Image was not closed correctly. The check is mandatory */
-s->header_unclean = true;
-if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
-error_setg(errp, "parallels: Image was not closed correctly; "
- "cannot be opened read/write");
-ret = -EACCES;
-goto fail;
-}
-}
-
opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
if (!opts) {
goto fail_options;
@@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
error_free(s->migration_blocker);
goto fail;
}
+
qemu_co_mutex_init(&s->lock);
+
+if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+s->header_unclean = true;
+}
+
+high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
+if (high_off >= s->data_end) {
+s->data_end = high_off + s->tracks;
+}
+
+/*
+ * We don't repair the image here if it is opened for checks.
+ * Also let to work with images in RO mode.
+ */
+if ((flags & BDRV_O_CHECK) || !(flags & BDRV_O_RDWR)) {
+return 0;
+}
+
+/*
+ * Repair the image if it's dirty or
+ * out-of-image corruption was detected.
+ */
+if (s->data_end > file_size ||
+le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+BdrvCheckResult res = {0};
+Coroutine *co;
+ParallelsOpenCheckCo poc = {
+.bs = bs,
+.res = &res,
+.fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
+.ret = -EINPROGRESS
+};
+
+if (qemu_in_coroutine()) {
+/* From bdrv_co_create. */
+parallels_co_open_check(&poc);
+} else {
+assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+co = qemu_coroutine_create(parallels_co_open_check, &poc);
+qemu_coroutine_enter(co);
+BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
+}
+
+if (poc.ret < 0) {
+error_setg_errno(errp, -poc.ret,
+ "Could not repair corrupted image");
+goto fail;
+}
+}
+
return 0;
fail_format:
--
2.34.1
[PATCH 0/6] parallels: Add duplication check, repair at open, fix bugs
This patchset is based on git: https://src.openvz.org/~den/qemu.git parallels Fix incorrect data end calculation in parallels_open(). Add parallels_handle_leak() and highest_offset() helpers. Add checking and repairing duplicate offsets in BAT. Deduplicate highest offset calculation. Replace fprintf() by qemu_log(). Add check and repair to parallels_open(). Alexander Ivanov (6): parallels: Incorrect data end calculation in parallels_open() parallels: Create parallels_handle_leak() to truncate excess clusters parallels: Add checking and repairing duplicate offsets in BAT parallels: Use highest_offset() helper in leak check parallels: Replace fprintf by qemu_log parallels: Image repairing in parallels_open() block/parallels.c | 297 +- 1 file changed, 241 insertions(+), 56 deletions(-) -- 2.34.1
[PATCH 1/6] parallels: Incorrect data end calculation in parallels_open()
The BDRVParallelsState structure contains data_end field that
is measured in sectors.
In parallels_open() initially this field is set by data_off field from
parallels image header.
According to the parallels format documentation,
data_off field contains an offset, in sectors, from the start of the file
to the start of the data area.
For "WithoutFreeSpace" images: if data_off is zero,
the offset is calculated as the end of the BAT table
plus some padding to ensure sector size alignment.
The parallels_open() function has code for handling zero value
in data_off, but in the result data_end contains the offset in bytes.
Replace the alignment to sector size by division by sector size
and fix the comparision with s->header_size.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index e6e8b9e369..7b4e997ebd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -863,9 +863,9 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_end = le32_to_cpu(ph.data_off);
if (s->data_end == 0) {
-s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
}
-if (s->data_end < s->header_size) {
+if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
/* there is not enough unused space to fit to block align between BAT
and actual data. We can't avoid read-modify-write... */
s->header_size = size;
--
2.34.1
Re: [RFC v4 11/11] virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint
On 30.08.22 22:16, Stefan Hajnoczi wrote:
> On Thu, Aug 25, 2022 at 09:43:16AM +0200, David Hildenbrand wrote:
>> On 23.08.22 21:22, Stefan Hajnoczi wrote:
>>> On Tue, Aug 23, 2022 at 10:01:59AM +0200, David Hildenbrand wrote:
On 23.08.22 00:24, Stefan Hajnoczi wrote:
> Register guest RAM using BlockRAMRegistrar and set the
> BDRV_REQ_REGISTERED_BUF flag so block drivers can optimize memory
> accesses in I/O requests.
>
> This is for vdpa-blk, vhost-user-blk, and other I/O interfaces that rely
> on DMA mapping/unmapping.
Can you explain why we're monitoring RAMRegistrar to hook into "guest
RAM" and not go the usual path of the MemoryListener?
>>>
>>> The requirements are similar to VFIO, which uses RAMBlockNotifier. We
>>
>> Only VFIO NVME uses RAMBlockNotifier. Ordinary VFIO uses the MemoryListener.
>>
>> Maybe the difference is that ordinary VFIO has to replicate the actual
>> guest physical memory layout, and VFIO NVME is only interested in
>> possible guest RAM inside guest physical memory.
>>
>>> need to learn about all guest RAM because that's where I/O buffers are
>>> located.
>>>
>>> Do you think RAMBlockNotifier should be avoided?
>>
>> I assume it depends on the use case. For saying "this might be used for
>> I/O" it might be good enough I guess.
>>
>>>
What will BDRV_REQ_REGISTERED_BUF actually do? Pin all guest memory in
the worst case such as io_uring fixed buffers would do ( I hope not ).
>>>
>>> BLK_REQ_REGISTERED_BUF is a hint that no bounce buffer is necessary
>>> because the I/O buffer is located in memory that was previously
>>> registered with bdrv_registered_buf().
>>>
>>> The RAMBlockNotifier calls bdrv_register_buf() to let the libblkio
>>> driver know about RAM. Some libblkio drivers ignore this hint, io_uring
>>> may use the fixed buffers feature, vhost-user sends the shared memory
>>> file descriptors to the vhost device server, and VFIO/vhost may pin
>>> pages.
>>>
>>> So the blkio block driver doesn't add anything new, it's the union of
>>> VFIO/vhost/vhost-user/etc memory requirements.
>>
>> The issue is if that backend pins memory inside any of these regions.
>> Then, you're instantly incompatible to anything the relies on sparse
>> RAMBlocks, such as memory ballooning or virtio-mem, and have to properly
>> fence it.
>>
>> In that case, you'd have to successfully trigger
>> ram_block_discard_disable(true) first, before pinning. Who would do that
>> now conditionally, just like e.g., VFIO does?
>>
>> io_uring fixed buffers would be one such example that pins memory and is
>> problematic. vfio (unless on s390x) is another example, as you point out.
>
> Okay, I think libblkio needs to expose a bool property called
> "mem-regions-pinned" so QEMU whether or not the registered buffers will
> be pinned.
>
> Then the QEMU BlockDriver can do:
>
> if (mem_regions_pinned) {
> if (ram_block_discard_disable(true) < 0) {
> ...fail to open block device...
> }
> }
>
> Does that sound right?
Yes, I think so.
>
> Is "pinned" the best word to describe this or is there a more general
> characteristic we are looking for?
pinning should be the right term. We want to express that all user page
tables will immediately get populated and that a kernel subsystem will
take longterm references on mapped page that will go out of sync as soon
as we discard memory e.g., using madvise(MADV_DONTEED).
We just should not confuse it with memlock / locking into memory, which
are yet different semantics (e.g., don't swap it out).
>
>>
>> This has to be treated with care. Another thing to consider is that
>> different backends might only support a limited number of such regions.
>> I assume there is a way for QEMU to query this limit upfront? It might
>> be required for memory hot(un)plug to figure out how many memory slots
>> we actually have (for ordinary DIMMs, and if we ever want to make this
>> compatible to virtio-mem, it might be required as well when the backend
>> pins memory).
>
> Yes, libblkio reports the maximum number of blkio_mem_regions supported
> by the device. The property is called "max-mem-regions".
>
> The QEMU BlockDriver currently doesn't use this information. Are there
> any QEMU APIs that should be called to propagate this value?
I assume we have to do exactly the same thing as e.g.,
vhost_has_free_slot()/kvm_has_free_slot() does.
Especially, hw/mem/memory-device.c needs care and
slots_limit/used_memslots handling in hw/virtio/vhost.c might be
relevant as well.
Note that I have some patches pending that extend that handling, by also
providing how many used+free slots there are, such as:
https://lore.kernel.org/all/20211027124531.57561-3-da...@redhat.com/
--
Thanks,
David / dhildenb
