Re: [Qemu-devel] [PATCH v3] RISC-V: Fix riscv_isa_string memory size bug

[Michael Clark]

On Mon, Mar 19, 2018 at 2:39 PM, Michael Clark <address@hidden> wrote:

>
>
> On Mon, Mar 19, 2018 at 12:42 PM, Richard W.M. Jones <address@hidden>
> wrote:
>
>> On Mon, Mar 19, 2018 at 11:35:51AM -0700, Michael Clark wrote:
>> > The RISC-V post-merge spec conformance and cleanup series has had a lot
>> of
>> > testing. I've been using it to compile QEMU inside of QEMU using the
>> RISC-V
>> > Fedora Image and its native RISC-V GCC toolchain running inside SMP
>> Linux
>> > 4.16-rc2. It appears to be pretty rock-solid. The rcu lock fix would
>> likely
>> > only affect users who are ballooning memory while a guest is under load.
>> > The page walker changes have also been tested under load (including
>> > performance tests).
>>
>> Did you see the problem with restoring floating point registers on
>> context switch?  The test case is quite simple:
>>
>>   http://oirase.annexia.org/tmp/sched.c
>
>
> No I didn't. Thanks.
>
>
>> Note you must compile it with gcc -O2 to manifest the bug.  (We
>> originally thought it was a problem with gcc's optimization, but it
>> isn't.)
>>
>
> Okay. I must admit most of my testing has been integer heavy workloads.
>
>
>> In Fedora's qemu tree are carrying the following patch which is just a
>> workaround:
>>
>>   https://github.com/rwmjones/fedora-riscv-bootstrap/blob/mast
>> er/stage1-riscv-qemu/force-float-save.patch
>
>
> I will take a look...
>

>From my local testing, it appears to only show up with SMP enabled. With 1
CPU enabled the sched.c test passes.


> There were some changes around v4 of our port upstream patch series to add
> MSTATUS.FS (TB_FLAGS_FP_ENABLE) to to tb->flags to avoid checking the flags
> on all FP ops.
>

I'm curious about the change in v4 of our upstream patch series "Enforce
MSTATUS_FS via TB flags"

-
https://github.com/riscv/riscv-qemu/commit/1b0631573008d19b2cb4bb1ef388bd51d6a1d722

I'll could try and revert this and see if it makes any difference...
however I think the float code has changed substantially with respect to
handling of flags. We could backport the more conservative approach of
handling MSTATUS_FS in the float routines. We should also run the test case
on hardware to rule out linux-kernel MSTATIS_FS bugs...

BTW We didn't have MTTCG enabled until v6 of our patch series so given it
only shows up on SMP, it wouldn't have been visible until v6 or after.

static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong
> *pc,
>                                         target_ulong *cs_base, uint32_t
> *flags)
> {
>     *pc = env->pc;
>     *cs_base = 0;
> #ifdef CONFIG_USER_ONLY
>     *flags = TB_FLAGS_FP_ENABLE;
> #else
>     *flags = cpu_mmu_index(env, 0) | (env->mstatus & MSTATUS_FS);
> #endif
> }
>
> I'll have to get my head around the implication of this or with the
> privilege level and how MSTATUS.FS is currently set...
>