<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 15/08/18 10:45, andrey simiklit
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAHfu=g+JqGRVJ89MM91p2Mbpx0MK-2q5w5VUB5a+zhu0emA2TA@mail.gmail.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div dir="ltr">
<div>Hi all,</div>
<div><br>
</div>
<div>Thanks for your reply.<br>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>We shouldn't even get to use the iterator if it's an
unknown instruction.</div>
The decoder should just advance dword by dword until it finds
something that<br>
makes sense again.<br>
</blockquote>
<div><br>
</div>
<div>Got it) <br>
</div>
<div>So this is an expected behavior there:</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>return iter_group_offset_bits(iter, iter->group_iter +
1) <<br>
(gen_group_get_length(iter->group,
iter->p) * 32);<br>
</div>
</blockquote>
<div><br>
</div>
<div>when we convert a negative <b>int</b> to <b>uint</b> to
return true to continue our loop.<br>
</div>
<div><br>
</div>
<div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>return iter_group_offset_bits(iter, iter->group_iter
+ 1) <<br>
(<b>0xFFFFFFE0U</b>);</div>
</blockquote>
<div><br>
</div>
<div>Do you think it is good idea to add comment or something
like this into the "iter_more_groups" function:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>int <b>length</b> =
gen_group_get_length(iter->gro<wbr>up, iter->p); </div>
</blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">return <b>length <
0 ||</b><br>
iter_group_offset_bits(iter,
iter->group_iter + 1) < <br>
(<b>length</b> * 32);<br>
</blockquote>
<div> </div>
to show more explicitly here that we want to return true to
continue our loop<br>
when the -1 is returned from the "gen_group_get_length"
function <br>
</div>
<div>because at the moment it is a bit implicit)</div>
</div>
<div>Please let me know if I am incorrect.</div>
</div>
</blockquote>
<br>
Sorry for the late answer :(<br>
<br>
This implies an unknown size for the inspected instruction/struct.<br>
I think this shouldn't happen because the caller even try to
initialize the iterator to decode it.<br>
<br>
I would add an assert, because the iterator doesn't really deal with
that case.<br>
I'm not sure whether there is such case in the genxml files, but if
it happens we should probably look into it.<br>
<br>
Cheers,<br>
<br>
-<br>
Lionel<br>
<br>
<blockquote type="cite"
cite="mid:CAHfu=g+JqGRVJ89MM91p2Mbpx0MK-2q5w5VUB5a+zhu0emA2TA@mail.gmail.com">
<div dir="ltr">
<div><br>
</div>
<div>Regards,</div>
<div>Andrii.<br>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Aug 14, 2018 at 7:08 PM,
Lionel Landwerlin <span dir="ltr"><<a
href="mailto:lionel.g.landwerlin@intel.com"
target="_blank" moz-do-not-send="true">lionel.g.landwerlin@intel.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">On 14/08/18 16:16,
Rafael Antognolli wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
On Tue, Aug 14, 2018 at 03:36:18PM +0100, Lionel
Landwerlin wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
On 14/08/18 12:55, <a href="http://asimiklit.work"
rel="noreferrer" target="_blank"
moz-do-not-send="true">asimiklit.work</a> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
Hi Lionel,<br>
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
Hi Andrii,<br>
<br>
Again sorry, I don't think this is the right fix.<br>
I'm sending another patch to fix the parsing of<br>
MI_BATCH_BUFFER_START which seems to be the actual
issue.<br>
<br>
Thanks for working on this,<br>
</blockquote>
Thanks for your fast reply.<br>
I agree that it is not correct patch for this issue
but anyway<br>
"iter_more_groups" function will still work
incorrectly<br>
for unknown instructions when the
"iter->group->variable" field is true.<br>
I guess that this case should be fixed.<br>
Please let me know if I am incorrect.<br>
</blockquote>
Hey Andrii,<br>
<br>
We shouldn't even get to use the iterator if it's an
unknown instruction.<br>
The decoder should just advance dword by dword until
it finds something that<br>
makes sense again.<br>
<br>
If we run into that problem, I think we should fix the
caller.<br>
</blockquote>
In that case, would an unreachable() or assert be a good
thing to do?<br>
</blockquote>
<br>
Yep, I guess assert in gen_field_iterator_init() should be
a good thing.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0px 0px
0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
Regards,<br>
Andrii.<br>
<br>
On 2018-08-14 1:26 PM, Lionel Landwerlin wrote:<br>
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
Hi Andrii,<br>
<br>
Again sorry, I don't think this is the right fix.<br>
I'm sending another patch to fix the parsing of<br>
MI_BATCH_BUFFER_START which seems to be the actual
issue.<br>
<br>
Thanks for working on this,<br>
<br>
-<br>
Lionel<br>
<br>
On 14/08/18 10:04, <a
href="mailto:asimiklit.work@gmail.com"
target="_blank" moz-do-not-send="true">asimiklit.work@gmail.com</a>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
From: Andrii Simiklit <<a
href="mailto:asimiklit.work@gmail.com"
target="_blank" moz-do-not-send="true">asimiklit.work@gmail.com</a>><br>
<br>
The "gen_group_get_length" function can return a
negative value<br>
and it can lead to the out of bounds group_iter.<br>
<br>
v2: printing of "unknown command type" was added<br>
Bugzilla: <a
href="https://bugs.freedesktop.org/show_bug.cgi?id=107544"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://bugs.freedesktop.org/s<wbr>how_bug.cgi?id=107544</a><br>
Signed-off-by: Andrii Simiklit <<a
href="mailto:andrii.simiklit@globallogic.com"
target="_blank" moz-do-not-send="true">andrii.simiklit@globallogic.c<wbr>om</a>><br>
---<br>
src/intel/common/gen_decoder.c | 13
+++++++++++--<br>
1 file changed, 11 insertions(+), 2
deletions(-)<br>
<br>
diff --git a/src/intel/common/gen_decoder<wbr>.c<br>
b/src/intel/common/gen_decoder<wbr>.c<br>
index ec0a486..b36facf 100644<br>
--- a/src/intel/common/gen_decoder<wbr>.c<br>
+++ b/src/intel/common/gen_decoder<wbr>.c<br>
@@ -770,6 +770,13 @@ gen_group_get_length(struct
gen_group<br>
*group, const uint32_t *p)<br>
return -1;<br>
}<br>
}<br>
+ default: {<br>
+ fprintf(stderr, "Unknown command type %u
in '%s::%s'\n",<br>
+ type,<br>
+ (group->parent &&
group->parent->name) ?<br>
group->parent->name : "UNKNOWN",<br>
+ group->name ? group->name :
"UNKNOWN");<br>
+ break;<br>
+ }<br>
}<br>
return -1;<br>
@@ -803,8 +810,10 @@ static bool<br>
iter_more_groups(const struct
gen_field_iterator *iter)<br>
{<br>
if (iter->group->variable) {<br>
- return iter_group_offset_bits(iter,
iter->group_iter + 1) <<br>
- (gen_group_get_length(iter->gr<wbr>oup,
iter->p) * 32);<br>
+ const int length =
gen_group_get_length(iter->gro<wbr>up,
iter->p);<br>
+ return length > 0 &&<br>
+ iter_group_offset_bits(iter,
iter->group_iter + 1) <<br>
+ (length * 32);<br>
} else {<br>
return (iter->group_iter + 1) <
iter->group->group_count ||<br>
iter->group->next != NULL;<br>
</blockquote>
<br>
</blockquote>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org"
target="_blank" moz-do-not-send="true">mesa-dev@lists.freedesktop.org</a><br>
<a
href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>