Preventing zero GPU virtual address allocation

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

Quoted 7 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Quoted 3 lines

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

I'll mollify that: outside of full-ppgtt where you need to share the VM.

Quoted 12 lines · Chris Wilson

Quoted 10 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

Quoted 1 line

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Quoted 8 lines · Chris Wilson

Quoted 7 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Quoted 5 lines

Quoted 3 lines

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Essentially it applies the PIN_BIAS for all execbuf objects, which works even on ggtt execbufs.

Patches themselves look good, but we miss the igt to update the invalid ctx flags testcase. And a bare minimal function testcase (which checks the reloc offset with and without a ctx with this flag set) would be nice too. With that and an r-b from the beignet developers I'll pull this in. -Daniel

Quoted 13 lines · Daniel Vetter

Quoted 12 lines · Chris Wilson

Quoted 10 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

Quoted 6 lines

Quoted 1 line

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

That does not address the issue that *0 is an untrapped runtime bug that allows writing to other objects or reading from them.

Quoted 14 lines · Chris Wilson

Quoted 13 lines · Daniel Vetter

Quoted 12 lines · Chris Wilson

Quoted 10 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Any suggestion for a better description?

Quoted 2 lines

That does not address the issue that *0 is an untrapped runtime bug that allows writing to other objects or reading from them.

Not exactly sure what you suggest here?

On Thu, May 21, 2015 at 11:08:42AM +0300, David Weinehall wrote:

Quoted 1 line

Not exactly sure what you suggest here?

That you have an unmitigated security hole in your design.

Quoted 22 lines · David Weinehall

Quoted 20 lines · Chris Wilson

Quoted 19 lines · Daniel Vetter

Quoted 8 lines · Chris Wilson

Quoted 7 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Quoted 5 lines

Quoted 3 lines

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Quoted 6 lines

Any suggestion for a better description?

Quoted 2 lines

That does not address the issue that *0 is an untrapped runtime bug that allows writing to other objects or reading from them.

Not exactly sure what you suggest here?

That you have an unmitigated security hole in your design.

Quoted 30 lines · Chris Wilson

Quoted 28 lines · David Weinehall

Quoted 14 lines · Chris Wilson

Quoted 13 lines · Daniel Vetter

Quoted 12 lines · Chris Wilson

Quoted 10 lines · Chris Wilson

Quoted 5 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any other) to allow use of address zero to represent NULL.

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Any suggestion for a better description?

Quoted 2 lines

That does not address the issue that *0 is an untrapped runtime bug that allows writing to other objects or reading from them.

Not exactly sure what you suggest here?

Quoted 1 line

That you have an unmitigated security hole in your design.

There's no security issue afaics, there's only a correctness issue. opencl kernels must be able to assume that NULL never points to a valid buffer address of their own. It might point at other intersting stuff like batch buffers or what else, but never about anything created by the ocl client itself. This patch achieves that.

Ofc there's still the issue that anything else mapped can be stomped upon, but that's just undefined behaviour in C-spec speak. And there's still the isolation issues, but ocl doesn't make any new guarantees there afaik.

Quoted 1 line · Chris Wilson

On Thu, May 21, 2015 at 11:08:42AM +0300, David Weinehall wrote:

[snip]

Quoted 3 lines

Quoted 1 line

Not exactly sure what you suggest here?

That you have an unmitigated security hole in your design.

No, I meant what you suggest as a remedy.

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, May 21, 2015 12:01 AM
To: Chris Wilson; intel-gfx
Subject: Re: [Intel-gfx] [PATCH 00/03] Preventing zero GPU virtual address allocation

Quoted 9 lines · Chris Wilson

Quoted 8 lines · Chris Wilson

Quoted 6 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any
other) to allow use of address zero to represent NULL.

Quoted 5 lines

Quoted 3 lines

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Essentially it applies the PIN_BIAS for all execbuf objects, which works even on ggtt execbufs.

Patches themselves look good, but we miss the igt to update the invalid ctx flags testcase. And a bare minimal function testcase (which checks the reloc offset with and without a ctx with this flag set) would be nice too. With that and an r-b from the beignet developers I'll pull this in. -Daniel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

It also have just as much risk as reporting EBUSY due to the CL client trying to use a pinned buffer.

However, it is a security hole because the same process can arrange to have whatever buffer it likes at 0 then access it through the CL kernel.

Quoted 29 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Daniel Vetter
Sent: Thursday, May 21, 2015 12:01 AM
To: Chris Wilson; intel-gfx
Subject: Re: [Intel-gfx] [PATCH 00/03] Preventing zero GPU virtual address allocation

Quoted 9 lines · Chris Wilson

Quoted 8 lines · Chris Wilson

Quoted 6 lines · David Weinehall

This patch series (one patch each for libdrm, the kernel, and beignet) aims to provide a means to add a context-specific means to prevent a mapping to GPU virtual address zero. This is needed at least by Beignet (possibly in other use-cases too, though I don't know of any
other) to allow use of address zero to represent NULL.

Quoted 5 lines

Quoted 3 lines

Urm, you cannot allow absolute addressing period. What happens to the object at 0 when the user reads from it or writes to it? You have to have an object at 0 for the user's NULL pointer access.

I'll mollify that: outside of full-ppgtt where you need to share the VM.

The description is misleading, the new flag doesn't prevent anything from getting mapped at 0 but only prevents any bo submitted through execbuf on the given context from being bound at address 0. If that would happen compute kernels using NULL checks for some things would fall over.

Quoted 8 lines

Essentially it applies the PIN_BIAS for all execbuf objects, which works even on ggtt execbufs.

Patches themselves look good, but we miss the igt to update the invalid ctx flags testcase. And a bare minimal function testcase (which checks the reloc offset with and without a ctx with this flag set) would be nice too. With that and an r-b from the beignet developers I'll pull this in. -Daniel

Hi,
We can verify the patches work for Beignet.

Thanks
Zou Nanhai

Quoted 8 lines

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

tests/gem_ctx_param_basic: Expand ctx_param tests

Expand the context parameter tests to cover the
no-zeromap parameter.

Signed-off-by David Weinehall <[email protected]>

---
 gem_ctx_param_basic.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

tests/gem_ctx_param_basic.c +0 -0

index b44b37cf0538..ba9366d1a679 100644

tests/gem_ctx_param_basic.c +11 -0

 	igt_subtest("invalid-param-get") {
 		ctx_param.context = ctx;

@@ -132,6 +132,28 @@ igt_main
 		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
 	}
 
+	ctx_param.param  = LOCAL_CONTEXT_PARAM_NO_ZEROMAP;
+
+	igt_subtest("non-root-set-no-zeromap") {
+		igt_fork(child, 1) {
+			igt_drop_root();
+
+			ctx_param.context = ctx;
+			TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM);
+			ctx_param.value--;
+			TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM);
+		}

tests/gem_ctx_param_basic.c +1 -0

+
 	igt_fixture
 		close(fd);
 }

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 18 lines · David Weinehall

tests/gem_ctx_param_basic: Expand ctx_param tests

Expand the context parameter tests to cover the
no-zeromap parameter.

Signed-off-by David Weinehall <[email protected]>

---
 gem_ctx_param_basic.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

tests/gem_ctx_param_basic.c +0 -0

index b44b37cf0538..ba9366d1a679 100644
@@ -98,7 +98,7 @@ igt_main
 		ctx_param.size = 0;
 	}
 

tests/gem_ctx_param_basic.c igt_main · line 101

Quoted 2 lines

tests/gem_ctx_param_basic.c +1 -1

-	ctx_param.param  = LOCAL_CONTEXT_PARAM_BAN_PERIOD + 1;
+	ctx_param.param  = I915_CONTEXT_PARAM_NO_ZEROMAP + 1;

Please respin this one with a LOCAL_ define for NO_ZEROMAP. We generally don't want to have a hard coupling between the headers in libdrm and igt, would mean a libdrm release roughly every week ;-)

Quoted 18 lines

tests/gem_ctx_param_basic.c +11 -0

 	igt_subtest("invalid-param-get") {
 		ctx_param.context = ctx;

@@ -132,6 +132,28 @@ igt_main
 		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
 	}
 
+	ctx_param.param  = LOCAL_CONTEXT_PARAM_NO_ZEROMAP;
+
+	igt_subtest("non-root-set-no-zeromap") {
+		igt_fork(child, 1) {
+			igt_drop_root();
+
+			ctx_param.context = ctx;
+			TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM);
+			ctx_param.value--;
+			TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM);
+		}

tests/gem_ctx_param_basic.c igt_main · line 155

Quoted 10 lines

tests/gem_ctx_param_basic.c +10 -0

+
+		igt_waitchildren();
+	}
+
+	igt_subtest("root-set-no-zeromap") {
+		ctx_param.context = ctx;
+		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM);
+		ctx_param.value--;
+		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+	}

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Export a new context parameter that can be set/queried through the context_{get,set}param ioctls. This parameter is passed as a context flag and decides whether or not a GPU address mapping is allowed to be made at address zero. The default is to allow such mappings.

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Quoted 4 lines · David Weinehall

Export a new context parameter that can be set/queried through the context_{get,set}param ioctls. This parameter is passed as a context flag and decides whether or not a GPU address mapping is allowed to be made at address zero. The default is to allow such mappings.

Please revert this piece of unreviewed API.

The most obvious objection is what value of bias the kernel should be using.

Then given that what value does this hold over and above userspace specifying the layout they want?

Quoted 11 lines · Chris Wilson

Quoted 4 lines · David Weinehall

Export a new context parameter that can be set/queried through the context_{get,set}param ioctls. This parameter is passed as a context flag and decides whether or not a GPU address mapping is allowed to be made at address zero. The default is to allow such mappings.

Please revert this piece of unreviewed API.

The most obvious objection is what value of bias the kernel should be using.

Quoted 2 lines

Then given that what value does this hold over and above userspace specifying the layout they want?

Yeah it would be redundant with softpinning. But that's only for ocl2, and this is apparently needed for for ocl1.x already. Hence imo it's ok to pull this ahead a bit, even if redundant.

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

"Basic test for context set/get param input validation."

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Quoted 12 lines · Daniel Vetter

Quoted 11 lines · Chris Wilson

Quoted 4 lines · David Weinehall

Export a new context parameter that can be set/queried through the context_{get,set}param ioctls. This parameter is passed as a context flag and decides whether or not a GPU address mapping is allowed to be made at address zero. The default is to allow such mappings.

Please revert this piece of unreviewed API.

The most obvious objection is what value of bias the kernel should be using.

Quoted 6 lines

Quoted 2 lines

Then given that what value does this hold over and above userspace specifying the layout they want?

Yeah it would be redundant with softpinning. But that's only for ocl2, and this is apparently needed for for ocl1.x already. Hence imo it's ok to pull this ahead a bit, even if redundant.

Look at the interface:

CONTEXT_NO_ZEROMAP:
- it has nothing to do with mapping, it only concerns execbuf object location

- what is a sensible bias? I certainly do not wish for the hack we have right now to become permanent (as this patch makes it).
- it can still easily fail given a kernel that operates on pinned objects when not in full-ppgtt

For a context parameter, something like
CONTEXT_MM_START, CONTEXT_MM_END
that limited the drm_mm range manager for the context (downside is that such would only be available for full-ppgtt, but really full-ppgtt is mandatory for this in the first place otherwise it *is* exploitable - for it not requires a compiler that wouldn't need the NULL pointer to be all 0). The advantage of that parameter is that it is useful in igt testing, and even allows for limited rollout of 48bit full-ppgtt, vital given the workarounds that userspace must buy into before enabling.

And then there is "int flags;". Are we really expecting to use the sign bit on this bitmask?

Quoted 5 lines · Chris Wilson

It also have just as much risk as reporting EBUSY due to the CL client trying to use a pinned buffer.

However, it is a security hole because the same process can arrange to have whatever buffer it likes at 0 then access it through the CL kernel.

I don't really understand what you're getting at here. While it's true that userland can have whatever buffer it likes at 0, there's nothing in the current code that prevents this in the first place, so I cannot see how this could be a regression. This feature isn't intended as a security measure; its sole purpose is to help implementations that assume 0 = failure to avoid weird bugs.

Regards: David
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 16 lines · David Weinehall

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

"Basic test for context set/get param input validation."

Quoted 5 lines

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Yeah separate testcase would fit better, agreed.

Quoted 23 lines · Daniel Vetter

Quoted 21 lines · David Weinehall

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

"Basic test for context set/get param input validation."

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Quoted 1 line

Yeah separate testcase would fit better, agreed.

Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish.

Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug.

Quoted 23 lines · Daniel Vetter

Quoted 21 lines · David Weinehall

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

"Basic test for context set/get param input validation."

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Quoted 1 line

Yeah separate testcase would fit better, agreed.

Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish.

Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug.

Quoted 26 lines · Daniel Vetter

Quoted 24 lines · Daniel Vetter

Quoted 14 lines · David Weinehall

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

Quoted 7 lines

"Basic test for context set/get param input validation."

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Yeah separate testcase would fit better, agreed.

Quoted 6 lines

Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish.

Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug.

We really need man pages for new ioctls as well in libdrm.

Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish.

Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug.

Quoted 34 lines · Jesse Barnes

Quoted 32 lines · Daniel Vetter

Quoted 17 lines · Daniel Vetter

Quoted 16 lines · David Weinehall

Quoted 9 lines · Daniel Vetter

A simple functional test here which does:
a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase.
b) set the NO_ZEROMAP property.
c) re-run the same batch, assert that now the buffer is relocated to something non-0.

Just to make sure we have a bare minimal testcase to make sure we don't break this.

Maybe this should be added to another test rather than here? This test is described as a:

"Basic test for context set/get param input validation."

Quoted 7 lines

Quoted 5 lines

Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case.

But that might be over-engineering. Opinions?

Yeah separate testcase would fit better, agreed.

Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish.

Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug.

Quoted 1 line

We really need man pages for new ioctls as well in libdrm.

Hmmm, this isn't a new ioctl, just a context parameter that can be set/queried using a pre-existing ioctl, but I can modify the existing manual page (if there is one?) to include information about the new parameter.