Re: [Qemu-devel] [PATCH] Re-evaluate SVE vector length everytime ADDVL is executed

["Alex Bennée" <alex.bennee@linaro.org>]

Amir CHARIF <amir.charif@cea.fr> writes:

> Hello,
> Thanks for your answer.
>
> The wrong size was definitely being stored in the TB, and, it only affected ADDVL/RDVL/ADDPL (i.e. not all instructions are wrong). Here is what I think was happening:
>
> - The kernel disables SVE in EL0 (ZEN= 01).
> - When the user space application is entered, the TB containing ADDVL has its length set to 0 (16 bytes), as we are in EL0 (so sve_exception_el!=0), and FP is enabled.
> - ADDVL is executed (without trapping) on the basis of the current
> length (16). (Nested function calls in the same context will cause a
> ton of ADDVL instructions to be executed with a vecsize of 16.)

So this looks like the error. Certainly the pseudo code says:

  CheckSVEEnabled();
  bits(64) operand1 = if n == 31 then SP[] else X[n];
  bits(64) result = operand1 + (imm * (VL DIV 8));

  if d == 31 then
      SP[] = result;
  else
      X[d] = result;

so we should trap to the kernel and we won't without sve_access_check()

> - The next different SVE instruction traps as it should and the size is adjusted for the rest of the execution (but the stack space that was initially allocated is wrong so execution fails).
>
> So another fix would be to allow these instructions to trap by calling sve_access_check(), as is done with the rest of SVE instructions.
>
> I'm not that familiar with the instruction set, so could you please let me know if it is correct to call sve_access_check() in these instructions ?
>
> Thanks in advance,
>
> Best regards,
> Amir
>
> -----Message d'origine-----
> De : Richard Henderson <richard.henderson@linaro.org>
> Envoyé : mercredi 13 mars 2019 17:35
> À : Amir CHARIF <amir.charif@cea.fr>; qemu-devel@nongnu.org
> Cc : qemu-arm@nongnu.org
> Objet : Re: [Qemu-devel] [PATCH] Re-evaluate SVE vector length everytime ADDVL is executed
>
> On 3/13/19 7:41 AM, Amir Charif wrote:
>> In system emulation mode, the kernel may internally use 16-byte vectors.
>> If this size is saved in the DisasContext before entering a userspace
>> app that uses higher SVE sizes, the wrong size may be allocated on the
>> stack resulting in corruption (segfaults in user space).
>> This fix evaluates the vector size at runtime (as opposed to
>> translation time) to always allocate the correct size on the stack (when ADDVL is used).
>
> This is wrong.
>
> In particular, if the computation of VL is wrong for ADDVL, it is wrong for every other SVE instruction as well.  Most of which cannot have VL computed at runtime like this.
>
> That is why we break the TB at every change to VL.
>
> Where do we "enter a userspace app" without breaking the TB and recomputing?
> As far as I know this must have executed an ERET to return from EL1 to EL0, which most definitely happens between TBs, or else no system calls would work at all.
>
> Do you have an example that provokes this failure?
>
>
> r~


--
Alex Bennée