Re: [PATCH v5 6/7] xen/riscv: page table handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2024 16:42, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-08-29 at 14:14 +0200, Jan Beulich wrote:
>>>>>>  Also note that "`mfn` is
>>>>>> valid" isn't the same as "mfn != INVALID_MFN". You want to be
>>>>>> precise
>>>>>> here,
>>>>>> to avoid confusion later on. (I say that knowing that we're
>>>>>> still
>>>>>> fighting
>>>>>> especially shadow paging code on x86 not having those
>>>>>> properly
>>>>>> separated.)
>>>>> If it is needed to be precise and mfn is valid isn't the same
>>>>> as
>>>>> "mfn
>>>>> != INVALID_MFN" only for the case of shadow paging?
>>>>
>>>> No, I used shadow paging only as an example of where we have
>>>> similar
>>>> issues. I'd like to avoid that a new port starts out with
>>>> introducing
>>>> more instances of that. You want to properly separate INVALID_MFN
>>>> from
>>>> "invalid MFN", where the latter means any MFN where either
>>>> nothing
>>>> exists at all, or (see mfn_valid()) where no struct page_info
>>>> exists.
>>> Well, now I think I understand the difference between "INVALID_MFN"
>>> and
>>> "invalid MFN."
>>>
>>> Referring back to your original reply, I need to update the comment
>>> above pt_update():
>>> ```
>>>     ...
>>>       * If `mfn` is valid ( exist ) and flags has PTE_VALID bit set
>>> then it
>>>     means that inserting will be done.
>>> ```
>>> Would this be correct and more precise?
>>
>> That depends on whether it correctly describes what the code does. If
>> the code continues to check against INVALID_MFN, such a description
>> wouldn't be correct. Also, just to re-iterate, ...
>>
>>> Based on the code for mfn_valid(), the separation is currently done
>>> using the max_page value, which can't be initialized at the moment
>>> as
>>> it requires reading the device tree file to obtain the RAM end.
>>
>> ... mfn_valid() may return false for MMIO pages, for which it may
>> still
>> be legitimate to create mappings. IMO ...
>>
>>> We could use a placeholder for the RAM end (for example, a very
>>> high
>>> value like -1UL) and then add __mfn_valid() within pt_update().
>>> However, I'm not sure if this approach aligns with what you
>>> consider by
>>> proper separation between INVALID_MFN and "invalid MFN."
>>
>> ... throughout the code here you mean INVALID_MFN and never "invalid
>> MFN".
> IIC INVALID_MFN should mean that mfn exist ( correspond to some usable
> memory range of memory map ) but hasn't been mapped yet. Then for me
> what I have in the comment seems correct to me:
> ```
>    if `mfn` isn't equal to INVALID_MFN ( so it is valid/exist in terms
>    that there is real memory range in memory map to which this mfn
>    correspond ) and flags PTE_VALID bit set ...
> ```

Not really, no, as said ...

>> Populating page tables is lower a layer than where you want to be
>> concerned with that distinction; the callers of these low level
>> functions
>> will need to make the distinction where necessary.

... here. At this level I think you want to consider only INVALID_MFN,
and for anything else you're simply not concerned what the MFN provided
points at[1]. Specifically, said said before, it may point at an MMIO
range which may not be in the memory map (a PCI device BAR for example).

Jan

[1] One thing that could be checked at this layer is the number of
significant MFN bits, in case there were hardware setups in which you
know that not the full width is permitted that the PTE has room for. No
idea whether such exists in the RISC-V world.