The senario when SCOUNTEREN[TM] == 0 && MCOUNTEREN[TM]==1 ?

[Anup Patel]

Hi,

Can you please join OpenSBI mailing list ? Otherwise your reply will bounce.

> -----Original Message-----
> From: Dylan Jhung <dylan at andestech.com>
> Sent: 12 August 2020 13:28
> To: Anup Patel <Anup.Patel at wdc.com>
> Cc: opensbi-bounces at lists.infradead.org
> Subject: Re: The senario when SCOUNTEREN[TM] == 0 &&
> MCOUNTEREN[TM]==1 ?
> 
> On Tue, Aug 11, 2020 at 12:09:32PM +0000, Anup Patel wrote:
> 
> Hi Anup,
> 
> There are few questions about [S|M]COUNTEREN.
> 
> >
> >
> > > -----Original Message-----
> > > From: opensbi <opensbi-bounces at lists.infradead.org> On Behalf Of
> > > Ruinland ChuanTzu Tsai
> > > Sent: 11 August 2020 15:23
> > > To: opensbi at lists.infradead.org
> > > Subject: The senario when SCOUNTEREN[TM] == 0 &&
> > > MCOUNTEREN[TM]==1 ?
> > >
> > > Hi all,
> > > I'm having some questions about the timing spec for RISC-V and OpenSBI.
> > >
> > > If I understand the spec correctly, we allow implementation to use a
> > > real timer CSR or triggers an illegal instruction exception and
> > > request OpenSBI to do the routine e.g. read a memory-mapped register.
> >
> > Trap-n-emulate of TIME CSR in OpenSBI is only required for platforms
> > that do not implement TIME CSR in HW. This is because RISC-V does not
> > mandate TIME CSR to be provided by all RISC-V platforms.
> >
> > For platforms having TIME CSR implemented in HW, no illegal
> > instruction trap is generated hence no trap-n-emulate by OpenSBI on such
> platforms.
> > Example, QEMU virt machine.
> >
> > >
> > > The problem is :
> > > As the Privileged ISA Spec stated, once the SCOUNTEREN[TM] is set to
> > > 0, then U-mode is not allowed to access timer; and thus the OpenSBI
> > > will refuse to provide timer info - -
> > > https://github.com/riscv/opensbi/blob/master/lib/sbi/sbi_emulate_csr
> > > .c#L3
> > > 2
> > >
> > > This was not a problem before, yet recently Linux Kernel 5.8 changed
> > > its RISC-V vdso implementation to issue rdtime in the user space directly.
> > >
> > > On some platform, this causes a backward-compat issue - - the
> > > hardware implementation without a real timer CSR internally expects
> > > that only S-mode could issue rdtime and hard-wires SCOUNTEREN[TM] to
> > > be 0 so the CPU will issue an illegal instruction exception and let OpenSBI
> do the job.
> > >
> > > So when OpenSBI checks SCOUNTEREN[TM], there will be no one to
> > > provide timer info and the init on rootfs will crash.
> > >
> > > Furthermore, this raises a question to me - - when will
> > > SCOUNTEREN[TM] be set to 0 while MCOUNTEREN[TM] set to 1 ?
> > >
> > > My gut-feeling is that the hypervisor might want this feature, so
> > > the guest U- mode program won't be able to access time CSR directly
> > > and the OpenSBI will return -1 so the S-mode program (e.g.
> > > Xvisor/KVM/Xen) could do the corresponding emulation, such as faking a
> timer.
> > >
> > > Yet I haven't seen any related code there.
> > > Is there a reason that we're checking SCOUNTEREN[TM] while the
> > > previous privileged mode is U-mode ?
> >
> > We are checking SCOUNTEREN[TM] in TIME CSR read emulation to emulate
> > correct behavior SCOUNTEREN CSR but we should this bit when TIME CSR
> > is not implemented in HW.
> >
> > This can be fixed in OpenSBI because we detect whether TIME CSR is
> > present or not for each HART at boot time.
> >
> > Maybe we can do something like below:
> >
> > 	if (prev_mode == PRV_U) {
> > 		cen = csr_read(CSR_SCOUNTEREN);
> > 		if (csr_num == CSR_TIME &&
> > 		    !sbi_hart_has_feature(sbi_scratch_thishart_ptr(),
> > 					  SBI_HART_HAS_TIME))
> > 			cen |= (1UL << 1);
> > 	}
> >
> 
> Abount SCOUNTEREN:
> 
> For my understanding, the code above means that if TIME CSR is not
> implemented, we will ignore the checking of SCOUNTEREN[TM].

Yes, the above code will ignore SCOUNTERN[TM] checking when TIME CSR
is not implemented.

> 
> I think OpenSBI should keep the functionality of SCOUNTEREN[TM], which is
> checking wheather lower-privilege-mode may access TIME CSR.

I think we should never bypass [M|H|S]COUNTEREN CSRs when checking
permission for almost all COUNTERs (except TIME).

The TIME CSR is a special case because we use it as clocksource for Linux
kernel. I think we should ignore TM bit in [M|H|S]COUNTEREN CSRs if
TIME CSR is not implemented in HW. This will work well for VDSO as well.

> 
> 
> About MCOUNTEREN:
> 
> Currently, we just enable all bits as default in OpenSBI:
> 	ulong cen = -1UL;
> 
> why doesn't we have some checking like below in OpenSBI:
> 
>    if (prev_mode == PRV_S) {
>    		cen = csr_read(CSR_MCOUNTEREN);
>    }
> 
> Is there any reason that we should ignore the checking of MCOUNTEREN?

Till now we just gave S-mode free pass to access because the CSR emulation
code was inspired from BBL CSR emulation.

I agree with your comment. We should consider MCOUNTEREN, HCOUNTEREN
and SCOUNTEREN CSRs when checking permissions for COUNTER CSRs. I will
try to come up with a patch for this.

Thanks for pointing.

> 
> 
> > >
> > > Besides, I'm wondering whether we should read [M|S]COUNTEREN in
> the
> > > first place.
> >
> > Checking [M|S]COUNTEREN is right thing to do because a RISC-V platform
> > may not have HPMCOUNTER CSRs (alias of MHPMCOUNTER CSRs) even if
> > it has MHPMCOUNTER CSRs.
> >
> > > Since it's an enablement bit and it's designed as WARL, should it be just
> > > written by software and enables some hardware features, instead of
> being a
> > > "global status holder" for software to bridging different privileged modes
> ?
> >
> > The [M|H|S]COUNTEREN CSRs are like switches to enable/disable
> > lower-privilege-mode access to various HPMCOUNTER CSRs. A hypervisor
> > can certainly use this to explicitly emulate certain HPMCOUNTER CSRs.
> >
> 
> 
> 
> > Regards,
> > Anup
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
> 
> Regards,
> Dylan Jhong

Regards,
Anup