[PATCH] lib: sbi_pmu: Before using we should ensure PMU init done

[Xiang W]

在 2024-01-31星期三的 22:05 -0800，Bo Gan写道：
> On 1/31/24 6:01 PM, yang.zhang wrote:
> > From: "yang.zhang" <yang.zhang at hexintek.com>
> > 
> > If trap earlier before sbi_pmu_init done, some path would call
> > sbi_pmu_ctr_incr_fw, then it would go wrong:
> > 1. if phs_ptr_offset is zero, then it get a wrong pmu state ptr
> > 2. if phs_ptr_offset is ok, but we didn't call pmu_set_hart_state_ptr
> > it would be NULL POINT
> > 
> > Of course, the above situation will not occur at present, but i think
> > it's reasonable that we should check it before we use it.
> > 
> > Test:
> > 1. I test it on our platform, for now we only support LR/SC not AMO.
> > So when call spin_lock, would trap for amo instruction then emulate
> > it with LR/SC, then it goes wrong.Of course, this case is special only
> > for us, but other trap earlier cases maybe also encounter this situation.
> 
> I think trap-n-emulate AMO that originated from M mode itself is a bad idea.
> As you already mentioned, it can happen before sbi_pmu_ctr_incr_fw is ready
> to be called by sbi_trap_handler. If we are talking about other earlier traps,
> to me, that only means we have a bug in opensbi. Back to locks, if you do
> the trap-n-emulating route, it can incur significant performance penalty. Thus
> I would strongly prefer to use a LR/SC implementation.
> 
> We should try to make lock implementations more flexible. I've got a JH7110
> (visionfive2) on hand, and its hart1-4 supports both LR/SC and AMO, but its
> hart0 (S7) supports AMO not LR/SC (the opposite to yours).
> 
> Perhaps we can have one implementation that only uses LR/SC and the other that
> only uses AMO. Actually we already does. spin_trylock() uses LR/SC, and
> spin_lock() uses AMO. However, spin_trylock() is currently not used by anyone.
> I think the easiest way is to introduce a config option that tells opensbi to
> only use LR/SC and then use while(!spin_trylock()) to implement spin_lock. Or,
> use another, more optimized LR/SC spin lock implementation when such config
> option is in effect.
> 
> As for the JH7110 S7 hart, I'm kind of on the same boat as you, except that I'm
> on the opposite side. Right now, I can't boot from S7 hart, because the HSM code
> uses atomic_cmpxchg that relies on LR/SC. I'm seeing the same kind of access
> fault as you did. This time, I have no choice but to use a different form of
> synchronization. (No such thing as emulating LR/SC using AMO). I'll try to
> send out my patches for review later.

When gcc adds the -mcpu=sifive-s76 option, the builtin functions for atomic
operations will be implemented through lr/sc. I suggest that atomic related
operations be done through builtin functions instead of inline assembly.

Regards,
Xiang W

> 
> Bo
> 
> > 2.I aslo test on qemu thead-c906, but need do some changes for simulating
> > this scenario.
> >    2.1 revert f067bb84cf2dd6493ff3fa49294d3ec80481ad75, the commit causes
> >    we can't emulate some instructions from M mode, i think the requirement
> >    may be not reasonable.
> >    2.2 Then, insert a 'rdtime' instruction before pmu_get_hart_state_ptr in
> >    sbi_pmu_init, then something goes wrong.
> > 
> > Signed-off-by: yang.zhang <yang.zhang at hexintek.com>
> > ---
> >   lib/sbi/sbi_pmu.c | 27 +++++++++++++++++++++++++--
> >   1 file changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> > index 6209ccc..4eccb82 100644
> > --- a/lib/sbi/sbi_pmu.c
> > +++ b/lib/sbi/sbi_pmu.c
> > @@ -74,7 +74,7 @@ struct sbi_pmu_hart_state {
> >   static unsigned long phs_ptr_offset;
> >   
> >   #define pmu_get_hart_state_ptr(__scratch)				\
> > -	sbi_scratch_read_type((__scratch), void *, phs_ptr_offset)
> > +	phs_ptr_offset ? sbi_scratch_read_type((__scratch), void *, phs_ptr_offset) : NULL
> >   
> >   #define pmu_thishart_state_ptr()					\
> >   	pmu_get_hart_state_ptr(sbi_scratch_thishart_ptr())
> > @@ -207,6 +207,9 @@ int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
> >   	uint32_t event_code;
> >   	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
> >   
> > +	if (unlikely(!phs))
> > +		return SBI_EINVAL;
> > +
> >   	event_idx_type = pmu_ctr_validate(phs, cidx, &event_code);
> >   	if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
> >   		return SBI_EINVAL;
> > @@ -432,6 +435,10 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
> >   		      unsigned long flags, uint64_t ival)
> >   {
> >   	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
> > +
> > +	if (unlikely(!phs))
> > +		return SBI_EINVAL;
> > +
> >   	int event_idx_type;
> >   	uint32_t event_code;
> >   	int ret = SBI_EINVAL;
> > @@ -535,6 +542,10 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
> >   		     unsigned long flag)
> >   {
> >   	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
> > +
> > +	if (unlikely(!phs))
> > +		return SBI_EINVAL;
> > +
> >   	int ret = SBI_EINVAL;
> >   	int event_idx_type;
> >   	uint32_t event_code;
> > @@ -794,6 +805,10 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
> >   			  uint64_t event_data)
> >   {
> >   	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
> > +
> > +	if (unlikely(!phs))
> > +		return SBI_EINVAL;
> > +
> >   	int ret, event_type, ctr_idx = SBI_ENOTSUPP;
> >   	u32 event_code;
> >   
> > @@ -869,6 +884,9 @@ int sbi_pmu_ctr_incr_fw(enum sbi_pmu_fw_event_code_id fw_id)
> >   	uint64_t *fcounter = NULL;
> >   	struct sbi_pmu_hart_state *phs = pmu_thishart_state_ptr();
> >   
> > +	if (unlikely(!phs))
> > +		return 0;
> > +
> >   	if (likely(!phs->fw_counters_started))
> >   		return 0;
> >   
> > @@ -967,7 +985,12 @@ void sbi_pmu_exit(struct sbi_scratch *scratch)
> >   	if (sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_10)
> >   		csr_write(CSR_MCOUNTEREN, -1);
> >   
> > -	pmu_reset_event_map(pmu_get_hart_state_ptr(scratch));
> > +	struct sbi_pmu_hart_state *phs = pmu_get_hart_state_ptr(scratch);
> > +
> > +	if (unlikely(!phs))
> > +		return;
> > +
> > +	pmu_reset_event_map(phs);
> >   }
> >   
> >   int sbi_pmu_init(struct sbi_scratch *scratch, bool cold_boot)
> > 
> 
>