On Thu, Aug 18, 2022 at 12:25 PM Greg Favor <gfavor@...> wrote:

On Wed, Aug 17, 2022 at 5:19 AM Guo Ren <guoren@...> wrote:

Here is the third version of the proposal.

It seems like you have dropped a bunch of text from the prior versions - which is probably part of what leads to some of my questions below.

======================= Supervisor Extension Additions ========================
\subsection{``Svpbmt32'' Extension for Page-Based Memory Types}
\label{sec:translation}

Svpbmt32 support is being added to allow the two highest bits of a PTE
to be used as PBMT instead of PA[33:32] for Sv32.

Maybe better to say "The Svpbmt32 extension allows the two ..."

Okay

The S-mode and G-stage address translation under this extension are

"are" -> "is"

Okay

controlled by the
menvcfg.PBMTE. The VS-stage address translation under this extension
is controlled by henvcfg.PBMTE and indirectly by menvcfg.PBMTE.

This needs to specify what "controlled" means. There also is no explanation (here or below) of how menvcfg.PBMTE indirectly controls VS-stage address translation.

If this is all duplication of some of the currently defined *envcfg PBMTE functionality, then it is better to make clear that this is just reiterating a portion of the exact same functionality as currently exists for the *envcfg.PBMTE bits (and to refer to those current definitions for the details). Otherwise, by default, there is no clear understanding as to whether or not this extension specifies the exact same *envcfg.PBMTE behaviors as for the Svpbmt extension.

Okay, here is the rewritten sentence:

The Svpbmt32 extension allows the two highest bits of a PTE
(BIT[31:30]) to be used as PBMT (Page-Based Memory Type) instead of
PA[33:32] for Sv32. All modes and G-stage address translation under
this extension are controlled by the menvcfg.PBMTE. If menvcfg.PBMTE =
0, BIT[31:30] of PTE becomes PA[33:32]. If menvcfg.PBMTE = 1,
BIT[31:30] of PTE becomes PBMT for V=0. The henvcfg.PBMT is for V=1
and is controlled by menvcfg.PBMTE. If menvcfg.PBMTE = 0,
henvcfg.PBMTE couldn't be set to 1. If henvcfg.PBMTE = 1, BIT[31:30]
of PTE becomes PBMT for V=1 and G-stage address translation.

\begin{commentary}
For example, consider an RV32 system supporting Svpbmt32 and
Hypervisor Extension (Chapter~\ref{hypervisor}). When menvcfg.PBMTE=1,
Svpbmt32 is available for S-mode and G-stage address translation. When
henvcfg.PBMTE=1, Svpbmt32 is available for VS-mode address translation.

This is being presented as non-normative text, but this sounds like it is or needs to be normative text? Or is this "non-normatively" just repeating existing defined functionality - in which case what extra value is this text providing past repeating existing arch definitions?

Okay, I've moved the text into the above section. But I rewrote the
commentary with PBMT type mapping explanation. So I won't explain more
about PBMT usage for V=0/V=1 in the section.

\begin{commentary}
The mechanism of the PBMT of Sv32 is the same as Sv39, Sv48, and Sv57.
The only difference is the bits' location. Please see
(Chapter~\ref{Svpbmt}) for detail.
\end{commentary}

\end{commentary}

Sv32 virtual address:

| 31 22 | 21 12 | 11 0 |
VPN[1] VPN[0] page offset
10 10 12


Sv32 physical address with Svpbmt:

| 31 22 | 21 12 | 11 0 |
PPN[1] PPN[0] page offset
10 10 12

Sv32 page table entry with Svpbmt:

| 31 30 | 29 20 | 19 10 | 9 8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
PBMT PPN[1] PPN[0] reserved for SW D A G U X W R V

========================================================================

There is no statement anywhere in this spec that says what exactly is the PBMT field. I know you intend for it to have the exact same definition as the currently defined PBMT field, but this should be stated explicitly (to remove any possible ambiguity around whether PBMT encodings and/or meanings may be a little different in RV32 versus RV64).

--
Best Regards
Guo Ren