Re: [PATCH v4 4/5] target/riscv: add fault-only-first unit stride load

[LIU Zhiwei]

On 2020/2/28 4:03, Richard Henderson wrote:
> On 2/25/20 2:35 AM, LIU Zhiwei wrote:
> > +GEN_VEXT_LD_ELEM(vlbff_v_b, int8_t,  int8_t,  H1, ldsb)
> > +GEN_VEXT_LD_ELEM(vlbff_v_h, int8_t,  int16_t, H2, ldsb)
> > +GEN_VEXT_LD_ELEM(vlbff_v_w, int8_t,  int32_t, H4, ldsb)
> > +GEN_VEXT_LD_ELEM(vlbff_v_d, int8_t,  int64_t, H8, ldsb)
> > +GEN_VEXT_LD_ELEM(vlhff_v_h, int16_t, int16_t, H2, ldsw)
> > +GEN_VEXT_LD_ELEM(vlhff_v_w, int16_t, int32_t, H4, ldsw)
> > +GEN_VEXT_LD_ELEM(vlhff_v_d, int16_t, int64_t, H8, ldsw)
> > +GEN_VEXT_LD_ELEM(vlwff_v_w, int32_t, int32_t, H4, ldl)
> > +GEN_VEXT_LD_ELEM(vlwff_v_d, int32_t, int64_t, H8, ldl)
> > +GEN_VEXT_LD_ELEM(vleff_v_b, int8_t,  int8_t,  H1, ldsb)
> > +GEN_VEXT_LD_ELEM(vleff_v_h, int16_t, int16_t, H2, ldsw)
> > +GEN_VEXT_LD_ELEM(vleff_v_w, int32_t, int32_t, H4, ldl)
> > +GEN_VEXT_LD_ELEM(vleff_v_d, int64_t, int64_t, H8, ldq)
> > +GEN_VEXT_LD_ELEM(vlbuff_v_b, uint8_t,  uint8_t,  H1, ldub)
> > +GEN_VEXT_LD_ELEM(vlbuff_v_h, uint8_t,  uint16_t, H2, ldub)
> > +GEN_VEXT_LD_ELEM(vlbuff_v_w, uint8_t,  uint32_t, H4, ldub)
> > +GEN_VEXT_LD_ELEM(vlbuff_v_d, uint8_t,  uint64_t, H8, ldub)
> > +GEN_VEXT_LD_ELEM(vlhuff_v_h, uint16_t, uint16_t, H2, lduw)
> > +GEN_VEXT_LD_ELEM(vlhuff_v_w, uint16_t, uint32_t, H4, lduw)
> > +GEN_VEXT_LD_ELEM(vlhuff_v_d, uint16_t, uint64_t, H8, lduw)
> > +GEN_VEXT_LD_ELEM(vlwuff_v_w, uint32_t, uint32_t, H4, ldl)
> > +GEN_VEXT_LD_ELEM(vlwuff_v_d, uint32_t, uint64_t, H8, ldl)
> We definitely should not have a 3rd copy of these.
Yes, I will remove it by add a parameter to GEN_VEXT_LDFF.
> > +        if (i == 0) {
> > +            probe_read_access(env, addr, nf * msz, ra);
> > +        } else {
> > +            /* if it triggles an exception, no need to check watchpoint */
> triggers.
Yes.
> > +            offset = -(addr | TARGET_PAGE_MASK);
> > +            remain = nf * msz;
> > +            while (remain > 0) {
> > +                host = tlb_vaddr_to_host(env, addr, MMU_DATA_LOAD, mmuidx);
> > +                if (host) {
> > +#ifdef CONFIG_USER_ONLY
> > +                    if (page_check_range(addr, nf * msz, PAGE_READ) < 0) {
> > +                        vl = i;
> > +                        goto ProbeSuccess;
> > +                    }
> > +#else
> > +                    probe_read_access(env, addr, nf * msz, ra);
> > +#endif
> Good job finding all of the corner cases.  I should invent a new cputlb
> function that handles this better.  For now, this is the best we can do.
That will be better.

I learn a lot from SVE and some S390  code. Thanks a lot.

Best Regards,
Zhiwei
> r~