On Mon, Sep 11, 2017 at 12:58:15PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Thu, Aug 24, 2017 at 08:27:17PM +0100, Dr. David Alan Gilbert (git)
> > wrote:
> > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> > >
> > > 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 <dgilb...@redhat.com>
> > > ---
> > > 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 / dgilb...@redhat.com / Manchester, UK
--
Peter Xu
