Re: [PATCH v3 1/3] xen/riscv: introduce setup_mm()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2024 13:32, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-11-07 at 10:19 +0100, Jan Beulich wrote:
>> On 06.11.2024 13:44, oleksii.kurochko@xxxxxxxxx wrote:
>>> On Tue, 2024-11-05 at 16:20 +0100, Jan Beulich wrote:
>>>> On 01.11.2024 14:16, Oleksii Kurochko wrote:> @@ -423,3 +429,138
>>>> Plus, unless I'm mistaken, the function continues to rely on ps
>>>> == 0
>>>> as
>>>> input. Just that the dependency is now better hidden.
>>>> Specifically if
>>>> you
>>>> calculate nr_mfns from the difference, and then use that for
>>>> allocation,
>>>> then you need to offset the start of the mapping you create
>>>> accordingly.
>>> I'm not quite understanding why the method of calculating nr_mfns
>>> affects how the frame_table is mapped. Isn’t it only necessary to
>>> calculate the correct size of the frame_table that needs to be
>>> allocated?
>>
>> Assume there's 4G of memory actually starting at 16G. Afaict you'll
>> allocate a frame table for those 4G, but you'll map it right at
>> FRAMETABLE_VIRT_START. Hence it'll cover the first 4G of physical
>> addresses, but _none_ of the actual memory you've got.
> I need to clarify some basics about the frame table.
> 
> Does Xen expect that frame_table[0] = 0 (PA), frame_table[1] = 4k (PA),
> ..., frame_table[x] = RAM_START_PA, frame_table[x+1] = RAM_START_PA +
> 4k, and so on?

Not strictly, no. You'll find that common code has very few uses of
frame_table, so many aspects are fully up to the arch. However, there
is the assumption you mention above in PDX space. When PDX == PFN
clearly that assumption would then also hold for PFNs.

> My understanding is that it could be done as follows: frame_table[0] =
> RAM_START_PA, frame_table[1] = RAM_START_PA + 4k, and so on, taking
> into account mfn_to_page() and page_to_mfn() logic. (And yes, in that
> case, the current implementations of mfn_to_page() and page_to_mfn()
> aren't correct and should be updated as suggested here:
> https://lore.kernel.org/xen-devel/cover.1730465154.git.oleksii.kurochko@xxxxxxxxx/T/#me2fc410f3d4758b71a9974d0be18a36f50d683b1as
> as these implementations are based on that ps == 0). With this setup,
> mapping FRAMETABLE_VIRT_START to base_mfn should be correct, shouldn’t
> it?

Yes.

> With the current implementations of mfn_to_page() and page_to_mfn(), we
> either need to allocate a larger frame_table to cover the [0, ram_end)
> range (in which case mapping FRAMETABLE_VIRT_START to base_mfn would
> work), or change the mapping to frame_table=( FRAMETABLE_VIRT_START +
> PFN_DOWN(ram_start) ) -> base_mfn. Or to not loose virtual addrees
> space of FRAMETABLE ( so the mapping will still be
> FRAMETABLE_VIRT_START -> base_mfn ) to do the similar to directmap
> mapping ( or what the changes suggested in the link above). Is my
> understanding correct?

Largely yes. There's one more aspect to consider: Even when frame_table[0]
maps MFN 0, the initial part of frame_table[] doesn't necessarily need
mapping to any memory when RAM starts at a much higher address. Valid
struct page_info instances only need to exist for any MFN for which
mfn_valid() returns true.

>>>>  At
>>>> which point you may need to apply extra care to cover the case
>>>> where
>>>> sizeof(*frame_table) is not a power of two, and hence e.g. the
>>>> first
>>>> valid
>>>> page might have a struct instance straddling a page boundary.
>>> The first valid page is aligned_ps ( which is aligned on a page
>>> boundary ) so assuming that sizeof(*frame_table) < PAGE_SIZE we
>>> can't
>>> straddle a page boundary, can we?
>>
>> But sizeof(*frame_table) < PAGE_SIZE means nothing as to the
>> alignment
>> of an individual struct instance in memory. Iirc sizeof(*frame_table)
>> is 48 for RISC-V, so the common alignment across struct instances
>> isn't
>> going to be better than 8, and there _will_ be instances straddling
>> page boundaries.
> If we speak about the alignment of an individual struct instance in
> memory, what is the issue with that, except that it could be less
> efficient when accessing this memory? This inefficiency could
> potentially be addressed by adding some padding to the struct page_info
> but then we will have bigger frame table size.
> Another issue I can see is that the size of the frame table could be
> calculated incorrectly. It may require an additional page to cover the
> case when the frame table size isn't aligned to PAGE_SIZE, but this is
> accounted for by rounding up the frame table size to 2MB
> (frametable_size = ROUNDUP(frametable_size, MB(2));) before allocating
> the frame table (base_mfn = alloc_boot_pages(frametable_size >>
> PAGE_SHIFT, PFN_DOWN(MB(2)));).
> 
> Something else should be considered?

Much depends on what your decision is as to the earlier aspect. If
you map [0,end), then what you have ought to be fine (except for
being wasteful).

Jan