On 17/07/2019 19:33, Rob Herring wrote:
> Executable buffers have an alignment restriction that they can't cross
> 16MB boundary as the GPU program counter is 24-bits. This restriction is
> currently not handled and we just get lucky. As current userspace
Actually it depends which GPU you are using - some do have a bigger
program counter - so it might be the choice of GPU to test on rather
than just luck. But kbase always assumes the worst case (24 bit) as in
practise that's enough.
> assumes all BOs are executable, that has to remain the default. So add a
> new PANFROST_BO_NOEXEC flag to allow userspace to indicate which BOs are
> not executable.
>
> There is also a restriction that executable buffers cannot start or end
> on a 4GB boundary. This is mostly avoided as there is only 4GB of space
> currently and the beginning is already blocked out for NULL ptr
> detection. Add support to handle this restriction fully regardless of
> the current constraints.
>
> For existing userspace, all created BOs remain executable, but the GPU
> VA alignment will be increased to the size of the BO. This shouldn't
> matter as there is plenty of GPU VA space.
>
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Steven Price <steven.price@xxxxxxx>
> Cc: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> drivers/gpu/drm/panfrost/panfrost_drv.c | 20 +++++++++++++++++++-
> drivers/gpu/drm/panfrost/panfrost_gem.c | 18 ++++++++++++++++--
> drivers/gpu/drm/panfrost/panfrost_gem.h | 3 ++-
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 3 +++
> include/uapi/drm/panfrost_drm.h | 2 ++
> 5 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index d354b92964d5..b91e991bc6a3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -49,7 +49,8 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> struct panfrost_gem_object *bo;
> struct drm_panfrost_create_bo *args = data;
>
> - if (!args->size || args->flags || args->pad)
> + if (!args->size || args->pad ||
> + (args->flags & ~PANFROST_BO_NOEXEC))
> return -EINVAL;
>
> bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
> @@ -367,6 +368,22 @@ static struct drm_driver panfrost_drm_driver = {
> .gem_prime_mmap = drm_gem_prime_mmap,
> };
>
> +#define PFN_4G_MASK ((SZ_4G - 1) >> PAGE_SHIFT)
> +
> +static void panfrost_drm_mm_color_adjust(const struct drm_mm_node *node,
> + unsigned long color,
> + u64 *start, u64 *end)
> +{
> + /* Executable buffers can't start or end on a 4GB boundary */
> + if (!color) {
> + if ((*start & PFN_4G_MASK) == 0)
> + (*start)++;
> +
> + if ((*end & PFN_4G_MASK) == 0)
> + (*end)--;
> + }
> +}
Unless I'm mistaken this won't actually provide the guarantee if the
memory region is >4GB (which admittedly it isn't at the moment). For
example a 8GB region would have the beginning/end trimmed off, but
there's another 4GB in the middle which could be allocated next to.
Also a minor issue, but we might want to consider having some constants
for 'color' - it's not obvious from this code that color==no_exec.
Steve
> +
> static int panfrost_probe(struct platform_device *pdev)
> {
> struct panfrost_device *pfdev;
> @@ -394,6 +411,7 @@ static int panfrost_probe(struct platform_device *pdev)
>
> /* 4G enough for now. can be 48-bit */
> drm_mm_init(&pfdev->mm, SZ_32M >> PAGE_SHIFT, (SZ_4G - SZ_32M) >> PAGE_SHIFT);
> + pfdev->mm.color_adjust = panfrost_drm_mm_color_adjust;
>
> pm_runtime_use_autosuspend(pfdev->dev);
> pm_runtime_set_autosuspend_delay(pfdev->dev, 50); /* ~3 frames */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index df70dcf3cb2f..37ffec8391da 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -66,11 +66,23 @@ static int panfrost_gem_map(struct panfrost_device *pfdev, struct panfrost_gem_o
> {
> int ret;
> size_t size = bo->base.base.size;
> - u64 align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
> + u64 align;
> +
> + /*
> + * Executable buffers cannot cross a 16MB boundary as the program
> + * counter is 24-bits. We assume executable buffers will be less than
> + * 16MB and aligning executable buffers to their size will avoid
> + * crossing a 16MB boundary.
> + */
> + if (!bo->noexec)
> + align = size >> PAGE_SHIFT;
> + else
> + align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
>
> spin_lock(&pfdev->mm_lock);
> ret = drm_mm_insert_node_generic(&pfdev->mm, &bo->node,
> - size >> PAGE_SHIFT, align, 0, 0);
> + size >> PAGE_SHIFT, align,
> + bo->noexec, 0);
> spin_unlock(&pfdev->mm_lock);
> if (ret)
> return ret;
> @@ -96,6 +108,7 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
> return ERR_CAST(shmem);
>
> bo = to_panfrost_bo(&shmem->base);
> + bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
>
> ret = panfrost_gem_map(pfdev, bo);
> if (ret)
> @@ -123,6 +136,7 @@ panfrost_gem_prime_import_sg_table(struct drm_device *dev,
> return ERR_CAST(obj);
>
> pobj = to_panfrost_bo(obj);
> + pobj->noexec = true;
>
> ret = panfrost_gem_map(pfdev, pobj);
> if (ret)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index ce065270720b..132f02399b7b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -11,7 +11,8 @@ struct panfrost_gem_object {
> struct drm_gem_shmem_object base;
>
> struct drm_mm_node node;
> - bool is_mapped;
> + bool is_mapped :1;
> + bool noexec :1;
> };
>
> static inline
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 5383b837f04b..d18484a07bfa 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -212,6 +212,9 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)
> if (WARN_ON(bo->is_mapped))
> return 0;
>
> + if (bo->noexec)
> + prot |= IOMMU_NOEXEC;
> +
> 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 b5d370638846..17fb5d200f7a 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -82,6 +82,8 @@ struct drm_panfrost_wait_bo {
> __s64 timeout_ns; /* absolute */
> };
>
> +#define PANFROST_BO_NOEXEC 1
> +
> /**
> * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
> *
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
