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

[Peter Xu]

On Mon, Sep 11, 2017 at 12:58:15PM +0100, Dr. David Alan Gilbert wrote:
> * 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)

I see, so memory layout can change along the way...

But I still doubt whether this single MIN() can work.

Say, we have these arrays already:

- array A: dev->mem->regions[]
- array B: u->region_rb[]
- array C: u->postcopy_client_bases[]

These arrays should always be aligned with each other (index "i" of
array "A/B/C" will always describe the same memory region).  But since
we can change the memory layout dynamically during postcopy, then
array A can grow/shrink/change in following path:

  vhost_region_{add|delete}
    updates array A              (1)
  vhost_region_{add|delete}
    updates array A              (2)
  vhost_region_{add|delete}
    updates array A              (3)
  ...
  vhost_commit
    vhost_set_mem_table
      align arrays B/C with A    (4)

IMHO array A may not really match B/C during step (1)-(3), until step
(4) to re-align them?  And if they are not aligned with each other, I
guess a single MIN() won't help much? (Since the indexing below would
be problematic?)

(Hmm, can we just disallow memory change during postcopy for now?)

> 
> > > +        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]) {

Ah, wait...

postcopy_client_bases[] is now defined with static size
VHOST_MEMORY_MAX_NREGIONS.  Shouldn't it be dynamically allocated as
well with dev->mem->nregions, just like vhost_user.region_rb[]?

Maybe we want to leave the postcopy_client_bases[i] be zeros when
dev->mem->regions[i] it's not a vhost-user supported region (without
"fd")?

> > > +            /* 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

-- 
Peter Xu