| Message ID | 20210930184723.1482426-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags | expand |
> + /* 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?
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
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
> > > + /* 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.
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.
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
> > > > > + /* 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>
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
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.
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(-)