Re: [PATCH v2 30/39] xen/riscv: define an address of frame table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.12.2023 20:59, Oleksii wrote:
> On Mon, 2023-12-18 at 12:22 +0100, Jan Beulich wrote:
>> On 18.12.2023 11:36, Oleksii wrote:
>>> On Thu, 2023-12-14 at 16:48 +0100, Jan Beulich wrote:
>>>> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>>>>> +#define SLOTN_ENTRY_SIZE        SLOTN(1)
>>>>> +
>>>>>  #define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1
>>>>> -
>>>>> GB(1)) */
>>>>> +
>>>>> +#define FRAMETABLE_VIRT_START   SLOTN(196)
>>>>> +#define FRAMETABLE_SIZE         GB(3)
>>>>> +#define FRAMETABLE_NR           (FRAMETABLE_SIZE /
>>>>> sizeof(*frame_table))
>>>>> +#define FRAMETABLE_VIRT_END     (FRAMETABLE_VIRT_START +
>>>>> FRAMETABLE_SIZE - 1)
>>>>> +
>>>>> +#define VMAP_VIRT_START         SLOTN(194)
>>>>> +#define VMAP_VIRT_SIZE          GB(1)
>>>>
>>>> May I suggest that you keep these blocks sorted by slot number?
>>>> Or
>>>> wait,
>>>> the layout comment further up is also in decreasing order, so
>>>> that's
>>>> fine here, but then can all of this please be moved next to the
>>>> comment
>>>> actually providing the necessary context (thus eliminating the
>>>> need
>>>> for
>>>> new comments)?
>>> Sure, I'll put this part close to layout comment.
>>>
>>>>  You'll then also notice that the generalization here
>>>> (keeping basically the same layout for e.g. SATP_MODE_SV48, just
>>>> shifted
>>>> by 9 bits) isn't in line with the comment there.
>>> Does it make sense to add another one table with updated addresses
>>> for
>>> SATP_MODE_SV48?
>>
>> Well, especially if you mean to support that mode, its layout surely
>> wants writing down. I was hoping though that maybe you/we could get
>> away
>> without multiple tables, but e.g. use one having multiple columns.
> I came up with the following but I am not sure that it is really
> convient:
> /*
>  * RISC-V64 Layout:
>  *
> #if RV_STAGE1_MODE == SATP_MODE_SV39
>  *
>  * From the riscv-privileged doc:
>  *   When mapping between narrower and wider addresses,
>  *   RISC-V zero-extends a narrower physical address to a wider size.
>  *   The mapping between 64-bit virtual addresses and the 39-bit usable
>  *   address space of Sv39 is not based on zero-extension but instead
>  *   follows an entrenched convention that allows an OS to use one or
>  *   a few of the most-significant bits of a full-size (64-bit) virtual
>  *   address to quickly distinguish user and supervisor address
> regions.
>  *
>  * It means that:
>  *   top VA bits are simply ignored for the purpose of translating to
> PA.
> #endif
>  *
>  *       SATP_MODE_SV32   SATP_MODE_SV39   SATP_MODE_SV48  
> SATP_MODE_SV57
>  *     ----------------------------------------------------------------
> -----------
>  * BA0 | FFFFFFFFFFE00000 | FFFFFFFFC0000000 | FFFFFF8000000000 |
> FFFF000000000000
>  * BA1 | 0000000019000000 | 0000003200000000 | 0000640000000000 |
> 00C8000000000000
>  * BA2 | 0000000018800000 | 0000003100000000 | 0000620000000000 |
> 00C4000000000000
>  * BA3 | 0000000018400000 | 0000003080000000 | 0000610000000000 |
> 00C2000000000000
>  * 
>  *
> =======================================================================
> =====
>  * Start addr    |   End addr           |  Size  | Slot       |area
> description
>  *
> =======================================================================
> =====
>  * BA0 + 0x800000 |  FFFFFFFFFFFFFFFF   |1016 MB |
> L${HYP_PT_ROOT_LEVEL} 511     | Unused
>  * BA0 + 0x400000 |  BA0 + 0x800000     |  2 MB  |
> L${HYP_PT_ROOT_LEVEL} 511     | Fixmap
>  * BA0 + 0x200000 |  BA0 + 0x400000     |  4 MB  |
> L${HYP_PT_ROOT_LEVEL} 511     | FDT
>  * BA0            |  BA0 + 0x200000     |  2 MB  |
> L${HYP_PT_ROOT_LEVEL} 511     | Xen
>  *                 ...                  |  1 GB  |
> L${HYP_PT_ROOT_LEVEL} 510     | Unused
>  * BA1 + 0x000000 |  BA1 + 0x4D80000000 | 309 GB |
> L${HYP_PT_ROOT_LEVEL} 200-509 | Direct map
>  *                 ...                  |  1 GB  |
> L${HYP_PT_ROOT_LEVEL} 199     | Unused
>  * BA2 + 0x000000 |  BA2 + 0xC0000000   |  3 GB  |
> L${HYP_PT_ROOT_LEVEL} 196-198 | Frametable
>  *                 ...                  |  1 GB  |
> L${HYP_PT_ROOT_LEVEL} 195     | Unused
>  * BA3 + 0x000000 |  BA3 + 0x40000000   |  1 GB  |
> L${HYP_PT_ROOT_LEVEL} 194     | VMAP
>  *                 ...                  | 194 GB |
> L${HYP_PT_ROOT_LEVEL} 0 - 193 | Unused
>  *
> =======================================================================
> =====
>  */
> 
> Do you have better ideas?

It doesn't look too bad imo, at the first glance, albeit the line
wrapping damage of course makes it a little hard to look at. In the
last table with all lines saying L${HYP_PT_ROOT_LEVEL}, perhaps that
could be put in the table heading (instead of "Slot" say e.g. "Root
PT slot")?

Jan