Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
On Mon, Aug 31, 2015 at 01:19:47AM +0300, Francisco Jerez wrote: > Francisco Jerez writes: > > > Daniel Vetter writes: > > > >> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote: > >>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: > >>> > Daniel Vetter writes: > >>> > > >>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: > >>> > >> The patchset LGTM and works well with beignet. The 80%+ performance > >>> > >> regression issue in darktable also has been fixed > >>> > >> after this patchset applied and enable the atomic in L3 at beignet > >>> > >> side. So, > >>> > >> > >>> > >> Reviewed-by: Zhigang Gong > >>> > > > >>> > > All three merged. > >>> > > >>> > Thanks Daniel. > >>> > > >>> > > Aside: Dont we need an increment for the cmd parser version for > >>> > > userspace to be able to detect this? > >>> > > > >>> > Yeah, that would be a good idea, patch attached. > >>> > >>> The old version alloweed userspace to write basically any register, the > >>> new version allows only the whitelisted registers. I don't see how a > >>> version number bump would help anyone. > >> > >> Oops, totally missed the context of patch 1. Jani I think that one's for > >> you too ... > >> > > IMHO the version bump is still useful for userspace to find out whether > > it can use plain LRIs to write the L3 atomic chicken bits. It's true > > that as Ville said it would have been possible for userspace to write > > the same bits before this series by building a batch specifically > > crafted to cheat the command parser, but I don't think we want userspace > > to rely on a command parser bug (e.g. because we may want to back-port > > the fix to earlier kernel versions). > > > Ping. I cannot use these registers from userspace until the command > parser version number is bumped. Queued for -next, thanks for the patch. -Daniel > > >> Thanks for pointing this out. > >> -Daniel > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> http://blog.ffwll.ch > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Francisco Jerez writes: > Daniel Vetter writes: > >> On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote: >>> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: >>> > Daniel Vetter writes: >>> > >>> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: >>> > >> The patchset LGTM and works well with beignet. The 80%+ performance >>> > >> regression issue in darktable also has been fixed >>> > >> after this patchset applied and enable the atomic in L3 at beignet >>> > >> side. So, >>> > >> >>> > >> Reviewed-by: Zhigang Gong >>> > > >>> > > All three merged. >>> > >>> > Thanks Daniel. >>> > >>> > > Aside: Dont we need an increment for the cmd parser version for >>> > > userspace to be able to detect this? >>> > > >>> > Yeah, that would be a good idea, patch attached. >>> >>> The old version alloweed userspace to write basically any register, the >>> new version allows only the whitelisted registers. I don't see how a >>> version number bump would help anyone. >> >> Oops, totally missed the context of patch 1. Jani I think that one's for >> you too ... >> > IMHO the version bump is still useful for userspace to find out whether > it can use plain LRIs to write the L3 atomic chicken bits. It's true > that as Ville said it would have been possible for userspace to write > the same bits before this series by building a batch specifically > crafted to cheat the command parser, but I don't think we want userspace > to rely on a command parser bug (e.g. because we may want to back-port > the fix to earlier kernel versions). > Ping. I cannot use these registers from userspace until the command parser version number is bumped. >> Thanks for pointing this out. >> -Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
On Mon, 15 Jun 2015, Daniel Vetter wrote: > On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote: >> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: >> > Daniel Vetter writes: >> > >> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: >> > >> The patchset LGTM and works well with beignet. The 80%+ performance >> > >> regression issue in darktable also has been fixed >> > >> after this patchset applied and enable the atomic in L3 at beignet >> > >> side. So, >> > >> >> > >> Reviewed-by: Zhigang Gong >> > > >> > > All three merged. >> > >> > Thanks Daniel. >> > >> > > Aside: Dont we need an increment for the cmd parser version for >> > > userspace to be able to detect this? >> > > >> > Yeah, that would be a good idea, patch attached. >> >> The old version alloweed userspace to write basically any register, the >> new version allows only the whitelisted registers. I don't see how a >> version number bump would help anyone. > > Oops, totally missed the context of patch 1. Jani I think that one's for > you too ... Cherry-picked to drm-intel-next-fixes. BR, Jani. > > Thanks for pointing this out. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Daniel Vetter writes: > On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote: >> On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: >> > Daniel Vetter writes: >> > >> > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: >> > >> The patchset LGTM and works well with beignet. The 80%+ performance >> > >> regression issue in darktable also has been fixed >> > >> after this patchset applied and enable the atomic in L3 at beignet >> > >> side. So, >> > >> >> > >> Reviewed-by: Zhigang Gong >> > > >> > > All three merged. >> > >> > Thanks Daniel. >> > >> > > Aside: Dont we need an increment for the cmd parser version for >> > > userspace to be able to detect this? >> > > >> > Yeah, that would be a good idea, patch attached. >> >> The old version alloweed userspace to write basically any register, the >> new version allows only the whitelisted registers. I don't see how a >> version number bump would help anyone. > > Oops, totally missed the context of patch 1. Jani I think that one's for > you too ... > IMHO the version bump is still useful for userspace to find out whether it can use plain LRIs to write the L3 atomic chicken bits. It's true that as Ville said it would have been possible for userspace to write the same bits before this series by building a batch specifically crafted to cheat the command parser, but I don't think we want userspace to rely on a command parser bug (e.g. because we may want to back-port the fix to earlier kernel versions). > Thanks for pointing this out. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
On Mon, Jun 15, 2015 at 02:26:22PM +0300, Ville Syrjälä wrote: > On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: > > Daniel Vetter writes: > > > > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: > > >> The patchset LGTM and works well with beignet. The 80%+ performance > > >> regression issue in darktable also has been fixed > > >> after this patchset applied and enable the atomic in L3 at beignet side. > > >> So, > > >> > > >> Reviewed-by: Zhigang Gong > > > > > > All three merged. > > > > Thanks Daniel. > > > > > Aside: Dont we need an increment for the cmd parser version for > > > userspace to be able to detect this? > > > > > Yeah, that would be a good idea, patch attached. > > The old version alloweed userspace to write basically any register, the > new version allows only the whitelisted registers. I don't see how a > version number bump would help anyone. Oops, totally missed the context of patch 1. Jani I think that one's for you too ... Thanks for pointing this out. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
On Mon, Jun 15, 2015 at 02:18:01PM +0300, Francisco Jerez wrote: > Daniel Vetter writes: > > > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: > >> The patchset LGTM and works well with beignet. The 80%+ performance > >> regression issue in darktable also has been fixed > >> after this patchset applied and enable the atomic in L3 at beignet side. > >> So, > >> > >> Reviewed-by: Zhigang Gong > > > > All three merged. > > Thanks Daniel. > > > Aside: Dont we need an increment for the cmd parser version for > > userspace to be able to detect this? > > > Yeah, that would be a good idea, patch attached. The old version alloweed userspace to write basically any register, the new version allows only the whitelisted registers. I don't see how a version number bump would help anyone. > > > And please follow up with a link to the beignet patches used to validate > > these kernel patches for references. > > > Zhigang, do you have a link to your Beignet patch? > > > Thanks, Daniel > > > >> > >> Thanks, > >> Zhigang Gong. > >> > From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001 > From: Francisco Jerez > Date: Mon, 15 Jun 2015 14:03:29 +0300 > Subject: [PATCH] drm/i915: Bump command parser version number. > > Signed-off-by: Francisco Jerez > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c > b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 0146fe6..6722098 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -1219,6 +1219,7 @@ int i915_cmd_parser_get_version(void) >* 2. Allow access to the MI_PREDICATE_SRC0 and >*MI_PREDICATE_SRC1 registers. >* 3. Allow access to the GPGPU_THREADS_DISPATCHED register. > + * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3. >*/ > - return 3; > + return 4; > } > -- > 2.4.3 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Daniel Vetter writes: > On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote: >> The patchset LGTM and works well with beignet. The 80%+ performance >> regression issue in darktable also has been fixed >> after this patchset applied and enable the atomic in L3 at beignet side. So, >> >> Reviewed-by: Zhigang Gong > > All three merged. Thanks Daniel. > Aside: Dont we need an increment for the cmd parser version for > userspace to be able to detect this? > Yeah, that would be a good idea, patch attached. > And please follow up with a link to the beignet patches used to validate > these kernel patches for references. > Zhigang, do you have a link to your Beignet patch? > Thanks, Daniel > >> >> Thanks, >> Zhigang Gong. >> From 9f26beaf96473800252db35c4513933ae43e3c84 Mon Sep 17 00:00:00 2001 From: Francisco Jerez Date: Mon, 15 Jun 2015 14:03:29 +0300 Subject: [PATCH] drm/i915: Bump command parser version number. Signed-off-by: Francisco Jerez --- drivers/gpu/drm/i915/i915_cmd_parser.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 0146fe6..6722098 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -1219,6 +1219,7 @@ int i915_cmd_parser_get_version(void) * 2. Allow access to the MI_PREDICATE_SRC0 and *MI_PREDICATE_SRC1 registers. * 3. Allow access to the GPGPU_THREADS_DISPATCHED register. + * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3. */ - return 3; + return 4; } -- 2.4.3 signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> The patchset LGTM and works well with beignet. The 80%+ performance
> regression issue in darktable also has been fixed
> after this patchset applied and enable the atomic in L3 at beignet side. So,
>
> Reviewed-by: Zhigang Gong
All three merged. Aside: Dont we need an increment for the cmd parser
version for userspace to be able to detect this?
And please follow up with a link to the beignet patches used to validate
these kernel patches for references.
Thanks, Daniel
>
> Thanks,
> Zhigang Gong.
>
> > -Original Message-
> > From: Francisco Jerez [mailto:curroje...@riseup.net]
> > Sent: Friday, May 29, 2015 9:44 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Ville Syrjälä; Zhigang Gong; Brad Volkin
> > Subject: [PATCH 1/3] drm/i915: Fix command parser to validate multiple
> > register access with the same command.
> >
> > Until now the software command checker assumed that commands could read
> > or write at most a single register per packet. This is not necessarily the
> > case,
> > MI_LOAD_REGISTER_IMM expects a variable-length list of offset/value pairs
> > and writes them in sequence. The previous code would only check whether
> > the first entry was valid, effectively allowing userspace to write
> > unrestricted
> > registers of the MMIO space by sending a multi-register write with a legal
> > first
> > register, with potential security implications on Gen6 and 7 hardware.
> >
> > Fix it by extending the drm_i915_cmd_descriptor table to represent
> > multi-register access and making validate_cmd() iterate for all register
> > offsets
> > present in the command packet.
> >
> > Signed-off-by: Francisco Jerez
> > ---
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 74
> > --
> > drivers/gpu/drm/i915/i915_drv.h| 5 +++
> > 2 files changed, 48 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 61ae8ff..c4a5f73 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor
> > common_cmds[] = {
> > CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF,
> > R ),
> > CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF,
> > R ),
> > CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF,
> > W,
> > - .reg = { .offset = 1, .mask = 0x007C } ),
> > + .reg = { .offset = 1, .mask = 0x007C, .step = 2 }),
> > CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF,
> > W | B,
> > .reg = { .offset = 1, .mask = 0x007C },
> > .bits = {{
> > @@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs
> > *ring)
> >
> > static bool check_cmd(const struct intel_engine_cs *ring,
> > const struct drm_i915_cmd_descriptor *desc,
> > - const u32 *cmd,
> > + const u32 *cmd, u32 length,
> > const bool is_master,
> > bool *oacontrol_set)
> > {
> > @@ -955,38 +955,49 @@ static bool check_cmd(const struct intel_engine_cs
> > *ring,
> > }
> >
> > if (desc->flags & CMD_DESC_REGISTER) {
> > - u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
> > -
> > /*
> > -* OACONTROL requires some special handling for writes. We
> > -* want to make sure that any batch which enables OA also
> > -* disables it before the end of the batch. The goal is to
> > -* prevent one process from snooping on the perf data from
> > -* another process. To do that, we need to check the value
> > -* that will be written to the register. Hence, limit
> > -* OACONTROL writes to only MI_LOAD_REGISTER_IMM
> > commands.
> > +* Get the distance between individual register offset
> > +* fields if the command can perform more than one
> > +* access at a time.
> > */
> > - if (reg_addr == OACONTROL) {
> > - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
> > - DRM_DEBUG_DRIVER("CMD: Rejected LRM to
> > OACONTROL\n");
> > - return false;
> > + const u32 step = desc->reg.step ? desc->reg.step : length;
> > + u32 offset;
> > +
> > + for (offset = desc->reg.offset; offset < length;
> > +offset += step) {
> > + const u32 reg_addr = cmd[offset] & desc->reg.mask;
> > +
> > + /*
> > +* OACONTROL requires some special handling for
> > +* writes. We want to make sure that any batch which
> > +* enables OA also disables it before the end of the
> > +* batch. Th
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Zhigang Gong writes:
> The patchset LGTM and works well with beignet. The 80%+ performance
> regression issue in darktable also has been fixed
> after this patchset applied and enable the atomic in L3 at beignet side. So,
>
> Reviewed-by: Zhigang Gong
>
> Thanks,
> Zhigang Gong.
>
Thanks!
>> -Original Message-
>> From: Francisco Jerez [mailto:curroje...@riseup.net]
>> Sent: Friday, May 29, 2015 9:44 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Ville Syrjälä; Zhigang Gong; Brad Volkin
>> Subject: [PATCH 1/3] drm/i915: Fix command parser to validate multiple
>> register access with the same command.
>>
>> Until now the software command checker assumed that commands could read
>> or write at most a single register per packet. This is not necessarily the
>> case,
>> MI_LOAD_REGISTER_IMM expects a variable-length list of offset/value pairs
>> and writes them in sequence. The previous code would only check whether
>> the first entry was valid, effectively allowing userspace to write
>> unrestricted
>> registers of the MMIO space by sending a multi-register write with a legal
>> first
>> register, with potential security implications on Gen6 and 7 hardware.
>>
>> Fix it by extending the drm_i915_cmd_descriptor table to represent
>> multi-register access and making validate_cmd() iterate for all register
>> offsets
>> present in the command packet.
>>
>> Signed-off-by: Francisco Jerez
>> ---
>> drivers/gpu/drm/i915/i915_cmd_parser.c | 74
>> --
>> drivers/gpu/drm/i915/i915_drv.h| 5 +++
>> 2 files changed, 48 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> index 61ae8ff..c4a5f73 100644
>> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
>> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
>> @@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor
>> common_cmds[] = {
>> CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF,
>> R ),
>> CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF,
>> R ),
>> CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF,
>> W,
>> - .reg = { .offset = 1, .mask = 0x007C } ),
>> + .reg = { .offset = 1, .mask = 0x007C, .step = 2 }),
>> CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF,
>> W | B,
>>.reg = { .offset = 1, .mask = 0x007C },
>>.bits = {{
>> @@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs
>> *ring)
>>
>> static bool check_cmd(const struct intel_engine_cs *ring,
>>const struct drm_i915_cmd_descriptor *desc,
>> - const u32 *cmd,
>> + const u32 *cmd, u32 length,
>>const bool is_master,
>>bool *oacontrol_set)
>> {
>> @@ -955,38 +955,49 @@ static bool check_cmd(const struct intel_engine_cs
>> *ring,
>> }
>>
>> if (desc->flags & CMD_DESC_REGISTER) {
>> -u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
>> -
>> /*
>> - * OACONTROL requires some special handling for writes. We
>> - * want to make sure that any batch which enables OA also
>> - * disables it before the end of the batch. The goal is to
>> - * prevent one process from snooping on the perf data from
>> - * another process. To do that, we need to check the value
>> - * that will be written to the register. Hence, limit
>> - * OACONTROL writes to only MI_LOAD_REGISTER_IMM
>> commands.
>> + * Get the distance between individual register offset
>> + * fields if the command can perform more than one
>> + * access at a time.
>> */
>> -if (reg_addr == OACONTROL) {
>> -if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
>> -DRM_DEBUG_DRIVER("CMD: Rejected LRM to
>> OACONTROL\n");
>> -return false;
>> +const u32 step = desc->reg.step ? desc->reg.step : length;
>> +u32 offset;
>> +
>> +for (offset = desc->reg.offset; offset < length;
>> + offset += step) {
>> +const u32 reg_addr = cmd[offset] & desc->reg.mask;
>> +
>> +/*
>> + * OACONTROL requires some special handling for
>> + * writes. We want to make sure that any batch which
>> + * enables OA also disables it before the end of the
>> + * batch. The goal is to prevent one process from
>> + * snooping on the perf data from another process. To do
>> + * that, we need to check the value that will be written
>> + * to the register. Hence, limit OACONTROL writes to
>> + * only MI_LOAD_REGISTER_IMM comman
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
The patchset LGTM and works well with beignet. The 80%+ performance regression
issue in darktable also has been fixed
after this patchset applied and enable the atomic in L3 at beignet side. So,
Reviewed-by: Zhigang Gong
Thanks,
Zhigang Gong.
> -Original Message-
> From: Francisco Jerez [mailto:curroje...@riseup.net]
> Sent: Friday, May 29, 2015 9:44 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä; Zhigang Gong; Brad Volkin
> Subject: [PATCH 1/3] drm/i915: Fix command parser to validate multiple
> register access with the same command.
>
> Until now the software command checker assumed that commands could read
> or write at most a single register per packet. This is not necessarily the
> case,
> MI_LOAD_REGISTER_IMM expects a variable-length list of offset/value pairs
> and writes them in sequence. The previous code would only check whether
> the first entry was valid, effectively allowing userspace to write
> unrestricted
> registers of the MMIO space by sending a multi-register write with a legal
> first
> register, with potential security implications on Gen6 and 7 hardware.
>
> Fix it by extending the drm_i915_cmd_descriptor table to represent
> multi-register access and making validate_cmd() iterate for all register
> offsets
> present in the command packet.
>
> Signed-off-by: Francisco Jerez
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 74
> --
> drivers/gpu/drm/i915/i915_drv.h| 5 +++
> 2 files changed, 48 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 61ae8ff..c4a5f73 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor
> common_cmds[] = {
> CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF,
> R ),
> CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF,
> R ),
> CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF,
> W,
> - .reg = { .offset = 1, .mask = 0x007C } ),
> + .reg = { .offset = 1, .mask = 0x007C, .step = 2 }),
> CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF,
> W | B,
> .reg = { .offset = 1, .mask = 0x007C },
> .bits = {{
> @@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs
> *ring)
>
> static bool check_cmd(const struct intel_engine_cs *ring,
> const struct drm_i915_cmd_descriptor *desc,
> - const u32 *cmd,
> + const u32 *cmd, u32 length,
> const bool is_master,
> bool *oacontrol_set)
> {
> @@ -955,38 +955,49 @@ static bool check_cmd(const struct intel_engine_cs
> *ring,
> }
>
> if (desc->flags & CMD_DESC_REGISTER) {
> - u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
> -
> /*
> - * OACONTROL requires some special handling for writes. We
> - * want to make sure that any batch which enables OA also
> - * disables it before the end of the batch. The goal is to
> - * prevent one process from snooping on the perf data from
> - * another process. To do that, we need to check the value
> - * that will be written to the register. Hence, limit
> - * OACONTROL writes to only MI_LOAD_REGISTER_IMM
> commands.
> + * Get the distance between individual register offset
> + * fields if the command can perform more than one
> + * access at a time.
>*/
> - if (reg_addr == OACONTROL) {
> - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
> - DRM_DEBUG_DRIVER("CMD: Rejected LRM to
> OACONTROL\n");
> - return false;
> + const u32 step = desc->reg.step ? desc->reg.step : length;
> + u32 offset;
> +
> + for (offset = desc->reg.offset; offset < length;
> + offset += step) {
> + const u32 reg_addr = cmd[offset] & desc->reg.mask;
> +
> + /*
> + * OACONTROL requires some special handling for
> + * writes. We want to make sure that any batch which
> + * enables OA also disables it before the end of the
> + * batch. The goal is to prevent one process from
> + * snooping on the perf data from another process. To do
> + * that, we need to check the value that will be written
> + * to the register. Hence, limit OACONTROL writes to
> + * only MI_LOAD_REGISTER_IMM commands.
> + */
> + if (reg_addr == OACONTROL) {
> +
[Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Until now the software command checker assumed that commands could
read or write at most a single register per packet. This is not
necessarily the case, MI_LOAD_REGISTER_IMM expects a variable-length
list of offset/value pairs and writes them in sequence. The previous
code would only check whether the first entry was valid, effectively
allowing userspace to write unrestricted registers of the MMIO space
by sending a multi-register write with a legal first register, with
potential security implications on Gen6 and 7 hardware.
Fix it by extending the drm_i915_cmd_descriptor table to represent
multi-register access and making validate_cmd() iterate for all
register offsets present in the command packet.
Signed-off-by: Francisco Jerez
---
drivers/gpu/drm/i915/i915_cmd_parser.c | 74 --
drivers/gpu/drm/i915/i915_drv.h| 5 +++
2 files changed, 48 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 61ae8ff..c4a5f73 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] =
{
CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, R ),
CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
- .reg = { .offset = 1, .mask = 0x007C } ),
+ .reg = { .offset = 1, .mask = 0x007C, .step = 2 }),
CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W | B,
.reg = { .offset = 1, .mask = 0x007C },
.bits = {{
@@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs *ring)
static bool check_cmd(const struct intel_engine_cs *ring,
const struct drm_i915_cmd_descriptor *desc,
- const u32 *cmd,
+ const u32 *cmd, u32 length,
const bool is_master,
bool *oacontrol_set)
{
@@ -955,38 +955,49 @@ static bool check_cmd(const struct intel_engine_cs *ring,
}
if (desc->flags & CMD_DESC_REGISTER) {
- u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
-
/*
-* OACONTROL requires some special handling for writes. We
-* want to make sure that any batch which enables OA also
-* disables it before the end of the batch. The goal is to
-* prevent one process from snooping on the perf data from
-* another process. To do that, we need to check the value
-* that will be written to the register. Hence, limit
-* OACONTROL writes to only MI_LOAD_REGISTER_IMM commands.
+* Get the distance between individual register offset
+* fields if the command can perform more than one
+* access at a time.
*/
- if (reg_addr == OACONTROL) {
- if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
- DRM_DEBUG_DRIVER("CMD: Rejected LRM to
OACONTROL\n");
- return false;
+ const u32 step = desc->reg.step ? desc->reg.step : length;
+ u32 offset;
+
+ for (offset = desc->reg.offset; offset < length;
+offset += step) {
+ const u32 reg_addr = cmd[offset] & desc->reg.mask;
+
+ /*
+* OACONTROL requires some special handling for
+* writes. We want to make sure that any batch which
+* enables OA also disables it before the end of the
+* batch. The goal is to prevent one process from
+* snooping on the perf data from another process. To do
+* that, we need to check the value that will be written
+* to the register. Hence, limit OACONTROL writes to
+* only MI_LOAD_REGISTER_IMM commands.
+*/
+ if (reg_addr == OACONTROL) {
+ if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
+ DRM_DEBUG_DRIVER("CMD: Rejected LRM to
OACONTROL\n");
+ return false;
+ }
+
+ if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
+ *oacontrol_set = (cmd[offset + 1] != 0);
}
- if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
- *oacontrol_set = (cmd[2] != 0);
- }
-
- if (!valid_reg(ring->reg_table,
-
