Open wmat opened 1 year ago
Hi Folks, I'm just circling back on this PR to finalize the move to asciidoc. I'd like to take the next steps and merge the asciidoc into main asap. Can I get some feedback on whether we can go ahead with this change?
@jhauser-us, can you help here?
Note that before this PR is merged a LaTeX branch should be created from main to preserve the LaTeX content at a point in time.
Insert a "RISC-V" between "for" and ","
Do you know the line number and/or the sentence?
@wmat, isn't that comment against line #4 of intro.adoc?
BTW, still working on the review, finding words that have been dropped. It's quite tedious, even with tooling.
Hmmm, something doesn't seem right as I can see that I added that RISC-V text back in August 2023 with this commit: https://github.com/riscv/riscv-aia/commit/ebb0ed5642940b4f225f943fd079ed8b23865294#diff-532a9fe2a40c6696777f2473a7a54fd9dda4bbe75ae915501a64016c92d6144a
It looks like this commit: https://github.com/riscv/riscv-aia/commit/5923f6b1d684ddedc8f6b09bd2cb37a0fd7b0165#diff-532a9fe2a40c6696777f2473a7a54fd9dda4bbe75ae915501a64016c92d6144a overwrote many of the changes I'd made in August.
Bummer. So, do you want me to continue my search for other missing "RISC-V" words or do you want to "undo" the lost changes?
I'll work through the changes that were overwritten and fix.
I believe I've completed all of the changes to this PR to complete the move to AsciiDoc for AIA. The original latex src lives now in the latex branch. I'd appreciate a review of the PDF produced in the last run of the Workflow. You'll notice broken LaTeX math in places due to Linux fonts not being present, which I'm working on. Once that's fixed I believe we can flip the switch.
@jhauser-us I believe AIA is ready for the asciidoc branch to become the main branch. Can you please review the latest asciidoc generated PDF and provide your signoff to move forward? riscv-interrupts.pdf
@wmat: Where do you want me to post issues that I find? In the comments for this PR?
Yes, that would be great, thanks.
Bill Traynor Documentation Build and Release Engineer RISC-V International
Join us in Munich, Germany at RISC-V Summit Europe https://riscv-europe.org/summit/2024/ from 24-28 June. Be a part of the new wave of European computing innovation!
On Wed, Jun 26, 2024 at 6:35 PM John Hauser @.***> wrote:
@wmat https://github.com/wmat: Where do you want me to post issues that I find? In the comments for this PR?
— Reply to this email directly, view it on GitHub https://github.com/riscv/riscv-aia/pull/53#issuecomment-2192729743, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN6ZDYGVYBUAL5XGNH6UTZJM62JAVCNFSM6AAAAAA4W6T336VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSG4ZDSNZUGM . You are receiving this because you were mentioned.Message ID: @.***>
@jhauser-us any updates here? Would love to get this spec moved over to AsciiDoc.
I'm working through it as time permits. Not done yet.
In the meantime, I'll say that I'm quite dismayed with how often AsciiDoc breaks tables across pages. It's really ugly and a detriment to comprehension.
Yes, that's come up before. I can fix that by adding page breaks right before tables that cross page boundaries. Let me know if that's you preference.
Similarly, do you want a List of Tables and List of Figures right after the TOC?
Yes, that's come up before. I can fix that by adding page breaks right before tables that cross page boundaries. Let me know if that's you preference.
I don't have an opinion about whether that's a good solution. It would mean that edits would regularly require human re-evaluation of whether those inserted page breaks are still correct. But personally I'd find that preferable to the bad breaks we currently get.
Similarly, do you want a List of Tables and List of Figures right after the TOC?
Not me. I've almost never found those useful. (And you'll notice, they're not in the LaTeX version.)
Some errors I've found:
In the Preface:
A subsection, "Changes for RC6 (Ratification Candidate 6)", was added that duplicates "Changes for the ratified version 1.0". RC6 became ratified as 1.0, so they are two names for the same thing.
In Section 1.4, "Interrupt identities at a hart":
In the section's first sentence, register names mcause and scause are dropped, causing "... written to CSR mcause or scause on an interrupt trap" to become "... written to CSR or on an interrupt trap".
In Section 2.1, "Machine-level CSRs":
In the sentence that begins "miselect may also support values ...", the end of the sentence changed from "values above 0xFF" to "values listed above 0xFF".
In Table 3 ("Machine-level CSRs"), the descriptions for all of the high-half CSRs have become wrongly recursive. For example, the description for register mieh should say "Upper 32 bits of mie", not "Upper 32 bits of mieh".
(Also in Table 3, may as well fix "of of" for midelegh. This is not a transcription error but is a mistake in the original.)
In Section 2.2, "Supervisor-level CSRs":
In the text "The allocated values for siselect in the range 0 to 0xFF are once again these:", an extra word to got added to become "... in the range 0 to 0xFF to are once again these:"
In the sentence that begins "Because the VS CSR vsiselect (Section 2.3) ...", the reference to Section 2.3 got changed to "Table 5".
In the sentence that begins "If extension Sscsrind is also implemented, then when siselect has a value in the range 0x30-0x3F or 0x70-0xFF, attempts ...", the word range was moved and a comma was lost, resulting in "... a value in the 0x30-0x3F or 0x70-0xFF range attempts ...."
In Section 2.3, "Hypervisor and VS CSRs":
In Table 5 ("Hypervisor and VS CSRs"), the width is missing for register hviprio2.
In Section 2.4, "Virtual instruction exceptions":
In the sentence that begins "For sireg and vsireg, see both the previous section, 2.3, ...", the reference to Section 2.3 got changed to "Table 5".
In Section 2.5, "Access control by the state-enable CSRs":
In the sentence starting "Bit 59 controls access to AIA CSRs ...", register stopi is misnamed "stope".
In the sentence starting "If bit 60 of mstateen0 is one, then regardless of any other mstateen bits ...", the reference to Section 2.3 has turned into "Table 5".
In the description of bit 60 of hstateen0, "CSRs siselect and sireg" has become "CSRs and", dropping the register names. Also, register vsiselect is misnamed as "viselect".
In the sentence that begins "When XLEN > 32, an attempt to access siph or sieh ...", register sieh is misnamed "seph".
In the sentence that begins "If bit 60 is one in mstateen0 but is zero in hstateen0 ...", the end of the sentence changed from "... any other mstateen bits" to "... any other bits", dropping "mstateen".
In the sentence that begins "Bit 58 is implemented in hstateen0 only if ...", register hstateen0 is misnamed "hstaeen0".
In Section 3.5, "Memory region for an interrupt file":
Register seteipnum_le is misidentified as "seteipnum_be" in this sentence from the original: "A write to seteipnum_le is ignored if the value written is not an implemented interrupt identity number in little-endian byte order."
In Section 3.6, "Arrangement of the memory regions of multiple interrupt files":
The equations "((2^j-1) X 2^E) & A = 0" and "((2^j-1) X 2^E) & B = 0" should be formatted like other mathematical expressions, and in particular, the X's should be multiplication signs to avoid any misunderstanding.
In the formula "g × 2^E | A | (h × 2^C)", the parentheses that were around "g × 2^E" have been lost.
In Section 3.8.2, "External interrupt enable threshold register (eithreshold)":
In the second sentence of the first paragraph, the register name eithreshold has been dropped.
At the end of the first sentence of the second paragraph, "... in the eie array" has become "... in the array".
In Section 3.10, "Interrupt delivery and handling":
In the example algorithm for an external interrupt handler, in the step "call the interrupt handler for external interrupt i (minor identity)", the "i" has been dropped.
Hi @jhauser-us all of the issues you listed above have been addressed.
Hi @jhauser-us just wanted to check in to see if you've had a chance to review.
The next batch of errors I've found:
In Section 4.2, "Interrupt domains":
In the caption for Figure 4, the word that is missing (should be "... four harts that implement M-mode and S-mode, ...").
In Section 4.5, "Memory-mapped control region for an interrupt domain":
In Table 6, the underscore characters are missing from the names of the in_clrip registers, and the names of registers setipnum_le and setipnum_be.
In Table 6, the register at offset 0x1F7C is mislabeled as setie[31]; it should instead be clrie[31].
In Section 4.5.2, "Source configurations (sourcecfg[1]-sourcecfg[1023])":
The comma in this sentence has become misplaced: "When interrupt source i is not delegated to a child domain, sourcecfg[i] has this format: ...."
In Section 4.5.3, "Machine MSI address configuration (mmsiaddrcfg and mmsiaddrcfgh)":
In the first sentence of the second paragraph, register domaincfg is misspelled as "comaincfg".
The comma in this sentence has become misplaced: "When implemented, mmsiaddrcfg has this format: ...."
The format shown for register mmsiaddrcfgh incorrectly has field L as bits 31:0 instead of just bit 31.
In the sentence that begins "In that case, if these other fields were given nonzero values when L was first set ...", register names "mmsiaddrcfg" and "mmsiaddrcfgh" are dropped at the end (should be "... visible by reading mmsiaddrcfg and mmsiaddrcfgh.").
In the sentence that begins "If an APLIC supports additional forms of reset ...", the parenthetical "(as well as smsiaddrcfg and smsiaddrcfgh)" has been changed to "(as well smsiaddrcfg as and smsiaddrcfgh)".
In Section 4.5.4, "Supervisor MSI address configuration (smsiaddrcfg and smsiaddrcfgh)":
The section title has an extra left parenthesis.
In Section 4.5.5, "Set interrupt-pending bits (setip[0]–setip[31])":
The first sentence is missing the word writing (should be "Reading or writing register setip[k] ...").
In Section 4.5.8, "Clear interrupt-pending bit by number (clripnum)":
The first sentence is missing the register name clripnum (should be "... writing 32-bit value i to register clripnum causes ...").
In Section 4.5.9, "Set interrupt-enable bits (setie[0]–setie[31])":
The first sentence is missing the word register (should be "Reading or writing register setie[k] ...").
In Section 4.5.12, "Clear interrupt-enable bit by number (clrienum)":
The last sentence is missing the word of (should be "A read of clrienum always returns zero.").
In Section 4.5.13, "Set interrupt-pending bit by number, little-endian (setipnum le)":
In the second paragraph, register setipnum_le has been incorrectly replaced by setipnum_be (should be "... setipnum_le need not be implemented, ..."), and the word are is missing (should be "... the four bytes at this offset are simply read-only zeros ...").
In Section 4.5.14, "Set interrupt-pending bit by number, big-endian (setipnum be)":
The name of register setipnum_be is misspelled as "setipnym_be".
In Section 4.6, "Reset":
The word UNSPECIFIED and the register name domaincfg are missing (should be "Upon reset of an APLIC, all its state becomes valid and consistent but otherwise UNSPECIFIED, except for: the domaincfg register of each interrupt domain ...").
In the last bullet item, a left parenthesis is missing before the word Section.
In Section 4.8.1.4, "Top interrupt (topi)":
In the sentence that begins "A read of topi returns zero either if ...", the first instance of register name ithreshold is missing (should be "... or if ithreshold is not zero ...").
In Section 4.8.2, "Interrupt delivery and handling":
In the example pseudocode for a trap handler, the variable i is missing from this step: "call the interrupt handler for external interrupt i (minor identity)".
In the last sentence, the closing quotation marks are missing around "external interrupt 0".
In Section 4.9.1, "Addresses and data for outgoing MSIs":
In the first paragraph, parentheses are missing around "Section 4.5.3 and Section 4.5.4".
The formula for a machine-level interrupt domain's MSI address is missing a second right parenthesis after "LHXS". Also, for clarity, it might be smart to add a space after the first left parenthesis (before "Base PPN") and between the last two right parentheses, so the outermost parentheses are better set off.
In the sentence that begins "The address for the MSI is then computed using this machine-level hart index ...", register name mmsiaddrcfgh is missing.
For a supervisor-level domain, the formulas for g and h have the parentheses misplaced that should be around "2^HHXW - 1" and "2^LHXW - 1".
The formula for a supervisor-level domain's MSI address is missing a second right parenthesis after "HHXS + 12".
In Section 5.1, "Defined major interrupts and default priorities":
In Table 9, "Categorization of current and future major interrupts", the word Designated is misspelled as "Deisgnated".
In Section 5.2.1, "Configuring priorities of major interrupts at machine level":
In the sentence that begins "When XLEN = 64 and miselect is an odd value ...", the number 0x3F is miswritten as "ox3F".
Table 10 ("Effect of the machine-level iprio array on the priorities of interrupts taken in M-mode") is incorrectly located within a "NOTE".
In Section 5.2.2, "Machine top interrupt CSR (mtopi)":
In the sentence that begins "If all bytes of the machine-level iprio array are read-only zeros ...", the register name mtopi is missing (should be "... whenever mtopi is not zero").
In the example pseudocode for a machine-level trap handler, the variable i is missing from this step: "call the interrupt handler for major interrupt i".
In Section 5.3, "Interrupt filtering and virtual interrupts for supervisor level":
In the last sentence ("When supervisor mode is not implemented, ..."), register name mvip is miswritten as "svip".
In Section 5.4.1, "Configuring priorities of major interrupts at supervisor level":
In the sentence that begins "When XLEN = 64 and siselect is an odd value ...", the number 0x3F is miswritten as "ox3F".
In Section 5.4.2, "Supervisor top interrupt CSR (stopi)":
In the sentence that begins "The following pseudocode shows ...", the register name stopi is missing (should be "... handler might read stopi to avoid ...").
In the example pseudocode for a supervisor-level trap handler, the variable i is missing from this step: "call the interrupt handler for major interrupt i".
In Section 5.5, "WFI (Wait for Interrupt) instruction":
In the sentence that begins "And if the hypervisor extension is implemented, ...", register name vstopi is missing (should be "... at VS level if vstopi (Section 6.3.3) is not zero").
Thanks again, @jhauser-us all of these issues have been fixed. Note that a few of them exist in the released spec, so the asciidoc version is moving forward with corrections that the original spec does not have.
Note that a few of them exist in the released spec,
Really? Which ones? I've been comparing directly with the released spec.
I believe one of the - i in the pseudocode and one of the spelling mistakes that I can't remember off the top of my head. I can check again tomorrow. It's possible my pdf of the released spec is old though.
Another batch of errors I've found, probably the last ones:
In Section 6.1, "VS-level external interrupts with a guest interrupt file":
In the first sentence of the second paragraph, register name vsiselect is miswritten as "viselect".
In Section 6.3.1, "Configuring priorities of major interrupts at VS level":
In the first sentence of the second paragraph, register name vsiselect is miswritten as "vsiseelect".
In Section 6.3.2, "Virtual interrupts for VS level":
In the sentence that is supposed to read "Bits 12:0 of hvien are reserved and must be read-only zeros, ...", register hvien has been incorrectly replaced by hvip. (The first part of this sentence is about hvien; the second part is about hvip.)
In the sentence that begins "For interrupt numbers 13-63, a bit in vsie is writable if and only if ...", register name hvien is miswritten as "hiven".
In the bullet point that says "asserting for VS level a major interrupt not supported by hvien and hvip;", register name hvien is miswritten as "hvvien".
In the title for Section 6.3.3, "Virtual supervisor top interrupt CSR (vstopi)", the register name vstopi is missing.
In Chapter 7, "Interprocessor Interrupts (IPIs)":
In a note, in the sentence that begins "On such a trap, software can send a higher-level IPI via IMSIC ...", register name sip is missing (should be, "set the SSIP bit in sip").
In Section 8.3.2, "Recording of incoming MSIs to memory-resident interrupt files":
In the sentence that begins "IMSIC interrupt files may also accept MSIs in big-endian byte order ...", register name seteipnum_be is miswritten as "setipnum_be".
Thanks @jhauser-us , I've made all of the fixes.
Hi @jhauser-us , so the next step would be to rename the main branch to "latex" and rename the asciidoc branch as "main" to set the asciidoc as the default branch. You'll have to audit the 27 open issues against the LaTeX branch and perhaps encourage the authors to move the issue to the asciidoc.
The caption for Table 10 should not be in the middle of the table. See similar Table 12 for comparison.
Conversion of LaTeX specification to asciidoc.