Re: [PATCH v6 03/20] xen/riscv: introduce extenstion support check by compiler
On Wed, Mar 20, 2024 at 07:58:05PM +0100, Oleksii wrote:
> On Mon, 2024-03-18 at 17:58 +0100, Jan Beulich wrote:
> > On 15.03.2024 19:05, Oleksii Kurochko wrote:
> > > Currently, RISC-V requires two extensions: _zbb and _zihintpause.
> >
> > Do we really require Zbb already?
> After an introduction of Andrew C. patches [1] it is requited for
> __builtin_ffs{l}
>
> [1]
> https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.coop...@citrix.com/T/#t
> >
> > > This patch introduces a compiler check to check if these extensions
> > > are supported.
> > > Additionally, it introduces the riscv/booting.txt file, which
> > > contains
> > > information about the extensions that should be supported by the
> > > platform.
> > >
> > > In the future, a feature will be introduced to check whether an
> > > extension
> > > is supported at runtime.
> > > However, this feature requires functionality for parsing device
> > > tree
> > > source (DTS), which is not yet available.
> >
> > Can't you query the CPU for its features?
> I couldn't find such reg ( or SBI call ) in the spec.
There isn't.
> SBI call sbi_probe_extension() exists, but it doesn't check for every
> possible extension. As far as I understand it checks only for that one
> which are present in SBI spec.
Yeah, it only checks for SBI extensions, not ISA extensions.
> The most closest thing I see how to check that without dts is how it is
> done in OpenSBI:
IIRC this only "works" because the OpenSBI devs assume that there are no
non-normative behaviours and all CSRs have their ~God~ RVI defined
meanings. Reading a CSR to see if it traps is not behaviour you can really
rely on unless the platform claims to support Sstrict - but Sstrict you'd
have to detect from the DT so chicken and egg for you! It's one of these
new "extensions" from the profiles spec, so it doesn't even have support
in Linux's dt-bindings yet. Up to Xen devs if you guys want to make the
same assumptions as OpenSBI. Linux doesn't and when we discussed this
not too long ago on the U-Boot ML in the context of the rng CSR it was
also decided not to do make the assumption there either.
Personally I wonder if you can just apply the same policy here as you
did with Zbb in the other thread and assume that something with H will
have Zihintpause and leave implementing a "fake" pause as an exercise
for someone that introduces such a system?
If Jess is correct, and I do remember testing this, it's actually
"always" safe to call the pause instruction on CPUs where the extension
is not supported as it uses an encoding of fence that effectively makes
it a into a nop:
https://lore.kernel.org/linux-riscv/2e96a836-764d-4d07-ab79-3861b9cc2...@jrtc27.com/
At worst, that'd make cpu_relax() a nop if someone didn't meet that
requirement.
FWIW, Linux's cpu_relax() on RISC-V looks like:
static inline void cpu_relax(void)
{
#ifdef __riscv_muldiv
int dummy;
/* In lieu of a halt instruction, induce a long-latency stall.
*/
__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
#endif
#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
/*
* Reduce instruction retirement.
* This assumes the PC changes.
*/
__asm__ __volatile__ ("pause");
#else
/* Encoding of the pause instruction */
__asm__ __volatile__ (".4byte 0x10F");
#endif
barrier();
}
I figure having div is part of your base requirements, so maybe you can
just do something similar in the !zihintpause case if making that
extension a requirement is problematic?
Doing that invalid div used to be conditional, but cpu_relax() is in the
vdso so the static branch it used to be gated with got removed and its
now unconditional. Probably that's not a constraint on Xen's cpu_relax()?
Oh ye, and we do the .4byte crap so that toolchain support wasn't needed
for Zihintpause given we are using it in exactly one place.
Cheers,
Conor.
> #define csr_read_allowed(csr_num, trap) \
> ({ \
> register ulong tinfo asm("a3") = (ulong)trap; \
> register ulong ttmp asm("a4"); \
> register ulong mtvec = sbi_hart_expected_trap_addr(); \
> register ulong ret =
> 0;\
> ((struct sbi_trap_info *)(trap))->cause = 0;\
> asm volatile( \
> "add %[ttmp], %[tinfo],
> zero\n" \
> "csrrw %[mtvec], " STR(CSR_MTVEC) ", %[mtvec]\n"\
> "csrr %[ret],
> %[csr]\n" \
> "csrw " STR(CSR_MTVEC) ", %[mtvec]" \
> : [mtvec] "+&r"(mtvec), [tinfo] "+&r"(tinfo), \
>
Re: [PATCH v6 03/20] xen/riscv: introduce extenstion support check by compiler
On Thu, Mar 21, 2024 at 09:31:59AM +0100, Jan Beulich wrote:
> On 20.03.2024 20:44, Conor Dooley wrote:
> > On Wed, Mar 20, 2024 at 07:58:05PM +0100, Oleksii wrote:
> >> On Mon, 2024-03-18 at 17:58 +0100, Jan Beulich wrote:
> >>> On 15.03.2024 19:05, Oleksii Kurochko wrote:
> >>>> Currently, RISC-V requires two extensions: _zbb and _zihintpause.
> >>>
> >>> Do we really require Zbb already?
> >> After an introduction of Andrew C. patches [1] it is requited for
> >> __builtin_ffs{l}
> >>
> >> [1]
> >> https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.coop...@citrix.com/T/#t
> >>>
> >>>> This patch introduces a compiler check to check if these extensions
> >>>> are supported.
> >>>> Additionally, it introduces the riscv/booting.txt file, which
> >>>> contains
> >>>> information about the extensions that should be supported by the
> >>>> platform.
> >>>>
> >>>> In the future, a feature will be introduced to check whether an
> >>>> extension
> >>>> is supported at runtime.
> >>>> However, this feature requires functionality for parsing device
> >>>> tree
> >>>> source (DTS), which is not yet available.
> >>>
> >>> Can't you query the CPU for its features?
> >> I couldn't find such reg ( or SBI call ) in the spec.
> >
> > There isn't.
> >
> >> SBI call sbi_probe_extension() exists, but it doesn't check for every
> >> possible extension. As far as I understand it checks only for that one
> >> which are present in SBI spec.
> >
> > Yeah, it only checks for SBI extensions, not ISA extensions.
>
> And there was never a consideration to add, at the architecture level,
> some straightforward way for all layers of software to be able to
> easily detect availability of extensions? I find the lack thereof
> pretty surprising ...
No, there sorta is. There's a "configuration structure" spec in the
works, but the (public?) documentation for it is very nascent:
https://github.com/riscv/configuration-structure/
It uses (what I seem to recall is an m-mode) CSR to give the address
of an ASN.1 structure in memory which is intended to encode a whole
raft of information. The CSR itself might be M-Mode, but that doesn't
prevent accessing the data itself at any layer in the stack. The last
time I read it I got the impression it was supposed to be usable for
describing an entire system (including things like i2c or spi
controllers). I don't think it ever explicitly says that in the spec,
but there's a note (IIRC) that mentions being unsuitable for describing
devices on dynamic buses like PCI, so that kinda implies anything else
is fair game.
Hope that helps?
signature.asc
Description: PGP signature
Re: [PATCH v3] xen/riscv: identify specific ISA supported by cpu
On Thu, Feb 06, 2025 at 02:05:31PM +0100, Oleksii Kurochko wrote:
> On 2/5/25 8:07 PM, Conor Dooley wrote:
> > On Thu, Jan 23, 2025 at 03:46:36PM +0100, Oleksii Kurochko wrote:
> > > Supported ISA extensions are specified in the device tree within the CPU
> > > node, using two properties: `riscv,isa-extensions` and `riscv,isa`.
> > >
> > > Currently, Xen does not support the `riscv,isa-extensions` property, as
> > > all available device tree source (DTS) files in the Linux kernel
> > > (v6.12-rc3)
> > > and DTBs generated by QEMU use only the `riscv,isa` property.
> > That's not true? The riscv,isa-extensions property went into linux in
> > late 2023 (6.7 or so) and QEMU in v9.0.0 about a year ago, so all source
> > files in linux and blobs generated by QEMU should have both. OpenSBI &
> > U-Boot support both also. Might not affect your decision about what to
> > support here - but the rationale provided for implementing the deprecated
> > (per the binding at least) property isn't accurate.
>
> I confused something with Linux kernel sources. Double-check and
> riscv,isa-extensions
> is really supported.
>
> Regarding QEMU, at the momemnt, Xen uses Debian bookworm and the following
> version
> is used:
> QEMU emulator version 7.2.11 (Debian 1:7.2+dfsg-7+deb12u6)
>
> I will update the commit message.
>
> Do you ( Conor and Jan ) think that it makes sense to support deprecated
> property?
> Or it would be better start to use QEMU v9.0.0 and just drop support for
> deprecated property?
I'm not entirely sure how Xen re-assembles the dtb that it passes to the
guess (cos you need to strip things you don't support yet, right?), but
to keep things simpler as you're doing bringup I think you could
continue on as you are. There's some guests that only support riscv,isa
like the BSDs (IIRC), so you'd get yourself a better testing base by
sticking to dealing with just that one property.
If I were you though, I'd probably try to match interpretation with
what guests are doing as that's less likely to cause problems.
> > > Therefore, only `riscv,isa` parsing is implemented.
> > >
> > > The `riscv,isa` property is parsed for each CPU, and the common extensions
> > > are stored in the `host_riscv_isa` bitmap.
> > > This bitmap is then used by `riscv_isa_extension_available()` to check
> > > if a specific extension is supported.
> > >
> > > The current implementation is based on Linux kernel v6.12-rc3
> > > implementation with the following changes:
> > > - Drop unconditional setting of {RISCV_ISA_EXT_ZICSR,
> > > RISCV_ISA_EXT_ZIFENCEI, RISCV_ISA_EXT_ZICNTR, RISCV_ISA_EXT_ZIHPM} as
> > > they
> > > are now part of the riscv,isa string.
> > Hmm, not sure I follow your logic here. Linux unconditionally sets these
> > extensions because the binding was written when these these were part of
> > the base instruction set*. That they can be put into the "riscv,isa"
> > string is actually the reason that the code setting them unconditionally
> > in linux exists! Linux cannot tell the difference between an "old" dtb
> > containing "rv64ima" meaning that Zicsr is present & a "new" one containing
> > "rv64ima" meaning that it is not! For backwards compatibility reasons,
> > linux is then forced to set its internal flags for Zicsr et al when it sees
> > "i" in riscv,isa. If you were to use the "new" definition of "i", and use
> > that to construct a dtb to pass to a linux guest, things would blow up.
>
> My fault was that I didn't consider that someone will use "old" dtb and it
> was the reason
> why "the binding was written when these these were part of the base
> instruction set*" was
> interpreted as it isn't so any more and the mentioned extension should be
> explicitly
> mentioned in riscv,isa.
>
> >
> > This is the whole reason that riscv,isa is marked deprecated in the binding
> > and replaced by riscv,isa-extensions - there are no concrete definitions
> > for what extensions mean using "riscv,isa".
> >
> > I suppose you're free to decide to interpret the property in Xen
> > differently to linux - particularly because the hypervisor extension
> > requirement means that you're only going to run on hardware produced after
> > the aforementioned extensions were split out of "i" - but if you are
> > doing that, the rationale for a differing interpretation should be correct
> > IMO
Re: [PATCH v3] xen/riscv: identify specific ISA supported by cpu
Yo,
On Thu, Jan 23, 2025 at 03:46:36PM +0100, Oleksii Kurochko wrote:
> Supported ISA extensions are specified in the device tree within the CPU
> node, using two properties: `riscv,isa-extensions` and `riscv,isa`.
>
> Currently, Xen does not support the `riscv,isa-extensions` property, as
> all available device tree source (DTS) files in the Linux kernel (v6.12-rc3)
> and DTBs generated by QEMU use only the `riscv,isa` property.
That's not true? The riscv,isa-extensions property went into linux in
late 2023 (6.7 or so) and QEMU in v9.0.0 about a year ago, so all source
files in linux and blobs generated by QEMU should have both. OpenSBI &
U-Boot support both also. Might not affect your decision about what to
support here - but the rationale provided for implementing the deprecated
(per the binding at least) property isn't accurate.
> Therefore, only `riscv,isa` parsing is implemented.
>
> The `riscv,isa` property is parsed for each CPU, and the common extensions
> are stored in the `host_riscv_isa` bitmap.
> This bitmap is then used by `riscv_isa_extension_available()` to check
> if a specific extension is supported.
>
> The current implementation is based on Linux kernel v6.12-rc3
> implementation with the following changes:
> - Drop unconditional setting of {RISCV_ISA_EXT_ZICSR,
>RISCV_ISA_EXT_ZIFENCEI, RISCV_ISA_EXT_ZICNTR, RISCV_ISA_EXT_ZIHPM} as they
>are now part of the riscv,isa string.
Hmm, not sure I follow your logic here. Linux unconditionally sets these
extensions because the binding was written when these these were part of
the base instruction set*. That they can be put into the "riscv,isa"
string is actually the reason that the code setting them unconditionally
in linux exists! Linux cannot tell the difference between an "old" dtb
containing "rv64ima" meaning that Zicsr is present & a "new" one containing
"rv64ima" meaning that it is not! For backwards compatibility reasons,
linux is then forced to set its internal flags for Zicsr et al when it sees
"i" in riscv,isa. If you were to use the "new" definition of "i", and use
that to construct a dtb to pass to a linux guest, things would blow up.
This is the whole reason that riscv,isa is marked deprecated in the binding
and replaced by riscv,isa-extensions - there are no concrete definitions
for what extensions mean using "riscv,isa".
I suppose you're free to decide to interpret the property in Xen
differently to linux - particularly because the hypervisor extension
requirement means that you're only going to run on hardware produced after
the aforementioned extensions were split out of "i" - but if you are
doing that, the rationale for a differing interpretation should be correct
IMO.
Perhaps the wording of my comment in linux was not "strong" enough, and
the "can be set" should be changed to "must be set"?
/*
* These ones were as they were part of the base ISA when the
* port & dt-bindings were upstreamed, and so can be set
* unconditionally where `i` is in riscv,isa on DT systems.
*/
if (acpi_disabled) {
set_bit(RISCV_ISA_EXT_ZICSR, source_isa);
set_bit(RISCV_ISA_EXT_ZIFENCEI, source_isa);
set_bit(RISCV_ISA_EXT_ZICNTR, source_isa);
set_bit(RISCV_ISA_EXT_ZIHPM, source_isa);
}
* IIRC only 2 of them were part of i, the other two were assumed to be present
by Linux etc and only became independent extensions later on.
> - Remove saving of the ISA for each CPU, only the common available ISA is
>saved.
> - Remove ACPI-related code as ACPI is not supported by Xen.
> - Drop handling of elf_hwcap, since Xen does not provide hwcap to
>userspace.
> - Replace of_cpu_device_node_get() API, which is not available in Xen,
>with a combination of dt_for_each_child_node(), dt_device_type_is_equal(),
>and dt_get_cpuid_from_node() to retrieve cpuid and riscv,isa in
>riscv_fill_hwcap_from_isa_string().
> - Rename arguments of __RISCV_ISA_EXT_DATA() from _name to ext_name, and
>_id to ext_id for clarity.
> - Replace instances of __RISCV_ISA_EXT_DATA with RISCV_ISA_EXT_DATA.
> - Replace instances of __riscv_isa_extension_available with
>riscv_isa_extension_available for consistency. Also, update the type of
>`bit` argument of riscv_isa_extension_available().
> - Redefine RISCV_ISA_EXT_DATA() to work only with ext_name and ext_id,
>as other fields are not used in Xen currently.
> - Add check of first 4 letters of riscv,isa string to
>riscv_isa_parse_string() as Xen doesn't do this check before so it is
>necessary to check correctness of riscv,isa string. ( it should start with
>rv{32,64} with taking into account upper and lower case of "rv").
> - Drop an argument of riscv_fill_hwcap() and
> riscv_fill_hwcap_from_isa_string()
>as it isn't used, at the moment.
> - Update
