[Qemu-devel] [PATCH risu 0/2] risu: Fix handling of ARM sigframe

[Qemu-devel] [PATCH risu 0/2] risu: Fix handling of ARM sigframe

[Peter Maydell]

This patchset has two patches: the major one fixes bugs in
our code for parsing the VFP parts of the ARM signal frame
structures. These weren't causing a practical problem because
of the way the kernel happens to lay out the sigframe,
but I noticed them while looking at the code for another
reason.

The second is just moving an orphan comment to somewhere
more appropriate.

thanks
-- PMM

Peter Maydell (2):
  risu_reginfo_arm.c: Fix handling of size values in sigframe
  risu_reginfo_arm.c: Move orphan comment to risu.h.

 risu.h             |  5 +++++
 risu_reginfo_arm.c | 20 ++++++++++----------
 2 files changed, 15 insertions(+), 10 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] risu_reginfo_arm.c: Fix handling of size values in sigframe

[Peter Maydell]

The code in reginfo_init_vfp() to parse the signal frame
was mishandling the size counts:
 * the size includes the bytes for the magic and size fields,
   so the code to skip forward over unknown or undersize blocks
   was adding 4 more than it should
 * the size is in bytes but the "is this block too small"
   test was checking against an expected size in words

This didn't cause any problems because the kernel happens
to generate signal frames with the VFP section first.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 risu_reginfo_arm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
index 0cb9087..b0d5da7 100644
--- a/risu_reginfo_arm.c
+++ b/risu_reginfo_arm.c
@@ -36,7 +36,12 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
     unsigned long *rs = uc->uc_regspace;
 
     for (;;) {
-        switch (*rs++) {
+        unsigned long magic = *rs++;
+        unsigned long size = *rs++;
+
+        size -= 8; /* Account for the magic/size fields */
+
+        switch (magic) {
         case 0:
         {
             /* We didn't find any VFP at all (probably a no-VFP
@@ -57,11 +62,11 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
              */
             int i;
             /* Skip if it's smaller than we expected (should never happen!) */
-            if (*rs < ((32 * 2) + 1)) {
-                rs += (*rs / 4);
+            if (size < ((32 * 2) + 1) * 4) {
+                rs += size / 4;
                 break;
             }
-            rs++;
+
             for (i = 0; i < 32; i++) {
                 ri->fpregs[i] = *rs++;
                 ri->fpregs[i] |= (uint64_t) (*rs++) << 32;
@@ -86,7 +91,7 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
         }
         default:
             /* Some other kind of block, ignore it */
-            rs += (*rs / 4);
+            rs += size / 4;
             break;
         }
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] risu_reginfo_arm.c: Fix handling of size values in sigframe

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> The code in reginfo_init_vfp() to parse the signal frame
> was mishandling the size counts:
>  * the size includes the bytes for the magic and size fields,
>    so the code to skip forward over unknown or undersize blocks
>    was adding 4 more than it should
>  * the size is in bytes but the "is this block too small"
>    test was checking against an expected size in words
>
> This didn't cause any problems because the kernel happens
> to generate signal frames with the VFP section first.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I guess this would have tripped up once the kernel started dumping SVE
registers in the context?

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


> ---
>  risu_reginfo_arm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
> index 0cb9087..b0d5da7 100644
> --- a/risu_reginfo_arm.c
> +++ b/risu_reginfo_arm.c
> @@ -36,7 +36,12 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
>      unsigned long *rs = uc->uc_regspace;
>
>      for (;;) {
> -        switch (*rs++) {
> +        unsigned long magic = *rs++;
> +        unsigned long size = *rs++;
> +
> +        size -= 8; /* Account for the magic/size fields */
> +
> +        switch (magic) {
>          case 0:
>          {
>              /* We didn't find any VFP at all (probably a no-VFP
> @@ -57,11 +62,11 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
>               */
>              int i;
>              /* Skip if it's smaller than we expected (should never happen!) */
> -            if (*rs < ((32 * 2) + 1)) {
> -                rs += (*rs / 4);
> +            if (size < ((32 * 2) + 1) * 4) {
> +                rs += size / 4;
>                  break;
>              }
> -            rs++;
> +
>              for (i = 0; i < 32; i++) {
>                  ri->fpregs[i] = *rs++;
>                  ri->fpregs[i] |= (uint64_t) (*rs++) << 32;
> @@ -86,7 +91,7 @@ static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
>          }
>          default:
>              /* Some other kind of block, ignore it */
> -            rs += (*rs / 4);
> +            rs += size / 4;
>              break;
>          }
>      }


--
Alex Bennée

Re: [Qemu-devel] [PATCH 1/2] risu_reginfo_arm.c: Fix handling of size values in sigframe

[Peter Maydell]

On 20 June 2017 at 16:03, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> The code in reginfo_init_vfp() to parse the signal frame
>> was mishandling the size counts:
>>  * the size includes the bytes for the magic and size fields,
>>    so the code to skip forward over unknown or undersize blocks
>>    was adding 4 more than it should
>>  * the size is in bytes but the "is this block too small"
>>    test was checking against an expected size in words
>>
>> This didn't cause any problems because the kernel happens
>> to generate signal frames with the VFP section first.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> I guess this would have tripped up once the kernel started dumping SVE
> registers in the context?

Probably not if it put them after the VFP registers (where
you'd expect them to be), though if we supported SVE regs in
risu we'd probably have found this bug in the process of
getting that working ;-)

thanks
-- PMM

[Qemu-devel] [PATCH 2/2] risu_reginfo_arm.c: Move orphan comment to risu.h.

[Peter Maydell]

Move an orphan comment that describes the reginfo structure
into risu.h, and expand it a little.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 risu.h             | 5 +++++
 risu_reginfo_arm.c | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/risu.h b/risu.h
index 3fbeda8..70c2184 100644
--- a/risu.h
+++ b/risu.h
@@ -48,6 +48,11 @@ extern int test_fp_exc;
 /* The memory block should be this long */
 #define MEMBLOCKLEN 8192
 
+/* This is the data structure we pass over the socket for OP_COMPARE
+ * and OP_TESTEND. It is a simplified and reduced subset of what can
+ * be obtained with a ucontext_t*, and is architecture specific
+ * (defined in risu_reginfo_*.h).
+ */
 struct reginfo;
 
 /* Functions operating on reginfo */
diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
index b0d5da7..6b9ee7b 100644
--- a/risu_reginfo_arm.c
+++ b/risu_reginfo_arm.c
@@ -19,11 +19,6 @@
 
 extern int insnsize(ucontext_t *uc);
 
-/* This is the data structure we pass over the socket.
- * It is a simplified and reduced subset of what can
- * be obtained with a ucontext_t*
- */
-
 static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
 {
     /* Read VFP registers. These live in uc->uc_regspace, which is
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] risu_reginfo_arm.c: Move orphan comment to risu.h.

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> Move an orphan comment that describes the reginfo structure
> into risu.h, and expand it a little.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  risu.h             | 5 +++++
>  risu_reginfo_arm.c | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/risu.h b/risu.h
> index 3fbeda8..70c2184 100644
> --- a/risu.h
> +++ b/risu.h
> @@ -48,6 +48,11 @@ extern int test_fp_exc;
>  /* The memory block should be this long */
>  #define MEMBLOCKLEN 8192
>
> +/* This is the data structure we pass over the socket for OP_COMPARE
> + * and OP_TESTEND. It is a simplified and reduced subset of what can
> + * be obtained with a ucontext_t*, and is architecture specific
> + * (defined in risu_reginfo_*.h).
> + */
>  struct reginfo;
>
>  /* Functions operating on reginfo */
> diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
> index b0d5da7..6b9ee7b 100644
> --- a/risu_reginfo_arm.c
> +++ b/risu_reginfo_arm.c
> @@ -19,11 +19,6 @@
>
>  extern int insnsize(ucontext_t *uc);
>
> -/* This is the data structure we pass over the socket.
> - * It is a simplified and reduced subset of what can
> - * be obtained with a ucontext_t*
> - */
> -
>  static void reginfo_init_vfp(struct reginfo *ri, ucontext_t *uc)
>  {
>      /* Read VFP registers. These live in uc->uc_regspace, which is

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée