Re: [PATCH v11 15/26] gunyah: rsc_mgr: Add platform ops on mem_lend/mem_reclaim

[Elliot Berman <quic_eberman@xxxxxxxxxxx>]

On 3/21/2023 7:23 AM, Srinivas Kandagatla wrote:
> On 04/03/2023 01:06, Elliot Berman wrote:
> > On Qualcomm platforms, there is a firmware entity which controls access
> > to physical pages. In order to share memory with another VM, this entity
> > needs to be informed that the guest VM should have access to the memory.
> >
> > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@xxxxxxxxxxx>
> > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@xxxxxxxxxxx>
> > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> > ---
> >   drivers/virt/gunyah/Kconfig                 |  4 ++
> >   drivers/virt/gunyah/Makefile                |  1 +
> >   drivers/virt/gunyah/gunyah_platform_hooks.c | 80 +++++++++++++++++++++
> >   drivers/virt/gunyah/rsc_mgr.h               |  3 +
> >   drivers/virt/gunyah/rsc_mgr_rpc.c           | 18 ++++-
> >   include/linux/gunyah_rsc_mgr.h              | 17 +++++
> >   6 files changed, 121 insertions(+), 2 deletions(-)
> >   create mode 100644 drivers/virt/gunyah/gunyah_platform_hooks.c
> >
> > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> > index 1a737694c333..de815189dab6 100644
> > --- a/drivers/virt/gunyah/Kconfig
> > +++ b/drivers/virt/gunyah/Kconfig
> > @@ -4,6 +4,7 @@ config GUNYAH
> >       tristate "Gunyah Virtualization drivers"
> >       depends on ARM64
> >       depends on MAILBOX
> > +    select GUNYAH_PLATFORM_HOOKS
> >       help
> >         The Gunyah drivers are the helper interfaces that run in a 
> > guest VM
> >         such as basic inter-VM IPC and signaling mechanisms, and 
> > higher level
> > @@ -11,3 +12,6 @@ config GUNYAH
> >         Say Y/M here to enable the drivers needed to interact in a Gunyah
> >         virtual environment.
> > +
> > +config GUNYAH_PLATFORM_HOOKS
> > +    tristate
> > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
> > index ff8bc4925392..6b8f84dbfe0d 100644
> > --- a/drivers/virt/gunyah/Makefile
> > +++ b/drivers/virt/gunyah/Makefile
> > @@ -1,6 +1,7 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   obj-$(CONFIG_GUNYAH) += gunyah.o
> > +obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o
> >   gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o vm_mgr.o vm_mgr_mm.o
> >   obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o
> > diff --git a/drivers/virt/gunyah/gunyah_platform_hooks.c 
> > b/drivers/virt/gunyah/gunyah_platform_hooks.c
> > new file mode 100644
> > index 000000000000..60da0e154e98
> > --- /dev/null
> > +++ b/drivers/virt/gunyah/gunyah_platform_hooks.c
> > @@ -0,0 +1,80 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All 
> > rights reserved.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/rwsem.h>
> > +#include <linux/gunyah_rsc_mgr.h>
> > +
> > +#include "rsc_mgr.h"
> > +
> > +static struct gh_rm_platform_ops *rm_platform_ops;
> > +static DECLARE_RWSEM(rm_platform_ops_lock);
> > +
> > +int gh_rm_platform_pre_mem_share(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
>
> I think I have asked this question but I can not find the answer to this 
> from old replies.
> Why is this platform hooks not part of core gunyah? Do we need a 
> dedicated module for this.
> By the looks of APIs I see this is very much close to rm and i think 
> this functionality should live with rm.

If the platform hooks were part of core gunyah, then core gunyah could 
only be enabled as =y in the default defconfig because QCOM_SCM would 
rely on the platform hooks (core gunyah).

With the suggestion to move the SCM platform hooks into separate module, 
I can/will bring the platform hooks back into core gunyah module.

- Elliot

> --srini
> +{
> > +    int ret = 0;
> > +
> > +    down_read(&rm_platform_ops_lock);
> > +    if (rm_platform_ops && rm_platform_ops->pre_mem_share)
> > +        ret = rm_platform_ops->pre_mem_share(rm, mem_parcel);
> > +    up_read(&rm_platform_ops_lock);
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gh_rm_platform_pre_mem_share);
> > +
> > +int gh_rm_platform_post_mem_reclaim(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
> > +{
> > +    int ret = 0;
> > +
> > +    down_read(&rm_platform_ops_lock);
> > +    if (rm_platform_ops && rm_platform_ops->post_mem_reclaim)
> > +        ret = rm_platform_ops->post_mem_reclaim(rm, mem_parcel);
> > +    up_read(&rm_platform_ops_lock);
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gh_rm_platform_post_mem_reclaim);
> > +
> > +int gh_rm_register_platform_ops(struct gh_rm_platform_ops *platform_ops)
> > +{
> > +    int ret = 0;
> > +
> > +    down_write(&rm_platform_ops_lock);
> > +    if (!rm_platform_ops)
> > +        rm_platform_ops = platform_ops;
> > +    else
> > +        ret = -EEXIST;
> > +    up_write(&rm_platform_ops_lock);
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(gh_rm_register_platform_ops);
> > +
> > +void gh_rm_unregister_platform_ops(struct gh_rm_platform_ops 
> > *platform_ops)
> > +{
> > +    down_write(&rm_platform_ops_lock);
> > +    if (rm_platform_ops == platform_ops)
> > +        rm_platform_ops = NULL;
> > +    up_write(&rm_platform_ops_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(gh_rm_unregister_platform_ops);
> > +
> > +static void _devm_gh_rm_unregister_platform_ops(void *data)
> > +{
> > +    gh_rm_unregister_platform_ops(data);
> > +}
> > +
> > +int devm_gh_rm_register_platform_ops(struct device *dev, struct 
> > gh_rm_platform_ops *ops)
> > +{
> > +    int ret;
> > +
> > +    ret = gh_rm_register_platform_ops(ops);
> > +    if (ret)
> > +        return ret;
> > +
> > +    return devm_add_action(dev, _devm_gh_rm_unregister_platform_ops, 
> > ops);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_gh_rm_register_platform_ops);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Gunyah Platform Hooks");
> > diff --git a/drivers/virt/gunyah/rsc_mgr.h 
> > b/drivers/virt/gunyah/rsc_mgr.h
> > index 3665ebc7b020..6838e736f361 100644
> > --- a/drivers/virt/gunyah/rsc_mgr.h
> > +++ b/drivers/virt/gunyah/rsc_mgr.h
> > @@ -13,4 +13,7 @@ struct gh_rm;
> >   int gh_rm_call(struct gh_rm *rsc_mgr, u32 message_id, void 
> > *req_buff, size_t req_buf_size,
> >           void **resp_buf, size_t *resp_buf_size);
> > +int gh_rm_platform_pre_mem_share(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel);
> > +int gh_rm_platform_post_mem_reclaim(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel);
> > +
> >   #endif
> > diff --git a/drivers/virt/gunyah/rsc_mgr_rpc.c 
> > b/drivers/virt/gunyah/rsc_mgr_rpc.c
> > index 3df15ad5b97d..733be4dc8dd2 100644
> > --- a/drivers/virt/gunyah/rsc_mgr_rpc.c
> > +++ b/drivers/virt/gunyah/rsc_mgr_rpc.c
> > @@ -204,6 +204,12 @@ static int gh_rm_mem_lend_common(struct gh_rm 
> > *rm, u32 message_id, struct gh_rm_
> >       if (!msg)
> >           return -ENOMEM;
> > +    ret = gh_rm_platform_pre_mem_share(rm, p);
> > +    if (ret) {
> > +        kfree(msg);
> > +        return ret;
> > +    }
> > +
> >       req_header = msg;
> >       acl_section = (void *)req_header + sizeof(*req_header);
> >       mem_section = (void *)acl_section + struct_size(acl_section, 
> > entries, p->n_acl_entries);
> > @@ -227,8 +233,10 @@ static int gh_rm_mem_lend_common(struct gh_rm 
> > *rm, u32 message_id, struct gh_rm_
> >       ret = gh_rm_call(rm, message_id, msg, msg_size, (void **)&resp, 
> > &resp_size);
> >       kfree(msg);
> > -    if (ret)
> > +    if (ret) {
> > +        gh_rm_platform_post_mem_reclaim(rm, p);
> >           return ret;
> > +    }
> >       p->mem_handle = le32_to_cpu(*resp);
> > @@ -283,8 +291,14 @@ int gh_rm_mem_reclaim(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *parcel)
> >       struct gh_rm_mem_release_req req = {
> >           .mem_handle = cpu_to_le32(parcel->mem_handle),
> >       };
> > +    int ret;
> > +
> > +    ret = gh_rm_call(rm, GH_RM_RPC_MEM_RECLAIM, &req, sizeof(req), 
> > NULL, NULL);
> > +    /* Do not call platform mem reclaim hooks: the reclaim didn't 
> > happen*/
> > +    if (ret)
> > +        return ret;
> > -    return gh_rm_call(rm, GH_RM_RPC_MEM_RECLAIM, &req, sizeof(req), 
> > NULL, NULL);
> > +    return gh_rm_platform_post_mem_reclaim(rm, parcel);
> >   }
> >   /**
> > diff --git a/include/linux/gunyah_rsc_mgr.h 
> > b/include/linux/gunyah_rsc_mgr.h
> > index 8b0b46f28e39..515087931a2b 100644
> > --- a/include/linux/gunyah_rsc_mgr.h
> > +++ b/include/linux/gunyah_rsc_mgr.h
> > @@ -145,4 +145,21 @@ int gh_rm_get_hyp_resources(struct gh_rm *rm, u16 
> > vmid,
> >                   struct gh_rm_hyp_resources **resources);
> >   int gh_rm_get_vmid(struct gh_rm *rm, u16 *vmid);
> > +struct gunyah_rm_platform_ops {
> > +    int (*pre_mem_share)(struct gh_rm *rm, struct gh_rm_mem_parcel 
> > *mem_parcel);
> > +    int (*post_mem_reclaim)(struct gh_rm *rm, struct gh_rm_mem_parcel 
> > *mem_parcel);
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_GUNYAH_PLATFORM_HOOKS)
> > +int gh_rm_register_platform_ops(struct gh_rm_platform_ops 
> > *platform_ops);
> > +void gh_rm_unregister_platform_ops(struct gh_rm_platform_ops 
> > *platform_ops);
> > +int devm_gh_rm_register_platform_ops(struct device *dev, struct 
> > gh_rm_platform_ops *ops);
> > +#else
> > +static inline int gh_rm_register_platform_ops(struct 
> > gh_rm_platform_ops *platform_ops)
> > +    { return 0; }
> > +static inline void gh_rm_unregister_platform_ops(struct 
> > gh_rm_platform_ops *platform_ops) { }
> > +static inline int devm_gh_rm_register_platform_ops(struct device *dev,
> > +    struct gh_rm_platform_ops *ops) { return 0; }
> > +#endif
> > +
> >   #endif