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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.11.2024 13:51, oleksii.kurochko@xxxxxxxxx wrote:
> On Mon, 2024-11-11 at 11:29 +0100, Jan Beulich wrote:
>> On 08.11.2024 13:51, Oleksii Kurochko wrote:
>>> @@ -37,9 +42,9 @@ static inline void *maddr_to_virt(paddr_t ma)
>>>   */
>>>  static inline unsigned long virt_to_maddr(unsigned long va)
>>>  {
>>> -    if ((va >= DIRECTMAP_VIRT_START) &&
>>> +    if ((va >= directmap_virt_start) &&
>>
>> Is this a valid / necessary change to make?
> You are right, this not valid change, va value is DIRECTMAP_VIRT_START-
> relative.
> 
>>  Right now there looks to be
>> nothing immediately below the directmap, yet that would need
>> guaranteeing
>> (e.g. by some BUILD_BIG_ON() or whatever else) if code builds upon
>> that.
> It is not really clear how to check that nothing below the directmap is
> present/used. But IIUC there is no need for this check if properly
> correct the condition above.

Right.

>>>          (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
>>> -        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
>>> +        return directmapoff_to_maddr(va - directmap_virt_start);
>>
>> FTAOD - no question about this part of the change.
>>
>>> @@ -423,3 +429,140 @@ void * __init early_fdt_map(paddr_t
>>> fdt_paddr)
>>>  
>>>      return fdt_virt;
>>>  }
>>> +
>>> +vaddr_t __ro_after_init directmap_virt_start =
>>> DIRECTMAP_VIRT_START;
>>> +
>>> +struct page_info *__ro_after_init frametable_virt_start;
>>
>> As for directmap_virt_start - perhaps better with initializer?
> Do you mean to initialized by NULL or frame_table?

The latter.

> If to initialize by frame_table then the if-condition won't work
> properly in setup_frametable_mappings() ( but I think that this
> condition could be dropped as setup_frametable_mappings() is supposed
> to be called only once ?! ). And you mentioned about this condition
> here ...
> 
>>
>>> +#ifndef CONFIG_RISCV_32
>>> +
>>> +/* Map a frame table to cover physical addresses ps through pe */
>>> +static void __init setup_frametable_mappings(paddr_t ps, paddr_t
>>> pe)
>>> +{
>>> +    paddr_t aligned_ps = ROUNDUP(ps, PAGE_SIZE);
>>> +    paddr_t aligned_pe = ROUNDDOWN(pe, PAGE_SIZE);
>>> +    unsigned long nr_mfns = PFN_DOWN(aligned_pe - aligned_ps);
>>> +    unsigned long frametable_size = nr_mfns *
>>> sizeof(*frame_table);
>>> +    mfn_t base_mfn;
>>> +
>>> +    if ( !frametable_virt_start )
>>> +        frametable_virt_start = frame_table -
>>> paddr_to_pfn(aligned_ps);
>>
>> If you make this conditional, then you need an "else" (or something
>> that's
>> effectively one) just like you have in setup_directmap_mappings().
>> Like
>> for the earlier assumption on ps being zero: Assumptions you make on
>> how
>> a function is used want to at least be self-consistent. I.e. here
>> either
>> you assume the function may be called more than once, or you don't.
> ...
> 
> Do we have in Xen something to be sure that the function is called only
> once or I have to come up with static variable inside the function?

There's no checking needed. All I'm asking for is that the function
either be indeed callable multiple times, or that it not wrongly give
the impression that it can be called more than once when it really can't
be.

>>> +    if ( base_mfn < mfn_x(directmap_mfn_start) )
>>> +        panic("can't add directmap mapping at %#lx below directmap
>>> start %#lx\n",
>>> +              base_mfn, mfn_x(directmap_mfn_start));
>>> +
>>> +    if ( map_pages_to_xen((vaddr_t)mfn_to_virt(base_mfn),
>>> +                          _mfn(base_mfn), nr_mfns,
>>> +                          PAGE_HYPERVISOR_RW) )
>>> +        panic("Directmap mappings for [%#"PRIpaddr", %#"PRIpaddr")
>>> failed\n",
>>> +              mfn_to_maddr(_mfn(base_mfn)),
>>> +              mfn_to_maddr(_mfn(base_mfn + nr_mfns)));
>>
>> Maybe worth also logging the error code?
> I am not really understand why do we need that as the use will see what
> is the issue in the message inside panic().

If the panic() triggers, the user will see that something went wrong with
the given range. The error code may give a hint at _what_ went wrong.

Jan