Re: [Qemu-devel] [PATCH v3 12/29] postcopy+vhost-user: Split set_mem_table for postcopy

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Fri, Feb 16, 2018 at 01:16:08PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Split the set_mem_table routines in both qemu and libvhost-user
> > because the postcopy versions are going to be quite different
> > once changes in the later patches are added.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  contrib/libvhost-user/libvhost-user.c | 53 ++++++++++++++++++++++++
> >  hw/virtio/vhost-user.c                | 77 
> > ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 128 insertions(+), 2 deletions(-)
> > 
> > diff --git a/contrib/libvhost-user/libvhost-user.c 
> > b/contrib/libvhost-user/libvhost-user.c
> > index beec7695a8..4922b2c722 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -448,6 +448,55 @@ vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
> >      return false;
> >  }
> >  
> > +static bool
> > +vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
> > +{
> > +    int i;
> > +    VhostUserMemory *memory = &vmsg->payload.memory;
> > +    dev->nregions = memory->nregions;
> > +    /* TODO: Postcopy specific code */
> > +    DPRINT("Nregions: %d\n", memory->nregions);
> > +    for (i = 0; i < dev->nregions; i++) {
> > +        void *mmap_addr;
> > +        VhostUserMemoryRegion *msg_region = &memory->regions[i];
> > +        VuDevRegion *dev_region = &dev->regions[i];
> > +
> > +        DPRINT("Region %d\n", i);
> > +        DPRINT("    guest_phys_addr: 0x%016"PRIx64"\n",
> > +               msg_region->guest_phys_addr);
> > +        DPRINT("    memory_size:     0x%016"PRIx64"\n",
> > +               msg_region->memory_size);
> > +        DPRINT("    userspace_addr   0x%016"PRIx64"\n",
> > +               msg_region->userspace_addr);
> > +        DPRINT("    mmap_offset      0x%016"PRIx64"\n",
> > +               msg_region->mmap_offset);
> > +
> > +        dev_region->gpa = msg_region->guest_phys_addr;
> > +        dev_region->size = msg_region->memory_size;
> > +        dev_region->qva = msg_region->userspace_addr;
> > +        dev_region->mmap_offset = msg_region->mmap_offset;
> > +
> > +        /* We don't use offset argument of mmap() since the
> > +         * mapped address has to be page aligned, and we use huge
> > +         * pages.  */
> > +        mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
> > +                         PROT_READ | PROT_WRITE, MAP_SHARED,
> > +                         vmsg->fds[i], 0);
> > +
> > +        if (mmap_addr == MAP_FAILED) {
> > +            vu_panic(dev, "region mmap error: %s", strerror(errno));
> > +        } else {
> > +            dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
> > +            DPRINT("    mmap_addr:       0x%016"PRIx64"\n",
> > +                   dev_region->mmap_addr);
> > +        }
> > +
> > +        close(vmsg->fds[i]);
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static bool
> >  vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> >  {
> > @@ -464,6 +513,10 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> >      }
> >      dev->nregions = memory->nregions;
> >  
> > +    if (dev->postcopy_listening) {
> > +        return vu_set_mem_table_exec_postcopy(dev, vmsg);
> > +    }
> > +
> >      DPRINT("Nregions: %d\n", memory->nregions);
> >      for (i = 0; i < dev->nregions; i++) {
> >          void *mmap_addr;
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index ec6a4a82fd..64f4b3b3f9 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -325,15 +325,86 @@ static int vhost_user_set_log_base(struct vhost_dev 
> > *dev, uint64_t base,
> >      return 0;
> >  }
> >  
> > +static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
> > +                                             struct vhost_memory *mem)
> > +{
> > +    int fds[VHOST_MEMORY_MAX_NREGIONS];
> > +    int i, fd;
> > +    size_t fd_num = 0;
> > +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> > +                                              
> > VHOST_USER_PROTOCOL_F_REPLY_ACK);
> > +    /* TODO: Add actual postcopy differences */
> > +    VhostUserMsg msg = {
> > +        .hdr.request = VHOST_USER_SET_MEM_TABLE,
> > +        .hdr.flags = VHOST_USER_VERSION,
> > +    };
> > +
> > +    if (reply_supported) {
> > +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> > +    }
> > +
> > +    for (i = 0; i < dev->mem->nregions; ++i) {
> > +        struct vhost_memory_region *reg = dev->mem->regions + i;
> > +        ram_addr_t offset;
> > +        MemoryRegion *mr;
> > +
> > +        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> > +        mr = memory_region_from_host((void 
> > *)(uintptr_t)reg->userspace_addr,
> > +                                     &offset);
> > +        fd = memory_region_get_fd(mr);
> > +        if (fd > 0) {
> > +            msg.payload.memory.regions[fd_num].userspace_addr =
> > +                reg->userspace_addr;
> > +            msg.payload.memory.regions[fd_num].memory_size  = 
> > reg->memory_size;
> > +            msg.payload.memory.regions[fd_num].guest_phys_addr =
> > +                reg->guest_phys_addr;
> > +            msg.payload.memory.regions[fd_num].mmap_offset = offset;
> > +            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> > +            fds[fd_num++] = fd;
> > +        }
> > +    }
> > +
> > +    msg.payload.memory.nregions = fd_num;
> > +
> > +    if (!fd_num) {
> > +        error_report("Failed initializing vhost-user memory map, "
> > +                     "consider using -object memory-backend-file 
> > share=on");
> > +        return -1;
> > +    }
> > +
> > +    msg.hdr.size = sizeof(msg.payload.memory.nregions);
> > +    msg.hdr.size += sizeof(msg.payload.memory.padding);
> > +    msg.hdr.size += fd_num * sizeof(VhostUserMemoryRegion);
> > +
> > +    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
> > +        return -1;
> > +    }
> > +
> > +    if (reply_supported) {
> > +        return process_message_reply(dev, &msg);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int vhost_user_set_mem_table(struct vhost_dev *dev,
> >                                      struct vhost_memory *mem)
> >  {
> > +    struct vhost_user *u = dev->opaque;
> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
> >      int i, fd;
> >      size_t fd_num = 0;
> > +    bool do_postcopy = u->postcopy_listen && u->postcopy_fd.handler;
> >      bool reply_supported = virtio_has_feature(dev->protocol_features,
> >                                                
> > VHOST_USER_PROTOCOL_F_REPLY_ACK);
> >  
> > +    if (do_postcopy) {
> > +        /* Postcopy has enough differences that it's best done in it's own
> > +         * version
> > +         */
> > +        return vhost_user_set_mem_table_postcopy(dev, mem);
> > +    }
> > +
> >      VhostUserMsg msg = {
> >          .hdr.request = VHOST_USER_SET_MEM_TABLE,
> >          .hdr.flags = VHOST_USER_VERSION,
> > @@ -357,9 +428,11 @@ static int vhost_user_set_mem_table(struct vhost_dev 
> > *dev,
> >                  error_report("Failed preparing vhost-user memory table 
> > msg");
> >                  return -1;
> >              }
> > -            msg.payload.memory.regions[fd_num].userspace_addr = 
> > reg->userspace_addr;
> > +            msg.payload.memory.regions[fd_num].userspace_addr =
> > +                reg->userspace_addr;
> >              msg.payload.memory.regions[fd_num].memory_size  = 
> > reg->memory_size;
> > -            msg.payload.memory.regions[fd_num].guest_phys_addr = 
> > reg->guest_phys_addr;
> > +            msg.payload.memory.regions[fd_num].guest_phys_addr =
> > +                reg->guest_phys_addr;
> 
> These newline changes might be avoided?

They could, but they're over 80 chars long, so while I was taking a copy
of the code I fixed the style on this copy so they were consistent.

> So after this patch there's no functional change, only the code
> splittion of set_mem_table operation, right?

Right; the changes to the postcopy version come later.

Dave

> Thanks,
> 
> >              msg.payload.memory.regions[fd_num].mmap_offset = offset;
> >              fds[fd_num++] = fd;
> >          }
> > -- 
> > 2.14.3
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK