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.
>>
>> 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,
>
>> msg.payload.memory.regions[fd_num].mmap_offset = offset;
>> fds[fd_num++] = fd;
>> }
>> --
>> 2.14.3
>>
>
> --
> Peter Xu
>
--
Marc-André Lureau
