Re: [Qemu-devel] [RFC v2 19/32] vhost+postcopy: Resolve client address

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Thu, Aug 24, 2017 at 08:27:17PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Resolve fault addresses read off the clients UFD into RAMBlock
> > and offset, and call back to the postcopy code to ask for the page.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  hw/virtio/trace-events |  3 +++
> >  hw/virtio/vhost-user.c | 30 +++++++++++++++++++++++++++++-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index 5067dee19b..f7d4b831fe 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -1,6 +1,9 @@
> >  # See docs/devel/tracing.txt for syntax documentation.
> >  
> >  # hw/virtio/vhost-user.c
> > +vhost_user_postcopy_fault_handler(const char *name, uint64_t 
> > fault_address, int nregions) "%s: @0x%"PRIx64" nregions:%d"
> > +vhost_user_postcopy_fault_handler_loop(int i, uint64_t client_base, 
> > uint64_t size) "%d: client 0x%"PRIx64" +0x%"PRIx64
> > +vhost_user_postcopy_fault_handler_found(int i, uint64_t region_offset, 
> > uint64_t rb_offset) "%d: region_offset: 0x%"PRIx64" rb_offset:0x%"PRIx64
> >  vhost_user_postcopy_listen(void) ""
> >  vhost_user_set_mem_table_postcopy(uint64_t client_addr, uint64_t qhva, int 
> > reply_i, int region_i) "client:0x%"PRIx64" for hva: 0x%"PRIx64" reply %d 
> > region %d"
> >  vhost_user_set_mem_table_withfd(int index, const char *name, uint64_t 
> > memory_size, uint64_t guest_phys_addr, uint64_t userspace_addr, uint64_t 
> > offset) "%d:%s: size:0x%"PRIx64" GPA:0x%"PRIx64" QVA/userspace:0x%"PRIx64" 
> > RB offset:0x%"PRIx64
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index fbe2743298..2897ff70b3 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -816,7 +816,35 @@ out:
> >  static int vhost_user_postcopy_fault_handler(struct PostCopyFD *pcfd,
> >                                               void *ufd)
> >  {
> > -    return 0;
> > +    struct vhost_dev *dev = pcfd->data;
> > +    struct vhost_user *u = dev->opaque;
> > +    struct uffd_msg *msg = ufd;
> > +    uint64_t faultaddr = msg->arg.pagefault.address;
> > +    RAMBlock *rb = NULL;
> > +    uint64_t rb_offset;
> > +    int i;
> > +
> > +    trace_vhost_user_postcopy_fault_handler(pcfd->idstr, faultaddr,
> > +                                            dev->mem->nregions);
> > +    for (i = 0; i < MIN(dev->mem->nregions, u->region_rb_len); i++) {
> 
> Should dev->mem->nregions always the same as u->region_rb_len?

u->region_rb_len only gets updated when vhost_user_set_mem_table is
called, so I think there are short periods of time when they don't
quite match.
(We do have to take some more care than we are at the moment during
updates, because this address resolution happens off the postcopy
thread)

> > +        trace_vhost_user_postcopy_fault_handler_loop(i,
> > +                u->postcopy_client_bases[i], 
> > dev->mem->regions[i].memory_size);
> > +        if (faultaddr >= u->postcopy_client_bases[i]) {
> > +            /* Ofset of the fault address in the vhost region */
> > +            uint64_t region_offset = faultaddr - 
> > u->postcopy_client_bases[i];
> > +            if (region_offset <= dev->mem->regions[i].memory_size) {
> 
> Should be "<" rather than "<="?  Say:
> 
> Region 1: [0, 1M), size 1M
> Region 2: [1M, 2M), size 1M
> 
> Looks like otherwise faultaddr=1M will fall into region 1, while it
> should be region 2?

Fixed; thanks.

> 
> > +                rb_offset = region_offset + u->region_rb_offset[i];
> > +                trace_vhost_user_postcopy_fault_handler_found(i,
> > +                        region_offset, rb_offset);
> > +                rb = u->region_rb[i];
> 
> Nit: this "rb" might be avoided if only used once.

It's only a local, ok if it makes it a little more readable.

Dave

> > +                return postcopy_request_shared_page(pcfd, rb, faultaddr,
> > +                                                    rb_offset);
> > +            }
> > +        }
> > +    }
> > +    error_report("%s: Failed to find region for fault %" PRIx64,
> > +                 __func__, faultaddr);
> > +    return -1;
> >  }
> >  
> >  /*
> > -- 
> > 2.13.5
> > 
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK