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

[Alex Bennée]

Amir CHARIF <address@hidden> 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 <address@hidden>
> Envoyé : mercredi 13 mars 2019 17:35
> À : Amir CHARIF <address@hidden>; address@hidden
> Cc : address@hidden
> 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