| Message ID | 20190717183352.22519-3-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] drm/panfrost: Restructure the GEM object creation | expand |
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@collabora.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Steven Price <steven.price@arm.com> > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > 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. > * >
On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> wrote: > > 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@collabora.com> > > Cc: Boris Brezillon <boris.brezillon@collabora.com> > > Cc: Robin Murphy <robin.murphy@arm.com> > > Cc: Steven Price <steven.price@arm.com> > > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > 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. Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm not sure how we solve that. I guess avoiding sizes of (n*4G - PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable buffer should be enough for anyone(TM). > 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. Yeah, I was just going worry about that when we had a second bit to pass. Rob
On 18/07/2019 18:03, Rob Herring wrote: > On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> wrote: >> >> 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@collabora.com> >>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>> Cc: Robin Murphy <robin.murphy@arm.com> >>> Cc: Steven Price <steven.price@arm.com> >>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> --- >>> 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. > > Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm > not sure how we solve that. I guess avoiding sizes of (n*4G - > PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable > buffer should be enough for anyone(TM). I was thinking of something like: if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { if ((*start & PFN_4G_MASK) + (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) *end = (*start & ~PFN_4G_MASK) + (SZ_4GB - SZ_16M) >> PAGE_SHIFT; else *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> PAGE_SHIFT; } So split the region depending on where we can find a free 16MB region which doesn't cross 4GB. But like you say: 4GB should be enough for anyone ;) >> 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. > > Yeah, I was just going worry about that when we had a second bit to pass. One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE is of course the CPU page size - we could have the situation of CPU and GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not sure whether we want to support this or not (kbase doesn't). Also in theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but kbase has never used this so I don't know if it works... ;) Steve
On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> wrote: > > On 18/07/2019 18:03, Rob Herring wrote: > > On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> wrote: > >> > >> 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@collabora.com> > >>> Cc: Boris Brezillon <boris.brezillon@collabora.com> > >>> Cc: Robin Murphy <robin.murphy@arm.com> > >>> Cc: Steven Price <steven.price@arm.com> > >>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> > >>> Signed-off-by: Rob Herring <robh@kernel.org> > >>> --- > >>> 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. > > > > Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm > > not sure how we solve that. I guess avoiding sizes of (n*4G - > > PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable > > buffer should be enough for anyone(TM). > > I was thinking of something like: > > if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { > if ((*start & PFN_4G_MASK) + > (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) > *end = (*start & ~PFN_4G_MASK) + > (SZ_4GB - SZ_16M) >> PAGE_SHIFT; > else > *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> PAGE_SHIFT; > } > > So split the region depending on where we can find a free 16MB region > which doesn't cross 4GB. Here's what I ended up with. It's slightly different in that the start and end don't get 16MB aligned. The code already takes care of the alignment which also is not necessarily 16MB, but 'align = size' as that's sufficient to not cross a 16MB boundary. #define PFN_4G (SZ_4G >> PAGE_SHIFT) #define PFN_4G_MASK (PFN_4G - 1) #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { if ((*start & PFN_4G_MASK) == 0) (*start)++; if ((*end & PFN_4G_MASK) == 0) (*end)--; /* Ensure start and end are in the same 4GB range */ if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { if ((*start & PFN_4G_MASK) + PFN_16M < PFN_4G_MASK) *end = (*start & ~PFN_4G_MASK) + PFN_4G - 1; else *start = (*end & ~PFN_4G_MASK) + 1; } } } > > But like you say: 4GB should be enough for anyone ;) > > >> 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. > > > > Yeah, I was just going worry about that when we had a second bit to pass. > > One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE > is of course the CPU page size - we could have the situation of CPU and > GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not > sure whether we want to support this or not (kbase doesn't). Also in > theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but > kbase has never used this so I don't know if it works... ;) Shhh! I have thought about that, but was ignoring for now. Rob
On 19/07/2019 23:07, Rob Herring wrote: > On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> wrote: >> >> On 18/07/2019 18:03, Rob Herring wrote: >>> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> wrote: >>>> >>>> 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@collabora.com> >>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>> Cc: Steven Price <steven.price@arm.com> >>>>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>> --- >>>>> 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. >>> >>> Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm >>> not sure how we solve that. I guess avoiding sizes of (n*4G - >>> PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable >>> buffer should be enough for anyone(TM). >> >> I was thinking of something like: >> >> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >> if ((*start & PFN_4G_MASK) + >> (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) >> *end = (*start & ~PFN_4G_MASK) + >> (SZ_4GB - SZ_16M) >> PAGE_SHIFT; >> else >> *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> PAGE_SHIFT; >> } >> >> So split the region depending on where we can find a free 16MB region >> which doesn't cross 4GB. > > Here's what I ended up with. It's slightly different in that the start > and end don't get 16MB aligned. The code already takes care of the > alignment which also is not necessarily 16MB, but 'align = size' as > that's sufficient to not cross a 16MB boundary. > > #define PFN_4G (SZ_4G >> PAGE_SHIFT) > #define PFN_4G_MASK (PFN_4G - 1) > #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { > if ((*start & PFN_4G_MASK) == 0) > (*start)++; > > if ((*end & PFN_4G_MASK) == 0) > (*end)--; > > /* Ensure start and end are in the same 4GB range */ > if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { > if ((*start & PFN_4G_MASK) + PFN_16M < PFN_4G_MASK) > *end = (*start & ~PFN_4G_MASK) + PFN_4G - 1; > else > *start = (*end & ~PFN_4G_MASK) + 1; > > } > } > } Yes, that's a much more readable version of what I wrote ;) Steve >> >> But like you say: 4GB should be enough for anyone ;) >> >>>> 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. >>> >>> Yeah, I was just going worry about that when we had a second bit to pass. >> >> One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE >> is of course the CPU page size - we could have the situation of CPU and >> GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not >> sure whether we want to support this or not (kbase doesn't). Also in >> theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but >> kbase has never used this so I don't know if it works... ;) > > Shhh! I have thought about that, but was ignoring for now. > > Rob > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 19/07/2019 23:07, Rob Herring wrote: > On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> wrote: >> >> On 18/07/2019 18:03, Rob Herring wrote: >>> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> wrote: >>>> >>>> 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@collabora.com> >>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>> Cc: Steven Price <steven.price@arm.com> >>>>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>> --- >>>>> 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. >>> >>> Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm >>> not sure how we solve that. I guess avoiding sizes of (n*4G - >>> PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable >>> buffer should be enough for anyone(TM). >> >> I was thinking of something like: >> >> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >> if ((*start & PFN_4G_MASK) + >> (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) >> *end = (*start & ~PFN_4G_MASK) + >> (SZ_4GB - SZ_16M) >> PAGE_SHIFT; >> else >> *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> PAGE_SHIFT; >> } >> >> So split the region depending on where we can find a free 16MB region >> which doesn't cross 4GB. > > Here's what I ended up with. It's slightly different in that the start > and end don't get 16MB aligned. The code already takes care of the > alignment which also is not necessarily 16MB, but 'align = size' as > that's sufficient to not cross a 16MB boundary. > > #define PFN_4G (SZ_4G >> PAGE_SHIFT) > #define PFN_4G_MASK (PFN_4G - 1) > #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { > if ((*start & PFN_4G_MASK) == 0) > (*start)++; > > if ((*end & PFN_4G_MASK) == 0) > (*end)--; > > /* Ensure start and end are in the same 4GB range */ > if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { > if ((*start & PFN_4G_MASK) + PFN_16M < PFN_4G_MASK) > *end = (*start & ~PFN_4G_MASK) + PFN_4G - 1; > else > *start = (*end & ~PFN_4G_MASK) + 1; > > } > } > } If you're happy with the additional restriction that a buffer can't cross a 4GB boundary (which does seem to be required for certain kinds of buffers anyway), then I don't think it needs to be anywhere near that complex: if (!(*start & PFN_4G_MASK)) *start++ *end = min(*end, ALIGN(*start, PFN_4G) - 1); >> >> But like you say: 4GB should be enough for anyone ;) >> >>>> 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. >>> >>> Yeah, I was just going worry about that when we had a second bit to pass. >> >> One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE >> is of course the CPU page size - we could have the situation of CPU and >> GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not >> sure whether we want to support this or not (kbase doesn't). Also in >> theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but >> kbase has never used this so I don't know if it works... ;) > > Shhh! I have thought about that, but was ignoring for now. In general, I don't think that should be too great a concern - we already handle it for IOMMUs, provided the minimum IOMMU page size is no larger than PAGE_SIZE, which should always be the case here too. Looking at how panfrost_mmu_map() is implemented, and given that we're already trying to handle stuff at 2MB granularity where possible, I don't see any obvious reasons for 64K pages not to work already (assuming there aren't any 4K assumptions baked into the userspace side). I might have to give it a try... Robin.
On 22/07/2019 10:50, Robin Murphy wrote: > On 19/07/2019 23:07, Rob Herring wrote: >> On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> >> wrote: >>> >>> On 18/07/2019 18:03, Rob Herring wrote: >>>> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> >>>> wrote: >>>>> >>>>> 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@collabora.com> >>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>>> Cc: Steven Price <steven.price@arm.com> >>>>>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>>> --- >>>>>> 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. >>>> >>>> Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm >>>> not sure how we solve that. I guess avoiding sizes of (n*4G - >>>> PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable >>>> buffer should be enough for anyone(TM). >>> >>> I was thinking of something like: >>> >>> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >>> if ((*start & PFN_4G_MASK) + >>> (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) >>> *end = (*start & ~PFN_4G_MASK) + >>> (SZ_4GB - SZ_16M) >> PAGE_SHIFT; >>> else >>> *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> >>> PAGE_SHIFT; >>> } >>> >>> So split the region depending on where we can find a free 16MB region >>> which doesn't cross 4GB. >> >> Here's what I ended up with. It's slightly different in that the start >> and end don't get 16MB aligned. The code already takes care of the >> alignment which also is not necessarily 16MB, but 'align = size' as >> that's sufficient to not cross a 16MB boundary. >> >> #define PFN_4G (SZ_4G >> PAGE_SHIFT) >> #define PFN_4G_MASK (PFN_4G - 1) >> #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { >> if ((*start & PFN_4G_MASK) == 0) >> (*start)++; >> >> if ((*end & PFN_4G_MASK) == 0) >> (*end)--; >> >> /* Ensure start and end are in the same 4GB range */ >> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >> if ((*start & PFN_4G_MASK) + PFN_16M < >> PFN_4G_MASK) >> *end = (*start & ~PFN_4G_MASK) + >> PFN_4G - 1; >> else >> *start = (*end & ~PFN_4G_MASK) + 1; >> >> } >> } >> } > > If you're happy with the additional restriction that a buffer can't > cross a 4GB boundary (which does seem to be required for certain kinds > of buffers anyway), then I don't think it needs to be anywhere near that > complex: > > if (!(*start & PFN_4G_MASK)) > *start++ > *end = min(*end, ALIGN(*start, PFN_4G) - 1); What happens if *start is very near the end of a 4GB boundary? In that case *end - *start might not be as big as 16MB and we could end up failing the allocation IIUC. >>> >>> But like you say: 4GB should be enough for anyone ;) >>> >>>>> 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. >>>> >>>> Yeah, I was just going worry about that when we had a second bit to >>>> pass. >>> >>> One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE >>> is of course the CPU page size - we could have the situation of CPU and >>> GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not >>> sure whether we want to support this or not (kbase doesn't). Also in >>> theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but >>> kbase has never used this so I don't know if it works... ;) >> >> Shhh! I have thought about that, but was ignoring for now. > > In general, I don't think that should be too great a concern - we > already handle it for IOMMUs, provided the minimum IOMMU page size is no > larger than PAGE_SIZE, which should always be the case here too. Looking > at how panfrost_mmu_map() is implemented, and given that we're already > trying to handle stuff at 2MB granularity where possible, I don't see > any obvious reasons for 64K pages not to work already (assuming there > aren't any 4K assumptions baked into the userspace side). I might have > to give it a try... I'd be interested to know if it works. I know that kbase incorrectly uses PAGE_SIZE in several places (in particular assuming it is the size of the page tables). And I was aware I was in danger of slipping into the same mindset here. User space shouldn't care too much - other than the size of buffers allocated being rounded up to the CPU's page size. At least the Panfrost user/kernel ABI has sizes in bytes not pages (unlike kbase). Steve
On 22/07/2019 11:07, Steven Price wrote: > On 22/07/2019 10:50, Robin Murphy wrote: >> On 19/07/2019 23:07, Rob Herring wrote: >>> On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> >>> wrote: >>>> >>>> On 18/07/2019 18:03, Rob Herring wrote: >>>>> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> >>>>> wrote: >>>>>> >>>>>> 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@collabora.com> >>>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>>>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>>>> Cc: Steven Price <steven.price@arm.com> >>>>>>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>>>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>>>> --- >>>>>>> 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. >>>>> >>>>> Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), I'm >>>>> not sure how we solve that. I guess avoiding sizes of (n*4G - >>>>> PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable >>>>> buffer should be enough for anyone(TM). >>>> >>>> I was thinking of something like: >>>> >>>> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >>>> if ((*start & PFN_4G_MASK) + >>>> (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) >>>> *end = (*start & ~PFN_4G_MASK) + >>>> (SZ_4GB - SZ_16M) >> PAGE_SHIFT; >>>> else >>>> *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> >>>> PAGE_SHIFT; >>>> } >>>> >>>> So split the region depending on where we can find a free 16MB region >>>> which doesn't cross 4GB. >>> >>> Here's what I ended up with. It's slightly different in that the start >>> and end don't get 16MB aligned. The code already takes care of the >>> alignment which also is not necessarily 16MB, but 'align = size' as >>> that's sufficient to not cross a 16MB boundary. >>> >>> #define PFN_4G (SZ_4G >> PAGE_SHIFT) >>> #define PFN_4G_MASK (PFN_4G - 1) >>> #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { >>> if ((*start & PFN_4G_MASK) == 0) >>> (*start)++; >>> >>> if ((*end & PFN_4G_MASK) == 0) >>> (*end)--; >>> >>> /* Ensure start and end are in the same 4GB range */ >>> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >>> if ((*start & PFN_4G_MASK) + PFN_16M < >>> PFN_4G_MASK) >>> *end = (*start & ~PFN_4G_MASK) + >>> PFN_4G - 1; >>> else >>> *start = (*end & ~PFN_4G_MASK) + 1; >>> >>> } >>> } >>> } >> >> If you're happy with the additional restriction that a buffer can't >> cross a 4GB boundary (which does seem to be required for certain kinds >> of buffers anyway), then I don't think it needs to be anywhere near that >> complex: >> >> if (!(*start & PFN_4G_MASK)) >> *start++ >> *end = min(*end, ALIGN(*start, PFN_4G) - 1); > > What happens if *start is very near the end of a 4GB boundary? In that > case *end - *start might not be as big as 16MB and we could end up > failing the allocation IIUC. Ahem... well, the thing about complicated-and-hard-to-read code is that it turns out to be complicated and hard to read correctly :) FWIW, having taken a closer look, my interpretation would be something like (even more untested than before): u64 start_seg = ALIGN(*start, PFN_4G); u64 end_seg = ALIGN_DOWN(*end, PFN_4G); if (start_seg - start < end - end_seg) { *start = end_seg + 1; *end = min(*end, end_seg + PFN_4G - 1); } else { *end = start_seg - 1; *start = max(*start, start_seg - PFN_4G + 1); } but at this point I'm sure we're well into personal preference and "shorter does not necessarily imply clearer" territory. More importantly, though, it now occurs to me that the "pick the biggest end" approach seems inherently suboptimal for cases where the [start,end] interval crosses *more* than one boundary. For, say, start = PFN_4G - 1, end = 2 * PFN_4G + 1, either way we'd get caught up on the single page at one end and ignore the full 4GB in the middle :/ >>>> >>>> But like you say: 4GB should be enough for anyone ;) >>>> >>>>>> 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. >>>>> >>>>> Yeah, I was just going worry about that when we had a second bit to >>>>> pass. >>>> >>>> One other 'minor' issue I noticed as I was writing the above. PAGE_SIZE >>>> is of course the CPU page size - we could have the situation of CPU and >>>> GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not >>>> sure whether we want to support this or not (kbase doesn't). Also in >>>> theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - but >>>> kbase has never used this so I don't know if it works... ;) >>> >>> Shhh! I have thought about that, but was ignoring for now. >> >> In general, I don't think that should be too great a concern - we >> already handle it for IOMMUs, provided the minimum IOMMU page size is no >> larger than PAGE_SIZE, which should always be the case here too. Looking >> at how panfrost_mmu_map() is implemented, and given that we're already >> trying to handle stuff at 2MB granularity where possible, I don't see >> any obvious reasons for 64K pages not to work already (assuming there >> aren't any 4K assumptions baked into the userspace side). I might have >> to give it a try... > > I'd be interested to know if it works. I know that kbase incorrectly > uses PAGE_SIZE in several places (in particular assuming it is the size > of the page tables). And I was aware I was in danger of slipping into > the same mindset here. > > User space shouldn't care too much - other than the size of buffers > allocated being rounded up to the CPU's page size. At least the Panfrost > user/kernel ABI has sizes in bytes not pages (unlike kbase). Sounds promising - my Juno branch is in a bit of a mess at the moment but once I've got that cleaned up I'll have a quick play. Robin.
On 22/07/2019 13:09, Robin Murphy wrote: > On 22/07/2019 11:07, Steven Price wrote: >> On 22/07/2019 10:50, Robin Murphy wrote: >>> On 19/07/2019 23:07, Rob Herring wrote: >>>> On Fri, Jul 19, 2019 at 4:39 AM Steven Price <steven.price@arm.com> >>>> wrote: >>>>> >>>>> On 18/07/2019 18:03, Rob Herring wrote: >>>>>> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@arm.com> >>>>>> wrote: >>>>>>> >>>>>>> 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@collabora.com> >>>>>>>> Cc: Boris Brezillon <boris.brezillon@collabora.com> >>>>>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>>>>> Cc: Steven Price <steven.price@arm.com> >>>>>>>> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> >>>>>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>>>>> --- >>>>>>>> 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. >>>>>> >>>>>> Humm, other than not allowing sizes greater than 4G-(2*PAGE_SIZE), >>>>>> I'm >>>>>> not sure how we solve that. I guess avoiding sizes of (n*4G - >>>>>> PAGE_SIZE) would avoid the problem. Seems like almost 4GB executable >>>>>> buffer should be enough for anyone(TM). >>>>> >>>>> I was thinking of something like: >>>>> >>>>> if ((*end & ~PFN_4G_MASK) != (*start & ~PFN_4G_MASK)) { >>>>> if ((*start & PFN_4G_MASK) + >>>>> (SZ_16M >> PAGE_SHIFT) < PFN_4G_MASK) >>>>> *end = (*start & ~PFN_4G_MASK) + >>>>> (SZ_4GB - SZ_16M) >> PAGE_SHIFT; >>>>> else >>>>> *start = (*end & ~PFN_4G_MASK) + (SZ_16M) >> >>>>> PAGE_SHIFT; >>>>> } >>>>> >>>>> So split the region depending on where we can find a free 16MB region >>>>> which doesn't cross 4GB. >>>> >>>> Here's what I ended up with. It's slightly different in that the start >>>> and end don't get 16MB aligned. The code already takes care of the >>>> alignment which also is not necessarily 16MB, but 'align = size' as >>>> that's sufficient to not cross a 16MB boundary. >>>> >>>> #define PFN_4G (SZ_4G >> PAGE_SHIFT) >>>> #define PFN_4G_MASK (PFN_4G - 1) >>>> #define PFN_16M (SZ_16M >> 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 & PANFROST_BO_NOEXEC)) { >>>> if ((*start & PFN_4G_MASK) == 0) >>>> (*start)++; >>>> >>>> if ((*end & PFN_4G_MASK) == 0) >>>> (*end)--; >>>> >>>> /* Ensure start and end are in the same 4GB range */ >>>> if ((*end & ~PFN_4G_MASK) != (*start & >>>> ~PFN_4G_MASK)) { >>>> if ((*start & PFN_4G_MASK) + PFN_16M < >>>> PFN_4G_MASK) >>>> *end = (*start & ~PFN_4G_MASK) + >>>> PFN_4G - 1; >>>> else >>>> *start = (*end & ~PFN_4G_MASK) + 1; >>>> >>>> } >>>> } >>>> } >>> >>> If you're happy with the additional restriction that a buffer can't >>> cross a 4GB boundary (which does seem to be required for certain kinds >>> of buffers anyway), then I don't think it needs to be anywhere near that >>> complex: >>> >>> if (!(*start & PFN_4G_MASK)) >>> *start++ >>> *end = min(*end, ALIGN(*start, PFN_4G) - 1); >> >> What happens if *start is very near the end of a 4GB boundary? In that >> case *end - *start might not be as big as 16MB and we could end up >> failing the allocation IIUC. > > Ahem... well, the thing about complicated-and-hard-to-read code is that > it turns out to be complicated and hard to read correctly :) > > FWIW, having taken a closer look, my interpretation would be something > like (even more untested than before): > > u64 start_seg = ALIGN(*start, PFN_4G); > u64 end_seg = ALIGN_DOWN(*end, PFN_4G); > > if (start_seg - start < end - end_seg) { > *start = end_seg + 1; > *end = min(*end, end_seg + PFN_4G - 1); > } else { > *end = start_seg - 1; > *start = max(*start, start_seg - PFN_4G + 1); > } > > but at this point I'm sure we're well into personal preference and > "shorter does not necessarily imply clearer" territory. More > importantly, though, it now occurs to me that the "pick the biggest end" > approach seems inherently suboptimal for cases where the [start,end] > interval crosses *more* than one boundary. For, say, start = PFN_4G - 1, > end = 2 * PFN_4G + 1, either way we'd get caught up on the single page > at one end and ignore the full 4GB in the middle :/ Indeed, that case was just occurring to me too! How about: u64 next_seg = ALIGN(*start, PFN_4G); if (next_seg - *start <= PFN_16M) *start = next_seg + 1; *end = min(*end, ALIGN(*start, PFN_4G) - 1); So always allocate at the beginning, but skip past the next 4GB boundary if there's less than 16MB left (or equal to avoid the 4GB boundary). Steve >>>>> >>>>> But like you say: 4GB should be enough for anyone ;) >>>>> >>>>>>> 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. >>>>>> >>>>>> Yeah, I was just going worry about that when we had a second bit to >>>>>> pass. >>>>> >>>>> One other 'minor' issue I noticed as I was writing the above. >>>>> PAGE_SIZE >>>>> is of course the CPU page size - we could have the situation of CPU >>>>> and >>>>> GPU page size being different (e.g. CONFIG_ARM64_64K_PAGES). I'm not >>>>> sure whether we want to support this or not (kbase doesn't). Also in >>>>> theory some GPUs do support 64K (AS_TRANSCFG_ADRMODE_AARCH64_64K) - >>>>> but >>>>> kbase has never used this so I don't know if it works... ;) >>>> >>>> Shhh! I have thought about that, but was ignoring for now. >>> >>> In general, I don't think that should be too great a concern - we >>> already handle it for IOMMUs, provided the minimum IOMMU page size is no >>> larger than PAGE_SIZE, which should always be the case here too. Looking >>> at how panfrost_mmu_map() is implemented, and given that we're already >>> trying to handle stuff at 2MB granularity where possible, I don't see >>> any obvious reasons for 64K pages not to work already (assuming there >>> aren't any 4K assumptions baked into the userspace side). I might have >>> to give it a try... >> >> I'd be interested to know if it works. I know that kbase incorrectly >> uses PAGE_SIZE in several places (in particular assuming it is the size >> of the page tables). And I was aware I was in danger of slipping into >> the same mindset here. >> >> User space shouldn't care too much - other than the size of buffers >> allocated being rounded up to the CPU's page size. At least the Panfrost >> user/kernel ABI has sizes in bytes not pages (unlike kbase). > > Sounds promising - my Juno branch is in a bit of a mess at the moment > but once I've got that cleaned up I'll have a quick play. > > Robin. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 22/07/2019 13:19, Steven Price wrote: [...] > Indeed, that case was just occurring to me too! How about: > > u64 next_seg = ALIGN(*start, PFN_4G); > > if (next_seg - *start <= PFN_16M) > *start = next_seg + 1; > > *end = min(*end, ALIGN(*start, PFN_4G) - 1); > > So always allocate at the beginning, but skip past the next 4GB boundary > if there's less than 16MB left (or equal to avoid the 4GB boundary). Ah, there it is! I think it generalises even further by just changing the condition to "if (next_seg - *start < *end - next_seg)", although we'd need to ensure a signed comparison to cover the case where start and end are already in the same segment. Robin.
> Seems like almost 4GB executable > buffer should be enough for anyone(TM). (TM) indeed. For stats, Panfrost right now uses a single 16MB shader buffer per context and never reallocates. I have never seen it run out of space, not even once on a conformance test. Mali shader binaries are small (a HUGE shader would be several kilobytes!!!) so... honestly if you're doing more than 32MB executable that's super raising eyebrows.
> User space shouldn't care too much - other than the size of buffers > allocated being rounded up to the CPU's page size. At least the Panfrost > user/kernel ABI has sizes in bytes not pages (unlike kbase). We've been rounding everything up to the nearest 4k in mesa, out of old habit from kbase. I don't know if that's strictly necessary / helpful anymore?
This patch is: Acked-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> On Wed, Jul 17, 2019 at 12:33:50PM -0600, 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 > 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@collabora.com> > Cc: Boris Brezillon <boris.brezillon@collabora.com> > Cc: Robin Murphy <robin.murphy@arm.com> > Cc: Steven Price <steven.price@arm.com> > Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > 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)--; > + } > +} > + > 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. > * > -- > 2.20.1
On Mon, Jul 22, 2019 at 7:25 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 22/07/2019 13:19, Steven Price wrote: > [...] > > Indeed, that case was just occurring to me too! How about: > > > > u64 next_seg = ALIGN(*start, PFN_4G); > > > > if (next_seg - *start <= PFN_16M) > > *start = next_seg + 1; This could make start > end... It also doesn't handle not starting on a 4G boundary (or was that condition check supposed to be included still). > > > > *end = min(*end, ALIGN(*start, PFN_4G) - 1); > > > > So always allocate at the beginning, but skip past the next 4GB boundary > > if there's less than 16MB left (or equal to avoid the 4GB boundary). > > Ah, there it is! I think it generalises even further by just changing > the condition to "if (next_seg - *start < *end - next_seg)", although > we'd need to ensure a signed comparison to cover the case where start > and end are already in the same segment. IMO, relying on signed comparsion doesn't really improve the readability... Rob
On 22/07/2019 17:18, Rob Herring wrote: > On Mon, Jul 22, 2019 at 7:25 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 22/07/2019 13:19, Steven Price wrote: >> [...] >>> Indeed, that case was just occurring to me too! How about: >>> >>> u64 next_seg = ALIGN(*start, PFN_4G); >>> >>> if (next_seg - *start <= PFN_16M) >>> *start = next_seg + 1; > > This could make start > end... But only in the case where (*end - *start < 16MB) - i.e. this region wasn't big enough. The generic code will deal with that and reject the hole. > It also doesn't handle not starting on a 4G boundary (or was that > condition check supposed to be included still). I was assuming we still had this before that code: if ((*start & PFN_4G_MASK) == 0) (*start)++; if ((*end & PFN_4G_MASK) == 0) (*end)--; And when bumping *start to next_seg, there's a +1 to deal with the 4GB boundary case. Steve >>> >>> *end = min(*end, ALIGN(*start, PFN_4G) - 1); >>> >>> So always allocate at the beginning, but skip past the next 4GB boundary >>> if there's less than 16MB left (or equal to avoid the 4GB boundary). >> >> Ah, there it is! I think it generalises even further by just changing >> the condition to "if (next_seg - *start < *end - next_seg)", although >> we'd need to ensure a signed comparison to cover the case where start >> and end are already in the same segment. > > IMO, relying on signed comparsion doesn't really improve the readability... > > Rob > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
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)--; + } +} + 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. *
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 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@collabora.com> Cc: Boris Brezillon <boris.brezillon@collabora.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Steven Price <steven.price@arm.com> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io> Signed-off-by: Rob Herring <robh@kernel.org> --- 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(-)