| Message ID | 1427988653-754-88-git-send-email-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 02, 2015 at 05:30:29PM +0200, Lucas Stach wrote: > It is legal for the userspace to pass in a command stream of a size > aligned to 32 bit, if that is where the last user command ends. The > kernel then needs to insert a LINK command at the end of the stream, > which needs to be aligned to 64 bit, so the kernel may insert an > additional 32bits of padding in the stream. Align the stream size > to account for that in the size and command stream validator checks. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/staging/etnaviv/etnaviv_gem_submit.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/etnaviv/etnaviv_gem_submit.c b/drivers/staging/etnaviv/etnaviv_gem_submit.c > index 7bd4912ab8ad..965096be5219 100644 > --- a/drivers/staging/etnaviv/etnaviv_gem_submit.c > +++ b/drivers/staging/etnaviv/etnaviv_gem_submit.c > @@ -377,8 +377,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, > > /* > * We must have space to add a LINK command at the end of > - * the command buffer. > + * the command buffer. Align buffer size to the next 64bit > + * quantity, as that's the point where we need to insert the > + * next command. > */ > + submit_cmd.size = ALIGN(submit_cmd.size, 8); > max_size = etnaviv_obj->base.size - 8; I wonder if it's an error if the command size is not a multiple of 8? I know that the command stream is always aligned to 8 bytes for the 2D cores, but I don't know about the 3D or VG cores.
Am Donnerstag, den 02.04.2015, 17:20 +0100 schrieb Russell King - ARM Linux: > On Thu, Apr 02, 2015 at 05:30:29PM +0200, Lucas Stach wrote: > > It is legal for the userspace to pass in a command stream of a size > > aligned to 32 bit, if that is where the last user command ends. The > > kernel then needs to insert a LINK command at the end of the stream, > > which needs to be aligned to 64 bit, so the kernel may insert an > > additional 32bits of padding in the stream. Align the stream size > > to account for that in the size and command stream validator checks. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > drivers/staging/etnaviv/etnaviv_gem_submit.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/etnaviv/etnaviv_gem_submit.c b/drivers/staging/etnaviv/etnaviv_gem_submit.c > > index 7bd4912ab8ad..965096be5219 100644 > > --- a/drivers/staging/etnaviv/etnaviv_gem_submit.c > > +++ b/drivers/staging/etnaviv/etnaviv_gem_submit.c > > @@ -377,8 +377,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, > > > > /* > > * We must have space to add a LINK command at the end of > > - * the command buffer. > > + * the command buffer. Align buffer size to the next 64bit > > + * quantity, as that's the point where we need to insert the > > + * next command. > > */ > > + submit_cmd.size = ALIGN(submit_cmd.size, 8); > > max_size = etnaviv_obj->base.size - 8; > > I wonder if it's an error if the command size is not a multiple of 8? > I know that the command stream is always aligned to 8 bytes for the 2D > cores, but I don't know about the 3D or VG cores. > The start of the commands must always be 64bit aligned, it's the same for all pipes. The size can be dword aligned if the last command in the stream is something like SET_STATE with a length of 2. In that case one needs to insert another padding dword, but as it's the end of the stream userspace may omit that. So that more a question of policy: do we want userspace to always specify the size including the padding and reject streams with a size not double-dword aligned, or do we just fix it up in the kernel, as it's equally easily done there. I was a bit on the fence here and decided to go the "let the kernel fix things" route. Regards, Lucas
On Thu, Apr 02, 2015 at 06:29:24PM +0200, Lucas Stach wrote: > The start of the commands must always be 64bit aligned, it's the same > for all pipes. The size can be dword aligned if the last command in the > stream is something like SET_STATE with a length of 2. In that case one > needs to insert another padding dword, but as it's the end of the stream > userspace may omit that. > > So that more a question of policy: do we want userspace to always > specify the size including the padding and reject streams with a size > not double-dword aligned, or do we just fix it up in the kernel, as it's > equally easily done there. I was a bit on the fence here and decided to > go the "let the kernel fix things" route. Without really knowing the hardware, it's hard to say. However, they seem to be designed around a 64-bit architecture, and I would not be surprised if the front end DMA always fetches from the command buffer in 64-bit quantities. You mention SET_STATE, but the same applies to NOP, WAIT and all of the other commands for the 2D cores - the command word must always be in the first 32-bits of each 64-bit naturally aligned word in memory. Given that, my feeling (and it's only a feeling) is that it would be potentially dangerous to allow userspace to pass a buffer which isn't aligned. As you've found, the kernel would have to align the buffer size up to a 64-bit quantity to add the LINK command at the end anyway. So, I would err on the side of having userspace always do that, and we initially enforce that on the kernel side. If we find that's too strict, we can always relax the kernel side - and we still remain compatible with userspace. If we do the reverse, it's harder for the kernel to become more strict without breaking userspace. Given the "no regressions" rule, that's something we all want to avoid. :)
Am Donnerstag, den 02.04.2015, 17:45 +0100 schrieb Russell King - ARM Linux: > On Thu, Apr 02, 2015 at 06:29:24PM +0200, Lucas Stach wrote: > > The start of the commands must always be 64bit aligned, it's the same > > for all pipes. The size can be dword aligned if the last command in the > > stream is something like SET_STATE with a length of 2. In that case one > > needs to insert another padding dword, but as it's the end of the stream > > userspace may omit that. > > > > So that more a question of policy: do we want userspace to always > > specify the size including the padding and reject streams with a size > > not double-dword aligned, or do we just fix it up in the kernel, as it's > > equally easily done there. I was a bit on the fence here and decided to > > go the "let the kernel fix things" route. > > Without really knowing the hardware, it's hard to say. However, they > seem to be designed around a 64-bit architecture, and I would not be > surprised if the front end DMA always fetches from the command buffer > in 64-bit quantities. > > You mention SET_STATE, but the same applies to NOP, WAIT and all of the > other commands for the 2D cores - the command word must always be in > the first 32-bits of each 64-bit naturally aligned word in memory. > > Given that, my feeling (and it's only a feeling) is that it would be > potentially dangerous to allow userspace to pass a buffer which isn't > aligned. As you've found, the kernel would have to align the buffer > size up to a 64-bit quantity to add the LINK command at the end anyway. > > So, I would err on the side of having userspace always do that, and > we initially enforce that on the kernel side. If we find that's too > strict, we can always relax the kernel side - and we still remain > compatible with userspace. If we do the reverse, it's harder for the > kernel to become more strict without breaking userspace. Given the > "no regressions" rule, that's something we all want to avoid. :) > Yes, seems reasonable. I'll change this to just reject non 64bit aligned sizes and change my userspace accordingly. Thanks, Lucas
2015-04-02 18:45 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Thu, Apr 02, 2015 at 06:29:24PM +0200, Lucas Stach wrote: >> The start of the commands must always be 64bit aligned, it's the same >> for all pipes. The size can be dword aligned if the last command in the >> stream is something like SET_STATE with a length of 2. In that case one >> needs to insert another padding dword, but as it's the end of the stream >> userspace may omit that. >> >> So that more a question of policy: do we want userspace to always >> specify the size including the padding and reject streams with a size >> not double-dword aligned, or do we just fix it up in the kernel, as it's >> equally easily done there. I was a bit on the fence here and decided to >> go the "let the kernel fix things" route. > > Without really knowing the hardware, it's hard to say. However, they > seem to be designed around a 64-bit architecture, and I would not be > surprised if the front end DMA always fetches from the command buffer > in 64-bit quantities. Thats what the hardware does. > > You mention SET_STATE, but the same applies to NOP, WAIT and all of the > other commands for the 2D cores - the command word must always be in > the first 32-bits of each 64-bit naturally aligned word in memory. > Yup - see https://github.com/laanwj/etna_viv/blob/master/doc/hardware.md#command-stream > Given that, my feeling (and it's only a feeling) is that it would be > potentially dangerous to allow userspace to pass a buffer which isn't > aligned. As you've found, the kernel would have to align the buffer > size up to a 64-bit quantity to add the LINK command at the end anyway. > > So, I would err on the side of having userspace always do that, and > we initially enforce that on the kernel side. If we find that's too > strict, we can always relax the kernel side - and we still remain > compatible with userspace. If we do the reverse, it's harder for the > kernel to become more strict without breaking userspace. Given the > "no regressions" rule, that's something we all want to avoid. :) > -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner
diff --git a/drivers/staging/etnaviv/etnaviv_gem_submit.c b/drivers/staging/etnaviv/etnaviv_gem_submit.c index 7bd4912ab8ad..965096be5219 100644 --- a/drivers/staging/etnaviv/etnaviv_gem_submit.c +++ b/drivers/staging/etnaviv/etnaviv_gem_submit.c @@ -377,8 +377,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, /* * We must have space to add a LINK command at the end of - * the command buffer. + * the command buffer. Align buffer size to the next 64bit + * quantity, as that's the point where we need to insert the + * next command. */ + submit_cmd.size = ALIGN(submit_cmd.size, 8); max_size = etnaviv_obj->base.size - 8; if (submit_cmd.size > max_size ||
It is legal for the userspace to pass in a command stream of a size aligned to 32 bit, if that is where the last user command ends. The kernel then needs to insert a LINK command at the end of the stream, which needs to be aligned to 64 bit, so the kernel may insert an additional 32bits of padding in the stream. Align the stream size to account for that in the size and command stream validator checks. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/staging/etnaviv/etnaviv_gem_submit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)