Re: [PATCH v3 27/34] xen/riscv: define an address of frame table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2024 17:50, Oleksii wrote:
> On Tue, 2024-01-23 at 12:32 +0100, Jan Beulich wrote:
>> On 22.12.2023 16:13, Oleksii Kurochko wrote:
>>> @@ -22,25 +30,56 @@
>>>   *
>>>   * It means that:
>>>   *   top VA bits are simply ignored for the purpose of translating
>>> to PA.
>>> +#endif
>>>   *
>>> - *
>>> ===================================================================
>>> =========
>>> - *    Start addr    |   End addr        |  Size  | Slot      
>>> |area description
>>> - *
>>> ===================================================================
>>> =========
>>> - * FFFFFFFFC0800000 |  FFFFFFFFFFFFFFFF |1016 MB | L2 511     |
>>> Unused
>>> - * FFFFFFFFC0600000 |  FFFFFFFFC0800000 |  2 MB  | L2 511     |
>>> Fixmap
>>> - * FFFFFFFFC0200000 |  FFFFFFFFC0600000 |  4 MB  | L2 511     |
>>> FDT
>>> - * FFFFFFFFC0000000 |  FFFFFFFFC0200000 |  2 MB  | L2 511     |
>>> Xen
>>> - *                 ...                  |  1 GB  | L2 510     |
>>> Unused
>>> - * 0000003200000000 |  0000007F80000000 | 309 GB | L2 200-509 |
>>> Direct map
>>> - *                 ...                  |  1 GB  | L2 199     |
>>> Unused
>>> - * 0000003100000000 |  00000031C0000000 |  3 GB  | L2 196-198 |
>>> Frametable
>>> - *                 ...                  |  1 GB  | L2 195     |
>>> Unused
>>> - * 0000003080000000 |  00000030C0000000 |  1 GB  | L2 194     |
>>> VMAP
>>> - *                 ...                  | 194 GB | L2 0 - 193 |
>>> Unused
>>> - *
>>> ===================================================================
>>> =========
>>> + *       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
>>>   *
>>> -#endif
>>> + *
>>> ===================================================================
>>> ============
>>> + * Start addr     |   End addr          |  Size  | Root PT slot |
>>> Area description
>>> + *
>>> ===================================================================
>>> ============
>>> + * BA0 + 0x800000 |  FFFFFFFFFFFFFFFF   |1016 MB |     511      |
>>> Unused
>>> + * BA0 + 0x400000 |  BA0 + 0x800000     |  2 MB  |     511      |
>>> Fixmap
>>> + * BA0 + 0x200000 |  BA0 + 0x400000     |  4 MB  |     511      |
>>> FDT
>>> + * BA0            |  BA0 + 0x200000     |  2 MB  |     511      |
>>> Xen
>>> + *                 ...                  |  1 GB  |     510      |
>>> Unused
>>> + * BA1 + 0x000000 |  BA1 + 0x4D80000000 | 309 GB |   200-509    |
>>> Direct map
>>
>> This definitely can't be right for SV32. Others may be problematic,
>> too, like ...
>>
>>> + *                 ...                  |  1 GB  |     199      |
>>> Unused
>>> + * BA2 + 0x000000 |  BA2 + 0xC0000000   |  3 GB  |   196-198    |
>>> Frametable
>>
>> ... this one. Otoh I'd expect both to potentially be much larger in
>> SV48 and SV57 modes.
> Regarding Sv32, it looks to me the only BA0 and End addr at the first
> line isn't correct as address size is 32.
> 
> Regarding other modes, yes, it should be changed Size column. Also, the
> size of frame table should be recalculated.
> 
> Do we really need size column?
> 
> Wouldn't it be enough only have PT slot number?

Perhaps.

> Would it be better to have separate table for each mode?

Don't know.

>>> +#define VPN_BITS    (9)
>>
>> This need to move ...
>>
>>> +#define HYP_PT_ROOT_LEVEL (CONFIG_PAGING_LEVELS - 1)
>>> +
>>> +#ifdef CONFIG_RISCV_64
>>
>> ... here, I think, for not being applicable to SV32?
> You are right, it is not applicable for Sv32. In case of Sv32, it
> should be 10.
> But I am not sure that it is correct only to move this definition as
> RISCV-64 can also use Sv32. So it looks like VPN_BITS should be "#ifdef
> RV_STAGE1_MODE == Sv32".

Can it? The spec talks of SXLEN=32 implying SV32, while SXLEN=64 permits
SV39, SV48, and SV57. No mention of SV32 there.

>>> +#define SLOTN_ENTRY_BITS        (HYP_PT_ROOT_LEVEL * VPN_BITS +
>>> PAGE_SHIFT)
>>> +#define SLOTN(slot)             (_AT(vaddr_t, slot) <<
>>> SLOTN_ENTRY_BITS)
>>> +#define SLOTN_ENTRY_SIZE        SLOTN(1)
>>
>> Do you have any example of how/where this going to be used?
> Yes, it will be used to define DIRECTMAP_SIZE:
> #define DIRECTMAP_SIZE          (SLOTN_ENTRY_SIZE * (509-200))

How about

#define DIRECTMAP_SIZE          (SLOTN(509) - SLOTN(200))

instead?

>>> +#define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1 -
>>> GB(1)) */
>>
>> Won't /* -GB(1) */ do, thus allowing the line to also be padded such
>> that
>> it matches neighboring ones in layout?
> Could you please clarify what do you mean by padded here? The intention
> was to show that 1 GB is used for Xen, FDT and fixmap.

I'm talking of blank padding in the source file. Note how preceding and
following #define-s blank-pad expansions so they all align. Just this
one in the middle does not.

Jan

>>> +#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)
>>> [...]
>>
>