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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2024 17:49, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-11-14 at 17:30 +0100, oleksii.kurochko@xxxxxxxxx wrote:
>> On Thu, 2024-11-14 at 10:49 +0100, Jan Beulich wrote:
>>>>>> @@ -423,3 +429,147 @@ 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 =
>>>>>> frame_table;
>>>>>> +
>>>>>> +#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)
>>>>>> +{
>>>>>> +    static mfn_t __initdata frametable_mfn_start =
>>>>>> INVALID_MFN_INITIALIZER;
>>>>>> +
>>>>>> +    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 ( mfn_eq(frametable_mfn_start, INVALID_MFN) )
>>>>>> +    {
>>>>>> +        frametable_mfn_start = maddr_to_mfn(aligned_ps);
>>>>>> +
>>>>>> +        frametable_virt_start -= paddr_to_pfn(aligned_ps);
>>>>>> +    }
>>>>>> +    else
>>>>>> +        panic("%s shouldn't be called twice\n", __func__);
>>>>>
>>>>> As said on the v4 thread - I don't think this is needed. Aiui
>>>>> Misra
>>>>> would
>>>>> actually dislike it, as it's unreachable code. Just to re-
>>>>> iterate: My
>>>>> complaint there wasn't about this missing check, but about the
>>>>> function
>>>>> partly giving the impression of expecting to be called more
>>>>> than
>>>>> once.
>>>> I’ll revert this check, then. Would it be better—and
>>>> sufficient—to
>>>> add
>>>> a comment before setup_frametable_mappings() indicating that this
>>>> function should only be called once, to avoid any impression that
>>>> it
>>>> might be expected to be called multiple times?
>>>
>>> You can add such a comment if you like, we we have many functions
>>> of
>>> this
>>> kind. The important aspect - as said before - is that the function
>>> should
>>> not - nowhere - give the impression of possibly expecting to be
>>> called
>>> more than once.
>> Then I am not 100% sure how to deal with this impression specifically
>> in the case of setup_frametable_mapping() which should be called
>> once.
>>
>> The only options I have in my head are:
>>    Option 1:
>>        static void __init setup_frametable_mappings(paddr_t ps,
>> paddr_t
>>       pe)
>>        {
>>       +    static bool __read_mostly once;
>>       +
>>            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;
>>        
>>       +    ASSERT(!once);
>>       +
>>       +    once = true;
>>       +
>>            frametable_virt_start -= paddr_to_pfn(aligned_ps);
>>        
>>            if ( frametable_size > FRAMETABLE_SIZE )
>>
>>
>>    Option 2:
>>    
>>       -struct page_info *__ro_after_init frametable_virt_start =
>>       frame_table;
>>       +struct page_info *__ro_after_init frametable_virt_start;
>>        
>>        #ifndef CONFIG_RISCV_32
>>        
>>       @@ -442,7 +442,9 @@ static void __init
>>       setup_frametable_mappings(paddr_t ps, paddr_t pe)
>>            unsigned long frametable_size = nr_mfns *
>>    sizeof(*frame_table);
>>            mfn_t base_mfn;
>>        
>>       -    frametable_virt_start -= paddr_to_pfn(aligned_ps);
>>       +    ASSERT(!frametable_virt_start);
>>       +
>>       +    frametable_virt_start = frame_table -
>>    paddr_to_pfn(aligned_ps);
>>        
>>            if ( frametable_size > FRAMETABLE_SIZE )
>>                panic("The frametable cannot cover [%#"PRIpaddr",
>>       %#"PRIpaddr")\n",
>>    
> Or your intention was that I can drop any check at all in
> setup_frametable_mappings():
>    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;
>    
>        frametable_virt_start -= paddr_to_pfn(aligned_ps);
>    
>    ...
>    }
>    
> and it should be enough as this function by its nature shouldn't be called
> twice.

Indeed, that's what I was after.

Jan