Thanks Jeffrey for the addition. Hi Rob, krzysztof, Just a ping. Is Jeffrey's reply ok for you? Thanks. On Tue, 2023-09-19 at 15:15 -0700, Jeffrey Kardatzke wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Mon, Sep 18, 2023 at 3:47 AM Yong Wu (吴勇) <Yong.Wu@xxxxxxxxxxxx> > wrote: > > > > On Tue, 2023-09-12 at 10:53 -0500, Rob Herring wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > On Tue, Sep 12, 2023 at 11:13:50AM +0100, Robin Murphy wrote: > > > > On 12/09/2023 9:28 am, Krzysztof Kozlowski wrote: > > > > > On 12/09/2023 08:16, Yong Wu (吴勇) wrote: > > > > > > Hi Rob, > > > > > > > > > > > > Thanks for your review. > > > > > > > > > > > > On Mon, 2023-09-11 at 10:44 -0500, Rob Herring wrote: > > > > > > > > > > > > > > External email : Please do not click links or open > > > attachments until > > > > > > > you have verified the sender or the content. > > > > > > > On Mon, Sep 11, 2023 at 10:30:37AM +0800, Yong Wu > wrote: > > > > > > > > This adds the binding for describing a CMA memory for > > > MediaTek > > > > > > > SVP(Secure > > > > > > > > Video Path). > > > > > > > > > > > > > > CMA is a Linux thing. How is this related to CMA? > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx> > > > > > > > > --- > > > > > > > > .../mediatek,secure_cma_chunkmem.yaml | 42 > > > > > > > +++++++++++++++++++ > > > > > > > > 1 file changed, 42 insertions(+) > > > > > > > > create mode 100644 > > > Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > > > > > > > > > diff --git > a/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > b/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > new file mode 100644 > > > > > > > > index 000000000000..cc10e00d35c4 > > > > > > > > --- /dev/null > > > > > > > > +++ b/Documentation/devicetree/bindings/reserved- > > > > > > > memory/mediatek,secure_cma_chunkmem.yaml > > > > > > > > @@ -0,0 +1,42 @@ > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2- > Clause) > > > > > > > > +%YAML 1.2 > > > > > > > > +--- > > > > > > > > +$id: > > > > > > > > > > > http://devicetree.org/schemas/reserved-memory/mediatek,secure_cma_chunkmem.yaml# > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > > + > > > > > > > > +title: MediaTek Secure Video Path Reserved Memory > > > > > > > > > > > > > > What makes this specific to Mediatek? Secure video path > is > > > fairly > > > > > > > common, right? > > > > > > > > > > > > Here we just reserve a buffer and would like to create a > dma- > > > buf secure > > > > > > heap for SVP, then the secure engines(Vcodec and DRM) could > > > prepare > > > > > > secure buffer through it. > > > > > > But the heap driver is pure SW driver, it is not platform > > > device and > > > > > > > > > > All drivers are pure SW. > > > > > > > > > > > we don't have a corresponding HW unit for it. Thus I don't > > > think I > > > > > > could create a platform dtsi node and use "memory-region" > > > pointer to > > > > > > the region. I used RESERVEDMEM_OF_DECLARE currently(The > code is > > > in > > > > > > [9/9]). Sorry if this is not right. > > > > > > > > > > If this is not for any hardware and you already understand > this > > > (since > > > > > you cannot use other bindings) then you cannot have custom > > > bindings for > > > > > it either. > > > > > > > > > > > > > > > > > Then in our usage case, is there some similar method to do > > > this? or > > > > > > any other suggestion? > > > > > > > > > > Don't stuff software into DTS. > > > > > > > > Aren't most reserved-memory bindings just software policy if > you > > > look at it > > > > that way, though? IIUC this is a pool of memory that is visible > and > > > > available to the Non-Secure OS, but is fundamentally owned by > the > > > Secure > > > > TEE, and pages that the TEE allocates from it will become > > > physically > > > > inaccessible to the OS. Thus the platform does impose > constraints > > > on how the > > > > Non-Secure OS may use it, and per the rest of the reserved- > memory > > > bindings, > > > > describing it as a "reusable" reservation seems entirely > > > appropriate. If > > > > anything that's *more* platform-related and so DT-relevant than > > > typical > > > > arbitrary reservations which just represent "save some memory > to > > > dedicate to > > > > a particular driver" and don't actually bear any relationship > to > > > firmware or > > > > hardware at all. > > > > > > Yes, a memory range defined by hardware or firmware is within > scope > > > of > > > DT. (CMA at aribitrary address was questionable.) > > > > Before I reply, my context is that I'm using these patches from > Mediatek on ChromeOS to implement secure video playback. > > > I guess the memory range is not "defined" by HW in our case, but > this > > reserve buffer is indeed prepared for and used by HW. > > > The memory range is defined in the firmware. The TEE is configured > with the same address/size that is being set in this DT node. (so > based on comments already, this is appropriate to put in the DT). > > > If this is a normal reserved buffer for some device, we could > define a > > reserved-memory with "shared-dma-pool", then the device use it via > > "memory-region" property, is this right? > > > > Here it is a secure buffer case and this usage relationship is > > indirect. We create a new heap for this new secure type memory, > other > > users such as VCODEC and DRM allocate secure memory through the new > > heap. > > > > About the aribitrary address is because we have HW register for it. > As > > long as this is a legal dram address, it is fine. When this address > is > > passed into TEE, it will be protected by HW. > > > > > > > > My issue here is more that 'secure video memory' is not any way > > > Mediatek > > > specific. > > > > Sorry, I don't know if there already is an SVP case in the current > > kernel. If so, could you help share it? > > I don't think there is any SVP (Secure Video Path) case in the > current > kernel. I agree this shouldn't be a Mediatek specific setting, as > this > could be usable to other SVP implementations. > > I do think this should have 'cma' in the DT description, as it does > relate to what the driver is going to do with this memory region. It > will establish it as a CMA region in Linux. The reason it needs to be > a CMA region is that the entire memory region will need to transition > between secure (i.e. TEE owned) and non-secure (i.e. kernel owned). > Some chipsets have the ability to change memory states between secure > & non-secure at page level granularity, and in these cases you don't > need to worry about having a CMA region like this. That is not the > case on the Mediatek chips where there is a limit to how many regions > you can mark as secure. In order to deal with that limitation, once a > secure allocation needs to be performed, the kernel driver allocates > the entire CMA region so nothing else will use it. Then it marks that > whole region secure and the TEE can do its own allocations from that > region. When all those allocations are freed, it can mark that region > as non-secure again, drop the whole CMA allocation and the kernel can > make use of that CMA region again. (there is the other dma-heap in > their patches, which is for a smaller region that can always be > secure...but that one is a permanent carveout, the CMA region is > available to the kernel while not in use for secure memory) > > So maybe something like: > > +title:Secure Reserved CMA Region > + > +description: > + This binding describes a CMA region that can dynamically > transition > between secure and non-secure states that a TEE can allocate memory > from. > + > +maintainers: > + - Yong Wu <yong.wu@xxxxxxxxxxxx> > + > +allOf: > + - $ref: reserved-memory.yaml > + > +properties: > + compatible: > + const: secure_cma_region > + > +required: > + - compatible > + - reg > + - reusable > + > +unevaluatedProperties: false > + > +examples: > + - | > + > + reserved-memory { > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + reserved-memory@80000000 { > + compatible = "secure_cma_region"; > + reusable; > + reg = <0x80000000 0x18000000>; > + }; > + };
