While discussing a problem with how we're (not) setting vstart_eq_zero
Richard had the following to say w.r.t the conditional mark_vs_dirty()
calls on load/store functions [1]:

"I think it's required to have stores set dirty unconditionally, before
the operation.

Consider a store that traps on the 2nd element, leaving vstart = 2, and
exiting to the main loop via exception. The exception enters the kernel
page fault handler. The kernel may need to fault in the page for the
process, and in the meantime task switch.

If vs dirty is not already set, the kernel won't know to save vector
state on task switch."

Do a mark_vs_dirty() before store operations. Keep the mark_vs_dirty()
call at the end for loads - the function is a no-op if mstatus_vs is
already set to EXT_STATUS_DIRTY so there's no hurt in store functions
calling it twice.

[1] https://lore.kernel.org/qemu-riscv/72c7503b-0f43-44b8-aa82-fbafed2aac0c@linaro.org/

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/insn_trans/trans_rvv.c.inc | 29 +++++++++++++++----------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
index 9e101ab434..2065e9064e 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -636,12 +636,13 @@ static bool ldst_us_trans(uint32_t vd, uint32_t rs1, uint32_t data,
     tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
     tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
 
-    fn(dest, mask, base, tcg_env, desc);
-
-    if (!is_store) {
+    if (is_store) {
         mark_vs_dirty(s);
     }
 
+    fn(dest, mask, base, tcg_env, desc);
+
+    mark_vs_dirty(s);
     gen_set_label(over);
     return true;
 }
@@ -797,12 +798,13 @@ static bool ldst_stride_trans(uint32_t vd, uint32_t rs1, uint32_t rs2,
     tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
     tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
 
-    fn(dest, mask, base, stride, tcg_env, desc);
-
-    if (!is_store) {
+    if (is_store) {
         mark_vs_dirty(s);
     }
 
+    fn(dest, mask, base, stride, tcg_env, desc);
+
+    mark_vs_dirty(s);
     gen_set_label(over);
     return true;
 }
@@ -904,12 +906,13 @@ static bool ldst_index_trans(uint32_t vd, uint32_t rs1, uint32_t vs2,
     tcg_gen_addi_ptr(index, tcg_env, vreg_ofs(s, vs2));
     tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
 
-    fn(dest, mask, base, index, tcg_env, desc);
-
-    if (!is_store) {
+    if (is_store) {
         mark_vs_dirty(s);
     }
 
+    fn(dest, mask, base, index, tcg_env, desc);
+
+    mark_vs_dirty(s);
     gen_set_label(over);
     return true;
 }
@@ -1102,11 +1105,13 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1, uint32_t nf,
     base = get_gpr(s, rs1, EXT_NONE);
     tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
 
-    fn(dest, base, tcg_env, desc);
-
-    if (!is_store) {
+    if (is_store) {
         mark_vs_dirty(s);
     }
+
+    fn(dest, base, tcg_env, desc);
+
+    mark_vs_dirty(s);
     gen_set_label(over);
 
     return true;
-- 
2.43.2

On 2/20/24 09:26, Daniel Henrique Barboza wrote:
> While discussing a problem with how we're (not) setting vstart_eq_zero
> Richard had the following to say w.r.t the conditional mark_vs_dirty()
> calls on load/store functions [1]:
> 
> "I think it's required to have stores set dirty unconditionally, before
> the operation.
> 
> Consider a store that traps on the 2nd element, leaving vstart = 2, and
> exiting to the main loop via exception. The exception enters the kernel
> page fault handler. The kernel may need to fault in the page for the
> process, and in the meantime task switch.
> 
> If vs dirty is not already set, the kernel won't know to save vector
> state on task switch."
> 
> Do a mark_vs_dirty() before store operations. Keep the mark_vs_dirty()
> call at the end for loads - the function is a no-op if mstatus_vs is
> already set to EXT_STATUS_DIRTY so there's no hurt in store functions
> calling it twice.
> 
> [1] https://lore.kernel.org/qemu-riscv/72c7503b-0f43-44b8-aa82-fbafed2aac0c@linaro.org/
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/insn_trans/trans_rvv.c.inc | 29 +++++++++++++++----------
>   1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
> index 9e101ab434..2065e9064e 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -636,12 +636,13 @@ static bool ldst_us_trans(uint32_t vd, uint32_t rs1, uint32_t data,
>       tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
>       tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
>   
> -    fn(dest, mask, base, tcg_env, desc);
> -
> -    if (!is_store) {
> +    if (is_store) {
>           mark_vs_dirty(s);
>       }
>   
> +    fn(dest, mask, base, tcg_env, desc);
> +
> +    mark_vs_dirty(s);

You misunderstood here, I think.
Both loads and stores need to set dirty early, before any exit via exception path.

I see that I did say only stores in the quoted mail, but I believe that was merely in 
reference to stores not setting dirty *at all* beforehand.


r~

On 2/20/24 17:17, Richard Henderson wrote:
> On 2/20/24 09:26, Daniel Henrique Barboza wrote:
>> While discussing a problem with how we're (not) setting vstart_eq_zero
>> Richard had the following to say w.r.t the conditional mark_vs_dirty()
>> calls on load/store functions [1]:
>>
>> "I think it's required to have stores set dirty unconditionally, before
>> the operation.
>>
>> Consider a store that traps on the 2nd element, leaving vstart = 2, and
>> exiting to the main loop via exception. The exception enters the kernel
>> page fault handler. The kernel may need to fault in the page for the
>> process, and in the meantime task switch.
>>
>> If vs dirty is not already set, the kernel won't know to save vector
>> state on task switch."
>>
>> Do a mark_vs_dirty() before store operations. Keep the mark_vs_dirty()
>> call at the end for loads - the function is a no-op if mstatus_vs is
>> already set to EXT_STATUS_DIRTY so there's no hurt in store functions
>> calling it twice.
>>
>> [1] https://lore.kernel.org/qemu-riscv/72c7503b-0f43-44b8-aa82-fbafed2aac0c@linaro.org/
>>
>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/insn_trans/trans_rvv.c.inc | 29 +++++++++++++++----------
>>   1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
>> index 9e101ab434..2065e9064e 100644
>> --- a/target/riscv/insn_trans/trans_rvv.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
>> @@ -636,12 +636,13 @@ static bool ldst_us_trans(uint32_t vd, uint32_t rs1, uint32_t data,
>>       tcg_gen_addi_ptr(dest, tcg_env, vreg_ofs(s, vd));
>>       tcg_gen_addi_ptr(mask, tcg_env, vreg_ofs(s, 0));
>> -    fn(dest, mask, base, tcg_env, desc);
>> -
>> -    if (!is_store) {
>> +    if (is_store) {
>>           mark_vs_dirty(s);
>>       }
>> +    fn(dest, mask, base, tcg_env, desc);
>> +
>> +    mark_vs_dirty(s);
> 
> You misunderstood here, I think.
> Both loads and stores need to set dirty early, before any exit via exception path.
> 
> I see that I did say only stores in the quoted mail, but I believe that was merely in reference to stores not setting dirty *at all* beforehand.

hmmm it made sense when I read your reply to set just for stores because I thought
that loads wouldn't trigger page context switches in the kernel. TBH I got too
caught up by the existing "if (!is_store)" in the code, trying to figure it out
why it was there.

In another read in the spec there's nothing that indicates that stores needs
additional handling, which means that we can treat both equally in this regard.


I'll change it for v4. Thanks,


Daniel


> 
> 
> r~