linux-kernel - Re: [PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to the DSP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <44ec7cce-cd7d-27a3-149b-49fda6308777@linaro.org>
Date:   Tue, 30 May 2023 14:31:49 +0100
From:   Caleb Connolly <caleb.connolly@...aro.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Dylan Van Assche <me@...anvanassche.be>,
        amahesh@....qualcomm.com, arnd@...db.de, gregkh@...uxfoundation.org
Cc:     linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
        dan.carpenter@...aro.org, Ekansh Gupta <quic_ekangupt@...cinc.com>
Subject: Re: [PATCH v4 1/2] misc: fastrpc: support complete DMA pool access to
 the DSP



On 30/05/2023 11:52, Srinivas Kandagatla wrote:
> On 11/05/2023 15:11, Dylan Van Assche wrote:
>> To support FastRPC Context Banks which aren't mapped via the SMMU,
>> make the whole reserved memory region available to the DSP to allow
>> access to coherent buffers.
> 
> Mapping the whole region sounds very inefficient, and also possibly
> making the cma region not usable by others.

Probably I'm missing something obvious here... The downstream driver
maps the whole region, what are the advantages to doing it on a
per-allocation basis?

What are the other users?
> 
>>
> 
> AFAIU SDM845 does not have any context banks for SDSP. All new SoCs
> after 865 have moved to having a context bank.
> 
> For such cases (w/o cb) we can make fastrpc_session_alloc use channel
> context device instead of session ctx device. As this is going to be an
> issue when we try to allocate buffers dynamically for that cb.

Right.. This is what patch 2 does, but presumably the ALLOC_DMA_BUF,
MMAP, etc ioctls won't work with the current iteration?

> 
> In the newer platforms (from 865) there is support for iommu and context
> banks on SDSP, so the existing code flow is identical for both ADSP and
> SDSP.
> 
> 
> We should be careful not to break newer platfroms while adding support
> to this.
> 
> Both myself and Ekansh thought about this and see that the better way to
> add support to this is by
> 
> 1. extend fastrpc_session_alloc() to support zero context banks.
> 
> 2. add flags to mark this and allocate meta data using secure allocation
> when its required based on this flag.
> 
> 3.  buffer allocation can either go with 2 or with a new flag coming
> from userspace.

This sounds pretty good to me!
> 
> 
> 
>> This is performed by assigning the memory to the DSP via a hypervisor
>> call to set the correct permissions for the Virtual Machines on the DSP.
>> This is only necessary when a memory region is provided for SLPI DSPs
>> so guard this with a domain ID check.
>>
>> Signed-off-by: Dylan Van Assche <me@...anvanassche.be>
>> Reviewed-by: Caleb Connolly <caleb.connolly@...aro.org>
>> ---
>>   drivers/misc/fastrpc.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index f48466960f1b..1ced553ae959 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -2231,6 +2231,8 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>       int i, err, domain_id = -1, vmcount;
>>       const char *domain;
>>       bool secure_dsp;
>> +    struct device_node *rmem_node;
>> +    struct reserved_mem *rmem;
>>       unsigned int vmids[FASTRPC_MAX_VMIDS];
>>         err = of_property_read_string(rdev->of_node, "label", &domain);
>> @@ -2274,6 +2276,19 @@ static int fastrpc_rpmsg_probe(struct
>> rpmsg_device *rpdev)
>>           }
>>       }
>>   +    rmem_node = of_parse_phandle(rdev->of_node, "memory-region", 0);
>> +    if (domain_id == SDSP_DOMAIN_ID && rmem_node) {
>> +        rmem = of_reserved_mem_lookup(rmem_node);
>> +        if (!rmem) {
>> +            err = -EINVAL;
>> +            goto fdev_error;
>> +        }
>> +
>> +        qcom_scm_assign_mem(rmem->base, rmem->size, &data->perms,
>> +                    data->vmperms, data->vmcount);
> 
> vmperms need to be a bit field.
> 
>> +
>> +    }
>> +
>>       secure_dsp = !(of_property_read_bool(rdev->of_node,
>> "qcom,non-secure-domain"));
>>       data->secure = secure_dsp;
>>   
> 
> --srini

-- 
// Caleb (they/them)