Re: [PATCH v11 16/26] firmware: qcom_scm: Register Gunyah platform ops

[Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>]

On 21/03/2023 18:40, Elliot Berman wrote:
> On 3/21/2023 7:24 AM, Srinivas Kandagatla wrote:
> > Hi Elliot,
> >
> > On 04/03/2023 01:06, Elliot Berman wrote:
> > > Qualcomm platforms have a firmware entity which performs access control
> > > to physical pages. Dynamically started Gunyah virtual machines use the
> > > QCOM_SCM_RM_MANAGED_VMID for access. Linux thus needs to assign access
> > > to the memory used by guest VMs. Gunyah doesn't do this operation for us
> > > since it is the current VM (typically VMID_HLOS) delegating the access
> > > and not Gunyah itself. Use the Gunyah platform ops to achieve this so
> > > that only Qualcomm platforms attempt to make the needed SCM calls.
> > >
> > > 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/firmware/Kconfig       |   2 +
> > >   drivers/firmware/qcom_scm.c    | 100 +++++++++++++++++++++++++++++++++
> > >   include/linux/gunyah_rsc_mgr.h |   2 +-
> > >   3 files changed, 103 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > > index b59e3041fd62..b888068ff6f2 100644
> > > --- a/drivers/firmware/Kconfig
> > > +++ b/drivers/firmware/Kconfig
> > > @@ -214,6 +214,8 @@ config MTK_ADSP_IPC
> > >   config QCOM_SCM
> > >       tristate
> > > +    select VIRT_DRIVERS
> > > +    select GUNYAH_PLATFORM_HOOKS
> >
> > I still have concerns with this selects in Kconfig on older Qualcomm 
> > platforms that use SCM and do not have GUNYAH.
> >
> > In our last discussing you mentioned the requirement for
> > "CONFIG_GUNYAH=y and CONFIG_QCOM_SCM=m"
> >
> > I think that should be doable and remove selecting if you can make a 
> > separate GUNYAH_QCOM_PLATFORM_HOOKS driver
> >
> > Does this work?
>
> This works for Android and all the Qualcomm vendor (downstream) 
> platforms where we can explicitly load modules. I don't think this 
> module would be implicitly loaded by any kernel mechanism.

We could also load this module based on UUID match at the gunyah core 
level too, if that helps.


--srini

> >  >----------------------->cut<-------------------------------
> >  From 1fb7995aecf17caefd09ffb516579bc4ac9ac301 Mon Sep 17 00:00:00 2001
> > From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > Date: Tue, 21 Mar 2023 13:34:02 +0000
> > Subject: [PATCH] virt: gunyah: add qcom platform hooks
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >   drivers/firmware/Kconfig                      |  2 --
> >   drivers/firmware/qcom_scm.c                   | 14 +++-----
> >   drivers/virt/gunyah/Kconfig                   |  5 +++
> >   drivers/virt/gunyah/Makefile                  |  1 +
> >   .../virt/gunyah/gunyah_qcom_platform_hooks.c  | 35 +++++++++++++++++++
> >   include/linux/firmware/qcom/qcom_scm.h        |  3 ++
> >   6 files changed, 48 insertions(+), 12 deletions(-)
> >   create mode 100644 drivers/virt/gunyah/gunyah_qcom_platform_hooks.c
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index b888068ff6f2..b59e3041fd62 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -214,8 +214,6 @@ config MTK_ADSP_IPC
> >
> >   config QCOM_SCM
> >       tristate
> > -    select VIRT_DRIVERS
> > -    select GUNYAH_PLATFORM_HOOKS
> >
> >   config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> >       bool "Qualcomm download mode enabled by default"
> > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > index 5273cf64ee2a..194ea2bc9a1d 100644
> > --- a/drivers/firmware/qcom_scm.c
> > +++ b/drivers/firmware/qcom_scm.c
> > @@ -1301,7 +1301,7 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 
> > payload_reg, u32 payload_val,
> >   }
> >   EXPORT_SYMBOL(qcom_scm_lmh_dcvsh);
> >
> > -static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
> > +int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel *mem_parcel)
> >   {
> >       struct qcom_scm_vmperm *new_perms;
> >       u64 src, src_cpy;
> > @@ -1359,8 +1359,9 @@ static int qcom_scm_gh_rm_pre_mem_share(struct 
> > gh_rm *rm, struct gh_rm_mem_parce
> >       kfree(new_perms);
> >       return ret;
> >   }
> > +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_pre_mem_share);
> >
> > -static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
> > +int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel *mem_parcel)
> >   {
> >       struct qcom_scm_vmperm new_perms;
> >       u64 src = 0, src_cpy;
> > @@ -1388,11 +1389,7 @@ static int 
> > qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct gh_rm_mem_pa
> >
> >       return ret;
> >   }
> > -
> > -static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = {
> > -    .pre_mem_share = qcom_scm_gh_rm_pre_mem_share,
> > -    .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim,
> > -};
> > +EXPORT_SYMBOL_GPL(qcom_scm_gh_rm_post_mem_reclaim);
> >
> >   static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
> >   {
> > @@ -1597,9 +1594,6 @@ static int qcom_scm_probe(struct platform_device 
> > *pdev)
> >       if (download_mode)
> >           qcom_scm_set_download_mode(true);
> >
> > -    if (devm_gh_rm_register_platform_ops(&pdev->dev, 
> > &qcom_scm_gh_rm_platform_ops))
> > -        dev_warn(__scm->dev, "Gunyah RM platform ops were already 
> > registered\n");
> > -
> >       return 0;
> >   }
> >
> > diff --git a/drivers/virt/gunyah/Kconfig b/drivers/virt/gunyah/Kconfig
> > index bd8e31184962..a9c48d6518f7 100644
> > --- a/drivers/virt/gunyah/Kconfig
> > +++ b/drivers/virt/gunyah/Kconfig
> > @@ -16,6 +16,11 @@ config GUNYAH
> >   config GUNYAH_PLATFORM_HOOKS
> >       tristate
> >
> > +config GUNYAH_QCOM_PLATFORM_HOOKS
> > +    tristate "Gunyah Platform hooks for Qualcomm"
> > +        depends on ARCH_QCOM && QCOM_SCM
> > +    depends on GUNYAH
> > +
> >   config GUNYAH_VCPU
> >       tristate "Runnable Gunyah vCPUs"
> >       depends on GUNYAH
> > diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile
> > index 7347b1470491..c33f701bb5c8 100644
> > --- a/drivers/virt/gunyah/Makefile
> > +++ b/drivers/virt/gunyah/Makefile
> > @@ -2,6 +2,7 @@
> >
> >   obj-$(CONFIG_GUNYAH) += gunyah.o
> >   obj-$(CONFIG_GUNYAH_PLATFORM_HOOKS) += gunyah_platform_hooks.o
> > +obj-$(CONFIG_GUNYAH_QCOM_PLATFORM_HOOKS) += gunyah_qcom_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_qcom_platform_hooks.c 
> > b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c
> > new file mode 100644
> > index 000000000000..3332f84134d3
> > --- /dev/null
> > +++ b/drivers/virt/gunyah/gunyah_qcom_platform_hooks.c
> > @@ -0,0 +1,35 @@
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/firmware/qcom/qcom_scm.h>
> > +#include <linux/gunyah_rsc_mgr.h>
> > +
> > +static int qcom_gh_rm_pre_mem_share(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
> > +{
> > +    return qcom_scm_gh_rm_pre_mem_share(mem_parcel);
> > +}
> > +
> > +static int qcom_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct 
> > gh_rm_mem_parcel *mem_parcel)
> > +{
> > +    return qcom_scm_gh_rm_post_mem_reclaim(mem_parcel);
> > +}
> > +
> > +static struct gh_rm_platform_ops qcom_gh_platform_hooks_ops = {
> > +    .pre_mem_share = qcom_gh_rm_pre_mem_share,
> > +    .post_mem_reclaim = qcom_gh_rm_post_mem_reclaim,
> > +};
> > +
> > +static int __init qcom_gh_platform_hooks_register(void)
> > +{
> > +    return gh_rm_register_platform_ops(&qcom_gh_platform_hooks_ops);
> > +}
> > +
> > +static void __exit qcom_gh_platform_hooks_unregister(void)
> > +{
> > +    gh_rm_unregister_platform_ops(&qcom_gh_platform_hooks_ops);
> > +}
> > +
> > +module_init(qcom_gh_platform_hooks_register);
> > +module_exit(qcom_gh_platform_hooks_unregister);
> > +
> > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Gunyah Platform Hooks 
> > driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/firmware/qcom/qcom_scm.h 
> > b/include/linux/firmware/qcom/qcom_scm.h
> > index 1e449a5d7f5c..9b0d33db803d 100644
> > --- a/include/linux/firmware/qcom/qcom_scm.h
> > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > @@ -121,5 +121,8 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 
> > payload_reg, u32 payload_val,
> >                     u64 limit_node, u32 node_id, u64 version);
> >   extern int qcom_scm_lmh_profile_change(u32 profile_id);
> >   extern bool qcom_scm_lmh_dcvsh_available(void);
> > +struct gh_rm_mem_parcel;
> > +extern int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm_mem_parcel 
> > *mem_parcel);
> > +extern int qcom_scm_gh_rm_pre_mem_share(struct gh_rm_mem_parcel 
> > *mem_parcel);
> >
> >   #endif
> > --------------------------->cut<-----------------------
> >
> > >   config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> > >       bool "Qualcomm download mode enabled by default"
> > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> > > index b95616b35bff..89a261a9e021 100644
> > > --- a/drivers/firmware/qcom_scm.c
> > > +++ b/drivers/firmware/qcom_scm.c
> > > @@ -20,6 +20,7 @@
> > >   #include <linux/clk.h>
> > >   #include <linux/reset-controller.h>
> > >   #include <linux/arm-smccc.h>
> > > +#include <linux/gunyah_rsc_mgr.h>
> > >   #include "qcom_scm.h"
> > > @@ -30,6 +31,9 @@ module_param(download_mode, bool, 0);
> > >   #define SCM_HAS_IFACE_CLK    BIT(1)
> > >   #define SCM_HAS_BUS_CLK        BIT(2)
> > > +#define QCOM_SCM_RM_MANAGED_VMID    0x3A
> > > +#define QCOM_SCM_MAX_MANAGED_VMID    0x3F
> > > +
> > >   struct qcom_scm {
> > >       struct device *dev;
> > >       struct clk *core_clk;
> > > @@ -1299,6 +1303,99 @@ int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 
> > > payload_reg, u32 payload_val,
> > >   }
> > >   EXPORT_SYMBOL(qcom_scm_lmh_dcvsh);
> > > +static int qcom_scm_gh_rm_pre_mem_share(struct gh_rm *rm, struct 
> > > gh_rm_mem_parcel *mem_parcel)
> > > +{
> > > +    struct qcom_scm_vmperm *new_perms;
> > > +    u64 src, src_cpy;
> > > +    int ret = 0, i, n;
> > > +    u16 vmid;
> > > +
> > > +    new_perms = kcalloc(mem_parcel->n_acl_entries, 
> > > sizeof(*new_perms), GFP_KERNEL);
> > > +    if (!new_perms)
> > > +        return -ENOMEM;
> > > +
> > > +    for (n = 0; n < mem_parcel->n_acl_entries; n++) {
> > > +        vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid);
> > > +        if (vmid <= QCOM_SCM_MAX_MANAGED_VMID)
> > > +            new_perms[n].vmid = vmid;
> > > +        else
> > > +            new_perms[n].vmid = QCOM_SCM_RM_MANAGED_VMID;
> > > +        if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_X)
> > > +            new_perms[n].perm |= QCOM_SCM_PERM_EXEC;
> > > +        if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_W)
> > > +            new_perms[n].perm |= QCOM_SCM_PERM_WRITE;
> > > +        if (mem_parcel->acl_entries[n].perms & GH_RM_ACL_R)
> > > +            new_perms[n].perm |= QCOM_SCM_PERM_READ;
> > > +    }
> > > +
> > > +    src = (1ull << QCOM_SCM_VMID_HLOS);
> > > +
> > > +    for (i = 0; i < mem_parcel->n_mem_entries; i++) {
> > > +        src_cpy = src;
> > > +        ret = 
> > > qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base),
> > > +                        le64_to_cpu(mem_parcel->mem_entries[i].size),
> > > +                        &src_cpy, new_perms, 
> > > mem_parcel->n_acl_entries);
> > > +        if (ret) {
> > > +            src = 0;
> > > +            for (n = 0; n < mem_parcel->n_acl_entries; n++) {
> > > +                vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid);
> > > +                if (vmid <= QCOM_SCM_MAX_MANAGED_VMID)
> > > +                    src |= (1ull << vmid);
> > > +                else
> > > +                    src |= (1ull << QCOM_SCM_RM_MANAGED_VMID);
> > > +            }
> > > +
> > > +            new_perms[0].vmid = QCOM_SCM_VMID_HLOS;
> > > +
> > > +            for (i--; i >= 0; i--) {
> > > +                src_cpy = src;
> > > +                WARN_ON_ONCE(qcom_scm_assign_mem(
> > > + le64_to_cpu(mem_parcel->mem_entries[i].ipa_base),
> > > +                        le64_to_cpu(mem_parcel->mem_entries[i].size),
> > > +                        &src_cpy, new_perms, 1));
> > > +            }
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    kfree(new_perms);
> > > +    return ret;
> > > +}
> > > +
> > > +static int qcom_scm_gh_rm_post_mem_reclaim(struct gh_rm *rm, struct 
> > > gh_rm_mem_parcel *mem_parcel)
> > > +{
> > > +    struct qcom_scm_vmperm new_perms;
> > > +    u64 src = 0, src_cpy;
> > > +    int ret = 0, i, n;
> > > +    u16 vmid;
> > > +
> > > +    new_perms.vmid = QCOM_SCM_VMID_HLOS;
> > > +    new_perms.perm = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | 
> > > QCOM_SCM_PERM_READ;
> > > +
> > > +    for (n = 0; n < mem_parcel->n_acl_entries; n++) {
> > > +        vmid = le16_to_cpu(mem_parcel->acl_entries[n].vmid);
> > > +        if (vmid <= QCOM_SCM_MAX_MANAGED_VMID)
> > > +            src |= (1ull << vmid);
> > > +        else
> > > +            src |= (1ull << QCOM_SCM_RM_MANAGED_VMID);
> > > +    }
> > > +
> > > +    for (i = 0; i < mem_parcel->n_mem_entries; i++) {
> > > +        src_cpy = src;
> > > +        ret = 
> > > qcom_scm_assign_mem(le64_to_cpu(mem_parcel->mem_entries[i].ipa_base),
> > > +                        le64_to_cpu(mem_parcel->mem_entries[i].size),
> > > +                        &src_cpy, &new_perms, 1);
> > > +        WARN_ON_ONCE(ret);
> > > +    }
> > > +
> > > +    return ret;
> > > +}
> > > +
> > > +static struct gh_rm_platform_ops qcom_scm_gh_rm_platform_ops = {
> > > +    .pre_mem_share = qcom_scm_gh_rm_pre_mem_share,
> > > +    .post_mem_reclaim = qcom_scm_gh_rm_post_mem_reclaim,
> > > +};
> > > +
> > >   static int qcom_scm_find_dload_address(struct device *dev, u64 *addr)
> > >   {
> > >       struct device_node *tcsr;
> > > @@ -1502,6 +1599,9 @@ static int qcom_scm_probe(struct 
> > > platform_device *pdev)
> > >       if (download_mode)
> > >           qcom_scm_set_download_mode(true);
> > > +    if (devm_gh_rm_register_platform_ops(&pdev->dev, 
> > > &qcom_scm_gh_rm_platform_ops))
> > > +        dev_warn(__scm->dev, "Gunyah RM platform ops were already 
> > > registered\n");
> > > +
> > >       return 0;
> > >   }
> > > diff --git a/include/linux/gunyah_rsc_mgr.h 
> > > b/include/linux/gunyah_rsc_mgr.h
> > > index 515087931a2b..acf8c1545a6c 100644
> > > --- a/include/linux/gunyah_rsc_mgr.h
> > > +++ b/include/linux/gunyah_rsc_mgr.h
> > > @@ -145,7 +145,7 @@ 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 {
> > > +struct gh_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);
> > >   };