On Mon, May 27, 2019 at 03:18:39PM +0300, Yuval Shaia wrote:
> On Mon, May 27, 2019 at 09:11:34AM -0300, Jason Gunthorpe wrote:
> > On Sun, May 26, 2019 at 11:02:24AM +0300, Yuval Shaia wrote:
> > > The virtual address that is registered is used as a base for any address
> > > used later in post_recv and post_send operations.
> > >
> > > On a virtualised environment this is not correct.
> > >
> > > A guest cannot register its memory so hypervisor maps the guest physical
> > > address to a host virtual address and register it with the HW. Later on,
> > > at datapath phase, the guest fills the SGEs with addresses from its
> > > address space.
> > > Since HW cannot access guest virtual address space an extra translation
> > > is needed map those addresses to be based on the host virtual address
> > > that was registered with the HW.
> > >
> > > To avoid this, a logical separation between the address that is
> > > registered and the address that is used as a offset at datapath phase is
> > > needed.
> > > This separation is already implemented in the lower layer part
> > > (ibv_cmd_reg_mr) but blocked at the API level.
> > >
> > > Fix it by introducing a new API function that accepts a address from
> > > guest virtual address space as well.
> > >
> > > Signed-off-by: Yuval Shaia <yuval.shaia@xxxxxxxxxx>
> > > libibverbs/driver.h | 3 +++
> > > libibverbs/dummy_ops.c | 10 ++++++++++
> > > libibverbs/verbs.h | 26 ++++++++++++++++++++++++++
> > > providers/rxe/rxe.c | 16 ++++++++++++----
> > > 4 files changed, 51 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/libibverbs/driver.h b/libibverbs/driver.h
> > > index e4d624b2..73bc10e6 100644
> > > +++ b/libibverbs/driver.h
> > > @@ -339,6 +339,9 @@ struct verbs_context_ops {
> > > unsigned int access);
> > > struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr, size_t length,
> > > int access);
> > > + struct ibv_mr *(*reg_mr_virt_as)(struct ibv_pd *pd, void *addr,
> > > + size_t length, uint64_t hca_va,
> > > + int access);
> >
> > I don't want to see a new entry point, all HW already supports it, so
> > we should just add the hca_va to the main one and remove the
> > assumption that the void *addr should be used as the hca_va from the
> > drivers.
>
> So it is better to change the reg_mr signature? That would break API, i.e.
> all apps that are using reg_mr would have to be changed accordingly.
> I'm a newbie here so i might be talking nonsense.
driver.h is not ABI
> > > diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
> > > index cb2d8439..8bcc8388 100644
> > > +++ b/libibverbs/verbs.h
> > > @@ -2037,6 +2037,9 @@ struct verbs_context {
> > > struct ibv_mr *(*reg_dm_mr)(struct ibv_pd *pd, struct ibv_dm *dm,
> > > uint64_t dm_offset, size_t length,
> > > unsigned int access);
> > > + struct ibv_mr *(*reg_mr_virt_as)(struct ibv_pd *pd, void *addr,
> > > + size_t length, uint64_t hca_va,
> > > + int access);
> >
> > Can't add new functions here, breaks the ABI
>
> My assumption was that it is better to add new function than to change an
> existing function's signature.
verbs.h is ABI, so you have to change it by growing the structs, not
adding things in the middle.
Probably the best thing for an API like this is to just add a new
normally linked public function instead of using function pointers
here.
Jason
