On 04.11.25 01:19, Michael S. Tsirkin wrote:
> On Wed, Oct 15, 2025 at 05:58:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Raphael Norwitz <raphael.s.norwitz@gmail.com>
>> Reviewed-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
>
>
> This needs a better commit log.
> the subject makes it look like it's merely adding a helper
> but it is not the case: e.g. errors are now handled
> somewhat differently.
>
> Pls document the thinking process explaining why it does not
> matter. CB.
>
Will do. Or, split into separate commits for helper and for logic change.
>
>> ---
>> hw/virtio/vhost.c | 71 ++++++++++++++++++++++++++---------------------
>> 1 file changed, 40 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 34846edf36..56a812b06b 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1572,12 +1572,50 @@ static int vhost_dev_get_features(struct vhost_dev *hdev,
>> return r;
>> }
>>
>> +static bool check_memslots(struct vhost_dev *hdev, Error **errp)
>> +{
>> + unsigned int used, reserved, limit;
>> +
>> + limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
>> + if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
>> + memory_devices_memslot_auto_decision_active()) {
>> + error_setg(errp, "some memory device (like virtio-mem)"
>> + " decided how many memory slots to use based on the overall"
>> + " number of memory slots; this vhost backend would further"
>> + " restricts the overall number of memory slots");
>> + error_append_hint(errp, "Try plugging this vhost backend before"
>> + " plugging such memory devices.\n");
>> + return false;
>> + }
>> +
>> + /*
>> + * The listener we registered properly setup the number of required
>
> This comment worked in the original code but not now.
>
> The listener here is memory_listener_register which yes happens to
> be called before check_memslots but it is far from obvious.
>
>
>
>> + * memslots in vhost_commit().
>> + */
>> + used = hdev->mem->nregions;
>
>
>
>> +
>> + /*
>> + * We assume that all reserved memslots actually require a real memslot
>> + * in our vhost backend. This might not be true, for example, if the
>> + * memslot would be ROM. If ever relevant, we can optimize for that --
>> + * but we'll need additional information about the reservations.
>> + */
>> + reserved = memory_devices_get_reserved_memslots();
>> + if (used + reserved > limit) {
>> + error_setg(errp, "vhost backend memory slots limit (%d) is less"
>> + " than current number of used (%d) and reserved (%d)"
>> + " memory slots for memory devices.", limit, used, reserved);
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> VhostBackendType backend_type, uint32_t busyloop_timeout,
>> Error **errp)
>> {
>> uint64_t features[VIRTIO_FEATURES_NU64S];
>> - unsigned int used, reserved, limit;
>> int i, r, n_initialized_vqs = 0;
>>
>> hdev->vdev = NULL;
>> @@ -1603,19 +1641,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> goto fail;
>> }
>>
>> - limit = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
>> - if (limit < MEMORY_DEVICES_SAFE_MAX_MEMSLOTS &&
>> - memory_devices_memslot_auto_decision_active()) {
>> - error_setg(errp, "some memory device (like virtio-mem)"
>> - " decided how many memory slots to use based on the overall"
>> - " number of memory slots; this vhost backend would further"
>> - " restricts the overall number of memory slots");
>> - error_append_hint(errp, "Try plugging this vhost backend before"
>> - " plugging such memory devices.\n");
>> - r = -EINVAL;
>> - goto fail;
>> - }
>> -
>> for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i,
>> busyloop_timeout);
>> @@ -1674,23 +1699,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>> memory_listener_register(&hdev->memory_listener, &address_space_memory);
>
>
> So it looks like with your change
> this will temporarily register the listener
> restricting the number of slots, then check and unregister.
>
> Is this ever a problem?
>
> commit log needs better documentation why.
>
>
>
>> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
>>
>> - /*
>> - * The listener we registered properly setup the number of required
>> - * memslots in vhost_commit().
>> - */
>> - used = hdev->mem->nregions;
>> -
>> - /*
>> - * We assume that all reserved memslots actually require a real memslot
>> - * in our vhost backend. This might not be true, for example, if the
>> - * memslot would be ROM. If ever relevant, we can optimize for that --
>> - * but we'll need additional information about the reservations.
>> - */
>> - reserved = memory_devices_get_reserved_memslots();
>> - if (used + reserved > limit) {
>> - error_setg(errp, "vhost backend memory slots limit (%d) is less"
>> - " than current number of used (%d) and reserved (%d)"
>> - " memory slots for memory devices.", limit, used, reserved);
>> + if (!check_memslots(hdev, errp)) {
>> r = -EINVAL;
>> goto fail;
>> }
>> --
>> 2.48.1
>
--
Best regards,
Vladimir