Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

9 participants
3614 discussions

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Nicolas Dufresne

Le mardi 04 mars 2025 à 13:15 +0530, Sumit Garg a écrit : > On Tue, Mar 04, 2025 at 08:17:23AM +0100, Jens Wiklander wrote: > > Hi Daniel, > > > > On Fri, Feb 21, 2025 at 3:12 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > Hi Sumit, > > > > > > On Fri, 21 Feb 2025 at 11:24, Sumit Garg <sumit.garg(a)linaro.org> wrote: > > > > On Tue, 18 Feb 2025 at 21:52, Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > dma-heaps was created to solve the problem of having too many > > > > > 'allocate $n bytes from $specialplace' uAPIs. The proliferation was > > > > > painful and making it difficult for userspace to do what it needed to > > > > > do. Userspace doesn't _yet_ make full use of it, but the solution is > > > > > to make userspace make full use of it, not to go create entirely > > > > > separate allocation paths for unclear reasons. > > > > > > > > > > Besides, I'm writing this from a platform that implements SVP not via > > > > > TEE. I've worked on platforms which implement SVP without any TEE, > > > > > where the TEE implementation would be at best a no-op stub, and at > > > > > worst flat-out impossible. > > > > > > > > Can you elaborate the non-TEE use-case for Secure Video Path (SVP) a > > > > bit more? As to how the protected/encrypted media content pipeline > > > > works? Which architecture support does your use-case require? Is there > > > > any higher privileged level firmware interaction required to perform > > > > media content decryption into restricted memory? Do you plan to > > > > upstream corresponding support in near future? > > > > > > You can see the MTK SVP patches on list which use the MTK SMC to mediate it. > > > > > > There are TI Jacinto platforms which implement a 'secure' area > > > configured statically by (IIRC) BL2, with static permissions defined > > > for each AXI endpoint, e.g. CPU write + codec RW + dispc read. I've > > > heard of another SoC vendor doing the same, but I don't think I can > > > share those details. There is no TEE interaction. > > > > > > I'm writing this message from an AMD laptop which implements > > > restricted content paths outside of TEE. I don't have the full picture > > > of how SVP is implemented on AMD systems, but I do know that I don't > > > have any TEE devices exposed. > > > > > > > Let me try to elaborate on the Secure Video Path (SVP) flow requiring > > > > a TEE implementation (in general terms a higher privileged firmware > > > > managing the pipeline as the kernel/user-space has no access > > > > permissions to the plain text media content): > > > > > > > > - [...] > > > > > > Yeah, I totally understand the TEE usecase. I think that TEE is a good > > > design to implement this. I think that TEE should be used for SVP > > > where it makes sense. > > > > > > Please understand that I am _not_ arguing that no-one should use TEE for SVP! > > > > > > > > So, again, let's > > > > > please turn this around: _why_ TEE? Who benefits from exposing this as > > > > > completely separate to the more generic uAPI that we specifically > > > > > designed to handle things like this? > > > > > > > > The bridging between DMA heaps and TEE would still require user-space > > > > to perform an IOCTL into TEE to register the DMA-bufs as you can see > > > > here [1]. Then it will rather be two handles for user-space to manage. > > > > > > Yes, the decoder would need to do this. That's common though: if you > > > want to share a buffer between V4L2 and DRM, you have three handles: > > > the V4L2 buffer handle, the DRM GEM handle, and the dmabuf you use to > > > bridge the two. > > > > > > > Similarly during restricted memory allocation/free we need another > > > > glue layer under DMA heaps to TEE subsystem. > > > > > > Yep. > > > > > > > The reason is simply which has been iterated over many times in the > > > > past threads that: > > > > > > > > "If user-space has to interact with a TEE device for SVP use-case > > > > then why it's not better to ask TEE to allocate restricted DMA-bufs > > > > too" > > > > > > The first word in your proposition is load-bearing. > > > > > > Build out the usecase a little more here. You have a DRMed video > > > stream coming in, which you need to decode (involving TEE for this > > > usecase). You get a dmabuf handle to the decoded frame. You need to > > > pass the dmabuf across to the Wayland compositor. The compositor needs > > > to pass it to EGL/Vulkan to import and do composition, which in turn > > > passes it to the GPU DRM driver. The output of the composition is in > > > turn shared between the GPU DRM driver and the separate KMS DRM > > > driver, with the involvement of GBM. > > > > > > For the platforms I'm interested in, the GPU DRM driver needs to > > > switch into protected mode, which has no involvement at all with TEE - > > > it's architecturally impossible to have TEE involved without moving > > > most of the GPU driver into TEE and destroying performance. The > > > display hardware also needs to engage protected mode, which again has > > > no involvement with TEE and again would need to have half the driver > > > moved into TEE for no benefit in order to do so. The Wayland > > > compositor also has no interest in TEE: it tells the GPU DRM driver > > > about the protected status of its buffers, and that's it. > > > > > > What these components _are_ opinionated about, is the way buffers are > > > allocated and managed. We built out dmabuf modifiers for this usecase, > > > and we have a good negotiation protocol around that. We also really > > > care about buffer placement in some usecases - e.g. some display/codec > > > hardware requires buffers to be sourced from contiguous memory, other > > > hardware needs to know that when it shares buffers with another > > > device, it needs to place the buffers outside of inaccessible/slow > > > local RAM. So we built out dma-heaps, so every part of the component > > > in the stack can communicate their buffer-placement needs in the same > > > way as we do modifiers, and negotiate an acceptable allocation. > > > > > > That's my starting point for this discussion. We have a mechanism to > > > deal with the fact that buffers need to be shared between different IP > > > blocks which have their own constraints on buffer placement, avoiding > > > the current problem of having every subsystem reinvent their own > > > allocation uAPI which was burying us in impedance mismatch and > > > confusion. That mechanism is dma-heaps. It seems like your starting > > > point from this discussion is that you've implemented a TEE-centric > > > design for SVP, and so all of userspace should bypass our existing > > > cross-subsystem special-purpose allocation mechanism, and write > > > specifically to one implementation. I believe that is a massive step > > > backwards and an immediate introduction of technical debt. > > > > > > Again, having an implementation of SVP via TEE makes a huge amount of > > > sense. Having _most_ SVP implementations via TEE still makes a lot of > > > sense. Having _all_ SVP implementations eventually be via TEE would > > > still make sense. But even if we were at that point - which we aren't > > > - it still doesn't justify telling userspace 'use the generic dma-heap > > > uAPI for every device-specific allocation constraint, apart from SVP > > > which has a completely different way to allocate some bytes'. > > > > I must admit that I don't see how this makes a significant difference, > > but then I haven't hacked much in the stacks you're talking about, so > > I'm going to take your word for it. > > > > I've experimented with providing a dma-heap replacing the TEE API. The > > implementation is more complex than I first anticipated, adding about > > 400 lines to the patch set. > > I did anticipated this but let's give it a try and see if DMA heaps > really adds any value from user-space point of view. If it does then it > will be worth the maintenence overhead. > > > From user space, it looks like another > > dma-heap. I'm using the names you gave earlier, > > protected,secure-video, protected,trusted-ui, and > > protected,secure-video-record. However, I wonder if we shouldn't use > > "restricted" instead of "protected" since we had agreed to call it > > restricted memory earlier. > > Let's stick with "restricted" memory buffer references only. Until now, we didn't have a standard to balance our naming choice, we simply wanted to move away from "secure" which didn't mean much, and restricted met our needs. I think the discussion is worth having again, now that there is a standard that decided toward "protected". Matchcing the Khronos standard means reducing a lot of confusion. https://docs.vulkan.org/guide/latest/protected.html regards, Nicolas

1 year, 6 months

Re: [RFC PATCH 08/12] vfio/pci: Create host unaccessible dma-buf for private device

by Jason Gunthorpe

On Mon, Jan 20, 2025 at 08:45:51PM +1100, Alexey Kardashevskiy wrote: > > For CC I'm expecting the KVM fd to be the handle for the cVM, so any > > RPCs that want to call into the secure world need the KVM FD to get > > the cVM's identifier. Ie a "bind to cVM" RPC will need the PCI > > information and the cVM's handle. > > And keep KVM fd open until unbind? Or just for the short time to call the > PSP? iommufd will keep the KVM fd alive so long as the vIOMMU object exists. Other uses for kvm require it to work like this. > > But it also seems to me that VFIO should be able to support putting > > the device into the RUN state without involving KVM or cVMs. > > AMD's TDI bind handler in the PSP wants a guest handle ("GCTX") and a guest > device BDFn, and VFIO has no desire to dive into this KVM business beyond > IOMMUFD. As in my other email, VFIO is not restricted to running VMs, useful things should be available to apps like DPDK. There is a use case for using TDISP and getting devices up into an ecrypted/attested state on pure bare metal without any KVM, VFIO should work in that use case too. Jason

1 year, 6 months

Re: [PATCH] Replace nested max() with single max3()

by Christian König

Am 15.03.25 um 05:40 schrieb feng.wei8(a)zte.com.cn: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. Please add a "dma-buf: heaps: " prefix to the subject line, apart from that looks good to me. Regards, Christian. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) {

1 year, 6 months

Re: [PATCH v4] dma-buf: heap: Replace nested max() with single max3()

by Krzysztof Kozlowski

On 17/03/2025 03:37, feng.wei8(a)zte.com.cn wrote: > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> > --- > v3 -> v4 > fix the format of this patch. > drivers/dma-buf/dma-heap.c | 2 +- You sent five versions per day of the same patch. Look what was in v3: v2 -> v3 fix the format of this patch So you are doing the same over and over and sending it to us? Srsly, ZTE, slow down and be sure you follow the process BEFORE you send flood of patches like that and learn on the go. Best regards, Krzysztof

1 year, 6 months

Re: [PATCH v6 03/10] optee: account for direction while converting parameters

by Jens Wiklander

Hi Sumit, On Thu, Mar 13, 2025 at 11:41 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > Hi Jens, > > On Wed, Mar 05, 2025 at 02:04:09PM +0100, Jens Wiklander wrote: > > The OP-TEE backend driver has two internal function pointers to convert > > between the subsystem type struct tee_param and the OP-TEE type struct > > optee_msg_param. > > > > The conversion is done from one of the types to the other, which is then > > involved in some operation and finally converted back to the original > > type. When converting to prepare the parameters for the operation, all > > fields must be taken into account, but then converting back, it's enough > > to update only out-values and out-sizes. So, an update_out parameter is > > added to the conversion functions to tell if all or only some fields > > must be copied. > > > > This is needed in a later patch where it might get confusing when > > converting back in from_msg_param() callback since an allocated > > restricted SHM can be using the sec_world_id of the used restricted > > memory pool and that doesn't translate back well. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/call.c | 10 ++-- > > drivers/tee/optee/ffa_abi.c | 43 +++++++++++++---- > > drivers/tee/optee/optee_private.h | 42 +++++++++++------ > > drivers/tee/optee/rpc.c | 31 +++++++++---- > > drivers/tee/optee/smc_abi.c | 76 +++++++++++++++++++++++-------- > > 5 files changed, 144 insertions(+), 58 deletions(-) > > > > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c > > index 16eb953e14bb..f1533b894726 100644 > > --- a/drivers/tee/optee/call.c > > +++ b/drivers/tee/optee/call.c > > @@ -400,7 +400,8 @@ int optee_open_session(struct tee_context *ctx, > > export_uuid(msg_arg->params[1].u.octets, &client_uuid); > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params + 2, > > - arg->num_params, param); > > + arg->num_params, param, > > + false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -427,7 +428,8 @@ int optee_open_session(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params + 2)) { > > + msg_arg->params + 2, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_COMMUNICATION; > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > /* Close session again to avoid leakage */ > > @@ -541,7 +543,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > msg_arg->cancel_id = arg->cancel_id; > > > > rc = optee->ops->to_msg_param(optee, msg_arg->params, arg->num_params, > > - param); > > + param, false /*!update_out*/); > > if (rc) > > goto out; > > > > @@ -551,7 +553,7 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg, > > } > > > > if (optee->ops->from_msg_param(optee, param, arg->num_params, > > - msg_arg->params)) { > > + msg_arg->params, true /*update_out*/)) { > > msg_arg->ret = TEEC_ERROR_COMMUNICATION; > > msg_arg->ret_origin = TEEC_ORIGIN_COMMS; > > } > > diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c > > index 4ca1d5161b82..e4b08cd195f3 100644 > > --- a/drivers/tee/optee/ffa_abi.c > > +++ b/drivers/tee/optee/ffa_abi.c > > @@ -122,15 +122,21 @@ static int optee_shm_rem_ffa_handle(struct optee *optee, u64 global_id) > > */ > > > > static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > - u32 attr, const struct optee_msg_param *mp) > > + u32 attr, const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm = NULL; > > u64 offs_high = 0; > > u64 offs_low = 0; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_FMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_FMEM_INPUT; > > - p->u.memref.size = mp->u.fmem.size; > > > > if (mp->u.fmem.global_id != OPTEE_MSG_FMEM_INVALID_GLOBAL_ID) > > shm = optee_shm_from_ffa_handle(optee, mp->u.fmem.global_id); > > @@ -141,6 +147,8 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > offs_high = mp->u.fmem.offs_high; > > } > > p->u.memref.shm_offs = offs_low | offs_high << 32; > > +out: > > + p->u.memref.size = mp->u.fmem.size; > > } > > > > /** > > @@ -150,12 +158,14 @@ static void from_msg_param_ffa_mem(struct optee *optee, struct tee_param *p, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_from_msg_param(struct optee *optee, > > struct tee_param *params, size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > size_t n; > > > > @@ -166,18 +176,20 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_FMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_FMEM_INOUT: > > - from_msg_param_ffa_mem(optee, p, attr, mp); > > + from_msg_param_ffa_mem(optee, p, attr, mp, update_out); > > break; > > default: > > return -EINVAL; > > @@ -188,10 +200,16 @@ static int optee_ffa_from_msg_param(struct optee *optee, > > } > > > > static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > struct tee_shm *shm = p->u.memref.shm; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_FMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > @@ -211,6 +229,7 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > memset(&mp->u, 0, sizeof(mp->u)); > > mp->u.fmem.global_id = OPTEE_MSG_FMEM_INVALID_GLOBAL_ID; > > } > > +out: > > mp->u.fmem.size = p->u.memref.size; > > > > return 0; > > @@ -222,13 +241,15 @@ static int to_msg_param_ffa_mem(struct optee_msg_param *mp, > > * @optee: main service struct > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > - * @params: subsystem itnernal parameter representation > > + * @params: subsystem internal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_ffa_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > size_t num_params, > > - const struct tee_param *params) > > + const struct tee_param *params, > > + bool update_out) > > { > > size_t n; > > > > @@ -238,18 +259,20 @@ static int optee_ffa_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > - if (to_msg_param_ffa_mem(mp, p)) > > + if (to_msg_param_ffa_mem(mp, p, update_out)) > > return -EINVAL; > > break; > > default: > > Can we rather handle it as follows to improve code readability and > maintainence long term? Ditto for all other places. > > static int optee_ffa_to_msg_param(struct optee *optee, > struct optee_msg_param *msg_params, > size_t num_params, > const struct tee_param *params, > bool update_out) > { > size_t n; > > for (n = 0; n < num_params; n++) { > const struct tee_param *p = params + n; > struct optee_msg_param *mp = msg_params + n; > > if (update_out && (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_NONE || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT || > p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT)) > continue; You're missing updating the length field for memrefs. Cheers, Jens > > switch (p->attr) { > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > memset(&mp->u, 0, sizeof(mp->u)); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > optee_to_msg_param_value(mp, p); > break; > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > if (to_msg_param_ffa_mem(mp, p)) > return -EINVAL; > break; > default: > return -EINVAL; > } > } > > return 0; > } > > -Sumit > > > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h > > index dc0f355ef72a..20eda508dbac 100644 > > --- a/drivers/tee/optee/optee_private.h > > +++ b/drivers/tee/optee/optee_private.h > > @@ -185,10 +185,12 @@ struct optee_ops { > > bool system_thread); > > int (*to_msg_param)(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params); > > + size_t num_params, const struct tee_param *params, > > + bool update_out); > > int (*from_msg_param)(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params); > > + const struct optee_msg_param *msg_params, > > + bool update_out); > > }; > > > > /** > > @@ -316,23 +318,35 @@ void optee_release(struct tee_context *ctx); > > void optee_release_supp(struct tee_context *ctx); > > > > static inline void optee_from_msg_param_value(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > - p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > - attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > - p->u.value.a = mp->u.value.a; > > - p->u.value.b = mp->u.value.b; > > - p->u.value.c = mp->u.value.c; > > + if (!update_out) > > + p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT + > > + attr - OPTEE_MSG_ATTR_TYPE_VALUE_INPUT; > > + > > + if (attr == OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT || > > + attr == OPTEE_MSG_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + p->u.value.a = mp->u.value.a; > > + p->u.value.b = mp->u.value.b; > > + p->u.value.c = mp->u.value.c; > > + } > > } > > > > static inline void optee_to_msg_param_value(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, > > + bool update_out) > > { > > - mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > - TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > - mp->u.value.a = p->u.value.a; > > - mp->u.value.b = p->u.value.b; > > - mp->u.value.c = p->u.value.c; > > + if (!update_out) > > + mp->attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT + p->attr - > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT; > > + > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > + p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT || !update_out) { > > + mp->u.value.a = p->u.value.a; > > + mp->u.value.b = p->u.value.b; > > + mp->u.value.c = p->u.value.c; > > + } > > } > > > > void optee_cq_init(struct optee_call_queue *cq, int thread_count); > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c > > index ebbbd42b0e3e..580e6b9b0606 100644 > > --- a/drivers/tee/optee/rpc.c > > +++ b/drivers/tee/optee/rpc.c > > @@ -63,7 +63,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) > > + arg->params, false /*!update_out*/)) > > goto bad; > > > > for (i = 0; i < arg->num_params; i++) { > > @@ -107,7 +107,8 @@ static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx, > > } else { > > params[3].u.value.a = msg.len; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) > > + arg->num_params, params, > > + true /*update_out*/)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > else > > arg->ret = TEEC_SUCCESS; > > @@ -188,6 +189,7 @@ static void handle_rpc_func_cmd_wait(struct optee_msg_arg *arg) > > static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > struct optee_msg_arg *arg) > > { > > + bool update_out = false; > > struct tee_param *params; > > > > arg->ret_origin = TEEC_ORIGIN_COMMS; > > @@ -200,15 +202,21 @@ static void handle_rpc_supp_cmd(struct tee_context *ctx, struct optee *optee, > > } > > > > if (optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params)) { > > + arg->params, update_out)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > > > arg->ret = optee_supp_thrd_req(ctx, arg->cmd, arg->num_params, params); > > > > + /* > > + * Special treatment for OPTEE_RPC_CMD_SHM_ALLOC since input is a > > + * value type, but the output is a memref type. > > + */ > > + if (arg->cmd != OPTEE_RPC_CMD_SHM_ALLOC) > > + update_out = true; > > if (optee->ops->to_msg_param(optee, arg->params, arg->num_params, > > - params)) > > + params, update_out)) > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > out: > > kfree(params); > > @@ -270,7 +278,7 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > @@ -280,7 +288,8 @@ static void handle_rpc_func_rpmb_probe_reset(struct tee_context *ctx, > > params[0].u.value.b = 0; > > params[0].u.value.c = 0; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -324,7 +333,7 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -358,7 +367,8 @@ static void handle_rpc_func_rpmb_probe_next(struct tee_context *ctx, > > params[0].u.value.b = rdev->descr.capacity; > > params[0].u.value.c = rdev->descr.reliable_wr_count; > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > return; > > } > > @@ -384,7 +394,7 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > > > if (arg->num_params != ARRAY_SIZE(params) || > > optee->ops->from_msg_param(optee, params, arg->num_params, > > - arg->params) || > > + arg->params, false /*!update_out*/) || > > params[0].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT || > > params[1].attr != TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > @@ -401,7 +411,8 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx, > > goto out; > > } > > if (optee->ops->to_msg_param(optee, arg->params, > > - arg->num_params, params)) { > > + arg->num_params, params, > > + true /*update_out*/)) { > > arg->ret = TEEC_ERROR_BAD_PARAMETERS; > > goto out; > > } > > diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c > > index 165fadd9abc9..cfdae266548b 100644 > > --- a/drivers/tee/optee/smc_abi.c > > +++ b/drivers/tee/optee/smc_abi.c > > @@ -81,20 +81,26 @@ static int optee_cpuhp_disable_pcpu_irq(unsigned int cpu) > > */ > > > > static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > phys_addr_t pa; > > int rc; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_TMEM_INPUT) > > + return 0; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_TMEM_INPUT; > > - p->u.memref.size = mp->u.tmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.tmem.shm_ref; > > if (!shm) { > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(shm, 0, &pa); > > @@ -103,18 +109,25 @@ static int from_msg_param_tmp_mem(struct tee_param *p, u32 attr, > > > > p->u.memref.shm_offs = mp->u.tmem.buf_ptr - pa; > > p->u.memref.shm = shm; > > - > > +out: > > + p->u.memref.size = mp->u.tmem.size; > > return 0; > > } > > > > static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > - const struct optee_msg_param *mp) > > + const struct optee_msg_param *mp, > > + bool update_out) > > { > > struct tee_shm *shm; > > > > + if (update_out) { > > + if (attr == OPTEE_MSG_ATTR_TYPE_RMEM_INPUT) > > + return; > > + goto out; > > + } > > + > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT + > > attr - OPTEE_MSG_ATTR_TYPE_RMEM_INPUT; > > - p->u.memref.size = mp->u.rmem.size; > > shm = (struct tee_shm *)(unsigned long)mp->u.rmem.shm_ref; > > > > if (shm) { > > @@ -124,6 +137,8 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > p->u.memref.shm_offs = 0; > > p->u.memref.shm = NULL; > > } > > +out: > > + p->u.memref.size = mp->u.rmem.size; > > } > > > > /** > > @@ -133,11 +148,13 @@ static void from_msg_param_reg_mem(struct tee_param *p, u32 attr, > > * @params: subsystem internal parameter representation > > * @num_params: number of elements in the parameter arrays > > * @msg_params: OPTEE_MSG parameters > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > size_t num_params, > > - const struct optee_msg_param *msg_params) > > + const struct optee_msg_param *msg_params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -149,25 +166,27 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > > > switch (attr) { > > case OPTEE_MSG_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > p->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&p->u, 0, sizeof(p->u)); > > break; > > case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > - optee_from_msg_param_value(p, attr, mp); > > + optee_from_msg_param_value(p, attr, mp, update_out); > > break; > > case OPTEE_MSG_ATTR_TYPE_TMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_TMEM_INOUT: > > - rc = from_msg_param_tmp_mem(p, attr, mp); > > + rc = from_msg_param_tmp_mem(p, attr, mp, update_out); > > if (rc) > > return rc; > > break; > > case OPTEE_MSG_ATTR_TYPE_RMEM_INPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_OUTPUT: > > case OPTEE_MSG_ATTR_TYPE_RMEM_INOUT: > > - from_msg_param_reg_mem(p, attr, mp); > > + from_msg_param_reg_mem(p, attr, mp, update_out); > > break; > > > > default: > > @@ -178,20 +197,25 @@ static int optee_from_msg_param(struct optee *optee, struct tee_param *params, > > } > > > > static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > int rc; > > phys_addr_t pa; > > > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_TMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.tmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.tmem.size = p->u.memref.size; > > > > if (!p->u.memref.shm) { > > mp->u.tmem.buf_ptr = 0; > > - return 0; > > + goto out; > > } > > > > rc = tee_shm_get_pa(p->u.memref.shm, p->u.memref.shm_offs, &pa); > > @@ -201,19 +225,27 @@ static int to_msg_param_tmp_mem(struct optee_msg_param *mp, > > mp->u.tmem.buf_ptr = pa; > > mp->attr |= OPTEE_MSG_ATTR_CACHE_PREDEFINED << > > OPTEE_MSG_ATTR_CACHE_SHIFT; > > - > > +out: > > + mp->u.tmem.size = p->u.memref.size; > > return 0; > > } > > > > static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > - const struct tee_param *p) > > + const struct tee_param *p, bool update_out) > > { > > + if (update_out) { > > + if (p->attr == TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT) > > + return 0; > > + goto out; > > + } > > + > > mp->attr = OPTEE_MSG_ATTR_TYPE_RMEM_INPUT + p->attr - > > TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT; > > > > mp->u.rmem.shm_ref = (unsigned long)p->u.memref.shm; > > - mp->u.rmem.size = p->u.memref.size; > > mp->u.rmem.offs = p->u.memref.shm_offs; > > +out: > > + mp->u.rmem.size = p->u.memref.size; > > return 0; > > } > > > > @@ -223,11 +255,13 @@ static int to_msg_param_reg_mem(struct optee_msg_param *mp, > > * @msg_params: OPTEE_MSG parameters > > * @num_params: number of elements in the parameter arrays > > * @params: subsystem itnernal parameter representation > > + * @update_out: update parameter for output only > > * Returns 0 on success or <0 on failure > > */ > > static int optee_to_msg_param(struct optee *optee, > > struct optee_msg_param *msg_params, > > - size_t num_params, const struct tee_param *params) > > + size_t num_params, const struct tee_param *params, > > + bool update_out) > > { > > int rc; > > size_t n; > > @@ -238,21 +272,23 @@ static int optee_to_msg_param(struct optee *optee, > > > > switch (p->attr) { > > case TEE_IOCTL_PARAM_ATTR_TYPE_NONE: > > + if (update_out) > > + break; > > mp->attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE; > > memset(&mp->u, 0, sizeof(mp->u)); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INOUT: > > - optee_to_msg_param_value(mp, p); > > + optee_to_msg_param_value(mp, p, update_out); > > break; > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_OUTPUT: > > case TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT: > > if (tee_shm_is_dynamic(p->u.memref.shm)) > > - rc = to_msg_param_reg_mem(mp, p); > > + rc = to_msg_param_reg_mem(mp, p, update_out); > > else > > - rc = to_msg_param_tmp_mem(mp, p); > > + rc = to_msg_param_tmp_mem(mp, p, update_out); > > if (rc) > > return rc; > > break; > > -- > > 2.43.0 > >

1 year, 6 months

Re: [PATCH] dma-buf: Replace nested max() with single max3()

by T.J. Mercier

On Fri, Mar 14, 2025 at 10:02 PM <feng.wei8(a)zte.com.cn> wrote: > > From: FengWei <feng.wei8(a)zte.com.cn> > > Use max3() macro instead of nesting max() to simplify the return > statement. > > Signed-off-by: FengWei <feng.wei8(a)zte.com.cn> The commit subject should include "dma-buf: heaps:" as a prefix. With that: Reviewed-by: T.J. Mercier <tjmercier(a)google.com> > --- > drivers/dma-buf/dma-heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c > index 3cbe87d4a464..96cb9ab5731a 100644 > --- a/drivers/dma-buf/dma-heap.c > +++ b/drivers/dma-buf/dma-heap.c > @@ -147,7 +147,7 @@ static long dma_heap_ioctl(struct file *file, unsigned int ucmd, > in_size = 0; > if ((ucmd & kcmd & IOC_OUT) == 0) > out_size = 0; > - ksize = max(max(in_size, out_size), drv_size); > + ksize = max3(in_size, out_size, drv_size); > > /* If necessary, allocate buffer for ioctl argument */ > if (ksize > sizeof(stack_kdata)) { > -- > 2.25.1

1 year, 6 months

Re: [PATCH v5 RESEND 1/2] dmaengine: qcom: gpi: Add GPI Block event interrupt support

by Vinod Koul

On 12-02-25, 17:35, Jyothi Kumar Seerapu wrote: > GSI hardware generates an interrupt for each transfer completion. > For multiple messages within a single transfer, this results in > N interrupts for N messages, leading to significant software > interrupt latency. > > To mitigate this latency, utilize Block Event Interrupt (BEI) mechanism. > Enabling BEI instructs the GSI hardware to prevent interrupt generation > and BEI is disabled when an interrupt is necessary. > > When using BEI, consider splitting a single multi-message transfer into > chunks of 8 messages internally and so interrupts are not expected for > the first 7 message completions, only the last message triggers > an interrupt, indicating the completion of 8 messages. > > This BEI mechanism enhances overall transfer efficiency. That sounds good but I dont like the idea that we add a custom interface for this. Please use DMA_PREP_INTERRUPT instead. Adding this flag should trigger N interrupts, absence of this should lead to Block events only -- ~Vinod

1 year, 7 months

[PATCH v4] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Reviewed-by: T.J. Mercier <tjmercier(a)google.com> Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v3: - Add TJ RB - Fix a few typos. Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..535f49047ce6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represents a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers. + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

1 year, 7 months

[PATCH v3] Documentation: dma-buf: heaps: Add heap name definitions

by Maxime Ripard

Following a recent discussion at last Plumbers, John Stultz, Sumit Sewal, TJ Mercier and I came to an agreement that we should document what the dma-buf heaps names are expected to be, and what the buffers attributes you'll get should be documented. Let's create that doc to make sure those attributes and names are guaranteed going forward. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes from v2: - Remove exhaustive list of names for platforms, and just mention the alternatives. - Add MAINTAINERS entry Changes from v1: - Add the mention that the cma / reserved heap is optional. --- Documentation/userspace-api/dma-buf-heaps.rst | 25 +++++++++++++++++++ Documentation/userspace-api/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-heaps.rst diff --git a/Documentation/userspace-api/dma-buf-heaps.rst b/Documentation/userspace-api/dma-buf-heaps.rst new file mode 100644 index 000000000000..5b92d69646f6 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-heaps.rst @@ -0,0 +1,25 @@ +.. SPDX-License-Identifier: GPL-2.0 + +============================== +Allocating dma-buf using heaps +============================== + +Dma-buf Heaps are a way for userspace to allocate dma-buf objects. They are +typically used to allocate buffers from a specific allocation pool, or to share +buffers across frameworks. + +Heaps +===== + +A heap represent a specific allocator. The Linux kernel currently supports the +following heaps: + + - The ``system`` heap allocates virtually contiguous, cacheable, buffers + + - The ``cma`` heap allocates physically contiguous, cacheable, + buffers. Only present if a CMA region is present. Such a region is + usually created either through the kernel commandline through the + `cma` parameter, a memory region Device-Tree node with the + `linux,cma-default` property set, or through the `CMA_SIZE_MBYTES` or + `CMA_SIZE_PERCENTAGE` Kconfig options. Depending on the platform, it + might be called ``reserved``, ``linux,cma``, or ``default-pool``. diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst index b1395d94b3fd..9cbe4390c872 100644 --- a/Documentation/userspace-api/index.rst +++ b/Documentation/userspace-api/index.rst @@ -42,10 +42,11 @@ Devices and I/O .. toctree:: :maxdepth: 1 accelerators/ocxl + dma-buf-heaps dma-buf-alloc-exchange gpio/index iommufd media/index dcdbas diff --git a/MAINTAINERS b/MAINTAINERS index 8e0736dc2ee0..ef0364e65aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6911,10 +6911,11 @@ R: T.J. Mercier <tjmercier(a)google.com> L: linux-media(a)vger.kernel.org L: dri-devel(a)lists.freedesktop.org L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Maintained T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/userspace-api/dma-buf-heaps.rst F: drivers/dma-buf/dma-heap.c F: drivers/dma-buf/heaps/* F: include/linux/dma-heap.h F: include/uapi/linux/dma-heap.h F: tools/testing/selftests/dmabuf-heaps/ -- 2.48.1

1 year, 7 months

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Sumit Garg

On Fri, 14 Feb 2025 at 21:19, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > > On Fri, 14 Feb 2025 18:37:14 +0530 > Sumit Garg <sumit.garg(a)linaro.org> wrote: > > > On Fri, 14 Feb 2025 at 15:37, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > > Hi, > > > > > > On Thu, Feb 13, 2025 at 6:39 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 13 Feb 2025 at 15:57, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > On Thu, Feb 13, 2025 at 3:05 PM Daniel Stone <daniel(a)fooishbar.org> wrote: > > > > > > But just because TEE is one good backend implementation, doesn't mean > > > > > > it should be the userspace ABI. Why should userspace care that TEE has > > > > > > mediated the allocation instead of it being a predefined range within > > > > > > DT? > > > > > > > > > > The TEE may very well use a predefined range that part is abstracted > > > > > with the interface. > > > > > > > > Of course. But you can also (and this has been shipped on real > > > > devices) handle this without any per-allocation TEE needs by simply > > > > allocating from a memory range which is predefined within DT. > > > > > > > > From the userspace point of view, why should there be one ABI to > > > > allocate memory from a predefined range which is delivered by DT to > > > > the kernel, and one ABI to allocate memory from a predefined range > > > > which is mediated by TEE? > > > > > > We need some way to specify the protection profile (or use case as > > > I've called it in the ABI) required for the buffer. Whether it's > > > defined in DT seems irrelevant. > > > > > > > > > > > > > What advantage > > > > > > does userspace get from having to have a different codepath to get a > > > > > > different handle to memory? What about x86? > > > > > > > > > > > > I think this proposal is looking at it from the wrong direction. > > > > > > Instead of working upwards from the implementation to userspace, start > > > > > > with userspace and work downwards. The interesting property to focus > > > > > > on is allocating memory, not that EL1 is involved behind the scenes. > > > > > > > > > > From what I've gathered from earlier discussions, it wasn't much of a > > > > > problem for userspace to handle this. If the kernel were to provide it > > > > > via a different ABI, how would it be easier to implement in the > > > > > kernel? I think we need an example to understand your suggestion. > > > > > > > > It is a problem for userspace, because we need to expose acceptable > > > > parameters for allocation through the entire stack. If you look at the > > > > dmabuf documentation in the kernel for how buffers should be allocated > > > > and exchanged, you can see the negotiation flow for modifiers. This > > > > permeates through KMS, EGL, Vulkan, Wayland, GStreamer, and more. > > > > > > What dma-buf properties are you referring to? > > > dma_heap_ioctl_allocate() accepts a few flags for the resulting file > > > descriptor and no flags for the heap itself. > > > > > > > > > > > Standardising on heaps allows us to add those in a similar way. > > > > > > How would you solve this with heaps? Would you use one heap for each > > > protection profile (use case), add heap_flags, or do a bit of both? > > I would say one heap per-profile. > And then it would have a per vendor multiplication factor as each vendor enforces memory restriction in a platform specific manner which won't scale. > > > > Christian gave an historical background here [1] as to why that hasn't > > worked in the past with DMA heaps given the scalability issues. > > > > [1] https://lore.kernel.org/dri-devel/e967e382-6cca-4dee-8333-39892d532f71@gmai… > > Hm, I fail to see where Christian dismiss the dma-heaps solution in > this email. He even says: > > > If the memory is not physically attached to any device, but rather just > memory attached to the CPU or a system wide memory controller then > expose the memory as DMA-heap with specific requirements (e.g. certain > sized pages, contiguous, restricted, encrypted, ...). I am not saying Christian dismissed DMA heaps but rather how scalability is an issue. What we are proposing here is a generic interface via TEE to the firmware/Trusted OS which can perform all the platform specific memory restrictions. This solution will scale across vendors. > > > > > > > > > > If we > > > > have to add different allocation mechanisms, then the complexity > > > > increases, permeating not only into all the different userspace APIs, > > > > but also into the drivers which need to support every different > > > > allocation mechanism even if they have no opinion on it - e.g. Mali > > > > doesn't care in any way whether the allocation comes from a heap or > > > > TEE or ACPI or whatever, it cares only that the memory is protected. > > > > > > > > Does that help? > > > > > > I think you're missing the stage where an unprotected buffer is > > > received and decrypted into a protected buffer. If you use the TEE for > > > decryption or to configure the involved devices for the use case, it > > > makes sense to let the TEE allocate the buffers, too. A TEE doesn't > > > have to be an OS in the secure world, it can be an abstraction to > > > support the use case depending on the design. So the restricted buffer > > > is already allocated before we reach Mali in your example. > > > > > > Allocating restricted buffers from the TEE subsystem saves us from > > > maintaining proxy dma-buf heaps. > > Honestly, when I look at dma-heap implementations, they seem > to be trivial shells around existing (more complex) allocators, and the > boiler plate [1] to expose a dma-heap is relatively small. The dma-buf > implementation, you already have, so we're talking about a hundred > lines of code to maintain, which shouldn't be significantly more than > what you have for the new ioctl() to be honest. It will rather be redundant vendor specific code under DMA heaps calling into firmware/Trusted OS to enforce memory restrictions as you can look into Mediatek example [1]. With TEE subsystem managing that it won't be the case as we will provide a common abstraction for the communication with underlying firmware/Trusted OS. [1] https://lore.kernel.org/linux-arm-kernel/20240515112308.10171-1-yong.wu@med… > And I'll insist on what > Daniel said, it's a small price to pay to have a standard interface to > expose to userspace. If dma-heaps are not used for this kind things, I > honestly wonder what they will be used for... Let's try not to forcefully find a use-case for DMA heaps when there is a better alternative available. I am still failing to see why you don't consider following as a standardised user-space interface: "When user-space has to work with restricted memory, ask TEE device to allocate it" -Sumit

1 year, 7 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig