Re: [PATCH v6 1/5] riscv: Introduce support for hardware break/watchpoints

[Chen Pei <cp0613@linux.alibaba.com>]

Hi Himanshu,

Thanks for the series. Two small observations on the tdata1.hit
handling, based on my reading of the Sdtrig spec.

> +			/*
> +			 * Prefer tdata1.hit from SBI trigger readout whenever
> +			 * possible. Fall back to address-based matching if HIT
> +			 * isn't observed/supported.
> +			 */
> +			raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> +					      *this_cpu_ptr(&ecall_lock_flags));
> +			shmem = this_cpu_ptr(sbi_dbtr_shmem);
> +			sret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,
> +					 i, 1, 0, 0, 0, 0);
> +			if (!sret.error) {
> +				tdata1 = le_to_cpu(shmem->data.tdata1);
> +
> +				if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
> +					hit = !!(tdata1 & RISCV_DBTR_MC_HIT_BIT_MASK);
> +				else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
> +					hit = !!(tdata1 & RISCV_DBTR_MC6_HIT_BIT_MASK);
> +			}

It looks like the hit bit is read here but not cleared afterwards.
The debug spec describes mcontrol.hit as:

  "If this bit is implemented then it must become set when this
   trigger fires and may become set when this trigger matches.
   The trigger's user can set or clear it at any time."

and for mcontrol6 hit0/hit1:

  "The TM updates this field when the trigger fires. After the
   debugger has seen the update, it will normally write 0 to this
   field so it can see future changes."

So the hardware only sets hit, and clearing seems to be left to the
trigger's user. Without a clear, a trigger that fired once would keep
hit=1 and might be reported as matching on later unrelated traps on
the same hart. Would it make sense to write tdata1 back with the hit
bit(s) cleared (e.g. via TRIG_UPDATE) once a hit has been observed?

A related minor point: for mcontrol6 with tinfo.version > 0, hit1 and
hit0 combine into a 2-bit field (0=false, 1=before, 2=after). If only
hit0 is tested, the "after" encoding (hit1=1, hit0=0) might be missed
and would then rely on the address-based fallback. Checking (and
clearing) both bits could make the hit detection a bit more robust.

Best regards,
Pei

-- 
Chen Pei <cp0613@linux.alibaba.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv