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.
>
>
> > int (*req_notify_cq)(struct ibv_cq *cq, int solicited_only);
> > int (*rereg_mr)(struct verbs_mr *vmr, int flags, struct ibv_pd *pd,
> > void *addr, size_t length, int access);
> > diff --git a/libibverbs/dummy_ops.c b/libibverbs/dummy_ops.c
> > index c861c3a0..aab61a17 100644
> > +++ b/libibverbs/dummy_ops.c
> > @@ -416,6 +416,14 @@ static struct ibv_mr *reg_mr(struct ibv_pd *pd, void *addr, size_t length,
> > return NULL;
> > }
> >
> > +static struct ibv_mr *reg_mr_virt_as(struct ibv_pd *pd, void *addr,
> > + size_t length, uint64_t hca_va,
> > + int access)
> > +{
> > + errno = ENOSYS;
>
> > + return NULL;
> > +}
> > +
> > static int req_notify_cq(struct ibv_cq *cq, int solicited_only)
> > {
> > return ENOSYS;
> > @@ -508,6 +516,7 @@ const struct verbs_context_ops verbs_dummy_ops = {
> > read_counters,
> > reg_dm_mr,
> > reg_mr,
> > + reg_mr_virt_as,
> > req_notify_cq,
> > rereg_mr,
> > resize_cq,
> > @@ -623,6 +632,7 @@ void verbs_set_ops(struct verbs_context *vctx,
> > SET_PRIV_OP(ctx, query_srq);
> > SET_OP(vctx, reg_dm_mr);
> > SET_PRIV_OP(ctx, reg_mr);
> > + SET_OP(vctx, reg_mr_virt_as);
> > SET_OP(ctx, req_notify_cq);
> > SET_PRIV_OP(ctx, rereg_mr);
> > SET_PRIV_OP(ctx, resize_cq);
> > 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.
>
> Jason
