* Marc-André Lureau (marcandre.lur...@gmail.com) wrote: > Hi > > On Mon, Mar 12, 2018 at 10:57 AM, Peter Xu <pet...@redhat.com> wrote: > > On Thu, Mar 08, 2018 at 07:57:54PM +0000, Dr. David Alan Gilbert (git) > > wrote: > >> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > >> > >> 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. > > You could add that this does not introduce functional change.
Done. Dave > >> > >> Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > >> --- > >> 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 ee200f703e..311addc33b 100644 > >> --- a/hw/virtio/vhost-user.c > >> +++ b/hw/virtio/vhost-user.c > >> @@ -340,15 +340,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, > >> @@ -372,9 +443,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; > > > > I would still prefer to generalize things out as shared functions, > > since otherwise we are adding somehow duplicate codes, which is IMHO > > harder to maintain. And even if to do the duplication, I would keep > > the original code untouched (so these newlines will still not be > > needed). > > > > But considering that the series has been there for a long time, I'll > > try to be less harsh... And yes, I know it's a burden to maintain > > private trees and keep rebasing. > > > > Feel free to remove the newlines (so at least commit log of those > > lines won't be modified), and either way I'll offer mine: > > > > Reviewed-by: Peter Xu <pet...@redhat.com> > > I have the same feeling as Peter, but otherwise no objection - we can > try to factorize again later if possible: > > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> Thanks Dave > > > > > Thanks, > > > >> msg.payload.memory.regions[fd_num].mmap_offset = offset; > >> fds[fd_num++] = fd; > >> } > >> -- > >> 2.14.3 > >> > > > > -- > > Peter Xu > > > > > > -- > Marc-André Lureau -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK