[PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

So we can create GPU mappings without R/W permissions. Particularly
useful to debug corruptions caused by out-of-bound writes.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 14 ++++++++++++--
 drivers/gpu/drm/panfrost/panfrost_gem.c |  2 ++
 drivers/gpu/drm/panfrost/panfrost_gem.h |  2 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c |  8 +++++++-
 include/uapi/drm/panfrost_drm.h         |  2 ++
 5 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 82ad9a67f251..40e4a4db3ab1 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -75,6 +75,10 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
 	return 0;
 }
 
+#define PANFROST_BO_FLAGS \
+	(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP | \
+	 PANFROST_BO_NOREAD | PANFROST_BO_NOWRITE)
+
 static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
@@ -84,7 +88,7 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 	struct panfrost_gem_mapping *mapping;
 
 	if (!args->size || args->pad ||
-	    (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
+	    (args->flags & ~PANFROST_BO_FLAGS))
 		return -EINVAL;
 
 	/* Heaps should never be executable */
@@ -92,6 +96,11 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 	    !(args->flags & PANFROST_BO_NOEXEC))
 		return -EINVAL;
 
+	/* Executable implies readable */
+	if ((args->flags & PANFROST_BO_NOREAD) &&
+	    !(args->flags & PANFROST_BO_NOEXEC))
+		return -EINVAL;
+
 	bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
 					     &args->handle);
 	if (IS_ERR(bo))
@@ -520,6 +529,7 @@ DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
  * - 1.0 - initial interface
  * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO
  * - 1.2 - adds AFBC_FEATURES query
+ * - 1.3 - adds PANFROST_BO_NO{READ,WRITE} flags
  */
 static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
@@ -532,7 +542,7 @@ static const struct drm_driver panfrost_drm_driver = {
 	.desc			= "panfrost DRM",
 	.date			= "20180908",
 	.major			= 1,
-	.minor			= 2,
+	.minor			= 3,
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 23377481f4e3..d6c1bb1445f2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -251,6 +251,8 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
 
 	bo = to_panfrost_bo(&shmem->base);
 	bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
+	bo->noread = !!(flags & PANFROST_BO_NOREAD);
+	bo->nowrite = !!(flags & PANFROST_BO_NOWRITE);
 	bo->is_heap = !!(flags & PANFROST_BO_HEAP);
 
 	/*
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 8088d5fd8480..6246b5fef446 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -37,6 +37,8 @@ struct panfrost_gem_object {
 	atomic_t gpu_usecount;
 
 	bool noexec		:1;
+	bool noread		:1;
+	bool nowrite		:1;
 	bool is_heap		:1;
 };
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index f51d3f791a17..6a5c9d94d6f2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -307,7 +307,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	struct drm_gem_object *obj = &bo->base.base;
 	struct panfrost_device *pfdev = to_panfrost_device(obj->dev);
 	struct sg_table *sgt;
-	int prot = IOMMU_READ | IOMMU_WRITE;
+	int prot = 0;
 
 	if (WARN_ON(mapping->active))
 		return 0;
@@ -315,6 +315,12 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	if (bo->noexec)
 		prot |= IOMMU_NOEXEC;
 
+	if (!bo->nowrite)
+		prot |= IOMMU_WRITE;
+
+	if (!bo->noread)
+		prot |= IOMMU_READ;
+
 	sgt = drm_gem_shmem_get_pages_sgt(obj);
 	if (WARN_ON(IS_ERR(sgt)))
 		return PTR_ERR(sgt);
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index 061e700dd06c..a2de81225125 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -86,6 +86,8 @@ struct drm_panfrost_wait_bo {
 
 #define PANFROST_BO_NOEXEC	1
 #define PANFROST_BO_HEAP	2
+#define PANFROST_BO_NOREAD	4
+#define PANFROST_BO_NOWRITE	8
 
 /**
  * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
-- 
2.31.1

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Alyssa Rosenzweig]

> +	/* Executable implies readable */
> +	if ((args->flags & PANFROST_BO_NOREAD) &&
> +	    !(args->flags & PANFROST_BO_NOEXEC))
> +		return -EINVAL;

Generally, executable also implies not-writeable. Should we check that?

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

On Thu, 30 Sep 2021 15:13:29 -0400
Alyssa Rosenzweig <alyssa@collabora.com> wrote:

> > +	/* Executable implies readable */
> > +	if ((args->flags & PANFROST_BO_NOREAD) &&
> > +	    !(args->flags & PANFROST_BO_NOEXEC))
> > +		return -EINVAL;  
> 
> Generally, executable also implies not-writeable. Should we check that?

We were allowing it until now, so doing that would break the backward
compat, unfortunately. Steve also mentioned that the DDK might use
shaders modifying other shaders here [1], it clearly doesn't happen in
panfrost, but I think I'd prefer to keep the existing behavior by
default, just to be safe. I'll send a patch setting the RO flag on
all executable BOs in mesa/panfrost.

[1]https://oftc.irclog.whitequark.org/panfrost/2021-09-02

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

On Thu, 30 Sep 2021 20:47:23 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> So we can create GPU mappings without R/W permissions. Particularly
> useful to debug corruptions caused by out-of-bound writes.

Oops, I forgot to add the PANFROST_BO_PRIVATE flag suggested by Robin
here [1]. I'll send a v2.

[1]https://oftc.irclog.whitequark.org/panfrost/2021-09-02

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Alyssa Rosenzweig]

> > > +	/* Executable implies readable */
> > > +	if ((args->flags & PANFROST_BO_NOREAD) &&
> > > +	    !(args->flags & PANFROST_BO_NOEXEC))
> > > +		return -EINVAL;  
> > 
> > Generally, executable also implies not-writeable. Should we check that?
> 
> We were allowing it until now, so doing that would break the backward
> compat, unfortunately.

Not a problem if you only enforce this starting with the appropriate
UABI version, but...

> Steve also mentioned that the DDK might use shaders modifying other
> shaders here [1]

What? I believe it, but what?

For the case of pilot shaders, that shouldn't require self-modifying
code. As I understand, the DDK binds the push uniform (FAU / RMU) buffer
as global shader memory (SSBO) and uses regular STORE instructions on
it. That requires writability on that BO but that should be fine.

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

On Thu, 30 Sep 2021 18:12:11 -0400
Alyssa Rosenzweig <alyssa@collabora.com> wrote:

> > > > +	/* Executable implies readable */
> > > > +	if ((args->flags & PANFROST_BO_NOREAD) &&
> > > > +	    !(args->flags & PANFROST_BO_NOEXEC))
> > > > +		return -EINVAL;    
> > > 
> > > Generally, executable also implies not-writeable. Should we check that?  
> > 
> > We were allowing it until now, so doing that would break the backward
> > compat, unfortunately.  
> 
> Not a problem if you only enforce this starting with the appropriate
> UABI version, but...

I still don't see how that solves the <old-userspace,new-kernel>
situation, since old-userspace doesn't know about the new UABI, and
there's no version field on the CREATE_BO ioctl() to let the kernel
know about the UABI used by this userspace program. I mean, we could
add one, or add a new PANFROST_BO_EXTENDED_FLAGS flag to enforce this
'noexec implies nowrite' behavior, but is it really simpler than
explicitly passing the NOWRITE flag when NOEXEC is passed?

> 
> > Steve also mentioned that the DDK might use shaders modifying other
> > shaders here [1]  
> 
> What? I believe it, but what?
> 
> For the case of pilot shaders, that shouldn't require self-modifying
> code. As I understand, the DDK binds the push uniform (FAU / RMU) buffer
> as global shader memory (SSBO) and uses regular STORE instructions on
> it. That requires writability on that BO but that should be fine.

Okay.

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Boris Brezillon]

Hi Robin,

On Thu, 30 Sep 2021 21:44:24 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu, 30 Sep 2021 20:47:23 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > So we can create GPU mappings without R/W permissions. Particularly
> > useful to debug corruptions caused by out-of-bound writes.  
> 
> Oops, I forgot to add the PANFROST_BO_PRIVATE flag suggested by Robin
> here [1]. I'll send a v2.

When you're talking about a PANFROST_BO_GPU_PRIVATE flag (or
PANFROST_BO_NO_CPU_ACCESS), you mean something that can set
ARM_LPAE_PTE_SH_IS instead of the unconditional ARM_LPAE_PTE_SH_OS we
have right now [1], right? In this case, how would you pass this info
to the iommu? Looks like we have an IOMMU_CACHE, but I don't think
it reflects what we're trying to do. IOMMU_PRIV is about privileged
mappings, so definitely not what we want. Should we add a new
IOMMU_NO_{EXTERNAL,HOST,CPU}_ACCESS flag for that?

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/iommu/io-pgtable-arm.c#L453

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Alyssa Rosenzweig]

> > > > > +	/* Executable implies readable */
> > > > > +	if ((args->flags & PANFROST_BO_NOREAD) &&
> > > > > +	    !(args->flags & PANFROST_BO_NOEXEC))
> > > > > +		return -EINVAL;    
> > > > 
> > > > Generally, executable also implies not-writeable. Should we check that?  
> > > 
> > > We were allowing it until now, so doing that would break the backward
> > > compat, unfortunately.  
> > 
> > Not a problem if you only enforce this starting with the appropriate
> > UABI version, but...
> 
> I still don't see how that solves the <old-userspace,new-kernel>
> situation, since old-userspace doesn't know about the new UABI, and
> there's no version field on the CREATE_BO ioctl() to let the kernel
> know about the UABI used by this userspace program. I mean, we could
> add one, or add a new PANFROST_BO_EXTENDED_FLAGS flag to enforce this
> 'noexec implies nowrite' behavior, but is it really simpler than
> explicitly passing the NOWRITE flag when NOEXEC is passed?

For some reason I thought the ABI version was negotiated (it is in
kbase). Don't worry about it.

That commit is

	Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Re: [PATCH] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags

[Steven Price]

On 30/09/2021 23:12, Alyssa Rosenzweig wrote:
>>>> +	/* Executable implies readable */
>>>> +	if ((args->flags & PANFROST_BO_NOREAD) &&
>>>> +	    !(args->flags & PANFROST_BO_NOEXEC))
>>>> +		return -EINVAL;  
>>>
>>> Generally, executable also implies not-writeable. Should we check that?
>>
>> We were allowing it until now, so doing that would break the backward
>> compat, unfortunately.
> 
> Not a problem if you only enforce this starting with the appropriate
> UABI version, but...
> 
>> Steve also mentioned that the DDK might use shaders modifying other
>> shaders here [1]
> 
> What? I believe it, but what?

*might* ;) I've not looked in detail and a quick look through the code
just now I can't actually find anything which does.

> For the case of pilot shaders, that shouldn't require self-modifying
> code. As I understand, the DDK binds the push uniform (FAU / RMU) buffer
> as global shader memory (SSBO) and uses regular STORE instructions on
> it. That requires writability on that BO but that should be fine.

Yeah it looks like you're correct here - I've never looked closely into
exactly how pilot shaders work. It would appear that the DDK checks (in
user space) for the GPU-executable + GPU-writable condition and moans.
So this obviously isn't used by the DDK as it stands.

For the actual patch:

Reviewed-by: Steven Price <steven.price@arm.com>

Although if you can figure out how to add the "private mapping" flag at
the same time we can avoid bumping the version number too many times.
And today I'm actually in the office so (perversely) I haven't got the
hardware to actually test it at the moment - I really need to get remote
access set up!

Thanks,

Steve