u->postcopy_client_bases[i] corresponds to dev->mem->regions[i], not
to u->shadow_regions[i]. So we should use j-index, following indexation
of dev->mem->regions.

Fixes: f1aeb14b0809e313c7424 "Transmit vhost-user memory regions individually"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

Hi all!

Honestly, I don't follow the logic of vhost-user postcopy, but I can't
believe, that the same array should be used in different indexing
semantics.

 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index a8907cca74e..91de08a5128 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -602,7 +602,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
                     if (fd > 0) {
                         u->region_rb_offset[j] = offset;
                         u->region_rb[j] = mr->ram_block;
-                        shadow_pcb[j] = u->postcopy_client_bases[i];
+                        shadow_pcb[j] = u->postcopy_client_bases[j];
                     } else {
                         u->region_rb_offset[j] = 0;
                         u->region_rb[j] = NULL;
-- 
2.52.0

On 03.04.26 12:34, Vladimir Sementsov-Ogievskiy wrote:
> u->postcopy_client_bases[i] corresponds to dev->mem->regions[i], not
> to u->shadow_regions[i]. So we should use j-index, following indexation
> of dev->mem->regions.
> 
> Fixes: f1aeb14b0809e313c7424 "Transmit vhost-user memory regions individually"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> Hi all!
> 
> Honestly, I don't follow the logic of vhost-user postcopy, but I can't
> believe, that the same array should be used in different indexing
> semantics.
> 
>   hw/virtio/vhost-user.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index a8907cca74e..91de08a5128 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -602,7 +602,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
>                       if (fd > 0) {
>                           u->region_rb_offset[j] = offset;
>                           u->region_rb[j] = mr->ram_block;
> -                        shadow_pcb[j] = u->postcopy_client_bases[i];
> +                        shadow_pcb[j] = u->postcopy_client_bases[j];
>                       } else {
>                           u->region_rb_offset[j] = 0;
>                           u->region_rb[j] = NULL;


Hmm, now I think, that postcopy + CONFIGURE_MEM_SLOTS is totally broken
both in protocol and in the code.

Look:

for VHOST_USER_SET_MEM_TABLE we have not in the spec:

.. Note::
    ``NEED_REPLY_MASK`` is not set in this case.  QEMU will then
    reply back to the list of mappings with an empty
    ``VHOST_USER_SET_MEM_TABLE`` as an acknowledgement; only upon
    reception of this message may the guest start accessing the memory
    and generating faults.


and in code we do in vhost_user_set_mem_table_postcopy():

         msg.hdr.size = sizeof(msg.payload.u64);
         msg.payload.u64 = 0; /* OK */
         ret = vhost_user_write(dev, &msg, NULL, 0);
         if (ret < 0) {
             return ret;
         }

Note, that msg.hdr.request is set to VHOST_USER_SET_MEM_TABLE by
vhost_user_fill_set_mem_table_msg() call.



But when CONFIGURE_MEM_SLOTS is negotiated we don't use VHOST_USER_SET_MEM_TABLE
message, instead we use VHOST_USER_REM_MEM_REG and VHOST_USER_ADD_MEM_REG.

for VHOST_USER_REM_MEM_REG we have nothing about postcopy in documentation,
and for VHOST_USER_ADD_MEM_REG we have only:

   In postcopy mode (see ``VHOST_USER_POSTCOPY_LISTEN``), the back-end
   replies with the bases of the memory mapped region to the front-end.
   For further details on postcopy, see ``VHOST_USER_SET_MEM_TABLE``.
   They apply to ``VHOST_USER_ADD_MEM_REG`` accordingly.

how should it work about "an acknowledgement"? Should QEMU send
an empty VHOST_USER_SET_MEM_TABLE ? And when?


Let's see what we have in code:

vhost_user_add_remove_regions() {

    if (nr_rem_reg) { send_remove_regions() }

    if (nr_add_reg) { send_add_regions() }

    ... and for postcopy:

    msg->hdr.size = sizeof(msg->payload.u64);
    msg->payload.u64 = 0; /* OK */

    ret = vhost_user_write(dev, msg, NULL, 0);
    if (ret < 0) {
        return ret;
    }


Note that:

1. We send only one acknowledgement, after sending several (or zero)
    ADD/REM requests.

2. We send an acknowledgement with request set to either VHOST_USER_ADD_MEM_REG,
    or VHOST_USER_REM_MEM_REG, or zero (if bot nr_rem_reg and nr_add_reg are zero).


I think, it far far away from documentation. And it can't be simply "fixed". If we
need postcopy for CONFIGURE_MEM_SLOTS case, the protocol extension to support it
should first be invented.


I think, it would be correct to abandon using postcopy together with CONFIGURE_MEM_SLOTS
for now, and document that behavior of server is actually unpredictable if use postcopy
commands together with CONFIGURE_MEM_SLOTS negotiated.

-- 
Best regards,
Vladimir