On 5/18/23 10:06, Conor Dooley wrote: > On Thu, May 18, 2023 at 07:13:15PM +0530, Anup Patel wrote: >> On Thu, May 18, 2023 at 4:02 PM Andrew Jones <ajones@xxxxxxxxxxxxxxxx> wrote: >> > On Thu, May 18, 2023 at 09:58:30AM +0100, Conor Dooley wrote: > >> > > - riscv,isa: >> > > - description: >> > > - Identifies the specific RISC-V instruction set architecture >> > > - supported by the hart. These are documented in the RISC-V >> > > - User-Level ISA document, available from >> > > - https://riscv.org/specifications/ >> > > - >> > > - Due to revisions of the ISA specification, some deviations >> > > - have arisen over time. >> > > - Notably, riscv,isa was defined prior to the creation of the >> > > - Zicsr and Zifencei extensions and thus "i" implies >> > > - "zicsr_zifencei". >> > > - >> > > - While the isa strings in ISA specification are case >> > > - insensitive, letters in the riscv,isa string must be all >> > > - lowercase to simplify parsing. >> > > - $ref: "/schemas/types.yaml#/definitions/string" >> > > - pattern: ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:[hsxz](?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$ >> > > - >> > > # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here >> > > timebase-frequency: false >> > > >> > > @@ -133,8 +117,13 @@ properties: >> > > DMIPS/MHz, relative to highest capacity-dmips-mhz >> > > in the system. >> > > >> > > +oneOf: >> > > + - required: >> > > + - riscv,isa >> > >> > This is the part Anup keeps reminding me about. We can create better ways >> > to handle extensions in DT and ACPI, but we'll still need to parse ISA >> > strings to handle legacy DTs and holdouts that keep creating ISA strings, >> > at least during the deprecation period, since ISA strings are still "the >> > way to do it" according to the spec. >> >> Coming up with an alternate way in DT is fine but we can't deprecate >> ISA strings since ISA strings are widely used: >> 1) Various bootloaders > > Aye, for the reason, as I mentioned earlier and in the RFC thread, > removing existing parsers isn't a good idea. > >> 2) It is part of /proc/cpuinfo > > That is irrelevant. > >> 3) Hypervisors use it to communicate HW features to Guest/VM. >> Hypervisors can't get away from generating ISA strings because >> Hypervisors don't know what is running inside Guest/VM. > > Generate both :) As things stand, your guests could interpret what you > communicate to them via riscv,isa differently! > >> In the case of ACPI, it is a very different situation. Like Sunil mentioned, >> ACPI will always follow mechanisms defined by RVI (such as ISA string). >> Other ACPI approaches such as GUID for ISA extension are simply not >> scalable and will take a lot more memory for ACPI tables compared to >> ISA strings. > > My proposal should actually suit ACPI, at least for Linux, as it would > be a chance to align currently misaligned definitions. I won't speak to > GUIDs or whatever as that's someone else's problem :) > >> > Also, if we assume the wording in the spec does get shored up, then, >> > unless I'm missing something, the list of advantages for this boolean >> > proposal from your commit message would be >> >> IMO, we should try our best to have the wordings changed in RVI spec. > > Yes, doing so is beneficial for all of us regardless of what happens > here. I do think that it is partially orthogonal - it allows us to not > design an interface that needs to be capable of communicating a wide > variety of versions, but I don't think it solves some of the issues > that riscv,isa has. If I thought it did, I would not have gone to the > trouble of respinning this patch out of the other approach. > >> > * More character choices for name -- probably not a huge gain for ratified >> > extensions, since the boolean properties will likely still use the same >> > name as the ISA string (riscv,isa-extension-<name>). But, for vendor >> > extensions, this is indeed a major improvement, since vendor extension >> > boolean property names may need to be extended in unambiguous ways to >> > handle changes in the extension. >> > >> > * Simpler, more complete DT validation (but we still need a best effort >> > for legacy ISA strings) >> > >> > * Simpler DT parsing (but we still need the current parser for legacy ISA >> > strings) >> > >> > > + - required: >> > > + - riscv,isa-base >> > > + >> > > required: >> > > - - riscv,isa >> > > - interrupt-controller >> > > >> > > additionalProperties: true >> > > @@ -177,7 +166,13 @@ examples: >> > > i-tlb-size = <32>; >> > > mmu-type = "riscv,sv39"; >> > > reg = <1>; >> > > - riscv,isa = "rv64imafdc"; >> > > + riscv,isa-base = "rv64i"; >> > > + riscv,isa-extension-i; >> > > + riscv,isa-extension-m; >> > > + riscv,isa-extension-a; >> > > + riscv,isa-extension-f; >> > > + riscv,isa-extension-d; >> > > + riscv,isa-extension-c; >> >> One downside of this new approach is it will increase the size of DTB. >> Imaging 50 such DT properties in 46 CPU DT nodes. > > I should do a comparison between 50 extensions in riscv,isa and doing > this 50 times and see what the sizes are. Why not just have something like mycpu { ... riscv,isa { i; m; a; zicsr; ... }; }; ? --Sean
