Am 09.11.23 um 04:20 schrieb Mina Almasry:
> [SNIP]
>> But we might be able to do something as folio is doing now, mm subsystem
>> is still seeing 'struct folio/page', but other subsystem like slab is using
>> 'struct slab', and there is still some common fields shared between
>> 'struct folio' and 'struct slab'.
>>
> In my eyes this is almost exactly what I suggested in RFC v1 and got
> immediately nacked with no room to negotiate. What we did for v1 is to
> allocate struct pages for dma-buf to make dma-bufs look like struct
> page to mm subsystem. Almost exactly what you're describing above.
> It's a no-go. I don't think renaming struct page to netmem is going to
> move the needle (it also re-introduces code-churn). What I feel like I
> learnt is that dma-bufs are not struct pages and can't be made to look
> like one, I think.
Yeah, that pretty much hits the nail on the head. What was not mentioned
yet and you could potentially try to do is to go the other way around.
In other words instead of importing a DMA-buf file descriptor into the
page-pool, you take some netdev page-pool pages and export them as
DMA-buf handle.
All you then need to do is to implement the necessary DMA-buf interface,
e.g. wait for DMA-fences before accessing stuff, when you have an async
operation install a DMA-fence so that other can wait for your operation
to finish etc.. etc..
This still doesn't gives you peer 2 peer over for example the PCIe bus,
but it would give you zero copy in the sense that a GPU or other
acceleration device directly writes stuff into memory a network device
can work with.
We already have some similar functionality for at least Intel and AMD hw
where an userptr mechanism is used to make malloced memory (backed by
normal struct pages) available to the GPU. The problem with this
approach is that most GPUs currently can't do good recoverable page
faults which in turn makes the whole MMU notifier based approach just
horrible inefficient.
Just giving a few more pointers you might want to look into...
Cheers,
Christian.
From: Mina Almasry
> Sent: 06 November 2023 02:44
>
> For device memory TCP, we expect the skb headers to be available in host
> memory for access, and we expect the skb frags to be in device memory
> and unaccessible to the host. We expect there to be no mixing and
> matching of device memory frags (unaccessible) with host memory frags
> (accessible) in the same skb.
>
> Add a skb->devmem flag which indicates whether the frags in this skb
> are device memory frags or not.
>
...
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 1fae276c1353..8fb468ff8115 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -805,6 +805,8 @@ typedef unsigned char *sk_buff_data_t;
> * @csum_level: indicates the number of consecutive checksums found in
> * the packet minus one that have been verified as
> * CHECKSUM_UNNECESSARY (max 3)
> + * @devmem: indicates that all the fragments in this skb are backed by
> + * device memory.
> * @dst_pending_confirm: need to confirm neighbour
> * @decrypted: Decrypted SKB
> * @slow_gro: state present at GRO time, slower prepare step required
> @@ -991,7 +993,7 @@ struct sk_buff {
> #if IS_ENABLED(CONFIG_IP_SCTP)
> __u8 csum_not_inet:1;
> #endif
> -
> + __u8 devmem:1;
> #if defined(CONFIG_NET_SCHED) || defined(CONFIG_NET_XGRESS)
> __u16 tc_index; /* traffic control index */
> #endif
> @@ -1766,6 +1768,12 @@ static inline void skb_zcopy_downgrade_managed(struct sk_buff *skb)
> __skb_zcopy_downgrade_managed(skb);
> }
Doesn't that bloat struct sk_buff?
I'm not sure there are any spare bits available.
Although CONFIG_NET_SWITCHDEV and CONFIG_NET_SCHED seem to
already add padding.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Oct 30, 2023 at 11:58:20AM +0100, Marco Pagani wrote:
> On 2023-10-25 10:43, Maxime Ripard wrote:
> > On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote:
> >>>> +static void drm_gem_shmem_test_obj_create_private(struct kunit *test)
> >>>> +{
> >>>> + struct fake_dev *fdev = test->priv;
> >>>> + struct drm_gem_shmem_object *shmem;
> >>>> + struct drm_gem_object *gem_obj;
> >>>> + struct dma_buf buf_mock;
> >>>> + struct dma_buf_attachment attach_mock;
> >>>> + struct sg_table *sgt;
> >>>> + char *buf;
> >>>> + int ret;
> >>>> +
> >>>> + /* Create a mock scatter/gather table */
> >>>> + buf = kunit_kzalloc(test, TEST_SIZE, GFP_KERNEL);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, buf);
> >>>> +
> >>>> + sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, sgt);
> >>>> +
> >>>> + ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> >>>> + KUNIT_ASSERT_EQ(test, ret, 0);
> >>>> + sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >>>> +
> >>>> + /* Init a mock DMA-BUF */
> >>>> + buf_mock.size = TEST_SIZE;
> >>>> + attach_mock.dmabuf = &buf_mock;
> >>>> +
> >>>> + gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> >>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> >>>> + KUNIT_ASSERT_EQ(test, gem_obj->size, TEST_SIZE);
> >>>> + KUNIT_ASSERT_NULL(test, gem_obj->filp);
> >>>> + KUNIT_ASSERT_NOT_NULL(test, gem_obj->funcs);
> >>>> +
> >>>> + shmem = to_drm_gem_shmem_obj(gem_obj);
> >>>> + KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
> >>>> +
> >>>> + /* The scatter/gather table is freed by drm_gem_shmem_free */
> >>>> + drm_gem_shmem_free(shmem);
> >>>> +}
> >>>
> >>> KUNIT_ASSERT_* will stop the execution of the test on failure, you
> >>> should probably use a bit more of KUNIT_EXPECT_* calls otherwise you'll
> >>> leak resources.
> >>>
> >>> You also probably want to use a kunit_action to clean up and avoid that
> >>> whole discussion
> >>>
> >>
> >> You are right. I slightly prefer using KUnit expectations (unless actions
> >> are strictly necessary) since I feel using actions makes test cases a bit
> >> less straightforward to understand. Is this okay for you?
> >
> > I disagree. Actions make it easier to reason about, even when comparing
> > assertion vs expectation
> >
> > Like, for the call to sg_alloc_table and
> > drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs
> > expect would be something like:
> >
> > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > KUNIT_ASSERT_NOT_NULL(test, sgt);
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > /*
> > * Here, it's already not super clear whether you want to expect vs
> > * assert. expect will make you handle the failure case later, assert will
> > * force you to call kfree on sgt. Both kind of suck in their own ways.
> > */
> >
> > sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >
> > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> >
> > /*
> > * If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt).
> > */
> >
> > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
> > KUNIT_EXPECT_NULL(test, gem_obj->filp);
> > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
> >
> > /*
> > * And here we have to handle the case where the expectation was wrong,
> > * but the test still continued.
> > */
> >
> > But if you're not using an action, you still have to call kfree(sgt),
> > which means that you might still
> >
> > shmem = to_drm_gem_shmem_obj(gem_obj);
> > KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
> >
> > /*
> > * If the assertion fails, we now have to call drm_gem_shmem_free(shmem)
> > */
> >
> > /* The scatter/gather table is freed by drm_gem_shmem_free */
> > drm_gem_shmem_free(shmem);
> >
> > /* everything's fine now */
> >
> > The semantics around drm_gem_shmem_free make it a bit convoluted, but
> > doing it using goto/labels, plus handling the assertions and error
> > reporting would be difficult.
> >
> > Using actions, we have:
> >
> > sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > KUNIT_ASSERT_NOT_NULL(test, sgt);
> >
> > ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt);
> > KUNIT_ASSERT_EQ(test, ret, 0);
> >
> > sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >
> > gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> > KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
> > KUNIT_EXPECT_NULL(test, gem_obj->filp);
> > KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
> >
> > /* drm_gem_shmem_free will free the struct sg_table itself */
> > kunit_remove_action(test, sg_free_table_wrapper, sgt);
> > kunit_remove_action(test, kfree_wrapper, sgt);
>
> I agree that using actions makes error handling cleaner. However, I still
> have some concerns about the additional complexity that actions introduce.
> For instance, I feel these two lines make the testing harness more complex
> without asserting any additional property of the component under test.
If anything, the API makes it more difficult to deal with. It would
actually be harder/messier to handle without an action.
> In some sense, I wonder if it is worth worrying about memory leaks when
> a test case fails. At that point, the system is already in an inconsistent
> state due to a bug in the component under test, so it is unsafe to continue
> anyway.
I guess the larger issue is: once that code will be merged, we're going
to have patches to convert to actions because they make it nicer and fix
a couple of issues anyway.
So, if it's still the state we're going to end up in, why not doing it
right from the beginning?
Maxime
On Wed, 2023-11-01 at 11:20 +0530, Jaskaran Singh wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 10/20/2023 3:20 PM, Yong Wu (吴勇) wrote:
> > On Thu, 2023-10-19 at 10:16 +0530, Vijayanand Jitta wrote:
> >>
> >> Instead of having a vendor specific binding for cma area, How
> about
> >> retrieving
> >>
> >
> https://lore.kernel.org/lkml/1594948208-4739-1-git-send-email-hayashi.kunih…
> >> ?
> >> dma_heap_add_cma can just associate cma region and create a heap.
> So,
> >> we can reuse cma heap
> >> code for allocation instead of replicating that code here.
> >>
> >
> > Thanks for the reference. I guess we can't use it. There are two
> > reasons:
> >
> > a) The secure heap driver is a pure software driver and we have no
> > device for it, therefore we cannot call dma_heap_add_cma.
> >
>
> Hi Yong,
>
> We're considering using struct cma as the function argument to
> dma_heap_add_cma() rather than struct device. Would this help
> resolve the problem of usage with dma_heap_add_cma()?
Yes. If we use "struct cma", I guess it works.
>
> > b) The CMA area here is dynamic for SVP. Normally this CMA can be
> used
> > in the kernel. In the SVP case we use cma_alloc to get it and pass
> the
> > entire CMA physical start address and size into TEE to protect the
> CMA
> > region. The original CMA heap cannot help with the TEE part.
> >
>
> Referring the conversation at
>
https://lore.kernel.org/lkml/7a2995de23c24ef22c071c6976c02b97e9b50126.camel…
> ;
>
> since we're considering abstracting secure mem ops, would it make
> sense
> to use the default CMA heap ops (cma_heap_ops), allocate buffers from
> it
> and secure each allocated buffer?
Then it looks you also need tee operation like tee_client_open_session
and tee_client_invoke_func, right?
It seems we also need change a bit for "cma_heap_allocate" to allow cma
support operations from secure heap.
I will send a v2 to move the discussion forward. The v2 is based on our
case, It won't include the cma part.
>
> Thanks,
> Jaskaran.
>
> > Thanks.
> >
> >> Thanks,
> >> Vijay
> >>
> >>
> >>
>
This patchset consists of two parts, the first is from John and TJ.
It adds some heap interfaces, then our kernel users could allocate buffer
from special heap. The second part is adding MTK secure heap for SVP
(Secure Video Path). A total of two heaps are added, one is mtk_svp and
the other is mtk_svp_cma. The mtk_svp buffer is reserved for the secure
world after bootup and it is used for ES/working buffer, while the
mtk_svp_cma buffer is dynamically reserved for the secure world and will
be get ready when we start playing secure videos, this heap is used for the
frame buffer. Once the security video playing is complete, the CMA will be
released.
For easier viewing, I've split the new heap file into several patches.
The consumers of new heap and new interfaces are our codec and drm which
will send upstream soon, probably this week.
Base on v6.6-rc1.
John Stultz (2):
dma-heap: Add proper kref handling on dma-buf heaps
dma-heap: Provide accessors so that in-kernel drivers can allocate
dmabufs from specific heaps
T.J. Mercier (1):
dma-buf: heaps: Deduplicate docs and adopt common format
Yong Wu (6):
dma-buf: heaps: Initialise MediaTek secure heap
dma-buf: heaps: mtk_sec_heap: Initialise tee session
dma-buf: heaps: mtk_sec_heap: Add tee service call for buffer
allocating/freeing
dma-buf: heaps: mtk_sec_heap: Add dma_ops
dt-bindings: reserved-memory: MediaTek: Add reserved memory for SVP
dma_buf: heaps: mtk_sec_heap: Add a new CMA heap
.../mediatek,secure_cma_chunkmem.yaml | 42 ++
drivers/dma-buf/dma-heap.c | 127 +++--
drivers/dma-buf/heaps/Kconfig | 8 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/mtk_secure_heap.c | 458 ++++++++++++++++++
include/linux/dma-heap.h | 42 +-
6 files changed, 630 insertions(+), 48 deletions(-)
create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,secure_cma_chunkmem.yaml
create mode 100644 drivers/dma-buf/heaps/mtk_secure_heap.c
--
2.18.0
Hi,
On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote:
> >> +static void drm_gem_shmem_test_obj_create_private(struct kunit *test)
> >> +{
> >> + struct fake_dev *fdev = test->priv;
> >> + struct drm_gem_shmem_object *shmem;
> >> + struct drm_gem_object *gem_obj;
> >> + struct dma_buf buf_mock;
> >> + struct dma_buf_attachment attach_mock;
> >> + struct sg_table *sgt;
> >> + char *buf;
> >> + int ret;
> >> +
> >> + /* Create a mock scatter/gather table */
> >> + buf = kunit_kzalloc(test, TEST_SIZE, GFP_KERNEL);
> >> + KUNIT_ASSERT_NOT_NULL(test, buf);
> >> +
> >> + sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> >> + KUNIT_ASSERT_NOT_NULL(test, sgt);
> >> +
> >> + ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
> >> + KUNIT_ASSERT_EQ(test, ret, 0);
> >> + sg_init_one(sgt->sgl, buf, TEST_SIZE);
> >> +
> >> + /* Init a mock DMA-BUF */
> >> + buf_mock.size = TEST_SIZE;
> >> + attach_mock.dmabuf = &buf_mock;
> >> +
> >> + gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
> >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
> >> + KUNIT_ASSERT_EQ(test, gem_obj->size, TEST_SIZE);
> >> + KUNIT_ASSERT_NULL(test, gem_obj->filp);
> >> + KUNIT_ASSERT_NOT_NULL(test, gem_obj->funcs);
> >> +
> >> + shmem = to_drm_gem_shmem_obj(gem_obj);
> >> + KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
> >> +
> >> + /* The scatter/gather table is freed by drm_gem_shmem_free */
> >> + drm_gem_shmem_free(shmem);
> >> +}
> >
> > KUNIT_ASSERT_* will stop the execution of the test on failure, you
> > should probably use a bit more of KUNIT_EXPECT_* calls otherwise you'll
> > leak resources.
> >
> > You also probably want to use a kunit_action to clean up and avoid that
> > whole discussion
> >
>
> You are right. I slightly prefer using KUnit expectations (unless actions
> are strictly necessary) since I feel using actions makes test cases a bit
> less straightforward to understand. Is this okay for you?
I disagree. Actions make it easier to reason about, even when comparing
assertion vs expectation
Like, for the call to sg_alloc_table and
drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs
expect would be something like:
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, sgt);
ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
KUNIT_ASSERT_EQ(test, ret, 0);
/*
* Here, it's already not super clear whether you want to expect vs
* assert. expect will make you handle the failure case later, assert will
* force you to call kfree on sgt. Both kind of suck in their own ways.
*/
sg_init_one(sgt->sgl, buf, TEST_SIZE);
gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
/*
* If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt).
*/
KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
KUNIT_EXPECT_NULL(test, gem_obj->filp);
KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
/*
* And here we have to handle the case where the expectation was wrong,
* but the test still continued.
*/
But if you're not using an action, you still have to call kfree(sgt),
which means that you might still
shmem = to_drm_gem_shmem_obj(gem_obj);
KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
/*
* If the assertion fails, we now have to call drm_gem_shmem_free(shmem)
*/
/* The scatter/gather table is freed by drm_gem_shmem_free */
drm_gem_shmem_free(shmem);
/* everything's fine now */
The semantics around drm_gem_shmem_free make it a bit convoluted, but
doing it using goto/labels, plus handling the assertions and error
reporting would be difficult.
Using actions, we have:
sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, sgt);
ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt);
KUNIT_ASSERT_EQ(test, ret, 0);
ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
KUNIT_ASSERT_EQ(test, ret, 0);
ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt);
KUNIT_ASSERT_EQ(test, ret, 0);
sg_init_one(sgt->sgl, buf, TEST_SIZE);
gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE);
KUNIT_EXPECT_NULL(test, gem_obj->filp);
KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);
/* drm_gem_shmem_free will free the struct sg_table itself */
kunit_remove_action(test, sg_free_table_wrapper, sgt);
kunit_remove_action(test, kfree_wrapper, sgt);
shmem = to_drm_gem_shmem_obj(gem_obj);
KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem);
KUNIT_ASSERT_EQ(test, ret, 0);
The last one is arguable, but for the previous ones it makes error
handling much more convenient and easy to reason about.
The wrappers are also a bit inconvenient to use, but it's mostly there
to avoid a compiler warning at the moment.
This patch will help hopefully:
https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@g…
Maxime
Hi,
On Mon, Oct 23, 2023 at 10:25:50AM -0700, Doug Anderson wrote:
> On Mon, Oct 23, 2023 at 9:31 AM Yuran Pereira <yuran.pereira(a)hotmail.com> wrote:
> >
> > Since "Clean up checks for already prepared/enabled in panels" has
> > already been done and merged [1], I think there is no longer a need
> > for this item to be in the gpu TODO.
> >
> > [1] https://patchwork.freedesktop.org/patch/551421/
> >
> > Signed-off-by: Yuran Pereira <yuran.pereira(a)hotmail.com>
> > ---
> > Documentation/gpu/todo.rst | 25 -------------------------
> > 1 file changed, 25 deletions(-)
>
> It's not actually all done. It's in a bit of a limbo state right now,
> unfortunately. I landed all of the "simple" cases where panels were
> needlessly tracking prepare/enable, but the less simple cases are
> still outstanding.
>
> Specifically the issue is that many panels have code to properly power
> cycle themselves off at shutdown time and in order to do that they
> need to keep track of the prepare/enable state. After a big, long
> discussion [1] it was decided that we could get rid of all the panel
> code handling shutdown if only all relevant DRM KMS drivers would
> properly call drm_atomic_helper_shutdown().
>
> I made an attempt to get DRM KMS drivers to call
> drm_atomic_helper_shutdown() [2] [3] [4]. I was able to land the
> patches that went through drm-misc, but currently many of the
> non-drm-misc ones are blocked waiting for attention.
>
> ...so things that could be done to help out:
>
> a) Could review patches that haven't landed in [4]. Maybe adding a
> Reviewed-by tag would help wake up maintainers?
>
> b) Could see if you can identify panels that are exclusively used w/
> DRM drivers that have already been converted and then we could post
> patches for just those panels. I have no idea how easy this task would
> be. Is it enough to look at upstream dts files by "compatible" string?
I think it is, yes.
Maxime
The patch series provides drm driver support for enabling secure video
path (SVP) playback on MediaiTek hardware in the Linux kernel.
Memory Definitions:
secure memory - Memory allocated in the TEE (Trusted Execution
Environment) which is inaccessible in the REE (Rich Execution
Environment, i.e. linux kernel/userspace).
secure handle - Integer value which acts as reference to 'secure
memory'. Used in communication between TEE and REE to reference
'secure memory'.
secure buffer - 'secure memory' that is used to store decrypted,
compressed video or for other general purposes in the TEE.
secure surface - 'secure memory' that is used to store graphic buffers.
Memory Usage in SVP:
The overall flow of SVP starts with encrypted video coming in from an
outside source into the REE. The REE will then allocate a 'secure
buffer' and send the corresponding 'secure handle' along with the
encrypted, compressed video data to the TEE. The TEE will then decrypt
the video and store the result in the 'secure buffer'. The REE will
then allocate a 'secure surface'. The REE will pass the 'secure
handles' for both the 'secure buffer' and 'secure surface' into the
TEE for video decoding. The video decoder HW will then decode the
contents of the 'secure buffer' and place the result in the 'secure
surface'. The REE will then attach the 'secure surface' to the overlay
plane for rendering of the video.
Everything relating to ensuring security of the actual contents of the
'secure buffer' and 'secure surface' is out of scope for the REE and
is the responsibility of the TEE.
DRM driver handles allocation of gem objects that are backed by a 'secure
surface' and for displaying a 'secure surface' on the overlay plane.
This introduces a new flag for object creation called
DRM_MTK_GEM_CREATE_ENCRYPTED which indicates it should be a 'secure
surface'. All changes here are in MediaTek specific code.
---
Based on 3 series and 1 patch:
[1] dma-buf: heaps: Add MediaTek secure heap
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776
[2] add driver to support secure video decoder
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=782922
[3] soc: mediatek: Add register definitions for GCE
- https://patchwork.kernel.org/project/linux-mediatek/patch/20231017064717.21…
[4] Add CMDQ secure driver for SVP
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=795502
---
Change in v2:
1. remove the DRIVER_RDNDER flag for mtk_drm_ioctl
2. move cmdq_insert_backup_cookie into client driver
3. move secure gce node define from mt8195-cherry.dtsi to mt8195.dtsi
---
CK Hu (1):
drm/mediatek: Add interface to allocate MediaTek GEM buffer.
Jason-JH.Lin (10):
drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED flag
drm/mediatek: Add secure buffer control flow to mtk_drm_gem
drm/mediatek: Add secure identify flag and funcution to mtk_drm_plane
drm/mediatek: Add mtk_ddp_sec_write to config secure buffer info
drm/mediatek: Add get_sec_port interface to mtk_ddp_comp
drm/mediatek: Add secure layer config support for ovl
drm/mediatek: Add secure layer config support for ovl_adaptor
drm/mediatek: Add secure flow support to mediatek-drm
drm/mediatek: Add cmdq_insert_backup_cookie before secure pkt finalize
arm64: dts: mt8195: Add secure mbox settings for vdosys
arch/arm64/boot/dts/mediatek/mt8195.dtsi | 6 +-
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 31 +-
.../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 15 +
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 274 +++++++++++++++++-
drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 14 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 13 +
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 121 ++++++++
drivers/gpu/drm/mediatek/mtk_drm_gem.h | 16 +
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 7 +
drivers/gpu/drm/mediatek/mtk_drm_plane.h | 2 +
drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 11 +-
drivers/gpu/drm/mediatek/mtk_mdp_rdma.h | 2 +
include/uapi/drm/mediatek_drm.h | 59 ++++
16 files changed, 570 insertions(+), 18 deletions(-)
create mode 100644 include/uapi/drm/mediatek_drm.h
--
2.18.0
Currently, a NULL pointer dereference will happen in function
`dma_fence_enable_sw_signaling()` (at line 615), in case `chain`
is not allocated in `mock_chain()` and this function returns
`NULL` (at line 86). See below:
drivers/dma-buf/st-dma-fence-chain.c:
86 chain = mock_chain(NULL, f, 1);
87 if (!chain)
88 err = -ENOMEM;
89
90 dma_fence_enable_sw_signaling(chain);
drivers/dma-buf/dma-fence.c:
611 void dma_fence_enable_sw_signaling(struct dma_fence *fence)
612 {
613 unsigned long flags;
614
615 spin_lock_irqsave(fence->lock, flags);
^^^^^^^^^^^
|
NULL pointer reference
if fence == NULL
616 __dma_fence_enable_signaling(fence);
617 spin_unlock_irqrestore(fence->lock, flags);
618 }
Fix this by adding a NULL check before dereferencing `fence` in
`dma_fence_enable_sw_signaling()`. This will prevent any other NULL
pointer dereference when the `fence` passed as an argument is `NULL`.
Addresses-Coverity: ("Dereference after null check")
Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for selftests")
Cc: stable(a)vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars(a)kernel.org>
---
drivers/dma-buf/dma-fence.c | 9 ++++++++-
include/linux/dma-fence.h | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 8aa8f8cb7071..4d2f13560d0f 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -607,14 +607,21 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
* This will request for sw signaling to be enabled, to make the fence
* complete as soon as possible. This calls &dma_fence_ops.enable_signaling
* internally.
+ *
+ * Returns 0 on success and a negative error value when @fence is NULL.
*/
-void dma_fence_enable_sw_signaling(struct dma_fence *fence)
+int dma_fence_enable_sw_signaling(struct dma_fence *fence)
{
unsigned long flags;
+ if (!fence)
+ return -EINVAL;
+
spin_lock_irqsave(fence->lock, flags);
__dma_fence_enable_signaling(fence);
spin_unlock_irqrestore(fence->lock, flags);
+
+ return 0;
}
EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index ebe78bd3d121..1e4025e925e6 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -399,7 +399,7 @@ int dma_fence_add_callback(struct dma_fence *fence,
dma_fence_func_t func);
bool dma_fence_remove_callback(struct dma_fence *fence,
struct dma_fence_cb *cb);
-void dma_fence_enable_sw_signaling(struct dma_fence *fence);
+int dma_fence_enable_sw_signaling(struct dma_fence *fence);
/**
* dma_fence_is_signaled_locked - Return an indication if the fence
--
2.34.1
Hello.
I've got a VM from a cloud provider, and since v6.5 I observe the following kfence splat in dmesg during boot:
```
BUG: KFENCE: memory corruption in drm_gem_put_pages+0x186/0x250
Corrupted memory at 0x00000000e173a294 [ ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ! ] (in kfence-#108):
drm_gem_put_pages+0x186/0x250
drm_gem_shmem_put_pages_locked+0x43/0xc0
drm_gem_shmem_object_vunmap+0x83/0xe0
drm_gem_vunmap_unlocked+0x46/0xb0
drm_fbdev_generic_helper_fb_dirty+0x1dc/0x310
drm_fb_helper_damage_work+0x96/0x170
process_one_work+0x254/0x470
worker_thread+0x55/0x4f0
kthread+0xe8/0x120
ret_from_fork+0x34/0x50
ret_from_fork_asm+0x1b/0x30
kfence-#108: 0x00000000cda343af-0x00000000aec2c095, size=3072, cache=kmalloc-4k
allocated by task 51 on cpu 0 at 14.668667s:
drm_gem_get_pages+0x94/0x2b0
drm_gem_shmem_get_pages+0x5d/0x110
drm_gem_shmem_object_vmap+0xc4/0x1e0
drm_gem_vmap_unlocked+0x3c/0x70
drm_client_buffer_vmap+0x23/0x50
drm_fbdev_generic_helper_fb_dirty+0xae/0x310
drm_fb_helper_damage_work+0x96/0x170
process_one_work+0x254/0x470
worker_thread+0x55/0x4f0
kthread+0xe8/0x120
ret_from_fork+0x34/0x50
ret_from_fork_asm+0x1b/0x30
freed by task 51 on cpu 0 at 14.668697s:
drm_gem_put_pages+0x186/0x250
drm_gem_shmem_put_pages_locked+0x43/0xc0
drm_gem_shmem_object_vunmap+0x83/0xe0
drm_gem_vunmap_unlocked+0x46/0xb0
drm_fbdev_generic_helper_fb_dirty+0x1dc/0x310
drm_fb_helper_damage_work+0x96/0x170
process_one_work+0x254/0x470
worker_thread+0x55/0x4f0
kthread+0xe8/0x120
ret_from_fork+0x34/0x50
ret_from_fork_asm+0x1b/0x30
CPU: 0 PID: 51 Comm: kworker/0:2 Not tainted 6.5.0-pf4 #1 8b557a4173114d86eef7240f7a080080cfc4617e
Hardware name: Red Hat KVM, BIOS 1.11.0-2.el7 04/01/2014
Workqueue: events drm_fb_helper_damage_work
```
This repeats a couple of times and then stops.
Currently, I'm running v6.5.5. So far, there's no impact on how VM functions for me.
The VGA adapter is as follows: 00:02.0 VGA compatible controller: Cirrus Logic GD 5446
Please check.
Thanks.
--
Oleksandr Natalenko (post-factum)
You can remove the DRIVER_RENDER flag from this patchset. That should
not be upstreamed. The IOCTLs are still needed though because of the
flag for allocating a secure surface that is in the next patch. If
that flag wasn't needed, then dumb buffer allocations could be used
instead.
Thanks,
Jeff Kardatzke
The buffer->pages[] has "buffer->pagecount" elements so this > comparison
has to be changed to >= to avoid reading beyond the end of the array.
The buffer->pages[] array is allocated in cma_heap_allocate().
Fixes: a5d2d29e24be ("dma-buf: heaps: Move heap-helper logic into the cma_heap implementation")
Signed-off-by: Dan Carpenter <dan.carpenter(a)linaro.org>
---
drivers/dma-buf/heaps/cma_heap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index ee899f8e6721..bea7e574f916 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -165,7 +165,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf)
struct vm_area_struct *vma = vmf->vma;
struct cma_heap_buffer *buffer = vma->vm_private_data;
- if (vmf->pgoff > buffer->pagecount)
+ if (vmf->pgoff >= buffer->pagecount)
return VM_FAULT_SIGBUS;
vmf->page = buffer->pages[vmf->pgoff];
--
2.39.2
If 'list_limit' is set to a very high value, 'lsize' computation could
overflow if 'head.count' is big enough.
In such a case, udmabuf_create() will access to memory beyond 'list'.
Use size_mul() to saturate the value, and have memdup_user() fail.
Fixes: fbb0de795078 ("Add udmabuf misc device")
Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr>
---
drivers/dma-buf/udmabuf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index c40645999648..fb4c4b5b3332 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -314,13 +314,13 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
struct udmabuf_create_list head;
struct udmabuf_create_item *list;
int ret = -EINVAL;
- u32 lsize;
+ size_t lsize;
if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
return -EFAULT;
if (head.count > list_limit)
return -EINVAL;
- lsize = sizeof(struct udmabuf_create_item) * head.count;
+ lsize = size_mul(sizeof(struct udmabuf_create_item), head.count);
list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
if (IS_ERR(list))
return PTR_ERR(list);
--
2.34.1
On Tue, Sep 19, 2023 at 10:26 PM CK Hu (胡俊光) <ck.hu(a)mediatek.com> wrote:
>
> Hi, Jason:
>
> On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> > The patch series provides drm driver support for enabling secure
> > video
> > path (SVP) playback on MediaiTek hardware in the Linux kernel.
> >
> > Memory Definitions:
> > secure memory - Memory allocated in the TEE (Trusted Execution
> > Environment) which is inaccessible in the REE (Rich Execution
> > Environment, i.e. linux kernel/userspace).
> > secure handle - Integer value which acts as reference to 'secure
> > memory'. Used in communication between TEE and REE to reference
> > 'secure memory'.
> > secure buffer - 'secure memory' that is used to store decrypted,
> > compressed video or for other general purposes in the TEE.
> > secure surface - 'secure memory' that is used to store graphic
> > buffers.
> >
> > Memory Usage in SVP:
> > The overall flow of SVP starts with encrypted video coming in from an
> > outside source into the REE. The REE will then allocate a 'secure
> > buffer' and send the corresponding 'secure handle' along with the
> > encrypted, compressed video data to the TEE. The TEE will then
> > decrypt
> > the video and store the result in the 'secure buffer'. The REE will
> > then allocate a 'secure surface'. The REE will pass the 'secure
> > handles' for both the 'secure buffer' and 'secure surface' into the
> > TEE for video decoding. The video decoder HW will then decode the
> > contents of the 'secure buffer' and place the result in the 'secure
> > surface'. The REE will then attach the 'secure surface' to the
> > overlay
> > plane for rendering of the video.
> >
> > Everything relating to ensuring security of the actual contents of
> > the
> > 'secure buffer' and 'secure surface' is out of scope for the REE and
> > is the responsibility of the TEE.
> >
> > DRM driver handles allocation of gem objects that are backed by a
> > 'secure
> > surface' and for displaying a 'secure surface' on the overlay plane.
> > This introduces a new flag for object creation called
> > DRM_MTK_GEM_CREATE_ENCRYPTED which indicates it should be a 'secure
> > surface'. All changes here are in MediaTek specific code.
>
> How do you define SVP? Is there standard requirement we could refer to?
> If the secure video buffer is read by display hardware and output to
> HDMI without any protection and user could capture HDMI signal, is this
> secure?
SVP (Secure Video Path) is essentially the video being completed
isolated from the kernel/userspace. The specific requirements for it
vary between implementations.
Regarding HDMI/HDCP output; it's the responsibility of the TEE to
enforce that. Nothing on the kernel/userspace side needs to be
concerned about enforcing HDCP. The only thing userspace is involved
in there is actually turning on HDCP via the kernel drivers; and then
the TEE ensures that it is active if the policy for the encrypted
content requires it.
>
> Regards,
> CK
>
> >
> > ---
> > Based on 2 series:
> > [1] Add CMDQ secure driver for SVP
> > -
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-medi…
> >
> >
> > [2] dma-buf: heaps: Add MediaTek secure heap
> > -
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-medi…
> >
> > ---
> >
> > CK Hu (1):
> > drm/mediatek: Add interface to allocate MediaTek GEM buffer.
> >
> > Jason-JH.Lin (9):
> > drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED flag
> > drm/mediatek: Add secure buffer control flow to mtk_drm_gem
> > drm/mediatek: Add secure identify flag and funcution to
> > mtk_drm_plane
> > drm/mediatek: Add mtk_ddp_sec_write to config secure buffer info
> > drm/mediatek: Add get_sec_port interface to mtk_ddp_comp
> > drm/mediatek: Add secure layer config support for ovl
> > drm/mediatek: Add secure layer config support for ovl_adaptor
> > drm/mediatek: Add secure flow support to mediatek-drm
> > arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys
> >
> > .../boot/dts/mediatek/mt8195-cherry.dtsi | 10 +
> > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
> > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 31 +-
> > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 15 +
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 271
> > +++++++++++++++++-
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 +
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 14 +
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 16 +-
> > drivers/gpu/drm/mediatek/mtk_drm_gem.c | 121 ++++++++
> > drivers/gpu/drm/mediatek/mtk_drm_gem.h | 16 ++
> > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 7 +
> > drivers/gpu/drm/mediatek/mtk_drm_plane.h | 2 +
> > drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 11 +-
> > drivers/gpu/drm/mediatek/mtk_mdp_rdma.h | 2 +
> > include/uapi/drm/mediatek_drm.h | 59 ++++
> > 16 files changed, 575 insertions(+), 17 deletions(-)
> > create mode 100644 include/uapi/drm/mediatek_drm.h
> >
The patch series provides drm driver support for enabling secure video
path (SVP) playback on MediaiTek hardware in the Linux kernel.
Memory Definitions:
secure memory - Memory allocated in the TEE (Trusted Execution
Environment) which is inaccessible in the REE (Rich Execution
Environment, i.e. linux kernel/userspace).
secure handle - Integer value which acts as reference to 'secure
memory'. Used in communication between TEE and REE to reference
'secure memory'.
secure buffer - 'secure memory' that is used to store decrypted,
compressed video or for other general purposes in the TEE.
secure surface - 'secure memory' that is used to store graphic buffers.
Memory Usage in SVP:
The overall flow of SVP starts with encrypted video coming in from an
outside source into the REE. The REE will then allocate a 'secure
buffer' and send the corresponding 'secure handle' along with the
encrypted, compressed video data to the TEE. The TEE will then decrypt
the video and store the result in the 'secure buffer'. The REE will
then allocate a 'secure surface'. The REE will pass the 'secure
handles' for both the 'secure buffer' and 'secure surface' into the
TEE for video decoding. The video decoder HW will then decode the
contents of the 'secure buffer' and place the result in the 'secure
surface'. The REE will then attach the 'secure surface' to the overlay
plane for rendering of the video.
Everything relating to ensuring security of the actual contents of the
'secure buffer' and 'secure surface' is out of scope for the REE and
is the responsibility of the TEE.
DRM driver handles allocation of gem objects that are backed by a 'secure
surface' and for displaying a 'secure surface' on the overlay plane.
This introduces a new flag for object creation called
DRM_MTK_GEM_CREATE_ENCRYPTED which indicates it should be a 'secure
surface'. All changes here are in MediaTek specific code.
---
Based on 2 series:
[1] Add CMDQ secure driver for SVP
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332
[2] dma-buf: heaps: Add MediaTek secure heap
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776
---
CK Hu (1):
drm/mediatek: Add interface to allocate MediaTek GEM buffer.
Jason-JH.Lin (9):
drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED flag
drm/mediatek: Add secure buffer control flow to mtk_drm_gem
drm/mediatek: Add secure identify flag and funcution to mtk_drm_plane
drm/mediatek: Add mtk_ddp_sec_write to config secure buffer info
drm/mediatek: Add get_sec_port interface to mtk_ddp_comp
drm/mediatek: Add secure layer config support for ovl
drm/mediatek: Add secure layer config support for ovl_adaptor
drm/mediatek: Add secure flow support to mediatek-drm
arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys
.../boot/dts/mediatek/mt8195-cherry.dtsi | 10 +
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 31 +-
.../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 15 +
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 271 +++++++++++++++++-
drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 14 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 16 +-
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 121 ++++++++
drivers/gpu/drm/mediatek/mtk_drm_gem.h | 16 ++
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 7 +
drivers/gpu/drm/mediatek/mtk_drm_plane.h | 2 +
drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 11 +-
drivers/gpu/drm/mediatek/mtk_mdp_rdma.h | 2 +
include/uapi/drm/mediatek_drm.h | 59 ++++
16 files changed, 575 insertions(+), 17 deletions(-)
create mode 100644 include/uapi/drm/mediatek_drm.h
--
2.18.0
Dzień dobry,
czy jest możliwość, by na pewien czas Państwa zakład produkcyjny mógł zrezygnować z części lub całości zużywanej energii?
W zamian za gotowość do redukcji i jej wykonanie mogą otrzymać Państwo stałe wynagrodzenie, które w przeliczeniu na 1 MW redukcji mocy wynosi od 500 do 720 tys. zł w zależności od czasu zdolności do redukcji.
Uczestnictwo w programie DSR (Demand Side Response) to dla Państwa brak kosztów implementacji i wzrost bezpieczeństwa energetycznego.
Jeśli interesuje Państwa generowanie wieloletnich przychodów z programu DSR, proszę o wiadomość.
Pozdrawiam
Dawid Jarocki